rm EnableAllSupportedFeatures & add AllGatewayFeatures #1907

arkodg · 2023-04-04T18:49:21Z

With Initial Mesh (GAMMA) conformance tests #1878 the existing flag EnableAllSupportedFeatures used in implementations to run conformance tests pertaining to all Gateway/Ingress features will now also run Mesh conformance tests which might not be the intent.
rm EnableAllSupportedFeatures and ask Ingress implementations to set SupportedFeatures to AllGatewayFeatures to achieve the same intent as before. Mesh implementations that also support Gateway features can set SupportedFeatures to AllFeatures.

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

the `EnableAllSupportedFeatures` flag has been removed. You can set `SupportedFeatures` to `AllGatewayFeatures` to run all tests related to Gateway/Ingress or you may set `SupportedFeatures` to `AllFeatures` to run all tests related to Gateway as well as Mesh

* With kubernetes-sigs#1878 the existing flag `EnableAllSupportedFeatures` used in implementations to run conformance tests pertaining to all Gateway/Ingress features will now also run Mesh conformance tests which might not be the intent. * rm `EnableAllSupportedFeatures` and ask Ingress implementations to set `SupportedFeatures` to `AllGatewayFeatures` to achieve the same intent as before. Mesh implementations that also support Gateway features can set `SupportedFeatures` to `AllFeatures`. Signed-off-by: Arko Dasgupta <arko@tetrate.io>

k8s-ci-robot · 2023-04-04T18:49:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arkodg
Once this PR has been reviewed and has the lgtm label, please assign shaneutt for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

conformance/OWNERS

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

arkodg · 2023-04-04T22:27:01Z

ptal @shaneutt

sunjayBhatia

I don't think theres anything correlating the passed in string AllGatewayFeatures or AllFeatures to the right list of tests when using go test ./conformance -supported-features=...? Looks good when using your own suite but when using go test on the suite from this repo it only runs the tests from StandardCoreFeatures

conformance/utils/suite/features.go

arkodg · 2023-04-05T00:39:28Z

I don't think theres anything correlating the passed in string AllGatewayFeatures or AllFeatures to the right list of tests when using go test ./conformance -supported-features=...? Looks good when using your own suite but when using go test on the suite from this repo it only runs the tests from StandardCoreFeatures

looks like some parse logic exists here

gateway-api/conformance/conformance_test.go

Line 72 in 37768ab

