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.

Sign up for GitHub

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

Jump to bottom

Add documentation for Eventing Authorization #6104

Merged
merged 10 commits into from
Sep 5, 2024
Merged

Add documentation for Eventing Authorization #6104

merged 10 commits into from
Sep 5, 2024

Conversation

creydr
Copy link
Member

@creydr creydr commented Aug 28, 2024

Fixes #6099

Proposed Changes

@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 28, 2024
@knative-prow knative-prow bot requested review from csantanapr and pierDipi August 28, 2024 10:15
@netlify Netlify
Copy link

netlify bot commented Aug 28, 2024 •
edited
Loading

✅ Deploy Preview for knative ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 993c2a3
🔍 Latest deploy log https://app.netlify.com/sites/knative/deploys/66d6a0de8ec45f0008ecd72c
😎 Deploy Preview https://deploy-preview-6104--knative.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@creydr
Copy link
Member Author

creydr commented Aug 28, 2024

/cc @pierDipi @matzew
Could you give this an initial review?
-> https://deploy-preview-6104--knative.netlify.app/docs/eventing/features/authorization/

@knative-prow knative-prow bot requested a review from matzew August 28, 2024 10:42
@creydr
Copy link
Member Author

creydr commented Aug 29, 2024

We probably want to wait for

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2024
@creydr creydr requested review from Cali0707 and pierDipi August 29, 2024 16:08
@Cali0707
Copy link
Member

I think for the filters, it's normally "All" of the filters that just be true for a specific policy, not one of the filters for a policy

lberk
lberk reviewed Aug 30, 2024
View reviewed changes
docs/eventing/features/authorization.md Outdated

### Specify for who the `EventPolicy` applies

The `.spec.to` section specifies **where** the events are allowed to be sent. This field is optional; if left empty, the policy applies to all resources within the namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Is the to.* fields logically combined in an or statement? or an and statement? I'm not sure from the description if specifying multiple to.* targets widens the scope of the event policy, or narrows it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting point of view to this. Let me try to make it clearer.
For background information: AuthZ is checked on the receiving side, so we don't know at this point, where the events got sent to elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Does this make it clearer?

lberk
lberk reviewed Aug 30, 2024
View reviewed changes
docs/eventing/features/authorization.md Outdated

2. `from.sub`:

* **Definition**: Specifies a subject, such as a service account, that is allowed to send events. It can include wildcard patterns as a postfix (`*`) for broader matching.
Copy link
Member

Choose a reason for hiding this comment

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

Is there any other subject other than a service account?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet. But when we integrate this with other IDPs and allow sending events from outside the mesh, this would be more relevant.
But to not confuse, I'd better remove it

lberk
lberk reviewed Aug 30, 2024
View reviewed changes
docs/eventing/features/authorization.md
kind: PingSource
metadata:
name: pingsource-1
namespace: namespace-1
Copy link
Member

@lberk lberk Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

namespace-2?
Edit: Oops, this is be design. Maybe worth pointing this out in a comment that this pingsource will be rejected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Depending on the step in the example, events from this source will be allowed or rejected. So I tried to make it clearer in the different steps. I also tried to make it clear in the image, with the "architecture overview" (not sure, if you checked the code or the rendered output of this PR 😄 )

@creydr
Copy link
Member Author

creydr commented Sep 2, 2024

/unhold
as we enabled the support in EKB

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 2, 2024
Cali0707
Cali0707 approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve
/hold for other reviews

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 3, 2024
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Sep 3, 2024

[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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 3, 2024
@creydr creydr requested a review from lberk September 3, 2024 20:41
@creydr
Copy link
Member Author

creydr commented Sep 4, 2024

@pierDipi @lberk any other comments, or can we unhold this?

@pierDipi
Copy link
Member

pierDipi commented Sep 5, 2024

/lgtm

Thanks!

@creydr
Copy link
Member Author

creydr commented Sep 5, 2024

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2024
@knative-prow knative-prow bot merged commit 3b522f7 into knative:main Sep 5, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cali0707 Cali0707 Cali0707 approved these changes

@csantanapr csantanapr Awaiting requested review from csantanapr

@matzew matzew Awaiting requested review from matzew

@pierDipi pierDipi Awaiting requested review from pierDipi

@lberk lberk Awaiting requested review from lberk

Assignees

@pierDipi pierDipi

@Cali0707 Cali0707

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add documentation for Authorization support in Knative-Eventing
4 participants
@creydr @Cali0707 @pierDipi @lberk