Skip to content

[GEP-2162] Updated a new field on supported features inference from boolean to enum and remove from report. #3885

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 20 commits into from
Jul 16, 2025
Merged

[GEP-2162] Updated a new field on supported features inference from boolean to enum and remove from report. #3885

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
af67628
Update inferred supported features report from bool to enum to accoun…
bexxmodd Jun 24, 2025
79ef1ab
Changed enum names.
bexxmodd Jun 27, 2025
8cb2054
Added comment to report enum
bexxmodd Jun 27, 2025
dceb6f6
Revert change caused by merge.
bexxmodd Jun 27, 2025
9166ef2
Converted enums from int to string for better readability.
bexxmodd Jun 30, 2025
ce0a071
Removed unused import.
bexxmodd Jun 30, 2025
8906f69
Updated unit tests to reflect enum change to strings.
bexxmodd Jun 30, 2025
dc78f69
Removed skip tests from determining if supported features are inferre…
bexxmodd Jul 1, 2025
d0e0045
Removed source update.
bexxmodd Jul 1, 2025
94e5b27
Updated unit tests.
bexxmodd Jul 1, 2025
ce227ae
Removed source enum from report.
bexxmodd Jul 3, 2025
44e7a5c
start error string with lowercase.
bexxmodd Jul 3, 2025
9da830a
Removed source getter.
bexxmodd Jul 3, 2025
39b4bf1
removed undefined enum and added check for mesh features.
bexxmodd Jul 12, 2025
968565b
return error if mesh features are populated in gwc.
bexxmodd Jul 14, 2025
abf9d40
Updated unit tests.
bexxmodd Jul 14, 2025
28bddef
Removed check for mesh profile before throwing error for publishing m…
bexxmodd Jul 14, 2025
84b269a
removed extra else
bexxmodd Jul 14, 2025
0b7a15e
Removed not needed field from function.
bexxmodd Jul 14, 2025
61bc37a
Cleanup of extra field
bexxmodd Jul 14, 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
4 changes: 0 additions & 4 deletions conformance/apis/v1/conformancereport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,6 @@ type ConformanceReport struct {
// SucceededProvisionalTests is a list of the names of the provisional tests that
// have been successfully run.
SucceededProvisionalTests []string `json:"succeededProvisionalTests,omitempty"`

// InferredSupportedFeatures indicates whether the supported features were
// automatically detected by the conformance suite.
InferredSupportedFeatures bool `json:"inferredSupportedFeatures"`
}

