Update HTTPRoute BackendRef docs to clarify error behavior #1243
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
approved
| // | ||
| // A BackendRef is considered invalid when it refers to: | ||
| // If *all* entries in BackendRefs are invalid, and there are also no filters |
There was a problem hiding this comment.
Is this saying something different than "When a HTTPBackendRef is invalid, 500 status codes MUST be returned for"? They seem the same to me, and I wasn't sure if there was some subtle difference I was missing or if we were just re-iterating? Especially the "...also no filters..." part?
There was a problem hiding this comment.
Not really in this update, no, but there are a few places where "no valid backends" and "some valid backends" were treated differently, and I wanted to make doubly sure that there's no confusion.
There was a problem hiding this comment.
Will review the proposed docs changes more closely once this is out of draft, but suggested a few tweaks to the conformance tests for now.
Tagging @skriss for review as I'm suggesting restoring the route acceptance check that was removed in #1170 (review)
conformance/tests/httproute-invalid-cross-namespace-backend-ref.go
Outdated
Show resolved
Hide resolved
|
I updated the conformance tests quite a bit, because the changes to the docs imply some reasonably large changes:
So this is a much bigger conformance change than I anticipated. I've left this as WIP for now, because I think there's a couple more cases to test to be sure I've got everything I've changed in the docs, and I want to make all this behavior super clear, since it will have knock-on effects to #1112 and Inclusion/Delegation. |
youngnick
force-pushed
the
httproute-docs-update
branch
from
b2fb1de to
18f9c91
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Okay, I think I've covered everything I can for now in here, so marking this ready for review. The one case I haven't written a conformance test for is the "two backendRefs where one is invalid means that half of the traffic gets a 500", since I think we'll need some big helper changes for that, and I think we should talk about how we do that. I'll open an issue. |
youngnick
force-pushed
the
httproute-docs-update
branch
from
99fed27 to
ac7333a
Compare
k8s-ci-robot
added
the
do-not-merge/work-in-progress
|
I think that's everything. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
youngnick
force-pushed
the
httproute-docs-update
branch
from
91c7512 to
f1faa87
Compare
There was a problem hiding this comment.
Thanks @youngnick! This mostly LGTM, but I need to spend a bit more time to run through conformance tests.
apis/v1alpha2/httproute_types.go
Outdated
| // When a HTTPBackendRef refers to a Service that has no ready endpoints, | ||
| // implementations MAY return a 503 for requests to that backend instead. |
There was a problem hiding this comment.
How should this be interpreted? Is it valid to return 500 or 503 in this case?
There was a problem hiding this comment.
Agreed, it's not very clear. I guess our two options are either make it mandatory, or just mandate 500 for everything. I'm leaning towards the latter, so I'll just remove this.
conformance/tests/httproute-invalid-backendref-unknown-kind.yaml
Outdated
Show resolved
Hide resolved
| // HTTPRouteMustHaveConditions checks that the supplied HTTPRoute has the supplied Condition, | ||
| // halting after the specified timeout is exceeded. | ||
| func HTTPRouteMustHaveCondition(t *testing.T, client client.Client, routeNN types.NamespacedName, condition metav1.Condition, seconds int) { |
There was a problem hiding this comment.
It seems like we probably want to specify which parent this condition should exist for. Otherwise we could get rather imprecise results if more than one parent exists. This is not that common in conformance tests now, but it probably will become more common.
There was a problem hiding this comment.
Good point, I changed this to take the Gateway name it should be checking for.
youngnick
force-pushed
the
httproute-docs-update
branch
from
1e021b4 to
e5df467
Compare
| if cond.Status == metav1.ConditionStatus(condValue) { | ||
| return true | ||
| if cond.Reason == condReason { |
There was a problem hiding this comment.
I'm seeing a couple of tests start failing with this PR that I think should still pass:
- httproute-disallowed-kind
- httproute-invalid-cross-namespace-parent-ref
These both use the HTTPRouteMustHaveNoAcceptedParents function, which asserts that a Route has either no Accepted condition, or an "Accepted: false" condition.
At least in Contour's implementation, we're setting an "Accepted: false" condition in both of these scenarios. The problem arises because HTTPRouteMustHaveNoAcceptedParents does not pass a reason when checking for an "Accepted: false" condition, so findConditionsInList expects the actual condition to have an empty reason.
Not sure what the best fix is here, but one option would be to consider an empty reason string to mean "match any reason" instead of "match an empty string"? Otherwise, we might have to plumb a reason through from the place where HTTPRouteMustHaveNoAcceptedParents is called.
There was a problem hiding this comment.
Yeah, I agree that having an empty string passed for Reason can mean "match any Reason", since Reason strings should not normally be empty.
There was a problem hiding this comment.
Thanks @youngnick! This mostly LGTM, just a couple comments.
| @@ -81,7 +81,7 @@ func GWCMustBeAccepted(t *testing.T, c client.Client, gwcName string, seconds in | |||
| } | |||
|
|
|||
| controllerName = string(gwc.Spec.ControllerName) | |||
| return findConditionInList(t, gwc.Status.Conditions, "Accepted", "True"), nil | |||
| return findConditionInList(t, gwc.Status.Conditions, "Accepted", "True", "Accepted"), nil | |||
There was a problem hiding this comment.
I don't think we need to require reason to exactly match the condition name when it is in a success state. For example, when Istio sets the Ready condition, it sets the reason to "ListenersValid". Adding this kind of change will cause any implementations like that to immediately fail all conformance tests since this is part of the fundamental check if the base resources are ready. I think we could consider adding this check in the future if interested, but would prefer to leave it open to any reason now to avoid significant disruption before a release.
There was a problem hiding this comment.
I changed this to the empty string, with a comment.
| @@ -106,7 +106,7 @@ func NamespacesMustBeReady(t *testing.T, c client.Client, namespaces []string, s | |||
| t.Errorf("Error listing Gateways: %v", err) | |||
| } | |||
| for _, gw := range gwList.Items { | |||
| if !findConditionInList(t, gw.Status.Conditions, "Ready", "True") { | |||
| if !findConditionInList(t, gw.Status.Conditions, "Ready", "True", "Ready") { | |||
There was a problem hiding this comment.
Same comment as above, would prefer "" instead of "Ready" as the last arg. (If I understand that code, that will allow any reason).
There was a problem hiding this comment.
I changed this to the empty string, with a comment.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: skriss, 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-rebase
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
Signed-off-by: Nick Young <ynick@vmware.com>
youngnick
force-pushed
the
httproute-docs-update
branch
from
675a132 to
8abd902
Compare
k8s-ci-robot
removed
the
needs-rebase
robscott
added
the
tide/merge-method-squash
|
Thanks @youngnick! /lgtm |
k8s-ci-robot
added
the
lgtm
What type of PR is this?
/kind cleanup
/kind documentation
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1211
Updates #1112
Does this PR introduce a user-facing change?:
This PR is still WIP, since I will need to change the conformance tests to check for the behavior. But I wanted to check that we really are in agreement on this behavior while I work on those.