support auto generation of SinkBindings identity service account #7327

rahulii · 2023-10-02T10:55:19Z

Proposed Changes

🎁 Expose the name of the OIDC service account in the SinkBinding .status.auth.serviceAccountName
🎁 Create the OIDC service account of the SinkBinding

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Expose the SinkBinding OIDC service account name in the SinkBinding .status.auth.serviceAccountName

knative-prow · 2023-10-02T10:55:27Z

Welcome @rahulii! It looks like this is your first PR to knative/eventing 🎉

knative-prow · 2023-10-02T10:55:28Z

Hi @rahulii. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rahulii · 2023-10-02T10:56:40Z

@creydr please review.
Thanks!

creydr

Hi @rahulii,
Thanks a lot for your PR. I left 2 comments so far.
Besides of them, I am getting a crashloop backoff when running your PR locally. Seems like some insufficient permissions. Can you check on them?

pkg/reconciler/sinkbinding/controller.go

creydr · 2023-10-04T09:19:26Z

/ok-to-test

rahulii · 2023-10-04T12:06:40Z

status:
    auth:
      serviceAccountName: oidc-sources.knative.dev-sinkbinding-heartbeat
    conditions:
    - lastTransitionTime: "2023-10-04T12:04:51Z"
      status: "True"
      type: OIDCIdentityCreated
    - lastTransitionTime: "2023-10-04T12:04:52Z"
      message: Addressable could not be extracted from destination
      reason: NoAddressable
      status: "False"
      type: Ready
    - lastTransitionTime: "2023-10-04T12:04:51Z"
      status: Unknown
      type: SinkProvided

here is the o/p - @creydr

pkg/reconciler/sinkbinding/controller.go

creydr

Can you also fix the goformat/style issues?

config/core/roles/webhook-clusterrole.yaml

creydr · 2023-10-05T10:59:46Z

@rahulii Thanks for the update. Can you fix the unit tests?
Can we also add some reconciler tests, to check if the service account was created and the AuthStatus field set correctly? Something like we did in 0eaf9e8:

eventing/pkg/reconciler/broker/trigger/trigger_test.go

Lines 1489 to 1558 in 402f6ac

    
           { 
        
           	Name: "OIDC: creates OIDC service account", 
        
           	Key:  testKey, 
        
           	Ctx: feature.ToContext(context.Background(), feature.Flags{ 
        
           		feature.OIDCAuthentication: feature.Enabled, 
        
           	}), 
        
           	Objects: allBrokerObjectsReadyPlus([]runtime.Object{ 
        
           		makeReadySubscriptionWithAudience(testNS), 
        
           		NewTrigger(triggerName, testNS, brokerName, 
        
           			WithTriggerUID(triggerUID), 
        
           			WithTriggerSubscriberURI(subscriberURI), 
        
           			WithInitTriggerConditions, 
        
           		)}...), 
        
           	WantErr: false, 
        
           	WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ 
        
           		Object: NewTrigger(triggerName, testNS, brokerName, 
        
           			WithTriggerUID(triggerUID), 
        
           			WithTriggerSubscriberURI(subscriberURI), 
        
           			WithTriggerBrokerReady(), 
        
           			// The first reconciliation will initialize the status conditions. 
        
           			WithInitTriggerConditions, 
        
           			WithTriggerDependencyReady(), 
        
           			WithTriggerSubscribed(), 
        
           			WithTriggerStatusSubscriberURI(subscriberURI), 
        
           			WithTriggerSubscriberResolvedSucceeded(), 
        
           			WithTriggerDeadLetterSinkNotConfigured(), 
        
           			WithTriggerOIDCIdentityCreatedSucceeded(), 
        
           			WithTriggerOIDCServiceAccountName(makeTriggerOIDCServiceAccount().Name), 
        
           		), 
        
           	}}, 
        
           	WantCreates: []runtime.Object{ 
        
           		makeTriggerOIDCServiceAccount(), 
        
           	}, 
        
           }, 
        
           { 
        
           	Name: "OIDC: Trigger not ready on invalid OIDC service account", 
        
           	Key:  testKey, 
        
           	Ctx: feature.ToContext(context.Background(), feature.Flags{ 
        
           		feature.OIDCAuthentication: feature.Enabled, 
        
           	}), 
        
           	Objects: allBrokerObjectsReadyPlus([]runtime.Object{ 
        
           		makeReadySubscriptionWithAudience(testNS), 
        
           		makeTriggerOIDCServiceAccountWithoutOwnerRef(), 
        
           		NewTrigger(triggerName, testNS, brokerName, 
        
           			WithTriggerUID(triggerUID), 
        
           			WithTriggerSubscriberURI(subscriberURI), 
        
           			WithInitTriggerConditions, 
        
           		)}...), 
        
           	WantErr: true, 
        
           	WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ 
        
           		Object: NewTrigger(triggerName, testNS, brokerName, 
        
           			WithTriggerUID(triggerUID), 
        
           			WithTriggerSubscriberURI(subscriberURI), 
        
           			WithTriggerBrokerReady(), 
        
           			// The first reconciliation will initialize the status conditions. 
        
           			WithInitTriggerConditions, 
        
           			WithTriggerDependencyUnknown("", ""), 
        
           			WithTriggerSubscribed(), 
        
           			WithTriggerStatusSubscriberURI(subscriberURI), 
        
           			WithTriggerSubscriberResolvedSucceeded(), 
        
           			WithTriggerDeadLetterSinkNotConfigured(), 
        
           			WithTriggerOIDCIdentityCreatedFailed("Unable to resolve service account for OIDC authentication", fmt.Sprintf("service account %s not owned by Trigger %s", makeTriggerOIDCServiceAccountWithoutOwnerRef().Name, triggerName)), 
        
           			WithTriggerOIDCServiceAccountName(makeTriggerOIDCServiceAccountWithoutOwnerRef().Name), 
        
           			WithTriggerSubscribedUnknown("", ""), 
        
           		), 
        
           	}}, 
        
           	WantEvents: []string{ 
        
           		Eventf(corev1.EventTypeWarning, "InternalError", fmt.Sprintf("service account %s not owned by Trigger %s", makeTriggerOIDCServiceAccountWithoutOwnerRef().Name, triggerName)), 
        
           	}, 
        
           },

