GatewayUnsupportedRoutabilityMutation test seems unnecessary

[sunjayBhatia]

Routability conformance tests were added in #2171 and various issues are being fixed in #2207

The GatewayUnsupportedRoutabilityMutation added in the original PR has some issues and seems unnecessary, or at least has parts of it that are:

gateway-api/conformance/tests/gateway-routability.go

Line 126 in 88bb307

var GatewayUnsupportedRoutabilityMutation = suite.ConformanceTest{

From what I understand, the test updates a Gateway with a routability value that is invalid, specifically invalid in that it does not pass the api field validation. It seems this is just testing the kubebuilder validation annotation?

This error classification also does not encompass field validation errors:

gateway-api/conformance/tests/gateway-routability.go

Line 157 in 88bb307

if apierrs.IsBadRequest(err) {

(the following line is still run and the test fails there)

=== RUN   TestGatewayConformance/GatewayUnsupportedRoutabilityMutation
    suite.go:230: Applying tests/gateway-routability-bad-mutation.yaml
    apply.go:177: Creating gateway-bad-routability-mutation Gateway
    helpers.go:302: Gateway expected observedGeneration to be updated to 1 for all conditions, only 0/2 were updated. stale conditions are: Accepted (generation 0), Programmed (generation 0)
    helpers.go:302: Gateway expected observedGeneration to be updated to 1 for all conditions, only 0/2 were updated. stale conditions are: Accepted (generation 0), Programmed (generation 0)
    helpers.go:302: Gateway expected observedGeneration to be updated to 1 for all conditions, only 1/2 were updated. stale conditions are: Programmed (generation 0)
    gateway-routability.go:152: 
        	Error Trace:	/home/sunjayb/workspace/gateway-api/conformance/tests/gateway-routability.go:152
        	            				/home/sunjayb/workspace/gateway-api/conformance/utils/suite/suite.go:234
        	            				/home/sunjayb/workspace/gateway-api/conformance/utils/suite/experimental_suite.go:236
        	Error:      	Received unexpected error:
        	            	Gateway.gateway.networking.k8s.io "gateway-bad-routability-mutation" is invalid: spec.infrastructure.routability: Invalid value: "a.bad.vendor.prefix/bad!!": spec.infrastructure.routability in body should match '^Public|Private|Cluster|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-_]+$'
        	Test:       	TestGatewayConformance/GatewayUnsupportedRoutabilityMutation
        	Messages:   	error updating Gateway: Gateway.gateway.networking.k8s.io "gateway-bad-routability-mutation" is invalid: spec.infrastructure.routability: Invalid value: "a.bad.vendor.prefix/bad!!": spec.infrastructure.routability in body should match '^Public|Private|Cluster|[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-_]+$'
    apply.go:185: Deleting gateway-bad-routability-mutation Gateway

If we want to keep the test, we could just assert updating the Gateway fails with an invalid routability value, and remove the rest of the test.

[robscott]

Yeah good catch, I think we should just remove the conformance test. For testing API validation we usually add invalid manifests to hack/invalid-examples. It could be useful to add an example there at the same time as the test is removed.

[sunjayBhatia]

/assign

Looks like there are existing examples there already that have good coverage

[dprotaso]

The invalid-examples is there because we have language that disallows creation of gateways with a invalid routability values.

For mutations (UPDATE) the GEP has the following requirement

Implementations MAY prevent end-users from updating the routability value of a Gateway. If updates are allowed the semantics and behaviour will depend on the underlying implementation.

If a Gateway is mutated but does not support the desired routability it MUST set the conditions Accepted, Programmed to False with Reason set to UnsupportedRoutability. Implementations MAY choose to leave the old Gateway running with the previous generation's configuration.

That's why the test was asserting that it's rejected by the webhook or the conditions are set to the appropriate value - since it could be either or.

Thus I think the right thing to do is fix the apierrs.IsBadRequest to apierrs.IsInvalid

[sunjayBhatia]

I might be wrong, but I think one of the expected conditions of the conformance tests is to run in an environment with the webhook enabled, so I'm not sure an implementation will ever see a routability with a value as written in the test (a.bad.vendor.prefix/bad!!) since it will always be rejected by the webhook, if the test is intended to let implementations prevent updating the field then a different routability value should have probably been set there (and we can bring the test back)

[dprotaso]

I agree - I don't know what's the best course of action for the invalid routability fixture string constant.

In theory implementations should reject a value they do not support. I picked a constant knowing it would fail the field regexp - a.bad.vendor.prefix/bad!!

I could pick something that passes the regexp but should fail - eg. does the literal example.com/bad-value make sense?

[sunjayBhatia]

yeah choosing a routability value that is clearly invalid, e.g. contains the word invalid should be fine I would guess for 99.9% of implementations, very unlikely someone would support that