Make hasMatchingLabels generic

kubernetes-sigs · Feb 26, 2020 · ecf8450 · ecf8450
1 parent 2ccd5c2
commit ecf8450
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 127 deletions.
diff --git a/controllers/machine_helpers.go b/controllers/machine_helpers.go
@@ -20,6 +20,8 @@ import (
 	"context"
 
 	"github.com/pkg/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -47,3 +49,20 @@ func getActiveMachinesInCluster(ctx context.Context, c client.Client, namespace,
 	}
 	return machines, nil
 }
+
+// hasMatchingLabels verifies that the Label Selector matches the given Labels
+func hasMatchingLabels(matchSelector metav1.LabelSelector, matchLabels map[string]string) bool {
+	// This should never fail, validating webhook should catch this first
+	selector, err := metav1.LabelSelectorAsSelector(&matchSelector)
+	if err != nil {
+		return false
+	}
+	// If a MachineHealthCheck with a nil or empty selector creeps in, it should match nothing, not everything.
+	if selector.Empty() {
+		return false
+	}
+	if !selector.Matches(labels.Set(matchLabels)) {
+		return false
+	}
+	return true
+}
diff --git a/controllers/machine_helpers_test.go b/controllers/machine_helpers_test.go
@@ -127,3 +127,74 @@ func Test_getActiveMachinesInCluster(t *testing.T) {
 		})
 	}
 }
+
+func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
+	testCases := []struct {
+		name     string
+		selector metav1.LabelSelector
+		labels   map[string]string
+		expected bool
+	}{
+		{
+			name: "selector matches labels",
+
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+			},
+
+			labels: map[string]string{
+				"foo": "bar",
+			},
+
+			expected: true,
+		},
+		{
+			name: "selector does not match labels",
+
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+			},
+
+			labels: map[string]string{
+				"no": "match",
+			},
+			expected: false,
+		},
+		{
+			name:     "selector is empty",
+			selector: metav1.LabelSelector{},
+			labels:   map[string]string{},
+			expected: false,
+		},
+		{
+			name: "seelctor is invalid",
+			selector: metav1.LabelSelector{
+				MatchLabels: map[string]string{
+					"foo": "bar",
+				},
+				MatchExpressions: []metav1.LabelSelectorRequirement{
+					{
+						Operator: "bad-operator",
+					},
+				},
+			},
+			labels: map[string]string{
+				"foo": "bar",
+			},
+			expected: false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			got := hasMatchingLabels(tc.selector, tc.labels)
+			g.Expect(got).To(Equal(tc.expected))
+		})
+	}
+}
diff --git a/controllers/machinehealthcheck_controller.go b/controllers/machinehealthcheck_controller.go
@@ -27,7 +27,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kerrors "k8s.io/apimachinery/pkg/util/errors"
@@ -283,7 +282,7 @@ func (r *MachineHealthCheckReconciler) machineToMachineHealthCheck(o handler.Map
 	var requests []reconcile.Request
 	for k := range mhcList.Items {
 		mhc := &mhcList.Items[k]
-		if r.hasMatchingLabels(mhc, m) {
+		if hasMatchingLabels(mhc.Spec.Selector, m.Labels) {
 			key := util.ObjectKey(mhc)
 			requests = append(requests, reconcile.Request{NamespacedName: key})
 		}
@@ -318,7 +317,7 @@ func (r *MachineHealthCheckReconciler) nodeToMachineHealthCheck(o handler.MapObj
 	var requests []reconcile.Request
 	for k := range mhcList.Items {
 		mhc := &mhcList.Items[k]
-		if r.hasMatchingLabels(mhc, machine) {
+		if hasMatchingLabels(mhc.Spec.Selector, machine.Labels) {
 			key := util.ObjectKey(mhc)
 			requests = append(requests, reconcile.Request{NamespacedName: key})
 		}
@@ -406,21 +405,3 @@ func (r *MachineHealthCheckReconciler) indexMachineByNodeName(object runtime.Obj
 
 	return nil
 }
-
-// hasMatchingLabels verifies that the MachineHealthCheck's label selector
-// matches the given Machine
-func (r *MachineHealthCheckReconciler) hasMatchingLabels(machineHealthCheck *clusterv1.MachineHealthCheck, machine *clusterv1.Machine) bool {
-	// This should never fail, validating webhook should catch this first
-	selector, err := metav1.LabelSelectorAsSelector(&machineHealthCheck.Spec.Selector)
-	if err != nil {
-		return false
-	}
-	// If a MachineHealthCheck with a nil or empty selector creeps in, it should match nothing, not everything.
-	if selector.Empty() {
-		return false
-	}
-	if !selector.Matches(labels.Set(machine.Labels)) {
-		return false
-	}
-	return true
-}
diff --git a/controllers/machinehealthcheck_controller_test.go b/controllers/machinehealthcheck_controller_test.go
@@ -31,7 +31,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes/scheme"
-	"k8s.io/klog/klogr"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/controllers/external"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -420,111 +419,6 @@ func newTestMachineHealthCheck(name, namespace, cluster string, labels map[strin
 	}
 }
 
-func TestMachineHealthCheckHasMatchingLabels(t *testing.T) {
-	r := &MachineHealthCheckReconciler{
-		Log: klogr.New(),
-	}
-
-	testCases := []struct {
-		name               string
-		machineHealthCheck clusterv1.MachineHealthCheck
-		machine            clusterv1.Machine
-		expected           bool
-	}{
-		{
-			name: "machine set and machine have matching labels",
-			machineHealthCheck: clusterv1.MachineHealthCheck{
-				Spec: clusterv1.MachineHealthCheckSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "matchSelector",
-					Labels: map[string]string{
-						"foo": "bar",
-					},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "machine set and machine do not have matching labels",
-			machineHealthCheck: clusterv1.MachineHealthCheck{
-				Spec: clusterv1.MachineHealthCheckSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "doesNotMatchSelector",
-					Labels: map[string]string{
-						"no": "match",
-					},
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "machine set has empty selector",
-			machineHealthCheck: clusterv1.MachineHealthCheck{
-				Spec: clusterv1.MachineHealthCheckSpec{
-					Selector: metav1.LabelSelector{},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "doesNotMatter",
-				},
-			},
-			expected: false,
-		},
-		{
-			name: "machine set has bad selector",
-			machineHealthCheck: clusterv1.MachineHealthCheck{
-				Spec: clusterv1.MachineHealthCheckSpec{
-					Selector: metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"foo": "bar",
-						},
-						MatchExpressions: []metav1.LabelSelectorRequirement{
-							{
-								Operator: "bad-operator",
-							},
-						},
-					},
-				},
-			},
-			machine: clusterv1.Machine{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "match",
-					Labels: map[string]string{
-						"foo": "bar",
-					},
-				},
-			},
-			expected: false,
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			got := r.hasMatchingLabels(&tc.machineHealthCheck, &tc.machine)
-			g.Expect(got).To(Equal(tc.expected))
-		})
-	}
-}
-
 func TestMachineToMachineHealthCheck(t *testing.T) {
 	// This test sets up a proper test env to allow testing of the cache index
 	// that is used as part of the clusterToMachineHealthCheck map function