Skip to content

fix: use inferred supported features to set extendedSupportedFeatures #4113

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
k8s-ci-robot merged 3 commits into from
Sep 23, 2025
Merged

fix: use inferred supported features to set extendedSupportedFeatures #4113

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7eac26c
fix: use inferred supported features to set extendedSupportedFeatures
snorwin Sep 22, 2025
68dd9e7
extend unit tests with assertion for extendedSupportedFeatures
snorwin Sep 23, 2025
2e152a8
refactor NewConformanceTestSuite
snorwin Sep 23, 2025
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
51 changes: 18 additions & 33 deletions conformance/utils/suite/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,22 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,
return nil, fmt.Errorf("no supported features were determined for test suite")
}

extendedSupportedFeatures := make(map[ConformanceProfileName]FeaturesSet, 0)
extendedUnsupportedFeatures := make(map[ConformanceProfileName]FeaturesSet, 0)

for _, conformanceProfileName := range options.ConformanceProfiles.UnsortedList() {
conformanceProfile, err := getConformanceProfileForName(conformanceProfileName)
if err != nil {
return nil, fmt.Errorf("failed to retrieve conformance profile: %w", err)
}
// the use of a conformance profile implicitly enables any features of
// that profile which are supported at a Core level of support.
supportedFeatures = supportedFeatures.Union(conformanceProfile.CoreFeatures)

extendedSupportedFeatures[conformanceProfileName] = conformanceProfile.ExtendedFeatures.Intersection(supportedFeatures)
extendedUnsupportedFeatures[conformanceProfileName] = conformanceProfile.ExtendedFeatures.Difference(supportedFeatures)
}

config.SetupTimeoutConfig(&options.TimeoutConfig)

roundTripper := options.RoundTripper
Expand Down Expand Up @@ -290,8 +306,8 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,
UsableNetworkAddresses: options.UsableNetworkAddresses,
UnusableNetworkAddresses: options.UnusableNetworkAddresses,
results: make(map[string]testResult),
extendedUnsupportedFeatures: make(map[ConformanceProfileName]sets.Set[features.FeatureName]),
extendedSupportedFeatures: make(map[ConformanceProfileName]sets.Set[features.FeatureName]),
extendedUnsupportedFeatures: extendedUnsupportedFeatures,
extendedSupportedFeatures: extendedSupportedFeatures,
conformanceProfiles: options.ConformanceProfiles,
implementation: options.Implementation,
mode: mode,
Expand All @@ -301,37 +317,6 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,
Hook: options.Hook,
}

for _, conformanceProfileName := range options.ConformanceProfiles.UnsortedList() {
conformanceProfile, err := getConformanceProfileForName(conformanceProfileName)
if err != nil {
return nil, fmt.Errorf("failed to retrieve conformance profile: %w", err)
}
// the use of a conformance profile implicitly enables any features of
// that profile which are supported at a Core level of support.
for _, f := range conformanceProfile.CoreFeatures.UnsortedList() {
if !options.SupportedFeatures.Has(f) {
suite.SupportedFeatures.Insert(f)
}
}
for _, f := range conformanceProfile.ExtendedFeatures.UnsortedList() {
if options.SupportedFeatures.Has(f) {
Copy link
Copy Markdown
Contributor

@bexxmodd bexxmodd Sep 22, 2025

Choose a reason for hiding this comment

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

if !supportedFeatures.Has(f) {
    suite.SupportedFeatures.Insert(f)
}

It's a temporary variable until final supported features are determined for testing and NewConformanceTestSuite returns ConformanceTestSuite. Nothing limits it's scope until suite is initialized. There's no scope conflict or a misleading name so I don't see why it should go out of usage after suite is initialized. it's the sound representation of populated features from the Status field.

Copy link
Copy Markdown
Member Author

@snorwin snorwin Sep 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In my opinion, it doesn’t make sense to use supportedFeatures after it has already been assigned to suite.SupportedFeatures. Modifying it at that point would even be incorrect, why should we still rely on it?

In case additional code was inserted above this section that changed suite.SupportedFeatures, then the logic here is supposed to use the actual (possibly modified) features of the suite (suite.SupportedFeatures) rather than the originally derived ones in supportedFeatures.

If it is important that supportedFeatures is used in this part, I would suggest moving the entire section before the initialization of the ConformanceTestSuite object.

Copy link
Copy Markdown
Contributor

@bexxmodd bexxmodd Sep 23, 2025

Choose a reason for hiding this comment

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

It is actually a really good idea to move the whole section before initializing the ConformanceTestSuite.

Copy link
Copy Markdown
Member Author

@snorwin snorwin Sep 23, 2025

Choose a reason for hiding this comment

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

@bexxmodd could you take a close look at the refactoring? I simplified the logic quite a bit.

Copy link
Copy Markdown
Contributor

@bexxmodd bexxmodd Sep 23, 2025

Choose a reason for hiding this comment

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

This is way cleaner! Thanks Norwin!

if suite.extendedSupportedFeatures[conformanceProfileName] == nil {
suite.extendedSupportedFeatures[conformanceProfileName] = FeaturesSet{}
}
suite.extendedSupportedFeatures[conformanceProfileName].Insert(f)
} else {
if suite.extendedUnsupportedFeatures[conformanceProfileName] == nil {
suite.extendedUnsupportedFeatures[conformanceProfileName] = FeaturesSet{}
}
suite.extendedUnsupportedFeatures[conformanceProfileName].Insert(f)
}
// Add Exempt Features into unsupported features list
if options.ExemptFeatures.Has(f) {
suite.extendedUnsupportedFeatures[conformanceProfileName].Insert(f)
}
}
}

