Authenticate requests from PingSources by Zazzscoot · Pull Request #7525 · knative/eventing

Zazzscoot · 2023-12-25T04:10:17Z

Fixes #7320

Proposed Changes

(Prerequisites: OIDC mode is enabled, and sink has a defined audience)

Binds the source's service account to allow the source to create a JWT token
Edited existing tests to add sinkAudience

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

Docs

linux-foundation-easycla · 2023-12-25T04:10:21Z

The committers listed above are authorized under a signed CLA.

✅ login: Zazzscoot / name: Scott Hirano (17c775a, acdba24, 36c0f81, 1518b67, 709f368, 16e5faa, 4f3c536, c28078e, cdbb2b5, 6531a80, 6b1c2f8, 0a5396b, 3e3cf30, 396bf6c, f1d4428, c5e26e7, 292ecb6, de55635, 5e92caa, c687fd6, 4ac3395, 1eb1883, aa3569e, 99ed180, 5bfe16f, 9352fa8)
✅ login: yijie-04 / name: Yijie Wang (7bef2bc, 23e898c, f9185dd, 00c2a92)

knative-prow · 2023-12-25T04:10:26Z

Welcome @Zazzscoot! It looks like this is your first PR to knative/eventing 🎉

knative-prow · 2023-12-25T04:10:27Z

Hi @Zazzscoot. Thanks for your PR.

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

Details

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 · 2023-12-27T11:22:35Z

Hello @Zazzscoot,
Thanks for your work on this so far. I just got roughly over your PR and saw you want to create some roles & rolebindings dynamically. This is not needed in case of the pingsource, as we have only one pingsource managing pod running (in the knative-eventing namespace) which handles all pingsource resources (not like in case of the apiserversource, which creates a deployment for each apiserversource resource). So you need only to adjust the permissions of the pingsource deployment/pod (pingsource-mt-adapter-clusterrole.yaml)

Leo6Leo · 2024-01-02T16:41:32Z

@Zazzscoot Thanks for the PR! Here are some review comments I have:

As this is your very first PR in the LFX(linux fundation) community, please make sure you complete the EasyCLA so that you are authorized to contribute to Knative.

Here is the link on what is EasyCLA and how to complete it.

And for the description of the PR, make sure you put the issue number and the keyword "fixes" on the same line, as GitHub will automatically detect it and link the issue and your PR together.

For the PRs that you are still working on, you can add a keyword "[WIP]" at the beginning of the title of your PR.
For example, your PR's title is Authenticate requests from PingSources, you can edit it and make it become [WIP] Authenticate requests from PingSources ", and the bot will automatically add a label to it and indicate that this PR is work in progress. And you can also open this PR as a draft PR.

Leo6Leo · 2024-01-03T14:01:22Z

@Zazzscoot I made some graphics and hope to assist your understanding of the whole workflow!

On how to approach solving this issue, as suggested in my graphic:

Granting the access
According to @creydr 's comment, we need to grant permission to PingSource's adaptor, so it can create the JWT token (OIDC Token)! Take a look at my PR here and it will help!
Testing it is actually working
Take a look at here and ApiserversourceSendEventWithJWT() function on how to make a e2e test on this and how to make reconciler test here

Tips:

You might be encountering the trouble when you are copying the lines similar to these
https://github.com/knative/eventing/pull/7452/files#diff-d2cf61e462d4d61f80a631d9fc5f72ffd47999876b91b472e7d3af5ea700f7e5R63-R64
And your IDE will complain about it by saying "file not found" or something similar. Don't worry, you may just need to update the dependency! Simply runing hack/update-deps.sh will simply solve the problem by adding a few new files in the vendor folder.

Please let me know if you have any other questions or need further explainations!

creydr · 2024-01-08T12:16:28Z

Hi @Zazzscoot,
the 1.13 release is in ~ 2 weeks (Jan 24th). Do you think we can complete this by that date (including the reviews) as we would like to see this in? How can I help you?

Zazzscoot · 2024-01-08T17:59:56Z

/cc @Leo6Leo @creydr

Leo6Leo · 2024-01-08T18:00:51Z

/ok-to-test

Zazzscoot · 2024-01-17T09:17:10Z

/ok-to-test

creydr · 2024-01-17T09:20:11Z

/ok-to-test

Hi @Zazzscoot,
to run the Github workflows, an org member has to approve them manually (on PRs from non-org members). The /ok-to-test label can only be applied once and tells Prow to run these tests

Zazzscoot · 2024-01-17T09:20:16Z

Please see my review comments. After that, your errors should be fixed. Please let me know if you have any other questions! @Zazzscoot

Hey @Leo6Leo, I have made your changes and the TestPingSourceSendsEventsWithOIDC is now passing! Thank you so much for the comment!

Zazzscoot · 2024-01-17T09:21:26Z

/ok-to-test

Hi @Zazzscoot, to run the Github workflows, an org member has to approve them manually (on PRs from non-org members). The /ok-to-test label can only be applied once and tells Prow to run these tests

Noted, thanks for the clarification!

creydr

Only one tiny comment. Otherwise looking good!
Thanks for your work so far @Zazzscoot and @yijie-04 👍

pkg/adapter/mtping/runner.go

creydr · 2024-01-17T10:41:45Z

And it seems to have a couple of style issues too. Could you PTAL at them?

format.sh

creydr · 2024-01-17T21:46:42Z

/retest

creydr · 2024-01-18T08:39:37Z

/retest

creydr · 2024-01-18T10:26:07Z

/retest

creydr

Only one unneeded/leftover comment to be removed

pkg/adapter/mtping/runner.go

creydr

Thanks a lot @Zazzscoot and @yijie-04 for you work and patience for finishing it!
🎉 🎉 🎉

/lgtm

knative-prow · 2024-01-18T14:52:24Z

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [creydr]

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

Leo6Leo · 2024-01-18T16:31:02Z

@Zazzscoot @yijie-04 Great work guys! You guys did it! Hope to see more contributions from you in the knative community :)

yijie-04 · 2024-01-18T17:30:03Z

Thank you so much for the help and support! @Leo6Leo @creydr

Zazzscoot added 2 commits December 23, 2023 20:33

knative#7320 WIP

17c775a

added missing address attributes to unit tests

acdba24

knative-prow bot requested review from creydr and lionelvillard December 25, 2023 04:10

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 25, 2023

Zazzscoot changed the title ~~Authenticate requests from PingSources~~ [WIP] Authenticate requests from PingSources Jan 5, 2024

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 Jan 5, 2024

Zazzscoot marked this pull request as draft January 5, 2024 03:33

Zazzscoot added 3 commits January 4, 2024 23:37

added serviceaccoutns/token to pingsource role

36c0f81

added rule for service accounts (token)

1518b67

wrote e2e tests for pingsource OIDC

709f368

knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Jan 8, 2024

Zazzscoot added 2 commits January 8, 2024 12:37

removed dynamic role adding

16e5faa

removed unnecessary packages

4f3c536

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

Zazzscoot marked this pull request as ready for review January 8, 2024 17:59

knative-prow bot requested review from Leo6Leo and evankanderson January 8, 2024 17:59

knative-prow bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 8, 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 Jan 17, 2024

Zazzscoot requested a review from Leo6Leo January 17, 2024 09:17

ran goimports

c687fd6

creydr reviewed Jan 17, 2024

View reviewed changes

pkg/adapter/mtping/runner.go Outdated Show resolved Hide resolved

removed dupe ceode from mtping/runner.go

4ac3395

creydr reviewed Jan 17, 2024

View reviewed changes

format.sh Outdated Show resolved Hide resolved

Zazzscoot added 3 commits January 17, 2024 11:43

ran goimportrs on test/auth/oidc_test.go

1eb1883

fixing boilerplate for test/auth/features/oidc/pingsource.go

aa3569e

removed format.sh

99ed180

Merge branch 'knative:main' into main

5bfe16f

creydr requested changes Jan 18, 2024

View reviewed changes

pkg/adapter/mtping/runner.go Outdated Show resolved Hide resolved

removed unnecessary comments

9352fa8

Zazzscoot requested a review from creydr January 18, 2024 14:45

creydr approved these changes Jan 18, 2024

View reviewed changes

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

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

knative-prow bot merged commit fb9be2b into knative:main Jan 18, 2024

yijie-04 mentioned this pull request Mar 4, 2024

adding yijie-04 as community member knative/community#1526

Merged

github-actions bot mentioned this pull request Mar 2, 2026

🌱 CNCF mission generation 2026-03-02 kubestellar/console-kb#251

Merged

Conversation

Zazzscoot commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Uh oh!

linux-foundation-easycla bot commented Dec 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow bot commented Dec 25, 2023

Uh oh!

knative-prow bot commented Dec 25, 2023

Uh oh!

creydr commented Dec 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Leo6Leo commented Jan 2, 2024

Uh oh!

Leo6Leo commented Jan 3, 2024

Uh oh!

creydr commented Jan 8, 2024

Uh oh!

Zazzscoot commented Jan 8, 2024

Uh oh!

Leo6Leo commented Jan 8, 2024

Uh oh!

Zazzscoot commented Jan 17, 2024

Uh oh!

creydr commented Jan 17, 2024

Uh oh!

Zazzscoot commented Jan 17, 2024

Uh oh!

Zazzscoot commented Jan 17, 2024

Uh oh!

creydr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

creydr commented Jan 17, 2024

Uh oh!

Uh oh!

creydr commented Jan 17, 2024

Uh oh!

creydr commented Jan 18, 2024

Uh oh!

creydr commented Jan 18, 2024

Uh oh!

creydr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

creydr left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow bot commented Jan 18, 2024

Uh oh!

Leo6Leo commented Jan 18, 2024

Uh oh!

yijie-04 commented Jan 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Zazzscoot commented Dec 25, 2023 •

edited

Loading

linux-foundation-easycla bot commented Dec 25, 2023 •

edited

Loading

creydr commented Dec 27, 2023 •

edited

Loading