pkg/apis/sources/v1/sinkbinding_lifecycle.go

rahulii · 2023-10-05T11:32:33Z

Can we also add some reconciler tests, to check if the service account was created and the AuthStatus field set correctly?

@creydr there are no tests implemented currently for SB reconciler, should I go ahead and create it (a new _test.go file) ?

codecov · 2023-10-05T11:37:23Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (ba02f4a) 76.90% compared to head (fdda99e) 76.92%.
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7327      +/-   ##
==========================================
+ Coverage   76.90%   76.92%   +0.02%     
==========================================
  Files         252      252              
  Lines       13693    13701       +8     
==========================================
+ Hits        10530    10539       +9     
  Misses       2637     2637              
+ Partials      526      525       -1

Files	Coverage Δ
pkg/apis/sources/v1/sinkbinding_types.go	`100.00% <ø> (ø)`
pkg/apis/sources/v1/sinkbinding_lifecycle.go	`89.28% <75.00%> (-1.10%)`	⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pierDipi · 2023-10-09T16:47:03Z

pkg/reconciler/sinkbinding/controller.go

+		sb.Status.MarkOIDCIdentityCreatedSucceeded()
+	} else {
+		sb.Status.Auth = nil
+		sb.Status.MarkOIDCIdentityCreatedSucceededWithReason(fmt.Sprintf("%s feature disabled", feature.OIDCAuthentication), "")


Do we have a constant string we can use across the board for this reason? /cc @creydr

Not yet. But makes sense to introduce one, as we have this at multiple places 👍

I think that can be a part of seperate PR!

pierDipi · 2023-10-09T16:49:51Z

pkg/reconciler/sinkbinding/controller.go

+	// Reconcile SinkBinding when the OIDC service account changes
+	serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: controller.FilterController(&v1.SinkBinding{}),
+		Handler:    controller.HandleAll(impl.EnqueueControllerOf),
+	})
+


@creydr it's a general common not specific to this PR, but we should evaluate if it makes sense to label all of our service accounts and use a "filtered informer" so that we're not watching all service accounts into a cluster which could be quite large but only "ours"

I think it makes sense to label the service account 👍🏼

That's a good point @pierDipi. I think we can do this in a separate PR

created #7341

