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

bexxmodd · 2025-06-27T05:03:13Z

What type of PR is this?
/kind feature
/kind gep
/area conformance-test

What this PR does / why we need it:
This is an update from boolean flag to enum for the report that should capture case when Conformance profile is Mesh without GWC and we can't infer supported features.

Does this PR introduce a user-facing change?:
None

…t for exceptional case when only Mesh profile is being tested and no GWC available to determine supported features.

k8s-ci-robot · 2025-06-27T05:03:15Z

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

k8s-ci-robot · 2025-06-27T05:03:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bexxmodd
Once this PR has been reviewed and has the lgtm label, please assign arkodg for approval. For more information see the 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

k8s-ci-robot · 2025-06-27T05:03:23Z

Hi @bexxmodd. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

conformance/apis/v1/conformancereport.go

youngnick · 2025-06-30T03:51:55Z

Change overall looks good with some minor changes to make things more readable.

/ok-to-test

LiorLieberman · 2025-06-30T05:14:49Z

/cc

LiorLieberman

Thanks @bexxmodd overall looks good. left some comments

/hold

conformance/utils/suite/suite.go

…d or manually selected.

youngnick · 2025-07-01T05:03:37Z

This LGTM but I'll defer to @mlavacca for a final approval.

conformance/utils/suite/suite.go

mlavacca · 2025-07-01T09:45:10Z

conformance/apis/v1/conformancereport.go

 	// automatically detected by the conformance suite.
-	InferredSupportedFeatures bool `json:"inferredSupportedFeatures"`
+	SupportedFeaturesSource SupportedFeaturesSource `json:"supportedFeaturesSource"`


I have a broader question around this field (that here we are changing from bool to enum): what's the value of including this data in the report? From my point of view, this is something meaningful when running the suite, but I don't see much value in including it in the final report. Why is this information important to users who are looking at conformance reports?
Am I missing something?

In order for to verify that the list of features reported are actually supported by GatewayClass we want to run the tests based on the features the implementation claim to support (through publishing it on GatewayClass' Status). This field communicates to the users if reported features are actually supported (thus inferred), they were manually hand-picked or there is no GWC at all.

+1 to @bexxmodd point

And the plan is to follow up realtively quickly (we'll continue to communicate in slack and the community meetings), and block reports (by changing the csuite.Report function) to not report things that are manually stamped

To summarize, we can have the following cases:

Features manually picked (as it is today) - I guess this is the case @LiorLieberman is talking about when saying "block reports (by changing the csuite.Report function) to not report things that are manually stamped"

Inferred features - the suite gets them from the provided GWC status

undefined - only mesh features (I guess they are manually provided?)

However, there is a fourth case that I'd like to figure out, which is Gateway profiles and Mesh profiles together: I don't think mesh features should be set in the GWC status, so how do we proceed in this case?

However, there is a fourth case that I'd like to figure out, which is Gateway profiles and Mesh profiles together: I don't think mesh features should be set in the GWC status, so how do we proceed in this case?

Right now, implementations that are both a Gateway and a Mesh implementation, do have gateway class, and the current state is that they report (or will report) their supportedFeatures (mesh features included) on GWC. From here we can go to either direction:

Change the guidance to not report Mesh related features on GatewayClass. When a Mesh resource is introduced (if it is introduced cc: @kflynn) or when we figure out a different way for inferring mesh features, we are updating the GEP to guide implementations to report there.

We let this dual implementations (mesh and gateway) publish Mesh features on GWC. And later if there is additional Mesh resource introduced, they can migrate there.

I think 1) is cleaner. However it would require those implementations to provide Mesh Features manually, and more complex code in the suite.

I think 2) is more straightforward.

If we go with 1) the way to handle this is provide inferred stamp in case no non-mesh features were provided. (potentially we introduce a new flag for "mesh-features" to enforce it more cleanly, or we just check based on Mesh prefix)

/cc @mikemorris @kflynn @howardjohn for addtl feedback

My initial take is that 1️⃣ is preferable - dual GatewayClass/Mesh impls should not report mesh features on GatewayClass.

At best it would be inconsistent (mesh-only implementations would have a different reporting path), and at worst could lead to confusion like #3581, where N/S or mesh functionality might inaccurately imply conformance for the same feature on the other architecture (whether by coding error or just misinterpretation by users).

I would also vote for option 1. I think reporting mesh features on the GatewayClass is a bad idea.

(I also anticipate having a Mesh resource in Gateway API 1.4.0, but even if that misses, I think option 1 is the way to go.)

That said, I don't entirely follow @bexxmodd's comment here:

This field communicates to the users if reported features are actually supported (thus inferred), they were manually hand-picked or there is no GWC at all.

If the point is to figure out whether a given feature was actually tested vs just inserted by a human, I don't know that this gains us much, but if the point is different, I don't think I'm following the point. 🙂

