search

kubernetes-sigs / gateway-api

Repository for the next iteration of composite service (e.g. Ingress) and load balancing APIs.
https://gateway-api.sigs.k8s.io
Apache License 2.0
1.85k stars 480 forks source link

Request Filtering Between Gateways and Namespaced Routes #634

Closed Freyert closed 2 years ago

Freyert commented 3 years ago

What would you like to be added: A method for Gateway administrators to use routing rules to filter which traffic can be forwarded to downstream routes.

As a Gateway administrator managing an HTTP load balancer I would like to delegate particular path prefixes to namespaced HTTPRoutes to prevent namespace administrators from writing rules that conflict with the rules from other namespaces.

For example, I should be able to delegate /cats to the cats namespace and /dogs to the dogs namespace. Then the administrators for each of those namespaces can safely write any rule they want, but with the expectation that the path rules are prefixed with /cats or /dogs. /cats/food and /dogs/food would never collide because the Gateway administrator had already pre-filtered the traffic.

Why is this needed: Currently the API allows request filtering for HTTPRoutes and TCPRoutes. Gateway administrators can delegate traffic routing down to those routes which should be owned by individual namespace administrators (Think Hub and Spoke).

This immediately raises the issue of how can a central administrator prevent multiple namespaces from writing conflicting routing rules. Current documentation states that if namespace A and namespace B create the same routes the conflict will be resolved via timestamp (HTTPRouteRule).

howardjohn commented 3 years ago

I didn't put too much thought into this, but would it makes sense to have something like: Gateway my-gw in ns gw: select routes in same namespace only HTTPRoute my-route in ns gw: match /cats, forwardTo HTTPRoute cats in cats namespace, match /dogs, forwardTo HTTPRoute dogs in dogs namespace HTTPRoute dogs in ns dogs: ... HTTPRoute cats in ns cats: ...

This introduces a new concept of forwardTo another route. I am pretty sure this was in some of the early designs?

robscott commented 3 years ago

Yep this concept was in the early designs and something I wanted to revisit for v1alpha2. It really covers 2 related ideas - Routes forwarding to other namespaces, and Routes forwarding to other Routes. I think both are worth exploration as part of v1alpha2. There's also a bit more context for this in the related Slack thread.

robscott commented 3 years ago

@howardjohn I think you've provided a great example of how this could work but it also shows how it could significantly alter the namespace boundary. If Routes can forward to other Routes, it would be difficult/impossible for a Gateway owner to ensure that all Routes it forwarded to were actually in the same zone. It also makes any tree visualization of how resources are connected in the API more complicated.

So although I think this idea has a lot of potential, I also want to recognize that it will be difficult to get it right.

stevesloka commented 3 years ago

We did something similar in Contour with IngressRoute which was called delegation. It matches @howardjohn's example where a Route would delegate a match condition off to another resource by namespace+name. The successor to IngressRoute, HTTPProxy does something similar but uses an idea of includes where like a header file in code, you include the route conditions on another resource giving that resource full rights to manage whatever was included.

Something that has come out of this is folks sometimes want both models. A way to allow users fully manage their own fqdns+match conditions without a "delegation model", and others do want the enforced delegation model. It's tricky to implement both at the same time.

Maybe if the gateway selected a "delegatedRoute" of sorts, that would inform that you'd have to pass off information other Routes? This doesn't address your concerns @robscott, however. 😞

youngnick commented 3 years ago

The thing I would take away from Contour's IngressRoute and HTTPProxy experience here is that naming is important. :) Initially, IngressRoute used "delegation" because we were thinking of it like a DNS delegation. However, functionally, it worked out more similar to inclusion, and when we built HTTPProxy and needed to allow for more conditions than just path (headers as well to start with), it kind of broke the model to an extent, because if you are delegating control of routes and imposing conditions on that delegation, it feels very different to what we normally think of as "delegation". "Inclusion" was the answer to trying to make the conditions around the inclusion multi-dimensional (that is, not just based on path/prefix).

tl;dr Contour's experience shows there is definitely strong demand for something like this, but we should consider how the delegation/inclusion works when we are naming it. I'm happy to talk at a meeting one time about this, since "inclusion" was my suggestion for HTTPProxy.

banks commented 3 years ago

