From 261c3373e317fe3c9736d48c1deb96b87be8f492 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Thu, 16 Jul 2020 13:41:58 +0200 Subject: [PATCH 1/2] Revert "Set clusterID label on machine" This reverts commit 75857f190b1430d08a6f7fa9eac7ca54e7598495. --- pkg/apis/machine/v1beta1/machine_types.go | 9 +++- pkg/controller/machine/controller.go | 27 ------------ pkg/controller/machine/controller_test.go | 43 ------------------- .../machine/machine_controller_suite_test.go | 6 +-- .../machine/machine_controller_test.go | 15 ------- 5 files changed, 9 insertions(+), 91 deletions(-) diff --git a/pkg/apis/machine/v1beta1/machine_types.go b/pkg/apis/machine/v1beta1/machine_types.go index 5cf1fc2968..ff69037560 100644 --- a/pkg/apis/machine/v1beta1/machine_types.go +++ b/pkg/apis/machine/v1beta1/machine_types.go @@ -17,6 +17,8 @@ limitations under the License. package v1beta1 import ( + "fmt" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -194,8 +196,13 @@ type LastOperation struct { func (m *Machine) Validate() field.ErrorList { errors := field.ErrorList{} - // validate provider config is set + // validate spec.labels fldPath := field.NewPath("spec") + if m.Labels[MachineClusterIDLabel] == "" { + errors = append(errors, field.Invalid(fldPath.Child("labels"), m.Labels, fmt.Sprintf("missing %v label.", MachineClusterIDLabel))) + } + + // validate provider config is set if m.Spec.ProviderSpec.Value == nil { errors = append(errors, field.Invalid(fldPath.Child("spec").Child("providerspec"), m.Spec.ProviderSpec, "value field must be set")) } diff --git a/pkg/controller/machine/controller.go b/pkg/controller/machine/controller.go index 22b4768685..8b84b7e0ab 100644 --- a/pkg/controller/machine/controller.go +++ b/pkg/controller/machine/controller.go @@ -22,7 +22,6 @@ import ( "fmt" "time" - configv1 "github.com/openshift/api/config/v1" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" "github.com/openshift/machine-api-operator/pkg/util" corev1 "k8s.io/api/core/v1" @@ -92,8 +91,6 @@ const ( unknownInstanceState = "Unknown" skipWaitForDeleteTimeoutSeconds = 60 * 5 - - globalInfrastuctureName = "cluster" ) var DefaultActuator Actuator @@ -178,11 +175,6 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul return reconcile.Result{}, err } - // Add clusterID label - if err := r.setClusterIDLabel(ctx, m); err != nil { - return reconcile.Result{}, err - } - // If object hasn't been deleted and doesn't have a finalizer, add one // Add a finalizer to newly created objects. if m.ObjectMeta.DeletionTimestamp.IsZero() { @@ -470,25 +462,6 @@ func (r *ReconcileMachine) patchFailedMachineInstanceAnnotation(machine *machine return nil } -func (r *ReconcileMachine) setClusterIDLabel(ctx context.Context, m *machinev1.Machine) error { - infra := &configv1.Infrastructure{} - infraName := client.ObjectKey{Name: globalInfrastuctureName} - - if err := r.Client.Get(ctx, infraName, infra); err != nil { - return err - } - - clusterID := infra.Status.InfrastructureName - - if m.Labels == nil { - m.Labels = make(map[string]string) - } - - m.Labels[machinev1.MachineClusterIDLabel] = clusterID - - return nil -} - func machineIsProvisioned(machine *machinev1.Machine) bool { return len(machine.Status.Addresses) > 0 || stringPointerDeref(machine.Spec.ProviderID) != "" } diff --git a/pkg/controller/machine/controller_test.go b/pkg/controller/machine/controller_test.go index 2540a8812a..3cf43202ad 100644 --- a/pkg/controller/machine/controller_test.go +++ b/pkg/controller/machine/controller_test.go @@ -24,7 +24,6 @@ import ( "time" . "github.com/onsi/gomega" - configv1 "github.com/openshift/api/config/v1" machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -41,20 +40,7 @@ var ( _ reconcile.Reconciler = &ReconcileMachine{} ) -func init() { - configv1.AddToScheme(scheme.Scheme) -} - func TestReconcileRequest(t *testing.T) { - infra := configv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: globalInfrastuctureName, - }, - Status: configv1.InfrastructureStatus{ - InfrastructureName: "test-id", - }, - } - machineProvisioning := machinev1.Machine{ TypeMeta: metav1.TypeMeta{ Kind: "Machine", @@ -292,7 +278,6 @@ func TestReconcileRequest(t *testing.T) { &machineDeleting, &machineFailed, &machineRunning, - &infra, ), scheme: scheme.Scheme, actuator: act, @@ -729,31 +714,3 @@ func TestDelayIfRequeueAfterError(t *testing.T) { }) } } - -func TestSetClusterIDLabel(t *testing.T) { - clusterID := "test-id" - infra := &configv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: globalInfrastuctureName, - }, - Status: configv1.InfrastructureStatus{ - InfrastructureName: clusterID, - }, - } - - machine := &machinev1.Machine{} - - reconciler := &ReconcileMachine{ - Client: fake.NewFakeClientWithScheme(scheme.Scheme, infra), - scheme: scheme.Scheme, - } - - if err := reconciler.setClusterIDLabel(ctx, machine); err != nil { - t.Fatal(err) - } - - actualClusterID := machine.Labels[machinev1.MachineClusterIDLabel] - if actualClusterID != clusterID { - t.Fatalf("Expected clusterID %s, got %s", clusterID, actualClusterID) - } -} diff --git a/pkg/controller/machine/machine_controller_suite_test.go b/pkg/controller/machine/machine_controller_suite_test.go index 84c013ec80..73fafa124f 100644 --- a/pkg/controller/machine/machine_controller_suite_test.go +++ b/pkg/controller/machine/machine_controller_suite_test.go @@ -23,7 +23,6 @@ import ( "path/filepath" "testing" - configv1 "github.com/openshift/api/config/v1" "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" @@ -38,12 +37,9 @@ var ( func TestMain(m *testing.M) { t := &envtest.Environment{ - CRDDirectoryPaths: []string{ - filepath.Join("..", "..", "..", "install"), - filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")}, + CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "install")}, } v1beta1.AddToScheme(scheme.Scheme) - configv1.AddToScheme(scheme.Scheme) var err error if cfg, err = t.Start(); err != nil { diff --git a/pkg/controller/machine/machine_controller_test.go b/pkg/controller/machine/machine_controller_test.go index 544d963a4a..45583780c1 100644 --- a/pkg/controller/machine/machine_controller_test.go +++ b/pkg/controller/machine/machine_controller_test.go @@ -21,7 +21,6 @@ import ( "time" . "github.com/onsi/gomega" - configv1 "github.com/openshift/api/config/v1" machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1" "golang.org/x/net/context" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -53,15 +52,6 @@ func TestReconcile(t *testing.T) { }, } - infra := &configv1.Infrastructure{ - ObjectMeta: metav1.ObjectMeta{ - Name: globalInfrastuctureName, - }, - Status: configv1.InfrastructureStatus{ - InfrastructureName: "test-id", - }, - } - // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a // channel when it is finished. mgr, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"}) @@ -84,11 +74,6 @@ func TestReconcile(t *testing.T) { } }() - if err := c.Create(context.TODO(), infra); err != nil { - t.Fatalf("error creating instance: %v", err) - } - defer c.Delete(context.TODO(), infra) - // Create the Machine object and expect Reconcile and the actuator to be called if err := c.Create(context.TODO(), instance); err != nil { t.Fatalf("error creating instance: %v", err) From 870b9e59c53adf0373dbf3804254bd113e1a9ab9 Mon Sep 17 00:00:00 2001 From: Enxebre Date: Thu, 16 Jul 2020 13:34:37 +0200 Subject: [PATCH 2/2] Enforce machine.openshift.io/cluster-api-cluster value for machines and machineSet via webhook With https://github.com/openshift/machine-api-operator/pull/608 we dropped the burden from the user to set the clusterID label on machines. As elaborated in https://github.com/openshift/machine-api-operator/pull/608#issuecomment-638772630 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 | 9 +++++++++ pkg/apis/machine/v1beta1/machine_webhook_test.go | 1 + pkg/apis/machine/v1beta1/machineset_webhook.go | 14 ++++++++++++++ .../machine/v1beta1/machineset_webhook_test.go | 2 ++ 4 files changed, 26 insertions(+) diff --git a/pkg/apis/machine/v1beta1/machine_webhook.go b/pkg/apis/machine/v1beta1/machine_webhook.go index fe5401921a..09dababd02 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook.go +++ b/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 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 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()) } diff --git a/pkg/apis/machine/v1beta1/machine_webhook_test.go b/pkg/apis/machine/v1beta1/machine_webhook_test.go index 80913fab16..0614c243e7 100644 --- a/pkg/apis/machine/v1beta1/machine_webhook_test.go +++ b/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()) } }) diff --git a/pkg/apis/machine/v1beta1/machineset_webhook.go b/pkg/apis/machine/v1beta1/machineset_webhook.go index 595f406028..6e3bd90cfd 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook.go +++ b/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 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) + } + 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 } diff --git a/pkg/apis/machine/v1beta1/machineset_webhook_test.go b/pkg/apis/machine/v1beta1/machineset_webhook_test.go index 2ea3be2932..c9b00f3084 100644 --- a/pkg/apis/machine/v1beta1/machineset_webhook_test.go +++ b/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()) } })