feat: make oidc discovery url configurable

[Cali0707]

Fixes #8121

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]

/cc @pierDipi @creydr

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

[codecov[bot]]

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 :warning:
cmd/jobsink/main.go	0.00%	8 Missing :warning:
pkg/apis/feature/features.go	55.55%	2 Missing and 2 partials :warning:
cmd/broker/filter/main.go	0.00%	3 Missing :warning:
cmd/broker/ingress/main.go	0.00%	3 Missing :warning:
...econciler/inmemorychannel/dispatcher/controller.go	62.50%	2 Missing and 1 partial :warning:

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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[Cali0707]

/cc @creydr

[Cali0707]

/cc @creydr @pierDipi

[pierDipi]

/assign @creydr

Can you take a look?

[Cali0707]

/cc @creydr @pierDipi

[pierDipi]

@creydr can you take a look?

[creydr]

/cc

[creydr]

Rebased after #8195

[creydr]

@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 eee13207e67235355d6ad3c2674970cedac9f2d8 (which should support Trustbundles too).

[creydr]

I can also check on https://github.com/knative/eventing/pull/8145#discussion_r1713221355 and get the features from the ctx in auth.NewVerifier()

[Cali0707]

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]

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 :thinking:

[creydr]

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

[Cali0707]

/cc @creydr @pierDipi

[Cali0707]

/cc @creydr

[creydr]

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 e5b9e23f8ef2abe7eae5e8b041820445cbb9799f

[knative-prow[bot]]

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

/test unit-tests

[creydr]

/retest