Why do you think we don't gain much? Also, how much gain should be this change?

The fundamental purpose is to "verify that the list of features reported are actually supported by GatewayClass". When features are inferred from the GatewayClass status field, it creates an unbreakable, programmatic link between the implementation's public claims and the conformance tests being run. This is not just about whether a feature was tested, but whether a feature claimed by the implementation was tested and passed. A manually supplied list offers no such guarantee.

It terms of gain, it provides crucial context to anyone reading a conformance report.

This is step in that direction of creating the necessary distinction between trusted and untrusted feature sources.

k8s-ci-robot · 2025-07-02T16:37:47Z

@LiorLieberman: GitHub didn't allow me to request PR reviews from the following users: for.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

However, there is a fourth case that I'd like to figure out, which is Gateway profiles and Mesh profiles together: I don't think mesh features should be set in the GWC status, so how do we proceed in this case?

Right now, implementations that are both a Gateway and a Mesh implementation, do have gateway class, and the current state is that they report (or will report) their supportedFeatures (mesh features included) on GWC. From here we can go to either direction:

Change the guidance to not report Mesh related features on GatewayClass. When a Mesh resource is introduced (if it is introduced cc: @kflynn) or when we figure out a different way for inferring mesh features, we are updating the GEP to guide implementations to report there.

We let this dual implementations (mesh and gateway) publish Mesh features on GWC. And later if there is additional Mesh resource introduced, they can migrate there.

I think 1) is cleaner. However it would require those implementations to provide Mesh Features manually, and more complex code in the suite.

I think 2) is more straightforward.

If we go with 1) the way to handle this is provide inferred stamp in case no non-mesh features were provided. (potentially we introduce a new flag for "mesh-features" to enforce it more cleanly, or we just check based on Mesh prefix)

/cc @mikemorris @kflynn @howardjohn for addtl feedback

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-sigs/prow repository.

mikemorris · 2025-07-02T16:57:03Z

conformance/utils/suite/suite.go

+	case isOnlyMeshProfile(&options):
+		source = confv1.SupportedFeaturesSourceUndefined


I don't think I understand the intent of this logic.

Do you envision mesh sources to eventually fall under Inferred once a resource for reporting SupportedFeatures is available? Or if it's intended to remain distinct, should Inferred be named something more specific like GatewayClassInferred?

the former. It is currently undefined, and hence allowed. they would become Inferred once we have a resource or a way for reporting - yes

bexxmodd added 4 commits June 24, 2025 16:22

Update inferred supported features report from bool to enum to accoun…

af67628

…t for exceptional case when only Mesh profile is being tested and no GWC available to determine supported features.

Changed enum names.

79ef1ab

Added comment to report enum

8cb2054

Revert change caused by merge.

dceb6f6

k8s-ci-robot requested review from candita and LiorLieberman June 27, 2025 05:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 27, 2025

youngnick reviewed Jun 30, 2025

View reviewed changes

conformance/apis/v1/conformancereport.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 30, 2025

bexxmodd added 3 commits June 30, 2025 08:30

Converted enums from int to string for better readability.

9166ef2

Removed unused import.

ce0a071

Updated unit tests to reflect enum change to strings.

8906f69

LiorLieberman reviewed Jun 30, 2025

View reviewed changes

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

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

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

Removed skip tests from determining if supported features are inferre…

dc78f69

…d or manually selected.

youngnick requested a review from mlavacca July 1, 2025 05:03

youngnick added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 1, 2025

k8s-ci-robot removed the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 1, 2025

mlavacca added area/conformance-machinery Issues or PRs related to the machinery and the suite used to run conformance tests. and removed area/conformance-test Issues or PRs related to Conformance tests. labels Jul 1, 2025

mlavacca reviewed Jul 1, 2025

View reviewed changes

bexxmodd added 2 commits July 1, 2025 06:58

Removed source update.

d0e0045

Updated unit tests.

94e5b27

k8s-ci-robot requested review from mikemorris, kflynn and howardjohn July 2, 2025 16:37

mikemorris reviewed Jul 2, 2025

View reviewed changes

		case isOnlyMeshProfile(&options):
		source = confv1.SupportedFeaturesSourceUndefined

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

Are you sure you want to change the base?

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

Conversation

bexxmodd commented Jun 27, 2025

Uh oh!

k8s-ci-robot commented Jun 27, 2025

Uh oh!

k8s-ci-robot commented Jun 27, 2025

Uh oh!

k8s-ci-robot commented Jun 27, 2025

Uh oh!

Uh oh!

youngnick commented Jun 30, 2025

Uh oh!

LiorLieberman commented Jun 30, 2025

Uh oh!

LiorLieberman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

youngnick commented Jul 1, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiorLieberman Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikemorris Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LiorLieberman Jul 1, 2025 •

edited

Loading

mikemorris Jul 2, 2025 •

edited

Loading