Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend #644

enxebre · 2020-07-16T11:54:10Z

With #608 we dropped the burden from the user to set the clusterID label on machines.
As elaborated in #608 (comment) the motivation is that this is an implementation detail that users shouldn't care about.

However as the labels are used by machineSet to determine ownership, the change introduced above might result in edge scenarios where the machineSet and machine label has a different value. This would result in machines going orphan and the machineSet recreating new instances. Bad.

Therefore we choose now to remove the burden from users by enforcing the label value via webhhooks and keeping the old behaviour in the backend to avoid any chance of breaking existing environments where bad input might have been set as in https://bugzilla.redhat.com/show_bug.cgi?id=1857175.

openshift/cluster-api-actuator-pkg#178
openshift/cluster-api-provider-gcp#106
openshift/cluster-api-provider-azure#153
openshift/cluster-api-provider-aws#341

This reverts commit 75857f1.

openshift-ci-robot · 2020-07-16T11:54:44Z

@enxebre: This pull request references Bugzilla bug 1857175, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend

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.

enxebre · 2020-07-16T11:54:54Z

/hold
to do some manual test.

openshift-ci-robot · 2020-07-16T11:55:23Z

@enxebre: This pull request references Bugzilla bug 1857175, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend

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.

JoelSpeed

Should we update the validating webhooks so that the label can't be modified later as well? I expect we would want the clusterID label to be immutable?

JoelSpeed · 2020-07-16T12:01:19Z

pkg/apis/machine/v1beta1/machine_webhook.go

+	if m.GetLabels() == nil {
+		m.Labels = make(map[string]string)
+	}
+	m.Labels[MachineClusterIDLabel] = h.clusterID


Why use the interface accessor on one line, then the struct member on another? Seems odd to me

Suggested change

if m.GetLabels() == nil {

m.Labels = make(map[string]string)

}

m.Labels[MachineClusterIDLabel] = h.clusterID

if m.Labels == nil {

m.Labels = make(map[string]string)

}

m.Labels[MachineClusterIDLabel] = h.clusterID

ok, updated.

JoelSpeed · 2020-07-16T12:01:52Z

pkg/apis/machine/v1beta1/machineset_webhook.go

+		if ms.GetLabels() == nil {
+			ms.Labels = make(map[string]string)
+		}
+		ms.Labels[MachineClusterIDLabel] = h.clusterID


Same as above

Suggested change

if ms.GetLabels() == nil {

ms.Labels = make(map[string]string)

}

ms.Labels[MachineClusterIDLabel] = h.clusterID

if ms.Labels == nil {

ms.Labels = make(map[string]string)

}

ms.Labels[MachineClusterIDLabel] = h.clusterID

enxebre · 2020-07-16T12:12:42Z

Should we update the validating webhooks so that the label can't be modified later as well? I expect we would want the clusterID label to be immutable?

Yeh but this might have other implications on existing resources difficult to predict. Let's scope this PR to fix the existing issue while improving the current experience for creation.

JoelSpeed · 2020-07-16T12:47:00Z

Yeh but this might have other implications on existing resources difficult to predict. Let's scope this PR to fix the existing issue while improving the current experience for creation.

Make sense

/approve

openshift-ci-robot · 2020-07-16T12:47:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

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

enxebre · 2020-07-16T17:44:19Z

tested this on a running cluster and realised I was defaulting the wrong labels for the machineSet. Fixed now.
I'll keep the hold while I put a counter PR for e2e testing the scenario described in the BZ.

…nd machineSet via webhook With openshift#608 we dropped the burden from the user to set the clusterID label on machines. As elaborated in openshift#608 (comment) the motivation is that this is an implementation detail that users shouldn't care about. However as the labels are used by machineSet to determine ownership, the change introduced above might result in edge scenarios where the machineSet and machine label has a different value. This would result in machines going orphan and the machineSet recreating new instances. Bad. Therefore we choose now to remove the burden from users by enforcing the label value via webhhooks and keeping the old behaviour in the backend to avoid any chance of breaking existing environments where bad input might have been set as in https://bugzilla.redhat.com/show_bug.cgi?id=1857175.

