-
Notifications
You must be signed in to change notification settings - Fork 468
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
Make gwctl ReferenceGrant aware. Capture any errors observed while constructing the resource graph. #3029
Make gwctl ReferenceGrant aware. Capture any errors observed while constructing the resource graph. #3029
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauravkghildiyal 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 |
/cc @mlavacca |
/retest |
22b3fab
to
f33488c
Compare
c26f1f9
to
7cea75b
Compare
incorrect. For now, we only calculate effective policy once the references are corrected.
…HTTPRoute to a Backend is valid.
…ermitted) in the resourceModel. This will be available for consumption by the views.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work on this @gauravkghildiyal!
// Ensure that if this is a cross namespace reference, then it is accepted | ||
// through some ReferenceGrant. | ||
if httpRoute.GetNamespace() != backendRef.Namespace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker for this PR, but I think it would be very helpful to have a more generic RefGrant library that both gwctl and Gateway implementations can use. This has some of the foundational bits for this, but it would just need to be structured differently if that were a goal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. I'll try to come up with something more generic separately and will bring it up for discussion.
…e from HTTPRoute to a Backend is valid.
…e from HTTPRoute to a Backend is valid.
…ayClass is incorrect.
7cea75b
to
bf4bff6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a bunch for the review, @robscott!
// Ensure that if this is a cross namespace reference, then it is accepted | ||
// through some ReferenceGrant. | ||
if httpRoute.GetNamespace() != backendRef.Namespace { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea. I'll try to come up with something more generic separately and will bring it up for discussion.
Thanks @gauravkghildiyal! /lgtm |
/retest |
What type of PR is this?
/kind feature
What this PR does / why we need it:
to a Backend is valid.
in the resourceModel. This will be available for consumption by the views.
Which issue(s) this PR fixes:
Fixes #3039
Does this PR introduce a user-facing change?:
/area gwctl