One potential issue opened up by if the spec simply allowed ForwardTo to name another Route resource would be routing cycles. These might be somewhat hard to detect manually. It would certainly be possible for implementations to perform cycle checking on the graph at validation time and reject them but that does impose complication on the implementations. Beyond cycles, they also need to verify that every hop is to a route of a compatible type/protocol (maybe only same type makes sense?). Arbitrary length forwarding paths similarly would make visualising and debugging harder.

None of those is a deal breaker (we did something similar in Consul) but I agree with @robscott that it could be "difficult to get it right".

One idea to throw into the ring (without a huge amount of detailed thought) would be to have a more limited redirection capability. At it's simplest that could just be a validation requirement that *Route.ForwardTo may only select another *Route provided that destination route is terminal (i.e. doesn't forward to another route). That does still put a complexity burden on implementations though. Perhaps a shared library could mitigate.

An alternative might be to have a distinct family of route types that are allowed to Forward to other routes but not destinations, perhaps called *InternalRoute or something. Gateways would be allowed to select both internal and regular route resource types. That has downsides of some duplication in the types and extra API surface area for users to understand, but could make implementation and reasoning about the system simpler - internal routes can only select non-internal routes. It would be easy enough to prevent application teams ever creating internal routes too within their namespaces with RBAC if that was deemed dangerous so they could exist only too allow a central team to handle "prefix" routing while delegating further routing to application teams. It also limits the DAG to be at most two levels of routing with no cycles which makes validating/debugging and other tooling easier to reason about.

Our experience with Consul's Routing is that cycle checking during validation and arbitrarily deep routing DAGs are possible but handling all the edge cases that could arise is relatively expensive and complex despite the fact that probably no one actually needs more than 2 or so levels of redirection. Building UIs to visualise that requires spending time working out how to deal with those edge cases of arbitrary nesting too.

jpeach commented 3 years ago

For example, I should be able to delegate /cats to the cats namespace and /dogs to the dogs namespace.

This issue is talking about delegation based on path, but in practice delegation will need to be based on arbitrary filtering criteria. This is consistent with John's comment, but I wanted to make it explicit.

Configuration cycle detection is necessary and feasible (e.g. Contour does it today). I don't think the per-hop protocol consistency is an issues, since we are talking about logical "configuration hops", not actual networking hops.

The biggest issue in my mind is how this fits with our security and configuration model. We already have a lot of complexity based on conflicting deployment models, and I'm concerned about reproducing another layer of the same complexity.

youngnick commented 3 years ago

This issue is talking about delegation based on path, but in practice delegation will need to be based on arbitrary filtering criteria. This is consistent with John's comment, but I wanted to make it explicit.

@jpeach has said what I was trying to say earlier about inclusion versus delegation, much more clearly. In Contour's case, once we allowed more filtering criteria, the verb "delegation" didn't make sense.

That's super important in this case, because, as James says below, allowing a Route to reference another Route isn't about having the routes create actual routing hops, it's effectively about making a big, flattened object that has per-level RBAC. That's what Routes importing/including/referencing another Route is all about, breaking up the responsibility for the overall Gateway config up between (application developer) teams.

youngnick commented 3 years ago

Leaving aside the exact mechanics of how the Route to Route referencing would work, there's some other data we can draw from Contour's experience here, but it will require a little explanation. I'll try and do a tl;dr as well.

In HTTPProxy, there are effectively two types of objects. The "root" HTTPProxy, which specifies an FQDN, and a "child" HTTPProxy, which doesn't. Referencing (including) a "root" HTTPProxy inside another root is an error, the include will be marked as invalid.

A root HTTPProxy that is including something looks like this:

apiVersion: projectcontour.io/v1
kind: HTTPProxy
metadata:
  name: include-root
  namespace: default
spec:
  virtualhost:
    fqdn: root.bar.com
  includes:
  # Includes the /service2 path from service2 in the same namespace
  - name: service2
    namespace: default
    conditions:
    - prefix: /service2

However, you can see two useful things from this example: Firstly, there's a namespace reference in the "includes" section, which allows cross-namespace inclusion, and secondly, there's a set (poorly-named before upstream Conditions were a thing) of conditions that are imposed on any routes included. Effectively the included Route is "wrapped" with those conditions, and adding things that contradict them is an error.

The condtions here can also include header presence or values, so you can include with both a prefix and a header, or just one or the other. Theoretically though, we could have added support for any of the routing filters we support.

