🐛 Scope MHC machines to the cluster matching their clusterName

api/v1alpha3/machinehealthcheck_webhook.go

@@ -106,6 +106,12 @@ func (m *MachineHealthCheck) validate(old *MachineHealthCheck) error {
 		)
 	}
 
+	if clusterName, ok := m.Spec.Selector.MatchLabels[ClusterLabelName]; ok && clusterName != m.Spec.ClusterName {
+		allErrs = append(
+			allErrs,
+			field.Invalid(field.NewPath("spec", "selector"), m.Spec.Selector, "cannot specify a cluster selector other than the one specified by ClusterName"))
+	}
+
 	if old != nil && old.Spec.ClusterName != m.Spec.ClusterName {
 		allErrs = append(
 			allErrs,

api/v1alpha3/machinehealthcheck_webhook_test.go

@@ -260,3 +260,26 @@ func TestMachineHealthCheckSelectorValidation(t *testing.T) {
 	g.Expect(err).ToNot(BeNil())
 	g.Expect(err.Error()).To(ContainSubstring("selector must not be empty"))
 }
+
+func TestMachineHealthCheckClusterNameSelectorValidation(t *testing.T) {
+	g := NewWithT(t)
+	mhc := &MachineHealthCheck{
+		Spec: MachineHealthCheckSpec{
+			ClusterName: "foo",
+			Selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					ClusterLabelName: "bar",
+					"baz":            "qux",
+				},
+			},
+		},
+	}
+	err := mhc.validate(nil)
+	g.Expect(err).To(HaveOccurred())
+	g.Expect(err.Error()).To(ContainSubstring("cannot specify a cluster selector other than the one specified by ClusterName"))
+
+	mhc.Spec.Selector.MatchLabels[ClusterLabelName] = "foo"
+	g.Expect(mhc.validate(nil)).To(Succeed())
+	delete(mhc.Spec.Selector.MatchLabels, ClusterLabelName)
+	g.Expect(mhc.validate(nil)).To(Succeed())
+}

controllers/machinehealthcheck_controller.go

@@ -373,7 +373,7 @@ func (r *MachineHealthCheckReconciler) getMachineFromNode(nodeName string) (*clu
 		}
 	}
 	if len(items) != 1 {
-		return nil, errors.Errorf("expecting one machine for node %v, got %v", nodeName, items)
+		return nil, errors.Errorf("expecting one machine for node %v, got %v", nodeName, machineNames(items))
 	}
 	return items[0], nil
 }
@@ -424,3 +424,11 @@ func isAllowedRemediation(mhc *clusterv1.MachineHealthCheck) bool {
 	unhealthy := mhc.Status.ExpectedMachines - mhc.Status.CurrentHealthy
 	return int(unhealthy) <= maxUnhealthy
 }
+
+func machineNames(machines []*clusterv1.Machine) []string {
+	result := make([]string, 0, len(machines))
+	for _, m := range machines {
+		result = append(result, m.Name)
+	}
+	return result
+}

controllers/machinehealthcheck_targets.go

@@ -197,17 +197,20 @@ func (r *MachineHealthCheckReconciler) getTargetsFromMHC(clusterClient client.Re
 //getMachinesFromMHC fetches Machines matched by the MachineHealthCheck's
 // label selector
 func (r *MachineHealthCheckReconciler) getMachinesFromMHC(mhc *clusterv1.MachineHealthCheck) ([]clusterv1.Machine, error) {
-	selector, err := metav1.LabelSelectorAsSelector(&mhc.Spec.Selector)
+	selector, err := metav1.LabelSelectorAsSelector(metav1.CloneSelectorAndAddLabel(
+		&mhc.Spec.Selector, clusterv1.ClusterLabelName, mhc.Spec.ClusterName,
+	))
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to build selector")
 	}
 
-	options := client.ListOptions{
-		LabelSelector: selector,
-		Namespace:     mhc.GetNamespace(),
-	}
-	machineList := &clusterv1.MachineList{}
-	if err := r.Client.List(context.Background(), machineList, &options); err != nil {
+	var machineList clusterv1.MachineList
+	if err := r.Client.List(
+		context.Background(),
+		&machineList,
+		client.MatchingLabelsSelector{Selector: selector},
+		client.InNamespace(mhc.GetNamespace()),
+	); err != nil {
 		return nil, errors.Wrap(err, "failed to list machines")
 	}
 	return machineList.Items, nil

controllers/machinehealthcheck_targets_test.go

@@ -48,6 +48,7 @@ func TestGetTargetsFromMHC(t *testing.T) {
 			Namespace: namespace,
 		},
 		Spec: clusterv1.MachineHealthCheckSpec{
+			ClusterName: clusterName,
 			Selector: metav1.LabelSelector{
 				MatchLabels: mhcSelector,
 			},
@@ -72,7 +73,7 @@ func TestGetTargetsFromMHC(t *testing.T) {
 	testNode3 := newTestNode("node3")
 	testMachine3 := newTestMachine("machine3", namespace, clusterName, testNode3.Name, mhcSelector)
 	testNode4 := newTestNode("node4")
-	testMachine4 := newTestMachine("machine4", namespace, clusterName, testNode4.Name, mhcSelector)
+	testMachine4 := newTestMachine("machine4", namespace, "other-cluster", testNode4.Name, mhcSelector)
 
 	testCases := []struct {
 		desc            string
@@ -97,7 +98,7 @@ func TestGetTargetsFromMHC(t *testing.T) {
 			},
 		},
 		{
-			desc:     "when a machine's labels do not match",
+			desc:     "when a machine's labels do not match the selector",
 			toCreate: append(baseObjects, testMachine1, testMachine2, testNode1),
 			expectedTargets: []healthCheckTarget{
 				{
@@ -121,11 +122,6 @@ func TestGetTargetsFromMHC(t *testing.T) {
 					MHC:     testMHC,
 					Node:    testNode3,
 				},
-				{
-					Machine: testMachine4,
-					MHC:     testMHC,
-					Node:    testNode4,
-				},
 			},
 		},
 	}