fix: block cluster-scoped resources from being wrapped in an envelope (#32)

michaelawyu · web-flow · commit 8ab0edbac719 · 2025-04-27T17:33:00.000-07:00
diff --git a/pkg/controllers/workgenerator/controller.go b/pkg/controllers/workgenerator/controller.go
@@ -1280,8 +1280,11 @@ func extractResFromConfigMap(uConfigMap *unstructured.Unstructured) ([]fleetv1be
 			klog.ErrorS(unMarshallErr, "manifest has invalid content", "manifestKey", key, "envelopeResource", klog.KObj(uConfigMap))
 			return nil, fmt.Errorf("the object with manifest key `%s` in envelope config `%s` is malformatted, err: %w", key, klog.KObj(uConfigMap), unMarshallErr)
 		}
+		if len(uManifest.GetNamespace()) == 0 {
+			// Block cluster-scoped resources.
+			return nil, fmt.Errorf("cannot wrap cluster-scoped resource %s in the envelope %s", uManifest.GetName(), klog.KObj(uConfigMap))
+		}
 		if len(uManifest.GetNamespace()) != 0 && uManifest.GetNamespace() != configMap.Namespace {
-			// if the manifest is a namespaced object but no namespace is specified, it will fail at the apply time instead of here.
 			return nil, fmt.Errorf("the namespaced object `%s` in envelope config `%s` is placed in a different namespace `%s` ", uManifest.GetName(), klog.KObj(uConfigMap), uManifest.GetNamespace())
 		}
 		manifests = append(manifests, fleetv1beta1.Manifest{
diff --git a/pkg/controllers/workgenerator/controller_integration_test.go b/pkg/controllers/workgenerator/controller_integration_test.go
@@ -698,7 +698,6 @@ var _ = Describe("Test Work Generator Controller", func() {
 						Workload: placementv1beta1.WorkloadTemplate{
 							Manifests: []placementv1beta1.Manifest{
 								{RawExtension: runtime.RawExtension{Raw: testEnvelopeResourceQuota}},
-								{RawExtension: runtime.RawExtension{Raw: testEnvelopeWebhook}},
 							},
 						},
 					},
@@ -823,7 +822,7 @@ var _ = Describe("Test Work Generator Controller", func() {
 					Spec: placementv1beta1.WorkSpec{
 						Workload: placementv1beta1.WorkloadTemplate{
 							Manifests: []placementv1beta1.Manifest{
-								{RawExtension: runtime.RawExtension{Raw: testEnvelopeWebhook}},
+								{RawExtension: runtime.RawExtension{Raw: testEnvelopeResourceQuota2}},
 							},
 						},
 					},
diff --git a/pkg/controllers/workgenerator/controller_test.go b/pkg/controllers/workgenerator/controller_test.go
@@ -208,6 +208,23 @@ func TestExtractResFromConfigMap(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
+		"config map with cluster scoped resource should fail": {
+			uConfigMap: &unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"apiVersion": "v1",
+					"kind":       "ConfigMap",
+					"metadata": map[string]interface{}{
+						"name":      "test-config",
+						"namespace": "default",
+					},
+					"data": map[string]interface{}{
+						"resource": `{"apiVersion": "admissionregistration.k8s.io/v1", "kind": "ValidatingWebhookConfiguration", "metadata": {"name": "test-webhook"}}`,
+					},
+				},
+			},
+			want:    nil,
+			wantErr: true,
+		},
 		"config map with valid and invalid entries should fail": {
 			uConfigMap: &unstructured.Unstructured{
 				Object: map[string]interface{}{
@@ -226,7 +243,7 @@ func TestExtractResFromConfigMap(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 		},
-		"config map with cluster and namespace scoped data in the correct namespace should pass": {
+		"config map with cluster and namespace scoped data in the correct namespace should fail": {
 			uConfigMap: &unstructured.Unstructured{
 				Object: map[string]interface{}{
 					"apiVersion": "v1",
@@ -241,11 +258,8 @@ func TestExtractResFromConfigMap(t *testing.T) {
 					},
 				},
 			},
-			want: []fleetv1beta1.Manifest{
-				{RawExtension: runtime.RawExtension{Raw: []byte(`{"apiVersion": "v1", "kind": "Pod", "metadata": {"name": "test-pod", "namespace": "default"}}`)}},
-				{RawExtension: runtime.RawExtension{Raw: []byte(`{"apiVersion": "v1", "kind": "ClusterRole", "metadata": {"name": "test-role"}}`)}},
-			},
-			wantErr: false,
+			want:    nil,
+			wantErr: true,
 		},
 		"config map with cluster scoped and cross namespaced resources data in a different namespace should fail": {
 			uConfigMap: &unstructured.Unstructured{
diff --git a/pkg/controllers/workgenerator/manifests/resourcequota2.yaml b/pkg/controllers/workgenerator/manifests/resourcequota2.yaml
@@ -0,0 +1,11 @@
+apiVersion: v1
+kind: ResourceQuota
+metadata:
+  name: mem-cpu-demo
+  namespace: app
+spec:
+  hard:
+    requests.cpu: "2"
+    requests.memory: 2Gi
+    limits.cpu: "4"
+    limits.memory: 4Gi
diff --git a/pkg/controllers/workgenerator/manifests/test-envelop-configmap.yaml b/pkg/controllers/workgenerator/manifests/test-envelop-configmap.yaml
@@ -18,34 +18,3 @@ data:
         requests.memory: 1Gi
         limits.cpu: "2"
         limits.memory: 2Gi
-  webhook.yaml: |
-    apiVersion: admissionregistration.k8s.io/v1
-    kind: MutatingWebhookConfiguration
-    metadata:
-      creationTimestamp: null
-      labels:
-        azure-workload-identity.io/system: "true"
-      name: azure-wi-webhook-mutating-webhook-configuration
-    webhooks:
-    - admissionReviewVersions:
-      - v1
-      - v1beta1
-      clientConfig:
-        service:
-          name: azure-wi-webhook-webhook-service
-          namespace: app
-          path: /mutate-v1-pod
-      failurePolicy: Fail
-      matchPolicy: Equivalent
-      name: mutation.azure-workload-identity.io
-      rules:
-      - apiGroups:
-        - ""
-        apiVersions:
-        - v1
-        operations:
-        - CREATE
-        - UPDATE
-        resources:
-        - pods
-      sideEffects: None
diff --git a/pkg/controllers/workgenerator/manifests/test-envelop-configmap2.yaml b/pkg/controllers/workgenerator/manifests/test-envelop-configmap2.yaml
@@ -6,34 +6,15 @@ metadata:
   annotations:
     kubernetes-fleet.io/envelope-configmap: "true"
 data:
-  webhook.yaml: |
-    apiVersion: admissionregistration.k8s.io/v1
-    kind: MutatingWebhookConfiguration
+  resourceQuota.yaml: |
+    apiVersion: v1
+    kind: ResourceQuota
     metadata:
-      creationTimestamp: null
-      labels:
-        azure-workload-identity.io/system: "true"
-      name: azure-wi-webhook-mutating-webhook-configuration
-    webhooks:
-    - admissionReviewVersions:
-      - v1
-      - v1beta1
-      clientConfig:
-        service:
-          name: azure-wi-webhook-webhook-service
-          namespace: app
-          path: /mutate-v1-pod
-      failurePolicy: Fail
-      matchPolicy: Equivalent
-      name: mutation.azure-workload-identity.io
-      rules:
-      - apiGroups:
-        - ""
-        apiVersions:
-        - v1
-        operations:
-        - CREATE
-        - UPDATE
-        resources:
-        - pods
-      sideEffects: None
+      name: mem-cpu-demo
+      namespace: app
+    spec:
+      hard:
+        requests.cpu: "2"
+        requests.memory: 2Gi
+        limits.cpu: "4"
+        limits.memory: 4Gi
diff --git a/pkg/controllers/workgenerator/suite_test.go b/pkg/controllers/workgenerator/suite_test.go
@@ -65,7 +65,7 @@ var (
 	wantOverriddenTestResource []byte
 
 	// the content of the enveloped resources
-	testEnvelopeWebhook, testEnvelopeResourceQuota []byte
+	testEnvelopeResourceQuota, testEnvelopeResourceQuota2 []byte
 )
 
 func TestAPIs(t *testing.T) {
@@ -365,15 +365,15 @@ func readTestManifests() {
 	testPdb, err = yaml.ToJSON(rawByte)
 	Expect(err).Should(Succeed())
 
-	By("Read EnvelopeWebhook")
-	rawByte, err = os.ReadFile("manifests/webhook.yaml")
-	Expect(err).Should(Succeed())
-	testEnvelopeWebhook, err = yaml.ToJSON(rawByte)
-	Expect(err).Should(Succeed())
-
 	By("Read ResourceQuota")
 	rawByte, err = os.ReadFile("manifests/resourcequota.yaml")
 	Expect(err).Should(Succeed())
 	testEnvelopeResourceQuota, err = yaml.ToJSON(rawByte)
 	Expect(err).Should(Succeed())
+
+	By("Read ResourceQuota2")
+	rawByte, err = os.ReadFile("manifests/resourcequota2.yaml")
+	Expect(err).Should(Succeed())
+	testEnvelopeResourceQuota2, err = yaml.ToJSON(rawByte)
+	Expect(err).Should(Succeed())
 }
diff --git a/test/e2e/actuals_test.go b/test/e2e/actuals_test.go
@@ -613,7 +613,11 @@ func resourcePlacementOverrideFailedConditions(generation int64) []metav1.Condit
 	}
 }
 
-func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {
+func resourcePlacementWorkSynchronizedFailedConditions(generation int64, hasOverrides bool) []metav1.Condition {
+	overridenCondReason := condition.OverrideNotSpecifiedReason
+	if hasOverrides {
+		overridenCondReason = condition.OverriddenSucceededReason
+	}
 	return []metav1.Condition{
 		{
 			Type:               string(placementv1beta1.ResourceScheduledConditionType),
@@ -631,7 +635,7 @@ func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav
 			Type:               string(placementv1beta1.ResourceOverriddenConditionType),
 			Status:             metav1.ConditionTrue,
 			ObservedGeneration: generation,
-			Reason:             condition.OverriddenSucceededReason,
+			Reason:             overridenCondReason,
 		},
 		{
 			Type:               string(placementv1beta1.ResourceWorkSynchronizedConditionType),
@@ -642,7 +646,11 @@ func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav
 	}
 }
 
-func crpWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {
+func crpWorkSynchronizedFailedConditions(generation int64, hasOverrides bool) []metav1.Condition {
+	overridenCondReason := condition.OverrideNotSpecifiedReason
+	if hasOverrides {
+		overridenCondReason = condition.OverriddenSucceededReason
+	}
 	return []metav1.Condition{
 		{
 			Type:               string(placementv1beta1.ClusterResourcePlacementScheduledConditionType),
@@ -659,7 +667,7 @@ func crpWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {
 		{
 			Type:               string(placementv1beta1.ClusterResourcePlacementOverriddenConditionType),
 			Status:             metav1.ConditionTrue,
-			Reason:             condition.OverriddenSucceededReason,
+			Reason:             overridenCondReason,
 			ObservedGeneration: generation,
 		},
 		{
@@ -782,17 +790,18 @@ func crpStatusWithWorkSynchronizedUpdatedFailedActual(
 		}
 
 		var wantPlacementStatus []placementv1beta1.ResourcePlacementStatus
+		hasOverrides := len(wantResourceOverrides) > 0 || len(wantClusterResourceOverrides) > 0
 		for _, name := range wantSelectedClusters {
 			wantPlacementStatus = append(wantPlacementStatus, placementv1beta1.ResourcePlacementStatus{
 				ClusterName:                        name,
-				Conditions:                         resourcePlacementWorkSynchronizedFailedConditions(crp.Generation),
+				Conditions:                         resourcePlacementWorkSynchronizedFailedConditions(crp.Generation, hasOverrides),
 				ApplicableResourceOverrides:        wantResourceOverrides,
 				ApplicableClusterResourceOverrides: wantClusterResourceOverrides,
 			})
 		}
 
 		wantStatus := placementv1beta1.ClusterResourcePlacementStatus{
-			Conditions:            crpWorkSynchronizedFailedConditions(crp.Generation),
+			Conditions:            crpWorkSynchronizedFailedConditions(crp.Generation, hasOverrides),
 			PlacementStatuses:     wantPlacementStatus,
 			SelectedResources:     wantSelectedResourceIdentifiers,
 			ObservedResourceIndex: wantObservedResourceIndex,
diff --git a/test/e2e/enveloped_object_placement_test.go b/test/e2e/enveloped_object_placement_test.go
diff --git a/test/e2e/join_and_leave_test.go b/test/e2e/join_and_leave_test.go
diff --git a/test/e2e/resources/test-envelop-configmap.yaml b/test/e2e/resources/test-envelop-configmap.yaml

Original file line number	Diff line number	Diff line change
`@@ -1280,8 +1280,11 @@ func extractResFromConfigMap(uConfigMap *unstructured.Unstructured) ([]fleetv1be`
`1280`	`1280`	`klog.ErrorS(unMarshallErr, "manifest has invalid content", "manifestKey", key, "envelopeResource", klog.KObj(uConfigMap))`
`1281`	`1281`	return nil, fmt.Errorf("the object with manifest key `%s` in envelope config `%s` is malformatted, err: %w", key, klog.KObj(uConfigMap), unMarshallErr)
`1282`	`1282`	`}`
	`1283`	`+ if len(uManifest.GetNamespace()) == 0 {`
	`1284`	`+ // Block cluster-scoped resources.`
	`1285`	`+ return nil, fmt.Errorf("cannot wrap cluster-scoped resource %s in the envelope %s", uManifest.GetName(), klog.KObj(uConfigMap))`
	`1286`	`+ }`
`1283`	`1287`	`if len(uManifest.GetNamespace()) != 0 && uManifest.GetNamespace() != configMap.Namespace {`
`1284`		`- // if the manifest is a namespaced object but no namespace is specified, it will fail at the apply time instead of here.`
`1285`	`1288`	return nil, fmt.Errorf("the namespaced object `%s` in envelope config `%s` is placed in a different namespace `%s` ", uManifest.GetName(), klog.KObj(uConfigMap), uManifest.GetNamespace())
`1286`	`1289`	`}`
`1287`	`1290`	`manifests = append(manifests, fleetv1beta1.Manifest{`
Original file line number	Diff line number	Diff line change
`@@ -613,7 +613,11 @@ func resourcePlacementOverrideFailedConditions(generation int64) []metav1.Condit`
`613`	`613`	`}`
`614`	`614`	`}`
`615`	`615`
`616`		`-func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {`
	`616`	`+func resourcePlacementWorkSynchronizedFailedConditions(generation int64, hasOverrides bool) []metav1.Condition {`
	`617`	`+ overridenCondReason := condition.OverrideNotSpecifiedReason`
	`618`	`+ if hasOverrides {`
	`619`	`+ overridenCondReason = condition.OverriddenSucceededReason`
	`620`	`+ }`
`617`	`621`	`return []metav1.Condition{`
`618`	`622`	`{`
`619`	`623`	`Type: string(placementv1beta1.ResourceScheduledConditionType),`
`@@ -631,7 +635,7 @@ func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav`
`631`	`635`	`Type: string(placementv1beta1.ResourceOverriddenConditionType),`
`632`	`636`	`Status: metav1.ConditionTrue,`
`633`	`637`	`ObservedGeneration: generation,`
`634`		`- Reason: condition.OverriddenSucceededReason,`
	`638`	`+ Reason: overridenCondReason,`
`635`	`639`	`},`
`636`	`640`	`{`
`637`	`641`	`Type: string(placementv1beta1.ResourceWorkSynchronizedConditionType),`
`@@ -642,7 +646,11 @@ func resourcePlacementWorkSynchronizedFailedConditions(generation int64) []metav`
`642`	`646`	`}`
`643`	`647`	`}`
`644`	`648`
`645`		`-func crpWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {`
	`649`	`+func crpWorkSynchronizedFailedConditions(generation int64, hasOverrides bool) []metav1.Condition {`
	`650`	`+ overridenCondReason := condition.OverrideNotSpecifiedReason`
	`651`	`+ if hasOverrides {`
	`652`	`+ overridenCondReason = condition.OverriddenSucceededReason`
	`653`	`+ }`
`646`	`654`	`return []metav1.Condition{`
`647`	`655`	`{`
`648`	`656`	`Type: string(placementv1beta1.ClusterResourcePlacementScheduledConditionType),`
`@@ -659,7 +667,7 @@ func crpWorkSynchronizedFailedConditions(generation int64) []metav1.Condition {`
`659`	`667`	`{`
`660`	`668`	`Type: string(placementv1beta1.ClusterResourcePlacementOverriddenConditionType),`
`661`	`669`	`Status: metav1.ConditionTrue,`
`662`		`- Reason: condition.OverriddenSucceededReason,`
	`670`	`+ Reason: overridenCondReason,`
`663`	`671`	`ObservedGeneration: generation,`
`664`	`672`	`},`
`665`	`673`	`{`
`@@ -782,17 +790,18 @@ func crpStatusWithWorkSynchronizedUpdatedFailedActual(`
`782`	`790`	`}`
`783`	`791`
`784`	`792`	`var wantPlacementStatus []placementv1beta1.ResourcePlacementStatus`
	`793`	`+ hasOverrides := len(wantResourceOverrides) > 0 \|\| len(wantClusterResourceOverrides) > 0`
`785`	`794`	`for _, name := range wantSelectedClusters {`
`786`	`795`	`wantPlacementStatus = append(wantPlacementStatus, placementv1beta1.ResourcePlacementStatus{`
`787`	`796`	`ClusterName: name,`
`788`		`- Conditions: resourcePlacementWorkSynchronizedFailedConditions(crp.Generation),`
	`797`	`+ Conditions: resourcePlacementWorkSynchronizedFailedConditions(crp.Generation, hasOverrides),`
`789`	`798`	`ApplicableResourceOverrides: wantResourceOverrides,`
`790`	`799`	`ApplicableClusterResourceOverrides: wantClusterResourceOverrides,`
`791`	`800`	`})`
`792`	`801`	`}`
`793`	`802`
`794`	`803`	`wantStatus := placementv1beta1.ClusterResourcePlacementStatus{`
`795`		`- Conditions: crpWorkSynchronizedFailedConditions(crp.Generation),`
	`804`	`+ Conditions: crpWorkSynchronizedFailedConditions(crp.Generation, hasOverrides),`
`796`	`805`	`PlacementStatuses: wantPlacementStatus,`
`797`	`806`	`SelectedResources: wantSelectedResourceIdentifiers,`
`798`	`807`	`ObservedResourceIndex: wantObservedResourceIndex,`