pierDipi · 2023-10-09T16:53:13Z

pkg/reconciler/sinkbinding/controller.go

+	// reconcile sinkindings on changes on the features configmap
+	configmapInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
+		FilterFunc: controller.FilterWithNameAndNamespace(system.Namespace(), feature.FlagsConfigName),
+		Handler: controller.HandleAll(func(i interface{}) {
+			impl.GlobalResync(sbInformer.Informer())
+		}),
+	})


Instead of using a cluster-wide configmap informer, we can configure a callback function when calling NewStore, that saves quite a bit of memory since that watcher is watching only configmaps in the system namespace as opposed to watching every configmap in the cluster /cc @creydr

see example in pkg/reconciler/broker/controller.go

like featureStore.WatchConfigs(cmw) , right ?

yes. featureStore.WatchConfigs(cmw) takes additional callback functions which get called on configmap updates. E.g.:

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) } })

with globalResync being something like:

globalResync = func(obj interface{}) { impl.GlobalResync(sbInformer.Informer()) }

thanks @creydr

rahulii · 2023-10-10T07:12:43Z

/ok-to-test

creydr · 2023-10-10T07:49:08Z

@rahulii maybe #7285 (comment) helps you too on your e2e test 🤷

rahulii · 2023-10-10T08:05:20Z

@rahulii maybe #7285 (comment) helps you too on your e2e test 🤷

hmm @creydr do you want to do e2e as part of this PR or create a bug and track it ?
quoting @pierDipi from slack :

For E2E tests, we need reconciler-test support for OIDC testing and we're not there yet, so we can track the remaining test part into a different issue and merge this PR ones ready

wdyt?

creydr · 2023-10-10T08:09:26Z

@rahulii maybe #7285 (comment) helps you too on your e2e test 🤷

hmm @creydr do you want to do e2e as part of this PR or create a bug and track it ? quoting @pierDipi from slack :
For E2E tests, we need reconciler-test support for OIDC testing and we're not there yet, so we can track the remaining test part into a different issue and merge this PR ones ready
wdyt?

I see. Works for me.

rahulii · 2023-10-10T08:14:58Z

@rahulii maybe #7285 (comment) helps you too on your e2e test 🤷

hmm @creydr do you want to do e2e as part of this PR or create a bug and track it ? quoting @pierDipi from slack :
For E2E tests, we need reconciler-test support for OIDC testing and we're not there yet, so we can track the remaining test part into a different issue and merge this PR ones ready
wdyt?
I see. Works for me.

thanks @creydr , LMK if you need any more changes on this PR! Thanks!

creydr · 2023-10-10T08:24:50Z

@rahulii the verify job failed. Probably because some relevant changes got merged into main in the meantime. Can you rerun ./hack/update-codegen.sh.?

Signed-off-by: rahulii <r.sawra@gmail.com>

rahulii · 2023-10-11T05:47:56Z

@creydr is the e2e test failure because of this PR changes ? 🤔
doen't looks like ! If no, is this PR good to go ?

creydr

Thanks a lot @rahulii for your work in this PR and your patience!

/lgtm

