ESO-447: Expand Vault e2e for TLS trustedCABundle and ExternalSecret templating#169
ESO-447: Expand Vault e2e for TLS trustedCABundle and ExternalSecret templating#169bharath-b-rh wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-447 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 the "5.0.0" version, but no target version was set. 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. |
WalkthroughThe E2E suite now installs cert-manager through OLM, provisions TLS-enabled Vault resources, tests trusted CA bundle recovery and reconciliation, validates merged Kubernetes/Vault registry credentials, and adds supporting polling, deletion, documentation, and fixtures. ChangesVault E2E coverage
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETests
participant CertManager
participant Vault
participant ExternalSecretsOperator
participant Kubernetes
E2ETests->>CertManager: provision Vault CA and TLS certificates
E2ETests->>Vault: deploy and initialize TLS-enabled Vault
E2ETests->>ExternalSecretsOperator: set trustedCABundle and apply SecretStores
ExternalSecretsOperator->>Vault: connect using the trusted CA
ExternalSecretsOperator->>Kubernetes: reconcile ExternalSecrets and PushSecrets
E2ETests->>Kubernetes: verify readiness and merged secret data
🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (1 warning, 10 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
…templating Signed-off-by: Bharath B <bhb@redhat.com>
|
[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: 4
🧹 Nitpick comments (3)
test/e2e/cert_manager_helpers_test.go (1)
109-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
k8serrors.IsNotFoundover string matching.
strings.Contains(err.Error(), "not found")is fragile compared to the structuredk8serrors.IsNotFound(err)check used elsewhere in this PR (e.g.ensureVaultNamespace). Text matching can silently misclassify errors if the message format changes.♻️ Proposed fix
pods, err := clientset.CoreV1().Pods(certManagerOperandNamespace).List(ctx, metav1.ListOptions{}) if err != nil { - if strings.Contains(err.Error(), "not found") { + if k8serrors.IsNotFound(err) { return false, nil } return false, err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/cert_manager_helpers_test.go` around lines 109 - 141, Update waitForCertManagerOperandPods to use the Kubernetes structured not-found check k8serrors.IsNotFound(err) instead of matching "not found" in err.Error(), while preserving the existing retry behavior for not-found errors and propagation of all other errors.test/e2e/e2e_test.go (1)
2759-2816: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winWrap ConfigMap update with
retry.RetryOnConflict.
createVaultCAConfigMapandcreateSampleCAConfigMapdo a plain Get→mutate.Data→Update without conflict retry. The referenced ConfigMap (vaultCAConfigMapName) is the same object the operator's watch-label reconciler patches once it's referenced bytrustedCABundle(pertrusted_ca_bundle_test.go's watch-label restoration test), so a concurrent operator patch between our Get and Update can produce a conflict error that isn't retried, unlike the establishedretry.RetryOnConflictpattern used insetTrustedCABundle/clearTrustedCABundlein this same file.♻️ Proposed fix (apply the same pattern to both functions)
- existing, err := clientset.CoreV1().ConfigMaps(operandNamespace).Get(ctx, vaultCAConfigMapName, metav1.GetOptions{}) - if k8serrors.IsNotFound(err) { - _, err = clientset.CoreV1().ConfigMaps(operandNamespace).Create(ctx, cm, metav1.CreateOptions{}) - return err - } - if err != nil { - return err - } - existing.Data = cm.Data - _, err = clientset.CoreV1().ConfigMaps(operandNamespace).Update(ctx, existing, metav1.UpdateOptions{}) - return err + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + existing, err := clientset.CoreV1().ConfigMaps(operandNamespace).Get(ctx, vaultCAConfigMapName, metav1.GetOptions{}) + if k8serrors.IsNotFound(err) { + _, err = clientset.CoreV1().ConfigMaps(operandNamespace).Create(ctx, cm, metav1.CreateOptions{}) + return err + } + if err != nil { + return err + } + existing.Data = cm.Data + _, err = clientset.CoreV1().ConfigMaps(operandNamespace).Update(ctx, existing, metav1.UpdateOptions{}) + return err + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/e2e_test.go` around lines 2759 - 2816, Wrap the existing ConfigMap Get, Data mutation, and Update flow in both createVaultCAConfigMap and createSampleCAConfigMap with retry.RetryOnConflict, re-fetching the ConfigMap on each retry and returning non-conflict errors immediately. Preserve the existing create-on-NotFound behavior and ensure the retry callback propagates the final update error.test/e2e/trusted_ca_bundle_test.go (1)
236-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd failure messages to newly added Gomega assertions. Both sites add new
Expect/g.Expectassertions without a descriptive failure message, unlike neighboring assertions in the same PR (e.g. the restoration check a few lines below in the same test) that do include one — this is explicitly called out for**/*_test.gofiles.
test/e2e/trusted_ca_bundle_test.go#L236-L238: add a message tog.Expect(cm.Labels).To(HaveKeyWithValue(...)), e.g."ConfigMap %s should have the watch label after ExternalSecretsConfig is Ready".test/e2e/e2e_test.go#L2012-L2014: add messages toExpect(createVaultCAConfigMap(...)).To(Succeed())andExpect(createSampleCAConfigMap(...)).To(Succeed()), e.g."failed to create Vault CA ConfigMap"/"failed to create sample CA ConfigMap".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/trusted_ca_bundle_test.go` around lines 236 - 238, Add descriptive failure messages to all newly added Gomega assertions: update the ConfigMap label assertion near the ExternalSecretsConfig readiness check in test/e2e/trusted_ca_bundle_test.go (lines 236-238), and add distinct creation-failure messages to the createVaultCAConfigMap and createSampleCAConfigMap assertions in test/e2e/e2e_test.go (lines 2012-2014).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/e2e/e2e_test.go`:
- Around line 164-167: Remove the suite-level ensureCertManagerOperatorReady
call from the top-level Ordered Describe BeforeAll, and invoke it in the “Vault
Secret Manager” Context’s own BeforeAll near its existing setup. Keep
cert-manager installation scoped to Vault tests, and add the appropriate
MicroShift skip or guard alongside the Context’s existing Skipped:Disconnected
handling.
In `@test/e2e/README.md`:
- Line 155: Move the “Custom Network Policy Naming” paragraph from beneath
trusted_ca_bundle_test.go to the section containing the NetworkPolicy entry for
e2e_test.go. Preserve the paragraph text unchanged and keep it associated with
the NetworkPolicy spec documentation.
- Around line 87-91: The executable default e2e filter must match the documented
exclusion of Vault. Update the default filter in the Makefile target governing
make test-e2e to exclude both Bitwarden and Vault, preserving the existing
behavior for other suites and avoiding changes to the README unless Vault is
intentionally meant to run by default.
In `@test/e2e/testdata/vault/vault.yaml`:
- Around line 51-52: Update the Deployment pod security configuration in
vault.yaml to explicitly set runAsNonRoot and readOnlyRootFilesystem, retain
capability dropping and allowPrivilegeEscalation: false, and disable
service-account token automount unless required. Ensure the /vault/data volume
remains writable while the container root filesystem is read-only.
---
Nitpick comments:
In `@test/e2e/cert_manager_helpers_test.go`:
- Around line 109-141: Update waitForCertManagerOperandPods to use the
Kubernetes structured not-found check k8serrors.IsNotFound(err) instead of
matching "not found" in err.Error(), while preserving the existing retry
behavior for not-found errors and propagation of all other errors.
In `@test/e2e/e2e_test.go`:
- Around line 2759-2816: Wrap the existing ConfigMap Get, Data mutation, and
Update flow in both createVaultCAConfigMap and createSampleCAConfigMap with
retry.RetryOnConflict, re-fetching the ConfigMap on each retry and returning
non-conflict errors immediately. Preserve the existing create-on-NotFound
behavior and ensure the retry callback propagates the final update error.
In `@test/e2e/trusted_ca_bundle_test.go`:
- Around line 236-238: Add descriptive failure messages to all newly added
Gomega assertions: update the ConfigMap label assertion near the
ExternalSecretsConfig readiness check in test/e2e/trusted_ca_bundle_test.go
(lines 236-238), and add distinct creation-failure messages to the
createVaultCAConfigMap and createSampleCAConfigMap assertions in
test/e2e/e2e_test.go (lines 2012-2014).
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 30b54f36-5d8e-478d-9d6f-0961fa177082
📒 Files selected for processing (20)
Makefiletest/e2e/README.mdtest/e2e/cert_manager_helpers_test.gotest/e2e/e2e_test.gotest/e2e/testdata/cert-manager/operator.yamltest/e2e/testdata/vault/ca_certificate.yamltest/e2e/testdata/vault/ca_issuer.yamltest/e2e/testdata/vault/certificate.yamltest/e2e/testdata/vault/external_secret.yamltest/e2e/testdata/vault/issuer.yamltest/e2e/testdata/vault/push_secret.yamltest/e2e/testdata/vault/push_source_secret.yamltest/e2e/testdata/vault/templating_external_secret.yamltest/e2e/testdata/vault/templating_k8s_backend.yamltest/e2e/testdata/vault/templating_push_secret.yamltest/e2e/testdata/vault/templating_source_secrets.yamltest/e2e/testdata/vault/vault.yamltest/e2e/trusted_ca_bundle_test.gotest/utils/conditions.gotest/utils/dynamic_resources.go
| By("Ensuring Red Hat cert-manager Operator is installed and ready") | ||
| Expect(ensureCertManagerOperatorReady(ctx, clientset, dynamicClient)).To(Succeed(), | ||
| "Red Hat cert-manager Operator is required for e2e (OLM install from redhat-operators); see test/e2e/README.md") | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Scope cert-manager operator install to the Vault context, not the whole suite.
ensureCertManagerOperatorReady runs in the top-level suite BeforeAll, which executes before every spec in this Ordered Describe block (AWS, env-var, annotations, network-policy, proxy tests, etc.), not just the Vault tests that actually need it. Since the PR explicitly excludes Provider:Vault from the default filter, requiring OLM/cert-manager install for non-Vault runs undermines that goal — and because this is the suite-wide BeforeAll, a failure here (e.g. missing redhat-operators catalog source, disconnected cluster, or MicroShift without OLM/CSV support) aborts the entire suite, not just Vault specs.
Move the ensureCertManagerOperatorReady call into the "Vault Secret Manager" Context's own BeforeAll (near line 1906), and consider adding a MicroShift skip (e.g. Skipped:MicroShift label or exutil.IsMicroShiftCluster() guard) alongside the existing Skipped:Disconnected label on that Context, since installing cert-manager via OLM Subscription/OperatorGroup is not supported on MicroShift.
Also applies to: 1888-1888
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/e2e_test.go` around lines 164 - 167, Remove the suite-level
ensureCertManagerOperatorReady call from the top-level Ordered Describe
BeforeAll, and invoke it in the “Vault Secret Manager” Context’s own BeforeAll
near its existing setup. Keep cert-manager installation scoped to Vault tests,
and add the appropriate MicroShift skip or guard alongside the Context’s
existing Skipped:Disconnected handling.
Source: Coding guidelines
| | `Provider:Vault` | Vault HTTPS + trustedCABundle failure/recovery (uses suite-installed Red Hat cert-manager Operator) | | ||
| | `API:Bitwarden` | bitwarden-sdk-server HTTP API (deploys plugin + `bitwarden-tls-certs` automatically) | | ||
| | `API:Bitwarden \|\| Provider:Bitwarden` | All Bitwarden HTTP and provider tests (requires `bitwarden-creds` for Secrets API / provider sync) | | ||
| | `Feature:TrustedCABundle` | Trusted CA bundle suite | | ||
| | `Feature:TrustedCABundle` | Trusted CA bundle suite (mount/validation + Vault TLS path when combined with `Provider:Vault`) | | ||
| | `Feature:ExternalSecretsTemplating` | ExternalSecret templating merge (Kubernetes + Vault dockerconfig) | |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the executable default filter aligned with the Vault documentation.
The README and PR objective say Provider:Vault is excluded by default, but Makefile Line 217 excludes only Bitwarden. Vault specs that satisfy the platform predicate can therefore run during make test-e2e, unexpectedly requiring cert-manager/OLM prerequisites.
Update the Makefile filter to exclude {Bitwarden, Vault}, or correct this documentation if Vault is intended to run by default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/README.md` around lines 87 - 91, The executable default e2e filter
must match the documented exclusion of Vault. Update the default filter in the
Makefile target governing make test-e2e to exclude both Bitwarden and Vault,
preserving the existing behavior for other suites and avoiding changes to the
README unless Vault is intentionally meant to run by default.
| | `TrustedCABundle` | Trusted CA Bundle | | ||
| | `TrustedCABundle` | Trusted CA Bundle (mount / `SSL_CERT_DIR` / Degraded / watch-label restore) | | ||
|
|
||
| The **Custom Network Policy Naming** spec adds a dummy egress port to `ExternalSecretsConfig` (if not already present — entries cannot be removed due to CEL immutability), verifies the operator creates `eso-user-e2e-test-custom-np` in the operand namespace (`external-secrets`), and leaves the CR entry in place. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Move the Custom Network Policy paragraph to its owning section.
This paragraph appears immediately under trusted_ca_bundle_test.go, although the feature table identifies Custom Network Policy Naming as an e2e_test.go NetworkPolicy spec. Move it near the NetworkPolicy entry to keep the documentation mapping accurate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/README.md` at line 155, Move the “Custom Network Policy Naming”
paragraph from beneath trusted_ca_bundle_test.go to the section containing the
NetworkPolicy entry for e2e_test.go. Preserve the paragraph text unchanged and
keep it associated with the NetworkPolicy spec documentation.
| annotations: | ||
| openshift.io/scc: restricted-v2 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Declare the required pod hardening explicitly.
The Deployment currently sets capability dropping and allowPrivilegeEscalation: false, but not runAsNonRoot or readOnlyRootFilesystem; the service-account token is also automounted by default. Add these controls, keeping /vault/data writable through its volume.
Suggested hardening
serviceAccountName: vault
+ automountServiceAccountToken: false
...
securityContext:
+ runAsNonRoot: true
+ readOnlyRootFilesystem: true
allowPrivilegeEscalation: falseAs per path instructions, Kubernetes/OpenShift manifests must set runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false, and disable service-account token automount unless needed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/testdata/vault/vault.yaml` around lines 51 - 52, Update the
Deployment pod security configuration in vault.yaml to explicitly set
runAsNonRoot and readOnlyRootFilesystem, retain capability dropping and
allowPrivilegeEscalation: false, and disable service-account token automount
unless required. Ensure the /vault/data volume remains writable while the
container root filesystem is read-only.
Source: Path instructions
|
@bharath-b-rh: 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. |
2 participants
Summary
BeforeAll) with a real CA→server cert chain fortrustedCABundlevalidation.InvalidProviderConfig→Ready, PushSecret/ExternalSecret sync, and ExternalSecret templating merge (Kubernetes + Vault dockerconfig).Provider:Vaultfrom the default e2e filter.Test plan
make test-e2e E2E_GINKGO_LABEL_FILTER="!(Feature: containsAny {Proxy, Upgrade})"Summary by CodeRabbit
New Features
Documentation
Tests