gateway-api icon indicating copy to clipboard operation
gateway-api copied to clipboard

GEP-1058 Add support for Route Inclusion and Delegation

Open Jeff-Apple opened this issue 2 years ago • 14 comments

What type of PR is this? /kind gep

What this PR does / why we need it: This is the formal GEP-10058 document

NONE

Jeff-Apple avatar Apr 04 '22 16:04 Jeff-Apple

Hi @Jeff-Apple. Thanks for your PR.

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

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.

k8s-ci-robot avatar Apr 04 '22 16:04 k8s-ci-robot

Thanks @Jeff-Apple!

/ok-to-test

robscott avatar Apr 04 '22 16:04 robscott

Looks like this test failure was an issue caused by my webhook integration, hopefully it's just a flake.

/retest

robscott avatar Apr 04 '22 16:04 robscott

I haven't got to it today, but I will open a PR back to your branch @Jeff-Apple, with my changes to the status section tomorrow.

youngnick avatar Apr 05 '22 05:04 youngnick

/assign bowei

bowei avatar Apr 26 '22 17:04 bowei

Not sure policy attachment is well defined - if a policy is attached to a parent, does it apply to child ? Or the other way ? What happens if policies conflict as result of merging ?

And more important - what are the security implications, child seems to have a lot of power influencing parent routing.

costinm avatar Jul 26 '22 00:07 costinm

We were discussing the mesh use cases in GAMMA - in particular how HttpRoutes may be attached to a Service ( or something similar ).

What happens if a user has both a mesh and gateway, and they have Gateway forward to a backend Service, which in turns has HttpRoutes attached ? One way or another - we need to address this, since it'll be a common case for users of both APIs. Is it treated as a case of 'inclusion or delegation' ? What if the impl of Gateway and mesh is different ? If not - wouldn't it be pretty confusing for users ?

My proposal to do delegation with the equivalent semantics of having 2 chained gateways ( even if it is implemented in one physical gateway ) holds for this mode - if Gateway is also a Mesh implementation it may optimize, else users may have 2 proxies. But either way - the model for gateway delegation will be the same with the model for using gateway and mesh routes.

Keeping the flexibility of having 2 layers of proxies as needed is also useful when policies like WASM or other extensions are attached - the implementation may choose to not allow the 'child' owner to inject risky code or policies into the trusted top level gateway, and instantiate a dedicated proxy for childrens that have such policies.

On Tue, Jul 26, 2022 at 10:14 PM Nick Young @.***> wrote:

@.**** commented on this pull request.

Big review from me, sorry.

I'd summarize this as:

  • we need another pass over the status design, especially keeping in mind some of the changes we've made to what counts as "Attached".
  • we need to define the interaction between this and Policy Attachment.

Before we do all of that though, I think that there seems to be some reasonably strong objections to this whole concept - it seems like the value proposition here is not clear, and I think that we may need to do more to explain:

  • the circumstances under which an implementation would support this
  • that it's completely valid for an implementation that already has some of these capabilities to not support it (in particular, I think that Service Mesh implementations have many similar functions as part of their mesh).

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930606065 :

+- Ephemeral Route: The route configuration that results from combining (i.e.

  • reconciling) a parent route and the child route(s) that attaches to it. The

  • ephemeral route represents the configuration that is actually applied to the

  • data plane.

⬇️ Suggested change

-- Ephemeral Route: The route configuration that results from combining (i.e.

  • reconciling) a parent route and the child route(s) that attaches to it. The

  • ephemeral route represents the configuration that is actually applied to the

  • data plane.

+- Ephemeral Route: A logical construct used in this document to explain the

+route configuration that results from combining (i.e. reconciling) a parent route

+and the child route(s) that attaches to it. This ephemeral route is a way of thinking

+about the configuration that is actually applied to the data plane, and should not

+be considered an implementation requirement.


In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930607158 :

+- Provide a 2-tier hierarchy of route configuration to allow partial delegation

+of route settings.

⬇️ Suggested change

-- Provide a 2-tier hierarchy of route configuration to allow partial delegation

-of route settings.

+- Provide a 2-tier hierarchy of route configuration to allow partial delegation

+of route settings at Extended conformance.

I think we need to make very clear that this functionality is not and probably should never be core functionality. It's pretty advanced and useful for specific use cases; outside of those use cases it's a lot of extra complexity for minimal gain. Inside those use cases, it's absolutely vital.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930613973 :

    1. A parent Route may include RouteRules that use BackendRefs AND
  •   `RouteRules` that use `AllowedRoutes`.
    

