feat: simplify selection count check in NamespaceWithResourceSelectors selection scope (#488)

Author: weng271190436

pkg/utils/controller/resource_selector_resolver.go

@@ -248,22 +248,18 @@ func (rs *ResourceSelectorResolver) gatherSelectedResource(placementKey types.Na
 			Kind:    selector.Kind,
 		}
 		if gvk == utils.NamespaceGVK && placementKey.Namespace == "" && selector.SelectionScope == placementv1beta1.NamespaceWithResourceSelectors {
-			namespaces, err := rs.fetchSelectedNamespaces(selector, placementKey.Name)
+			namespace, found, err := rs.fetchSelectedNamespace(selector, placementKey.Name)
 			if err != nil {
 				return nil, err
 			}
 			// NamespaceWithResourceSelectors mode requires exactly one namespace
-			if len(namespaces) == 0 {
+			if !found {
 				err := fmt.Errorf("invalid clusterResourcePlacement %s: NamespaceWithResourceSelectors mode requires exactly one namespace, but no namespaces were selected", placementKey.Name)
 				klog.ErrorS(err, "No namespaces selected with NamespaceWithResourceSelectors mode", "placement", placementKey.Name)
 				return nil, NewUserError(err)
 			}
-			if len(namespaces) > 1 {
-				err := fmt.Errorf("invalid clusterResourcePlacement %s: NamespaceWithResourceSelectors mode requires exactly one namespace, but %d namespaces were selected", placementKey.Name, len(namespaces))
-				klog.ErrorS(err, "Multiple namespaces selected with NamespaceWithResourceSelectors mode", "placement", placementKey.Name, "namespaceCount", len(namespaces))
-				return nil, NewUserError(err)
-			}
-			selectedNamespace = namespaces[0]
+			// Note: CEL validation ensures that at most one namespace can be selected.
+			selectedNamespace = namespace
 			break
 		}
 	}
@@ -591,32 +587,37 @@ func (rs *ResourceSelectorResolver) fetchResources(selector placementv1beta1.Res
 	return selectedObjs, nil
 }
 
-// fetchSelectedNamespaces retrieves the namespace names based on the namespace selector.
-func (rs *ResourceSelectorResolver) fetchSelectedNamespaces(selector placementv1beta1.ResourceSelectorTerm, placementName string) ([]string, error) {
-	klog.V(2).InfoS("start to fetch selected namespaces", "selector", selector, "placement", placementName)
+// fetchSelectedNamespace retrieves the namespace name based on the namespace selector.
+// This function assumes the selector is a name-based selector (not label-based) as guaranteed by CEL validation.
+// Returns the namespace name if found and not skipped, or empty string if not found/skipped.
+func (rs *ResourceSelectorResolver) fetchSelectedNamespace(selector placementv1beta1.ResourceSelectorTerm, placementName string) (string, bool, error) {
+	klog.V(2).InfoS("start to fetch selected namespace", "selector", selector, "placement", placementName)
 
 	// Use fetchResources to get namespace objects (handles label selector, name selector, deletion timestamps)
 	placementKey := types.NamespacedName{Name: placementName}
 	objs, err := rs.fetchResources(selector, placementKey)
 	if err != nil {
-		return nil, err
+		return "", false, err
 	}
 
-	// Filter by skipped namespaces and extract names
-	namespaceNames := make([]string, 0, len(objs))
-	for _, obj := range objs {
-		ns, err := meta.Accessor(obj)
-		if err != nil {
-			return nil, NewUnexpectedBehaviorError(fmt.Errorf("failed to access the namespace object: %w", err))
-		}
-		if !utils.ShouldPropagateNamespace(ns.GetName(), rs.SkippedNamespaces) {
-			klog.V(2).InfoS("skip namespace that is not allowed to propagate", "namespace", ns.GetName(), "placement", placementName)
-			continue
-		}
-		namespaceNames = append(namespaceNames, ns.GetName())
+	// Since CEL validation ensures name-based selection only, we expect at most 1 namespace object
+	if len(objs) == 0 {
+		return "", false, nil
+	}
+
+	// We should have exactly 1 namespace object due to CEL validation constraints
+	ns, err := meta.Accessor(objs[0])
+	if err != nil {
+		return "", false, NewUnexpectedBehaviorError(fmt.Errorf("failed to access the namespace object: %w", err))
+	}
+
+	// Check if this namespace should be propagated
+	if !utils.ShouldPropagateNamespace(ns.GetName(), rs.SkippedNamespaces) {
+		klog.V(2).InfoS("skip namespace that is not allowed to propagate", "namespace", ns.GetName(), "placement", placementName)
+		return "", false, nil
 	}
 
-	return namespaceNames, nil
+	return ns.GetName(), true, nil
 }
 
 func (rs *ResourceSelectorResolver) ShouldPropagateObj(namespace, placementName string, obj runtime.Object) (bool, error) {

pkg/utils/controller/resource_selector_resolver_test.go

@@ -1493,56 +1493,6 @@ func TestGatherSelectedResource(t *testing.T) {
 			// Should select only the namespace, not its child resources
 			want: []*unstructured.Unstructured{testNamespace},
 		},
-		{
-			name:          "should error when selecting multiple namespaces with NamespaceWithResourceSelectors mode using label selector",
-			placementName: types.NamespacedName{Name: "test-placement"},
-			selectors: []fleetv1beta1.ResourceSelectorTerm{
-				{
-					Group:   "",
-					Version: "v1",
-					Kind:    "Namespace",
-					LabelSelector: &metav1.LabelSelector{
-						MatchLabels: map[string]string{
-							"environment": "test",
-						},
-					},
-					SelectionScope: fleetv1beta1.NamespaceWithResourceSelectors,
-				},
-				{
-					Group:   "apps",
-					Version: "v1",
-					Kind:    "Deployment",
-					Name:    "test-deployment",
-				},
-			},
-			resourceConfig: utils.NewResourceConfig(false), // default deny list
-			informerManager: func() *testinformer.FakeManager {
-				// Create a second test namespace with the same label
-				testNamespace2 := &unstructured.Unstructured{
-					Object: map[string]interface{}{
-						"apiVersion": "v1",
-						"kind":       "Namespace",
-						"metadata": map[string]interface{}{
-							"name": "test-ns-2",
-							"labels": map[string]interface{}{
-								"environment": "test",
-							},
-						},
-					},
-				}
-				testNamespace2.SetGroupVersionKind(utils.NamespaceGVK)
-
-				return &testinformer.FakeManager{
-					Listers: map[schema.GroupVersionResource]*testinformer.FakeLister{
-						utils.NamespaceGVR:  {Objects: []runtime.Object{testNamespace, testNamespace2}},
-						utils.DeploymentGVR: {Objects: []runtime.Object{testDeployment}},
-					},
-					NamespaceScopedResources: []schema.GroupVersionResource{utils.DeploymentGVR},
-				}
-			}(),
-			// Should error because multiple namespaces match the label selector
-			wantError: ErrUserError,
-		},
 		{
 			name:          "should error when selecting zero namespaces with NamespaceWithResourceSelectors mode",
 			placementName: types.NamespacedName{Name: "test-placement"},
@@ -2201,7 +2151,7 @@ func TestSortResources(t *testing.T) {
 	}
 }
 
-func TestFetchSelectedNamespaces(t *testing.T) {
+func TestFetchSelectedNamespace(t *testing.T) {
 	testNs1 := &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"apiVersion": "v1",
@@ -2246,7 +2196,8 @@ func TestFetchSelectedNamespaces(t *testing.T) {
 		selector          fleetv1beta1.ResourceSelectorTerm
 		skippedNamespaces map[string]bool
 		informerManager   *testinformer.FakeManager
-		want              []string
+		want              string
+		wantFound         bool
 		wantErr           bool
 	}{
 		{
@@ -2263,43 +2214,25 @@ func TestFetchSelectedNamespaces(t *testing.T) {
 					utils.NamespaceGVR: {Objects: []runtime.Object{testNs1, testNs2}},
 				},
 			},
-			want: []string{"test-ns-1"},
-		},
-		{
-			name: "select namespaces by label selector",
-			selector: fleetv1beta1.ResourceSelectorTerm{
-				Group:   "",
-				Version: "v1",
-				Kind:    "Namespace",
-				LabelSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{"env": "prod"},
-				},
-			},
-			skippedNamespaces: nil,
-			informerManager: &testinformer.FakeManager{
-				Listers: map[schema.GroupVersionResource]*testinformer.FakeLister{
-					utils.NamespaceGVR: {Objects: []runtime.Object{testNs1, testNs2}},
-				},
-			},
-			want: []string{"test-ns-1"},
+			want:      "test-ns-1",
+			wantFound: true,
 		},
 		{
 			name: "filter out skipped namespaces",
 			selector: fleetv1beta1.ResourceSelectorTerm{
 				Group:   "",
 				Version: "v1",
 				Kind:    "Namespace",
-				LabelSelector: &metav1.LabelSelector{
-					MatchLabels: map[string]string{},
-				},
+				Name:    "kube-system", // Use name-based selection instead of label selector
 			},
 			skippedNamespaces: map[string]bool{"kube-system": true},
 			informerManager: &testinformer.FakeManager{
 				Listers: map[schema.GroupVersionResource]*testinformer.FakeLister{
-					utils.NamespaceGVR: {Objects: []runtime.Object{testNs1, skippedNs}},
+					utils.NamespaceGVR: {Objects: []runtime.Object{testNs1, testNs2, skippedNs}},
 				},
 			},
-			want: []string{"test-ns-1"},
+			want:      "", // Empty because kube-system is skipped
+			wantFound: false,
 		},
 		{
 			name: "no namespaces match selector",
@@ -2315,7 +2248,8 @@ func TestFetchSelectedNamespaces(t *testing.T) {
 					utils.NamespaceGVR: {Objects: []runtime.Object{testNs1, testNs2}},
 				},
 			},
-			want: []string{},
+			want:      "",
+			wantFound: false,
 		},
 	}
 
@@ -2328,13 +2262,16 @@ func TestFetchSelectedNamespaces(t *testing.T) {
 				RestMapper:        newFakeRESTMapper(),
 			}
 
-			got, err := rsr.fetchSelectedNamespaces(tt.selector, "test-placement")
+			gotNamespace, gotFound, err := rsr.fetchSelectedNamespace(tt.selector, "test-placement")
 			if (err != nil) != tt.wantErr {
-				t.Errorf("fetchSelectedNamespaces() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("fetchSelectedNamespace() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
-			if diff := cmp.Diff(tt.want, got); diff != "" {
-				t.Errorf("fetchSelectedNamespaces() mismatch (-want +got):\n%s", diff)
+			if gotNamespace != tt.want {
+				t.Errorf("fetchSelectedNamespace() namespace = %v, want %v", gotNamespace, tt.want)
+			}
+			if gotFound != tt.wantFound {
+				t.Errorf("fetchSelectedNamespace() found = %v, wantFound %v", gotFound, tt.wantFound)
 			}
 		})
 	}