Move AggregationRuleTransform to shared common package (#2316)

AggregationRuleTransform was only applied to KnativeServing but not to
KnativeEventing. This caused the operator to continuously overwrite the
rules of aggregated ClusterRoles (e.g. channelable-manipulator) with
empty rules from the manifest, creating a race condition with the
Kubernetes aggregation controller. During the race window, RBAC lookups
against the aggregated role return 403 Forbidden.

Move the transform from knativeserving/common to the shared
reconciler/common package and apply it inside Transform(), so every
component (Serving, Eventing, Kafka) benefits automatically.

Signed-off-by: Christoph Stäbler <cstabler@redhat.com>

Author: creydr

…nativeserving/common/aggregated_rules.go pkg/reconciler/common/aggregated_rules.gopkg/reconciler/knativeserving/common/aggregated_rules.go renamed to pkg/reconciler/common/aggregated_rules.go pkg/reconciler/knativeserving/common/aggregated_rules.go renamed to pkg/reconciler/common/aggregated_rules.go

@@ -26,12 +26,13 @@ type unstructuredGetter interface {
 	Get(obj *unstructured.Unstructured) (*unstructured.Unstructured, error)
 }
 
-// AggregationRuleTransform
+// AggregationRuleTransform preserves the rules of aggregated ClusterRoles.
+// The Kubernetes aggregation controller manages the rules field of aggregated
+// ClusterRoles. Without this transform, manifestival overwrites the aggregated
+// rules with the empty rules from the manifest, causing a race condition.
 func AggregationRuleTransform(client unstructuredGetter) mf.Transformer {
 	return func(u *unstructured.Unstructured) error {
 		if u.GetKind() == "ClusterRole" && u.Object["aggregationRule"] != nil {
-			// we rely on the controller manager to fill in rules so
-			// ours will always trigger an unnecessary update
 			current, err := client.Get(u)
 			if errors.IsNotFound(err) {
 				return nil

…eserving/common/aggregated_rules_test.go …conciler/common/aggregated_rules_test.gopkg/reconciler/knativeserving/common/aggregated_rules_test.go renamed to pkg/reconciler/common/aggregated_rules_test.go pkg/reconciler/knativeserving/common/aggregated_rules_test.go renamed to pkg/reconciler/common/aggregated_rules_test.go

@@ -74,6 +74,41 @@ func TestAggregationRuleTransform(t *testing.T) {
           serving.knative.dev/controller: "true"
     rules: []
   overwriteExpected: false
+- name: "existing eventing aggregated role has rules"
+  input:
+    kind: ClusterRole
+    apiVersion: rbac.authorization.k8s.io/v1
+    metadata:
+      name: channelable-manipulator
+    aggregationRule:
+      clusterRoleSelectors:
+      - matchLabels:
+          duck.knative.dev/channelable: "true"
+    rules: []
+  existing:
+    kind: ClusterRole
+    apiVersion: rbac.authorization.k8s.io/v1
+    metadata:
+      name: channelable-manipulator
+    aggregationRule:
+      clusterRoleSelectors:
+      - matchLabels:
+          duck.knative.dev/channelable: "true"
+    rules:
+    - apiGroups:
+      - messaging.knative.dev
+      resources:
+      - channels
+      - channels/status
+      verbs:
+      - create
+      - get
+      - list
+      - watch
+      - update
+      - patch
+      - delete
+  overwriteExpected: true
 `)
 	err := yaml.Unmarshal(testData, &tests)
 	if err != nil {

pkg/reconciler/common/transformers.go

@@ -65,6 +65,7 @@ func Transform(ctx context.Context, manifest *mf.Manifest, instance base.KCompon
 	logger.Debug("Transforming manifest")
 
 	transformers := transformers(ctx, instance)
+	transformers = append(transformers, AggregationRuleTransform(manifest.Client))
 	transformers = append(transformers, extra...)
 
 	m, err := manifest.Transform(transformers...)

pkg/reconciler/knativeserving/knativeserving.go

@@ -160,7 +160,6 @@ func (r *Reconciler) transform(ctx context.Context, manifest *mf.Manifest, comp
 	extra = append(extra,
 		common.InjectOwner(instance, anchorOwner),
 		ksc.CustomCertsTransform(instance, logger),
-		ksc.AggregationRuleTransform(manifest.Client),
 		// Ensure all resources have the selector applied so that the controller re-queues applied resources when they change.
 		common.InjectLabel(SelectorKey, SelectorValue),
 	)