generalize cluster selector method (#204)

Signed-off-by: haoqing0110 <qhao@redhat.com>

Makefile

@@ -87,7 +87,7 @@ verify-fmt-imports: install-golang-gci
 	    echo "Diff output is empty"; \
 	fi
 
-verify: verify-crds verify-gocilint
+verify: verify-fmt-imports verify-crds verify-gocilint
 
 ensure-operator-sdk:
 ifeq "" "$(wildcard $(OPERATOR_SDK))"

pkg/placement/controllers/scheduling/schedule_test.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	clusterapiv1beta2 "open-cluster-management.io/api/cluster/v1beta2"
 	"reflect"
 	"sort"
 	"testing"
@@ -16,6 +15,7 @@ import (
 	clusterapiv1 "open-cluster-management.io/api/cluster/v1"
 	clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
 	clusterlisterv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
+	clusterapiv1beta2 "open-cluster-management.io/api/cluster/v1beta2"
 
 	"open-cluster-management.io/ocm/pkg/placement/controllers/framework"
 	testinghelpers "open-cluster-management.io/ocm/pkg/placement/helpers/testing"

pkg/placement/helpers/clusters.go

@@ -0,0 +1,74 @@
+package helpers
+
+import (
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+
+	clusterapiv1 "open-cluster-management.io/api/cluster/v1"
+	clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
+)
+
+type ClusterSelector struct {
+	labelSelector labels.Selector
+	claimSelector labels.Selector
+}
+
+func NewClusterSelector(selector clusterapiv1beta1.ClusterSelector) (*ClusterSelector, error) {
+	// build label selector
+	labelSelector, err := convertLabelSelector(&selector.LabelSelector)
+	if err != nil {
+		return nil, err
+	}
+	// build claim selector
+	claimSelector, err := convertClaimSelector(&selector.ClaimSelector)
+	if err != nil {
+		return nil, err
+	}
+	return &ClusterSelector{
+		labelSelector: labelSelector,
+		claimSelector: claimSelector,
+	}, nil
+}
+
+func (c *ClusterSelector) Matches(clusterlabels, clusterclaims map[string]string) bool {
+	// match with label selector
+	if ok := c.labelSelector.Matches(labels.Set(clusterlabels)); !ok {
+		return false
+	}
+	// match with claim selector
+	if ok := c.claimSelector.Matches(labels.Set(clusterclaims)); !ok {
+		return false
+	}
+	return true
+}
+
+// convertLabelSelector converts metav1.LabelSelector to labels.Selector
+func convertLabelSelector(labelSelector *metav1.LabelSelector) (labels.Selector, error) {
+	selector, err := metav1.LabelSelectorAsSelector(labelSelector)
+	if err != nil {
+		return labels.Nothing(), err
+	}
+
+	return selector, nil
+}
+
+// convertClaimSelector converts ClusterClaimSelector to labels.Selector
+func convertClaimSelector(clusterClaimSelector *clusterapiv1beta1.ClusterClaimSelector) (labels.Selector, error) {
+	selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
+		MatchExpressions: clusterClaimSelector.MatchExpressions,
+	})
+	if err != nil {
+		return labels.Nothing(), err
+	}
+
+	return selector, nil
+}
+
+// GetClusterClaims returns a map containing cluster claims from the status of cluster
+func GetClusterClaims(cluster *clusterapiv1.ManagedCluster) map[string]string {
+	claims := map[string]string{}
+	for _, claim := range cluster.Status.ClusterClaims {
+		claims[claim.Name] = claim.Value
+	}
+	return claims
+}

pkg/placement/helpers/clusters_test.go

