From 7cc45ccde8d0bcaa8fd375fa1de530467e59a01b Mon Sep 17 00:00:00 2001 From: Ben Moss Date: Tue, 23 Jun 2020 14:37:21 +0000 Subject: [PATCH] Scope MHC machines to the cluster matching their clusterName --- api/v1alpha3/machinehealthcheck_webhook.go | 6 +++++ .../machinehealthcheck_webhook_test.go | 23 +++++++++++++++++++ controllers/machinehealthcheck_controller.go | 10 +++++++- controllers/machinehealthcheck_targets.go | 17 ++++++++------ .../machinehealthcheck_targets_test.go | 10 +++----- 5 files changed, 51 insertions(+), 15 deletions(-) diff --git a/api/v1alpha3/machinehealthcheck_webhook.go b/api/v1alpha3/machinehealthcheck_webhook.go index f48cc638a323..4aad8ea226f3 100644 --- a/api/v1alpha3/machinehealthcheck_webhook.go +++ b/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, diff --git a/api/v1alpha3/machinehealthcheck_webhook_test.go b/api/v1alpha3/machinehealthcheck_webhook_test.go index 881b1d640dd9..1ca885b75eb2 100644 --- a/api/v1alpha3/machinehealthcheck_webhook_test.go +++ b/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()) +} diff --git a/controllers/machinehealthcheck_controller.go b/controllers/machinehealthcheck_controller.go index 52c8ef5cc5a3..63c5071c74bf 100644 --- a/controllers/machinehealthcheck_controller.go +++ b/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 +} diff --git a/controllers/machinehealthcheck_targets.go b/controllers/machinehealthcheck_targets.go index e26ece445d4b..cc31adf4adca 100644 --- a/controllers/machinehealthcheck_targets.go +++ b/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 diff --git a/controllers/machinehealthcheck_targets_test.go b/controllers/machinehealthcheck_targets_test.go index fdc8bc5b717e..e090e537a413 100644 --- a/controllers/machinehealthcheck_targets_test.go +++ b/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, - }, }, }, }