conformance: check HTTPRoute connectivity after deleting relevant ReferenceGrant

[pmalek]

What type of PR is this?

/kind test
/area conformance

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #1838

Does this PR introduce a user-facing change?:

HTTPRoute connectivity is in now enforced in conformance tests if a relevant ReferenceGrant gets deleted.

[k8s-ci-robot]

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

[shaneutt]

This seems correct ultimately, but I do want some feedback from other implementers before we merge so let's hold. I think the 503 is particularly going to draw some questions.

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pmalek, shaneutt

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

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

[sunjayBhatia]

can you specify where in the spec we ask for a 503 specifically?

this section only specifies 500 return code:

gateway-api/apis/v1beta1/httproute_types.go

Lines 210 to 237 in cd1b6f5

	// BackendRefs defines the backend(s) where matching requests should be
	// sent.
	//
	// Failure behavior here depends on how many BackendRefs are specified and
	// how many are invalid.
	//
	// If *all* entries in BackendRefs are invalid, and there are also no filters
	// specified in this route rule, *all* traffic which matches this rule MUST
	// receive a 500 status code.
	//
	// See the HTTPBackendRef definition for the rules about what makes a single
	// HTTPBackendRef invalid.
	//
	// When a HTTPBackendRef 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.
	//
	// For example, if two backends are specified with equal weights, and one is
	// invalid, 50 percent of traffic must receive a 500. Implementations may
	// choose how that 50 percent is determined.
	//
	// Support: Core for Kubernetes Service
	//
	// Support: Implementation-specific for any other resource
	//
	// Support for weight: Core

500 or 503 seem like they could work, but if we want a specific code we should specify it

[pmalek]

can you specify where in the spec we ask for a 503 specifically?

this section only specifies 500 return code:

gateway-api/apis/v1beta1/httproute_types.go

Lines 210 to 237 in cd1b6f5

	// BackendRefs defines the backend(s) where matching requests should be
	// sent.
	//
	// Failure behavior here depends on how many BackendRefs are specified and
	// how many are invalid.
	//
	// If *all* entries in BackendRefs are invalid, and there are also no filters
	// specified in this route rule, *all* traffic which matches this rule MUST
	// receive a 500 status code.
	//
	// See the HTTPBackendRef definition for the rules about what makes a single
	// HTTPBackendRef invalid.
	//
	// When a HTTPBackendRef 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.
	//
	// For example, if two backends are specified with equal weights, and one is
	// invalid, 50 percent of traffic must receive a 500. Implementations may
	// choose how that 50 percent is determined.
	//
	// Support: Core for Kubernetes Service
	//
	// Support: Implementation-specific for any other resource
	//
	// Support for weight: Core

500 or 503 seem like they could work, but if we want a specific code we should specify it

That might have been an incorrect assumption on my side which just felt "right".

I believe this should indeed be specified just as you mentioned. What would be the best course of action for this then? Should this change include that mention or does it have to go through a different process where maintainers comment on this idea?

[sunjayBhatia]

That might have been an incorrect assumption on my side which just felt "right".

I believe this should indeed be specified just as you mentioned. What would be the best course of action for this then? Should this change include that mention or does it have to go through a different process where maintainers comment on this idea?

Not a maintainer, just a reviewer, but I am ok with making an API change along with the relevant test change as I think that keeps the justification for the test change bundled nicely. However, you may want to open an issue about the API spec change before making it just to make sure there's enough signal for people to discuss such a change

[pmalek]

@sunjayBhatia Got it. Created #1902 to track it.

[shaneutt]

@pmalek I think to move forward with this one, given some time has passed we need to make the appropriate spec change as noted in #1902 and signal that one as in progress. Let us know if you need any assistance!

[youngnick]

I just commented on #1902, but it's actually really important for security to ensure that the result code is the same if the service doesn't exist or if there's no ReferenceGrant so that you can't use that difference to enumerate service names.

[pmalek]

@youngnick As per the discussion in #1902 I've updated the PR to use HTTP 500. PTAL

[youngnick]

/lgtm