fix: add CEL to block user from changing External rollout strategy type to others

apis/placement/v1beta1/clusterresourceplacement_types.go

@@ -524,9 +524,9 @@ const (
 type RolloutStrategy struct {
 	// Type of rollout. The only supported types are "RollingUpdate" and "External".
 	// Default is "RollingUpdate".
-	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=RollingUpdate
 	// +kubebuilder:validation:Enum=RollingUpdate;External
+	// +kubebuilder:validation:XValidation:rule="!(self != 'External' && oldSelf == 'External')",message="cannot change rollout strategy type from 'External' to other types"
 	Type RolloutStrategyType `json:"type,omitempty"`
 
 	// Rolling update config params. Present only if RolloutStrategyType = RollingUpdate.

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

@@ -2482,6 +2482,10 @@ spec:
                     - RollingUpdate
                     - External
                     type: string
+                    x-kubernetes-validations:
+                    - message: cannot change rollout strategy type from 'External'
+                        to other types
+                      rule: '!(self != ''External'' && oldSelf == ''External'')'
                 type: object
             required:
             - resourceSelectors

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

@@ -992,6 +992,10 @@ spec:
                     - RollingUpdate
                     - External
                     type: string
+                    x-kubernetes-validations:
+                    - message: cannot change rollout strategy type from 'External'
+                        to other types
+                      rule: '!(self != ''External'' && oldSelf == ''External'')'
                 type: object
             required:
             - resourceSelectors

pkg/controllers/rollout/controller_integration_test.go

@@ -1013,76 +1013,6 @@ var _ = Describe("Test the rollout Controller", func() {
 		}
 	})
 
-	It("Should rollout all the selected bindings when strategy type is changed from External to RollingUpdate", func() {
-		By("Creating CRP with External strategy")
-		var targetCluster int32 = 10
-		rolloutCRP = clusterResourcePlacementForTest(testCRPName,
-			createPlacementPolicyForTest(placementv1beta1.PickNPlacementType, targetCluster),
-			createPlacementRolloutStrategyForTest(placementv1beta1.ExternalRolloutStrategyType, nil, nil))
-		Expect(k8sClient.Create(ctx, rolloutCRP)).Should(Succeed())
-
-		By("Creating the latest master resource snapshot")
-		masterSnapshot := generateClusterResourceSnapshot(rolloutCRP.Name, 0, true)
-		Expect(k8sClient.Create(ctx, masterSnapshot)).Should(Succeed())
-		By(fmt.Sprintf("master resource snapshot %s created", masterSnapshot.Name))
-
-		By("Creating scheduled bindings for master snapshot on target clusters")
-		clusters := make([]string, targetCluster)
-		for i := 0; i < int(targetCluster); i++ {
-			clusters[i] = "cluster-" + utils.RandStr()
-			binding := generateClusterResourceBinding(placementv1beta1.BindingStateScheduled, masterSnapshot.Name, clusters[i])
-			Expect(k8sClient.Create(ctx, binding)).Should(Succeed())
-			By(fmt.Sprintf("resource binding %s created", binding.Name))
-			bindings = append(bindings, binding)
-		}
-
-		By("Checking bindings are not rolled out consistently")
-		verifyBindingsNotRolledOutConsistently(controller.ConvertCRBArrayToBindingObjs(bindings))
-
-		By("Updating CRP rollout strategy type to RollingUpdate")
-		rolloutCRP.Spec.Strategy.Type = placementv1beta1.RollingUpdateRolloutStrategyType
-		rolloutCRP.Spec.Strategy.RollingUpdate = generateDefaultRollingUpdateConfig()
-		Expect(k8sClient.Update(ctx, rolloutCRP)).Should(Succeed(), "Failed to update CRP")
-
-		By("Verifying that rollout is unblocked")
-		verifyBindingsRolledOut(controller.ConvertCRBArrayToBindingObjs(bindings), masterSnapshot, timeout)
-	})
-
-	It("Should rollout all the selected bindings when strategy type is changed from External to empty", func() {
-		By("Creating CRP with External strategy")
-		var targetCluster int32 = 10
-		rolloutCRP = clusterResourcePlacementForTest(testCRPName,
-			createPlacementPolicyForTest(placementv1beta1.PickNPlacementType, targetCluster),
-			createPlacementRolloutStrategyForTest(placementv1beta1.ExternalRolloutStrategyType, nil, nil))
-		Expect(k8sClient.Create(ctx, rolloutCRP)).Should(Succeed())
-
-		By("Creating the latest master resource snapshot")
-		masterSnapshot := generateClusterResourceSnapshot(rolloutCRP.Name, 0, true)
-		Expect(k8sClient.Create(ctx, masterSnapshot)).Should(Succeed())
-		By(fmt.Sprintf("master resource snapshot %s created", masterSnapshot.Name))
-
-		By("Creating scheduled bindings for master snapshot on target clusters")
-		clusters := make([]string, targetCluster)
-		for i := 0; i < int(targetCluster); i++ {
-			clusters[i] = "cluster-" + utils.RandStr()
-			binding := generateClusterResourceBinding(placementv1beta1.BindingStateScheduled, masterSnapshot.Name, clusters[i])
-			Expect(k8sClient.Create(ctx, binding)).Should(Succeed())
-			By(fmt.Sprintf("resource binding %s created", binding.Name))
-			bindings = append(bindings, binding)
-		}
-
-		By("Checking bindings are not rolled out consistently")
-		verifyBindingsNotRolledOutConsistently(controller.ConvertCRBArrayToBindingObjs(bindings))
-
-		By("Updating CRP rollout strategy type to empty")
-		rolloutCRP.Spec.Strategy.Type = ""
-		rolloutCRP.Spec.Strategy.RollingUpdate = nil
-		Expect(k8sClient.Update(ctx, rolloutCRP)).Should(Succeed(), "Failed to update CRP")
-
-		By("Verifying that rollout is unblocked")
-		verifyBindingsRolledOut(controller.ConvertCRBArrayToBindingObjs(bindings), masterSnapshot, timeout)
-	})
-
 	It("Should not rollout anymore if the rollout strategy type is changed from RollingUpdate to External", func() {
 		By("Creating CRP with RollingUpdate strategy")
 		var targetCluster int32 = 10

pkg/controllers/updaterun/validation_integration_test.go

@@ -178,9 +178,12 @@ var _ = Describe("UpdateRun validation tests", func() {
 			})
 
 			It("Should fail to validate if CRP does not have external rollout strategy type", func() {
-				By("Updating CRP's rollout strategy type")
+				// Re-create the CRP with different strategy type as we cannot update the strategy type directly from `External` to others.
+				By("Re-creating the CRP with rolling update rollout strategy type")
+				Expect(k8sClient.Delete(ctx, crp)).To(Succeed())
+				crp = generateTestClusterResourcePlacement()
 				crp.Spec.Strategy.Type = placementv1beta1.RollingUpdateRolloutStrategyType
-				Expect(k8sClient.Update(ctx, crp)).To(Succeed())
+				Expect(k8sClient.Create(ctx, crp)).To(Succeed())
 
 				By("Validating the validation failed")
 				wantStatus = generateFailedValidationStatus(updateRun, wantStatus)

test/apis/placement/v1beta1/api_validation_integration_test.go

@@ -142,6 +142,9 @@ var _ = Describe("Test placement v1beta1 API validation", func() {
 						PlacementType: placementv1beta1.PickFixedPlacementType,
 						ClusterNames:  []string{"cluster1", "cluster2"},
 					},
+					Strategy: placementv1beta1.RolloutStrategy{
+						Type: placementv1beta1.ExternalRolloutStrategyType,
+					},
 				},
 			}
 			Expect(hubClient.Create(ctx, &crp)).Should(Succeed())
@@ -166,6 +169,14 @@ var _ = Describe("Test placement v1beta1 API validation", func() {
 			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"))
 		})
+
+		It("should deny update of RolloutStrategy type when External", func() {
+			crp.Spec.Strategy.Type = placementv1beta1.RollingUpdateRolloutStrategyType
+			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("cannot change rollout strategy type from 'External' to other types"))
+		})
 	})
 
 	Context("Test ClusterResourcePlacement StatusReportingScope validation - create, allow cases", func() {

test/e2e/cluster_staged_updaterun_test.go

@@ -1366,128 +1366,6 @@ var _ = Describe("test CRP rollout with staged update run", func() {
 		})
 	})
 
-	Context("Test CRP rollout strategy transition from external to rollingUpdate", Ordered, func() {
-		var strategy *placementv1beta1.ClusterStagedUpdateStrategy
-		updateRunName := fmt.Sprintf(clusterStagedUpdateRunNameWithSubIndexTemplate, GinkgoParallelProcess(), 0)
-		var oldConfigMap, newConfigMap corev1.ConfigMap
-
-		BeforeAll(func() {
-			// Create a test namespace and a configMap inside it on the hub cluster.
-			createWorkResources()
-
-			// Create the CRP with external rollout strategy initially.
-			crp := &placementv1beta1.ClusterResourcePlacement{
-				ObjectMeta: metav1.ObjectMeta{
-					Name: crpName,
-					// Add a custom finalizer; this would allow us to better observe
-					// the behavior of the controllers.
-					Finalizers: []string{customDeletionBlockerFinalizer},
-				},
-				Spec: placementv1beta1.PlacementSpec{
-					ResourceSelectors: workResourceSelector(),
-					Strategy: placementv1beta1.RolloutStrategy{
-						Type: placementv1beta1.ExternalRolloutStrategyType,
-					},
-				},
-			}
-			Expect(hubClient.Create(ctx, crp)).To(Succeed(), "Failed to create CRP")
-
-			// Create the clusterStagedUpdateStrategy.
-			strategy = createClusterStagedUpdateStrategySucceed(strategyName)
-
-			oldConfigMap = appConfigMap()
-			newConfigMap = appConfigMap()
-			newConfigMap.Data["data"] = testConfigMapDataValue
-		})
-
-		AfterAll(func() {
-			// Remove the custom deletion blocker finalizer from the CRP.
-			ensureCRPAndRelatedResourcesDeleted(crpName, allMemberClusters)
-
-			// Delete the clusterStagedUpdateRun.
-			ensureClusterStagedUpdateRunDeletion(updateRunName)
-
-			// Delete the clusterStagedUpdateStrategy.
-			ensureClusterUpdateRunStrategyDeletion(strategyName)
-		})
-
-		It("Should not rollout any resources to member clusters with external strategy", checkIfRemovedWorkResourcesFromAllMemberClustersConsistently)
-
-		It("Should have the latest resource snapshot", func() {
-			validateLatestClusterResourceSnapshot(crpName, resourceSnapshotIndex1st)
-		})
-
-		It("Should update crp status as pending rollout", func() {
-			crpStatusUpdatedActual := crpStatusWithExternalStrategyActual(nil, "", false, allMemberClusterNames, []string{"", "", ""}, []bool{false, false, false}, nil, nil)
-			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
-		})
-
-		It("Create updateRun and verify resources are rolled out", func() {
-			createClusterStagedUpdateRunSucceed(updateRunName, crpName, resourceSnapshotIndex1st, strategyName, placementv1beta1.StateRun)
-
-			// Approval for AfterStageTasks of canary stage
-			validateAndApproveClusterApprovalRequests(updateRunName, envCanary, placementv1beta1.AfterStageApprovalTaskNameFmt, placementv1beta1.AfterStageTaskLabelValue)
-
-			// Approval for BeforeStageTasks of prod stage
-			validateAndApproveClusterApprovalRequests(updateRunName, envProd, placementv1beta1.BeforeStageApprovalTaskNameFmt, placementv1beta1.BeforeStageTaskLabelValue)
-
-			csurSucceededActual := clusterStagedUpdateRunStatusSucceededActual(updateRunName, resourceSnapshotIndex1st, policySnapshotIndex1st, len(allMemberClusters), defaultApplyStrategy, &strategy.Spec, [][]string{{allMemberClusterNames[1]}, {allMemberClusterNames[0], allMemberClusterNames[2]}}, nil, nil, nil, true)
-			Eventually(csurSucceededActual, updateRunEventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to validate updateRun %s succeeded", updateRunName)
-
-			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun(allMemberClusters)
-		})
-
-		It("Should update crp status as completed", func() {
-			crpStatusUpdatedActual := crpStatusWithExternalStrategyActual(workResourceIdentifiers(), resourceSnapshotIndex1st, true, allMemberClusterNames,
-				[]string{resourceSnapshotIndex1st, resourceSnapshotIndex1st, resourceSnapshotIndex1st}, []bool{true, true, true}, nil, nil)
-			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status as expected", crpName)
-		})
-
-		It("Update the configmap on hub but should not rollout to member clusters with external strategy", func() {
-			updateConfigMapSucceed(&newConfigMap)
-
-			// Verify old configmap is still on all member clusters
-			for _, cluster := range allMemberClusters {
-				configMapActual := configMapPlacedOnClusterActual(cluster, &oldConfigMap)
-				Consistently(configMapActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to keep old configmap %s data on cluster %s", oldConfigMap.Name, cluster.ClusterName)
-			}
-		})
-
-		It("Should have new resource snapshot but CRP status should remain completed with old snapshot", func() {
-			validateLatestClusterResourceSnapshot(crpName, resourceSnapshotIndex2nd)
-
-			// CRP status should still show completed with old snapshot
-			crpStatusUpdatedActual := crpStatusWithExternalStrategyActual(workResourceIdentifiers(), resourceSnapshotIndex1st, true, allMemberClusterNames,
-				[]string{resourceSnapshotIndex1st, resourceSnapshotIndex1st, resourceSnapshotIndex1st}, []bool{true, true, true}, nil, nil)
-			Consistently(crpStatusUpdatedActual, consistentlyDuration, consistentlyInterval).Should(Succeed(), "Failed to keep CRP %s status as expected", crpName)
-		})
-
-		It("Update CRP to use rollingUpdate strategy", func() {
-			Eventually(func() error {
-				crp := &placementv1beta1.ClusterResourcePlacement{}
-				if err := hubClient.Get(ctx, client.ObjectKey{Name: crpName}, crp); err != nil {
-					return fmt.Errorf("failed to get the crp: %w", err)
-				}
-				crp.Spec.Strategy = placementv1beta1.RolloutStrategy{
-					Type: placementv1beta1.RollingUpdateRolloutStrategyType,
-				}
-				return hubClient.Update(ctx, crp)
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP strategy to rollingUpdate")
-		})
-
-		It("Should automatically rollout new resources to all member clusters with rollingUpdate strategy", func() {
-			// Verify CRP status shows all clusters with new resource snapshot
-			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, nil, resourceSnapshotIndex2nd)
-			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP %s status with rollingUpdate strategy", crpName)
-
-			// Verify new configmap is on all member clusters
-			for _, cluster := range allMemberClusters {
-				configMapActual := configMapPlacedOnClusterActual(cluster, &newConfigMap)
-				Eventually(configMapActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update to the new configmap %s on cluster %s", newConfigMap.Name, cluster.ClusterName)
-			}
-		})
-	})
-
 	Context("Test parallel cluster updates with maxConcurrency set to 3", Ordered, func() {
 		var strategy *placementv1beta1.ClusterStagedUpdateStrategy
 		updateRunName := fmt.Sprintf(clusterStagedUpdateRunNameWithSubIndexTemplate, GinkgoParallelProcess(), 0)
@@ -2075,39 +1953,6 @@ var _ = Describe("Test member cluster join and leave flow with updateRun", Label
 			Eventually(bindingBoundActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to mark binding for member cluster %s as bound", allMemberClusterNames[0])
 		})
 	})
-
-	Context("Rejoin a member cluster and change to rollout CRP with rollingUpdate", Label("joinleave"), Ordered, Serial, func() {
-		It("Should be able to rejoin member cluster 1", func() {
-			setMemberClusterToJoin(allMemberClusters[0])
-			checkIfMemberClusterHasJoined(allMemberClusters[0])
-		})
-
-		It("Should update the CRP rollout strategy to use rollingUpdate", func() {
-			Eventually(func() error {
-				var crp placementv1beta1.ClusterResourcePlacement
-				if err := hubClient.Get(ctx, client.ObjectKey{Name: crpName}, &crp); err != nil {
-					return fmt.Errorf("failed to get CRP %s: %w", crpName, err)
-				}
-				crp.Spec.Strategy = placementv1beta1.RolloutStrategy{
-					Type: placementv1beta1.RollingUpdateRolloutStrategyType,
-				}
-				return hubClient.Update(ctx, &crp)
-			}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP rollout strategy to rolling update")
-		})
-
-		It("Should verify resources are placed to member cluster 1 and binding status becomes bound", func() {
-			// Verify CRP status shows all clusters as bounded with rolling update.
-			crpStatusUpdatedActual := crpStatusUpdatedActual(workResourceIdentifiers(), allMemberClusterNames, nil, resourceSnapshotIndex1st)
-			Eventually(crpStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update CRP status as expected with rolling update")
-
-			// Verify resources are placed on all member clusters.
-			checkIfPlacedWorkResourcesOnMemberClustersInUpdateRun(allMemberClusters)
-
-			// Verify binding for member cluster 1 becomes bound.
-			bindingBoundActual := bindingStateActual(crpName, allMemberClusterNames[0], placementv1beta1.BindingStateBound)
-			Eventually(bindingBoundActual, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to mark binding for member cluster %s as bound with rolling update", allMemberClusterNames[0])
-		})
-	})
 })
 
 func createClusterStagedUpdateStrategySucceed(strategyName string) *placementv1beta1.ClusterStagedUpdateStrategy {