From d4367384c8b96dce4c5fff5941894589a4e0982b Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 10 Oct 2023 16:11:26 -0400 Subject: [PATCH 1/7] Implement the OIDC service account for PingSource --- pkg/apis/sources/v1/ping_lifecycle.go | 22 +++++++++++++++++++++- pkg/reconciler/pingsource/controller.go | 14 +++++++++++--- pkg/reconciler/pingsource/pingsource.go | 23 +++++++++++++++++++++++ 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/pkg/apis/sources/v1/ping_lifecycle.go b/pkg/apis/sources/v1/ping_lifecycle.go index 24222d62112..8fa7cea6fae 100644 --- a/pkg/apis/sources/v1/ping_lifecycle.go +++ b/pkg/apis/sources/v1/ping_lifecycle.go @@ -35,11 +35,15 @@ const ( // PingSourceConditionDeployed has status True when the PingSource has had it's receive adapter deployment created. PingSourceConditionDeployed apis.ConditionType = "Deployed" + + // PingSourceConditionOIDCIdentityCreated has status True when the PingSource has had it's OIDC identity created. + PingSourceConditionOIDCIdentityCreated apis.ConditionType = "OIDCIdentityCreated" ) var PingSourceCondSet = apis.NewLivingConditionSet( PingSourceConditionSinkProvided, - PingSourceConditionDeployed) + PingSourceConditionDeployed, + PingSourceConditionOIDCIdentityCreated) const ( // PingSourceEventType is the default PingSource CloudEvent type. @@ -122,3 +126,19 @@ func (s *PingSourceStatus) PropagateDeploymentAvailability(d *appsv1.Deployment) PingSourceCondSet.Manage(s).MarkUnknown(PingSourceConditionDeployed, "DeploymentUnavailable", "The Deployment '%s' is unavailable.", d.Name) } } + +func (s *PingSourceStatus) MarkOIDCIdentityCreatedSucceeded() { + PingSourceCondSet.Manage(s).MarkTrue(PingSourceConditionOIDCIdentityCreated) +} + +func (s *PingSourceStatus) MarkOIDCIdentityCreatedSucceededWithReason(reason, messageFormat string, messageA ...interface{}) { + PingSourceCondSet.Manage(s).MarkTrueWithReason(PingSourceConditionOIDCIdentityCreated, reason, messageFormat, messageA...) +} + +func (s *PingSourceStatus) MarkOIDCIdentityCreatedFailed(reason, messageFormat string, messageA ...interface{}) { + PingSourceCondSet.Manage(s).MarkFalse(PingSourceConditionOIDCIdentityCreated, reason, messageFormat, messageA...) +} + +func (s *PingSourceStatus) MarkOIDCIdentityCreatedUnknown(reason, messageFormat string, messageA ...interface{}) { + PingSourceCondSet.Manage(s).MarkUnknown(PingSourceConditionOIDCIdentityCreated, reason, messageFormat, messageA...) +} diff --git a/pkg/reconciler/pingsource/controller.go b/pkg/reconciler/pingsource/controller.go index ff52bad2b0b..6298027d16c 100644 --- a/pkg/reconciler/pingsource/controller.go +++ b/pkg/reconciler/pingsource/controller.go @@ -18,6 +18,7 @@ package pingsource import ( "context" + serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount" "go.uber.org/zap" @@ -67,11 +68,13 @@ func NewController( deploymentInformer := deploymentinformer.Get(ctx) pingSourceInformer := pingsourceinformer.Get(ctx) + serviceaccountInformer := serviceaccountinformer.Get(ctx) r := &Reconciler{ - kubeClientSet: kubeclient.Get(ctx), - leConfig: leConfig, - configAcc: reconcilersource.WatchConfigurations(ctx, component, cmw), + kubeClientSet: kubeclient.Get(ctx), + leConfig: leConfig, + configAcc: reconcilersource.WatchConfigurations(ctx, component, cmw), + serviceAccountLister: serviceaccountInformer.Lister(), } impl := pingsourcereconciler.NewImpl(ctx, r, func(impl *controller.Impl) controller.Options { @@ -97,5 +100,10 @@ func NewController( )), }) + serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ + FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), mtadapterName), + Handler: controller.HandleAll(impl.EnqueueControllerOf), + }) + return impl } diff --git a/pkg/reconciler/pingsource/pingsource.go b/pkg/reconciler/pingsource/pingsource.go index ea1afae131e..6253d259dee 100644 --- a/pkg/reconciler/pingsource/pingsource.go +++ b/pkg/reconciler/pingsource/pingsource.go @@ -20,6 +20,9 @@ import ( "context" "encoding/json" "fmt" + v1 "k8s.io/client-go/listers/core/v1" + "knative.dev/eventing/pkg/apis/feature" + "knative.dev/eventing/pkg/auth" "go.uber.org/zap" @@ -74,6 +77,8 @@ type Reconciler struct { // Leader election configuration for the mt receive adapter leConfig string + + serviceAccountLister v1.ServiceAccountLister } // Check that our Reconciler implements ReconcileKind @@ -99,6 +104,24 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, source *sourcesv1.PingSo } } + // OIDC authentication + featureFlags := feature.FromContext(ctx) + if featureFlags.IsOIDCAuthentication() { + saName := auth.GetOIDCServiceAccountNameForResource(sourcesv1.SchemeGroupVersion.WithKind("PingSource"), source.ObjectMeta) + source.Status.Auth = &duckv1.AuthStatus{ + ServiceAccountName: &saName, + } + + if err := auth.EnsureOIDCServiceAccountExistsForResource(ctx, r.serviceAccountLister, r.kubeClientSet, sourcesv1.SchemeGroupVersion.WithKind("PingSource"), source.ObjectMeta); err != nil { + source.Status.MarkOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", "%v", err) + return err + } + source.Status.MarkOIDCIdentityCreatedSucceeded() + } else { + source.Status.Auth = nil + source.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "") + } + sinkAddr, err := r.sinkResolver.AddressableFromDestinationV1(ctx, *dest, source) if err != nil { source.Status.MarkNoSink("NotFound", "") From dd60e7b8b533619f41a036be6892caf9c7c42c08 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 10 Oct 2023 17:03:04 -0400 Subject: [PATCH 2/7] Adding the tests --- pkg/apis/sources/v1/ping_lifecycle_test.go | 59 ++++++-- pkg/reconciler/pingsource/pingsource_test.go | 148 ++++++++++++++++++- pkg/reconciler/testing/v1/pingsource.go | 30 ++++ 3 files changed, 218 insertions(+), 19 deletions(-) diff --git a/pkg/apis/sources/v1/ping_lifecycle_test.go b/pkg/apis/sources/v1/ping_lifecycle_test.go index e01ce800eb7..266005baeae 100644 --- a/pkg/apis/sources/v1/ping_lifecycle_test.go +++ b/pkg/apis/sources/v1/ping_lifecycle_test.go @@ -59,14 +59,16 @@ func TestPingSourceStatusIsReady(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - wantConditionStatus corev1.ConditionStatus - want bool + name string + s *PingSourceStatus + wantConditionStatus corev1.ConditionStatus + want bool + oidcServiceAccountStatus bool }{{ - name: "uninitialized", - s: &PingSourceStatus{}, - want: false, + name: "uninitialized", + s: &PingSourceStatus{}, + want: false, + oidcServiceAccountStatus: true, }, { name: "initialized", s: func() *PingSourceStatus { @@ -74,8 +76,9 @@ func TestPingSourceStatusIsReady(t *testing.T) { s.InitializeConditions() return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, + wantConditionStatus: corev1.ConditionUnknown, + want: false, + oidcServiceAccountStatus: true, }, { name: "mark deployed", s: func() *PingSourceStatus { @@ -84,8 +87,9 @@ func TestPingSourceStatusIsReady(t *testing.T) { s.PropagateDeploymentAvailability(availableDeployment) return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, + wantConditionStatus: corev1.ConditionUnknown, + want: false, + oidcServiceAccountStatus: true, }, { name: "mark sink", s: func() *PingSourceStatus { @@ -95,8 +99,9 @@ func TestPingSourceStatusIsReady(t *testing.T) { s.MarkSink(exampleAddr) return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, + wantConditionStatus: corev1.ConditionUnknown, + want: false, + oidcServiceAccountStatus: true, }, { name: "mark sink and deployed", s: func() *PingSourceStatus { @@ -106,12 +111,29 @@ func TestPingSourceStatusIsReady(t *testing.T) { s.PropagateDeploymentAvailability(availableDeployment) return s }(), - wantConditionStatus: corev1.ConditionTrue, - want: true, - }} + wantConditionStatus: corev1.ConditionTrue, + want: true, + oidcServiceAccountStatus: true, + }, + { + name: "oidc status false with mark sink and deployed", + s: func() *PingSourceStatus { + s := &PingSourceStatus{} + s.InitializeConditions() + s.MarkSink(exampleAddr) + s.PropagateDeploymentAvailability(availableDeployment) + s.MarkOIDCIdentityCreatedSucceeded() + return s + }(), + wantConditionStatus: corev1.ConditionUnknown, + want: true, + oidcServiceAccountStatus: false, + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { + ss := &PingSourceStatus{} if test.wantConditionStatus != "" { gotConditionStatus := test.s.GetTopLevelCondition().Status if gotConditionStatus != test.wantConditionStatus { @@ -122,6 +144,11 @@ func TestPingSourceStatusIsReady(t *testing.T) { if got != test.want { t.Errorf("unexpected readiness: want %v, got %v", test.want, got) } + if test.oidcServiceAccountStatus { + ss.MarkOIDCIdentityCreatedSucceeded() + } else { + ss.MarkOIDCIdentityCreatedFailed("Unable to ...", "") + } }) } } diff --git a/pkg/reconciler/pingsource/pingsource_test.go b/pkg/reconciler/pingsource/pingsource_test.go index d2c099983f6..0c3c206e134 100644 --- a/pkg/reconciler/pingsource/pingsource_test.go +++ b/pkg/reconciler/pingsource/pingsource_test.go @@ -18,6 +18,9 @@ package pingsource import ( "context" + "fmt" + "knative.dev/eventing/pkg/apis/feature" + "knative.dev/eventing/pkg/auth" "os" "testing" @@ -141,6 +144,7 @@ func TestAllCases(t *testing.T) { rtv1.WithInitPingSourceConditions, rtv1.WithPingSourceStatusObservedGeneration(generation), rtv1.WithPingSourceSinkNotFound, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -184,6 +188,7 @@ func TestAllCases(t *testing.T) { rtv1.WithInitPingSourceConditions, rtv1.WithPingSourceStatusObservedGeneration(generation), rtv1.WithPingSourceSinkNotFound, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -239,6 +244,7 @@ func TestAllCases(t *testing.T) { rtv1.WithInitPingSourceConditions, rtv1.WithPingSourceSink(sinkAddressable), rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, }, { @@ -281,6 +287,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddressable), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -337,6 +344,7 @@ func TestAllCases(t *testing.T) { }), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -392,6 +400,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddressable), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -437,6 +446,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddressable), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -485,6 +495,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddressable), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }}, WantEvents: []string{ @@ -494,15 +505,127 @@ func TestAllCases(t *testing.T) { patchFinalizers(sourceName, testNS), }, }, + { + Name: "OIDC: creates OIDC service account", + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Enabled, + }), + Objects: []runtime.Object{ + rtv1.NewPingSource(sourceName, testNS, + rtv1.WithPingSourceSpec(sourcesv1.PingSourceSpec{ + Schedule: testSchedule, + ContentType: testContentType, + Data: testData, + SourceSpec: duckv1.SourceSpec{ + Sink: sinkDest, + }, + }), + rtv1.WithPingSource(sourceUID), + rtv1.WithPingSourceObjectMetaGeneration(generation), + ), + rtv1.NewChannel(sinkName, testNS, + rtv1.WithInitChannelConditions, + rtv1.WithChannelAddress(sinkAddressable), + ), + makeAvailableMTAdapter(), + }, + Key: testNS + "/" + sourceName, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: rtv1.NewPingSource(sourceName, testNS, + rtv1.WithPingSourceSpec(sourcesv1.PingSourceSpec{ + Schedule: testSchedule, + ContentType: testContentType, + Data: testData, + SourceSpec: duckv1.SourceSpec{ + Sink: sinkDest, + }, + }), + rtv1.WithPingSource(sourceUID), + rtv1.WithPingSourceObjectMetaGeneration(generation), + // Status Update: + rtv1.WithInitPingSourceConditions, + rtv1.WithPingSourceDeployed, + rtv1.WithPingSourceSink(sinkAddressable), + rtv1.WithPingSourceCloudEventAttributes, + rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedSucceeded(), + rtv1.WithPingSourceOIDCServiceAccountName(makePingSourceOIDCServiceAccount().Name), + ), + }}, + WantCreates: []runtime.Object{ + makePingSourceOIDCServiceAccount(), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(sourceName, testNS), + }, + }, + { + Name: "OIDC: PingSource not ready on invalid OIDC service account", + Ctx: feature.ToContext(context.Background(), feature.Flags{ + feature.OIDCAuthentication: feature.Enabled, + }), + Objects: []runtime.Object{ + makePingSourceOIDCServiceAccountWithoutOwnerRef(), + rtv1.NewPingSource(sourceName, testNS, + rtv1.WithPingSourceSpec(sourcesv1.PingSourceSpec{ + Schedule: testSchedule, + ContentType: testContentType, + Data: testData, + SourceSpec: duckv1.SourceSpec{ + Sink: sinkDest, + }, + }), + rtv1.WithPingSource(sourceUID), + rtv1.WithPingSourceObjectMetaGeneration(generation), + ), + rtv1.NewChannel(sinkName, testNS, + rtv1.WithInitChannelConditions, + rtv1.WithChannelAddress(sinkAddressable), + ), + makeAvailableMTAdapter(), + }, + Key: testNS + "/" + sourceName, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: rtv1.NewPingSource(sourceName, testNS, + rtv1.WithPingSourceSpec(sourcesv1.PingSourceSpec{ + Schedule: testSchedule, + ContentType: testContentType, + Data: testData, + SourceSpec: duckv1.SourceSpec{ + Sink: sinkDest, + }, + }), + rtv1.WithPingSource(sourceUID), + rtv1.WithPingSourceObjectMetaGeneration(generation), + // Status Update: + rtv1.WithInitPingSourceConditions, + rtv1.WithPingSourceStatusObservedGeneration(generation), + rtv1.WithPingSourceOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", fmt.Sprintf("service account %s not owned by PingSource %s", makePingSourceOIDCServiceAccountWithoutOwnerRef().Name, sourceName)), + rtv1.WithPingSourceOIDCServiceAccountName(makePingSourceOIDCServiceAccount().Name), + ), + }}, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", sourceName), + Eventf(corev1.EventTypeWarning, "InternalError", fmt.Sprintf("service account %s not owned by PingSource %s", makePingSourceOIDCServiceAccountWithoutOwnerRef().Name, sourceName)), + }, + WantPatches: []clientgotesting.PatchActionImpl{ + patchFinalizers(sourceName, testNS), + }, + }, } logger := logtesting.TestLogger(t) table.Test(t, rtv1.MakeFactory(func(ctx context.Context, listers *rtv1.Listers, cmw configmap.Watcher) controller.Reconciler { ctx = addressable.WithDuck(ctx) r := &Reconciler{ - configAcc: &reconcilersource.EmptyVarsGenerator{}, - kubeClientSet: fakekubeclient.Get(ctx), - tracker: tracker.New(func(types.NamespacedName) {}, 0), + configAcc: &reconcilersource.EmptyVarsGenerator{}, + kubeClientSet: fakekubeclient.Get(ctx), + tracker: tracker.New(func(types.NamespacedName) {}, 0), + serviceAccountLister: listers.GetServiceAccountLister(), } r.sinkResolver = resolver.NewURIResolverFromTracker(ctx, tracker.New(func(types.NamespacedName) {}, 0)) @@ -561,3 +684,22 @@ func patchFinalizers(name, namespace string) clientgotesting.PatchActionImpl { action.Patch = []byte(patch) return action } + +func makePingSourceOIDCServiceAccount() *corev1.ServiceAccount { + return auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("PingSource"), metav1.ObjectMeta{ + Name: sourceName, + Namespace: testNS, + UID: sourceUID, + }) +} + +func makePingSourceOIDCServiceAccountWithoutOwnerRef() *corev1.ServiceAccount { + sa := auth.GetOIDCServiceAccountForResource(sourcesv1.SchemeGroupVersion.WithKind("PingSource"), metav1.ObjectMeta{ + Name: sourceName, + Namespace: testNS, + UID: sourceUID, + }) + sa.OwnerReferences = nil + + return sa +} diff --git a/pkg/reconciler/testing/v1/pingsource.go b/pkg/reconciler/testing/v1/pingsource.go index 86ae5b329f4..41d73d21649 100644 --- a/pkg/reconciler/testing/v1/pingsource.go +++ b/pkg/reconciler/testing/v1/pingsource.go @@ -18,6 +18,8 @@ package testing import ( "context" + "fmt" + "knative.dev/eventing/pkg/apis/feature" "time" "knative.dev/eventing/pkg/reconciler/testing" @@ -107,3 +109,31 @@ func WithPingSourceDeleted(c *v1.PingSource) { t := metav1.NewTime(time.Unix(1e9, 0)) c.SetDeletionTimestamp(&t) } + +func WithPingSourceOIDCIdentityCreatedSucceeded() PingSourceOption { + return func(c *v1.PingSource) { + c.Status.MarkOIDCIdentityCreatedSucceeded() + } +} + +func WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled() PingSourceOption { + return func(c *v1.PingSource) { + c.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "") + } +} + +func WithPingSourceOIDCIdentityCreatedFailed(reason, message string) PingSourceOption { + return func(c *v1.PingSource) { + c.Status.MarkOIDCIdentityCreatedFailed(reason, message) + } +} + +func WithPingSourceOIDCServiceAccountName(name string) PingSourceOption { + return func(c *v1.PingSource) { + if c.Status.Auth == nil { + c.Status.Auth = &duckv1.AuthStatus{} + } + + c.Status.Auth.ServiceAccountName = &name + } +} From 17710165a976cb5f7f2bf9b9852c9accbae2cc52 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 11 Oct 2023 11:57:08 -0400 Subject: [PATCH 3/7] Fixed th failing unit tests --- pkg/apis/sources/v1/ping_lifecycle_test.go | 100 ++++++++++++++++----- 1 file changed, 80 insertions(+), 20 deletions(-) diff --git a/pkg/apis/sources/v1/ping_lifecycle_test.go b/pkg/apis/sources/v1/ping_lifecycle_test.go index 266005baeae..9397e8e5952 100644 --- a/pkg/apis/sources/v1/ping_lifecycle_test.go +++ b/pkg/apis/sources/v1/ping_lifecycle_test.go @@ -116,24 +116,28 @@ func TestPingSourceStatusIsReady(t *testing.T) { oidcServiceAccountStatus: true, }, { - name: "oidc status false with mark sink and deployed", + name: "oidc status false", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() s.MarkSink(exampleAddr) s.PropagateDeploymentAvailability(availableDeployment) - s.MarkOIDCIdentityCreatedSucceeded() return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: true, + wantConditionStatus: corev1.ConditionFalse, + want: false, oidcServiceAccountStatus: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - ss := &PingSourceStatus{} + if test.oidcServiceAccountStatus { + test.s.MarkOIDCIdentityCreatedSucceeded() + } else { + test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") + } + if test.wantConditionStatus != "" { gotConditionStatus := test.s.GetTopLevelCondition().Status if gotConditionStatus != test.wantConditionStatus { @@ -144,11 +148,7 @@ func TestPingSourceStatusIsReady(t *testing.T) { if got != test.want { t.Errorf("unexpected readiness: want %v, got %v", test.want, got) } - if test.oidcServiceAccountStatus { - ss.MarkOIDCIdentityCreatedSucceeded() - } else { - ss.MarkOIDCIdentityCreatedFailed("Unable to ...", "") - } + }) } } @@ -160,13 +160,18 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - want *apis.Condition + name string + s *PingSourceStatus + want *apis.Condition + oidcServiceAccountStatus bool }{{ name: "uninitialized", s: &PingSourceStatus{}, - want: nil, + want: &apis.Condition{ + Type: PingSourceConditionReady, + Status: corev1.ConditionUnknown, + }, + oidcServiceAccountStatus: true, }, { name: "initialized", s: func() *PingSourceStatus { @@ -178,6 +183,7 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, }, { name: "mark deployed", s: func() *PingSourceStatus { @@ -190,6 +196,7 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, }, { name: "mark sink", s: func() *PingSourceStatus { @@ -202,6 +209,7 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, }, { name: "mark sink and deployed", s: func() *PingSourceStatus { @@ -215,10 +223,34 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionTrue, }, - }} + oidcServiceAccountStatus: true, + }, + { + name: "oidc fail", + s: func() *PingSourceStatus { + s := &PingSourceStatus{} + s.InitializeConditions() + s.MarkSink(exampleAddr) + s.PropagateDeploymentAvailability(availableDeployment) + return s + }(), + want: &apis.Condition{ + Type: PingSourceConditionReady, + Status: corev1.ConditionFalse, + Reason: "Unable to ...", + }, + oidcServiceAccountStatus: false, + }, + } for _, test := range tests { t.Run(test.name, func(t *testing.T) { + if test.oidcServiceAccountStatus { + test.s.MarkOIDCIdentityCreatedSucceeded() + } else { + test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") + } + got := test.s.GetTopLevelCondition() ignoreTime := cmpopts.IgnoreFields(apis.Condition{}, "LastTransitionTime", "Severity") @@ -236,15 +268,20 @@ func TestPingSourceStatusGetCondition(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - condQuery apis.ConditionType - want *apis.Condition + name string + s *PingSourceStatus + condQuery apis.ConditionType + want *apis.Condition + oidcServiceAccountStatus bool }{{ name: "uninitialized", s: &PingSourceStatus{}, condQuery: PingSourceConditionReady, - want: nil, + want: &apis.Condition{ + Type: PingSourceConditionReady, + Status: corev1.ConditionUnknown, + }, + oidcServiceAccountStatus: true, }, { name: "initialized", s: func() *PingSourceStatus { @@ -257,6 +294,7 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, }, { name: "mark deployed", s: func() *PingSourceStatus { @@ -270,6 +308,7 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, }, { name: "mark sink", s: func() *PingSourceStatus { @@ -283,10 +322,31 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, + oidcServiceAccountStatus: true, + }, { + name: "oidc failed", + s: func() *PingSourceStatus { + s := &PingSourceStatus{} + s.InitializeConditions() + s.MarkSink(exampleAddr) + return s + }(), + condQuery: PingSourceConditionReady, + want: &apis.Condition{ + Type: PingSourceConditionReady, + Status: corev1.ConditionFalse, + Reason: "Unable to ...", + }, + oidcServiceAccountStatus: false, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { + if test.oidcServiceAccountStatus { + test.s.MarkOIDCIdentityCreatedSucceeded() + } else { + test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") + } got := test.s.GetCondition(test.condQuery) ignoreTime := cmpopts.IgnoreFields(apis.Condition{}, "LastTransitionTime", "Severity") From ef2afbea4d754f41743dcd0d26c866052b0b6c36 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 16 Oct 2023 11:15:13 -0400 Subject: [PATCH 4/7] lint fix --- pkg/reconciler/pingsource/controller.go | 13 ++++++++++++- pkg/reconciler/pingsource/pingsource.go | 1 + pkg/reconciler/pingsource/pingsource_test.go | 5 +++-- pkg/reconciler/testing/v1/pingsource.go | 3 ++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/pingsource/controller.go b/pkg/reconciler/pingsource/controller.go index 6298027d16c..69e6fd0363d 100644 --- a/pkg/reconciler/pingsource/controller.go +++ b/pkg/reconciler/pingsource/controller.go @@ -18,6 +18,7 @@ package pingsource import ( "context" + serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount" "go.uber.org/zap" @@ -61,7 +62,13 @@ func NewController( logger.Fatalw("Error converting leader election configuration to JSON", zap.Error(err)) } - featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store")) + var globalResync func(obj interface{}) + + featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) { + if globalResync != nil { + globalResync(nil) + } + }) featureStore.WatchConfigs(cmw) // Configure the reconciler @@ -83,6 +90,10 @@ func NewController( } }) + globalResync = func(interface{}) { + impl.GlobalResync(pingSourceInformer.Informer()) + } + r.sinkResolver = resolver.NewURIResolver(ctx, cmw, impl.Tracker) pingSourceInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) diff --git a/pkg/reconciler/pingsource/pingsource.go b/pkg/reconciler/pingsource/pingsource.go index 6253d259dee..1bad1df76f5 100644 --- a/pkg/reconciler/pingsource/pingsource.go +++ b/pkg/reconciler/pingsource/pingsource.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + v1 "k8s.io/client-go/listers/core/v1" "knative.dev/eventing/pkg/apis/feature" "knative.dev/eventing/pkg/auth" diff --git a/pkg/reconciler/pingsource/pingsource_test.go b/pkg/reconciler/pingsource/pingsource_test.go index 0c3c206e134..4e84a050926 100644 --- a/pkg/reconciler/pingsource/pingsource_test.go +++ b/pkg/reconciler/pingsource/pingsource_test.go @@ -19,11 +19,12 @@ package pingsource import ( "context" "fmt" - "knative.dev/eventing/pkg/apis/feature" - "knative.dev/eventing/pkg/auth" "os" "testing" + "knative.dev/eventing/pkg/apis/feature" + "knative.dev/eventing/pkg/auth" + cloudevents "github.com/cloudevents/sdk-go/v2" "k8s.io/utils/pointer" diff --git a/pkg/reconciler/testing/v1/pingsource.go b/pkg/reconciler/testing/v1/pingsource.go index 41d73d21649..e930ed46027 100644 --- a/pkg/reconciler/testing/v1/pingsource.go +++ b/pkg/reconciler/testing/v1/pingsource.go @@ -19,9 +19,10 @@ package testing import ( "context" "fmt" - "knative.dev/eventing/pkg/apis/feature" "time" + "knative.dev/eventing/pkg/apis/feature" + "knative.dev/eventing/pkg/reconciler/testing" duckv1 "knative.dev/pkg/apis/duck/v1" From 22f6ba2ae81a93f1445d3e8b5d8f2f3d6f3ccf9f Mon Sep 17 00:00:00 2001 From: Leo Li Date: Mon, 16 Oct 2023 11:28:51 -0400 Subject: [PATCH 5/7] Fix the failed unit tests --- pkg/adapter/mtping/pingsource_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/adapter/mtping/pingsource_test.go b/pkg/adapter/mtping/pingsource_test.go index 8038d018a1b..f3fb45ee129 100644 --- a/pkg/adapter/mtping/pingsource_test.go +++ b/pkg/adapter/mtping/pingsource_test.go @@ -89,6 +89,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceDeployed, rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, @@ -112,6 +113,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceDeployed, rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, @@ -137,6 +139,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceDeployed, rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, @@ -162,6 +165,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceDeployed, rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, @@ -188,6 +192,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceDeleted, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, @@ -210,6 +215,7 @@ func TestAllCases(t *testing.T) { rtv1.WithPingSourceSink(sinkAddr), rtv1.WithPingSourceCloudEventAttributes, rtv1.WithPingSourceDeleted, + rtv1.WithPingSourceOIDCIdentityCreatedSucceededBecauseOIDCFeatureDisabled(), ), }, WantErr: false, From b8ee3a35bb6dffa1997e6099ce8548e9a7661204 Mon Sep 17 00:00:00 2001 From: Leo Li Date: Tue, 17 Oct 2023 17:24:51 -0400 Subject: [PATCH 6/7] Instead of having a new bool var, we mark oidc status when running the test --- pkg/apis/sources/v1/ping_lifecycle_test.go | 112 ++++++++------------- 1 file changed, 42 insertions(+), 70 deletions(-) diff --git a/pkg/apis/sources/v1/ping_lifecycle_test.go b/pkg/apis/sources/v1/ping_lifecycle_test.go index 9397e8e5952..1c5bd337d26 100644 --- a/pkg/apis/sources/v1/ping_lifecycle_test.go +++ b/pkg/apis/sources/v1/ping_lifecycle_test.go @@ -59,84 +59,76 @@ func TestPingSourceStatusIsReady(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - wantConditionStatus corev1.ConditionStatus - want bool - oidcServiceAccountStatus bool + name string + s *PingSourceStatus + wantConditionStatus corev1.ConditionStatus + want bool }{{ - name: "uninitialized", - s: &PingSourceStatus{}, - want: false, - oidcServiceAccountStatus: true, + name: "uninitialized", + s: &PingSourceStatus{}, + want: false, }, { name: "initialized", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, - oidcServiceAccountStatus: true, + wantConditionStatus: corev1.ConditionUnknown, + want: false, }, { name: "mark deployed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.PropagateDeploymentAvailability(availableDeployment) return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, - oidcServiceAccountStatus: true, + wantConditionStatus: corev1.ConditionUnknown, + want: false, }, { name: "mark sink", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() - + s.MarkOIDCIdentityCreatedSucceeded() s.MarkSink(exampleAddr) return s }(), - wantConditionStatus: corev1.ConditionUnknown, - want: false, - oidcServiceAccountStatus: true, + wantConditionStatus: corev1.ConditionUnknown, + want: false, }, { name: "mark sink and deployed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.MarkSink(exampleAddr) s.PropagateDeploymentAvailability(availableDeployment) return s }(), - wantConditionStatus: corev1.ConditionTrue, - want: true, - oidcServiceAccountStatus: true, + wantConditionStatus: corev1.ConditionTrue, + want: true, }, { name: "oidc status false", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedFailed("Unable to create the OIDC identity", "") s.MarkSink(exampleAddr) s.PropagateDeploymentAvailability(availableDeployment) return s }(), - wantConditionStatus: corev1.ConditionFalse, - want: false, - oidcServiceAccountStatus: false, + wantConditionStatus: corev1.ConditionFalse, + want: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.oidcServiceAccountStatus { - test.s.MarkOIDCIdentityCreatedSucceeded() - } else { - test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") - } if test.wantConditionStatus != "" { gotConditionStatus := test.s.GetTopLevelCondition().Status @@ -160,35 +152,31 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - want *apis.Condition - oidcServiceAccountStatus bool + name string + s *PingSourceStatus + want *apis.Condition }{{ name: "uninitialized", s: &PingSourceStatus{}, - want: &apis.Condition{ - Type: PingSourceConditionReady, - Status: corev1.ConditionUnknown, - }, - oidcServiceAccountStatus: true, + want: nil, }, { name: "initialized", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() return s }(), want: &apis.Condition{ Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "mark deployed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.PropagateDeploymentAvailability(availableDeployment) return s }(), @@ -196,12 +184,12 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "mark sink", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.MarkSink(exampleAddr) return s }(), @@ -209,12 +197,12 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "mark sink and deployed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.MarkSink(exampleAddr) s.PropagateDeploymentAvailability(availableDeployment) return s @@ -223,13 +211,13 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionTrue, }, - oidcServiceAccountStatus: true, }, { name: "oidc fail", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedFailed("Unable to create the OIDC identity", "") s.MarkSink(exampleAddr) s.PropagateDeploymentAvailability(availableDeployment) return s @@ -237,19 +225,13 @@ func TestPingSourceStatusGetTopLevelCondition(t *testing.T) { want: &apis.Condition{ Type: PingSourceConditionReady, Status: corev1.ConditionFalse, - Reason: "Unable to ...", + Reason: "Unable to create the OIDC identity", }, - oidcServiceAccountStatus: false, }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.oidcServiceAccountStatus { - test.s.MarkOIDCIdentityCreatedSucceeded() - } else { - test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") - } got := test.s.GetTopLevelCondition() ignoreTime := cmpopts.IgnoreFields(apis.Condition{}, @@ -268,25 +250,21 @@ func TestPingSourceStatusGetCondition(t *testing.T) { } tests := []struct { - name string - s *PingSourceStatus - condQuery apis.ConditionType - want *apis.Condition - oidcServiceAccountStatus bool + name string + s *PingSourceStatus + condQuery apis.ConditionType + want *apis.Condition }{{ name: "uninitialized", s: &PingSourceStatus{}, condQuery: PingSourceConditionReady, - want: &apis.Condition{ - Type: PingSourceConditionReady, - Status: corev1.ConditionUnknown, - }, - oidcServiceAccountStatus: true, + want: nil, }, { name: "initialized", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() return s }(), condQuery: PingSourceConditionReady, @@ -294,12 +272,12 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "mark deployed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.PropagateDeploymentAvailability(availableDeployment) return s }(), @@ -308,12 +286,12 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "mark sink", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedSucceeded() s.MarkSink(exampleAddr) return s }(), @@ -322,12 +300,12 @@ func TestPingSourceStatusGetCondition(t *testing.T) { Type: PingSourceConditionReady, Status: corev1.ConditionUnknown, }, - oidcServiceAccountStatus: true, }, { name: "oidc failed", s: func() *PingSourceStatus { s := &PingSourceStatus{} s.InitializeConditions() + s.MarkOIDCIdentityCreatedFailed("Unable to create the OIDC identity", "") s.MarkSink(exampleAddr) return s }(), @@ -335,18 +313,12 @@ func TestPingSourceStatusGetCondition(t *testing.T) { want: &apis.Condition{ Type: PingSourceConditionReady, Status: corev1.ConditionFalse, - Reason: "Unable to ...", + Reason: "Unable to create the OIDC identity", }, - oidcServiceAccountStatus: false, }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { - if test.oidcServiceAccountStatus { - test.s.MarkOIDCIdentityCreatedSucceeded() - } else { - test.s.MarkOIDCIdentityCreatedFailed("Unable to ...", "") - } got := test.s.GetCondition(test.condQuery) ignoreTime := cmpopts.IgnoreFields(apis.Condition{}, "LastTransitionTime", "Severity") From e9447dd09b04955b6e85a434e6ea1dcbc74ffdaf Mon Sep 17 00:00:00 2001 From: Leo Li Date: Wed, 18 Oct 2023 14:00:47 -0400 Subject: [PATCH 7/7] Fixing the comments regarding the serviceaccountInformer --- pkg/reconciler/pingsource/controller.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/pingsource/controller.go b/pkg/reconciler/pingsource/controller.go index 69e6fd0363d..724908e6a67 100644 --- a/pkg/reconciler/pingsource/controller.go +++ b/pkg/reconciler/pingsource/controller.go @@ -19,6 +19,8 @@ package pingsource import ( "context" + sourcesv1 "knative.dev/eventing/pkg/apis/sources/v1" + serviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount" "go.uber.org/zap" @@ -112,7 +114,7 @@ func NewController( }) serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{ - FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), mtadapterName), + FilterFunc: controller.FilterController(&sourcesv1.PingSource{}), Handler: controller.HandleAll(impl.EnqueueControllerOf), })