feat: add CEL validation to make placement type immutable for RP (#315)

Author: weng271190436

apis/placement/v1beta1/clusterresourceplacement_types.go

@@ -113,7 +113,7 @@ type ClusterResourcePlacement struct {
 
 	// The desired state of ClusterResourcePlacement.
 	// +kubebuilder:validation:Required
-	// +kubebuilder:validation:XValidation:rule="!((has(oldSelf.policy) && !has(self.policy)) || (has(oldSelf.policy) && has(self.policy) && has(self.policy.placementType) && has(oldSelf.policy.placementType) && self.policy.placementType != oldSelf.policy.placementType))",message="placement type is immutable"
+	// +kubebuilder:validation:XValidation:rule="!(has(oldSelf.policy) && !has(self.policy))",message="policy cannot be removed once set"
 	// +kubebuilder:validation:XValidation:rule="!(self.statusReportingScope == 'NamespaceAccessible' && size(self.resourceSelectors.filter(x, x.kind == 'Namespace')) != 1)",message="when statusReportingScope is NamespaceAccessible, exactly one resourceSelector with kind 'Namespace' is required"
 	// +kubebuilder:validation:XValidation:rule="!has(oldSelf.statusReportingScope) || self.statusReportingScope == oldSelf.statusReportingScope",message="statusReportingScope is immutable"
 	Spec PlacementSpec `json:"spec"`
@@ -135,6 +135,7 @@ type PlacementSpec struct {
 	// Policy defines how to select member clusters to place the selected resources.
 	// If unspecified, all the joined member clusters are selected.
 	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:XValidation:rule="!(self.placementType != oldSelf.placementType)",message="placement type is immutable"
 	Policy *PlacementPolicy `json:"policy,omitempty"`
 
 	// The rollout strategy to use to replace existing placement with new ones.
@@ -1628,6 +1629,7 @@ type ResourcePlacement struct {
 
 	// The desired state of ResourcePlacement.
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:XValidation:rule="!(has(oldSelf.policy) && !has(self.policy))",message="policy cannot be removed once set"
 	Spec PlacementSpec `json:"spec"`
 
 	// The observed status of ResourcePlacement.

config/crd/bases/placement.kubernetes-fleet.io_clusterresourceplacements.yaml

@@ -1583,6 +1583,9 @@ spec:
                       type: object
                     type: array
                 type: object
+                x-kubernetes-validations:
+                - message: placement type is immutable
+                  rule: '!(self.placementType != oldSelf.placementType)'
               resourceSelectors:
                 description: |-
                   ResourceSelectors is an array of selectors used to select cluster scoped resources. The selectors are `ORed`.
@@ -2059,10 +2062,8 @@ spec:
             - resourceSelectors
             type: object
             x-kubernetes-validations:
-            - message: placement type is immutable
-              rule: '!((has(oldSelf.policy) && !has(self.policy)) || (has(oldSelf.policy)
-                && has(self.policy) && has(self.policy.placementType) && has(oldSelf.policy.placementType)
-                && self.policy.placementType != oldSelf.policy.placementType))'
+            - message: policy cannot be removed once set
+              rule: '!(has(oldSelf.policy) && !has(self.policy))'
             - message: when statusReportingScope is NamespaceAccessible, exactly one
                 resourceSelector with kind 'Namespace' is required
               rule: '!(self.statusReportingScope == ''NamespaceAccessible'' && size(self.resourceSelectors.filter(x,

config/crd/bases/placement.kubernetes-fleet.io_resourceplacements.yaml

@@ -518,6 +518,9 @@ spec:
                       type: object
                     type: array
                 type: object
+                x-kubernetes-validations:
+                - message: placement type is immutable
+                  rule: '!(self.placementType != oldSelf.placementType)'
               resourceSelectors:
                 description: |-
                   ResourceSelectors is an array of selectors used to select cluster scoped resources. The selectors are `ORed`.
@@ -993,6 +996,9 @@ spec:
             required:
             - resourceSelectors
             type: object
+            x-kubernetes-validations:
+            - message: policy cannot be removed once set
+              rule: '!(has(oldSelf.policy) && !has(self.policy))'
           status:
             description: The observed status of ResourcePlacement.
             properties:

test/apis/placement/v1beta1/api_validation_integration_test.go

@@ -156,7 +156,7 @@ var _ = Describe("Test placement v1beta1 API validation", func() {
 			err := hubClient.Update(ctx, &crp)
 			var statusErr *k8sErrors.StatusError
 			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("placement type is immutable"))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("policy cannot be removed once set"))
 		})
 
 		It("should deny update of ClusterResourcePlacement with different placement type", func() {
@@ -613,6 +613,55 @@ var _ = Describe("Test placement v1beta1 API validation", func() {
 		})
 	})
 
+	Context("Test ResourcePlacement API validation - invalid cases", func() {
+		var rp placementv1beta1.ResourcePlacement
+		rpName := fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess())
+
+		BeforeEach(func() {
+			rp = placementv1beta1.ResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      rpName,
+					Namespace: testNamespace,
+				},
+				Spec: placementv1beta1.PlacementSpec{
+					ResourceSelectors: []placementv1beta1.ResourceSelectorTerm{
+						{
+							Group:   "",
+							Version: "v1",
+							Kind:    "ConfigMap",
+							Name:    "test-cm",
+						},
+					},
+					Policy: &placementv1beta1.PlacementPolicy{
+						PlacementType: placementv1beta1.PickFixedPlacementType,
+						ClusterNames:  []string{"cluster1", "cluster2"},
+					},
+				},
+			}
+			Expect(hubClient.Create(ctx, &rp)).Should(Succeed())
+		})
+
+		AfterEach(func() {
+			Expect(hubClient.Delete(ctx, &rp)).Should(Succeed())
+		})
+
+		It("should deny update of ResourcePlacement with nil policy", func() {
+			rp.Spec.Policy = nil
+			err := hubClient.Update(ctx, &rp)
+			var statusErr *k8sErrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update RP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("policy cannot be removed once set"))
+		})
+
+		It("should deny update of ResourcePlacement with different placement type", func() {
+			rp.Spec.Policy.PlacementType = placementv1beta1.PickAllPlacementType
+			err := hubClient.Update(ctx, &rp)
+			var statusErr *k8sErrors.StatusError
+			Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Update RP call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{})))
+			Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("placement type is immutable"))
+		})
+	})
+
 	Context("Test ResourcePlacement StatusReportingScope validation, allow cases", func() {
 		var rp placementv1beta1.ResourcePlacement
 		rpName := fmt.Sprintf(rpNameTemplate, GinkgoParallelProcess())