Adding GEP 724: Refresh Route-Gateway Binding #725

robscott · 2021-07-17T01:07:37Z

What type of PR is this?
/kind gep

What this PR does / why we need it:
This PR adds GEP #724 as a follow up to various proposals and docs around how we can improve Route-Gateway binding.

Does this PR introduce a user-facing change?:

NONE

Deploy Preview: https://deploy-preview-725--kubernetes-sigs-gateway-api.netlify.app/geps/gep-724/

robscott · 2021-07-17T01:07:58Z

/hold for discussion.

robscott · 2021-07-17T01:15:10Z

/cc @bowei @hbagdi @howardjohn @jpeach @youngnick

k8s-ci-robot · 2021-07-17T01:15:13Z

@robscott: GitHub didn't allow me to request PR reviews from the following users: howardjohn.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @bowei @hbagdi @howardjohn @jpeach @youngnick

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.

howardjohn

I really like this. Istio has a pretty similar model (but this is much clearer, IMO) that hasn't shown any obvious issues

site-src/geps/gep-724.md

howardjohn · 2021-07-19T15:29:47Z

site-src/geps/gep-724.md

+
+  // SectionName is the name of a section within the target resource. When
+  // unspecified, this targets the entire resource. In the following
+  // resources, SectionName is interpreted as the following:


nit: I assume for custom attachment points they can choose to make this something else? If so it may be best to clarify that in the comment

Yeah good point, will clarify that.

If custom attachment points are supported, maybe SectionName is too specific? Maybe Anchor, TargetName, Fragment, ID?

I personally think SectionName is sufficiently generic. It's meant to refer to a single section within a resource. I can see an extension that just took a jsonpath or something, but I think that may be too complicated for >99% of use cases. Were there any specific use cases/extensions you had in mind?

howardjohn · 2021-07-19T15:32:04Z

site-src/geps/gep-724.md

+* May be more difficult to understand which Routes are attached to a Gateway.
+* Adding/replacing a Gateway requires changes to Routes.
+
+### Potential Expansion


How does this impact possible route -> route?

I think we'd end up with 3 options here, I'm not sure which is best:

1:many Route->Routes: Nearly identical to Route->Gateway pattern described here, parent Route describes trusted namespaces per rule, child Routes use attachTo mechanism to attach to a parent.

1:1 Route->Route: Require direct reference in both directions (include + attachTo would both be direct refs).

1:1 Route->Route: Direct reference from parent to child, ReferencePolicy to denote trust from child ns.

I think 1 and 3 make the most sense, it really just depends on how we want the feature to be used. If we want Routes to be able to somewhat blindly delegate a path to a namespace, 1 makes more sense. If we want Routes to include a very specific Route in another namespace, 3 probably makes more sense.

(3) seems to make most sense to me.

We designed Contour's HTTPProxy to be more like (3), but we've definitely had requests for not requiring a specific Route name, which I think is more like (1).

I think that (3) requires more out-of-band coordination between parties than (1) does, which could be good or bad. I'm still inclined to err on the side of more coordination and explicit config, because blanked includes scare me a bit. But it's not a strong preference.

Yeah the more I've thought about this, the more I think 3 is the best approach. That means Route->Route would work nearly identically to Route->Service, which seems reasonable to me.

bowei · 2021-07-19T16:46:08Z

LGTM -- we've had some feedback from users that indirection of which GW a Route was attached to was not very useful: app owners desire to be in the loop when such Route => GW bindings change.

site-src/geps/gep-724.md

robscott · 2021-07-28T00:29:23Z

Thanks to everyone for the great feedback on this in the community meeting yesterday! As suggested, I've added a rationale section and made the conceptual scope of the change I'm proposing more clear. PTAL

robscott · 2021-07-28T00:45:18Z

/retest

site-src/geps/gep-724.md

jpeach · 2021-07-28T01:51:30Z

site-src/geps/gep-724.md

+
+  // SectionName is the name of a section within the target resource. When
+  // unspecified, this targets the entire resource. In the following
+  // resources, SectionName is interpreted as the following:


If custom attachment points are supported, maybe SectionName is too specific? Maybe Anchor, TargetName, Fragment, ID?

jpeach · 2021-07-28T01:55:27Z

site-src/geps/gep-724.md

+
+#### Disadvantages
+
+* Even the simplest cross-namespace reference from Gateway -> Route would


A cross-namespace reference from Route -> Gateway would equally require the Gateway to have a reference policy, right?

This relies on the existing namespace selector in Gateway listeners to denote where Routes are trusted to bind from. Although it would be nice to reuse ReferencePolicy here, the nested list of listeners makes it significantly more complex + verbose as shown in the alternatives.

site-src/geps/gep-724.md

jpeach · 2021-07-28T02:02:42Z

site-src/geps/gep-724.md

+
+The other way we could use ReferencePolicy would be with Routes referencing
+Gateways. Unfortunately the nested structure of Gateways makes this nearly
+impossible to do effectively. A core concept for Gateways is that each listener


My main objection to this approach is that ReferencePolicy is a access control object, not a binding object. Best to not mix those concerns.

Agree that we have to be careful here, but in this case ReferencePolicy would effectively been allowing access/references from Routes to Gateway listeners. To clarify I'm not saying this is a great alternative, just that I think it's still somewhat unique from binding.

jpeach · 2021-07-28T05:21:40Z

I really like this. Istio has a pretty similar model (but this is much clearer, IMO) that hasn't shown any obvious issues

@howardjohn could you post a link to the istio mechanism?

howardjohn · 2021-07-28T17:19:37Z

I really like this. Istio has a pretty similar model (but this is much clearer, IMO) that hasn't shown any obvious issues

@howardjohn could you post a link to the istio mechanism?

https://istio.io/latest/docs/reference/config/networking/gateway/, grep for "some-config-namespace/my-gateway". Basically VirtualService points to a specific Gateway, and the two way handshake is based on hostname alignment

site-src/geps/gep-724.md

hbagdi · 2021-08-05T18:36:53Z

site-src/geps/gep-724.md

