Conflicting guidance when a HTTPRoute refers to an invalid backend

[robscott]

I was trying to write an issue describing what conformance tests should look like for this, and I think.

On the HTTPBackendRef type definition we say that if the referent can't be found or is not allowed by a ReferenceGrant the BackendRef must be dropped from the Gateway.

gateway-api/apis/v1beta1/httproute_types.go

Lines 870 to 879 in 3f4b981

	// If the referent cannot be found, this HTTPBackendRef is invalid and must
	// be dropped from the Gateway. The controller must ensure the
	// "ResolvedRefs" condition on the Route is set to `status: False` and not
	// configure this backend in the underlying implementation.
	//
	// If there is a cross-namespace reference to an *existing* object
	// that is not covered by a ReferenceGrant, the controller must ensure the
	// "ResolvedRefs" condition on the Route is set to `status: False`,
	// with the "RefNotPermitted" reason and not configure this backend in the
	// underlying implementation.

On the BackendRefs field we say "When a BackendRef is invalid, 500 status codes MUST be returned for requests that would have otherwise been routed to an invalid backend." "Invalid" is defined to include both of the scenarios described above.

gateway-api/apis/v1beta1/httproute_types.go

Lines 207 to 218 in 3f4b981

	// A BackendRef is considered invalid when it refers to:
	//
	// * an unknown or unsupported kind of resource
	// * a resource that does not exist
	// * a resource in another namespace when the reference has not been
	// explicitly allowed by a ReferenceGrant (or equivalent concept).
	//
	// When a BackendRef is invalid, 500 status codes MUST be returned for
	// requests that would have otherwise been routed to an invalid backend. If
	// multiple backends are specified, and some are invalid, the proportion of
	// requests that would otherwise have been routed to an invalid backend
	// MUST receive a 500 status code.

I think the rationale behind both of these conflicting guidelines makes sense in isolation. Maybe the following guidelines make sense:

1. Don't attach a Route to a Gateway if a Route has no valid BackendRefs or include a filter with an invalid reference - this will result in a 404 unless another Route matches.
2. Do attach a Route to a Gateway if only a portion of BackendRefs are invalid - this will result in a 500 for traffic that would have otherwise been routed to the invalid BackendRef.

The motivation for 2 is to avoid dropping a Route entirely if someone adds an invalid reference as part of a traffic split, ie send 10% to canary service, but that new reference to canary service is broken. I'm not sure that 2 is worthwhile, so an alternative would be to just say that Routes should not be attached if they contain any invalid references.

Note: I think this is a blocker for v0.5.0, but not v0.5.0-rc1.

I know a lot of people have been involved in this broader discussion around status codes, apologies if I miss anyone here. As always, appreciate anyone's thoughts on this.

/cc @mikemorris @nathancoleman @youngnick @howardjohn @shaneutt

[mikemorris]

This had some prior discussion in #1081 (review) (tl;dr Contour currently supports partial acceptance, Consul API Gateway does not yet but is open to adding support if adopted by Gateway API Spec) and refs #1143 (#1112 has some related discussion but is a bit sprawling and this issue seems prefferrable to discuss a more narrow scope).

While 1 initially seems simple and possibly preferable for authoring new Routes, the strongest motivation I've seen for supporting partial acceptance is "lifecycle" watches - if a ReferenceGrant is revoked or a service deleted, having an entire Route become detached could be an unexpected UX and potentially a very large blast radius (and splitting that logic to have different things happen for new versus existing routes may be confusing or more difficult to implement). With the split between 404 and 500 responses, it should now be more straightforward for an operator to monitor and diagnose 500 responses as "bad config that needs to be corrected" so +1 for partial acceptance.

[robscott]

the strongest motivation I've seen for supporting partial acceptance is "lifecycle" watches - if a ReferenceGrant is revoked or a service deleted, having an entire Route become detached could be an unexpected UX and potentially a very large blast radius

+1 this is a great point.

Maybe we need some nuance in our spec (and conformance tests) that suggests the following:

• Never attach any routes that include invalid references
• If an existing reference becomes invalid in a route that already exists (ie ReferenceGrant or Service deletion) or a new invalid reference is added return a 500 for requests that would have been routed to that backend

Is it too convoluted to try and implement that kind of split logic based on if a Route has already been admitted?

[mikemorris]

