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

Authenticate Requests from ApiServerSources #7452

Merged

Conversation

Leo6Leo
Copy link
Member

@Leo6Leo Leo6Leo commented Nov 15, 2023

Fixes #7321

Proposed Changes

Prerequisite: When OIDC mode is enabled and sink has audience

  • All the outgoing event request will be appended with JWT Authorization header
  • Automatically create role and rolebinding for the source's serviceaccount so that they have permission to create the JWT token for it's OIDC service account and the audience
  • Added the tests for role and rolebinding creation

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

Under OIDC mode, all the outgoing event request will be appended with JWT Authorization header

Docs

@knative-prow 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 Nov 15, 2023
Copy link

knative-prow bot commented Nov 15, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Nov 15, 2023
Copy link

codecov bot commented Nov 15, 2023

Codecov Report

Attention: 143 lines in your changes are missing coverage. Please review.

Comparison is base (5d7e104) 76.19% compared to head (a73a26b) 75.53%.
Report is 1 commits behind head on main.

❗ Current head a73a26b differs from pull request most recent head 8890c5d. Consider uploading reports for the commit 8890c5d to get more accurate results

Files Patch % Lines
...iler/apiserversource/resources/oidc_rolebinding.go 0.00% 69 Missing ⚠️
pkg/reconciler/apiserversource/apiserversource.go 55.12% 27 Missing and 8 partials ⚠️
pkg/adapter/v2/cloudevents.go 38.23% 19 Missing and 2 partials ⚠️
...ciler/apiserversource/resources/receive_adapter.go 14.28% 10 Missing and 2 partials ⚠️
pkg/adapter/v2/config.go 40.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7452      +/-   ##
==========================================
- Coverage   76.19%   75.53%   -0.66%     
==========================================
  Files         260      261       +1     
  Lines       14447    14667     +220     
==========================================
+ Hits        11008    11079      +71     
- Misses       2873     3008     +135     
- Partials      566      580      +14     

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

@knative-prow knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 16, 2023
@knative-prow knative-prow bot added area/test-and-release Test infrastructure, tests or release 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 Nov 20, 2023
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
SECURITY.md Outdated Show resolved Hide resolved
pkg/adapter/v2/cloudevents.go Outdated Show resolved Hide resolved
pkg/auth/serviceaccount.go Outdated Show resolved Hide resolved
Leo6Leo and others added 4 commits November 24, 2023 11:19
Signed-off-by: Leo Li <leoli@redhat.com>
Co-authored-by: Christoph Stäbler <cstabler@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
@knative-prow knative-prow bot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 30, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023
…r for the role and rolebinding informers

Signed-off-by: Leo Li <leoli@redhat.com>
@Leo6Leo Leo6Leo force-pushed the authenticate-request-from-apiserversource branch from bb7c0d6 to 479aa7f Compare December 13, 2023 21:42
…r for the role and rolebinding informers

Signed-off-by: Leo Li <leoli@redhat.com>
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments on the role & rolebinding event handler filters.

You also need to adjust the tests too (added labels and later with the new owner reference)

pkg/reconciler/apiserversource/controller.go Show resolved Hide resolved
pkg/reconciler/apiserversource/controller.go Show resolved Hide resolved
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 14, 2023
Signed-off-by: Leo Li <leoli@redhat.com>
@Leo6Leo Leo6Leo requested a review from creydr December 15, 2023 06:43
cmd/controller/main.go Outdated Show resolved Hide resolved
pkg/reconciler/apiserversource/apiserversource_test.go Outdated Show resolved Hide resolved
pkg/reconciler/apiserversource/apiserversource_test.go Outdated Show resolved Hide resolved
pkg/reconciler/apiserversource/controller_test.go Outdated Show resolved Hide resolved
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
@Leo6Leo Leo6Leo requested a review from creydr December 18, 2023 06:03
…using only the selector oidc) and not by value

Signed-off-by: Leo Li <leoli@redhat.com>
…using only the selector oidc) and not by value

Signed-off-by: Leo Li <leoli@redhat.com>
Signed-off-by: Leo Li <leoli@redhat.com>
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @Leo6Leo for your hard work and patience on all the review cycles on this!

/lgtm

🎉🎉🎉

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2023
Copy link

knative-prow bot commented Dec 18, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: creydr, Leo6Leo

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:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 18, 2023
@knative-prow knative-prow bot merged commit 31abcdb into knative:main Dec 18, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Authenticate Requests from ApiServerSources
5 participants