enxebre · 2020-07-17T08:00:07Z

/retest

…uster-api-cluster With #608 we dropped the burden from the user to set the clusterID label on machines. As elaborated in #608 (comment) the motivation is that this is an implementation detail that users shouldn't care about. However as the labels are used by machineSet to determine ownership, the change introduced above might result in edge scenarios where the machineSet and machine label has a different value. This would result in machines going orphan and the machineSet recreating new instances. Bad. Therefore we choose now to remove the burden from users by enforcing the label value via webhhooks and keeping the old behaviour in the backend to avoid any chance of breaking existing environments where bad input might have been set as in https://bugzilla.redhat.com/show_bug.cgi?id=1857175. This should be fixed by openshift/machine-api-operator#644 and this PR validates that scenario.

…hift.io/cluster-api-cluster With #608 we dropped the burden from the user to set the clusterID label on machines. As elaborated in #608 (comment) the motivation is that this is an implementation detail that users shouldn't care about. However as the labels are used by machineSet to determine ownership, the change introduced above might result in edge scenarios where the machineSet and machine label has a different value. This would result in machines going orphan and the machineSet recreating new instances. Bad. Therefore we choose now to remove the burden from users by enforcing the label value via webhhooks and keeping the old behaviour in the backend to avoid any chance of breaking existing environments where bad input might have been set as in https://bugzilla.redhat.com/show_bug.cgi?id=1857175. This should be fixed by openshift/machine-api-operator#644 and this PR validates that scenario.

enxebre · 2020-07-17T08:38:19Z

/hold cancel
Once this is merged I'll create the PR for https://github.com/openshift/cluster-api-actuator-pkg/compare/master...enxebre:bug-1857175-test?expand=1.

openshift-ci-robot · 2020-07-17T08:38:32Z

@enxebre: This pull request references Bugzilla bug 1857175, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend

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.

enxebre · 2020-07-20T07:35:47Z

/test e2e-azure-operator

Danil-Grigorev

/lgtm

openshift-bot · 2020-07-20T09:16:13Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-07-20T10:10:07Z

@enxebre: All pull requests linked via external trackers have merged: openshift/machine-api-operator#644. Bugzilla bug 1857175 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend

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.

This enabled webhooks to enforce the clusterID labels on machineSet/machine creation openshift#644. To avoid any difficult to predict side effect we want to honour any existing value that is already set on existing resources.

This is to bring openshift/machine-api-operator#644 and so restoring the previous backend behaviour in the machine controller openshift/machine-api-operator@261c337

…-api-operator#644 This is to bring openshift/machine-api-operator#644 and so restoring the previous backend behaviour in the machine controller openshift/machine-api-operator@261c337

openshift-ci-robot · 2020-07-22T11:09:17Z

@enxebre: Some pull requests linked via external trackers have merged: . The following pull requests linked via external trackers have not merged:

openshift/machine-api-operator#653 is open
Bugzilla bug 1857175 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend

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.

Bug 1857175: Revendor mao to bring openshift/machine-api-operator#644

…-api-operator#644 This is to bring openshift/machine-api-operator#644 and so restoring the previous backend behaviour in the machine controller openshift/machine-api-operator@261c337

This tries to fix the following scenario: We set ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] if it's not present. It's not present and we set it to the correct value. If there happens to be a bad label in `ms.Spec.Template.Labels` this would result in a miss match. Follow for openshift#608, openshift#644 and openshift#653.

This enabled webhooks to enforce the clusterID labels on machineSet/machine creation openshift#644. To avoid any difficult to predict side effect we want to honour any existing value that is already set on existing resources.

Revert "Set clusterID label on machine"

261c337

This reverts commit 75857f1.

openshift-ci-robot requested review from JoelSpeed and alexander-demicev July 16, 2020 11:54

enxebre changed the title ~~Bug 1857175~~ Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend Jul 16, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Jul 16, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 16, 2020

JoelSpeed reviewed Jul 16, 2020

View reviewed changes

enxebre force-pushed the bug-1857175 branch from efbcca0 to 7e5eb14 Compare July 16, 2020 12:12

enxebre force-pushed the bug-1857175 branch from 7e5eb14 to adf1632 Compare July 16, 2020 12:27

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2020

enxebre force-pushed the bug-1857175 branch from adf1632 to fc01ccc Compare July 16, 2020 17:42

enxebre force-pushed the bug-1857175 branch from fc01ccc to 870b9e5 Compare July 16, 2020 18:05

enxebre mentioned this pull request Jul 17, 2020

Add webhook test for machineSet/machine clusterID label machine.openshift.io/cluster-api-cluster openshift/cluster-api-actuator-pkg#175

Closed

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2020

enxebre mentioned this pull request Jul 17, 2020

Show error message when fail to get clusterId from infrastructure #639

Closed

Danil-Grigorev approved these changes Jul 20, 2020

View reviewed changes

openshift-ci-robot assigned Danil-Grigorev Jul 20, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2020

openshift-merge-robot merged commit 86cba08 into openshift:master Jul 20, 2020

enxebre mentioned this pull request Jul 20, 2020

Add webhook test for machineSet/machine clusterID label machine.openshift.io/cluster-api-cluster openshift/cluster-api-actuator-pkg#178

Closed

enxebre mentioned this pull request Jul 22, 2020

bug 1857175: Let webhook only set clusterID label if it does not exist #653

Merged

enxebre added a commit to enxebre/cluster-api-provider-gcp-1 that referenced this pull request Jul 22, 2020

Revendor mao to bring openshift/machine-api-operator#644

55ee93a

enxebre added a commit to enxebre/cluster-api-provider-azure that referenced this pull request Jul 22, 2020

Revendor mao to bring openshift/machine-api-operator#644

b029710

enxebre mentioned this pull request Jul 22, 2020

Bug 1857175: Revendor mao to bring https://github.com/openshift/machine-api-operator/pull/644 openshift/cluster-api-provider-azure#153

Merged

openshift-merge-robot added a commit to openshift/cluster-api-provider-gcp that referenced this pull request Jul 22, 2020

Merge pull request #106 from enxebre/bug-1857175

c91b43b

Bug 1857175: Revendor mao to bring openshift/machine-api-operator#644

openshift-merge-robot added a commit to openshift/cluster-api-provider-aws that referenced this pull request Jul 22, 2020

Merge pull request #341 from enxebre/bug-1857175

709d54e

Bug 1857175: Revendor mao to bring openshift/machine-api-operator#644

enxebre mentioned this pull request Jul 27, 2020

Set default clusterID labels only on machine #659

Merged

alexander-demicev mentioned this pull request Dec 13, 2021

Rebase to latest upstream release openshift/cluster-api-provider-azure#248

Merged

This was referenced Jan 7, 2022

Rebase upstream latest openshift/cluster-api-provider-aws#433

Merged

Rebase upstream latest openshift/cluster-api-provider-gcp#181

Merged

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend #644

Bug 1857175: enforce clusterID label via webhook and preserve old behaviour in the backend #644

Uh oh!

Conversation

enxebre commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jul 16, 2020

Uh oh!

enxebre commented Jul 16, 2020

Uh oh!

openshift-ci-robot commented Jul 16, 2020

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

enxebre Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

JoelSpeed Jul 16, 2020

Choose a reason for hiding this comment

Uh oh!

enxebre commented Jul 16, 2020

Uh oh!

JoelSpeed commented Jul 16, 2020

Uh oh!

openshift-ci-robot commented Jul 16, 2020

Uh oh!

enxebre commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

enxebre commented Jul 17, 2020

Uh oh!

enxebre commented Jul 17, 2020

Uh oh!

openshift-ci-robot commented Jul 17, 2020

Uh oh!

enxebre commented Jul 20, 2020

Uh oh!

Danil-Grigorev left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-bot commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 20, 2020

Uh oh!

openshift-ci-robot commented Jul 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

enxebre commented Jul 16, 2020 •

edited

Loading

enxebre commented Jul 16, 2020 •

edited

Loading