CM-873: Add comprehensive scenario-based e2e tests for the trust-manager#394
CM-873: Add comprehensive scenario-based e2e tests for the trust-manager#394chiragkyal wants to merge 3 commits intoopenshift:masterfrom
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (3)
WalkthroughAdded extensive e2e tests and helpers for TrustManager Bundle CRs, updated e2e suite to register APIs and create a controller-runtime client, removed an old TrustManager builder helper, and bumped/added module dependencies in go.mod files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chiragkyal The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
chiragkyal
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CM-873 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. |
openshift-ci
bot
added
the
needs-rebase
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
tm-bundle-e2e
branch
from
02852e9 to
4fe0ecb
Compare
openshift-ci
bot
removed
the
needs-rebase
chiragkyal
force-pushed
the
tm-bundle-e2e
branch
from
7ab35bc to
cb67964
Compare
chiragkyal
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
@chiragkyal: This pull request references CM-873 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. |
chiragkyal
force-pushed
the
tm-bundle-e2e
branch
from
cb67964 to
ef7e84c
Compare
|
/cc @bharath-b-rh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/trustmanager_helpers_test.go (1)
232-249: Split exact-match and contains-match target waiters.The single-source and source-update scenarios in
test/e2e/trustmanager_bundle_test.gouse these helpers too. With substring matching, those specs still pass if the controller appends new PEM data and leaves stale certificates behind.Also applies to: 254-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_helpers_test.go` around lines 232 - 249, The current waitForConfigMapTarget function mixes substring and exact matching which allows false positives; split it into two clear helpers (e.g., waitForConfigMapTargetContains and waitForConfigMapTargetExact) and update callers to use the appropriate one: keep the existing substring behavior in waitForConfigMapTargetContains (use strings.Contains on trimmed values) and make waitForConfigMapTargetExact require exact trimmed equality (compare strings.TrimSpace(data) == strings.TrimSpace(expectedContent)); change any usages in test/e2e/trustmanager_bundle_test.go and the other helper referenced (the similar function at the 254-271 range) to call the new exact or contains variant as appropriate so single-source/source-update tests use the exact matcher.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/trustmanager_bundle_test.go`:
- Around line 487-517: The test currently only waits for
trustapi.BundleConditionSynced == False, which is too generic; replace or add a
check to assert the specific SecretTargetsDisabled condition. Use
waitForBundleCondition(bundleClient, bundleSecretDisable,
trustapi.BundleConditionSecretTargetsDisabled, metav1.ConditionTrue,
highTimeout) (or similar call) to wait for the SecretTargetsDisabled condition
to become True after disabling secretTargets; keep using bundleClient,
bundleSecretDisable, metav1.ConditionTrue and highTimeout to locate and time the
check.
- Around line 80-90: The BeforeAll block that calls patchSubscriptionWithEnvVars
to set "UNSUPPORTED_ADDON_FEATURES":"TrustManager=true" mutates cluster state
but does not restore it; capture the original UNSUPPORTED_ADDON_FEATURES value
before calling patchSubscriptionWithEnvVars (e.g., read it via the same
subscription helper), store it in a package-scoped variable, and add a matching
AfterAll/AfterSuite teardown that calls patchSubscriptionWithEnvVars to restore
the original value (or unset it if it was empty) and then waits for rollout with
waitForDeploymentEnvVarAndRollout to ensure the operator returns to the original
env state; update trustmanager_bundle_test.go to implement this restore logic
around the existing BeforeAll/patchSubscriptionWithEnvVars and
waitForDeploymentEnvVarAndRollout calls.
In `@test/e2e/trustmanager_helpers_test.go`:
- Around line 303-311: The helper createBundleWithCleanup is incorrectly
treating apierrors.IsAlreadyExists as a successful creation, which can hide
stale objects; update the logic in createBundleWithCleanup so that
bundleClient.Create(ctx, bundle) does not swallow AlreadyExists errors—remove
the special-case that returns nil for apierrors.IsAlreadyExists and instead
return the error so Eventually will retry/fail; keep the
DeferCleanup(deleteBundle...) call as-is so the bundle is still removed if the
create eventually succeeds.
---
Nitpick comments:
In `@test/e2e/trustmanager_helpers_test.go`:
- Around line 232-249: The current waitForConfigMapTarget function mixes
substring and exact matching which allows false positives; split it into two
clear helpers (e.g., waitForConfigMapTargetContains and
waitForConfigMapTargetExact) and update callers to use the appropriate one: keep
the existing substring behavior in waitForConfigMapTargetContains (use
strings.Contains on trimmed values) and make waitForConfigMapTargetExact require
exact trimmed equality (compare strings.TrimSpace(data) ==
strings.TrimSpace(expectedContent)); change any usages in
test/e2e/trustmanager_bundle_test.go and the other helper referenced (the
similar function at the 254-271 range) to call the new exact or contains variant
as appropriate so single-source/source-update tests use the exact matcher.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1db54250-194f-4f03-add6-8c909c257034
⛔ Files ignored due to path filters (28)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sumtest/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sumvendor/github.com/cert-manager/trust-manager/LICENSEis excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/LICENSESis excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/conversion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/types_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/zz_generated.conversion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/v1alpha2/conversion.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/v1alpha2/doc.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/v1alpha2/register.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/v1alpha2/types_cluster_bundle.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/cert-manager/trust-manager/pkg/apis/trustmanager/v1alpha2/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/.travis.ymlis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/README.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/error_1_13.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/error_backward.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/join_unwrap_1_20.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/go-errors/errors/join_unwrap_backward.gois excluded by!vendor/**,!**/vendor/**vendor/modules.txtis excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/schema/elements.gois excluded by!vendor/**,!**/vendor/**vendor/sigs.k8s.io/structured-merge-diff/v6/typed/remove.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (8)
go.modtest/e2e/suite_test.gotest/e2e/trustmanager_bundle_test.gotest/e2e/trustmanager_helpers_test.gotest/e2e/trustmanager_test.gotest/e2e/utils_test.gotest/go.modtools/go.mod
| It("should report SecretTargetsDisabled on existing synced Bundle after disabling secretTargets", func() { | ||
| bundle := newBundle(bundleSecretDisable). | ||
| WithInLineSource(testCertPEM1). | ||
| WithSecretTarget(bundleTargetKey). | ||
| Build() | ||
|
|
||
| createBundleWithCleanup(ctx, bundle) | ||
|
|
||
| By("verifying Secret target syncs while feature is enabled") | ||
| err := waitForSecretTarget(ctx, bundleClient, bundleSecretDisable, testNS.Name, bundleTargetKey, testCertPEM1, highTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) | ||
| verifyBundleSynced(ctx, bundleSecretDisable) | ||
|
|
||
| By("disabling secretTargets on TrustManager CR") | ||
| Eventually(func() error { | ||
| tm, err := trustManagerClient().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| tm.Spec.TrustManagerConfig.SecretTargets = v1alpha1.SecretTargetsConfig{ | ||
| Policy: v1alpha1.SecretTargetsPolicyDisabled, | ||
| } | ||
| _, err = trustManagerClient().Update(ctx, tm, metav1.UpdateOptions{}) | ||
| return err | ||
| }, lowTimeout, fastPollInterval).Should(Succeed()) | ||
|
|
||
| waitForTrustManagerReady(ctx) | ||
|
|
||
| By("verifying Bundle status transitions to Synced=False") | ||
| err = waitForBundleCondition(ctx, bundleClient, bundleSecretDisable, trustapi.BundleConditionSynced, metav1.ConditionFalse, highTimeout) | ||
| Expect(err).ShouldNot(HaveOccurred()) |
There was a problem hiding this comment.
Assert SecretTargetsDisabled, not just Synced=False.
This spec name promises a specific disabled-policy result, but the assertion only waits for a false Synced condition. Any unrelated reconciliation failure would satisfy the test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/e2e/trustmanager_bundle_test.go` around lines 487 - 517, The test
currently only waits for trustapi.BundleConditionSynced == False, which is too
generic; replace or add a check to assert the specific SecretTargetsDisabled
condition. Use waitForBundleCondition(bundleClient, bundleSecretDisable,
trustapi.BundleConditionSecretTargetsDisabled, metav1.ConditionTrue,
highTimeout) (or similar call) to wait for the SecretTargetsDisabled condition
to become True after disabling secretTargets; keep using bundleClient,
bundleSecretDisable, metav1.ConditionTrue and highTimeout to locate and time the
check.
|
/test e2e-operator-tech-preview |
chiragkyal
force-pushed
the
tm-bundle-e2e
branch
from
34e8d32 to
24857d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/trustmanager_test.go (1)
975-980: Strengthen propagation assertions to avoid false positives.Using only
ShouldNot(Equal(original...))can pass on unrelated background drift. Prefer asserting the injected bundle/package includes the test CA you added (or its fingerprint), not just “different from baseline.”As per coding guidelines “Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.”
Also applies to: 985-990
🤖 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 975 - 980, The current test only checks the injected CA bundle differs from originalInjectedData which can yield false positives; instead, within the Eventually block that fetches the ConfigMap via clientset.CoreV1().ConfigMaps(operatorNamespace).Get(...), assert that cm.Data[trustedCABundleKey] specifically contains the test CA you added (or a stable identifier like its fingerprint) — use the test fixture variable (e.g., the test CA PEM or its fingerprint) rather than inequality; apply the same change to the similar assertion around lines 985-990 so both checks verify presence of the expected CA content instead of mere difference.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/e2e/trustmanager_test.go`:
- Around line 828-1000: This test (It("should propagate CNO CA bundle update to
package ConfigMap and restart pod")) mutates cluster-global state
(configClient.Proxies() updates to Proxy/cluster and the openshift-config
user-ca-bundle ConfigMap) and must not run in parallel; mark the spec as
serial/isolated by adding the appropriate Ginkgo serial label or moving it into
a dedicated Serial/SerialDescribe block so it runs alone (e.g., apply
Label("Serial") or wrap in a SerialDescribe) to prevent cross-test flakiness
when touching Proxy/cluster and openshift-config/user-ca-bundle and keep the
DeferCleanup logic as-is.
---
Nitpick comments:
In `@test/e2e/trustmanager_test.go`:
- Around line 975-980: The current test only checks the injected CA bundle
differs from originalInjectedData which can yield false positives; instead,
within the Eventually block that fetches the ConfigMap via
clientset.CoreV1().ConfigMaps(operatorNamespace).Get(...), assert that
cm.Data[trustedCABundleKey] specifically contains the test CA you added (or a
stable identifier like its fingerprint) — use the test fixture variable (e.g.,
the test CA PEM or its fingerprint) rather than inequality; apply the same
change to the similar assertion around lines 985-990 so both checks verify
presence of the expected CA content instead of mere difference.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d50dffbe-8271-46fa-ae6c-202aad05ce38
📒 Files selected for processing (1)
test/e2e/trustmanager_test.go
|
/test e2e-operator-tech-preview |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
test/e2e/trustmanager_bundle_test.go (2)
515-517:⚠️ Potential issue | 🟡 MinorTest assertion does not verify
SecretTargetsDisabledcondition.The spec name promises verification of
SecretTargetsDisabled, but line 516 only waits forSynced=False. Any unrelated failure would satisfy this assertion. Consider additionally waiting fortrustapi.BundleConditionSecretTargetsDisabledto beTruefor a precise assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_bundle_test.go` around lines 515 - 517, The test currently only asserts Synced=False using waitForBundleCondition(bundleClient, bundleSecretDisable, trustapi.BundleConditionSynced, metav1.ConditionFalse, highTimeout); add an additional wait that asserts the BundleConditionSecretTargetsDisabled condition is True to precisely verify secret targets were disabled—call waitForBundleCondition with bundleClient, bundleSecretDisable, trustapi.BundleConditionSecretTargetsDisabled, metav1.ConditionTrue and the same timeout (highTimeout) and ensure the Expect(err).ShouldNot(HaveOccurred()) check follows that call as well.
80-90:⚠️ Potential issue | 🟠 MajorMissing teardown to restore
UNSUPPORTED_ADDON_FEATURES.The
BeforeAllmutates the subscription withTrustManager=truebut there's no correspondingAfterAllat the suite level to restore the original value. This can leave the cluster in a modified state affecting subsequent test suites or manual testing.Consider capturing the original value before patching and restoring it in an
AfterAllblock.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/trustmanager_bundle_test.go` around lines 80 - 90, Before mutating the subscription in BeforeAll (where patchSubscriptionWithEnvVars and waitForDeploymentEnvVarAndRollout are used to set "UNSUPPORTED_ADDON_FEATURES=TrustManager=true"), capture the current value of "UNSUPPORTED_ADDON_FEATURES" first, then add an AfterAll block that restores that captured original value by calling patchSubscriptionWithEnvVars (or a corresponding restore helper) to reset the environment variable; ensure the restore runs unconditionally (even on failures) and references the same subscription/loader used in BeforeAll so the cluster state is returned to its original value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/e2e/trustmanager_bundle_test.go`:
- Around line 515-517: The test currently only asserts Synced=False using
waitForBundleCondition(bundleClient, bundleSecretDisable,
trustapi.BundleConditionSynced, metav1.ConditionFalse, highTimeout); add an
additional wait that asserts the BundleConditionSecretTargetsDisabled condition
is True to precisely verify secret targets were disabled—call
waitForBundleCondition with bundleClient, bundleSecretDisable,
trustapi.BundleConditionSecretTargetsDisabled, metav1.ConditionTrue and the same
timeout (highTimeout) and ensure the Expect(err).ShouldNot(HaveOccurred()) check
follows that call as well.
- Around line 80-90: Before mutating the subscription in BeforeAll (where
patchSubscriptionWithEnvVars and waitForDeploymentEnvVarAndRollout are used to
set "UNSUPPORTED_ADDON_FEATURES=TrustManager=true"), capture the current value
of "UNSUPPORTED_ADDON_FEATURES" first, then add an AfterAll block that restores
that captured original value by calling patchSubscriptionWithEnvVars (or a
corresponding restore helper) to reset the environment variable; ensure the
restore runs unconditionally (even on failures) and references the same
subscription/loader used in BeforeAll so the cluster state is returned to its
original value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 10d99ed0-bcdb-487e-a3a3-0b5c91ca48a0
📒 Files selected for processing (1)
test/e2e/trustmanager_bundle_test.go
|
/test e2e-operator-tech-preview |
Signed-off-by: chiragkyal <ckyal@redhat.com>
Signed-off-by: chiragkyal <ckyal@redhat.com>
chiragkyal
force-pushed
the
tm-bundle-e2e
branch
from
b931304 to
dcea7cc
Compare
|
@chiragkyal: all tests passed! 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. |
|
PR needs rebase. 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. |
openshift-ci
bot
added
the
needs-rebase
2 participants
Summary
Adds comprehensive scenario-based e2e tests for the trust-manager Bundle CR, covering the full lifecycle from Bundle creation through target sync verification under various TrustManager configurations.
Test Coverage
Group 1 — Default TrustManager (no optional features):
Group 2 — SecretTargets enabled:
Group 3 — DefaultCAPackage enabled:
Group 4 — SecretTargets + DefaultCAPackage enabled:
Group 5 — Custom TrustNamespace:
Group 6 — FilterExpiredCertificates enabled: