Use filtered informer to watch OIDC service accounts #3719

karkabbage · 2024-02-25T01:46:23Z

Fixes #3655

Proposed Changes

Intial changes similar to #7527 to manage filtered accounts

Release Note

Docs

linux-foundation-easycla · 2024-02-25T01:46:27Z

The committers listed above are authorized under a signed CLA.

✅ login: karkabbage / name: Karyna Lim (a4f1797, 2000065, 7eafae0, 67b9c9a, 04fa4cd, 1cbeac3, fdf889e, 1dcf82a, bca29ca, 68c7da8, 5b5b633, ea52792)
✅ login: yijie-04 / name: Yijie Wang (4dd8713, 111a347, d42ec72)
✅ login: creydr / name: Christoph Stäbler (10470ec, 9c4f232, 971f43b, d56d0e5, 3e9129d, 9e71ba2, 73f4b86)

knative-prow · 2024-02-25T01:46:32Z

Welcome @karkabbage! It looks like this is your first PR to knative-extensions/eventing-kafka-broker 🎉

knative-prow · 2024-02-25T01:46:33Z

Hi @karkabbage. Thanks for your PR.

I'm waiting for a knative-extensions 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.

creydr · 2024-02-26T07:42:20Z

/cc

codecov · 2024-02-26T07:50:47Z

Codecov Report

Attention: Patch coverage is 48.71795% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 53.93%. Comparing base (aacc057) to head (73f4b86).
Report is 11 commits behind head on main.

Files	Patch %	Lines
...ol-plane/pkg/reconciler/trigger/v2/controllerv2.go	12.50%	13 Missing and 1 partial ⚠️
control-plane/pkg/reconciler/trigger/controller.go	68.42%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##               main    #3719       +/-   ##
=============================================
- Coverage     73.70%   53.93%   -19.78%     
+ Complexity      875      850       -25     
=============================================
  Files           100      343      +243     
  Lines          3419    18219    +14800     
  Branches        292      291        -1     
=============================================
+ Hits           2520     9826     +7306     
- Misses          723     7458     +6735     
- Partials        176      935      +759

Flag	Coverage Δ
java-unittests	`72.61% <ø> (-1.10%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

creydr · 2024-02-29T12:27:38Z

Hey @karkabbage,
Thanks for your work so far. Can you rebase this PR to solve the merge conflicts? Also it seems you're missing to run ./hack/update-codegen.sh or ./hack/update-deps.sh to vendor the newly imported packages.

karkabbage · 2024-03-08T15:36:33Z

Hi @creydr ,
I've rebased the merge conflicts and ran codegen, but no new files were made.
Is this expected ? I attempted using - "knative.dev/pkg/client/injection/kube/informers/factory/filtered/fake"
in controller_test.go
Thanks!
Update: I got files

Leo6Leo · 2024-03-08T16:11:42Z

/ok-to-test

karkabbage · 2024-03-08T16:27:03Z

/retest

karkabbage · 2024-03-08T16:29:47Z

@knative-prow-robot /retest

karkabbage · 2024-03-08T16:39:33Z

/retest

creydr · 2024-04-30T13:43:21Z

@karkabbage @yijie-04 do you need any more information or help besides #3719 (comment)?

…ces a trigger for correct broker class

creydr · 2024-05-02T12:56:50Z

/test upgrade-tests

…only, if SA references a trigger for correct broker class

creydr · 2024-05-02T16:00:39Z

@pierDipi @Cali0707 @Leo6Leo as I commited to this PR: could you PTAL?

Cali0707 · 2024-05-02T17:00:17Z

/retest-required

control-plane/pkg/reconciler/trigger/namespaced_controller.go

Cali0707 · 2024-05-02T17:06:24Z

test/e2e_new/trigger_finalizer_test.go

@@ -101,7 +99,7 @@ func hasNoKafkaBrokerFinalizer() func(ctx context.Context, t feature.T) {
 		time.Sleep(time.Second * 20) // "eventually"
 		tr := triggerfeatures.GetTrigger(ctx, t)
 		for _, f := range tr.Finalizers {
-			require.NotEqual(t, f, triggerreconciler.FinalizerName, "%+v", tr)
+			require.NotEqual(t, f, "kafka.triggers.eventing.knative.dev", "%+v", tr)


what's the need for this change?

Full context is here: #3719 (comment).
TL;DR; is, that this lead to an import an then registered an informer, which lead to a test failure in this case.

Co-authored-by: Calum Murray <cmurray@redhat.com>

creydr · 2024-05-03T09:45:19Z

/test reconciler-tests

creydr · 2024-05-03T11:28:06Z

/test reconciler-tests

Cali0707

/lgtm

knative-prow · 2024-05-06T12:14:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, karkabbage

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

Cali0707 · 2024-05-06T13:54:36Z

/retest-required

creydr · 2024-05-06T15:19:25Z

wait.go:200: test-mrqmxoln/containersource-eoaibzgi condition is {"type":"Ready","status":"False","lastTransitionTime":"2024-05-06T15:08:59Z","reason":"TrustBundlePropagation","message":"failed to create ConfigMap test-mrqmxoln/knative-eventing-bundlekne-bundle: configmaps \"knative-eventing-bundlekne-bundle\" already exists"}

/retest-required

creydr · 2024-05-07T06:04:46Z

/retest-required

creydr · 2024-05-07T09:04:32Z

/test integration-tests

Changes in pkg/reconciler/trigger based on #7527

fdf889e

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 Feb 25, 2024

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 Feb 25, 2024

knative-prow bot added the area/control-plane label Feb 25, 2024

knative-prow bot requested review from Leo6Leo and lionelvillard February 25, 2024 01:46

karkabbage changed the title ~~[WIP]Changes in pkg/reconciler/trigger based on #7527~~ [WIP] filtered informer to watch OIDC service accounts #3655 Feb 25, 2024

knative-prow bot requested a review from creydr February 26, 2024 07:42

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

Merge remote-tracking branch 'upstream/main'

ea52792

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

codegen updated

1cbeac3

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 8, 2024

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 Mar 8, 2024

fixing imported auth path

a4f1797

made fixes

04fa4cd

creydr added 2 commits May 2, 2024 12:21

Reconcile trigger on OIDC service account changes only, if SA referen…

3e9129d

…ces a trigger for correct broker class

Run goimport and gofmt

10470ec

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2024

Merge branch 'knative-extensions/main' into main

9e71ba2

creydr added 3 commits May 2, 2024 17:52

Namespaced broker: Reconcile trigger on OIDC service account changes …

971f43b

…only, if SA references a trigger for correct broker class

Remove unneeded comments

9c4f232

Remove one more of unneeded comments

d56d0e5

creydr changed the title ~~[WIP] filtered informer to watch OIDC service accounts #3655~~ Use filtered informer to watch OIDC service accounts May 2, 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 May 2, 2024

creydr requested a review from pierDipi May 2, 2024 15:59

Cali0707 reviewed May 2, 2024

View reviewed changes

Use correct BrokerClass for NamespacedBroker SA filter

73f4b86

Co-authored-by: Calum Murray <cmurray@redhat.com>

creydr requested a review from Cali0707 May 6, 2024 07:00

Cali0707 approved these changes May 6, 2024

View reviewed changes

knative-prow bot assigned Cali0707 May 6, 2024

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

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

knative-prow bot merged commit 6c4665c into knative-extensions:main May 7, 2024
32 of 39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use filtered informer to watch OIDC service accounts #3719

Use filtered informer to watch OIDC service accounts #3719

karkabbage commented Feb 25, 2024 •

edited

Loading

linux-foundation-easycla bot commented Feb 25, 2024 •

edited

Loading

knative-prow bot commented Feb 25, 2024

knative-prow bot commented Feb 25, 2024

creydr commented Feb 26, 2024

codecov bot commented Feb 26, 2024 •

edited

Loading

creydr commented Feb 29, 2024

karkabbage commented Mar 8, 2024 •

edited

Loading

Leo6Leo commented Mar 8, 2024

karkabbage commented Mar 8, 2024

karkabbage commented Mar 8, 2024

karkabbage commented Mar 8, 2024

creydr commented Apr 30, 2024

creydr commented May 2, 2024

creydr commented May 2, 2024

Cali0707 commented May 2, 2024

Cali0707 May 2, 2024

creydr May 3, 2024

creydr commented May 3, 2024

creydr commented May 3, 2024

Cali0707 left a comment

knative-prow bot commented May 6, 2024

Cali0707 commented May 6, 2024

creydr commented May 6, 2024

creydr commented May 7, 2024

creydr commented May 7, 2024

Use filtered informer to watch OIDC service accounts #3719

Use filtered informer to watch OIDC service accounts #3719

Conversation

karkabbage commented Feb 25, 2024 • edited Loading

Proposed Changes

linux-foundation-easycla bot commented Feb 25, 2024 • edited Loading

knative-prow bot commented Feb 25, 2024

knative-prow bot commented Feb 25, 2024

creydr commented Feb 26, 2024

codecov bot commented Feb 26, 2024 • edited Loading

Codecov Report

creydr commented Feb 29, 2024

karkabbage commented Mar 8, 2024 • edited Loading

Leo6Leo commented Mar 8, 2024

karkabbage commented Mar 8, 2024

karkabbage commented Mar 8, 2024

karkabbage commented Mar 8, 2024

creydr commented Apr 30, 2024

creydr commented May 2, 2024

creydr commented May 2, 2024

Cali0707 commented May 2, 2024

Cali0707 May 2, 2024

Choose a reason for hiding this comment

creydr May 3, 2024

Choose a reason for hiding this comment

creydr commented May 3, 2024

creydr commented May 3, 2024

Cali0707 left a comment

Choose a reason for hiding this comment

knative-prow bot commented May 6, 2024

Cali0707 commented May 6, 2024

creydr commented May 6, 2024

creydr commented May 7, 2024

creydr commented May 7, 2024

karkabbage commented Feb 25, 2024 •

edited

Loading

linux-foundation-easycla bot commented Feb 25, 2024 •

edited

Loading

codecov bot commented Feb 26, 2024 •

edited

Loading

karkabbage commented Mar 8, 2024 •

edited

Loading