// Implementation provides metadata information on the downstream
Expand Down
38 changes: 25 additions & 13 deletions conformance/utils/suite/suite.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ type ConformanceTestSuite struct {
// If SupportedFeatures are automatically determined from GWC Status.
// This will be required to report in future iterations as the passing
// will be determined based on this.
isInferredSupportedFeatures bool
supportedFeaturesSource supportedFeaturesSource

// mode is the operating mode of the implementation.
// The default value for it is "default".
Expand Down Expand Up @@ -188,24 +188,37 @@ const (
undefinedKeyword = "UNDEFINED"
)

// SupportedFeaturesSource represents the source from which supported features are derived.
// It is used to distinguish between them being inferred from GWC Status or manually
// supplied for the conformance report.
type supportedFeaturesSource string

const (
supportedFeaturesSourceUndefined supportedFeaturesSource = "Undefined"
supportedFeaturesSourceManual supportedFeaturesSource = "Manual"
supportedFeaturesSourceInferred supportedFeaturesSource = "Inferred"
)

// NewConformanceTestSuite is a helper to use for creating a new ConformanceTestSuite.
func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite, error) {
supportedFeatures := options.SupportedFeatures.Difference(options.ExemptFeatures)
isInferred := false
source := supportedFeaturesSourceManual
switch {
case options.EnableAllSupportedFeatures:
supportedFeatures = features.SetsToNamesSet(features.AllFeatures)
case shouldInferSupportedFeatures(&options):
var err error
supportedFeatures, err = fetchSupportedFeatures(options.Client, options.GatewayClassName)
if err != nil {
return nil, fmt.Errorf("Cannot infer supported features: %w", err)
return nil, fmt.Errorf("cannot infer supported features: %w", err)
}
isInferred = true
source = supportedFeaturesSourceInferred
case isOnlyMeshProfile(&options):
source = supportedFeaturesSourceUndefined
Copy link
Contributor

@mikemorris mikemorris Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm still not quite clear on if/why this case should be distinct from supportedFeaturesSourceManual now and if supportedFeaturesSourceUndefined is needed at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's Mesh profile, and has no GWC, there's no way for conformance to tell if features were inferred or not. Most likely this will get updated though in a follow up PR where isOnlyMeshProfile will be implemented based on previous discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my point is if it's not possible to infer features for mesh currently (if we're avoiding advertising on GatewayClass), doesn't that just imply supportedFeaturesSourceManual?

Copy link
Member

Choose a reason for hiding this comment

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

There are two aspects:

  1. Initial plan was to not limit (gw+mesh) implementations to not post it on GatewayClass. so undefined becomes better for mesh, since it was somehow "temporarily defined" in GWC. Ofc since we go with option 1 above, this argument is less relevant

  2. We wanted clear separation between implementations that choose to do manual and those who just have no choice.

That said, we could go an re-use manual here, until we come up with a solution for mesh, but it would be less clear to communicate the distinction for users. e.g. Mesh implementations would report "manual" while they have no other way to achieve that - this can look less compelling for users who browse the conformance results as they dont have the context we have here

I am fine with either

Copy link
Contributor Author

@bexxmodd bexxmodd Jul 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If there potential future work to have Mesh way of reporting supported features like GWC does, #2 makes more sense. Undefined will serve as a place holder, clearly separating if reporter chose to manually test features or service is not available (yet) for Mesh in this case.

Copy link
Member

Choose a reason for hiding this comment

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

To avoid confusion later, it seems like it may be simpler to ignore any Mesh features specified on a GatewayClass and always require Mesh features to be manually specified since we're already very likely going to end up with a Mesh resource thanks to @kflynn's work in #3894. That would also allow us to remove the concept of an undefined source here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, there's no conceptual difference for the final report if source has undefined or inferred. It doesn't appear on report and doesn't block report. It's only for internal use. If we convert this back to bool, should we treat mesh features as "inferred:true" ? that will be a lie. If we treat it "inferred:false" that it will block reports. So the only option is to have enum. Undefined serves as bridge between what is rn and what is expected to be there soon. If the issue is naming, we can change it to something like SourceIgnore or SourceSkipped because at the end that's what it's doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robscott per our offline discussion, removed Undefined field from enum and added check to determine if Mesh features are published under GWC.

}

// If features were not inferred from Status, it's a GWC issue.
if isInferred && supportedFeatures.Len() == 0 {
if source == supportedFeaturesSourceInferred && supportedFeatures.Len() == 0 {
return nil, fmt.Errorf("no supported features were determined for test suite")
}

Expand Down Expand Up @@ -273,7 +286,7 @@ func NewConformanceTestSuite(options ConformanceOptions) (*ConformanceTestSuite,
mode: mode,
apiVersion: apiVersion,
apiChannel: apiChannel,
isInferredSupportedFeatures: isInferred,
supportedFeaturesSource: source,
Hook: options.Hook,
}

Expand Down Expand Up @@ -394,10 +407,6 @@ func (suite *ConformanceTestSuite) Setup(t *testing.T, tests []ConformanceTest)
}
}

func (suite *ConformanceTestSuite) IsInferredSupportedFeatures() bool {
return suite.isInferredSupportedFeatures
}

