Merge branch 'main' into handleConfigMap

apis/placement/v1beta1/clusterresourceplacement_types.go

@@ -39,10 +39,10 @@ const (
 // +kubebuilder:resource:scope="Cluster",shortName=crp,categories={fleet,fleet-placement}
 // +kubebuilder:subresource:status
 // +kubebuilder:printcolumn:JSONPath=`.metadata.generation`,name="Gen",type=string
-// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="Scheduled")].status`,name="Scheduled",type=string
-// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="Scheduled")].observedGeneration`,name="ScheduledGen",type=string
-// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="Applied")].status`,name="Applied",type=string
-// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="Applied")].observedGeneration`,name="AppliedGen",type=string
+// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="ClusterResourcePlacementScheduled")].status`,name="Scheduled",type=string
+// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="ClusterResourcePlacementScheduled")].observedGeneration`,name="ScheduledGen",type=string
+// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="ClusterResourcePlacementApplied")].status`,name="Applied",type=string
+// +kubebuilder:printcolumn:JSONPath=`.status.conditions[?(@.type=="ClusterResourcePlacementApplied")].observedGeneration`,name="AppliedGen",type=string
 // +kubebuilder:printcolumn:JSONPath=`.metadata.creationTimestamp`,name="Age",type=date
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 

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

@@ -23,16 +23,16 @@ spec:
     - jsonPath: .metadata.generation
       name: Gen
       type: string
-    - jsonPath: .status.conditions[?(@.type=="Scheduled")].status
+    - jsonPath: .status.conditions[?(@.type=="ClusterResourcePlacementScheduled")].status
       name: Scheduled
       type: string
-    - jsonPath: .status.conditions[?(@.type=="Scheduled")].observedGeneration
+    - jsonPath: .status.conditions[?(@.type=="ClusterResourcePlacementScheduled")].observedGeneration
       name: ScheduledGen
       type: string
-    - jsonPath: .status.conditions[?(@.type=="Applied")].status
+    - jsonPath: .status.conditions[?(@.type=="ClusterResourcePlacementApplied")].status
       name: Applied
       type: string
-    - jsonPath: .status.conditions[?(@.type=="Applied")].observedGeneration
+    - jsonPath: .status.conditions[?(@.type=="ClusterResourcePlacementApplied")].observedGeneration
       name: AppliedGen
       type: string
     - jsonPath: .metadata.creationTimestamp

pkg/utils/validator/clusterresourceplacement.go

@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	apiErrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/validation"
 
 	placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
@@ -68,6 +69,10 @@ func ValidateClusterResourcePlacementAlpha(clusterResourcePlacement *fleetv1alph
 func ValidateClusterResourcePlacement(clusterResourcePlacement *placementv1beta1.ClusterResourcePlacement) error {
 	allErr := make([]error, 0)
 
+	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 {
@@ -80,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
@@ -222,7 +201,27 @@ func Test_validateClusterResourcePlacement(t *testing.T) {
 			},
 			wantErr: false,
 		},
-
+		"CRP with invalid name": {
+			crp: &placementv1beta1.ClusterResourcePlacement{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test-crp-with-very-long-name-field-exceeding-DNS1035LabelMaxLength",
+				},
+				Spec: placementv1beta1.ClusterResourcePlacementSpec{
+					ResourceSelectors: []placementv1beta1.ClusterResourceSelector{
+						{
+							Group:   "rbac.authorization.k8s.io",
+							Version: "v1",
+							Kind:    "ClusterRole",
+							Name:    "test-cluster-role",
+						},
+					},
+					Strategy: placementv1beta1.RolloutStrategy{
+						Type: placementv1beta1.RollingUpdateRolloutStrategyType,
+					},
+				},
+			},
+			wantErr: true,
+		},
 		"invalid Resource Selector with name & label selector": {
 			crp: &placementv1beta1.ClusterResourcePlacement{
 				ObjectMeta: metav1.ObjectMeta{
@@ -263,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": {
@@ -397,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)