mesh conformance: add extended level features

[howardjohn]

What type of PR is this?
/area conformance

What this PR does / why we need it:
This PR moves a few mesh tests to "extended" level because they do not have widespread support

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

[howardjohn]

/assign @robscott

[howardjohn]

These cannot be part of the feature set. IMO this is due to a limitation of the framework. What we want is

ProvesFeatures: [features.SupportMeshConsumerRoute]
DependsOnFeatures: [features.SupportHTTPRoute, features.SupportHTTPRouteResponseHeaderModification]

That is, you cannot be SupportMeshConsumerRoute without this test, but you can be SupportHTTPRoute without this test. However, you need SupportHTTPRoute to be SupportMeshConsumerRoute due to testing implications

[robscott]

I'm not sure why we need to remove these features? The test will not run if you don't claim support for any of the features in the list, so simply adding SupportMeshConsumerRoute should be sufficient here. We shouldn't assume that SupportMeshConsumerRoute implies SupportHTTPRouteResponseHeaderModification right?

[howardjohn]

IIUC, without this an implementation cannot claim SupportHTTPRouteResponseHeaderModification without passing this test.

This test does not test header modification. It depends on header modification as a mechanism to make assertions, but doesn't prove nor deny support for response header modifications.

[robscott]

IMO this would be testing the intersection between two extended features, and no implementation should be required to pass tests that cover features they're not claiming to support (x-ref #3025 (comment)). I realize that likely requires a docs update, but would rather do that than unnecessarily remove features from the list here.

[howardjohn]

Sorry, but that really doesn't make sense to me. Let me draw an analogy outside of these tests...

Consider two features, SupportsJSON and SupportsObscureFeature.

In order to test ObscureFeature, we happen to send JSON requests. But JSON and ObscureFeature are not correlated in any way other than implementation details of the test, which happen to use JSON for convience.

IIUC, you are saying "An implementation cannot claim JSON support without supporting ObscureFeature". Is that accurate?

(JSON=RouteResponseHeaderModification,ObscureFeature=MeshConsumerRoute)

[robscott]

I think we're agreeing with each other and I'm just not communicating well. I'm saying that any test that covers the intersection of two extended features should only be required if an implementation claims support for both extended features. By extension, that also means that we should always try to have at least some isolated test coverage for every extended feature that does not rely on other extended features.

[howardjohn]

Got it. So that implies splitting Features into 2 fields?

[robscott]

I think features already has the functionality you want? We only run a test if the implementation is claiming support for all features associated with the test. I don't really think we need any distinction here since the test is providing coverage for all features listed, the biggest thing we need to work on is ensuring that we actually have isolated test coverage for every extended feature that doesn't rely on other extended features. Open to suggestions for how to improve this further though.

[robscott]

Thanks @howardjohn! This change makes sense to me, but would be good to get some feedback from other mesh implementations as well.

/cc @kflynn @michaelbeaumont

[mlavacca]

Since we are introducing Mesh Extended features, we need to include them in the Mesh conformance profiles as well, here:

gateway-api/conformance/utils/suite/profiles.go

Lines 109 to 129 in 7352409

	// MeshHTTPConformanceProfile is a ConformanceProfile that covers testing HTTP
	// service mesh related functionality.
	MeshHTTPConformanceProfile = ConformanceProfile{
	Name: MeshHTTPConformanceProfileName,
	CoreFeatures: sets.New(
	features.SupportMesh,
	features.SupportHTTPRoute,
	),
	ExtendedFeatures: features.HTTPRouteExtendedFeatures,
	}
	// MeshGRPCConformanceProfile is a ConformanceProfile that covers testing GRPC
	// service mesh related functionality.
	MeshGRPCConformanceProfile = ConformanceProfile{
	Name: MeshHTTPConformanceProfileName,
	CoreFeatures: sets.New(
	features.SupportMesh,
	features.SupportGRPCRoute,
	),
	}
	)

[howardjohn]

@mlavacca thanks, done

[kflynn]

/lgtm

I completely agree with @howardjohn that separating things we're testing from things we depend on would be really nice, but I think that clarifying that consumer routes and IP matching are extended is worth doing.

[robscott]

Thanks @howardjohn!

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: howardjohn, robscott

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

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