From ba3f450a78a4399edd16d29e50510b970301af3b Mon Sep 17 00:00:00 2001 From: Enxebre Date: Mon, 27 Jul 2020 14:17:10 +0200 Subject: [PATCH 1/3] Set default clusterID labels only on machine 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 https://github.com/openshift/machine-api-operator/pull/608, https://github.com/openshift/machine-api-operator/pull/644 and https://github.com/openshift/machine-api-operator/pull/653. --- pkg/apis/machine/v1beta1/machine_webhook.go | 2 +- .../machine/v1beta1/machineset_webhook.go | 18 ------------- .../v1beta1/machineset_webhook_test.go | 25 +++---------------- 3 files changed, 4 insertions(+), 41 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index 2fa1558194..77dfd84b53 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/pkg/apis/machine/v1beta1/machine_webhook.go @@ -445,7 +445,7 @@ 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 Selector and machine labels. + // Only enforce the clusterID if it's not set. // 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 diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go index c79a7250a9..595f406028 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook.go @@ -124,24 +124,6 @@ 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 Selector 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) - } - if _, ok := ms.Spec.Selector.MatchLabels[MachineClusterIDLabel]; !ok { - ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] = h.clusterID - } - - if ms.Spec.Template.Labels == nil { - ms.Spec.Template.Labels = make(map[string]string) - } - if _, ok := ms.Spec.Template.Labels[MachineClusterIDLabel]; !ok { - ms.Spec.Template.Labels[MachineClusterIDLabel] = h.clusterID - } - // Restore the defaulted template ms.Spec.Template.Spec = m.Spec } diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index a3de21a224..9b02bf006e 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook_test.go @@ -47,7 +47,6 @@ func TestMachineSetCreation(t *testing.T) { name string platformType osconfigv1.PlatformType clusterID string - presetClusterID bool expectedError string providerSpecValue *runtime.RawExtension }{ @@ -174,10 +173,9 @@ func TestMachineSetCreation(t *testing.T) { expectedError: "[providerSpec.template: Required value: template must be provided, providerSpec.workspace: Required value: workspace must be provided, providerSpec.network.devices: Required value: at least 1 network device must be provided]", }, { - name: "with vSphere and the template, workspace and network devices set", - platformType: osconfigv1.VSpherePlatformType, - clusterID: "vsphere-cluster", - presetClusterID: true, + name: "with vSphere and the template, workspace and network devices set", + platformType: osconfigv1.VSpherePlatformType, + clusterID: "vsphere-cluster", providerSpecValue: &runtime.RawExtension{ Object: &vsphere.VSphereMachineProviderSpec{ Template: "template", @@ -259,15 +257,6 @@ func TestMachineSetCreation(t *testing.T) { }, } - presetClusterID := "anything" - if tc.presetClusterID { - ms.Spec.Selector.MatchLabels = make(map[string]string) - ms.Spec.Selector.MatchLabels[MachineClusterIDLabel] = presetClusterID - - ms.Spec.Template.Labels = make(map[string]string) - ms.Spec.Template.Labels[MachineClusterIDLabel] = presetClusterID - } - err = c.Create(ctx, ms) if err == nil { defer func() { @@ -279,14 +268,6 @@ func TestMachineSetCreation(t *testing.T) { gs.Expect(err).ToNot(BeNil()) gs.Expect(apierrors.ReasonForError(err)).To(BeEquivalentTo(tc.expectedError)) } else { - if tc.presetClusterID { - gs.Expect(ms.Spec.Selector.MatchLabels[MachineClusterIDLabel]).To(BeIdenticalTo(presetClusterID)) - gs.Expect(ms.Spec.Template.Labels[MachineClusterIDLabel]).To(BeIdenticalTo(presetClusterID)) - - } 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()) } }) From 137c0e3ec9bbf2e854502ab17b0404e66e62fc26 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Mon, 3 Aug 2020 09:41:09 +0200 Subject: [PATCH 2/3] Update comment for defaultMachineSet --- pkg/apis/machine/v1beta1/machineset_webhook.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go index 595f406028..d97bdaf321 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook.go @@ -124,7 +124,7 @@ func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, ut if ok, err := h.webhookOperations(m, h.clusterID); !ok { errs = append(errs, err.Errors()...) } else { - // Restore the defaulted template + // Update the template to the defaulted one. ms.Spec.Template.Spec = m.Spec } From 6c2f1dc1dcf573ef35af50426cd66aa88ea96650 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Mon, 3 Aug 2020 09:49:51 +0200 Subject: [PATCH 3/3] Return early if webhookOperations fail in defaultMachineSet --- pkg/apis/machine/v1beta1/machineset_webhook.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go index d97bdaf321..6dd8998551 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook.go +++ b/pkg/apis/machine/v1beta1/machineset_webhook.go @@ -117,19 +117,13 @@ func (h *machineSetValidatorHandler) validateMachineSet(ms *MachineSet) (bool, u } func (h *machineSetDefaulterHandler) defaultMachineSet(ms *MachineSet) (bool, utilerrors.Aggregate) { - var errs []error - // Create a Machine from the MachineSet and default the Machine template m := &Machine{Spec: ms.Spec.Template.Spec} if ok, err := h.webhookOperations(m, h.clusterID); !ok { - errs = append(errs, err.Errors()...) - } else { - // Update the template to the defaulted one. - ms.Spec.Template.Spec = m.Spec + return false, utilerrors.NewAggregate(err.Errors()) } - if len(errs) > 0 { - return false, utilerrors.NewAggregate(errs) - } + // Restore the defaulted template + ms.Spec.Template.Spec = m.Spec return true, nil }