Adding new PartiallyInvalid condition for Routes

[robscott]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This adds a new condition along with corresponding guidance for how to handle Routes that are partially invalid.

Note: I can't think of any way to write conformance tests for this condition since the most likely way to get into this state is when an implementation can't support a feature configured in a Route.

Which issue(s) this PR fixes:
Fixes #1696.

Does this PR introduce a user-facing change?:

New "PartiallyInvalid" condition has been added for all Route types.

/hold for consensus

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [robscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaneutt]

I think having these two options and saying that you MUST do one or the other makes sense, because we don't want to have completely undefined behavior for partial invalidity between implementations.

I suppose conformance tests shouldn't be too hard, it will be like opting into a "feature" from a test triggering point of view, except you MUST opt for one or the other behaviors.

It seems like really late in the cycle for v1.0.0 we're seeing a few more of these "do one or the other" kind of things crop up (also see #2416), which definitely gives me a bit of pause.

I think at a high level I'm on board because this is an improvement for users over the previously undefined behavior, I definitely think we should make sure a greater number of community members look it over too though.

/cc @arkodg @sunjayBhatia @LiorLieberman @candita @gcs278 @mlavacca @kflynn @michaelbeaumont @rainest @Xunzhuo

[youngnick]

I would really have preferred that we could pick one or the other, but from discussion, it seems that we can't, so this is the next best thing.

LGTM, but will leave it for others to review.

[howardjohn]

Using message as a typed/specified thing seems a bit odd/unexpected/nonstandard?

[youngnick]

I agree, but there we don't have another way to indicate this information, and we really need to have some guidance around this before GA. Ideas welcomed, I guess.

[sunjayBhatia]

Maybe adding new condition types with this info would work? Another condition that would only be set if PartiallyInvalid is also set

- type: PartiallyInvalid
  status: true
  reason: UnsupportedValue
  message: ...
- type: PartiallyInvalid[DropRouteRule|RouteFallBack]
  status: true
  reason: Implementation[DropsRulesOnPartialInvalidity|FallsBackOnPartialInvalidity]
  message: ...

[robscott]

Yeah, agree this is a bit wonky, but I don't love the alternatives here. @sunjayBhatia's idea to add additional conditions to more clearly reflect this state may actually be something we need to do in the future, but I'd like to see if we can survive without adding more conditions first. If we try it out and determine that it would really be best to add additional conditions, we can do that later.

[mikemorris]

My biggest concern with this proposal is that I haven't seen any implementations explicitly state their desire to support it yet - I agree that we need to do something to resolve the discrepancy between spec documentation and GEP-1364, but I'm not sure if we want to add this to resolve a somewhat-abstract problem before determining this is the solution that at least some implementations actually want, especially given that dropping invalid rules is not without risks.

[sunjayBhatia]

Looks like most concerns I would have are captured by comments from others 👍🏽

Another outcome of this spec change/resolving the linked issue IMO would be agreeing on a plan on how to test this in conformance, especially given we have a MUST statement added (When this happens, implementations MUST take one of the following approaches). If we can come up with route rules that pass CEL validation but are universally invalid (not just implementation-specific invalid) my first thought is to add testing like:

• Add a new core feature, SupportsPartiallyInvalidRoutes (name tbd) and test that makes sure the status is set to true when there is a partially invalid route, likely ignoring reason/message
• Add a couple new extended features, SupportsPartiallyInvalidRoutes[DropRule|Fallback]
• Implementations would opt-in to the test they support
• We can modify a route to becoming partially invalid and test the invalid rule is dropped or the old configuration is respected, depending on what test is run
• Status checked for the right values depending on what test is run

I can include this comment in the original issue for this feature or a new one if that makes sense instead!

[sunjayBhatia]

My biggest concern with this proposal is that I haven't seen any implementations explicitly state their desire to support it yet - I agree that we need to do something to resolve the discrepancy between spec documentation and GEP-1364, but I'm not sure if we want to add this to resolve a somewhat-abstract problem before determining this is the solution that at least some implementations actually want, especially given that dropping invalid rules is not without risks.

In Contour we haven't explicitly stated in our Gateway API support/in the community discussions about this, but representing partial validity has been a bit of an issue in our own CRD/status/etc., so I'm enthusiastic about trying to do this in a standardized way with representing in status what is going to happen to invalid routes.

[sunjayBhatia]

I like that the two options described below are explicitly stated. In Contour we've had a somewhat inconsistent mix in our own CRD throughout history of "dropping" invalid rules, programming 5xx responses for invalid rules while preserving valid ones, attempting to program as much of an invalid rule as we can but giving warnings/errors in status about invalid attributes of a rule, etc., which all make for a bit of an inconsistent UX. Explicitly giving users feedback in status about what is happening is a step forward IMO.

[robscott]

Yeah, completely agree, I think many implementations have inconsistent behavior here.

[robscott]

Another outcome of this spec change/resolving the linked issue IMO would be agreeing on a plan on how to test this in conformance, especially given we have a MUST statement added (When this happens, implementations MUST take one of the following approaches). If we can come up with route rules that pass CEL validation but are universally invalid (not just implementation-specific invalid) my first thought is to add testing like:

• Add a new core feature, SupportsPartiallyInvalidRoutes (name tbd) and test that makes sure the status is set to true when there is a partially invalid route, likely ignoring reason/message
• Add a couple new extended features, SupportsPartiallyInvalidRoutes[DropRule|Fallback]
• Implementations would opt-in to the test they support
• We can modify a route to becoming partially invalid and test the invalid rule is dropped or the old configuration is respected, depending on what test is run
• Status checked for the right values depending on what test is run

Agree that this would make a good follow up. I created #2458 based on this.

[robscott]

Thanks to everyone for the feedback! I think I've resolved everything now, PTAL.

[robscott]

I think we've got enough consensus here, going to remove the hold so the next LGTM merges this.

/hold cancel

[youngnick]

/lgtm