knative-prow · 2023-10-11T05:52:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, rahulii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [creydr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 2, 2023

knative-prow bot requested review from lionelvillard and matzew October 2, 2023 10:55

creydr requested changes Oct 4, 2023

View reviewed changes

pkg/reconciler/sinkbinding/controller.go Outdated Show resolved Hide resolved

pkg/reconciler/sinkbinding/controller.go Outdated Show resolved Hide resolved

knative-prow bot assigned creydr Oct 4, 2023

creydr reviewed Oct 4, 2023

View reviewed changes

pkg/reconciler/sinkbinding/controller.go Outdated Show resolved Hide resolved

rahulii requested a review from creydr October 4, 2023 12:07

creydr reviewed Oct 4, 2023

View reviewed changes

pkg/reconciler/sinkbinding/controller.go Outdated Show resolved Hide resolved

pkg/reconciler/sinkbinding/controller.go Outdated Show resolved Hide resolved

pkg/reconciler/sinkbinding/controller.go Show resolved Hide resolved

creydr reviewed Oct 4, 2023

View reviewed changes

config/core/roles/webhook-clusterrole.yaml Show resolved Hide resolved

config/core/roles/webhook-clusterrole.yaml Outdated Show resolved Hide resolved

creydr reviewed Oct 5, 2023

View reviewed changes

pkg/apis/sources/v1/sinkbinding_lifecycle.go Outdated Show resolved Hide resolved

pierDipi reviewed Oct 9, 2023

View reviewed changes

rahulii force-pushed the support-oidc-sinkbinding branch from f214983 to 9800d17 Compare October 9, 2023 20:06

rahulii requested a review from creydr October 9, 2023 20:08

rahulii added 11 commits October 10, 2023 13:57

support auto generation of SinkBindings identity service account

71d68a4

Signed-off-by: rahulii <r.sawra@gmail.com>

pass serviceAccountLister to reconciler

6409b61

Signed-off-by: rahulii <r.sawra@gmail.com>

give service account permission and minor fixes

88f52b4

Signed-off-by: rahulii <r.sawra@gmail.com>

fix review comments

7e48b41

Signed-off-by: rahulii <r.sawra@gmail.com>

fix linting and review comments

910f581

Signed-off-by: rahulii <r.sawra@gmail.com>

fix review comments

426c975

Signed-off-by: rahulii <r.sawra@gmail.com>

fix test

d0c24d0

Signed-off-by: rahulii <r.sawra@gmail.com>

review comments

650ae20

Signed-off-by: rahulii <r.sawra@gmail.com>

add handler to feature store

5bf370c

Signed-off-by: rahulii <r.sawra@gmail.com>

fix lint

ac09c62

Signed-off-by: rahulii <r.sawra@gmail.com>

remove comment

fdda99e

Signed-off-by: rahulii <r.sawra@gmail.com>

rahulii force-pushed the support-oidc-sinkbinding branch from cf83f7d to fdda99e Compare October 10, 2023 08:47

creydr mentioned this pull request Oct 10, 2023

Support auto generation of Subscriptions identity service account and… #7338

Merged

5 tasks

creydr approved these changes Oct 11, 2023

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2023

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 11, 2023

knative-prow bot merged commit c46bdd3 into knative:main Oct 11, 2023
35 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support auto generation of SinkBindings identity service account #7327

support auto generation of SinkBindings identity service account #7327

rahulii commented Oct 2, 2023

knative-prow bot commented Oct 2, 2023

knative-prow bot commented Oct 2, 2023

rahulii commented Oct 2, 2023

creydr left a comment

creydr commented Oct 4, 2023

rahulii commented Oct 4, 2023

creydr left a comment

creydr commented Oct 5, 2023

rahulii commented Oct 5, 2023 •

edited

Loading

codecov bot commented Oct 5, 2023 •

edited

Loading

pierDipi Oct 9, 2023

creydr Oct 9, 2023

rahulii Oct 9, 2023

pierDipi Oct 9, 2023

rahulii Oct 9, 2023

creydr Oct 10, 2023

creydr Oct 10, 2023

pierDipi Oct 9, 2023 •

edited

Loading

pierDipi Oct 9, 2023

rahulii Oct 9, 2023

creydr Oct 10, 2023

rahulii Oct 10, 2023

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 11, 2023 •

edited

Loading

creydr left a comment

knative-prow bot commented Oct 11, 2023

support auto generation of SinkBindings identity service account #7327

support auto generation of SinkBindings identity service account #7327

Conversation

rahulii commented Oct 2, 2023

Proposed Changes

Pre-review Checklist

knative-prow bot commented Oct 2, 2023

knative-prow bot commented Oct 2, 2023

rahulii commented Oct 2, 2023

creydr left a comment

Choose a reason for hiding this comment

creydr commented Oct 4, 2023

rahulii commented Oct 4, 2023

creydr left a comment

Choose a reason for hiding this comment

creydr commented Oct 5, 2023

rahulii commented Oct 5, 2023 • edited Loading

codecov bot commented Oct 5, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierDipi Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 10, 2023

creydr commented Oct 10, 2023

rahulii commented Oct 11, 2023 • edited Loading

creydr left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 11, 2023

rahulii commented Oct 5, 2023 •

edited

Loading

codecov bot commented Oct 5, 2023 •

edited

Loading

pierDipi Oct 9, 2023 •

edited

Loading

rahulii commented Oct 11, 2023 •

edited

Loading