Preventing cycles is the reason why we only allow substring prefix matching on path, without any wildcard or regex matching.

If we replace "root HTTPProxy" in the above with "Gateway", and "child HTTPProxy" with "HTTPRoute", then it's broadly similar, I think.

To summarise: Having something like HTTPProxy's Inclusion between Gateway and Routes might be a way to get some of the way to supporting @Freyert's use case here. It would give us a two-level object set, at the cost of needing to add something into the Listener that would wrap a Route in some set of things (like a AddRoutePredicate field or something). This may be more than we want to put into Gateway, which would suggest that we may need a two-tier Route system (basically, a RouteThatCanReference and a TerminatingRoute or something).

robscott commented 3 years ago

This is really helpful, some great discussion here. Some quick follow up questions for @youngnick or @jpeach on the Contour implementation:

  1. You mention that you only support prefix matching, what does that look like? In your example, the HTTPProxy includes service2 with a prefix condition on /service2, does that mean that all matches in the included proxy would have to start with /service2 (ie /service2/foo) or would /foo be automatically translated to /service2/foo?

  2. Is there any way for HTTPProxy resources to prevent themselves from being included from other namespaces? If not, has that ever been an issue for you?

  3. How broadly have you seen this inclusion pattern used? Has the direct reference model ever resulted in any scalability issues?

Just as a general note, I think this is a very important concept to keep in mind as we're considering changing the Gateway -> Route binding model (https://github.com/kubernetes-sigs/gateway-api/issues/594). My initial proposal there really focuses on Route -> Gateway selection as the primary mechanism. That would not map as naturally to the top-down Route inclusion method discussed here.

At it's simplest that could just be a validation requirement that Route.ForwardTo may only select another Route provided that destination route is terminal... Our experience with Consul's Routing is that cycle checking during validation and arbitrarily deep routing DAGs are possible but handling all the edge cases that could arise is relatively expensive and complex despite the fact that probably no one actually needs more than 2 or so levels of redirection.

+100. Agree that we should enforce some kind of limit here, that probably is simply just 1 extra layer of inclusion/delegation, ie the maximum complexity allowed would be Gateway -> Route -> Route.

youngnick commented 3 years ago

Answering your questions, @robscott:

  1. All routes on the included HTTPProxy have /service2 prepended to them, so /foo inside that proxy becomes /service2/foo.
  2. No, there's no way to prevent HTTPProxy objects from being included, noone has ever asked.
  3. People are definitely using the inclusion pattern, there are a couple of big prod users that use it. We haven't found any scaling issues with the direct reference model, and some of the prod users are using thousands of proxies (although I don't have good information on how many are included vs including).

I agree that a limit on inclusion makes sense, it certainly makes building the object parser much easier. :)

stevesloka commented 3 years ago

In preparation for the meeting on Monday, I've started a discussion document that tries to outline the discussions into a single point. I also added some more examples of how Contour implements the same today to allow for discussion on that as well as the places where more work/thought is needed.

https://docs.google.com/document/d/1-0mgRRAY784OgGQ1_LCOshpLLbeAtIr4eXd0YVYK4RY/edit?usp=sharing

robscott commented 3 years ago

We've got a lot of the pieces in place for Route delegation/inclusion thanks to GEP #709 and #724, but I think we're going to add this after the initial v1alpha2 release. I'm going to move this out of the v1alpha2 milestone to account for that.

k8s-triage-robot commented 3 years ago

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:

You can:

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

/lifecycle stale

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle rotten

robscott commented 2 years ago

@Jeff-Apple is working on a GEP for this.

/remove-lifecycle rotten

k8s-triage-robot commented 2 years ago

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:

You can:

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

/lifecycle stale

youngnick commented 2 years ago

This discussion is relevant for the GEP in #1058, but I think has probably been superseded. @robscott?

robscott commented 2 years ago

Yep, I think #1058 will cover this and we can safely close this issue now. Feel free to reopen if not.

/close

k8s-ci-robot commented 2 years ago

@robscott: Closing this issue.

In response to [this](https://github.com/kubernetes-sigs/gateway-api/issues/634#issuecomment-1079390576): >Yep, I think #1058 will cover this and we can safely close this issue now. Feel free to reopen if not. > >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.