We probably need to add something in here though: ⬇️ Suggested change

    1. A parent Route may include RouteRules that use BackendRefs AND
  •   `RouteRules` that use `AllowedRoutes`.
    
    1. A parent Route may include RouteRules that use BackendRefs AND
  •   `RouteRules` that use `AllowedRoutes`. A single RouteRule inside a Parent Route
    
  •   may only contain **either** `BackendRefs` or `AllowedRoutes`, however.
    

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930615285 :

+1. Support for Parent/child Route relationships is an "Extended" feature in the

  • Gateway API specification

+2. Add AllowedRoutes to [HTTPRouteRule][httpRouteRule] as optional

    1. Default value is nil
    1. A single RouteRule can contain BackendRefs OR AllowedRoutes, not both.
    1. A parent Route may include RouteRules that use BackendRefs AND
  •   `RouteRules` that use `AllowedRoutes`.
    
    1. AllowedRoutes MUST be non-nil in a RouteRule to permit child Routes
  •   to attach to that `RouteRule`.
    
  •    1. Explanation: A parent `Route` will allow delegation to one or more
    
  •       child `Routes` by specifying `AllowedRoutes` where `BackendRef` would
    
  •       be used otherwise.
    
    1. Multiple child routes may attach to a single RouteRule, as long as they
  •   are distinct.
    

+3. Allow a Route’s ParentRef Kind to be the same type as the Route’s type

I think we should make this stronger: ⬇️ Suggested change

-3. Allow a Route’s ParentRef Kind to be the same type as the Route’s type

+3. Enforce that a Route’s ParentRef Kind must be the same type as the Route’s type.


In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930620514 :

  •   to avoid cycles.
    
    1. The two path strings are concatenated to form the complete path to be
  •   matched.
    

+7. Hostname: Extend, to child routes, the current model of configuring the

  • hostname at both the Listener and Route.

    1. Allow setting hostname matching at any combination of listener, parent
  •   route and child route.  No change to the current way of combining the
    
  •   values of hostname when set at more than one level.
    

+## Route status

+### Things to remember

+If a route references a non-route resource in its parentRef field, and has an allowedRoute in one of its route rules, it is a parent Route.

+If a route reference another route in its parentRef field, it is a child Route and cannot also have allowedRoute stanzas.

⬇️ Suggested change

-If a route reference another route in its parentRef field, it is a child Route and cannot also have allowedRoute stanzas.

+If a route reference another route of the same Kind in its parentRef field, it is a child Route and cannot also have allowedRoute stanzas.


In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930614528 :

    1. AllowedRoutes MUST be non-nil in a RouteRule to permit child Routes
  •   to attach to that `RouteRule`.
    