func (suite *ConformanceTestSuite) setClientsetForTest(test ConformanceTest) error {
featureNames := []string{}
for _, v := range test.Features {
Expand Down Expand Up @@ -552,7 +561,6 @@ func (suite *ConformanceTestSuite) Report() (*confv1.ConformanceReport, error) {
GatewayAPIChannel: suite.apiChannel,
ProfileReports: profileReports.list(),
SucceededProvisionalTests: succeededProvisionalTests,
InferredSupportedFeatures: suite.IsInferredSupportedFeatures(),
}, nil
}

Expand Down Expand Up @@ -610,8 +618,6 @@ func shouldInferSupportedFeatures(opts *ConformanceOptions) bool {
return !opts.EnableAllSupportedFeatures &&
opts.SupportedFeatures.Len() == 0 &&
opts.ExemptFeatures.Len() == 0 &&
opts.ConformanceProfiles.Len() == 0 &&
len(opts.SkipTests) == 0 &&
opts.RunTest == ""
}

Expand Down Expand Up @@ -645,3 +651,9 @@ func getAPIVersionAndChannel(crds []apiextensionsv1.CustomResourceDefinition) (v

return version, channel, nil
}

func isOnlyMeshProfile(_ *ConformanceOptions) bool {
// TODO(bexxmodd): Currently a placeholder to add logic that determines if
// it's only Mesh profile without GWC.
return false
}
20 changes: 11 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 @@ -280,7 +280,6 @@ func TestSuiteReport(t *testing.T) {
coreProvisionalTest.ShortName,
extendedProvisionalTest.ShortName,
},
InferredSupportedFeatures: true,
},
},
{
Expand Down Expand Up @@ -390,7 +389,6 @@ func TestSuiteReport(t *testing.T) {
},
},
},
InferredSupportedFeatures: true,
},
},
}
Expand Down Expand Up @@ -434,33 +432,37 @@ func TestInferSupportedFeatures(t *testing.T) {
exemptFeatures FeaturesSet
ConformanceProfile sets.Set[ConformanceProfileName]
expectedFeatures FeaturesSet
expectedIsInferred bool
expectedSource supportedFeaturesSource
}{
{
name: "properly infer supported features",
expectedFeatures: namesToFeatureSet(statusFeatureNames),
expectedIsInferred: true,
name: "properly infer supported features",
expectedFeatures: namesToFeatureSet(statusFeatureNames),
expectedSource: supportedFeaturesSourceInferred,
},
{
name: "no features",
supportedFeatures: sets.New[features.FeatureName]("Gateway"),
expectedFeatures: sets.New[features.FeatureName]("Gateway"),
expectedSource: supportedFeaturesSourceManual,
},
{
name: "remove exempt features",
supportedFeatures: sets.New[features.FeatureName]("Gateway", "HTTPRoute"),
exemptFeatures: sets.New[features.FeatureName]("HTTPRoute"),
expectedFeatures: sets.New[features.FeatureName]("Gateway"),
expectedSource: supportedFeaturesSourceManual,
},
{
name: "allow all features",
allowAllFeatures: true,
expectedFeatures: features.SetsToNamesSet(features.AllFeatures),
expectedSource: supportedFeaturesSourceManual,
},
{
name: "supports conformance profile - core",
ConformanceProfile: sets.New(GatewayHTTPConformanceProfileName),
expectedFeatures: namesToFeatureSet([]string{"Gateway", "HTTPRoute", "ReferenceGrant"}),
expectedFeatures: namesToFeatureSet(statusFeatureNames),
expectedSource: supportedFeaturesSourceInferred,
},
}

Expand Down Expand Up @@ -512,8 +514,8 @@ func TestInferSupportedFeatures(t *testing.T) {
t.Fatalf("error initializing conformance suite: %v", err)
}

if cSuite.IsInferredSupportedFeatures() != tc.expectedIsInferred {
t.Errorf("InferredSupportedFeatures mismatch: got %v, want %v", cSuite.IsInferredSupportedFeatures(), tc.expectedIsInferred)
if cSuite.supportedFeaturesSource != tc.expectedSource {
t.Errorf("InferredSupportedFeatures mismatch: got %v, want %v", cSuite.supportedFeaturesSource, tc.expectedSource)
}

if equal := cSuite.SupportedFeatures.Equal(tc.expectedFeatures); !equal {
Expand Down