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

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"))
 	}

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())
 	}

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 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
 	}

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())
 			}
 		})

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) != ""
 }

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)
-	}
-}

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 {

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)