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

[bexxmodd]

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

[k8s-ci-robot]

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.

[youngnick]

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

/ok-to-test

[LiorLieberman]

/cc

[LiorLieberman]

Thanks @bexxmodd overall looks good. left some comments

/hold

[youngnick]

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

[mlavacca]

Thanks, @bexxmodd!

Deferring to someone else the lgtm

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bexxmodd, mlavacca

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

The pull request process is described here

Needs approval from an approver in each of these files:

• conformance/OWNERS [mlavacca]

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

[mikemorris]

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

[bexxmodd]

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.

[mikemorris]

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?

[LiorLieberman]

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

[bexxmodd]

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.

[robscott]

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.

[bexxmodd]

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.

[bexxmodd]

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

[LiorLieberman]

/cc @mikemorris @kflynn @mlavacca for final review and feedback on #3885 (comment)

[robscott]

Thanks @bexxmodd!

/lgtm
/hold cancel