Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use filtered informer to watch OIDC service accounts #7341

Closed
creydr opened this issue Oct 10, 2023 · 13 comments · Fixed by #7527
Closed

Use filtered informer to watch OIDC service accounts #7341

creydr opened this issue Oct 10, 2023 · 13 comments · Fixed by #7527
Assignees
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.

Comments

@creydr
Copy link
Member

creydr commented Oct 10, 2023

Currently we're watching all service accounts in the cluster for changes and reenque the objects which have an OIDC service account assigned if something changes. e.g.:

// Reconciler Trigger when the OIDC service account changes
serviceaccountInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
FilterFunc: controller.FilterController(&eventing.Trigger{}),
Handler: controller.HandleAll(impl.EnqueueControllerOf),
})

Instead we should label the OIDC service accounts and use a filtered serviceaccount informer based on that label/selector.

Additional information:

@creydr creydr added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Oct 10, 2023
@yijie-04
Copy link
Contributor

/assign

@yijie-04
Copy link
Contributor

yijie-04 commented Nov 21, 2023

@creydr Hey Christoph, since we are already using FilterController as a filter function in controller.go, I'm just wondering if you could explain more about the differences between this and the actual filtered informer, as well as how and where I should use the filtered informer/label. Thanks!

@creydr
Copy link
Member Author

creydr commented Nov 22, 2023

Hey @yijie-04,
the usual informers watch any of their resources in the cluster. So the serviceaccountInformer in the controller.go for example still watches all service accounts (and the controller.FilterController() is only for the event handler, while serviceaccountInformer still has all SAs).
So we want the serviceaccountInformer being setup as a filtered informer, only watching the OIDC service accounts (e.g. identified by a certain label).

Does this make it clearer?

@creydr
Copy link
Member Author

creydr commented Dec 12, 2023

Hello @yijie-04,
did the provided information help? Are you still working on this?

@yijie-04
Copy link
Contributor

Hi @creydr, yes it made sense, thank you! Sorry I've not been too active recently as I'm in my finals season. Is it ok if I work on this a bit later (in a week or so)?

@creydr
Copy link
Member Author

creydr commented Dec 20, 2023

Sure. Good luck with your exams.
Just to provide some additional information: @Leo6Leo did something similar in #7452 where he added a filtered informer for a role & rolebinding (see discussions at #7452 (comment))

@Leo6Leo
Copy link
Member

Leo6Leo commented Dec 20, 2023

@yijie-04 hey yijie, feel free to ping me here or on CNCF slack if you need any help

@yijie-04
Copy link
Contributor

@creydr @Leo6Leo Got it, thank you!

yijie-04 added a commit to yijie-04/eventing that referenced this issue Dec 24, 2023
@yijie-04
Copy link
Contributor

Hey @creydr @Leo6Leo I added the filter informer for OIDC service accounts and I'm trying to test it. However, I'm getting the error panic: Unable to fetch k8s.io/client-go/informers/core/v1.ServiceAccountInformer with selector oidc from context. and I'm just wondering if you have any suggestions or debugging tips on how I may proceed to resolve them. Thanks!

@Cali0707
Copy link
Member

Cali0707 commented Jan 2, 2024

@yijie-04 is this error occurring when you run the unit tests or when you deploy the code into minikube?

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 2, 2024

@yijie-04 I got this error when I was working on #7452. Here are some potential reasons that causing this happen:

  1. The informer selector is not injected to the context properly. Link
  2. The selector is not registered before the informers are being set up. Link1 Link2

@yijie-04
Copy link
Contributor

yijie-04 commented Jan 2, 2024

@Cali0707 @Leo6Leo The error occurred when I was running unit tests. I think it might have to do with the fake informers. There's a fake.go for service accounts but none for filtered. However, I'm not too sure of how to create one, as it seems to be generated by injection-gen? Thank you for your help!

@Leo6Leo
Copy link
Member

Leo6Leo commented Jan 2, 2024

@yijie-04 make sure to add this line as well:))
https://github.com/knative/eventing/pull/7452/files#diff-ec7c04b9a0859b30ea04c1e7cce6aeadf92597361e88f7373d9c7540321dcc97R43-R44

knative-prow bot pushed a commit that referenced this issue Jan 30, 2024
* controller.go changed

* #7320 WIP

* WIP: Testing filtered informer (#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>
creydr pushed a commit to creydr/knative-eventing that referenced this issue Apr 24, 2024
* 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>
openshift-merge-bot bot pushed a commit to openshift-knative/eventing that referenced this issue Apr 24, 2024
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines.
Projects
Status: ✅ Done
Development

Successfully merging a pull request may close this issue.

4 participants