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

enxebre · 2020-07-20T11:41:13Z

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.

JoelSpeed

Test looks good

Made me realise that at the moment we are presently just stomping the cluster label if anyone has set it to a different value, this may be surprising behaviour as users would see their desired spec has been changed as they create it, should we be warning or rejecting somehow?

Also, are we validating that people can't change the labels later via webhooks? I think we should probably be doing so otherwise users would be able to update the labels to make them invalid after the creation

enxebre · 2020-07-21T07:40:38Z

Also, are we validating that people can't change the labels later via webhooks? I think we should probably be doing so otherwise users would be able to update the labels to make them invalid after the creation

They can do that. Same that they can do it today with out this.
Default Webhooks are only enabled on creation. I agree we should probably enforce on update as well but enabling defaulting on update will have multiple other implications for other fields.
This is an improvement over what we have today where users need to handcraft the clusterID label manually when creating a machine.
Enabling defaulting on update will need to be analysed separately considering the implications for all the fields.

JoelSpeed · 2020-07-21T09:31:28Z

/approve

openshift-ci-robot · 2020-07-21T09:31:46Z

[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

elmiko

/lgtm

openshift-bot · 2020-07-22T14:33:49Z

/retest