feat: v1beta1 CRP Validation for PickFixed Placement Policy (#551)

pkg/utils/validator/clusterresourceplacement.go

@@ -72,6 +72,7 @@ func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1
 	if len(clusterResourcePlacement.Name) > validation.DNS1035LabelMaxLength {
 		allErr = append(allErr, fmt.Errorf("the name field cannot have length exceeding %d", validation.DNS1035LabelMaxLength))
 	}
+
 	for _, selector := range clusterResourcePlacement.Spec.ResourceSelectors {
 		//TODO: make sure the selector's gvk is valid
 		if selector.LabelSelector != nil {
@@ -84,28 +85,84 @@ func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1
 		}
 	}
 
+	if clusterResourcePlacement.Spec.Policy != nil {
+		if err := validatePlacementPolicy(clusterResourcePlacement.Spec.Policy); err != nil {
+			allErr = append(allErr, fmt.Errorf("the placement policy field is invalid: %w", err))
+		}
+	}
+
 	if clusterResourcePlacement.Spec.Policy != nil && clusterResourcePlacement.Spec.Policy.Affinity != nil &&
 		clusterResourcePlacement.Spec.Policy.Affinity.ClusterAffinity != nil {
 		if err := validateClusterAffinity(clusterResourcePlacement.Spec.Policy.Affinity.ClusterAffinity); err != nil {
 			allErr = append(allErr, fmt.Errorf("the clusterAffinity field is invalid: %w", err))
 		}
 	}
-
 	if err := validateRolloutStrategy(clusterResourcePlacement.Spec.Strategy); err != nil {
 		allErr = append(allErr, fmt.Errorf("the rollout Strategy field  is invalid: %w", err))
 	}
 
 	return apiErrors.NewAggregate(allErr)
 }
 