Is it too convoluted to try and implement that kind of split logic based on if a Route has already been admitted?

This has been my concern basically, that the controller/observer pattern and Kubernetes client-go API make this at best unintuitive to implement - the most direct approach I can think of would be for validation to check for a RouteCondition Accepted { status: true } matching the controller/gateway, but not sure if that's potentially subject to abuse? I'd expect comparing against actual gateway state would be more complex and possibly run into issues of "what counts as changed versus new?" when doing something like modifying the hostnames field which may cause a route to select a different gateway listener.

[youngnick]

Using a status field to communicate past state like that is fiddly and unusual, yeah.

The big problems around this lie in reconstructing the state after a controller restart, when it's lost the context of what config it's seen before.

If we were going to do it, we would have to mandate that all Conditions MUST update the observedGeneration field with the metadata.generation of the object as they last saw it. This would allow logic like this to infer that a status is for a previous version of the config. (The metadata.generation field is automatically incremented by the apiserver each time the spec of an object is changed.) This makes the idea possible, not safe.

@thockin did push an update to the API conventions recently to relax the old restriction that "Status must be able to be recreated totally from observation of the Kube API", to allow the storage of some information like this.

I'd definitely want to hear more from Tim or other upstream API reviewers about a proposal like this before we went very far with it though, it smells scary to me.

That said, I think that we badly need a solution for the problem of "This spec update breaks things, do I just pass on the broken config?", and this would definitely help. I tend to be a proponent of "Yes, just pass on the breakage and be careful", because there are so many dragons around storing state in objects that can be manipulated by others outside of an implementation's control, and in reconstructing the state after an implementation crashes or unexpectedly fails in some other way, but that doesn't mean I like that solution. 😄

[robscott]

The big problems around this lie in reconstructing the state after a controller restart, when it's lost the context of what config it's seen before.

I think we should be careful here to allow room for controllers to store their own state if they are able to. I think in some cases that will provide the best possible experience.

If we were going to do it, we would have to mandate that all Conditions MUST update the observedGeneration field with the metadata.generation of the object as they last saw it.

I think what I'm suggesting is simpler than that. If something in status indicates that this Route is attached to the Gateway we could act differently than if the Route has not been attached to the Gateway. We already have conformance tests that ensure that when a Route is attached, status is updated to indicate that. It seems like it could be reasonable to use that as a reference point to either leave the Route attached and return 500 for affected backends, or leave the Route unattached because there is a problem in the config. I think this lines up with our "prefer not to break things" guideline.

I definitely agree that relying on reconstructing state from status is dangerous, and we have to be very careful with what we rely on in status. My personal opinion here is that the benefits justify the risks here.

Alternatives we could choose instead:

1. Even if there are invalid references in a route, attach the route, and return 500 responses in place of those invalid references. This would be bad if the invalid reference came from a filter, in which case everything matching that Route rule would receive a 500 response.
2. Drop and/or don't attach the Route if there are invalid references. If there is a large Route and only one of many references has become invalid, this feels especially painful. With that said, it does seem preferable to avoid attaching anything broken to begin with.

The benefits of a hybrid approach as I'd suggested above are that we don't attach new config if it is bad, but if existing config becomes invalid (say a ReferenceGrant is deleted), we implement the smallest possible break by returning 500 for requests that would otherwise be destined for the backend.

I'd definitely want to hear more from Tim or other upstream API reviewers about a proposal like this before we went very far with it though, it smells scary to me.

+1, very interested in feedback from @thockin, @lavalamp, @apelisse and others on if this would be an acceptable use of status.

[mikemorris]

Instead of checking past status, could it be plausible to just check for invalid references in an admission webhook? It's a bit more work than a similar approach for preventing gateway listener conflicts due to checking for the existence of referenced objects beyond the applied config, and does have the more restrictive UX of not allowing routes to be deployed before a service (unclear if that's desirable or not though).