+#### Added
+To Route Specs:
+```go
+  // ParentRefs references the parent resources this Route should be attached


should is lenient and doesn't get the point across.
We want to communicate "anyone can attach to this route ONLY IF they are in this list".
Something along the lines:

Suggested change

// ParentRefs references the parent resources this Route should be attached

// ParentRefs references the resources that can process/attach/use this Route.

yeah, agreed, although I would nit that it should be attach to. :)

Do we think "can attach to" is sufficient here? Not exactly sure how to phrase this, but I think the behavior we want is for these referenced resources to be attached as long as they allow it. Exceptions to that would include conflicts or being referenced from a namespace or kind that isn't allowed. I think we need to be clear that this expresses a desire to attach to the given resource, not just permission for a resource to be attached.

site-src/geps/gep-724.md

Miciah · 2021-08-05T19:57:47Z

site-src/geps/gep-724.md

+* Provide a clear path to enable Route inclusion (Routes including Routes).
+* Simplify user experience based on initial feedback.
+* Enable other kinds of Route parents in addition to Gateway, this could include:
+    * Routes (as part of Route inclusion)


Is the contention here that this proposal makes the route→route use-case more cumbersome? If so, I share that concern. Seems like that should be called out in the list of disadvantages.

Miciah · 2021-08-05T19:59:33Z

site-src/geps/gep-724.md

+  //
+  // +kubebuilder:validation:MinLength=1
+  // +kubebuilder:validation:MaxLength=253
+  Kind string `json:"kind"`


When route-to-route is an option, that may become the majority. That said, using gateway as the default does make sense.

site-src/geps/gep-724.md

Miciah · 2021-08-05T19:59:52Z

site-src/geps/gep-724.md

+  // resource more than once.
+  //
+  // +optional
+  // +kubebuilder:validation:MaxItems=16


Is there some rationale for limiting this to 16 parents?

Just trying to avoid unbounded lists. Hard to think of any use case where this value would be more than 3 or 4. I guess it's possible that a single Route could be used to redirect to HTTPS, and that could get reused frequently. Did you have a use case for >16?

I'm thinking of route-to-route. Maybe I want to add the same microservice to many different applications. I don't have a concrete use-case though, so I don't feel too strongly about this.

Yeah I think it's unlikely that we'll use this mechanism for attaching to a Route, but since we don't have a GEP for that yet, we can't know for sure. At least with values like this, we can always increase the number of items allowed, nearly impossible to decrease in the future.

Miciah · 2021-08-05T20:02:52Z

site-src/geps/gep-724.md

+  // +kubebuilder:validation:MinLength=1
+  // +kubebuilder:validation:MaxLength=253
+  // +optional
+  SectionName string `json:"sectionName,omitempty"`


MinLength=0 or pointer type.

I suggested on a call that we could use ObjectFieldSelector or similar to specify the section, but I neglected to comment on the PR earlier. Would it make sense to use ObjectFieldSelector so the API is explicit about which section this references?

Will update to a pointer, thanks!

As far as ObjectFieldSelector (ref for anyone curious), I'm not sure it's quite what we're looking for here. As I understand it, it allows us to select a single field within a resource. That would allow for the valid spec.listeners[1], but lots of invalid values as well (someone may select some other part of a Gateway that may not make any sense).

Even the valid example of spec.listeners[1] has issues in that it would be tied to an index and not a name. That would require Gateway admins to maintain a very specific ordering of their listeners. If that order ever switched, lots of things could break. A similar argument could be made for changing a listener name, but that seems both more manageable and expected.

If I'm understanding the relevant API conventions correctly, this field should be interpreted with standard JavaScript syntax, so I don't think we'd even get the flexibility of jsonpath here.

What I had in mind was that ParentRef would have a field like SectionFieldRef corev1.ObjectFieldSelector in addition to the SectionName string field, and the user would specify sectionFieldRef: spec.listeners along with sectionName. The API could require that the field identified by sectionFieldRef be a +listType=map, and then sectionName would specify the value to match against the field identified by +listMapKey:

type GatewaySpec struct { // ... // +listType=map // +listMapKey=name Listeners []Listener `json:"listeners"` // ... } type ParentRef struct { // ... // SectionFieldRef specifies a field containing sections within the // referent. The specified field must be listType=map. If // SectionFieldRef is specified, then SectionName must be specified as // well, and the value of SectionName will be used to identify the // specific section by matching it against the field specified by // listTypeMap. // // Support: Core // // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=253 // +optional SectionFieldRef corev1.ObjectFieldSelector `json:"sectionFieldRef,omitempty"` // SectionName is ... SectionName string `json:"sectionName,omitempty"` // ... }

Example:

kind: Gateway metadata: namespace: infra name: lb spec: listeners: - name: foo # ... --- kind: HTTPRoute # ... spec: attachTo: - kind: Gateway namespace: infra name: lb sectionFieldRef: fieldPath: spec.listeners sectionName: foo

ObjectFieldSelector might not be the best type (maybe it would be better to define a new type that includes the field path and the item name), but the idea is to make explicit what SectionName is identifying.

Thanks for the explanation @Miciah! That does make a lot more sense. I can see that being a natural extension point if/when we want to support attaching to other arbitrary fields in Gateway or elsewhere. If I'm understanding correctly, it looks like it would be easy to add as a new optional field in the future without any kind of breaking changes.

site-src/geps/gep-724.md

youngnick

For the direction and overall design, this LGTM, but I think there are few more tidy-up changes needed.

I think we need to ensure that we s/networking.x-k8s.io/gateway.networking.k8s.io everywhere. :)

site-src/geps/gep-724.md

youngnick · 2021-08-06T00:03:36Z

site-src/geps/gep-724.md

+  // ...
+  // Kinds specifies the groups and kinds of Routes that are allowed to bind to
+  // this Gateway listener. When unspecified or empty, the only limitation on
+  // the kinds of Routes supported is the Listener protocol. Kind MUST


I'd completely agree that we should avoid this if we were still using selection, but with this new approach, I think the only thing most Gateway admins should have to worry about is the hostname, port, and protocol of a listener + the namespaces it supports. If you need to further restrict Route kinds, that's possible, but that seems like an advanced and usually unnecessary use case.

I think this is the argument that clinches this for me. As long as we mandate what controllers must do for the status of an object that attempts to attach to a listener that can't support it, I think it's okay.

However, I do think that it's important to have some status information that makes it clear to a Gateway owner that Routes have attempted to attach to a Listener that doesn't support them - we need to ensure there's a trail in both Gateway -> Route and Route -> Gateway directions for people to follow when things go wrong, because the objects have different owners.

In terms of having defaults, I think that it's important for the described behavior that this be defaulted to an empty list, but we can specify defaults in the RouteGroupKind struct. I feel like @hbagdi's concern here is about discoverability and status though?

youngnick · 2021-08-06T00:18:45Z

site-src/geps/gep-724.md

+  Kinds []RouteGroupKind `json:"kinds,omitempty"`
+}
+
+type RouteGroupKind struct {


This means that the way these fields are used is that if this stanza is not specified, match anything. If it is, match only this. You must specify at least a Kind, in which case the group defaults to gateway.networking.io. Otherwise, you may specify any Kind.

I like it.

youngnick · 2021-08-06T00:19:56Z

site-src/geps/gep-724.md

+#### Added
+To Route Specs:
+```go
+  // ParentRefs references the parent resources this Route should be attached


