Add MachineSet ControlPlane webhooks #617

michaelgugino · 2020-06-18T05:25:34Z

Add webhooks for Delete and Update operations
on MachineSets that are identified to be
Control Plane members. These webhooks prevent
deleting these MachineSets or updating them
to become non-CP MachineSets (in order to
further prevent them from being deleted or
misconfigured)

michaelgugino · 2020-06-18T05:41:51Z

install/0000_30_machine-api-operator_08_webhook.yaml

+        apiVersions:
+          - v1beta1
+        operations:
+          - UPDATE


The unit tests don't seem to like this being here.

mtnbikenc · 2020-06-18T14:47:46Z

FYI, this PR caused a failure when running oc delete machinesets:

failed: [localhost] (item=ci-op-0c4z64fq-bbd80-28ds4-worker-us-east-1a) => changed=true 
  ansible_loop_var: item
  cmd:
  - oc
  - delete
  - machinesets
  - ci-op-0c4z64fq-bbd80-28ds4-worker-us-east-1a
  - --kubeconfig=/tmp/artifacts/installer/auth/kubeconfig
  - --namespace=openshift-machine-api
  delta: '0:00:00.652124'
  end: '2020-06-18 06:23:01.170022'
  invocation:
    module_args:
      _raw_params: |-
        oc delete machinesets ci-op-0c4z64fq-bbd80-28ds4-worker-us-east-1a --kubeconfig=/tmp/artifacts/installer/auth/kubeconfig --namespace=openshift-machine-api
      _uses_shell: false
      argv: null
      chdir: null
      creates: null
      executable: null
      removes: null
      stdin: null
      stdin_add_newline: true
      strip_empty_ends: true
      warn: true
  item: ci-op-0c4z64fq-bbd80-28ds4-worker-us-east-1a
  msg: non-zero return code
  rc: 1
  start: '2020-06-18 06:23:00.517898'
  stderr: 'Error from server (InternalError): Internal error occurred: failed calling webhook "delete.cp.validation.machineset.machine.openshift.io": the server could not find the requested resource'
  stderr_lines: <omitted>
  stdout: ''
  stdout_lines: <omitted>

michaelgugino · 2020-06-18T22:12:34Z

go.mod

@@ -39,3 +39,5 @@ replace sigs.k8s.io/cluster-api-provider-aws => github.com/openshift/cluster-api
 replace sigs.k8s.io/cluster-api-provider-azure => github.com/openshift/cluster-api-provider-azure v0.1.0-alpha.3.0.20200529030741-17d4edc5142f

 replace sigs.k8s.io/cluster-api-provider-gcp => github.com/openshift/cluster-api-provider-gcp v0.0.1-0.20200528175251-4f2fdeb49fe1
+
+replace sigs.k8s.io/controller-runtime => github.com/mgugino-upstream-stage/controller-runtime v0.6.1-0.20200618201807-9d82bf2a7266


kubernetes-sigs/controller-runtime#1005

This is an outstanding issue from controller-runtime. If we want to test webhooks dynamically like this, we need this patch or some other alternative.

JoelSpeed · 2020-06-19T12:49:13Z

pkg/apis/machine/v1beta1/machineset_cp_webhook.go

+
+	// TODO(michaelgugino): Ensure we account for MachineDeployment ownership
+	// of a CP machineset in the future if we use them.
+	return admission.Denied(fmt.Sprintf("Requested %v of Control Plane MachineSet Not Allowed.", req.Operation))


Would be nice to have a better error on this, as in, more descriptive of the actual reason why we are rejecting it, I wonder if we could break the logic up so we have a the update and delete handled separately?

JoelSpeed · 2020-06-19T12:49:58Z

pkg/apis/machine/v1beta1/machineset_cp_webhook.go

