Enforce machine.openshift.io/cluster-api-cluster value for machines a…

…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.

pkg/apis/machine/v1beta1/machine_webhook.go

@@ -263,6 +263,15 @@ func (h *machineDefaulterHandler) Handle(ctx context.Context, req admission.Requ
 
 	klog.V(3).Infof("Mutate webhook called for Machine: %s", m.GetName())
 
+	// Enforce that the same clusterID is set for machineSet and machine labels.
+	// Otherwise a discrepancy on the value would leave the machine orphan
+	// and would trigger a new machine creation by the machineSet.
+	// https://bugzilla.redhat.com/show_bug.cgi?id=1857175
+	if m.Labels == nil {
+		m.Labels = make(map[string]string)
+	}
+	m.Labels[MachineClusterIDLabel] = h.clusterID
+
 	if ok, err := h.webhookOperations(m, h.clusterID); !ok {
 		return admission.Denied(err.Error())
 	}

pkg/apis/machine/v1beta1/machine_webhook_test.go

@@ -266,6 +266,7 @@ func TestMachineCreation(t *testing.T) {
 				gs.Expect(err).ToNot(BeNil())
 				gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError))
 			} else {
+				gs.Expect(m.Labels[MachineClusterIDLabel]).To(BeIdenticalTo(tc.clusterID))
 				gs.Expect(err).To(BeNil())
 			}
 		})

pkg/apis/machine/v1beta1/machineset_webhook.go

@@ -124,6 +124,20 @@ func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, ut
 	if ok, err := h.webhookOperations(m, h.clusterID); !ok {
 		errs = append(errs, err.Errors()...)
 	} else {
+		// Enforce that the same clusterID is set for machineSet and machine labels.
+		// Otherwise a discrepancy on the value would leave the machine orphan
+		// and would trigger a new machine creation by the machineSet.
+		// https://bugzilla.redhat.com/show_bug.cgi?id=1857175
+		if ms.Spec.Selector.MatchLabels == nil {
+			ms.Spec.Selector.MatchLabels = make(map[string]string)
+		}
+		ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] = h.clusterID
+
+		if ms.Spec.Template.Labels == nil {
+			ms.Spec.Template.Labels = make(map[string]string)
+		}
+		ms.Spec.Template.Labels[MachineClusterIDLabel] = h.clusterID
+
 		// Restore the defaulted template
 		ms.Spec.Template.Spec = m.Spec
 	}

pkg/apis/machine/v1beta1/machineset_webhook_test.go

@@ -267,6 +267,8 @@ func TestMachineSetCreation(t *testing.T) {
 				gs.Expect(err).ToNot(BeNil())
 				gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError))
 			} else {
+				gs.Expect(ms.Spec.Selector.MatchLabels[MachineClusterIDLabel]).To(BeIdenticalTo(tc.clusterID))
+				gs.Expect(ms.Spec.Template.Labels[MachineClusterIDLabel]).To(BeIdenticalTo(tc.clusterID))
 				gs.Expect(err).To(BeNil())
 			}
 		})