yeah, agreed, although I would nit that it should be attach to. :)

youngnick · 2021-08-06T00:21:58Z

site-src/geps/gep-724.md

+  //
+  // +kubebuilder:validation:MinLength=1
+  // +kubebuilder:validation:MaxLength=253
+  Kind string `json:"kind"`


I lean towards explicitness on this one as well. +1 to no default.

site-src/geps/gep-724.md

youngnick · 2021-08-06T00:31:36Z

site-src/geps/gep-724.md

+* May be more difficult to understand which Routes are attached to a Gateway.
+* Adding/replacing a Gateway requires changes to Routes.
+
+### Potential Expansion


We designed Contour's HTTPProxy to be more like (3), but we've definitely had requests for not requiring a specific Route name, which I think is more like (1).

I think that (3) requires more out-of-band coordination between parties than (1) does, which could be good or bad. I'm still inclined to err on the side of more coordination and explicit config, because blanked includes scare me a bit. But it's not a strong preference.

youngnick · 2021-08-06T00:37:11Z

site-src/geps/gep-724.md

+* Provide a clear path to enable Route inclusion (Routes including Routes).
+* Simplify user experience based on initial feedback.
+* Enable other kinds of Route parents in addition to Gateway, this could include:
+    * Routes (as part of Route inclusion)


The thing that makes Route -> Route inclusion much more complex is the semantics about how you carve up the URL/request metadata space. In HTTPProxy, we explicitly decided that the rules for assigning certain requests to a child HTTPProxy (in the Gateway API, this would be a child HTTPRoute) would become surrounding, uneditable constraints on that child, and that the child did not need to know about those constraints beforehand.

This has tradeoffs, obviously, but the key part here is that you can't design the Route -> Route reference mechanism without also designing how much the included Routes will know about where they are included.

In short, I think that what we have here is broad enough to allow us to experiment in either way, and we shouldn't hold this GEP up on figuring out how Route->Route inclusion works.

site-src/geps/gep-724.md

jpeach · 2021-08-06T03:10:24Z

Overall, this is in good shape. Fantastic work by @robscott to integrate the deluge of feedback :)

/lgtm

robscott · 2021-08-06T05:45:25Z

site-src/geps/gep-724.md

+}
+```
+
+### Gateway Status


@jpeach @youngnick @hbagdi I've added a new status section to describe suggested additions to status. I think this is relevant to different comments from all of you throughout the review of this GEP, so wanted to get your feedback on this addition if you have time.

This is great, love it.

youngnick

Status changes are excellent, as are the others.

/lgtm

youngnick · 2021-08-06T06:29:32Z

site-src/geps/gep-724.md

+}
+```
+
+### Gateway Status


This is great, love it.

robscott · 2021-08-06T15:55:03Z

Thanks to everyone for the great feedback on this GEP! Now that it's gotten 3 different LGTMs, I'm going to remove the hold and hope for one final LGTM to get it in. My last update clarifies that it is valid to attach a Route to multiple sections/listeners within the same Parent/Gateway. I'm thinking we should be able to cover any additional tweaks to this GEP in the implementation PR if necessary.

/hold cancel

hbagdi · 2021-08-06T20:57:46Z

Impressive heavy lifting here.
Thanks @robscott.
/lgtm

jsoref · 2024-10-16T18:49:13Z

site-src/geps/images/724-alt1.png

@robscott what font did you use for this / is there an original? I ask because ReferencePolicy was renamed but these two pictures weren't updated which left some of the documentation not making much sense.

For this picture, that's:
https://github.com/kubernetes-sigs/gateway-api/blob/9f820af97aaf6e8587712fec7b9266f42da81258/geps/gep-724/index.md#1-referencegrant-with-gateways-selecting-routes