+func validatePlacementPolicy(policy *placementv1beta1.PlacementPolicy) error {
+	allErr := make([]error, 0)
+	switch policy.PlacementType {
+	case placementv1beta1.PickFixedPlacementType:
+		if err := validatePolicyForPickFixedPlacementType(policy); err != nil {
+			allErr = append(allErr, err)
+		}
+	case placementv1beta1.PickAllPlacementType:
+		if err := validatePolicyForPickAllPlacementType(policy); err != nil {
+			allErr = append(allErr, err)
+		}
+	case placementv1beta1.PickNPlacementType:
+		if err := validatePolicyForPickNPolicyType(policy); err != nil {
+			allErr = append(allErr, err)
+		}
+	}
+
+	return apiErrors.NewAggregate(allErr)
+}
+
+func validatePolicyForPickFixedPlacementType(policy *placementv1beta1.PlacementPolicy) error {
+	allErr := make([]error, 0)
+	if len(policy.ClusterNames) == 0 {
+		allErr = append(allErr, fmt.Errorf("cluster names cannot be empty for policy type %s", placementv1beta1.PickFixedPlacementType))
+	}
+	if policy.NumberOfClusters != nil {
+		allErr = append(allErr, fmt.Errorf("number of clusters must be nil for policy type %s, only valid for PickN placement policy type", placementv1beta1.PickFixedPlacementType))
+	}
+	if policy.Affinity != nil {
+		allErr = append(allErr, fmt.Errorf("affinity must be nil for policy type %s, only valid for PickAll/PickN placement poliy types", placementv1beta1.PickFixedPlacementType))
+	}
+	if len(policy.TopologySpreadConstraints) > 0 {
+		allErr = append(allErr, fmt.Errorf("topology spread constraints needs to be empty for policy type %s, only valid for PickN policy type", placementv1beta1.PickFixedPlacementType))
+	}
+
+	return apiErrors.NewAggregate(allErr)
+}
+
+func validatePolicyForPickAllPlacementType(_ *placementv1beta1.PlacementPolicy) error {
+	// TODO(Arvindthiru): implement this.
+	allErr := make([]error, 0)
+	return apiErrors.NewAggregate(allErr)
+}
+
+func validatePolicyForPickNPolicyType(_ *placementv1beta1.PlacementPolicy) error {
+	// TODO(Arvindthiru): implement this.
+	allErr := make([]error, 0)
+	return apiErrors.NewAggregate(allErr)
+}
+
 func validateClusterAffinity(_ *placementv1beta1.ClusterAffinity) error {
 	// TODO: implement this
 	return nil
 }
 
 func validateRolloutStrategy(rolloutStrategy placementv1beta1.RolloutStrategy) error {
 	allErr := make([]error, 0)
-	if rolloutStrategy.Type != placementv1beta1.RollingUpdateRolloutStrategyType {
+
+	if rolloutStrategy.Type != "" && rolloutStrategy.Type != placementv1beta1.RollingUpdateRolloutStrategyType {
 		allErr = append(allErr, fmt.Errorf("unsupported rollout strategy type `%s`", rolloutStrategy.Type))
 	}
 

pkg/utils/validator/clusterresourceplacement_test.go

@@ -16,28 +16,6 @@ import (
 	"go.goms.io/fleet/pkg/utils/informer"
 )
 
-func Test_validateRolloutStrategy(t *testing.T) {
-	tests := map[string]struct {
-		rolloutStrategy placementv1beta1.RolloutStrategy
-		wantErr         bool
-	}{
-		// TODO: Add test cases.
-		"invalid RolloutStrategyType should fail": {
-			rolloutStrategy: placementv1beta1.RolloutStrategy{
-				Type: "random type",
-			},
-			wantErr: true,
-		},
-	}
-	for name, tt := range tests {
-		t.Run(name, func(t *testing.T) {
-			if err := validateRolloutStrategy(tt.rolloutStrategy); (err != nil) != tt.wantErr {
-				t.Errorf("validateRolloutStrategy() error = %v, wantErr %v", err, tt.wantErr)
-			}
-		})
-	}
-}
-
 func Test_validateClusterResourcePlacementAlpha(t *testing.T) {
 	tests := map[string]struct {
 		crp              *fleetv1alpha1.ClusterResourcePlacement
@@ -196,6 +174,7 @@ func Test_validateClusterResourcePlacementAlpha(t *testing.T) {
 
 func Test_validateClusterResourcePlacement(t *testing.T) {
 	unavailablePeriodSeconds := -10
+	var numberOfClusters int32 = 1
 	tests := map[string]struct {
 		crp              *placementv1beta1.ClusterResourcePlacement
 		resourceInformer informer.Manager
@@ -283,6 +262,27 @@ func Test_validateClusterResourcePlacement(t *testing.T) {
 					},
 				},
 			},
+			wantErr: false,
+		},
+		"invalid rollout strategy": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Strategy: placementv1beta1.RolloutStrategy{
+						Type: "random type",
+					},
+				},
+			},
 			wantErr: true,
 		},
 		"invalid rollout strategy - UnavailablePeriodSeconds": {
@@ -417,8 +417,122 @@ func Test_validateClusterResourcePlacement(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		"invalid placement policy - PickFixed with empty cluster names": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+					},
+					Strategy: placementv1beta1.RolloutStrategy{
+						Type: placementv1beta1.RollingUpdateRolloutStrategyType,
+						RollingUpdate: &placementv1beta1.RollingUpdateConfig{
+							MaxUnavailable: &intstr.IntOrString{
+								Type:   0,
+								IntVal: 10,
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		"invalid placement policy - PickFixed with non nil number of clusters": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType:    placementv1beta1.PickFixedPlacementType,
+						ClusterNames:     []string{"test-cluster"},
+						NumberOfClusters: &numberOfClusters,
+					},
+				},
+			},
+			wantErr: true,
+		},
+		"invalid placement policy - PickFixed with non nil affinity": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+						ClusterNames:  []string{"test-cluster"},
+						Affinity: &placementv1beta1.Affinity{
+							ClusterAffinity: &placementv1beta1.ClusterAffinity{
+								RequiredDuringSchedulingIgnoredDuringExecution: &placementv1beta1.ClusterSelector{
+									ClusterSelectorTerms: []placementv1beta1.ClusterSelectorTerm{
+										{
+											LabelSelector: metav1.LabelSelector{
+												MatchLabels: map[string]string{"test-key": "test-value"},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		"invalid placement policy - PickFixed with non empty topology constraints": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+						ClusterNames:  []string{"test-cluster"},
+						TopologySpreadConstraints: []placementv1beta1.TopologySpreadConstraint{
+							{
+								TopologyKey: "test-key",
+							},
+						},
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
-
 	for testName, testCase := range tests {
 		t.Run(testName, func(t *testing.T) {
 			ResourceInformer = testCase.resourceInformer

pkg/webhook/clusterresourceplacement/v1alpha1_clusterresourceplacement_validating_webhook.go

@@ -37,6 +37,7 @@ func AddV1Alpha1(mgr manager.Manager, _ []string) error {
 func (v *v1alpha1ClusterResourcePlacementValidator) Handle(_ context.Context, req admission.Request) admission.Response {
 	var crp fleetv1alpha1.ClusterResourcePlacement
 	if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update {
+		klog.V(2).InfoS("handling CRP", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name})
 		if err := v.decoder.Decode(req, &crp); err != nil {
 			klog.ErrorS(err, "failed to decode v1alpha1 CRP object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
 			return admission.Errored(http.StatusBadRequest, err)

pkg/webhook/clusterresourceplacement/v1beta1_clusterresourceplacement_validating_webhook.go

@@ -37,6 +37,7 @@ func Add(mgr manager.Manager, _ []string) error {
 func (v *clusterResourcePlacementValidator) Handle(_ context.Context, req admission.Request) admission.Response {
 	var crp placementv1beta1.ClusterResourcePlacement
 	if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update {
+		klog.V(2).InfoS("handling CRP", "operation", req.Operation, "namespacedName", types.NamespacedName{Name: req.Name})
 		if err := v.decoder.Decode(req, &crp); err != nil {
 			klog.ErrorS(err, "failed to decode v1beta1 CRP object for create/update operation", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
 			return admission.Errored(http.StatusBadRequest, err)

test/e2e/utils_test.go

@@ -161,12 +161,12 @@ func cleanupCRP(name string) {
 		if errors.IsNotFound(err) {
 			return nil
 		}
-		Expect(err).Should(Succeed(), "Failed to get CRP %s", name)
+		g.Expect(err).Should(Succeed(), "Failed to get CRP %s", name)
 
 		// Delete the CRP (again, if applicable).
 		//
-		// This helps the AfterAll node to run successfully even if the steps above fail early.
-		Expect(hubClient.Delete(ctx, crp)).To(Succeed(), "Failed to delete CRP %s", name)
+		// This helps the After All node to run successfully even if the steps above fail early.
+		g.Expect(hubClient.Delete(ctx, crp)).To(Succeed(), "Failed to delete CRP %s", name)
 
 		crp.Finalizers = []string{}
 		return hubClient.Update(ctx, crp)

test/e2e/webhook_test.go

@@ -82,7 +82,28 @@ var _ = Describe("webhook tests for CRP UPDATE operations", Ordered, func() {
 			}
 			var statusErr *k8sErrors.StatusError
 			g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update CRP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
-			Expect(string(statusErr.Status().Reason)).Should(Equal(fmt.Sprintf("the labelSelector and name fields are mutually exclusive in selector %+v", selector[0])))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("the labelSelector and name fields are mutually exclusive"))
+			return nil
+		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
+	})
+
+	It("should deny update on CRP with invalid placement policy", func() {
+		Eventually(func(g Gomega) error {
+			var numOfClusters int32 = 1
+			var crp placementv1beta1.ClusterResourcePlacement
+			g.Expect(hubClient.Get(ctx, types.NamespacedName{Name: crpName}, &crp)).Should(Succeed())
+			crp.Spec.Policy = &placementv1beta1.PlacementPolicy{
+				PlacementType:    placementv1beta1.PickFixedPlacementType,
+				NumberOfClusters: &numOfClusters,
+			}
+			err := hubClient.Update(ctx, &crp)
+			if k8sErrors.IsConflict(err) {
+				return err
+			}
+			var statusErr *k8sErrors.StatusError
+			g.Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update CRP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("cluster names cannot be empty for policy type"))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("number of clusters must be nil for policy type PickFixed"))
 			return nil
 		}, testutils.PollTimeout, testutils.PollInterval).Should(Succeed())
 	})