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