jsoref · 2024-10-16T18:49:34Z

site-src/geps/images/724-alt2.png

For this picture, that's: https://github.com/kubernetes-sigs/gateway-api/blob/9f820af97aaf6e8587712fec7b9266f42da81258/geps/gep-724/index.md#2-referencegrant-with-routes-referencing-gateways

k8s-ci-robot added kind/gep PRs related to Gateway Enhancement Proposal(GEP) cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 17, 2021

k8s-ci-robot requested review from danehans and thockin July 17, 2021 01:07

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 17, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 17, 2021

k8s-ci-robot requested review from bowei, hbagdi and jpeach July 17, 2021 01:15

k8s-ci-robot requested a review from youngnick July 17, 2021 01:15

robscott force-pushed the route-gateway-kep branch from 8b30c0e to 06aa81c Compare July 17, 2021 01:17

robscott added this to the v1alpha2 milestone Jul 17, 2021

howardjohn reviewed Jul 19, 2021

View reviewed changes

hbagdi reviewed Jul 22, 2021

View reviewed changes

site-src/geps/gep-724.md Show resolved Hide resolved

hbagdi reviewed Jul 22, 2021

View reviewed changes

site-src/geps/gep-724.md Show resolved Hide resolved

robscott force-pushed the route-gateway-kep branch 2 times, most recently from 43e0354 to 91be407 Compare July 28, 2021 00:22

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 28, 2021

robscott force-pushed the route-gateway-kep branch from 91be407 to 2de5839 Compare July 28, 2021 00:32

jpeach reviewed Jul 28, 2021

View reviewed changes

robscott added 2 commits August 3, 2021 16:31

Reworking Route kind and group limitations in Gateway

e6bbc77

Additional updates for GEP 724, including rename to ParentRefs

4693e5e

robscott force-pushed the route-gateway-kep branch from 404e956 to 4693e5e Compare August 3, 2021 23:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 3, 2021

robscott added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 4, 2021

hbagdi reviewed Aug 5, 2021

View reviewed changes

Miciah reviewed Aug 5, 2021

View reviewed changes

youngnick reviewed Aug 6, 2021

View reviewed changes

Fixing typos, indentation, and adding clarification

c868162

jpeach reviewed Aug 6, 2021

View reviewed changes

site-src/geps/gep-724.md Show resolved Hide resolved

k8s-ci-robot assigned jpeach Aug 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2021

Adding Listener status changes

7b17f6c

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2021

robscott commented Aug 6, 2021

View reviewed changes

youngnick reviewed Aug 6, 2021

View reviewed changes

site-src/geps/gep-724.md

}

```

### Gateway Status

Copy link

Contributor

youngnick Aug 6, 2021

Choose a reason for hiding this comment

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

This is great, love it.

k8s-ci-robot assigned youngnick Aug 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2021

Clarifying that a Route can be attached to multiple sections

af186bc

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 6, 2021

k8s-ci-robot assigned hbagdi Aug 6, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 6, 2021

k8s-ci-robot merged commit 9aacf14 into kubernetes-sigs:master Aug 6, 2021

robscott deleted the route-gateway-kep branch January 8, 2022 01:05

jsoref reviewed Oct 16, 2024

View reviewed changes

jsoref mentioned this pull request Oct 21, 2024

Spelling #3400

Open


		#### Disadvantages

		* Even the simplest cross-namespace reference from Gateway -> Route would

	// ParentRefs references the parent resources this Route should be attached
	// ParentRefs references the resources that can process/attach/use this Route.

Adding GEP 724: Refresh Route-Gateway Binding #725

Adding GEP 724: Refresh Route-Gateway Binding #725

Conversation

robscott commented Jul 17, 2021 • edited Loading

robscott commented Jul 17, 2021

robscott commented Jul 17, 2021

k8s-ci-robot commented Jul 17, 2021

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bowei commented Jul 19, 2021 • edited Loading

robscott commented Jul 28, 2021

robscott commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

jpeach commented Jul 28, 2021

howardjohn commented Jul 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miciah Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpeach commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Aug 6, 2021

hbagdi commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Jul 17, 2021 •

edited

Loading

bowei commented Jul 19, 2021 •

edited

Loading

robscott Jul 28, 2021 •

edited

Loading

Miciah Aug 6, 2021 •

edited

Loading