Fix attached routes confromance test

[frankbu]

What type of PR is this?

/kind bug
/area conformance

What this PR does / why we need it:

The spec does not say that a route with invalid backendRef and/or a parentRef to a gateway that is not "Programmed" should have the "Accepted" condition set to "False" (i.e., there is no corresponding route condition reason). All the spec is clear about is that the "ResolvedRefs" condition should be "False".

So, the conformance test should only be checking for the latter, not both conditions.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Fixed GatewayWithAttachedRoutes conformance test to not check that the HTTPRoute status includes an "Accepted: False" condition because this is not required by the specification.

[michaelbeaumont]

Ran into this as well, I think we can be explicit in the spec that Accepted is unrelated to both the parent listener's status and the ResolvedRefs condition.

[sunjayBhatia]

as pointed out here, looks like this test sets up an HTTPRoute with a nonexistent backend and the assertions made on its status conflict with an existing test: https://github.com/kubernetes-sigs/gateway-api/pull/2535/files#r1378444675

Regarding this particular HTTPRoute we can:

• ensure the assertions on the HTTPRoute status are equal between the two tests
• ensure the backend ref is valid on the HTTPRoute
  • however this has a side effect of no longer testing this part of the spec: https://github.com/kubernetes-sigs/gateway-api/blob/main/apis/v1/gateway_types.go#L855C37-L858C73

IMO for completeness we should keep the invalid HTTPRoute and also ensure that the Gateway in this test counts a fully valid HTTPRoute as being attached

[arkodg]

there's nothing the spec defining this, so shouldnt be enforced in the conformance test.
Envoy Gateway defines its own reason for this case NoReadyListeners

      - lastTransitionTime: "2023-10-27T01:38:14Z"
        message: There are no ready listeners for this parent ref
        observedGeneration: 1
        reason: NoReadyListeners
        status: "False"
        type: Accepted
      - lastTransitionTime: "2023-10-27T01:38:14Z"

it might make sense to add a similar reason upstream to improve debuggability for the end user

The listener is not ready in this case because the cert is invalid

      - lastTransitionTime: "2023-10-27T01:38:14Z"
        message: Secret gateway-conformance-infra/does-not-exist does not exist.
        observedGeneration: 2
        reason: InvalidCertificateRef
        status: "False"
        type: ResolvedRefs
      - lastTransitionTime: "2023-10-27T01:38:14Z"
        message: Listener is invalid, see other Conditions for details.
        observedGeneration: 2
        reason: Invalid
        status: "False"
        type: Programmed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, frankbu, michaelbeaumont

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:

• conformance/OWNERS [arkodg]

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

[sunjayBhatia]

there's nothing the spec defining this, so shouldnt be enforced in the conformance test. Envoy Gateway defines its own reason for this case NoReadyListeners

yeah, the Accepted condition documentation is pretty light:

gateway-api/apis/v1/shared_types.go

Lines 300 to 301 in c9cd2a1

	// This condition indicates whether the route has been accepted or rejected
	// by a Gateway, and why.

I tend to think of the Accepted condition here for routes with the context from how the Gateway/Listener version is documented in mind:

gateway-api/apis/v1/gateway_types.go

Lines 918 to 920 in c9cd2a1

	// This condition indicates that the listener is syntactically and
	// semantically valid, and that all features used in the listener's spec are
	// supported.

particularly the idea that the resource is syntactically and semantically valid means to me it should be Accepted=True but possibly Programmed=False due to the fact that the parent Listener has an invalid Secret

/cc @youngnick

since this is a bit more about ambiguity on the Accepted Condition

[sunjayBhatia]

regardless, this test is testing the AttachedRoutes count and the assertion on the HTTPRoute status is a secondary one basically to establish the test scenario/invariant

this change reduces some of the strength of the invariant assertions, but that seems ok, ultimately we want to make sure the AttachedRoutes count is correct

I still think we should also add and account for a fully valid HTTPRoute being attached to this "invalid" Gateway Listener for completeness

[frankbu]

I still think we should also add and account for a fully valid HTTPRoute being attached to this "invalid" Gateway Listener for completeness

That sounds reasonable but only AFTER we clarify the expectations of accepted. Right now, the test shows that as far as the Gateway is concerned, the route with a valid parentRef gets attached to an invalid gateway, even if the route is also invalid. The only thing that might change if the route is valid is the accepted condition on the route, but the test is currently punting on checking accepted completely, since it isn't clear from the spec what it should be in the broken backendRef case.

[youngnick]

I think that this PR is good for now - we do need to further clarify what Accepted means though, and this probably means releasing a 1.0.1, sigh.

[sunjayBhatia]

I still think we should also add and account for a fully valid HTTPRoute being attached to this "invalid" Gateway Listener for completeness

That sounds reasonable but only AFTER we clarify the expectations of accepted. Right now, the test shows that as far as the Gateway is concerned, the route with a valid parentRef gets attached to an invalid gateway, even if the route is also invalid. The only thing that might change if the route is valid is the accepted condition on the route, but the test is currently punting on checking accepted completely, since it isn't clear from the spec what it should be in the broken backendRef case.

just to get rid of any ambiguity then, maybe it is best to make the HTTPRoute in this test a fully valid one, and we can cover the case where an HTTPRoute is still counted when invalid once that spec clarification happens

[frankbu]

just to get rid of any ambiguity then, maybe it is best to make the HTTPRoute in this test a fully valid one, and we can cover the case where an HTTPRoute is still counted when invalid once that spec clarification happens

Personally, I would leave the test as is for now. It very clearly shows that an invalid route attached to an invalid gateway counts as an attached route, which the spec is very clear about: Listener or Route status does not impact successful attachment, i.e. the AttachedRoutes field count MUST be set for Listeners with condition Accepted: false and MUST count successfully attached Routes that may themselves have Accepted: false conditions.

Given the invalid -> invalid example, it's more obvious that a valid route -> invalid gateway would then also count. If we start by showing valid -> invalid works, I would say it is more ambiguous about what happens if the route is also invalid. As you said in your earlier comment this test is about attachedRoutes, the Accepted condition on the route (true or false) is a secondary concern, which I think the conformance test can more thoroughly check in the future, when we agree on the details.

[arkodg]

/lgtm
Will raise a follow up issue to track a better definition for the Route accepted status

[arkodg]

@frankbu can you add a Release Note in the PR Description ?

[robscott]

/cherry-pick release-1.0

[k8s-infra-cherrypick-robot]

@robscott: new pull request created: #2562

In response to this:

/cherry-pick release-1.0

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.