Skip to content

Add flags for SupportedFeatures and ExemptFeatures in conformance tests. #1394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 4, 2022
Merged

Add flags for SupportedFeatures and ExemptFeatures in conformance tests. #1394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bfc24a
Add flags for SupportedFeature and ExemptFeature in conformance tests.
Sep 20, 2022
c7c0164
Remove suite.SupportReferenceGrant as the default value of SupportedF…
Sep 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions conformance/conformance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ limitations under the License.
package conformance_test

import (
"strings"
"testing"

"sigs.k8s.io/gateway-api/apis/v1alpha2"
Expand Down Expand Up @@ -45,15 +46,37 @@ func TestConformance(t *testing.T) {

t.Logf("Running conformance tests with %s GatewayClass", *flags.GatewayClassName)

supportedFeatures := parseSupportedFeatures(*flags.SupportedFeatures)
exemptFeatures := parseExemptFeatures(*flags.ExemptFeatures)

cSuite := suite.New(suite.Options{
Client: client,
GatewayClassName: *flags.GatewayClassName,
Debug: *flags.ShowDebug,
CleanupBaseResources: *flags.CleanupBaseResources,
SupportedFeatures: []suite.SupportedFeature{
suite.SupportReferenceGrant,
Copy link
Contributor

@mikemorris mikemorris Sep 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the original intent of having this specific feature hardcoded was to include ReferenceGrant in the default set of conformance tests - I'm not sure if we should try to keep a "default supported features" list somehow (feels awkward), or instead just change conformance tests currently enabled with the flag to instead switch to an ExemptFeature flag to opt-out.

Copy link
Author

@gyohuangxin gyohuangxin Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can make a "default supported features" which runs all the conformance tests, and we can change enabled tests with SupportedFeatures flag or just opt-out tests with ExemptFeatures flag. What do you think?

Copy link
Contributor

@mikemorris mikemorris Sep 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could just remove the suite.SupportReferenceGrant constant, and remove it from the Features list on all conformance tests that currently expect it to be set.

This keeps those tests included by default, still allows using the ExemptReferenceGrant flag to opt-out, avoids any complexity of an extra "default" list, and feels okay given that we're intending to promote ReferenceGrant to v1beta1 in the upcoming v0.6.0 Gateway API release.

Copy link
Author

@gyohuangxin gyohuangxin Sep 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikemorris Thanks for your reply, I can understand it basically. What to do are:

  1. Remove suite.SupportReferenceGrant as the default value of SupportedFeatures
  2. Remove suite.SupportReferenceGrant from Features list on all tests, such as https://github.com/kubernetes-sigs/gateway-api/blob/main/conformance/tests/gateway-secret-invalid-reference-grant.go#L37. Then replace it to Exemptions: []suite.ExemptFeature{ suite.ExemptReferenceGrant, },, so that --exempt-features ExemptReferenceGrant flag can be set to opt-out.

Can I confirm it is correct?

Copy link
Contributor

@mikemorris mikemorris Sep 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that should be correct @gyohuangxin!

I think we can then do:
3. Remove the constant at

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

Lines 47 to 48 in 583e4e2

// This option indicates support for the ReferenceGrant object.
SupportReferenceGrant SupportedFeature = "ReferenceGrant"

In upcoming Gateway API v0.6.0, all implementations should assume ReferenceGrant is supported unless explicitly setting ExemptReferenceGrant.

Copy link
Author

@gyohuangxin gyohuangxin Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikemorris Thanks! I've updated the code and the "user-facing change" section per your comments, please review again.

},
SupportedFeatures: supportedFeatures,
ExemptFeatures: exemptFeatures,
})
cSuite.Setup(t)
cSuite.Run(t, tests.ConformanceTests)
}

// parseSupportedFeatures parses the arguments for supported-features flag,
// then converts the string to []suite.SupportedFeature
func parseSupportedFeatures(f string) []suite.SupportedFeature {
var res []suite.SupportedFeature
for _, value := range strings.Split(f, ",") {
res = append(res, suite.SupportedFeature(value))
}
return res
}

// parseExemptFeatures parses the arguments for exempt-features flag,
// then converts the string to []suite.ExemptFeature
func parseExemptFeatures(f string) []suite.ExemptFeature {
var res []suite.ExemptFeature
for _, value := range strings.Split(f, ",") {
res = append(res, suite.ExemptFeature(value))
}
return res
}
2 changes: 1 addition & 1 deletion conformance/tests/gateway-secret-invalid-reference-grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func init() {
var GatewaySecretInvalidReferenceGrant = suite.ConformanceTest{
ShortName: "GatewaySecretInvalidReferenceGrant",
Description: "A Gateway in the gateway-conformance-infra namespace should fail to become ready if the Gateway has a certificateRef for a Secret in the gateway-conformance-web-backend namespace and a ReferenceGrant exists but does not grant permission to that specific Secret",
Features: []suite.SupportedFeature{suite.SupportReferenceGrant},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/gateway-secret-invalid-reference-grant.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
gwNN := types.NamespacedName{Name: "gateway-secret-invalid-reference-grant", Namespace: "gateway-conformance-infra"}
Expand Down
2 changes: 1 addition & 1 deletion conformance/tests/gateway-secret-missing-reference-grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func init() {
var GatewaySecretMissingReferenceGrant = suite.ConformanceTest{
ShortName: "GatewaySecretMissingReferenceGrant",
Description: "A Gateway in the gateway-conformance-infra namespace should fail to become ready if the Gateway has a certificateRef for a Secret in the gateway-conformance-web-backend namespace and a ReferenceGrant granting permission to the Secret does not exist",
Features: []suite.SupportedFeature{suite.SupportReferenceGrant},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/gateway-secret-missing-reference-grant.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
gwNN := types.NamespacedName{Name: "gateway-secret-missing-reference-grant", Namespace: "gateway-conformance-infra"}
Expand Down
2 changes: 1 addition & 1 deletion conformance/tests/gateway-secret-reference-grant-all-in-namespace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func init() {
var GatewaySecretReferenceGrantAllInNamespace = suite.ConformanceTest{
ShortName: "GatewaySecretReferenceGrantAllInNamespace",
Description: "A Gateway in the gateway-conformance-infra namespace should become ready if the Gateway has a certificateRef for a Secret in the gateway-conformance-web-backend namespace and a ReferenceGrant granting permission to all Secrets in the namespace exists",
Features: []suite.SupportedFeature{suite.SupportReferenceGrant},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/gateway-secret-reference-grant-all-in-namespace.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
gwNN := types.NamespacedName{Name: "gateway-secret-reference-grant", Namespace: "gateway-conformance-infra"}
Expand Down
2 changes: 1 addition & 1 deletion conformance/tests/gateway-secret-reference-grant-specific.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func init() {
var GatewaySecretReferenceGrantSpecific = suite.ConformanceTest{
ShortName: "GatewaySecretReferenceGrantSpecific",
Description: "A Gateway in the gateway-conformance-infra namespace should become ready if the Gateway has a certificateRef for a Secret in the gateway-conformance-web-backend namespace and a ReferenceGrant granting permission to the specific Secret exists",
Features: []suite.SupportedFeature{suite.SupportReferenceGrant},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/gateway-secret-reference-grant-specific.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
gwNN := types.NamespacedName{Name: "gateway-secret-reference-grant", Namespace: "gateway-conformance-infra"}
Expand Down
6 changes: 2 additions & 4 deletions conformance/tests/httproute-invalid-cross-namespace-backend-ref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ func init() {
var HTTPRouteInvalidCrossNamespaceBackendRef = suite.ConformanceTest{
ShortName: "HTTPRouteInvalidCrossNamespaceBackendRef",
Description: "A single HTTPRoute in the gateway-conformance-infra namespace should set a ResolvedRefs status False with reason RefNotPermitted when attempting to bind to a Gateway in the same namespace if the route has a BackendRef Service in the gateway-conformance-web-backend namespace and a ReferenceGrant granting permission to route to that Service does not exist",
Exemptions: []suite.ExemptFeature{
suite.ExemptReferenceGrant,
},
Manifests: []string{"tests/httproute-invalid-cross-namespace-backend-ref.yaml"},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/httproute-invalid-cross-namespace-backend-ref.yaml"},
Test: func(t *testing.T, suite *suite.ConformanceTestSuite) {
routeNN := types.NamespacedName{Name: "invalid-cross-namespace-backend-ref", Namespace: "gateway-conformance-infra"}
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: "gateway-conformance-infra"}
Expand Down
6 changes: 2 additions & 4 deletions conformance/tests/httproute-invalid-reference-grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ func init() {
var HTTPRouteInvalidReferenceGrant = suite.ConformanceTest{
ShortName: "HTTPRouteInvalidReferenceGrant",
Description: "A single HTTPRoute in the gateway-conformance-infra namespace should fail to attach to a Gateway in the same namespace if the route has a backendRef Service in the gateway-conformance-app-backend namespace and a ReferenceGrant exists but does not grant permission to route to that specific Service",
Features: []suite.SupportedFeature{
suite.SupportReferenceGrant,
},
Manifests: []string{"tests/httproute-invalid-reference-grant.yaml"},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/httproute-invalid-reference-grant.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
routeNN := types.NamespacedName{Name: "invalid-reference-grant", Namespace: "gateway-conformance-infra"}
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: "gateway-conformance-infra"}
Expand Down
6 changes: 2 additions & 4 deletions conformance/tests/httproute-reference-grant.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,8 @@ func init() {
var HTTPRouteReferenceGrant = suite.ConformanceTest{
ShortName: "HTTPRouteReferenceGrant",
Description: "A single HTTPRoute in the gateway-conformance-infra namespace, with a backendRef in the gateway-conformance-web-backend namespace, should attach to Gateway in the gateway-conformance-infra namespace",
Features: []suite.SupportedFeature{
suite.SupportReferenceGrant,
},
Manifests: []string{"tests/httproute-reference-grant.yaml"},
Exemptions: []suite.ExemptFeature{suite.ExemptReferenceGrant},
Manifests: []string{"tests/httproute-reference-grant.yaml"},
Test: func(t *testing.T, s *suite.ConformanceTestSuite) {
routeNN := types.NamespacedName{Name: "reference-grant", Namespace: "gateway-conformance-infra"}
gwNN := types.NamespacedName{Name: "same-namespace", Namespace: "gateway-conformance-infra"}
Expand Down
2 changes: 2 additions & 0 deletions conformance/utils/flags/flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,6 @@ var (
GatewayClassName = flag.String("gateway-class", "gateway-conformance", "Name of GatewayClass to use for tests")
ShowDebug = flag.Bool("debug", false, "Whether to print debug logs")
CleanupBaseResources = flag.Bool("cleanup-base-resources", true, "Whether to cleanup base test resources after the run")
SupportedFeatures = flag.String("supported-features", "", "Supported features included in conformance tests suites")
ExemptFeatures = flag.String("exempt-features", "", "Exempt Features excluded from conformance tests suites")
)
3 changes: 0 additions & 3 deletions conformance/utils/suite/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ const (
type SupportedFeature string

const (
// This option indicates support for the ReferenceGrant object.
SupportReferenceGrant SupportedFeature = "ReferenceGrant"

// This option indicates support for HTTPRoute query param matching (extended conformance).
SupportHTTPRouteQueryParamMatching SupportedFeature = "HTTPRouteQueryParamMatching"

Expand Down