+
+}
+
+func isCPMS(ms *MachineSet) bool {


Not a fan of acronyms in code, any reason not to call this isControlPlaneMachineSet so that you don't need context to understand it?

JoelSpeed · 2020-06-19T12:51:20Z

pkg/apis/machine/v1beta1/machineset_cp_webhook.go

+
+	// If the user is updating a CP MachineSet, as long as machine role is
+	// unchanged, we're ok.
+	if newMS != nil && isCPMS(newMS) && isCPMS(oldMS) {


newMS is never nil since you initialise it on line 46, so this will always check newMS for the annotations

JoelSpeed · 2020-06-19T12:56:48Z

pkg/apis/machine/v1beta1/machineset_webhook_test.go

+				close(done)
+				<-stopped
+			}()
+


Might be worth checking the server has started with

gs.Eventually(func() (bool, error) { resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) if err != nil { return false, err } return resp.StatusCode == 404, nil }).Should(BeTrue())

JoelSpeed · 2020-06-19T12:59:19Z

pkg/apis/machine/v1beta1/machineset_webhook_test.go

+				close(done)
+				<-stopped
+			}()
+


Might be worth checking the server has started with

gs.Eventually(func() (bool, error) { resp, err := insecureHTTPClient.Get(fmt.Sprintf("https://127.0.0.1:%d", testEnv.WebhookInstallOptions.LocalServingPort)) if err != nil { return false, err } return resp.StatusCode == 404, nil }).Should(BeTrue())

Add webhooks for Delete and Update operations on MachineSets that are identified to be Control Plane members. These webhooks prevent deleting these MachineSets or updating them to become non-CP MachineSets (in order to further prevent them from being deleted or misconfigured)

openshift-ci-robot · 2020-06-23T22:56:11Z

@michaelgugino: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`0942080`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-azure-operator	`0942080`	link	`/test e2e-azure-operator`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci-robot · 2020-06-26T15:08:35Z

@michaelgugino: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2020-10-23T19:08:12Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-merge-robot · 2020-11-06T16:30:25Z

@michaelgugino: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/generate	`0942080`	link	`/test generate`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2020-12-06T21:55:47Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2021-01-05T23:45:01Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci-robot · 2021-01-05T23:45:19Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

michaelgugino · 2021-01-15T16:40:02Z

/reopen

openshift-ci-robot · 2021-01-15T16:40:23Z

@michaelgugino: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2021-01-15T16:40:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign michaelgugino after the PR has been reviewed.
You can assign the PR to them by writing /assign @michaelgugino in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-02-14T17:30:57Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci-robot · 2021-02-14T17:31:07Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

michaelgugino · 2021-08-06T12:48:08Z

/reopen

openshift-ci · 2021-08-06T12:48:17Z

@michaelgugino: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2021-08-06T12:48:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from michaelgugino after the PR has been reviewed.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-08-17T15:41:43Z

@michaelgugino: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-workers-rhel7	`0942080`	link	`/test e2e-aws-workers-rhel7`
ci/prow/e2e-vsphere	`0942080`	link	`/test e2e-vsphere`
ci/prow/e2e-vsphere-upgrade	`0942080`	link	`/test e2e-vsphere-upgrade`
ci/prow/e2e-libvirt	`0942080`	link	`/test e2e-libvirt`
ci/prow/goimports	`0942080`	link	`/test goimports`
ci/prow/golint	`0942080`	link	`/test golint`
ci/prow/e2e-gcp	`0942080`	link	`/test e2e-gcp`
ci/prow/generate	`0942080`	link	`/test generate`
ci/prow/e2e-azure	`0942080`	link	`/test e2e-azure`
ci/prow/yaml-lint	`0942080`	link	`/test yaml-lint`
ci/prow/e2e-aws-disruptive	`0942080`	link	`/test e2e-aws-disruptive`
ci/prow/e2e-gcp-operator	`0942080`	link	`/test e2e-gcp-operator`
ci/prow/e2e-metal-ipi-ovn-ipv6	`0942080`	link	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-azure-operator	`0942080`	link	`/test e2e-azure-operator`
ci/prow/images	`0942080`	link	`/test images`
ci/prow/unit	`0942080`	link	`/test unit`
ci/prow/govet	`0942080`	link	`/test govet`
ci/prow/e2e-aws	`0942080`	link	`/test e2e-aws`
ci/prow/e2e-aws-operator	`0942080`	link	`/test e2e-aws-operator`
ci/prow/e2e-aws-upgrade	`0942080`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-metal-ipi	`0942080`	link	`/test e2e-metal-ipi`
ci/prow/e2e-metal-ipi-ovn-dualstack	`0942080`	link	`/test e2e-metal-ipi-ovn-dualstack`
ci/prow/e2e-metal-ipi-upgrade	`0942080`	link	`/test e2e-metal-ipi-upgrade`
ci/prow/e2e-metal-ipi-virtualmedia	`0942080`	link	`/test e2e-metal-ipi-virtualmedia`
ci/prow/e2e-vsphere-serial	`0942080`	link	`/test e2e-vsphere-serial`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-09-16T20:41:21Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-09-16T20:41:44Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2020

openshift-ci-robot requested review from alexander-demicev and elmiko June 18, 2020 05:25

michaelgugino commented Jun 18, 2020

View reviewed changes

JoelSpeed added 2 commits June 18, 2020 14:46

Switch Machine validation to not require validation/defaulting handlers

453a5b0

Add MachineSet defaulting and validating webhook

05bd80c

michaelgugino force-pushed the machineset-cp-vwh branch from fcbca8b to 9c9b3d8 Compare June 18, 2020 22:09

michaelgugino changed the title ~~WIP: Machineset cp vwh~~ Add MachineSet ControlPlane webhooks Jun 18, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2020

michaelgugino commented Jun 18, 2020

View reviewed changes

michaelgugino force-pushed the machineset-cp-vwh branch from 13bb386 to 52baca0 Compare June 19, 2020 03:21

JoelSpeed added 3 commits June 19, 2020 10:35

Add tests for Machine/MachineSet creation/update via envtest

723927e

Switch handlers to wrap admissionHandler

a2a7f48

Ensure nil value is handled in machine webhooks

e375b24

JoelSpeed reviewed Jun 19, 2020

View reviewed changes

michaelgugino force-pushed the machineset-cp-vwh branch 2 times, most recently from c4f2c30 to 3fdfdb6 Compare June 23, 2020 17:40

michaelgugino added 2 commits June 23, 2020 17:00

update vendor

0942080

michaelgugino force-pushed the machineset-cp-vwh branch from 3fdfdb6 to 0942080 Compare June 23, 2020 21:01

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2020

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 23, 2020

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 6, 2020

openshift-ci-robot closed this Jan 5, 2021

openshift-ci-robot reopened this Jan 15, 2021

openshift-ci-robot closed this Feb 14, 2021

openshift-ci bot reopened this Aug 6, 2021

openshift-ci bot closed this Sep 16, 2021

Add MachineSet ControlPlane webhooks #617

Add MachineSet ControlPlane webhooks #617

Uh oh!

Conversation

michaelgugino commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelgugino Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

mtnbikenc commented Jun 18, 2020

Uh oh!

michaelgugino Jun 18, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Jun 23, 2020

Uh oh!

openshift-ci-robot commented Jun 26, 2020

Uh oh!

openshift-bot commented Oct 23, 2020

Uh oh!

openshift-merge-robot commented Nov 6, 2020

Uh oh!

openshift-bot commented Dec 6, 2020

Uh oh!

openshift-bot commented Jan 5, 2021

Uh oh!

openshift-ci-robot commented Jan 5, 2021

Uh oh!

michaelgugino commented Jan 15, 2021

Uh oh!

openshift-ci-robot commented Jan 15, 2021

Uh oh!

openshift-ci-robot commented Jan 15, 2021

Uh oh!

openshift-bot commented Feb 14, 2021

Uh oh!

openshift-ci-robot commented Feb 14, 2021

Uh oh!

michaelgugino commented Aug 6, 2021

Uh oh!

openshift-ci bot commented Aug 6, 2021

Uh oh!

openshift-ci bot commented Aug 6, 2021

Uh oh!

openshift-ci bot commented Aug 17, 2021

Uh oh!

openshift-bot commented Sep 16, 2021

Uh oh!

openshift-ci bot commented Sep 16, 2021

Uh oh!

Uh oh!

michaelgugino commented Jun 18, 2020 •

edited

Loading