@@ -0,0 +1,168 @@
+package helpers
+
+import (
+	"testing"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	clusterapiv1 "open-cluster-management.io/api/cluster/v1"
+	clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
+
+	testinghelpers "open-cluster-management.io/ocm/pkg/placement/helpers/testing"
+)
+
+func TestMatches(t *testing.T) {
+	cases := []struct {
+		name            string
+		clusterselector clusterapiv1beta1.ClusterSelector
+		clusterlabels   map[string]string
+		clusterclaims   map[string]string
+		expectedMatch   bool
+	}{
+		{
+			name: "match with label",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				LabelSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"cloud": "Amazon",
+					},
+				},
+			},
+			clusterlabels: map[string]string{"cloud": "Amazon"},
+			clusterclaims: map[string]string{},
+			expectedMatch: true,
+		},
+		{
+			name: "not match with label",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				LabelSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"cloud": "Amazon",
+					},
+				},
+			},
+			clusterlabels: map[string]string{"cloud": "Google"},
+			clusterclaims: map[string]string{},
+			expectedMatch: false,
+		},
+		{
+			name: "match with claim",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				ClaimSelector: clusterapiv1beta1.ClusterClaimSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      "cloud",
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"Amazon"},
+						},
+					},
+				},
+			},
+			clusterlabels: map[string]string{},
+			clusterclaims: map[string]string{"cloud": "Amazon"},
+			expectedMatch: true,
+		},
+		{
+			name: "not match with claim",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				ClaimSelector: clusterapiv1beta1.ClusterClaimSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      "cloud",
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"Amazon"},
+						},
+					},
+				},
+			},
+			clusterlabels: map[string]string{},
+			clusterclaims: map[string]string{"cloud": "Google"},
+			expectedMatch: false,
+		},
+		{
+			name: "match with both label and claim",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				LabelSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"cloud": "Amazon",
+					},
+				},
+				ClaimSelector: clusterapiv1beta1.ClusterClaimSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      "region",
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"us-east-1"},
+						},
+					},
+				},
+			},
+			clusterlabels: map[string]string{"cloud": "Amazon"},
+			clusterclaims: map[string]string{"region": "us-east-1"},
+			expectedMatch: true,
+		},
+		{
+			name: "not match with both label and claim",
+			clusterselector: clusterapiv1beta1.ClusterSelector{
+				LabelSelector: metav1.LabelSelector{
+					MatchLabels: map[string]string{
+						"cloud": "Amazon",
+					},
+				},
+				ClaimSelector: clusterapiv1beta1.ClusterClaimSelector{
+					MatchExpressions: []metav1.LabelSelectorRequirement{
+						{
+							Key:      "region",
+							Operator: metav1.LabelSelectorOpIn,
+							Values:   []string{"us-east-1"},
+						},
+					},
+				},
+			},
+			clusterlabels: map[string]string{"region": "us-east-1"},
+			clusterclaims: map[string]string{"cloud": "Amazon"},
+			expectedMatch: false,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			clusterSelector, err := NewClusterSelector(c.clusterselector)
+			if err != nil {
+				t.Errorf("unexpected err: %v", err)
+			}
+			result := clusterSelector.Matches(c.clusterlabels, c.clusterclaims)
+			if c.expectedMatch != result {
+				t.Errorf("expected match to be %v but get : %v", c.expectedMatch, result)
+			}
+		})
+	}
+}
+
+func TestGetClusterClaims(t *testing.T) {
+	cases := []struct {
+		name     string
+		cluster  *clusterapiv1.ManagedCluster
+		expected map[string]string
+	}{
+		{
+			name:     "convert cluster claim",
+			cluster:  testinghelpers.NewManagedCluster("cluster1").WithClaim("cloud", "Amazon").Build(),
+			expected: map[string]string{"cloud": "Amazon"},
+		},
+		{
+			name:     "convert emtpy cluster claim",
+			cluster:  testinghelpers.NewManagedCluster("cluster1").Build(),
+			expected: map[string]string{},
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			actual := GetClusterClaims(c.cluster)
+			if len(actual) != len(c.expected) {
+				t.Errorf("expected %v but get %v", c.expected, actual)
+			}
+		})
+	}
+}

pkg/placement/plugins/predicate/predicate.go

@@ -4,13 +4,11 @@ import (
 	"context"
 	"reflect"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
-
 	clusterapiv1 "open-cluster-management.io/api/cluster/v1"
 	clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1"
 
 	"open-cluster-management.io/ocm/pkg/placement/controllers/framework"
+	"open-cluster-management.io/ocm/pkg/placement/helpers"
 	"open-cluster-management.io/ocm/pkg/placement/plugins"
 )
 