Would that allow the "regular" lifecycle validation to remain stateless and just return the smallest scope 500s due to cluster changes while preventing the bigger operator footguns? (I've heard some mentions of how it's possible to circumvent the webhook - or just not deploy it, but I don't think that fundamentally changes anything here from a security posture, just removes a guardrail and makes it easier to apply bad config.)

[howardjohn]

IMO, doing it in a webhook is the wrong approach. Its possible, but it makes operations a huge pain to have cross-resource checking. Sorry to give a negative opinion without an alternative -- still trying to think it over..

[lavalamp]

Originally the "only reproducible things in status" rule was because we thought status might eventually have less durability than spec. That is absolutely not how anything ended up, that's not ever going to be a thing.

Nowadays, if you want to store some state in status, I'd say, knock yourself out. But it means you have to be very very careful in your controller to not accidentally mangle something important. There's lots of conditions to consider, such as accidentally two copies of your controller trying to reconcile at the same time. Use observedGeneration and preconditions on write (e.g. set resourceVersion to ensure you overwrite the copy of the object that you observed) to help.

[youngnick]

I think that whatever we end up doing, we should mandate the use of observedGeneration in all Conditions, and enforce it with conformance. It's optional in the type itself, (I suspect for back-compat reasons), but I think that Conditions are way more racy without it.

As I said earlier, I agree that the outcome is probably worth the risks of doing it, we'll just need to be careful in the design and explicit in the spec.

[rainest]

Invalid references that require cross-object checks (which ReferenceGrant-allowed backends would need) are tricky. The Listener one is nice and clean because it's possible to answer given a Gateway only.

From my default stance of assuming that users will apply broken configuration given the means to, I'd lean towards 2 with caveats. Currently, both expose clients to failures they cannot address; (1) just does so more completely, whereas (2) has:

the proportion of requests that would otherwise have been routed to an invalid backend MUST receive a 500 status code

which I read as "given 50/50 weight between a valid and invalid backend, fail half your requests randomly". Offhand I'd lean towards instead sending all of them to the valid backend and raising the problem to administrators via a Condition in the Route status, even if there's not a great fit for one (the only status information is in relation to the parent, which individual problem backends aren't really related to for an otherwise attached Route).

Absent spec permission to simply ignore bad backends on the gateway side, I do like the "(1), but skip applying if that would detach an attached Route" hybrid approach in theory, but I don't think it's able to handle cases where a backend becomes invalid rather than starts there. If you apply a backend that's permitted under some ReferenceGrant and then delete the ReferenceGrant, what do you apply? The currently attached configuration will send traffic that's no longer permitted, so it must be removed, but the proposed configuration will discard the route entirely (or break a portion of it with (2)).

If you're able to selectively exclude a problem backend to escape that state, and can apply updated, legal configuration after a policy change, it seems like it'd make sense to do that from the outset, rather than only allowing it in some cases when a now-problem backend was already in live configuration.

[youngnick]

Offhand I'd lean towards instead sending all of them to the valid backend and raising the problem to administrators via a Condition in the Route status, even if there's not a great fit for one (the only status information is in relation to the parent, which individual problem backends aren't really related to for an otherwise attached Route).

This exact problem is one of the reasons I've been more in favor of the more brutal, "break things if the config says to" approach. If we do what you say above, we're guessing that the only reason you'd ever do this is because it's a mistake, and so we're put in the position of assuming what people intend when configuring resources in a way that's not specified in the API.

To put it another way, we're guessing that you don't ever want anything to break, which seems reasonable at first glance, but historically, people are used to looking for things breaking in certain ways (of which, getting a percentage of 500s is one). I worry that by doing this, we're going to be subverting people's expectations of how load-balancing systems work. Old-school LB devices don't say "you made a misconfiguration, we don't think you wanted to do that, so we just won't accept that config". Is that the business we want to be in?

I know that's a slippery slope argument, which is not ideal, but I feel like the point of this API is to allow people to specify what they want without any magic. If you ask for a broken config, you get a broken config - as much as it sucks to end up with a broken config, I feel like it's better to have what you asked for be exactly what you get.

[EmilyShepherd]

I prefer explicit 500s rather than ignoring routes because I feel like, given an invalid configuration, we may not know what the user wanted, but we may be able to determine want they didn't want.

Say we have setup one rule to route / to a valid backend (A) and another rule to route /foo to an invalid backend (B).

When deciding how to handle this, we don't know if /foo was indeed meant to go to B but the user forgot to set up the referencegrant, or if it actually meant another backend (C), but we can be relatively certain that the user does not expect /foo traffic to go to backend A.

Simply ignoring invalid rules, I believe, would have that undesired effect I think. I am therefore a plus one for explicitly 500'ing all such cases.