The big advantage of using an optional struct with nil meaning "don't do this" is that if you're not engaging with this feature, you don't see anything about it (that is, no allowedRoutes in your YAML unless you're using this, thanks to omitempty.)

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930615729 :

+3. Allow a Route’s ParentRef Kind to be the same type as the Route’s type

    1. Kind is required when the ParentRefs is a route. Kind wil default to
  •   `gateway` (current behavior)
    

+4. Add Name (type is [SectionName][sectionName]) to

  • [HTTPRouteRule][httpRouteRule] as optional

    1. The Name field is expected to be added in the implementation of
  •   [GEP-995 - Add Name to HTTPRouteRule and HTTPRouteMatch][gep-995]
    
    1. Default value is nil
    1. A child route may include a SectionName when its ParentRef is a route.
  •    1. A child route “attaches" to the parent route’s `RouteRule` that matches
    
  •       that `SectionName`. The child route must also match the criteria
    
  •       specified in the `RouteRule’s AllowedRoutes`.
    
    1. If the ParentRefs of a child route doesn’t include a SectionName, the
  •   child route attaches to all of the Parent Route’s `RouteRules` having
    
  •   `AllowedRoutes` criteria that the child route meets.
    

+5. A parent Route and any related child Routes must be the same type of

Yes, my suggestion above makes this one redundant, agreed.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930625811 :

  • different way to attach routes to routes.

+## References

+The first draft of this GEP was presented in the document [K8s Gateway API -

+Route inclusion and delegation][first-draft]. It includes many comments from the

+Gateway API community.

+There are multiple other documents, GitHub Issues and discussions that are

+directly related to this topic and influenced the design of this GEP. This

+include the following:

+- Request Filtering Between Gateways and Namespaced Routes · Issue #634 · kubernetes-sigs/gateway-api (github.com)

+- Request Filtering Between Gateways and Namespaced Routes - Google Docs

+- Clarify how RouteGateways would work if we supported Route->Route delegation · Issue #411 · kubernetes-sigs/gateway-api (github.com)

+- Route inclusion/delegation security and policy implications · Issue #1042 · kubernetes-sigs/gateway-api (github.com)

Yes, I think that there's absolutely an action here to do some definition about how policy attachment interacts with route inclusion.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930605607 :

+route types in the future

+## Non-Goals

+- Explicitly define how route inclusion/delegation works for any specific route

  • type other than HTTPRoute

+## Terminology

+- Route: A generic substitute for the type of route. Equivalent to saying,

+“a route of type [type]”

+- Parent Route: A Route that specifies a listener as its parentRef object

+and is listed as a parentRef object in a different Route. Technically, a parent

+route is any route that has a non-nil AllowedRoutes block

+- Child Route: A Route that lists a Route as its parentRef object.

+- Ephemeral Route: The route configuration that results from combining (i.e.

Yes, I think that the way we've used language here makes this feel more like an implementation direction than a useful abstraction. I think we intended this more to be the latter.

I'll make a separate suggestion.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930607549 :

+- Route: A generic substitute for the type of route. Equivalent to saying,

+“a route of type [type]”

+- Parent Route: A Route that specifies a listener as its parentRef object

+and is listed as a parentRef object in a different Route. Technically, a parent

+route is any route that has a non-nil AllowedRoutes block

+- Child Route: A Route that lists a Route as its parentRef object.

+- Ephemeral Route: The route configuration that results from combining (i.e.

  • reconciling) a parent route and the child route(s) that attaches to it. The

  • ephemeral route represents the configuration that is actually applied to the

  • data plane.

+- Delegation: when an included route crosses a persona ownership boundary

+(e.g. a parent route is managed by an infrastructure team and a child route is

+managed by an app dev team)

+- Inclusion: When a child Route’s configuration is combined with a parent

+Route’s config during runtime. The two routes may or may not be owned by the same

+persona or organization. All delegations are inclusions, but not all inclusions

I agree that putting the definition of inclusion first makes sense.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930607997 :

+route is any route that has a non-nil AllowedRoutes block

+- Child Route: A Route that lists a Route as its parentRef object.

+- Ephemeral Route: The route configuration that results from combining (i.e.

  • reconciling) a parent route and the child route(s) that attaches to it. The

  • ephemeral route represents the configuration that is actually applied to the

  • data plane.

+- Delegation: when an included route crosses a persona ownership boundary

+(e.g. a parent route is managed by an infrastructure team and a child route is

+managed by an app dev team)

+- Inclusion: When a child Route’s configuration is combined with a parent

+Route’s config during runtime. The two routes may or may not be owned by the same

+persona or organization. All delegations are inclusions, but not all inclusions

+are delegations.

+## Introduction

I agree that we should be clear that this feature is all about Gateway API resources and how they are combined, not about what happens in the data plane.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930614826 :

+Here are the changes and related rules:

+1. Support for Parent/child Route relationships is an "Extended" feature in the

  • Gateway API specification

+2. Add AllowedRoutes to [HTTPRouteRule][httpRouteRule] as optional

    1. Default value is nil
    1. A single RouteRule can contain BackendRefs OR AllowedRoutes, not both.
    1. A parent Route may include RouteRules that use BackendRefs AND
  •   `RouteRules` that use `AllowedRoutes`.
    
    1. AllowedRoutes MUST be non-nil in a RouteRule to permit child Routes
  •   to attach to that `RouteRule`.
    
  •    1. Explanation: A parent `Route` will allow delegation to one or more
    
  •       child `Routes` by specifying `AllowedRoutes` where `BackendRef` would
    
  •       be used otherwise.
    
    1. Multiple child routes may attach to a single RouteRule, as long as they
  •   are distinct.
    

Maybe this should say "as long as they don't conflict"? I believe we're using "distinct" to mean "different and don't conflict".

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930610779 :

+Another use case is where there is a group of route config settings that are

+common across multiple Routes of the same type (e.g. httpRoute). Users want to

+be able to have the Routes include that group of config settings by reference

+instead of having to embed those settings directly in each Route or RouteRule.

+The configurations for Scenario 1 (shown lower on this page), demonstrate two

+ways this technique can be used. In the first case, traffic needs to be routed

+to "Core Services" (core-svces-ns) from two different starting points. Using

+route inclusion, the routing configuration for the Core Services are put in a

+child route and then that child route is included in two sections of a parent

+route where they are needed.

+Another way route inclusion is used in Scenario 1 is by configuring a

+HTTPRouteFilter, that inserts a header, in the parent route. This results in the

+filter being added to each RouteRule in the child route store-route. The child

+route is both shorter and easier to read than it would have been without this

The important part of having the resources be in separate namespaces is not that they're shorter, it's that they can be owned by different people, and those people can coordinate without having to coordinate edits of a single resource.

In Contour's implementation of inclusion, we had the requirement that the owner of the root HTTPProxy (equivalent in this proposal to the Parent HTTPRoute) had to specify the included resources by name, and "the owner of the parent resource has to make edits for each new thing" was explicitly called out by users as a pain point: see projectcontour/contour#2206 https://github.com/projectcontour/contour/issues/2206 for a lot more detail.

The point I'm trying to make here is that the people who want to break config up in this way need it desperately, so remove manual human action from the config management loop.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930616722 :

    1. If an implementation chooses to not enforce this limitation, that must be
  •   documented along with how many tiers of parent/child route relationships
    
  •   are allowed
    

+7. A Route’s ParentRefs can list more than one type of parent.

    1. Example: A HTTPRoute’s ParentRefs can list one (or more) Gateways and
  •   one (or more) `HTTPRoutes` at the same time.
    

+## Binding Parent and Child Routes

+Binding a child route to a parent route refers to the process of reconciling

+(i.e. combining) the two routes. Binding is treated as a single transaction for

+each Parent/Child route pair. Even when a child route attaches to multiple

+RouteRules in the parent route, the binding between the two routes is still a

+single transaction.

+The binding transaction has two stages; reconciliation and activation. The

I think the idea is describe a way of thinking about how the reconciliation and binding process works, but I can see how this reads like implementation guidance. I think we should update this to make that clearer.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930617868 :

+config but, all of them combined may produce an invalid config.

+This means that, when a parent route has multiple child routes that attach to

+it, the reconciliations of the children to the parent, must be done in series

+(as opposed to in parallel).

+NOTE: An Ephemeral route is not a formal Kubernetes object. It is a logical

+concept used, in this document, to help describe behavior. In some cases, the

+terms "parent" and "ephemeral" route are interchangeable depending on whether or

+not an ephemeral route already exists due to a previous, successful binding of a

+child to that parent.

+### Failed Binding

+The binding transaction MUST fail if there is A non-recoverable error during the

+binding process. A failed binding should result in no changes to the

Yes, the necessity of storing state makes being nice to users who make mistakes extremely tricky, sadly.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930618174 :

+A failed binding DOES NOT invalidate the parent RouteRules that the child

+route is trying to bind to. This is necessary because other child routes might

+bind to those parental RouteRules (might already be bound or may try to bind

+in the future).

+When a parent route's binding to a child route fails, that should not block the

+parent route's bindings with other child routes. In the same way, a child route

+failing to bind with a parent route should not block that child's bindings to

+other parent routes.

+## Reconciling Parent and Child Routes

+**A key principle guiding the design of how parent and child routes are

+combined, is that the data plane can apply the resulting route settings in a

+single processing cycle (per direction) of the network traffic.**

That's what the ephemeral route stuff is talking about, I think. So yes?

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930618805 :

+the entire Binding fails.

+A configuration conflict exists when the reconciliation process would result in

+an Ephemeral route configuration that would not be valid as a single, standalone

+Route. This means, in concept, a user could save the generated Ephemeral

+route config to my-foo-route.yaml, remove the parent and child routes from the

+cluster, then load my-foo-route.yaml into the cluster, and the system

+would accept and deploy it as a valid config.

+A config conflict may not be between config elements in the child and the

+parent. Instead, the conflict may be between config elements from two (or more)

+child routes that attach to the same RouteRule in the parent route. For this

+reason, when a config conflict is detected, implementations should identify the

+config elements that conflict, including the routes that are the original source

+of each element, and provide that info to users via appropriate methods of

+notification (e.g., logs, message in GUI, etc.).

We absolutely should, yes. All conflicts should be detectable via the status of the relevant resources.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930621532 :

    • conditions:
  •  - <conditions here>
    

+```

+We add a new grandparents stanza inside the the top-level parents status. This stanza will be +optional and omitempty, with the following rules:

+- grandparents will be nil in all non-parent HTTPRoutes.

+- Once a HTTPRoute is a parent, then this stanza must also be non-nil.

+We need to add this inside the parents stanza because it's possible that a parent HTTPRoute may be attached to more than one parent (probably Gateways) of its own, and there may be different status between different Gateways. For example, if a parent HTTPRoute is attached to two Gateways, one of which has a wildcard hostname, whether or not the child HTTPRoute attempting to attach to the parent HTTPRoute will be accepted is dependent on the hostname setting on the Gateway Listener. An example demonstrating this is included below.

+The struct itself has the same parentRef and conditions fields as the parents struct.

+The parentRef in the grandparents struct must be the same as the one on the parent Route's status. See the example for more clarity here.

+For Conditions:

+- when the controller first sees the parent Route, it must add the following Condition Types to the parent Route:

    • Attached: one or more Routes have successfully attached to this parent. False if zero Routes have attached successfully. Note that a new Reason has also been added, PartiallyAttached which indicates that some routes have tried to attach and failed.

It's a Reason, not a Condition, so I think we only ended up mentioning it here. I agree that this section could use more fleshing out though.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930621864 :

    • conditions:
  •  - <conditions here>
    

+```

+We add a new grandparents stanza inside the the top-level parents status. This stanza will be +optional and omitempty, with the following rules:

+- grandparents will be nil in all non-parent HTTPRoutes.

+- Once a HTTPRoute is a parent, then this stanza must also be non-nil.

+We need to add this inside the parents stanza because it's possible that a parent HTTPRoute may be attached to more than one parent (probably Gateways) of its own, and there may be different status between different Gateways. For example, if a parent HTTPRoute is attached to two Gateways, one of which has a wildcard hostname, whether or not the child HTTPRoute attempting to attach to the parent HTTPRoute will be accepted is dependent on the hostname setting on the Gateway Listener. An example demonstrating this is included below.

+The struct itself has the same parentRef and conditions fields as the parents struct.

+The parentRef in the grandparents struct must be the same as the one on the parent Route's status. See the example for more clarity here.

+For Conditions:

+- when the controller first sees the parent Route, it must add the following Condition Types to the parent Route:

    • Attached: one or more Routes have successfully attached to this parent. False if zero Routes have attached successfully. Note that a new Reason has also been added, PartiallyAttached which indicates that some routes have tried to attach and failed.

I should note that we've been talking about moving the Detached Condition to a positive-polarity Attached condition instead, to make this easier to understand. I think @mikemorris https://github.com/mikemorris logged the issue?

I may have been jumping ahead in this bit.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930622521 :

+We add a new grandparents stanza inside the the top-level parents status. This stanza will be +optional and omitempty, with the following rules:

+- grandparents will be nil in all non-parent HTTPRoutes.

+- Once a HTTPRoute is a parent, then this stanza must also be non-nil.

+We need to add this inside the parents stanza because it's possible that a parent HTTPRoute may be attached to more than one parent (probably Gateways) of its own, and there may be different status between different Gateways. For example, if a parent HTTPRoute is attached to two Gateways, one of which has a wildcard hostname, whether or not the child HTTPRoute attempting to attach to the parent HTTPRoute will be accepted is dependent on the hostname setting on the Gateway Listener. An example demonstrating this is included below.

+The struct itself has the same parentRef and conditions fields as the parents struct.

+The parentRef in the grandparents struct must be the same as the one on the parent Route's status. See the example for more clarity here.

+For Conditions:

+- when the controller first sees the parent Route, it must add the following Condition Types to the parent Route:

    • Attached: one or more Routes have successfully attached to this parent. False if zero Routes have attached successfully. Note that a new Reason has also been added, PartiallyAttached which indicates that some routes have tried to attach and failed.
    • Conflicted: one or more settings in the parent conflict with settings in one of the child routes. The message should contain more details. Routes that cause this to be true do not attach.
    • ResolvedRefs: one or more Routes have failed to attach because there is a problem with their reference. (for example, they could not be found). Those routes do not attach.
    • NotSupported: the controller that is reconciling this Route does not support attaching at least one of the routes that has attempted to attach, or the parent route is already a child route itself. Those routes do not attach.

I thought that having a separate condition to indicate that "this isn't supported" was better than overloading "Conflicted" (which I read to mean "this is a supported action, but some detail of it is incorrect").

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930622662 :

+- Once a HTTPRoute is a parent, then this stanza must also be non-nil.

+We need to add this inside the parents stanza because it's possible that a parent HTTPRoute may be attached to more than one parent (probably Gateways) of its own, and there may be different status between different Gateways. For example, if a parent HTTPRoute is attached to two Gateways, one of which has a wildcard hostname, whether or not the child HTTPRoute attempting to attach to the parent HTTPRoute will be accepted is dependent on the hostname setting on the Gateway Listener. An example demonstrating this is included below.

+The struct itself has the same parentRef and conditions fields as the parents struct.

+The parentRef in the grandparents struct must be the same as the one on the parent Route's status. See the example for more clarity here.

+For Conditions:

+- when the controller first sees the parent Route, it must add the following Condition Types to the parent Route:

    • Attached: one or more Routes have successfully attached to this parent. False if zero Routes have attached successfully. Note that a new Reason has also been added, PartiallyAttached which indicates that some routes have tried to attach and failed.
    • Conflicted: one or more settings in the parent conflict with settings in one of the child routes. The message should contain more details. Routes that cause this to be true do not attach.
    • ResolvedRefs: one or more Routes have failed to attach because there is a problem with their reference. (for example, they could not be found). Those routes do not attach.
    • NotSupported: the controller that is reconciling this Route does not support attaching at least one of the routes that has attempted to attach, or the parent route is already a child route itself. Those routes do not attach.

+It must also add the same types to the child Route.

Agreed, this needs more work.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930623262 :

+status:

  • parents:

    • parentRef:
  •  kind: HTTPRoute
    
  •  name: my-example
    
  •  namespace: prod-gw
    
  •  sectionName: store
    
  • controllerName: prod-gw

  • conditions:

    • type: Attached
  •  status: true
    
  •  reason: Attached
    
  •  message: Successfully attached to `store` section.
    
  • grandparents:

    • parentRef:
  •      kind: Gateway
    

Yes, I think the concept needs work, and it's probably worth implementing the rest and seeing what would be useful to have in each status.

The overall design goal here is for the owner of the child Route not to need to also fetch the parent Route and the Gateway to determine if their thing is being exposed correctly.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930624144 :

    • type: Attached
  •  status: true
    
  •  reason: Attached
    
  •  message: Successfully attached to `api` section.
    
  • grandparents:

    • parentRef:
  •      kind: Gateway
    
  •      name: prod-web-gw
    
  •    conditions:
    
  •    - type: Accepted
    
  •      status: true
    
  •      reason: Accepted
    
  •      Message: Route successfully attached to parent and gateway.
    

+```

+This is pretty verbose, but the extra information will be very important once we

Again, the idea here is to avoid the owner of the child Route needing to do a tree traversal to know the state of their child route. I can see the fanout concerns, but was hoping that by having the status refer back up the tree, rather than down, we should avoid some of those problems.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930624315 :

  •    kinds:
    
  •      - kind: HTTPRoute
    

This is from an earlier version that allowed other Kinds, and hasn't been updated.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930624635 :

  •  message: Accepted successfully
    
  • grandparents:

  •  - parentRef:
    
  •      name: gateway-a
    
  •    conditions:
    
  •    - type: Accepted
    
  •      status: true
    
  •      reason: Accepted
    
  •      message: Successfully accepted
    
    • controllerName: nadir
  • parentRef:

  •  name: http-parent
    
  • conditions:

    • type: Accepted
  •  status: true
    
  •  reason: GrandparentConfigError
    

Yeah, I agree this is confusing, and needs to be updated.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930625426 :

+## Alternatives

+### Extending backendRef to allow targeting a Route

+This idea was to allow backendRef to, optionally, specify a route instead of a

+service. Some of the reasons for not taking this approach include:

+- It would require changes to backendRefs. No changes to allowedRoutes are

  • needed to use it for this.

+- To minimize the changes to backendRef, it would require the owner of the

  • parent route to know the exact name of the child route. If the two routes are

  • in different namespaces, that might require cross-organization coordination

  • between administrators.

+- It would result in there being one way to attach routes to listeners and a

The other thing I don't think we mentioned here is that I think that the backendRef implies an extra routing hop, which I don't think will be common. This is a way to break up the config, not a way to represent extra routing hops.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930611713 :

+```yaml

+#### Parent Route #####

+kind: HTTPRoute

+metadata:

  • name: my-example

  • namespace: prod-gw

+spec:

  • parentRefs:

  • name: prod-web-gw

  • hostname: www.example.com

  • rules:

  • ...

    • name: store

+...

  •  allowedRoutes:
    

The problem with only allowing a single namespace is that it screws over migrations. If an app is moving to a new namespace, there will need to be a tightly coordinated sequence of edits to specific objects to ensure that config isn't lost.

In order to address that, we need more than one namespace available, and once we have more than one, it's easier to include a label selector than use a list of named namespaces (we've talked about this before).

In the interest of avoiding boilerplate, we could conceivably add a singular AllowedNamespace field that's just a string name, and a plural AllowedNamespaces field that has the label selector?

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930615599 :

  •       be used otherwise.
    
    1. Multiple child routes may attach to a single RouteRule, as long as they
  •   are distinct.
    

+3. Allow a Route’s ParentRef Kind to be the same type as the Route’s type

    1. Kind is required when the ParentRefs is a route. Kind wil default to
  •   `gateway` (current behavior)
    

+4. Add Name (type is [SectionName][sectionName]) to

  • [HTTPRouteRule][httpRouteRule] as optional

    1. The Name field is expected to be added in the implementation of
  •   [GEP-995 - Add Name to HTTPRouteRule and HTTPRouteMatch][gep-995]
    
    1. Default value is nil
    1. A child route may include a SectionName when its ParentRef is a route.
  •    1. A child route “attaches" to the parent route’s `RouteRule` that matches
    
  •       that `SectionName`. The child route must also match the criteria
    
  •       specified in the `RouteRule’s AllowedRoutes`.
    
    1. If the ParentRefs of a child route doesn’t include a SectionName, the

It's just about avoiding boilerplate in "simple" cases, I think. But I think it's equally fair to say that for this to work, route.Name must be specified, and the child route must reference it.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930616047 :

  •       that `SectionName`. The child route must also match the criteria
    
  •       specified in the `RouteRule’s AllowedRoutes`.
    
    1. If the ParentRefs of a child route doesn’t include a SectionName, the
  •   child route attaches to all of the Parent Route’s `RouteRules` having
    
  •   `AllowedRoutes` criteria that the child route meets.
    

+5. A parent Route and any related child Routes must be the same type of

  • Routes.

    1. Example: A child httpRoute can not point to a parent tcpRoute.

+6. A Route MUST NOT be a parent to a Route AND a child of another Route.

    1. Explanation: This limitation avoids the need for specific
  •   internal/terminal Route types, and the design of using a ParentRef from
    
  •   the child to an AllowedRoutes slot on a parent allows a Route to know if
    
  •   it is either a parent or child (or misconfigured as both) without
    
  •   requiring the graph traversal that a design like ForwardTo may need to
    
  •   avoid cycles.
    
    1. If an implementation chooses to not enforce this limitation, that must be

Agreed, we can leave this out because the whole feature is Extended conformance.

In site-src/geps/gep-1058.md https://github.com/kubernetes-sigs/gateway-api/pull/1085#discussion_r930617572 :

    1. If an implementation chooses to not enforce this limitation, that must be
  •   documented along with how many tiers of parent/child route relationships
    
  •   are allowed
    

+7. A Route’s ParentRefs can list more than one type of parent.

    1. Example: A HTTPRoute’s ParentRefs can list one (or more) Gateways and
  •   one (or more) `HTTPRoutes` at the same time.
    

+## Binding Parent and Child Routes

+Binding a child route to a parent route refers to the process of reconciling

+(i.e. combining) the two routes. Binding is treated as a single transaction for

+each Parent/Child route pair. Even when a child route attaches to multiple

+RouteRules in the parent route, the binding between the two routes is still a

+single transaction.

+The binding transaction has two stages; reconciliation and activation. The

The complexity of getting this right is why I'm of the opinion that we should err on the side of "what you ask for is what you get", and that if you ask for something that doesn't work, you get something that doesn't work, at least to begin with.

The alternative is that we need to carefully design the status to make clear that deadlocks have occurred and human intervention is required, since reconciliation cannot progress.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes-sigs/gateway-api/pull/1085#pullrequestreview-1051869404, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAUR2XPICTXIRVS6OW3CRTVWDATFANCNFSM5SQBPCAA . You are receiving this because you commented.Message ID: @.***>

costinm avatar Jul 27 '22 05:07 costinm

Before we do all of that though, I think that there seems to be some reasonably strong objections to this whole concept - it seems like the value proposition here is not clear, and I think that we may need to do more to explain:

I don't think I've seen any comment objecting to the concept or value of delegation - quite the opposite.

We may disagree about the requirements of how delegation should work ( in particular around security,policies, isolation), and on the spec imposing a specific implementation, or the complexity.

costinm avatar Jul 27 '22 16:07 costinm

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Jeff-Apple Once this PR has been reviewed and has the lgtm label, please ask for approval from bowei by writing /assign @bowei in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

k8s-ci-robot avatar Jul 30 '22 21:07 k8s-ci-robot

Looked at this over the weekend and the biggest problem that I had was from overloading the HTTPRoute object to be two different things depending on the context. This will likely be an ongoing problem, e.g. "is this HTTPRoute a parent route or a child route?", "should this HTTPRoute have done something or is it waiting for a child?" etc. The ephemeral route idea is an attempt to solve this problem but for me it just adds complexity.

I'd like to suggest an experiment: create a new object called "HTTPRouteTemplate" which would be what we're now calling a "parent route." It would attach to Gateways and would be attached to by HTTPRoutes. It would be a template for attaching HTTPRoutes and it would constrain its attached HTTPRoutes, just like parent routes do today. In other words, this is mostly the same functionality, just with clearer nomenclature.

This would eliminate a lot of confusion about what HTTPRoute is supposed to do, when, and in which context. Users would no longer need to try to figure out whether a given HTTPRoute was a parent or a child based on context. It no longer needs the ephemeral route idea: HTTPRouteTemplate does what it does, and HTTPRoute does what it does, and they're two different things. To me this is basically the same functionality as the current proposal but I find it much easier to understand if we give up the "grandparent-parent-child" idea.

caboteria avatar Aug 01 '22 12:08 caboteria

I think I like @caboteria's idea of a new resource focused entirely on HTTP delegation. If we kept it very limited in scope, it could end up simplifying this approach and resolving a number of the comments above. Each new resource we add comes with a significant cost, so we need to be careful here, but if it looks like delegation would only support a tiny subset of the overall HTTPRoute functionality it could be quite compelling.

I've been thinking about how we could move forward with this GEP as a whole, and I think one of the most important steps will be to outline how each capability of HTTPRoute today would or would not be supported by delegation. This seems like it could also help determine if a new delegation focused resource makes sense or if we really need most of the capabilities of an HTTPRoute for both parent and child resources.

Beyond that, here are some ideas that could potentially help get this GEP closer to merging. Some of these may not be relevant if we end up going further with @caboteria's suggestion above for a new resource to model this.

  1. Move expansion of status to later GEP, keep initial GEP focused on new reasons and conditions within existing structure
  2. Clearly state in the GEP that route delegation must be statically computable
  3. Simplify API by replacing "AllowedRoutes" struct with "DelegateToNamespace" string (name is not important here)
  4. Merge GEP as "Provisional" with the requirement that an OSS library that can support the requirements above can be developed within Gateway API before GEP graduates to "Implementable".

robscott avatar Aug 01 '22 21:08 robscott

I also like the idea of having a separate CRD for the parent route. In the spirit of bike shedding, I would call it HttpRouteDelegate or RouteDelegation - 'template' has a slightly different use.

That will make the use extremely clear and simple - and fits the 'policy attachment' model, it is a policy set on the gateway defining what routes can be attached by which namespace. We can start with what is likely the most common use cases - gateway admin delegating by URL prefix, etc. Would be great to have a list of the use cases and evaluate which are common enough.

Since you suggested Template - I can see the benefit of allowing an actual template - like /${namespace}/ - in the delegation policy.

As mentioned, keeping the HttpRoute ( which will be the most frequently used CR ) clean and simple and having the delegation as a separate policy is better for users.

Route 'inclusion' or some actual templating may also be a useful - but it would be ideal to separate delegation (which has a clear security and role separation purpose) from templating and inclusion.

costinm avatar Aug 07 '22 17:08 costinm

CLA Missing ID CLA Not Signed

  • :white_check_mark: login: Jeff-Apple / name: Jeff Apple (230a0e22104e344bf5d7b513dc4e077748d249a6, 1d3c2a2dd53f42e4d68c522a7f04b104f0243eac, 38af7c17e1c7829c60388a8d42b6595718d865ad, c1fbe4588ae1dda15e432c4ddbeb3d84dedb5b07, eec1d10fb0f0cc87beefc6819883a240b47a5ce7, 073ae7801ca0e1bdc0a90d094387f7586c973eb3, 419cdb4fc9263fa015be04e2c2a418700583cff2)
  • :white_check_mark: login: acnodal-tc / name: Toby Cabot (e97b2f0ceafef0bd93d504645d8b7e78f07ad493, 98cd15c23e9493f9ad44ae599766f25375310277, e34595620a609aa0f604d26ea0a915fff539bbd5, da445e031831163893e0afe9efe2c721e00db120, 626d399d01b2e58abbbdc0a16a5e026eaf0fe8e9, 6bc2c8d2ae1c7cc9576f5040af151c6f98d726c9, 421ee6f0cea03d94d6cc9b41c9a09cb5fe51398f, 46a36f7d2ef269c218f2f07939163afbef73d429)
  • :x: The commit (9655d7e23081678dc47a583dd802f98acdcce310). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

Talked about this in the sync: not considering a blocker for v0.6.0

shaneutt avatar Sep 19 '22 22:09 shaneutt

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Dec 18 '22 22:12 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar Jan 17 '23 23:01 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-triage-robot avatar Feb 16 '23 23:02 k8s-triage-robot

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

k8s-ci-robot avatar Feb 16 '23 23:02 k8s-ci-robot