@@ -20,11 +18,6 @@ const description = "Predicate filter filters the clusters based on predicate de
 
 type Predicate struct{}
 
-type predicateSelector struct {
-	labelSelector labels.Selector
-	claimSelector labels.Selector
-}
-
 func New(handle plugins.Handle) *Predicate {
 	return &Predicate{}
 }
@@ -53,10 +46,9 @@ func (p *Predicate) Filter(
 	}
 
 	// prebuild label/claim selectors for each predicate
-	predicateSelectors := []predicateSelector{}
+	clusterSelectors := []*helpers.ClusterSelector{}
 	for _, predicate := range placement.Spec.Predicates {
-		// build label selector
-		labelSelector, err := convertLabelSelector(predicate.RequiredClusterSelector.LabelSelector)
+		clusterSelector, err := helpers.NewClusterSelector(predicate.RequiredClusterSelector)
 		if err != nil {
 			return plugins.PluginFilterResult{}, framework.NewStatus(
 				p.Name(),
@@ -64,32 +56,15 @@ func (p *Predicate) Filter(
 				err.Error(),
 			)
 		}
-		// build claim selector
-		claimSelector, err := convertClaimSelector(predicate.RequiredClusterSelector.ClaimSelector)
-		if err != nil {
-			return plugins.PluginFilterResult{}, framework.NewStatus(
-				p.Name(),
-				framework.Misconfigured,
-				err.Error(),
-			)
-		}
-		predicateSelectors = append(predicateSelectors, predicateSelector{
-			labelSelector: labelSelector,
-			claimSelector: claimSelector,
-		})
+		clusterSelectors = append(clusterSelectors, clusterSelector)
 	}
 
 	// match cluster with selectors one by one
 	matched := []*clusterapiv1.ManagedCluster{}
 	for _, cluster := range clusters {
-		claims := getClusterClaims(cluster)
-		for _, ps := range predicateSelectors {
-			// match with label selector
-			if ok := ps.labelSelector.Matches(labels.Set(cluster.Labels)); !ok {
-				continue
-			}
-			// match with claim selector
-			if ok := ps.claimSelector.Matches(labels.Set(claims)); !ok {
+		claims := helpers.GetClusterClaims(cluster)
+		for _, cs := range clusterSelectors {
+			if ok := cs.Matches(cluster.Labels, claims); !ok {
 				continue
 			}
 			matched = append(matched, cluster)
@@ -105,34 +80,3 @@ func (p *Predicate) Filter(
 func (p *Predicate) RequeueAfter(ctx context.Context, placement *clusterapiv1beta1.Placement) (plugins.PluginRequeueResult, *framework.Status) {
 	return plugins.PluginRequeueResult{}, framework.NewStatus(p.Name(), framework.Success, "")
 }
-
-// getClusterClaims returns a map containing cluster claims from the status of cluster
-func getClusterClaims(cluster *clusterapiv1.ManagedCluster) map[string]string {
-	claims := map[string]string{}
-	for _, claim := range cluster.Status.ClusterClaims {
-		claims[claim.Name] = claim.Value
-	}
-	return claims
-}
-
-// convertLabelSelector converts metav1.LabelSelector to labels.Selector
-func convertLabelSelector(labelSelector metav1.LabelSelector) (labels.Selector, error) {
-	selector, err := metav1.LabelSelectorAsSelector(&labelSelector)
-	if err != nil {
-		return labels.Nothing(), err
-	}
-
-	return selector, nil
-}
-
-// convertClaimSelector converts ClusterClaimSelector to labels.Selector
-func convertClaimSelector(clusterClaimSelector clusterapiv1beta1.ClusterClaimSelector) (labels.Selector, error) {
-	selector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{
-		MatchExpressions: clusterClaimSelector.MatchExpressions,
-	})
-	if err != nil {
-		return labels.Nothing(), err
-	}
-
-	return selector, nil
-}