feat: make oidc discovery url configurable #8145

Cali0707 · 2024-08-09T16:48:40Z

Proposed Changes

Add feature flag for OIDC discovery base url
Use feature flag in token verifier when building discovery config

Release Note

The OIDC discovery url is now configurable with the oidc-discovery-base-url feature flag in the config-features configmap.

Cali0707 · 2024-08-09T16:48:59Z

/cc @pierDipi @creydr

Should we also backport this at all to make OIDC work on EKS in previous releases?

codecov · 2024-08-09T16:54:59Z

Codecov Report

Attention: Patch coverage is 13.69863% with 63 lines in your changes missing coverage. Please review.

Project coverage is 66.47%. Comparing base (e79f3b6) to head (e5b9e23).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/auth/verifier.go	0.00%	42 Missing ⚠️
cmd/jobsink/main.go	0.00%	8 Missing ⚠️
pkg/apis/feature/features.go	55.55%	2 Missing and 2 partials ⚠️
cmd/broker/filter/main.go	0.00%	3 Missing ⚠️
cmd/broker/ingress/main.go	0.00%	3 Missing ⚠️
...econciler/inmemorychannel/dispatcher/controller.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8145      +/-   ##
==========================================
- Coverage   67.47%   66.47%   -1.01%     
==========================================
  Files         371      371              
  Lines       18036    18318     +282     
==========================================
+ Hits        12169    12176       +7     
- Misses       5088     5351     +263     
- Partials      779      791      +12

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

creydr · 2024-08-12T06:29:51Z

pkg/auth/token_verifier.go

@@ -57,14 +53,14 @@ type IDToken struct {
 	AccessTokenHash string
 }

-func NewOIDCTokenVerifier(ctx context.Context) *OIDCTokenVerifier {
+func NewOIDCTokenVerifier(ctx context.Context, features feature.Flags) *OIDCTokenVerifier {


Is there a reason, for not getting the feature store from the given context?

This made it easier to re-create the verifier in the callback functions when the config-features cm changes

Cali0707 · 2024-08-16T14:28:26Z

/cc @creydr

Cali0707 · 2024-08-19T13:45:00Z

/cc @creydr @pierDipi

pierDipi · 2024-08-26T16:56:24Z

/assign @creydr

Can you take a look?

Cali0707 · 2024-09-10T17:13:26Z

/cc @creydr @pierDipi

pierDipi · 2024-09-16T06:52:24Z

@creydr can you take a look?

creydr · 2024-09-16T14:41:33Z

/cc

creydr · 2024-09-17T09:56:41Z

Rebased after #8195

Signed-off-by: Calum Murray <[email protected]>

…fault.svc

creydr · 2024-09-17T15:02:55Z

@Cali0707, on testing with oidc-discovery-base-url: "https://oidc.eks.eu-west-1.amazonaws.com/id/1", I saw

Get \"https://oidc.eks.eu-west-1.amazonaws.com/id/1/.well-known/openid-configuration\": tls: failed to verify certificate: x509: certificate signed by unknown authority}

This was because the used HTTP client of the TokenVerifier (now only Verifier) only took the CA certs from the API server. I addressed this in eee1320 (which should support Trustbundles too).

creydr · 2024-09-17T15:05:56Z

I can also check on #8145 (comment) and get the features from the ctx in auth.NewVerifier()

Cali0707 · 2024-09-17T15:13:45Z

I can also check on #8145 (comment) and get the features from the ctx in auth.NewVerifier()

Would this work if the features were updated to have a new url?

creydr · 2024-09-17T15:21:48Z

I can also check on #8145 (comment) and get the features from the ctx in auth.NewVerifier()

Would this work if the features were updated to have a new url?

When we make sure we pass a context, which has the features (e.g. featureStore.ToContext(ctx)), I'd guess so

Anyhow not totally sure about it, as having it as a parameter shows directly, that this is required for the function call 🤔

creydr · 2024-09-18T09:31:59Z

@pierDipi As I also committed to this PR, could you give it a quick review from your side too?

cmd/jobsink/main.go

… of feature flags Signed-off-by: Cali0707 <[email protected]>

Cali0707 · 2024-09-23T00:22:22Z

/cc @creydr @pierDipi

pkg/auth/verifier.go

Signed-off-by: Cali0707 <[email protected]>

Cali0707 · 2024-10-05T16:23:38Z

/cc @creydr

creydr · 2024-10-07T06:41:07Z

JobSink does not come up

        message: "{\"level\":\"info\",\"ts\":\"2024-10-05T16:42:12.130Z\",\"logger\":\"job-sink\",\"caller\":\"jobsink/main.go:112\",\"msg\":\"Starting
          the JobSink Ingress\",\"commit\":\"dcc5539\"}\npanic: runtime error: invalid
          memory address or nil pointer dereference\n[signal SIGSEGV: segmentation
          violation code=0x1 addr=0x0 pc=0x47f0d8]\n\ngoroutine 1 [running]:\nsync.(*RWMutex).Lock(0x21fd838?)\n\tsync/rwmutex.go:146
          +0x18\nknative.dev/eventing/pkg/auth.(*Verifier).initOIDCProvider(0xc0003e2640,
          {0x21fd838, 0xc000431830}, 0x0)\n\tknative.dev/eventing/pkg/auth/verifier.go:267
          +0x351\nknative.dev/eventing/pkg/auth.NewVerifier({0x21fd838, 0xc000431830},
          {0x21e9488, 0xc00034f8e0}, {0x21e9690, 0xc0004c79a0}, {0x21e41e8, 0xc000184c60})\n\tknative.dev/eventing/pkg/auth/verifier.go:88
          +0x5b5\nmain.main()\n\tknative.dev/eventing/cmd/jobsink/main.go:129 +0xc53\n"

I guess this is because https://github.com/knative/eventing/pull/8145/files#diff-09c972dc18a7eba8ddf0c27c3a5629ea43289af799f5af6aaf67b5a6848665c7R58 does not need to be a pointer / is unitialized

Edit: addressed in e5b9e23

pierDipi

/lgtm
/approve

knative-prow · 2024-10-15T07:10:00Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,pierDipi]

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

creydr · 2024-10-15T07:41:40Z

/test unit-tests

creydr · 2024-10-15T08:33:10Z

/retest

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 9, 2024

knative-prow bot requested review from creydr and Leo6Leo August 9, 2024 16:48

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

knative-prow bot requested a review from pierDipi August 9, 2024 16:49

creydr reviewed Aug 12, 2024

View reviewed changes

knative-prow bot requested a review from creydr August 16, 2024 14:28

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 16, 2024

knative-prow bot assigned creydr Aug 26, 2024

creydr force-pushed the add-oidc-discovery-config branch from 6ff7de7 to 3f2dfa6 Compare September 17, 2024 09:56

feat: make oidc discovery url configurable

837d926

Signed-off-by: Calum Murray <[email protected]>

creydr force-pushed the add-oidc-discovery-config branch from 3f2dfa6 to 837d926 Compare September 17, 2024 10:14

Use Trustbundles and public CAs, when issuer URL is not kubernetes.de…

eee1320

…fault.svc

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 Sep 17, 2024

pierDipi reviewed Sep 19, 2024

View reviewed changes

cmd/jobsink/main.go Outdated Show resolved Hide resolved

refactor: pass configmap watcher to auth verifier constructor instead…

4bd5aef

… of feature flags Signed-off-by: Cali0707 <[email protected]>

knative-prow bot requested a review from pierDipi September 23, 2024 00:22

creydr reviewed Sep 30, 2024

View reviewed changes

pkg/auth/verifier.go Outdated Show resolved Hide resolved

feat: add rw lock for provider access

057c69d

Signed-off-by: Cali0707 <[email protected]>

knative-prow bot requested a review from creydr October 5, 2024 16:23

Fix verifier rwmutex

e5b9e23

pierDipi approved these changes Oct 15, 2024

View reviewed changes

knative-prow bot assigned pierDipi Oct 15, 2024

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

knative-prow bot merged commit 33a9027 into knative:main Oct 15, 2024
35 of 36 checks passed

creydr mentioned this pull request Nov 26, 2024

Add support for oidc-discovery-base-url feature configuration knative-extensions/eventing-kafka-broker#4174

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make oidc discovery url configurable #8145

feat: make oidc discovery url configurable #8145

Cali0707 commented Aug 9, 2024

Cali0707 commented Aug 9, 2024

codecov bot commented Aug 9, 2024 •

edited

Loading

creydr Aug 12, 2024

Cali0707 Aug 12, 2024 •

edited

Loading

Cali0707 commented Aug 16, 2024

Cali0707 commented Aug 19, 2024

pierDipi commented Aug 26, 2024

Cali0707 commented Sep 10, 2024

pierDipi commented Sep 16, 2024

creydr commented Sep 16, 2024

creydr commented Sep 17, 2024

creydr commented Sep 17, 2024

creydr commented Sep 17, 2024

Cali0707 commented Sep 17, 2024

creydr commented Sep 17, 2024 •

edited

Loading

creydr commented Sep 18, 2024

Cali0707 commented Sep 23, 2024

Cali0707 commented Oct 5, 2024

creydr commented Oct 7, 2024 •

edited

Loading

pierDipi left a comment

knative-prow bot commented Oct 15, 2024

creydr commented Oct 15, 2024

creydr commented Oct 15, 2024

feat: make oidc discovery url configurable #8145

feat: make oidc discovery url configurable #8145

Conversation

Cali0707 commented Aug 9, 2024

Proposed Changes

Cali0707 commented Aug 9, 2024

codecov bot commented Aug 9, 2024 • edited Loading

Codecov Report

creydr Aug 12, 2024

Choose a reason for hiding this comment

Cali0707 Aug 12, 2024 • edited Loading

Choose a reason for hiding this comment

Cali0707 commented Aug 16, 2024

Cali0707 commented Aug 19, 2024

pierDipi commented Aug 26, 2024

Cali0707 commented Sep 10, 2024

pierDipi commented Sep 16, 2024

creydr commented Sep 16, 2024

creydr commented Sep 17, 2024

creydr commented Sep 17, 2024

creydr commented Sep 17, 2024

Cali0707 commented Sep 17, 2024

creydr commented Sep 17, 2024 • edited Loading

creydr commented Sep 18, 2024

Cali0707 commented Sep 23, 2024

Cali0707 commented Oct 5, 2024

creydr commented Oct 7, 2024 • edited Loading

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Oct 15, 2024

creydr commented Oct 15, 2024

creydr commented Oct 15, 2024

codecov bot commented Aug 9, 2024 •

edited

Loading

Cali0707 Aug 12, 2024 •

edited

Loading

creydr commented Sep 17, 2024 •

edited

Loading

creydr commented Oct 7, 2024 •

edited

Loading