Added logic to block conformance report if support features source is manual for non-gateway features. #3927
Conversation
… manual for non-gateway features.
k8s-ci-robot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: bexxmodd The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-ok-to-test
|
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 Once the patch is verified, the new status will be reflected by the 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. |
k8s-ci-robot
added
the
size/M
| if suite.supportedFeaturesSource == supportedFeaturesSourceManual && | ||
| !hasMeshFeatures(suite.SupportedFeatures) && | ||
| !suite.conformanceProfiles.HasAny(MeshHTTPConformanceProfileName, MeshGRPCConformanceProfileName) { | ||
| return nil, fmt.Errorf("can't generate report: Gateway's supported features should be read from Status and not supplied through flags") |
There was a problem hiding this comment.
This means its impossible to run the report if your gateway doesn't support the SupportFeatures feature. Which is a brand new feature. This seems far to early to do, if ever
There was a problem hiding this comment.
Agree that we should not block in v1.4. I think the corresponding GEP said that while this would be the default/encouraged path for v1.4+, it would only become required in v1.5. Of course mesh features will also have a longer extension here because there's currently no alternative way of listing supported features.
There was a problem hiding this comment.
Given that we decided to give grace period for Supported Features until v1.5 release, my intention with this PR was to merge it after Jul 22 so it falls under v1.5, when we'll start proactively blocking manually specified reports.
| if suite.supportedFeaturesSource == supportedFeaturesSourceManual && | ||
| !hasMeshFeatures(suite.SupportedFeatures) && | ||
| !suite.conformanceProfiles.HasAny(MeshHTTPConformanceProfileName, MeshGRPCConformanceProfileName) { | ||
| return nil, fmt.Errorf("can't generate report: Gateway's supported features should be read from Status and not supplied through flags") |
There was a problem hiding this comment.
Agree that we should not block in v1.4. I think the corresponding GEP said that while this would be the default/encouraged path for v1.4+, it would only become required in v1.5. Of course mesh features will also have a longer extension here because there's currently no alternative way of listing supported features.
| @@ -519,6 +519,12 @@ func (suite *ConformanceTestSuite) Report() (*confv1.ConformanceReport, error) { | |||
| } | |||
| defer suite.lock.RUnlock() | |||
|
|
|||
| if suite.supportedFeaturesSource == supportedFeaturesSourceManual && | |||
| !hasMeshFeatures(suite.SupportedFeatures) && | |||
There was a problem hiding this comment.
A bit orthogonal to this, but I think it would be useful to proactively ignore mesh features published in GatewayClass status so that it's clear that those should be reported separately and aren't really tied to the GatewayClass. Since it seems like a Mesh resource is ~imminent, it seems cleaner for Mesh features to be reported there in the future instead of both resources reporting all features.
Since I think this would exclusively affect Istio, would appreciate some feedback from that side on this idea:
There was a problem hiding this comment.
That makes sense to me, agreed clearing this up sooner sounds good.
There was a problem hiding this comment.
@robscott currently if Mesh features are read from GWC we throw an error
Are you implying to have some extra check here?
There was a problem hiding this comment.
That impl seems good to me (and easy to catch/fix as an implementer) @bexxmodd
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
What type of PR is this?
/area conformance-test
What this PR does / why we need it:
To guide conformance test users in a right direction, we want to discourage manually supplying supported features for conformance tests, instead those features should be inferred from GWC status. This change will block report generation if above mentioned logic is not followed.
Does this PR introduce a user-facing change?: