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

Channel dispatcher authenticates the requests with OIDC #7445

Merged

Conversation

Cali0707
Copy link
Member

Fixes #7364

Proposed Changes

  • Use audience from subscriber specs for requests
  • Use service account from subscriber statuses for requests

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

The channel dispatcher authenticates requests with OIDC

Docs

Signed-off-by: Calum Murray <cmurray@redhat.com>
Copy link

knative-prow bot commented Nov 13, 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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2023
@knative-prow knative-prow bot requested review from creydr and lberk November 13, 2023 21:23
@Cali0707
Copy link
Member Author

@creydr I think my changes work, but not sure how to set up a good rekt test for them. Any ideas?

Copy link

codecov bot commented Nov 13, 2023

Codecov Report

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

Comparison is base (311ee01) 76.81% compared to head (2a83906) 76.71%.
Report is 1 commits behind head on main.

Files Patch % Lines
.../apis/messaging/v1/in_memory_channel_validation.go 60.52% 11 Missing and 4 partials ⚠️
...econciler/inmemorychannel/dispatcher/controller.go 50.00% 5 Missing and 2 partials ⚠️
...iler/inmemorychannel/dispatcher/inmemorychannel.go 40.00% 5 Missing and 1 partial ⚠️
pkg/channel/fanout/fanout_event_handler.go 82.35% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7445      +/-   ##
==========================================
- Coverage   76.81%   76.71%   -0.10%     
==========================================
  Files         253      253              
  Lines       14136    14200      +64     
==========================================
+ Hits        10858    10894      +36     
- Misses       2736     2757      +21     
- Partials      542      549       +7     

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

@creydr
Copy link
Member

creydr commented Nov 14, 2023

@creydr I think my changes work, but not sure how to set up a good rekt test for them. Any ideas?

Hey @Cali0707,
thanks for working on this. I would do the following for an e2e test:

  1. Create an InMemoryChannel
  2. Create an eventshub receiver with the OIDCReceiverAudience(aud) option, so that the receiver expects and verifies the JWT
  3. Create a Subscription with the InMemoryChannel as channel and the eventshub receiver as subscriber
  4. Create a eventshub sender and send events to the InMemoryChannel
  5. Verify that the receiver got all requests from the sender

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow 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 Nov 14, 2023
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Nov 15, 2023
@Cali0707 Cali0707 marked this pull request as ready for review November 15, 2023 15:47
@Cali0707 Cali0707 changed the title [WIP]: Channel dispatcher authenticates the requests with OIDC Channel dispatcher authenticates the requests with OIDC Nov 15, 2023
@knative-prow 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 Nov 15, 2023
@Cali0707 Cali0707 requested a review from creydr November 15, 2023 15:47
@Cali0707
Copy link
Member Author

/test all

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 just some comments from going very quickly over it.
BTW: you can also run the e2e tests locally via something like the following:

SYSTEM_NAMESPACE=knative-eventing KO_DOCKER_REPO=localhost:5001 go test -race -count=1 -timeout=1h -parallel=12 -v -run TestChannelDispatcherAuthenticatesWithOID -tags=e2e ./test/auth

pkg/reconciler/subscription/subscription.go Outdated Show resolved Hide resolved
… eventing-controller

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Cali0707 and others added 4 commits November 20, 2023 10:29
Co-authored-by: Christoph Stäbler <cstabler@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@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.

Looks good so far @Cali0707. Thanks for your work so far.
Only left a comment.
Can you only give a bit context on the conformance test changes?

pkg/apis/messaging/v1/in_memory_channel_validation.go Outdated Show resolved Hide resolved
@Cali0707
Copy link
Member Author

@creydr the context on the conformance test changes is that we updated the webhook to reject changes to .Spec.Subscribers, so the conformance tests which were trying to validate that the channel CRD supports that field by directly patching it started to fail.

I tried to emulate those checks that the CRD supports the .Spec.Subscribers indirectly by creating a subscription and verifying that the CRD gets correctly populated in the .Spec.Subscribers and .Status.Subscribers

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

/test conformance-tests

@Cali0707 Cali0707 requested a review from creydr November 21, 2023 15:04
@creydr
Copy link
Member

creydr commented Nov 22, 2023

@creydr the context on the conformance test changes is that we updated the webhook to reject changes to .Spec.Subscribers, so the conformance tests which were trying to validate that the channel CRD supports that field by directly patching it started to fail.

I tried to emulate those checks that the CRD supports the .Spec.Subscribers indirectly by creating a subscription and verifying that the CRD gets correctly populated in the .Spec.Subscribers and .Status.Subscribers

Thanks for clarifying this @Cali0707.
The PR looks good from my side 👍

As the conformance tests might affect other implementations too: @pierDipi are you OK with updates too, or do you see any issues?

@Cali0707
Copy link
Member Author

/cc @pierDipi

@knative-prow knative-prow bot requested a review from pierDipi November 22, 2023 14:30
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023
@creydr
Copy link
Member

creydr commented Nov 23, 2023

/retest

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 @Cali0707 for working on this!

/lgtm

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

knative-prow bot commented Nov 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

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

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 merged commit bdca23d into knative:main Nov 24, 2023
37 of 41 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update channel dispatcher to authenticate requests
3 participants