CM-871: Updates featuregate check to include OpenShift/MicroShift featureset#383
Open
bharath-b-rh wants to merge 2 commits intoopenshift:masterfrom
Open
CM-871: Updates featuregate check to include OpenShift/MicroShift featureset#383bharath-b-rh wants to merge 2 commits intoopenshift:masterfrom
bharath-b-rh wants to merge 2 commits intoopenshift:masterfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references CM-871 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughAdds OpenShift/MicroShift feature-gate detection and public APIs to configure and query feature gates; replaces Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/features/features.go`:
- Around line 191-194: The discovery call using
discoveryClient.ServerPreferredResources() treats any err as fatal and disables
TrustManager; instead, detect partial-discovery errors
(discovery.ErrGroupDiscoveryFailed) and only fail on genuine discovery failures.
Update the handling around resources, err returned by
ServerPreferredResources(): if err is non-nil and is not errors.Is(err,
discovery.ErrGroupDiscoveryFailed) then return the error, but if it is
ErrGroupDiscoveryFailed log or warn and continue using the returned resources to
decide TrustManager; import and use k8s.io/apimachinery/pkg/util/errors or
k8s.io/client-go/discovery to check ErrGroupDiscoveryFailed and ensure
TrustManager logic uses the available resources list rather than bailing out on
partial errors.
In `@test/e2e/trustmanager_test.go`:
- Around line 187-190: The comment above the Describe block "TrustManager with
Default feature set" incorrectly suggests using
E2E_GINKGO_LABEL_FILTER=Feature:TrustManager to exclude TechPreview; update the
comment and test labels so the Default suite does not share the same
Label("Feature:TrustManager") as the TechPreview suite: either change the
suggested filter to "E2E_GINKGO_LABEL_FILTER=Feature:TrustManager &&
!TechPreview" in the comment, or give this Describe a distinct label (e.g.,
Label("Feature:TrustManagerDefault")) and update the comment to recommend that
unique label; ensure you modify the Describe invocation that currently contains
Label("Feature:TrustManager") and the corresponding comment text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a1fcc4dd-f1be-44bc-874a-17e7c448e989
📒 Files selected for processing (5)
pkg/features/features.gopkg/features/features_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/trustmanager_test.go
Contributor
Author
|
/test e2e-operator-tech-preview |
|
@bharath-b-rh: This pull request references CM-871 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target either version "4.22." or "openshift-4.22.", but it targets "cert-manager-1.19" instead. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
pkg/features/features.go (1)
43-48: Consider using typed constant for "OKD" feature set.The
"OKD"string at line 47 is a magic string, unlike the other feature sets which use typed constants fromocpfeaturegate. IfOKDis not defined in the OpenShift API, consider defining a local constant for consistency and to prevent typos.♻️ Suggested improvement
+// FeatureSetOKD is the OKD feature set value (not defined in openshift/api). +const FeatureSetOKD ocpfeaturegate.FeatureSet = "OKD" + var allowedTrustManagerFeatureSets = sets.New[ocpfeaturegate.FeatureSet]( ocpfeaturegate.CustomNoUpgrade, ocpfeaturegate.DevPreviewNoUpgrade, ocpfeaturegate.TechPreviewNoUpgrade, - "OKD", + FeatureSetOKD, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/features/features.go` around lines 43 - 48, allowedTrustManagerFeatureSets currently contains a magic string "OKD" alongside typed constants of type ocpfeaturegate.FeatureSet; define a local typed constant (e.g., const OKDFeatureSet ocpfeaturegate.FeatureSet = "OKD") and replace the plain "OKD" entry in the allowedTrustManagerFeatureSets set with that constant to ensure type consistency and avoid typos when constructing the set in pkg/features/features.go (look for allowedTrustManagerFeatureSets and ocpfeaturegate.FeatureSet).test/e2e/trustmanager_test.go (1)
244-248: Consider using Consistently for absence verification.Using
Eventuallyto verify that a resource does NOT exist will pass immediately if the resource is absent. For stronger verification that the ServiceAccount remains absent over time (not just initially missing), consider usingConsistentlyinstead.♻️ Suggested improvement
By("verifying ServiceAccount is not created (TrustManager controller is not enabled for Default feature set)") - Eventually(func(g Gomega) { + Consistently(func(g Gomega) { _, err := clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx, trustManagerServiceAccountName, metav1.GetOptions{}) g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "ServiceAccount %s/%s must not exist when cluster feature set is Default", trustManagerNamespace, trustManagerServiceAccountName) }, lowTimeout, fastPollInterval).Should(Succeed())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_test.go` around lines 244 - 248, The test currently uses Eventually to assert absence of the ServiceAccount via clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(...), which can pass immediately if the resource is absent; replace this check with Consistently to ensure the ServiceAccount remains absent for the duration. Update the block that calls Eventually(func(g Gomega) { _, err := clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx, trustManagerServiceAccountName, metav1.GetOptions{}); g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), ...) }, lowTimeout, fastPollInterval).Should(Succeed()) to use Consistently with an appropriate duration and poll interval, keeping the same Get call and IsNotFound assertion and retaining trustManagerNamespace and trustManagerServiceAccountName.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/features/features_test.go`:
- Around line 232-235: The defer currently calls SetupWithFlagValue("") which is
a no-op and doesn't reset TrustManager; modify the test teardown to explicitly
reset the mutable feature gate (e.g. call
mutableFeatureGate.Set(featuregates.TrustManager, false) or reinitialize a fresh
feature gate via SetupWithFlagValue with an explicit "TrustManager=false" value)
so TrustManager is disabled after the test; locate calls to SetupWithFlagValue
and mutableFeatureGate.Set in the test and replace the defer with an explicit
reset to TrustManager=false (or recreate the gate) to avoid test pollution.
- Around line 216-226: The test currently expects isCRDInstalled to return an
error when discovery returns a partial result, but the implementation returns
(true, nil) if the partial resources include the target CRD; update the test so
the fakeDiscoveryWithPartialResult supplies an APIResourceList that includes the
target CRD name (openshiftFeatureGateCRDName) and assert ok == true and err ==
nil (instead of require.Error and assert.False). Ensure the call still uses
isCRDInstalled and openshiftFeatureGateCRDGroupVersion so the test reflects the
intended partial-result success path.
In `@pkg/features/features.go`:
- Around line 191-199: The behavior for handling partial discovery results
currently lets partial resources pass but the test expects an error; update
isCRDInstalled to match the test by treating any error from
discoveryClient.ServerPreferredResources() as fatal: if err != nil { return
false, fmt.Errorf("failed to discover resources: %w", err) } (remove the
special-case that only returns on empty resources) and update the outdated
comment to reflect that any discovery error causes an immediate return; ensure
the log.V(1).Info call is removed or adjusted accordingly since it will no
longer be reached in isCRDInstalled.
---
Nitpick comments:
In `@pkg/features/features.go`:
- Around line 43-48: allowedTrustManagerFeatureSets currently contains a magic
string "OKD" alongside typed constants of type ocpfeaturegate.FeatureSet; define
a local typed constant (e.g., const OKDFeatureSet ocpfeaturegate.FeatureSet =
"OKD") and replace the plain "OKD" entry in the allowedTrustManagerFeatureSets
set with that constant to ensure type consistency and avoid typos when
constructing the set in pkg/features/features.go (look for
allowedTrustManagerFeatureSets and ocpfeaturegate.FeatureSet).
In `@test/e2e/trustmanager_test.go`:
- Around line 244-248: The test currently uses Eventually to assert absence of
the ServiceAccount via
clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(...), which can
pass immediately if the resource is absent; replace this check with Consistently
to ensure the ServiceAccount remains absent for the duration. Update the block
that calls Eventually(func(g Gomega) { _, err :=
clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx,
trustManagerServiceAccountName, metav1.GetOptions{});
g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), ...) }, lowTimeout,
fastPollInterval).Should(Succeed()) to use Consistently with an appropriate
duration and poll interval, keeping the same Get call and IsNotFound assertion
and retaining trustManagerNamespace and trustManagerServiceAccountName.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c00f5dfa-d172-4bc4-b88f-80e8d4bf3c11
📒 Files selected for processing (5)
pkg/features/features.gopkg/features/features_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/trustmanager_test.go
Contributor
Author
|
Depends on openshift/release#76300 |
…tureset Signed-off-by: Bharath B <bhb@redhat.com>
Contributor
Author
|
/test ci/prow/fips-image-scan-cert-manager |
Contributor
Author
|
/test fips-image-scan-operator |
Contributor
Author
|
/test fips-image-scan-cert-manager |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/features/features_test.go (1)
151-160: Make the platform split explicit in these tests.Line 156 currently treats
OKDas universally allowed, and the MicroShift coverage never asserts thatfeatureSet: OKDstays disabled. That mirrors the implementation bug instead of catching it.Suggested coverage update
-func TestAllowedTrustManagerFeatureSets(t *testing.T) { +func TestAllowedTrustManagerFeatureSets(t *testing.T) { assert.True(t, allowedTrustManagerFeatureSets.Has(ocpfeaturegate.CustomNoUpgrade), "CustomNoUpgrade should be allowed") assert.True(t, allowedTrustManagerFeatureSets.Has(ocpfeaturegate.DevPreviewNoUpgrade), "DevPreviewNoUpgrade should be allowed") assert.True(t, allowedTrustManagerFeatureSets.Has(ocpfeaturegate.TechPreviewNoUpgrade), "TechPreviewNoUpgrade should be allowed") - assert.True(t, allowedTrustManagerFeatureSets.Has(FeatureSetOKD), "OKD should be allowed") + assert.True(t, allowedOpenShiftTrustManagerFeatureSets.Has(FeatureSetOKD), "OKD should be allowed on OpenShift") + assert.False(t, allowedMicroShiftTrustManagerFeatureSets.Has(FeatureSetOKD), "OKD should not be allowed on MicroShift")+ t.Run("returns false for OKD on MicroShift", func(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + require.NoError(t, os.WriteFile(configPath, []byte("apiServer:\n featureGates:\n featureSet: OKD\n"), 0644)) + microshiftConfigPath = configPath + assert.False(t, isMicroShiftFeatureSetEnabled()) + })Also applies to: 347-390
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/features/features_test.go` around lines 151 - 160, Test treats OKD as universally allowed; update TestAllowedTrustManagerFeatureSets to make the platform split explicit by adding platform-aware assertions: locate TestAllowedTrustManagerFeatureSets and the allowedTrustManagerFeatureSets variable and modify the test to assert FeatureSetOKD is allowed only for non-MicroShift platforms and explicitly assert FeatureSetOKD is not allowed for MicroShift (e.g., add a sub-test or conditional check that when platform == "microshift" the allowedTrustManagerFeatureSets.Has(FeatureSetOKD) returns false), and mirror this pattern for the other affected tests mentioned (lines ~347-390) so the tests fail if the implementation incorrectly enables OKD for MicroShift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/features/features.go`:
- Around line 42-50: The shared var allowedTrustManagerFeatureSets incorrectly
includes FeatureSetOKD (OpenShift-only) and is reused for MicroShift; remove
FeatureSetOKD from allowedTrustManagerFeatureSets and instead either (a) create
a separate allowedTrustManagerFeatureSetsForOpenShift that adds FeatureSetOKD
for the OpenShift path, or (b) keep allowedTrustManagerFeatureSets limited to
ocpfeaturegate.CustomNoUpgrade, DevPreviewNoUpgrade, TechPreviewNoUpgrade and
conditionally append FeatureSetOKD only when evaluating the OpenShift
feature-gate path; update any code that references
allowedTrustManagerFeatureSets for MicroShift/API server featureGates checks to
use the MicroShift-only set and ensure FeatureSetOKD remains referenced only by
the OpenShift-specific set or conditional.
In `@test/e2e/trustmanager_test.go`:
- Around line 49-58: The test enables the UNSUPPORTED_ADDON_FEATURES env var via
patchSubscriptionWithEnvVars and rollouts via waitForDeploymentEnvVarAndRollout
in the BeforeAll blocks but never restores the original value, leaking cluster
state; modify the suites to read and save the current UNSUPPORTED_ADDON_FEATURES
value before patching (e.g., via the same loader/subscription helper), then add
an AfterAll (or a deferred cleanup) that calls patchSubscriptionWithEnvVars to
restore the saved original value and waits for operator rollout again with
waitForDeploymentEnvVarAndRollout to ensure the operator returns to its prior
config; reference the existing functions patchSubscriptionWithEnvVars,
waitForDeploymentEnvVarAndRollout and the env var name
UNSUPPORTED_ADDON_FEATURES when implementing the save-and-restore cleanup.
---
Nitpick comments:
In `@pkg/features/features_test.go`:
- Around line 151-160: Test treats OKD as universally allowed; update
TestAllowedTrustManagerFeatureSets to make the platform split explicit by adding
platform-aware assertions: locate TestAllowedTrustManagerFeatureSets and the
allowedTrustManagerFeatureSets variable and modify the test to assert
FeatureSetOKD is allowed only for non-MicroShift platforms and explicitly assert
FeatureSetOKD is not allowed for MicroShift (e.g., add a sub-test or conditional
check that when platform == "microshift" the
allowedTrustManagerFeatureSets.Has(FeatureSetOKD) returns false), and mirror
this pattern for the other affected tests mentioned (lines ~347-390) so the
tests fail if the implementation incorrectly enables OKD for MicroShift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a56b97ae-d37c-474b-b7cd-42cff580bfac
📒 Files selected for processing (5)
pkg/features/features.gopkg/features/features_test.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/trustmanager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/operator/starter.go
Member
|
/assign |
Signed-off-by: Bharath B <bhb@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/trustmanager_test.go (1)
289-292: Consider a shorter duration forConsistentlyassertion.Using
lowTimeout(3 minutes) for aConsistentlycheck means the test waits 3 minutes to verify the ServiceAccount remains absent. A shorter duration (e.g., 30-60 seconds) would typically suffice to confirm the controller isn't creating the resource, reducing test execution time while still providing confidence.- Consistently(func(g Gomega) { - _, err := clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx, trustManagerServiceAccountName, metav1.GetOptions{}) - g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "ServiceAccount %s/%s must not exist when cluster feature set is Default", trustManagerNamespace, trustManagerServiceAccountName) - }, lowTimeout, fastPollInterval).Should(Succeed()) + Consistently(func(g Gomega) { + _, err := clientset.CoreV1().ServiceAccounts(trustManagerNamespace).Get(ctx, trustManagerServiceAccountName, metav1.GetOptions{}) + g.Expect(apierrors.IsNotFound(err)).To(BeTrue(), "ServiceAccount %s/%s must not exist when cluster feature set is Default", trustManagerNamespace, trustManagerServiceAccountName) + }, 30*time.Second, fastPollInterval).Should(Succeed())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_test.go` around lines 289 - 292, The Consistently check currently uses lowTimeout (3 minutes) which is longer than necessary; update the call to Consistently in trustmanager_test.go (the block invoking clientset.CoreV1().ServiceAccounts(...).Get(...) inside Consistently(func(g Gomega) { ... }, lowTimeout, fastPollInterval)) to use a shorter duration (e.g., 30s or 60s) instead of lowTimeout, keeping the same poll interval (fastPollInterval) and assertion body to reduce test runtime.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/trustmanager_test.go`:
- Around line 289-292: The Consistently check currently uses lowTimeout (3
minutes) which is longer than necessary; update the call to Consistently in
trustmanager_test.go (the block invoking
clientset.CoreV1().ServiceAccounts(...).Get(...) inside Consistently(func(g
Gomega) { ... }, lowTimeout, fastPollInterval)) to use a shorter duration (e.g.,
30s or 60s) instead of lowTimeout, keeping the same poll interval
(fastPollInterval) and assertion body to reduce test runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58c58aec-03c2-48de-bbb4-01f5a5e1e46c
📒 Files selected for processing (2)
test/e2e/trustmanager_test.gotest/e2e/utils_test.go
Contributor
Author
|
/test e2e-operator-tech-preview |
Contributor
|
@bharath-b-rh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Extends the TrustManager feature gate logic so it works on both OpenShift and MicroShift. The operator still requires an allowed cluster feature set and --unsupported-addon-features=TrustManager=true; the way the feature set is read is now platform-specific.
Changes
OpenShift
MicroShift