Use filtered informer to watch OIDC service accounts #7527

yijie-04 · 2024-01-02T21:14:15Z

Fixes #7341

Proposed Changes

🎁 labelled the OIDC service accounts and used a filtered serviceaccount informer based on the label

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

Docs

This reverts commit 94cd51b, reversing changes made to 0bf2982.

linux-foundation-easycla · 2024-01-02T21:14:20Z

The committers listed above are authorized under a signed CLA.

✅ login: yijie-04 / name: Yijie Wang (0bf2982, 94cd51b, a82c2aa, d4bfe4e, 979911d, bbefcc2, de30fc5, ce5a778, 2ae1090, 695e58c, efc1cc3, 37f0dff, a414f3e, de787c6, c365d4a, 7d73360, c048610, 3d3bd2c, f5d583f, d934231, 08dbe1e, d3205ab, a31fc39, b69cc29, 51befc7, e95329e, e76634a, 8e112d8, c04375a, d7f6e43, f2fe553, b2941ac, 5244f19, 2c94ec7, 3d0d399, 8c30426, bee47f4, 5c25449, 63346a6, 9a44a73)
✅ login: Zazzscoot / name: Scott Hirano (17c775a)

knative-prow · 2024-01-02T21:14:24Z

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

knative-prow · 2024-01-02T21:14:25Z

Hi @yijie-04. 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.

Leo6Leo · 2024-01-03T14:01:49Z

@yijie-04 Thanks for the PR!
As this is your very first PR in the LFX(linux fundation) community, please make sure you complete the EasyCLA so that you are authorized to contribute to Knative.

Here is the link on what is EasyCLA and how to complete it.

Leo6Leo · 2024-01-05T19:44:45Z

@yijie-04 Replying your DM here so others can also benefit from this! Usually when you complete the EasyCLA authorization, check in the GitHub action should turn green immediately. If that doesn't change, there is probably something wrong. Please make sure the github account you used for this PR and the one you used with CLA are the same. Let me know if that helps!

Leo6Leo

/ok-to-test

Thanks @yijie-04 for the PR! Just left few comments. PTAL whenever you have time!

unfiltered service account informers for OIDC service accounts are currently used in multiple places. Most of the fixed Support auto generation of XYZ identity service account and expose in AuthStatus issues in https://github.com/orgs/knative/projects/66/views/4 will probably use it

As @creydr mentioned, there are multiple places are using the unfiltered service account informers. Could you try to update them to use filtered informers too?

Great work Yijie, and please let me know if you need any help!

pkg/reconciler/broker/trigger/controller.go

…ount

Leo6Leo · 2024-01-08T14:33:27Z

@yijie-04 Hello Yijie! Are there anything that blocking you? Any help I can provide you with?

yijie-04 · 2024-01-10T04:35:21Z

@Leo6Leo Thank you for checking in! It took me a bit of time to debug the unit-tests and it turned out to be simpler than I thought! I'm looking at the other tests right now and it seems that I'm receiving the timeout waiting for pods to come up error. Do you have any suggestions on how I can approach this?

Leo6Leo · 2024-01-10T19:09:11Z

@yijie-04 Hey Yijie, no problem, that's happened to me a lot too! Replying to your question, you can always click into the details for the failed prow test, and click on open Raw build-log.txt to gain the full error logs. For example, this is the real reason why your test is failing. Link

And as prow suggested, you will need to rebase the PR as some newly merged changes on the main branch is conflicting with your current changes.

See the screenshot on how to find the

"Raw build-log.txt`

lmk if you have any other questions!

…est files

Leo6Leo · 2024-01-26T20:48:29Z

@yijie-04 There are still some goimports issue in your PR.

To fix the formatting issue, here are some practical commands you should be aware of:

go fmt ./... to format all the files in the project
goimports -w ./... will run go imports for all the files

When you are running these commands, you should be careful that we don't want to format the files in the vendor folder and the third_party folder.

And there is a convenient tool that I have made to combine the gofmt and goimport to a single command. Take a look at the source code to understand how that work and Just make sure run this script every time before you commit!

Cali0707 · 2024-01-27T00:23:06Z

/retest-required
Looks like the failure is a flaky test @yijie-04 - don't worry about it

yijie-04 · 2024-01-27T01:46:40Z

@Cali0707 @Leo6Leo Got it, thank you! I just finished formatting the files and I'll wait to see the test results. I do have some questions as I was formatting the code:

I was running the commands for formatting earlier but nothing got changed (I also didn't receive any errors). However, after I had minikube running, the files were properly formatted. Why would formatting Go code require setting up a Kubernetes environment on my system?
I'm also curious about the difference between the two commands, go fmt and goimports. It seems that all the files were formatted after running go fmt ./..., so what's the use of goimports?

Thanks a lot!

Leo6Leo · 2024-01-29T05:44:03Z

Hey @yijie-04, Great to hear that you've managed to format the files!! Let's address your questions:

Formatting Go Code and Kubernetes Environment:
It's unusual that the Go code formatting seemed to require Minikube running. Typically, go fmt and goimports are independent of Kubernetes or any specific runtime environment. Their functionality is only focused on code formatting and organization.
Difference Between go fmt and goimports:
The key difference lies in their functionality scope. go fmt is a more basic tool, part of the Go toolchain, focusing on making the code's formatting style standardized (for examples like indentations and spacing). On the other hand, goimports does everything go fmt does, but it also manages import lines - it adds missing import packages and removes unused ones. link This additional feature of goimports makes it particularly useful since Go is quite strict about import usage.

In short, while go fmt ensures your code is stylistically consistent, goimports also helps in maintaining clean and effective import statements.

Hope this clarifies your doubts! Don't hesitate to reach out if you have more questions!

Cali0707 · 2024-01-29T14:28:30Z

/retest-required

yijie-04 · 2024-01-29T16:43:49Z

@Leo6Leo That makes a lot more sense, thank you so much for the clarification!

Cali0707

Hi @yijie-04 just a few nits and then this should be good to merge. Specifically, it looks like in the unit test files you are still having the label values as an empty string instead of "enabled". It doesn't seem to be breaking anything currently, but for the sake of keeping the unit tests close to the actual code I think we should probably fix those before we merge this PR. This may break a couple unit tests which are expecting the label value to be "", but you should be able to just go through those and switch it to "enabled"

pkg/reconciler/apiserversource/apiserversource_test.go

pkg/reconciler/apiserversource/resources/oidc_rolebinding.go

Cali0707

/lgtm
/approve

Thanks @yijie-04 for all of your work on this! 🎉🎉

knative-prow · 2024-01-30T14:10:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, yijie-04

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~~ [Cali0707]

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

Leo6Leo · 2024-01-30T16:07:44Z

@yijie-04 Congrats on the merging! Thanks for the effort you put into this and hope to see more contributions from you in Knative community!

* 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 <scottprotoss@gmail.com>

…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 <scottprotoss@gmail.com> * 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 <147119743+yijie-04@users.noreply.github.com> Co-authored-by: Scott <scottprotoss@gmail.com>

…native#7527) Signed-off-by: Calum Murray <cmurray@redhat.com>

* Watch only our own OIDC-related secrets (#8070) Filter OIDC secrets Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> * ./hack/update-deps.sh Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: serviceaccountInformer -> oidcServiceaccountInformer Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: add oidc label selector to main contexts (partial cherry pick of #7527) Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: don't use filtered sa informer when sa is not labelled Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com> Signed-off-by: Calum Murray <cmurray@redhat.com> Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

yijie-04 and others added 6 commits December 23, 2023 14:36

controller.go changed

0bf2982

knative#7320 WIP

17c775a

Merge remote-tracking branch 'otherfork/main' into main

94cd51b

WIP: Testing filtered informer (knative#7341)

a82c2aa

unit test passed

d4bfe4e

Revert "Merge remote-tracking branch 'otherfork/main' into main"

979911d

This reverts commit 94cd51b, reversing changes made to 0bf2982.

knative-prow bot requested review from lionelvillard and matzew January 2, 2024 21:14

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 2, 2024

Leo6Leo reviewed Jan 5, 2024

View reviewed changes

pkg/reconciler/broker/trigger/controller.go Outdated Show resolved Hide resolved

pkg/reconciler/broker/trigger/controller.go Outdated Show resolved Hide resolved

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2024

yijie-04 changed the title ~~Use filtered informer to watch OIDC service accounts~~ [WIP] Use filtered informer to watch OIDC service accounts Jan 6, 2024

knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 6, 2024

yijie-04 added 5 commits January 5, 2024 22:58

Removed comments

bbefcc2

Changed to filtered informer for Subscription identity service account

de30fc5

Changed to filtered informer for Sequence service accounts

ce5a778

Changed to filtered informer for Parallel identity service accounts

2ae1090

Changed to filtered informer for APIServerSource identity service acc…

695e58c

…ount

fixed unit tests

efc1cc3

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 10, 2024

added a valid value for OIDClabelkey

b2941ac

yijie-04 changed the title ~~[WIP] Use filtered informer to watch OIDC service accounts~~ Use filtered informer to watch OIDC service accounts Jan 26, 2024

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2024

yijie-04 added 6 commits January 26, 2024 15:01

Merge remote-tracking branch 'upstream/main' into main

5244f19

changed references of OIDCLabelKey

2c94ec7

fixed import path problem

3d0d399

changed OIDCLabelSelector in all main.go files

8c30426

changed instances of OIDCLabelSelector in controller and controller t…

bee47f4

…est files

deleted OIDC related labels from register.go

5c25449

fixed formatting issues

63346a6

Cali0707 reviewed Jan 29, 2024

View reviewed changes

Added value for OIDCLabelKey

9a44a73

Cali0707 approved these changes Jan 30, 2024

View reviewed changes

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2024

knative-prow bot merged commit ff52881 into knative:main Jan 30, 2024
36 of 39 checks passed

creydr mentioned this pull request Jan 31, 2024

[control-plane]: Use filtered informer to watch OIDC service accounts knative-extensions/eventing-kafka-broker#3655

Closed

yijie-04 mentioned this pull request Mar 4, 2024

adding yijie-04 as community member knative/community#1526

Merged

Cali0707 added a commit to Cali0707/eventing that referenced this pull request Jul 4, 2024

fix: add oidc label selector to main contexts (partial cherry pick of k…

858b6a9

…native#7527) Signed-off-by: Calum Murray <cmurray@redhat.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use filtered informer to watch OIDC service accounts #7527

Use filtered informer to watch OIDC service accounts #7527

yijie-04 commented Jan 2, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 2, 2024 •

edited

Loading

knative-prow bot commented Jan 2, 2024

knative-prow bot commented Jan 2, 2024

Leo6Leo commented Jan 3, 2024

Leo6Leo commented Jan 5, 2024 •

edited

Loading

Leo6Leo left a comment

Leo6Leo commented Jan 8, 2024

yijie-04 commented Jan 10, 2024 •

edited

Loading

Leo6Leo commented Jan 10, 2024 •

edited

Loading

Leo6Leo commented Jan 26, 2024 •

edited

Loading

Cali0707 commented Jan 27, 2024

yijie-04 commented Jan 27, 2024

Leo6Leo commented Jan 29, 2024

Cali0707 commented Jan 29, 2024

yijie-04 commented Jan 29, 2024

Cali0707 left a comment

Cali0707 left a comment

knative-prow bot commented Jan 30, 2024

Leo6Leo commented Jan 30, 2024

Use filtered informer to watch OIDC service accounts #7527

Use filtered informer to watch OIDC service accounts #7527

Conversation

yijie-04 commented Jan 2, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

linux-foundation-easycla bot commented Jan 2, 2024 • edited Loading

knative-prow bot commented Jan 2, 2024

knative-prow bot commented Jan 2, 2024

Leo6Leo commented Jan 3, 2024

Leo6Leo commented Jan 5, 2024 • edited Loading

Leo6Leo left a comment

Choose a reason for hiding this comment

Leo6Leo commented Jan 8, 2024

yijie-04 commented Jan 10, 2024 • edited Loading

Leo6Leo commented Jan 10, 2024 • edited Loading

Leo6Leo commented Jan 26, 2024 • edited Loading

Cali0707 commented Jan 27, 2024

yijie-04 commented Jan 27, 2024

Leo6Leo commented Jan 29, 2024

Cali0707 commented Jan 29, 2024

yijie-04 commented Jan 29, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jan 30, 2024

Leo6Leo commented Jan 30, 2024

yijie-04 commented Jan 2, 2024 •

edited

Loading

linux-foundation-easycla bot commented Jan 2, 2024 •

edited

Loading

Leo6Leo commented Jan 5, 2024 •

edited

Loading

yijie-04 commented Jan 10, 2024 •

edited

Loading

Leo6Leo commented Jan 10, 2024 •

edited

Loading

Leo6Leo commented Jan 26, 2024 •

edited

Loading