[release-v1.12] Reconcile trigger on OIDC service account changes onl…

…y, if SA references a trigger for correct broker class (#592) * Use filtered informer to watch OIDC service accounts (knative#7527) * controller.go changed * knative#7320 WIP * WIP: Testing filtered informer (knative#7341) * unit test passed * Revert "Merge remote-tracking branch 'otherfork/main' into main" This reverts commit 94cd51b, reversing changes made to 0bf2982. * Removed comments * Changed to filtered informer for Subscription identity service account * Changed to filtered informer for Sequence service accounts * Changed to filtered informer for Parallel identity service accounts * Changed to filtered informer for APIServerSource identity service account * fixed unit tests * added label selector for mtchannel_broker * added filtered informer for sinkbinding identity service accounts * added OIDC label selector in webhook * added filtered informer for containersource service accounts * added filtered informer for pingsource service accounts * added OIDC label selector in apiserver ctx * added OIDC label selector in broker/filter * added OIDC label selector in broker/ingress * added OIDC label selector in in_memory/channel_dispatcher * added OIDC label selector in mtping * fixed unit test issues for pingsource * fixed unit test for container source * formatted files * updated service account informer in apiserversource * updated service account informers in other places * small typo fix * added actual value for OIDC label * added a valid value for OIDClabelkey * changed references of OIDCLabelKey * fixed import path problem * changed OIDCLabelSelector in all main.go files * changed instances of OIDCLabelSelector in controller and controller test files * deleted OIDC related labels from register.go * fixed formatting issues * Added value for OIDCLabelKey --------- Co-authored-by: Scott <[email protected]> * Reconcile trigger on OIDC service account changes only, if SA references a trigger for correct broker class (knative#7849) * Reconcile trigger on OIDC service account changes only, if SA references a trigger for correct broker class * Run goimports and gofmt * Remove deprecated use of pointer.Bool(v) and switch to prt.Bool(v) --------- Co-authored-by: Yijie Wang <[email protected]> Co-authored-by: Scott <[email protected]>
openshift-knative · Apr 24, 2024 · bbb4c41 · bbb4c41
1 parent e51d7ba
commit bbb4c41
Show file tree

Hide file tree

Showing 31 changed files with 436 additions and 67 deletions.
diff --git a/cmd/apiserver_receive_adapter/main.go b/cmd/apiserver_receive_adapter/main.go
@@ -22,6 +22,7 @@ import (
 
 	"knative.dev/eventing/pkg/adapter/apiserver"
 	"knative.dev/eventing/pkg/adapter/v2"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 )
 
@@ -34,6 +35,7 @@ func main() {
 	ctx = adapter.WithInjectorEnabled(ctx)
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/broker/filter/main.go b/cmd/broker/filter/main.go
@@ -81,6 +81,7 @@ func main() {
 	log.Printf("Registering %d informers", len(injection.Default.GetInformers()))
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/broker/ingress/main.go b/cmd/broker/ingress/main.go
@@ -103,6 +103,7 @@ func main() {
 	log.Printf("Registering %d informers", len(injection.Default.GetInformers()))
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/controller/main.go b/cmd/controller/main.go
@@ -28,7 +28,7 @@ import (
 
 	"knative.dev/pkg/injection/sharedmain"
 
-	"knative.dev/eventing/pkg/apis/sources"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 
 	filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
@@ -79,7 +79,7 @@ func main() {
 	}()
 
 	ctx = filteredFactory.WithSelectors(ctx,
-		sources.OIDCTokenRoleLabelSelector,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/in_memory/channel_dispatcher/main.go b/cmd/in_memory/channel_dispatcher/main.go
@@ -27,6 +27,7 @@ import (
 	"knative.dev/pkg/injection/sharedmain"
 	"knative.dev/pkg/signals"
 
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 	inmemorychannel "knative.dev/eventing/pkg/reconciler/inmemorychannel/dispatcher"
 )
@@ -39,6 +40,7 @@ func main() {
 	}
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/mtchannel_broker/main.go b/cmd/mtchannel_broker/main.go
@@ -22,8 +22,12 @@ import (
 
 	"context"
 
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/pkg/injection/sharedmain"
 
+	filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
+	"knative.dev/pkg/signals"
+
 	"knative.dev/eventing/pkg/reconciler/broker"
 	mttrigger "knative.dev/eventing/pkg/reconciler/broker/trigger"
 )
@@ -33,7 +37,12 @@ const (
 )
 
 func main() {
-	sharedmain.Main(
+	ctx := signals.NewContext()
+
+	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector)
+
+	sharedmain.MainWithContext(ctx,
 		component,
 
 		broker.NewController,

diff --git a/cmd/mtping/main.go b/cmd/mtping/main.go
@@ -22,6 +22,7 @@ import (
 
 	"knative.dev/eventing/pkg/adapter/mtping"
 	"knative.dev/eventing/pkg/adapter/v2"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 )
 
@@ -57,6 +58,7 @@ func main() {
 	})
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go
@@ -26,6 +26,7 @@ import (
 	configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/filtered"
 
 	"knative.dev/eventing/pkg/apis/feature"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 
 	filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
@@ -287,6 +288,7 @@ func main() {
 	})
 
 	ctx = filteredFactory.WithSelectors(ctx,
+		auth.OIDCLabelSelector,
 		eventingtls.TrustBundleLabelSelector,
 	)
 

diff --git a/pkg/apis/sources/register.go b/pkg/apis/sources/register.go
@@ -32,12 +32,6 @@ const (
 	// SourceDuckLabelValue is the label value to indicate
 	// the CRD is a Source duck type.
 	SourceDuckLabelValue = "true"
-
-	//OIDCLabelKey is used to filter out all the informers that related to OIDC work
-	OIDCLabelKey = "oidc"
-
-	// OIDCTokenRoleLabelSelector is the label selector for the OIDC token creator role and rolebinding informers
-	OIDCTokenRoleLabelSelector = OIDCLabelKey
 )
 
 var (

diff --git a/pkg/auth/serviceaccount.go b/pkg/auth/serviceaccount.go
@@ -37,6 +37,14 @@ import (
 	"knative.dev/pkg/ptr"
 )
 
+const (
+	//OIDCLabelKey is used to filter out all the informers that related to OIDC work
+	OIDCLabelKey = "oidc"
+
+	// OIDCTokenRoleLabelSelector is the label selector for the OIDC token creator role and rolebinding informers
+	OIDCLabelSelector = OIDCLabelKey
+)
+
 // GetOIDCServiceAccountNameForResource returns the service account name to use
 // for OIDC authentication for the given resource.
 func GetOIDCServiceAccountNameForResource(gvk schema.GroupVersionKind, objectMeta metav1.ObjectMeta) string {
@@ -66,6 +74,9 @@ func GetOIDCServiceAccountForResource(gvk schema.GroupVersionKind, objectMeta me
 			Annotations: map[string]string{
 				"description": fmt.Sprintf("Service Account for OIDC Authentication for %s %q", gvk.GroupKind().Kind, objectMeta.Name),
 			},
+			Labels: map[string]string{
+				OIDCLabelKey: "enabled",
+			},
 		},
 	}
 }

diff --git a/pkg/auth/serviceaccount_test.go b/pkg/auth/serviceaccount_test.go
@@ -108,6 +108,9 @@ func TestGetOIDCServiceAccountForResource(t *testing.T) {
 			Annotations: map[string]string{
 				"description": "Service Account for OIDC Authentication for Broker \"my-broker\"",
 			},
+			Labels: map[string]string{
+				OIDCLabelKey: "enabled",
+			},
 		},
 	}
 

diff --git a/pkg/reconciler/apiserversource/apiserversource_test.go b/pkg/reconciler/apiserversource/apiserversource_test.go
@@ -21,8 +21,6 @@ import (
 	"fmt"
 	"testing"
 
-	"knative.dev/eventing/pkg/apis/sources"
-
 	"knative.dev/pkg/kmeta"
 
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -1356,7 +1354,7 @@ func makeOIDCRole() *rbacv1.Role {
 				"description": fmt.Sprintf("Role for OIDC Authentication for ApiServerSource %q", sourceName),
 			},
 			Labels: map[string]string{
-				sources.OIDCLabelKey: "",
+				auth.OIDCLabelKey: "enabled",
 			},
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(src),
@@ -1386,7 +1384,7 @@ func makeOIDCRoleBinding() *rbacv1.RoleBinding {
 				"description": fmt.Sprintf("Role Binding for OIDC Authentication for ApiServerSource %q", sourceName),
 			},
 			Labels: map[string]string{
-				sources.OIDCLabelKey: "",
+				auth.OIDCLabelKey: "enabled",
 			},
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(src),

diff --git a/pkg/reconciler/apiserversource/controller.go b/pkg/reconciler/apiserversource/controller.go
@@ -22,7 +22,7 @@ import (
 	configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/filtered"
 	"knative.dev/pkg/system"
 
-	"knative.dev/eventing/pkg/apis/sources"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 	eventingreconciler "knative.dev/eventing/pkg/reconciler"
 
@@ -42,7 +42,8 @@ import (
 	deploymentinformer "knative.dev/pkg/client/injection/kube/informers/apps/v1/deployment"
 	"knative.dev/pkg/client/injection/kube/informers/core/v1/namespace"
 
-	serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount"
+	serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/filtered"
+
 	roleinformer "knative.dev/pkg/client/injection/kube/informers/rbac/v1/role/filtered"
 	rolebindinginformer "knative.dev/pkg/client/injection/kube/informers/rbac/v1/rolebinding/filtered"
 
@@ -67,11 +68,11 @@ func NewController(
 	deploymentInformer := deploymentinformer.Get(ctx)
 	apiServerSourceInformer := apiserversourceinformer.Get(ctx)
 	namespaceInformer := namespace.Get(ctx)
-	serviceaccountInformer := serviceaccountinformer.Get(ctx)
+	oidcServiceaccountInformer := serviceaccountinformer.Get(ctx, auth.OIDCLabelSelector)
 
 	// Create a selector string
-	roleInformer := roleinformer.Get(ctx, sources.OIDCTokenRoleLabelSelector)
-	rolebindingInformer := rolebindinginformer.Get(ctx, sources.OIDCTokenRoleLabelSelector)
+	roleInformer := roleinformer.Get(ctx, auth.OIDCLabelSelector)
+	rolebindingInformer := rolebindinginformer.Get(ctx, auth.OIDCLabelSelector)
 
 	trustBundleConfigMapInformer := configmapinformer.Get(ctx, eventingtls.TrustBundleLabelSelector)
 
@@ -89,7 +90,7 @@ func NewController(
 		ceSource:                   GetCfgHost(ctx),
 		configs:                    reconcilersource.WatchConfigurations(ctx, component, cmw),
 		namespaceLister:            namespaceInformer.Lister(),
-		serviceAccountLister:       serviceaccountInformer.Lister(),
+		serviceAccountLister:       oidcServiceaccountInformer.Lister(),
 		roleLister:                 roleInformer.Lister(),
 		roleBindingLister:          rolebindingInformer.Lister(),
 		trustBundleConfigMapLister: trustBundleConfigMapInformer.Lister(),
@@ -142,7 +143,7 @@ func NewController(
 	})
 
 	// Reconciler ApiServerSource when the OIDC service account changes
-	serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+	oidcServiceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
 		FilterFunc: controller.FilterController(&v1.ApiServerSource{}),
 		Handler:    controller.HandleAll(impl.EnqueueControllerOf),
 	})

diff --git a/pkg/reconciler/apiserversource/controller_test.go b/pkg/reconciler/apiserversource/controller_test.go
@@ -23,7 +23,7 @@ import (
 
 	filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered"
 
-	"knative.dev/eventing/pkg/apis/sources"
+	"knative.dev/eventing/pkg/auth"
 	"knative.dev/eventing/pkg/eventingtls"
 
 	"knative.dev/eventing/pkg/apis/feature"
@@ -42,7 +42,7 @@ import (
 	// Fake injection informers
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/filtered/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/namespace/fake"
-	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake"
+	_ "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/filtered/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/factory/filtered/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/rbac/v1/role/filtered/fake"
 	_ "knative.dev/pkg/client/injection/kube/informers/rbac/v1/rolebinding/filtered/fake"
@@ -98,6 +98,6 @@ func TestNew(t *testing.T) {
 }
 
 func SetUpInformerSelector(ctx context.Context) context.Context {
-	ctx = filteredFactory.WithSelectors(ctx, eventingtls.TrustBundleLabelSelector, sources.OIDCTokenRoleLabelSelector)
+	ctx = filteredFactory.WithSelectors(ctx, eventingtls.TrustBundleLabelSelector, auth.OIDCLabelSelector)
 	return ctx
 }
diff --git a/pkg/reconciler/apiserversource/resources/oidc_rolebinding.go b/pkg/reconciler/apiserversource/resources/oidc_rolebinding.go
@@ -19,7 +19,7 @@ package resources
 import (
 	"fmt"
 
-	"knative.dev/eventing/pkg/apis/sources"
+	"knative.dev/eventing/pkg/auth"
 
 	"knative.dev/pkg/kmeta"
 
@@ -54,7 +54,7 @@ func MakeOIDCRole(source *v1.ApiServerSource) (*rbacv1.Role, error) {
 				"description": fmt.Sprintf("Role for OIDC Authentication for ApiServerSource %q", source.GetName()),
 			},
 			Labels: map[string]string{
-				sources.OIDCLabelKey: "",
+				auth.OIDCLabelKey: "enabled",
 			},
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(source),
@@ -92,7 +92,7 @@ func MakeOIDCRoleBinding(source *v1.ApiServerSource) (*rbacv1.RoleBinding, error
 				"description": fmt.Sprintf("Role Binding for OIDC Authentication for ApiServerSource %q", source.GetName()),
 			},
 			Labels: map[string]string{
-				sources.OIDCLabelKey: "",
+				auth.OIDCLabelKey: "enabled",
 			},
 			OwnerReferences: []metav1.OwnerReference{
 				*kmeta.NewControllerRef(source),

diff --git a/pkg/reconciler/broker/trigger/controller.go b/pkg/reconciler/broker/trigger/controller.go
@@ -19,12 +19,16 @@ package mttrigger
 import (
 	"context"
 
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"knative.dev/eventing/pkg/auth"
+
 	"go.uber.org/zap"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/client-go/tools/cache"
 	"knative.dev/pkg/client/injection/ducks/duck/v1/source"
 	configmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap"
-	serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount"
 	"knative.dev/pkg/configmap"
 	"knative.dev/pkg/controller"
 	"knative.dev/pkg/injection/clients/dynamicclient"
@@ -45,6 +49,8 @@ import (
 	eventinglisters "knative.dev/eventing/pkg/client/listers/eventing/v1"
 	"knative.dev/eventing/pkg/duck"
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
+
+	serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/filtered"
 )
 
 // NewController initializes the controller and is called by the generated code
@@ -59,7 +65,7 @@ func NewController(
 	subscriptionInformer := subscriptioninformer.Get(ctx)
 	configmapInformer := configmapinformer.Get(ctx)
 	secretInformer := secretinformer.Get(ctx)
-	serviceaccountInformer := serviceaccountinformer.Get(ctx)
+	oidcServiceaccountInformer := serviceaccountinformer.Get(ctx, auth.OIDCLabelSelector)
 
 	featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"))
 	featureStore.WatchConfigs(cmw)
@@ -74,7 +80,7 @@ func NewController(
 		triggerLister:        triggerLister,
 		configmapLister:      configmapInformer.Lister(),
 		secretLister:         secretInformer.Lister(),
-		serviceAccountLister: serviceaccountInformer.Lister(),
+		serviceAccountLister: oidcServiceaccountInformer.Lister(),
 	}
 	impl := triggerreconciler.NewImpl(ctx, r, func(impl *controller.Impl) controller.Options {
 		return controller.Options{
@@ -112,14 +118,42 @@ func NewController(
 	})
 
 	// Reconciler Trigger when the OIDC service account changes
-	serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
-		FilterFunc: controller.FilterController(&eventing.Trigger{}),
+	oidcServiceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: filterOIDCServiceAccounts(triggerInformer.Lister(), brokerInformer.Lister()),
 		Handler:    controller.HandleAll(impl.EnqueueControllerOf),
 	})
 
 	return impl
 }
 
+// filterOIDCServiceAccounts returns a function that returns true if the resource passed
+// is a service account, which is owned by a trigger pointing to a MTChannelBased broker.
+func filterOIDCServiceAccounts(triggerLister eventinglisters.TriggerLister, brokerLister eventinglisters.BrokerLister) func(interface{}) bool {
+	return func(obj interface{}) bool {
+		controlledByTrigger := controller.FilterController(&eventing.Trigger{})(obj)
+		if !controlledByTrigger {
+			return false
+		}
+
+		sa, ok := obj.(*corev1.ServiceAccount)
+		if !ok {
+			return false
+		}
+
+		owner := metav1.GetControllerOf(sa)
+		if owner == nil {
+			return false
+		}
+
+		trigger, err := triggerLister.Triggers(sa.Namespace).Get(owner.Name)
+		if err != nil {
+			return false
+		}
+
+		return filterTriggers(brokerLister)(trigger)
+	}
+}
+
 // filterTriggers returns a function that returns true if the resource passed
 // is a trigger pointing to a MTChannelBroker.
 func filterTriggers(lister eventinglisters.BrokerLister) func(interface{}) bool {