func parseSupportedFeatures(f string) sets.Set[suite.SupportedFeature] {

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

sunjayBhatia · 2023-04-05T13:09:08Z

I don't think theres anything correlating the passed in string AllGatewayFeatures or AllFeatures to the right list of tests when using go test ./conformance -supported-features=...? Looks good when using your own suite but when using go test on the suite from this repo it only runs the tests from StandardCoreFeatures

looks like some parse logic exists here

gateway-api/conformance/conformance_test.go

Line 72 in 37768ab

func parseSupportedFeatures(f string) sets.Set[suite.SupportedFeature] {

I tried it out and only StandardCoreFeatures tests are run, everything else is skipped

$ go test -v -timeout=20m ./conformance -gateway-class=contour -supported-features=AllGatewayFeatures
=== RUN   TestConformance
    conformance_test.go:54: Running conformance tests with contour GatewayClass
         cleanup: true
         debug: false
         supported features: [AllGatewayFeatures]
         exempt features: []
...
=== RUN   TestConformance/HTTPRouteCrossNamespace
    suite.go:200: Skipping HTTPRouteCrossNamespace: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteDisallowedKind
    suite.go:200: Skipping HTTPRouteDisallowedKind: suite does not support HTTPRoute
...

AllGatewayFeatures is parsed but when we filter tests that does not actually result in any tests being run outside of StandardCoreFeatures which are added by default:

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

Lines 201 to 205 in 26ce3a3

    
           for _, feature := range test.Features { 
        
           	if !suite.SupportedFeatures.Has(feature) { 
        
           		t.Skipf("Skipping %s: suite does not support %s", test.ShortName, feature) 
        
           	} 
        
           }

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

Lines 91 to 95 in 228104a

    
           } else { 
        
           	for feature := range StandardCoreFeatures { 
        
           		s.SupportedFeatures.Insert(feature) 
        
           	} 
        
           }

arkodg · 2023-04-05T19:33:30Z

I don't think theres anything correlating the passed in string AllGatewayFeatures or AllFeatures to the right list of tests when using go test ./conformance -supported-features=...? Looks good when using your own suite but when using go test on the suite from this repo it only runs the tests from StandardCoreFeatures

looks like some parse logic exists here

gateway-api/conformance/conformance_test.go

Line 72 in 37768ab

func parseSupportedFeatures(f string) sets.Set[suite.SupportedFeature] {

I tried it out and only StandardCoreFeatures tests are run, everything else is skipped
$ go test -v -timeout=20m ./conformance -gateway-class=contour -supported-features=AllGatewayFeatures
=== RUN   TestConformance
    conformance_test.go:54: Running conformance tests with contour GatewayClass
         cleanup: true
         debug: false
         supported features: [AllGatewayFeatures]
         exempt features: []
...
=== RUN   TestConformance/HTTPRouteCrossNamespace
    suite.go:200: Skipping HTTPRouteCrossNamespace: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteDisallowedKind
    suite.go:200: Skipping HTTPRouteDisallowedKind: suite does not support HTTPRoute
...
AllGatewayFeatures is parsed but when we filter tests that does not actually result in any tests being run outside of StandardCoreFeatures which are added by default:

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

Lines 201 to 205 in 26ce3a3

for _, feature := range test.Features {

if !suite.SupportedFeatures.Has(feature) {

t.Skipf("Skipping %s: suite does not support %s", test.ShortName, feature)

}

}

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

Lines 91 to 95 in 228104a

} else {

for feature := range StandardCoreFeatures {

s.SupportedFeatures.Insert(feature)

}

}

thanks for pointing this out. If support for the supported-features flag is broken on main, imo, would be better to raise a separate issue and tackle it in a another PR

sunjayBhatia · 2023-04-05T19:54:47Z

thanks for pointing this out. If support for the supported-features flag is broken on main, imo, would be better to raise a separate issue and tackle it in a another PR

This is specific to the added new features. This PR adds two new feature "sets" that can be used when you're writing your own suite, but the new feature names are not actually respected when using -supported-features. Yes they are passed through but this is a problem because no tests are actually tagged with AllGatewayFeatures or AllFeatures (because these new "features" are a collection of features). When a user does -supported-features=AllFeatures if we don't want to tag every test with AllFeatures, then we need to add (in this change) something that translates that passed in string to a set of tests for the suite to run, otherwise the suite logic from here will just run StandardCoreFeatures:

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

Lines 89 to 95 in 228104a

    
           if s.SupportedFeatures == nil { 
        
           	s.SupportedFeatures = StandardCoreFeatures 
        
           } else { 
        
           	for feature := range StandardCoreFeatures { 
        
           		s.SupportedFeatures.Insert(feature) 
        
           	} 
        
           }

arkodg · 2023-04-05T20:06:23Z

thanks for pointing this out. If support for the supported-features flag is broken on main, imo, would be better to raise a separate issue and tackle it in a another PR

This is specific to the added new features. This PR adds two new feature "sets" that can be used when you're writing your own suite, but the new feature names are not actually respected when using -supported-features. Yes they are passed through but this is a problem because no tests are actually tagged with AllGatewayFeatures or AllFeatures (because these new "features" are a collection of features). When a user does -supported-features=AllFeatures if we don't want to tag every test with AllFeatures, then we need to add (in this change) something that translates that passed in string to a set of tests for the suite to run, otherwise the suite logic from here will just run StandardCoreFeatures:

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

Lines 89 to 95 in 228104a

if s.SupportedFeatures == nil {

s.SupportedFeatures = StandardCoreFeatures

} else {

for feature := range StandardCoreFeatures {

s.SupportedFeatures.Insert(feature)

}

}

seeing this issue on main w/o this PR change

git log -n1
commit ccc922b1f186d519fcab9a2c430864a3b60e63ca (HEAD -> main, upstream/main)
Merge: 8dde5080 2acae499
Author: Kubernetes Prow Robot <k8s-ci-robot@users.noreply.github.com>
Date:   Tue Apr 4 09:53:42 2023 -0700

    Merge pull request #1894 from shaneutt/shaneutt/mesh-conformance-machinery-patch
    
    Conformance Tests: No implicit test runs without features

🐳 ~/go-workspace/src/github.com/kubernetes-sigs/gateway-api/conformance$ go test -v ./conformance_test.go -args -gateway-class=envoy-gateway -supported-features=AllFeatures debug=true
=== RUN   TestConformance
    conformance_test.go:54: Running conformance tests with envoy-gateway GatewayClass
         cleanup: true
         debug: false
         enable all features: false 
         supported features: [AllFeatures]
         exempt features: []
........
=== RUN   TestConformance/HTTPRouteQueryParamMatching
    suite.go:203: Skipping HTTPRouteQueryParamMatching: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRedirectHostAndStatus
    suite.go:203: Skipping HTTPRouteRedirectHostAndStatus: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRedirectPath
    suite.go:203: Skipping HTTPRouteRedirectPath: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRedirectPort
    suite.go:203: Skipping HTTPRouteRedirectPort: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRedirectScheme
    suite.go:203: Skipping HTTPRouteRedirectScheme: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteReferenceGrant
    suite.go:203: Skipping HTTPRouteReferenceGrant: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRequestHeaderModifier
    suite.go:203: Skipping HTTPRouteRequestHeaderModifier: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteResponseHeaderModifier
    suite.go:203: Skipping HTTPRouteResponseHeaderModifier: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRewriteHost
    suite.go:203: Skipping HTTPRouteRewriteHost: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteRewritePath
    suite.go:203: Skipping HTTPRouteRewritePath: suite does not support HTTPRoute
=== RUN   TestConformance/HTTPRouteSimpleSameNamespace
    suite.go:203: Skipping HTTPRouteSimpleSameNamespace: suite does not support HTTPRoute
=== RUN   TestConformance/MeshBasic
    suite.go:203: Skipping MeshBasic: suite does not support Mesh
=== RUN   TestConformance/MeshTrafficSplit
    suite.go:203: Skipping MeshTrafficSplit: suite does not support Mesh
=== RUN   TestConformance/TLSRouteSimpleSameNamespace
    suite.go:203: Skipping TLSRouteSimpleSameNamespace: suite does not support TLSRoute

sunjayBhatia · 2023-04-05T20:51:13Z

seeing this issue on main w/o this PR change

right, the -all-features flag covered that use case before, now that is taken away there is no way to run all the features in the suite via go test or AllGatewayFeatures

k8s-ci-robot · 2023-04-25T06:30:04Z

PR needs rebase.

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

So I don't mean to put a damper on this PR, but since this isn't making it into v0.7.0 anyway, and it's looking likely that v0.8.0 will actually just get collided into v1.0.0 where we will have conformance profiles which enable subscribing to large groups of tests maybe we can pass on this one for now? Or is there some really strong need I'm misunderstanding?

/hold

arkodg · 2023-05-03T17:24:57Z

closing since its not needed but a good to have

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 4, 2023

k8s-ci-robot requested review from howardjohn and sunjayBhatia April 4, 2023 18:49

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 4, 2023

sunjayBhatia suggested changes Apr 5, 2023

View reviewed changes

conformance/utils/suite/features.go Outdated Show resolved Hide resolved

rm all-features doc string

228104a

Signed-off-by: Arko Dasgupta <arko@tetrate.io>

sunjayBhatia mentioned this pull request Apr 18, 2023

Fixes to allow the mesh feature to be tested, including alone #1949

Merged

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2023

shaneutt reviewed May 3, 2023

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2023

arkodg closed this May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rm EnableAllSupportedFeatures & add AllGatewayFeatures #1907

rm EnableAllSupportedFeatures & add AllGatewayFeatures #1907

arkodg commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

arkodg commented Apr 4, 2023

sunjayBhatia left a comment

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023 •

edited

Loading

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023

k8s-ci-robot commented Apr 25, 2023

shaneutt left a comment

arkodg commented May 3, 2023

rm EnableAllSupportedFeatures & add AllGatewayFeatures #1907

rm EnableAllSupportedFeatures & add AllGatewayFeatures #1907

Conversation

arkodg commented Apr 4, 2023

k8s-ci-robot commented Apr 4, 2023

arkodg commented Apr 4, 2023

sunjayBhatia left a comment

Choose a reason for hiding this comment

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023 • edited Loading

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023

arkodg commented Apr 5, 2023

sunjayBhatia commented Apr 5, 2023

k8s-ci-robot commented Apr 25, 2023

shaneutt left a comment

Choose a reason for hiding this comment

arkodg commented May 3, 2023

sunjayBhatia commented Apr 5, 2023 •

edited

Loading