CM-869: Add test coverage for configurable TrustNamespace#391
CM-869: Add test coverage for configurable TrustNamespace#391openshift-merge-bot[bot] merged 14 commits intoopenshift:masterfrom
Conversation
Add missing unit test coverage for configurable trust namespace feature. The trust namespace configuration was already fully implemented in PR openshift#379. This commit adds comprehensive unit tests to verify the implementation and satisfy CM-869 acceptance criteria. Tests added: - Trust namespace validation when namespace doesn't exist - Custom trust namespace configuration with successful reconciliation - Status field updates for both default and custom namespace values Without these tests, the trust namespace validation logic was not adequately covered, making it harder to catch regressions. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add comprehensive end-to-end tests for trust namespace configuration. Tests verify: 1. Custom trust namespace RBAC placement - Role/RoleBinding created in trust namespace - Leader election Role/RoleBinding in operand namespace 2. Deployment configuration with correct --trust-namespace arg 3. Status field updates matching spec configuration 4. Degraded condition when trust namespace doesn't exist 5. Recovery to Ready=True after namespace creation These tests ensure the trust namespace feature works correctly in a real cluster environment and that all resources are created in the correct namespaces as required by CM-869. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@arun717: This pull request references CM-869 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-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds unit and e2e tests and builder helpers around TrustManager trust-namespace behavior, plus a one-line comment punctuation change; covers default/custom TrustNamespace status, degraded state when the namespace is missing, immutability checks, recovery after creating a missing namespace, and status-update logic tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 861-874: The test relies on an external reconcile trigger when the
missing trust Namespace is created, causing flakiness; fix by explicitly
triggering reconciliation instead of changing controller behavior: after
creating the missing Namespace in the test, perform an update/patch on the
TrustManager CR via trustManagerClient() (e.g., touch a metadata annotation or
increment a dummy field) to force the controller to requeue/reconcile, then
proceed to the existing Eventually assertions that inspect tm.Status conditions
(Degraded and Ready). Alternatively, if you prefer a controller change, add a
Namespace watch in the controller or change the missing-namespace error from
IRRECOVERABLE to RECOVERABLE so the controller requeues automatically (choose
one approach and implement consistently).
🪄 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: d7ef543c-ba42-4dae-9c4b-62f3c4c1e46c
📒 Files selected for processing (5)
pkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/test_utils.gopkg/controller/trustmanager/webhooks.gotest/e2e/trustmanager_test.gotest/e2e/utils_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/trustmanager/install_trustmanager_test.go (1)
119-130: Assert the object passed intoStatusUpdateto prevent false-positive testsRight now the mock ignores
obj, so the test can pass even ifStatusUpdatereceives stale/wrong data. Capture and assert theTrustManagerpayload passed toStatusUpdate(not just localtmstate).Suggested hardening
mock.StatusUpdateCalls(func(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { + gotTM, ok := obj.(*v1alpha1.TrustManager) + if !ok { + t.Fatalf("StatusUpdate called with unexpected type %T", obj) + } + gotTM = gotTM.DeepCopy() + _ = gotTM // store in outer var and assert via tt.assertStatus on updated payload return nil })As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/trustmanager/install_trustmanager_test.go` around lines 119 - 130, The mock currently ignores the object passed into StatusUpdate which can hide stale/wrong payloads; update the mock.StatusUpdateCalls handler in the test to capture the incoming obj parameter, assert it is a *v1alpha1.TrustManager (or cast accordingly), and compare its relevant fields to the expected tm (use deep-equal or cmp.Diff) and fail the test if they differ before returning nil; ensure the assertion checks the captured object rather than only relying on local tm and keep StatusUpdateCallCount checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controller/trustmanager/install_trustmanager_test.go`:
- Around line 119-130: The mock currently ignores the object passed into
StatusUpdate which can hide stale/wrong payloads; update the
mock.StatusUpdateCalls handler in the test to capture the incoming obj
parameter, assert it is a *v1alpha1.TrustManager (or cast accordingly), and
compare its relevant fields to the expected tm (use deep-equal or cmp.Diff) and
fail the test if they differ before returning nil; ensure the assertion checks
the captured object rather than only relying on local tm and keep
StatusUpdateCallCount checks intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d979f02-f02e-49d6-8bc0-2d4f564c332b
📒 Files selected for processing (2)
pkg/controller/trustmanager/controller_test.gopkg/controller/trustmanager/install_trustmanager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/trustmanager/controller_test.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 579-593: The tests create a fixed namespace name (customTrustNS)
and defer a delete without waiting, causing racey AlreadyExists errors; update
the test to generate a unique per-spec namespace (e.g., append a random or the
test's name/suffix) when creating the Namespace (replace usages of
customTrustNS) and change the deferred cleanup in the same scope to delete and
then poll until the namespace is actually removed (use the Kubernetes client to
Get the Namespace in a loop and treat NotFound as success) to ensure deletion is
complete before other specs reuse the name; apply the same change to the other
custom-namespace creation/cleanup blocks referenced in the file.
- Around line 598-602: The Eventually assertion indexes
deployment.Spec.Template.Spec.Containers[0] directly which can panic if the
container slice is empty; update the assertion in the Eventually block to first
assert the container slice has at least one element (e.g.
Expect(len(deployment.Spec.Template.Spec.Containers)).To(BeNumerically(">", 0)))
before accessing Containers[0], then check that Containers[0].Args contains the
expected "--trust-namespace=..." element using the existing
Expect/ContainElement pattern to avoid transient panics in reconcile states.
- Around line 694-701: The test accesses rb.Subjects[0] without asserting
Subjects is non-empty, which can panic; update the Eventually check that
retrieves the RoleBinding (via clientset.RbacV1().RoleBindings(...).Get) to
first assert that rb.Subjects is not nil/has length > 0 (e.g., Gomega
Expect(len(rb.Subjects)).Should(BeNumerically(">", 0)) or
Expect(rb.Subjects).ShouldNot(BeEmpty())) before accessing rb.Subjects[0], then
keep the existing assertions on rb.Subjects[0].Name and rb.Subjects[0].Namespace
and call verifyTrustManagerManagedLabels(rb.Labels).
🪄 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: 946f478a-7e82-45f1-9e44-70e5fbbcb96c
📒 Files selected for processing (1)
test/e2e/trustmanager_test.go
openshift-ci
bot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/trustmanager_test.go (1)
627-633: Assert that the default--trust-namespaceflag is gone too.This still passes if the deployment renders both
--trust-namespace=cert-managerand the custom value, so it doesn't fully verify the override.Suggested tightening
Eventually(func(g Gomega) { deployment, err := clientset.AppsV1().Deployments(trustManagerNamespace).Get(ctx, trustManagerDeploymentName, metav1.GetOptions{}) g.Expect(err).ShouldNot(HaveOccurred()) g.Expect(len(deployment.Spec.Template.Spec.Containers)).Should(BeNumerically(">", 0)) - g.Expect(deployment.Spec.Template.Spec.Containers[0].Args).Should(ContainElement(fmt.Sprintf("--trust-namespace=%s", customTrustNS))) + args := deployment.Spec.Template.Spec.Containers[0].Args + g.Expect(args).Should(ContainElement(fmt.Sprintf("--trust-namespace=%s", customTrustNS))) + g.Expect(args).ShouldNot(ContainElement("--trust-namespace=cert-manager")) }, lowTimeout, fastPollInterval).Should(Succeed())
🤖 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 696-721: Add assertions to ensure the trust-manager Role and
RoleBinding are absent from the operand namespace (cert-manager) after creating
the TrustManager with a custom trust namespace: after verifying the Role and
RoleBinding exist in customTrustNS (created via createTrustTestNamespace and
createTrustManager), call
clientset.RbacV1().Roles(certManagerNamespace).Get(...) for trustManagerRoleName
and expect a NotFound error, and similarly call
clientset.RbacV1().RoleBindings(certManagerNamespace).Get(...) for
trustManagerRoleBindingName and expect NotFound; keep verifying the subjects and
labels for the custom namespace checks (verifyTrustManagerManagedLabels,
trustManagerServiceAccountName, trustManagerNamespace) unchanged.
🪄 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: 5eb4dd53-a23d-49af-8014-164e904833f4
📒 Files selected for processing (2)
pkg/controller/trustmanager/install_trustmanager_test.gotest/e2e/trustmanager_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/trustmanager/install_trustmanager_test.go
openshift-ci
bot
added
needs-rebase
openshift-ci
bot
removed
the
needs-rebase
|
/retest |
|
It only extends test coverage. No new user-facing changes. /label docs-approved |
openshift-ci
bot
added
docs-approved
|
@arun717: This pull request references CM-869 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
openshift-ci
bot
removed
the
needs-rebase
|
/retest |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
@arun717: 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. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
There was a problem hiding this comment.
/lgtm
/label docs-approved
/label px-approved
Since changes are w.r.t tests, taking CI results as indicator adding the label.
/label qe-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: arun717, bharath-b-rh, chiragkyal 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
openshift-merge-bot
bot
merged commit 64ddff2
into
openshift:master
4 participants
This PR adds comprehensive test coverage for the configurable trust namespace feature.
The core functionality was already fully implemented in PR #379. This PR satisfies the remaining CM-869 acceptance criteria by adding the missing unit and e2e tests.
JIRA
What's Already Implemented (PR #379)
The trust namespace feature is fully functional:
spec.trustManagerConfig.trustNamespacewith default "cert-manager"--trust-namespaceargument configurationstatus.trustNamespaceupdatesWhat This PR Adds
Unit Tests
File:
pkg/controller/trustmanager/controller_test.goTestStatusTrustNamespaceUpdateFile:
pkg/controller/trustmanager/test_utils.goWithTrustNamespace()builder methodE2E Tests
File:
test/e2e/trustmanager_test.go--trust-namespaceargFile:
test/e2e/utils_test.goWithTrustNamespace()builder methodTest Results
All unit tests pass:
✅ TestProcessReconcileRequest: 5/5 subtests PASS
✅ TestStatusTrustNamespaceUpdate: 2/2 subtests PASS
✅ Build: SUCCESS
Acceptance Criteria
All CM-869 acceptance criteria are met:
--trust-namespacearg set correctly in deploymentCommits
Following conventional commits format:
test(trustmanager): add unit tests for trust namespace configurationtest(e2e): add e2e tests for trust namespace configurationTesting
Unit Tests