From e23ebab11c1de74d527e58aaf5b7c25a95663b9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Tue, 23 Apr 2024 17:48:38 +0200 Subject: [PATCH] Reconcile trigger on OIDC service account changes only, if SA references a trigger for correct broker class (#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) --- pkg/reconciler/broker/trigger/controller.go | 33 ++++- .../broker/trigger/controller_test.go | 132 ++++++++++++++++++ 2 files changed, 164 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/broker/trigger/controller.go b/pkg/reconciler/broker/trigger/controller.go index a575df9fc82..8e92136d93a 100644 --- a/pkg/reconciler/broker/trigger/controller.go +++ b/pkg/reconciler/broker/trigger/controller.go @@ -19,6 +19,9 @@ 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" @@ -116,13 +119,41 @@ func NewController( // Reconciler Trigger when the OIDC service account changes oidcServiceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ - FilterFunc: controller.FilterController(&eventing.Trigger{}), + 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 { diff --git a/pkg/reconciler/broker/trigger/controller_test.go b/pkg/reconciler/broker/trigger/controller_test.go index 772dec3bd9c..f9f090e5ba6 100644 --- a/pkg/reconciler/broker/trigger/controller_test.go +++ b/pkg/reconciler/broker/trigger/controller_test.go @@ -21,6 +21,10 @@ import ( "fmt" "testing" + "knative.dev/pkg/ptr" + + triggerinformer "knative.dev/eventing/pkg/client/injection/informers/eventing/v1/trigger" + "knative.dev/eventing/pkg/auth" filteredFactory "knative.dev/pkg/client/injection/kube/informers/factory/filtered" @@ -67,6 +71,134 @@ func SetUpInformerSelector(ctx context.Context) context.Context { return ctx } +func TestFilterOIDCServiceAccounts(t *testing.T) { + ctx, _ := SetupFakeContext(t, SetUpInformerSelector) + + tt := []struct { + name string + sa *corev1.ServiceAccount + trigger *eventing.Trigger + brokers []*eventing.Broker + pass bool + }{{ + name: "matching owner reference", + sa: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sa", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: eventing.SchemeGroupVersion.String(), + Kind: "Trigger", + Name: "tr", + Controller: ptr.Bool(true), + }, + }, + }, + }, + trigger: &eventing.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "tr", + }, + Spec: eventing.TriggerSpec{ + Broker: "br", + }, + }, + brokers: []*eventing.Broker{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "br", + Annotations: map[string]string{ + eventing.BrokerClassAnnotationKey: apiseventing.MTChannelBrokerClassValue, + }, + }, + }}, + pass: true, + }, { + name: "references trigger for wrong broker class", + sa: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sa", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: eventing.SchemeGroupVersion.String(), + Kind: "Trigger", + Name: "tr", + Controller: ptr.Bool(true), + }, + }, + }, + }, + trigger: &eventing.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "tr", + }, + Spec: eventing.TriggerSpec{ + Broker: "br", + }, + }, + brokers: []*eventing.Broker{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "br", + Annotations: map[string]string{ + eventing.BrokerClassAnnotationKey: "another-broker-class", + }, + }, + }}, + pass: false, + }, { + name: "no owner reference", + sa: &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "sa", + }, + }, + trigger: &eventing.Trigger{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "tr", + }, + Spec: eventing.TriggerSpec{ + Broker: "br", + }, + }, + brokers: []*eventing.Broker{{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns", + Name: "br", + Annotations: map[string]string{ + eventing.BrokerClassAnnotationKey: apiseventing.MTChannelBrokerClassValue, + }, + }, + }}, + pass: false, + }} + + for _, tc := range tt { + tc := tc + t.Run(tc.name, func(t *testing.T) { + brokerInformer := brokerinformer.Get(ctx) + for _, obj := range tc.brokers { + err := brokerInformer.Informer().GetStore().Add(obj) + assert.NoError(t, err) + } + + triggerInformer := triggerinformer.Get(ctx) + err := triggerInformer.Informer().GetStore().Add(tc.trigger) + assert.NoError(t, err) + + filter := filterOIDCServiceAccounts(triggerInformer.Lister(), brokerInformer.Lister()) + pass := filter(tc.sa) + assert.Equal(t, tc.pass, pass) + }) + } +} + func TestFilterTriggers(t *testing.T) { ctx, _ := SetupFakeContext(t, SetUpInformerSelector)