// apply defaults
if suite.BaseManifests == "" {
suite.BaseManifests = "base/manifests.yaml"
Expand Down
46 changes: 37 additions & 9 deletions conformance/utils/suite/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,20 +419,21 @@ var gwcStatusFeatureNames = []string{
"HTTPRouteHostRewrite",
"HTTPRouteMethodMatching",
"HTTPRoutePathRewrite",
"TTPRouteQueryParamMatching",
"HTTPRouteQueryParamMatching",
"HTTPRouteResponseHeaderModification",
"ReferenceGrant",
}

func TestInferGWCSupportedFeatures(t *testing.T) {
testCases := []struct {
name string
allowAllFeatures bool
supportedFeatures FeaturesSet
exemptFeatures FeaturesSet
ConformanceProfile sets.Set[ConformanceProfileName]
expectedFeatures FeaturesSet
expectedSource supportedFeaturesSource
name string
allowAllFeatures bool
supportedFeatures FeaturesSet
exemptFeatures FeaturesSet
ConformanceProfile sets.Set[ConformanceProfileName]
expectedFeatures FeaturesSet
expectedExtendedFeatures map[ConformanceProfileName]sets.Set[features.FeatureName]
expectedSource supportedFeaturesSource
}{
{
name: "properly infer supported features",
Expand All @@ -459,10 +460,32 @@ func TestInferGWCSupportedFeatures(t *testing.T) {
expectedSource: supportedFeaturesSourceManual,
},
{
name: "supports conformance profile - core",
name: "supports conformance profile core with specified extended features",
ConformanceProfile: sets.New(GatewayHTTPConformanceProfileName),
supportedFeatures: sets.New[features.FeatureName]("GatewayPort8080"),
expectedFeatures: sets.New[features.FeatureName]("Gateway", "HTTPRoute", "GatewayPort8080", "ReferenceGrant"),
expectedSource: supportedFeaturesSourceManual,
expectedExtendedFeatures: map[ConformanceProfileName]sets.Set[features.FeatureName]{
GatewayHTTPConformanceProfileName: namesToFeatureSet([]string{
"GatewayPort8080",
}),
},
},
{
name: "supports conformance profile core with inferred extended features",
ConformanceProfile: sets.New(GatewayHTTPConformanceProfileName),
expectedFeatures: namesToFeatureSet(gwcStatusFeatureNames),
expectedSource: supportedFeaturesSourceInferred,
expectedExtendedFeatures: map[ConformanceProfileName]sets.Set[features.FeatureName]{
GatewayHTTPConformanceProfileName: namesToFeatureSet([]string{
"GatewayPort8080",
"HTTPRouteHostRewrite",
"HTTPRouteMethodMatching",
"HTTPRoutePathRewrite",
"HTTPRouteQueryParamMatching",
"HTTPRouteResponseHeaderModification",
}),
},
},
}

Expand Down Expand Up @@ -521,6 +544,11 @@ func TestInferGWCSupportedFeatures(t *testing.T) {
if equal := cSuite.SupportedFeatures.Equal(tc.expectedFeatures); !equal {
t.Errorf("SupportedFeatures mismatch: got %v, want %v", cSuite.SupportedFeatures.UnsortedList(), tc.expectedFeatures.UnsortedList())
}

if tc.expectedExtendedFeatures == nil {
tc.expectedExtendedFeatures = make(map[ConformanceProfileName]sets.Set[features.FeatureName])
}
assert.Equal(t, tc.expectedExtendedFeatures, cSuite.extendedSupportedFeatures, "expectedExtendedFeatures mismatch")
})
}
}
Expand Down