Tweaks to ReferencePolicy GEP #722
Conversation
k8s-ci-robot
added
kind/gep
k8s-ci-robot
added
the
approved
|
I think we're pretty close to consensus on this, but will hold as a precaution. /hold |
k8s-ci-robot
added
the
do-not-merge/hold
site-src/geps/gep-709.md
Outdated
| @@ -82,8 +88,8 @@ decisions: | |||
| more powerful it may encourage unnecessarily insecure configuration. | |||
|
|
|||
| ```go | |||
| // ReferencePolicy identifies cross namespace relationships that are trusted for | |||
| // Gateway API. | |||
| // ReferencePolicy identifies cross namespace relationships between types of | |||
There was a problem hiding this comment.
In the previous PR, a suggestion was made to rename this to InboundReferencePolicy. Wanted to pick up the conversation in this PR. I think I slightly prefer the shorter ReferencePolicy name, but admit that InboundReferencePolicy would make room for something like OutboundReferencePolicy. That might allow admins to restrict where Gateways in their namespace could forward to. Thoughts?
There was a problem hiding this comment.
Yeh, my preference is also for a single ReferencePolicy with From+To.
There was a problem hiding this comment.
Yeah as I've been thinking about it more, it would not be that difficult to expand ReferencePolicy to cover both inbound and outbound references. I don't think that's something we actually want to do, but the only required change would be adding namespace to the to struct. The larger hurdle would be the complexity for users in trying to restrict/allow both inbound and outbound references.
There was a problem hiding this comment.
As discussed in community meeting today, we believe it is unlikely that we will ever have a need for an OutboundReferencePolicy. Given that, the preference is to keep the simpler ReferencePolicy name. I'll update the comments around ReferencePolicy to make it clearer that it refers to inbound references.
There was a problem hiding this comment.
I've updated the wording around this, PTAL
| @@ -33,6 +33,8 @@ nav: | |||
| - API specification: references/spec.md | |||
| - Releases: references/releases.md | |||
| - Implementations: references/implementations.md | |||
| - Enhancement Proposals: | |||
| - "709: Cross Namespace References from Routes": geps/gep-709.md | |||
There was a problem hiding this comment.
Adding full GEP names here could end up being a bit much for our existing nav structure, would anyone prefer a single GEP index page that then linked to each individual GEP? I don't feel strongly either way, but wanted to start with something. (https://deploy-preview-722--kubernetes-sigs-gateway-api.netlify.app/geps/gep-709/)
robscott
force-pushed
the
referencepolicy-gep-v2
branch
from
bd9ed32
to
92b63e0
Compare
|
Thanks to everyone for the great feedback around this! Based on the discussion at the community meeting today, it sounds like this was pretty close to consensus with no major objections. Given that, I'm going to remove the hold and move this back to a place where an LGTM can merge this in. /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I did the last one, so I should probably leave this to someone else? :) |
robscott
force-pushed
the
referencepolicy-gep-v2
branch
from
92b63e0
to
f9680a5
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, howardjohn, jpeach, robscott 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 |
What type of PR is this?
/kind gep
What this PR does / why we need it:
This is a quick follow up to #711 that addresses a few leftover comments on that PR + does some minor cleanup.
Does this PR introduce a user-facing change?:
Deploy Preview: https://deploy-preview-722--kubernetes-sigs-gateway-api.netlify.app/geps/gep-709/