Add PartiallyInvalid Route condition

[sunjayBhatia]

What type of PR is this?
/kind cleanup
/kind documentation

What this PR does / why we need it:
Fixes documentation on HTTPRoute filters for when incompatible filters are present.

Adds PartiallyInvalid Route condition with one example Reason, "IncompatibleFilters"

This condition can be used in cases where cases where some configuration is valid but some is not (so Accepted=True, but not everything is valid/supported)

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

Does this PR introduce a user-facing change?:

Add PartiallyInvalid Route condition to reflect Route status that is partially unsupported or invalid for an implementation.

[k8s-ci-robot]

Hi @sunjayBhatia. 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.

[robscott]

Thanks @sunjayBhatia!

[robscott]

/ok-to-test

[sunjayBhatia]

From discussion in the community meeting, this change may transition to be a more general "PartiallyInvalid" Condition that we can add the specific reason to, still to be discussed so no longer part of 0.6.0

This edge-case is caught by environments that have the webhook installed so not something that can come up in practice (at least with the existing known conflicting filters)

[shaneutt]

@sunjayBhatia just wanted to check in on the status of this one: is there any assistance we can provide to help move this one forward, as it's been sitting for some time?

[sunjayBhatia]

@sunjayBhatia just wanted to check in on the status of this one: is there any assistance we can provide to help move this one forward, as it's been sitting for some time?

just lost track of it, should make some more changes soon 👍🏽

[shaneutt]

just lost track of it, should make some more changes soon 👍🏽

Ok! Thanks Sunjay 🤜🤛

[pleshakov]

would it make sense to also consider how reporting incompatible filters would work when filters are part of an HTTPBackendRef? Will conditions and reasons be the same?

[shaneutt]

Just checking in again @sunjayBhatia, do you need any help here to move this one forward?

[pleshakov]

Hi @sunjayBhatia Please see my review.

In addition to the inline comments:

Currently, in httproute_types.go, there are a number of comments like below:

// Unknown values here must result in the implementation setting the
// Accepted Condition for the Route to status: False, with a
// Reason of UnsupportedValue.

Considering the new PartiallyInvalid condition, would it make sense to update them?

[pleshakov]

do we need to clarify somewhere what the data plane should return in any of those cases?

[sunjayBhatia]

I've been torn on what to do here, since this may end up being somewhat implementation-specific, depending on the field

[sunjayBhatia]

we could recommend a 500, similar to what happens when ResolvedRefs is set due to missing Service/ReferenceGrant (since a lot of these are covered by apiserver validation not sure how we'll write conformance tests etc.)

[youngnick]

Yeah, the exact behavior here depends on the field - for most HTTPRoute things, the field value tells you what to do for an invalid rule (return 500), and what to do if your whole Route is invalid (I think it's reject the whole route, but aren't sure).

[shaneutt]

@sunjayBhatia it appears to me in order to move this one forward there's some comments you'll need to look over. Sorry about how long it's taking, I'm doing a roundup of older PRs to see if we can start shoring some of these up. Let us know how we can best help you in this 🖖

[sunjayBhatia]

Hi @sunjayBhatia Please see my review.

In addition to the inline comments:

Currently, in httproute_types.go, there are a number of comments like below:

// Unknown values here must result in the implementation setting the
// Accepted Condition for the Route to status: False, with a
// Reason of UnsupportedValue.

Considering the new PartiallyInvalid condition, would it make sense to update them?

Feels like we should find a place to abstract this sort of thing out in the top-level Route documentation or otherwise rather than having to say this on each individual field, similar for the existing UnsupportedValue docs

[sunjayBhatia]

Hi @sunjayBhatia Please see my review.
In addition to the inline comments:
Currently, in httproute_types.go, there are a number of comments like below:

// Unknown values here must result in the implementation setting the
// Accepted Condition for the Route to status: False, with a
// Reason of UnsupportedValue.

Considering the new PartiallyInvalid condition, would it make sense to update them?

Feels like we should find a place to abstract this sort of thing out in the top-level Route documentation or otherwise rather than having to say this on each individual field, similar for the existing UnsupportedValue docs

for reviewers, take a look at 9b37a3e, put the documentation about partial invalidity in the route status section doc

[sunjayBhatia]

One thing for reviewers/participants on this issue, do we have a document where we state explicitly that implementations "MUST" try to reconcile/program/process a whole resource, regardless of whether they encounter an invalid value etc?

Seems like that is missing from https://gateway-api.sigs.k8s.io/concepts/guidelines/ but could be a worthy addition

[youngnick]

I'm happy with this as it stands today, so I'll approve and hold.

/approve
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sunjayBhatia, youngnick

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 [youngnick]

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

[robscott]

Thanks @sunjayBhatia! Sorry I missed this one, added a suggestion for how this could be split up into two smaller pieces.

[robscott]

This PR feels like it's combining 2 improvements that are related but different:

1. A new PartiallyInvalid reason when only a subset or rules in a Route are invalid
2. A new IncompatibleFilters reason

It seems like the following reasons could apply to an individual rule or the entire route:

• IncompatibleFilters
• UnsupportedValue (Accepted)
• BackendNotFound (ResolvedRefs)
• InvalidKind (ResolvedRefs)
• RefNotPermitted (ResolvedRefs)

Currently ResolvedRefs is only true if 100% of refs are resolved, that's probably fine to leave as is, but it may be confusing for ResolvedRefs to be true and PartiallyInvalid to be false.

I think the simplest path forward would be to add IncompatibleFilters as a possible reason for Accepted. Adding a PartiallyInvalid condition to reflect that a portion of rules are invalid makes sense, but I think we'd need to make broader changes to the other conditions on the route to have that make sense.

Another option would be to say that both IncompatibleFilters and UnsupportedValue reasons could be used for Accepted and PartiallyInvalid conditions. When used with the Accepted condition, nothing in the Route would be reconciled, but when used with the PartiallyInvalid condition, the valid subset of the Route would be reconciled. We could potentially leave ResolvedRefs and the associated reasons as a special case that always applies to the entire route since I think our recommended behavior is just to return 404s in those cases and not actually to stop reconciling anything. I'm less sure about this approach though, so maybe it's better as a follow up.

[sunjayBhatia]

added #2150 to start over and not have to disentagle the changes from this PR

[robscott]

Related to my comment above, this guidance doesn't really add up because the PartiallyInvalid condition is only intended for cases where a subset of rules are invalid, but here we're saying that it needs to be set anytime there are incompatible filters.

[k8s-ci-robot]

PR needs rebase.

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.

[shaneutt]

@sunjayBhatia from what I can tell, there's a couple open comments holding this up and we need a rebase in order to move forward. Let us know how we can be helpful with this one to keep it progressing? It's more than half a year old now and if there are some ways we can help pull this one over the finish line let us know? 🤔

[sunjayBhatia]

#2150 supercedes this so can close this one

[sunjayBhatia]

re: this comment: #1540 (comment)