MULTIARCH-5550: add vault test for ppc64le#134
Open
raja-0940 wants to merge 1 commit into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 15 minutes and 56 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (72)
📒 Files selected for processing (8)
WalkthroughA new end-to-end Vault Secret Manager Ginkgo test context is added that provisions a self-contained Vault instance in the ChangesVault Secret Manager e2e test suite
Sequence Diagram(s)sequenceDiagram
participant Spec as Vault Ginkgo Spec
participant ApplyVault
participant SetupVault
participant ExecCmd as ExecCommandInPod
participant K8sAPI as Kubernetes API
participant VaultPod
participant ESO as External Secrets Operator
rect rgba(100, 149, 237, 0.5)
note over Spec,K8sAPI: Vault provisioning and initialization
Spec->>ApplyVault: apply vault manifests with image and arch substitution
ApplyVault->>K8sAPI: create Namespace SA ConfigMap Deployment Service
ApplyVault->>ApplyVault: wait for Vault pod ready
Spec->>SetupVault: initialize and unseal
SetupVault->>ExecCmd: vault operator init
ExecCmd->>VaultPod: stream exec
VaultPod-->>SetupVault: unseal key and root token in stdout
SetupVault->>ExecCmd: vault operator unseal and vault login
end
rect rgba(144, 238, 144, 0.5)
note over Spec,K8sAPI: Token and secret configuration
Spec->>K8sAPI: create vault-token Secret
Spec->>ExecCmd: enable KV engine
ExecCmd->>VaultPod: enable KV v2 at secret path
Spec->>ExecCmd: create test secret
ExecCmd->>VaultPod: vault kv put foo my-value bar
Spec->>K8sAPI: ensure allow-vault-egress policy
K8sAPI-->>Spec: patch ExternalSecretsConfig if missing
end
rect rgba(255, 165, 100, 0.5)
note over ESO,K8sAPI: Secret reconciliation and verification
Spec->>K8sAPI: apply SecretStore and ExternalSecret
ESO->>K8sAPI: watch ExternalSecret vault-example
ESO->>VaultPod: read secret data from Vault
VaultPod-->>ESO: return secret value
ESO->>K8sAPI: create k8s-secret-to-create password bar
Spec->>K8sAPI: assert k8s-secret-to-create password equals bar
Spec->>K8sAPI: delete vault-test namespace
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 4 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (3)
test/e2e/e2e_test.go (2)
726-727: Remove unused constants.
clusterSecretStoreFileandexternalSecretFileare declared but never used. The actual file paths are defined locally in the test function (lines 790-792).Context("Vault Secret Manager", Label("Platform:Vault"), func() { const ( - clusterSecretStoreFile = "testdata/vault/secret_store.yaml" - externalSecretFile = "testdata/vault/external_secret.yaml" vaultSecretName = "foo" vaultSecretKey = "my-value" vaultSecretValue = "bar" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 726 - 727, Remove the unused constants clusterSecretStoreFile and externalSecretFile from the top-level declarations in the test file; they are never referenced and duplicate the local paths that are used inside the test function (where the actual file paths are defined around lines ~790-792). Locate and delete the constant definitions named clusterSecretStoreFile and externalSecretFile so only the locally scoped variables remain.
1031-1037: Shell command construction may break with special characters.The
fmt.Sprintfconstructs a shell command with direct string interpolation. Ifsecretname,key, orvaluecontain shell metacharacters (quotes, backticks,$), the command could fail or behave unexpectedly.For test code with controlled inputs this is low risk, but consider using separate arguments to
vault kv putto avoid shell interpretation issues.Alternative: pass key-value without shell quoting
cmd := exec.Command( - "oc", "exec", "-n", vaultNamespace, podName, "--", "sh", "-c", - fmt.Sprintf( - "vault kv put secret/%s %s=\"%s\"", - secretname, key, value, - ), + "oc", "exec", "-n", vaultNamespace, podName, "--", + "vault", "kv", "put", fmt.Sprintf("secret/%s", secretname), + fmt.Sprintf("%s=%s", key, value), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 1031 - 1037, The test builds a shell command via fmt.Sprintf and sh -c which allows shell metacharacters in secretname/key/value to break behavior; update the exec.Command invocation (the cmd := exec.Command(...) that currently uses "sh", "-c" and fmt.Sprintf("vault kv put secret/%s %s=\"%s\"")) to call vault kv put with separate arguments instead of a shell string—e.g., remove "sh", "-c" and pass "vault","kv","put", fmt.Sprintf("secret/%s", secretname), fmt.Sprintf("%s=%s", key, value) (or construct the key=value arg safely) so that exec.Command handles argument quoting and no shell interpolation occurs.Makefile (1)
219-230: Inconsistencies with existingtest-e2etarget may cause issues.The new
e2e-test-vaulttarget differs fromtest-e2ein several ways that could cause problems:
- Missing
GOFLAGS=override — line 182 sets this fortest-e2eto avoid conflicts withgo.work, bute2e-test-vaultis not included- Missing
@prefix to suppress command echo- Missing
-ginkgo.traceflag- Different working directory approach (
./test/e2evs-C $(PROJECT_ROOT)/test ./e2e)Suggested fix for consistency
- -.PHONY: e2e-test-vault # Run the e2e tests against a Kind k8s instance that is spun up. + +# Targets that need Go workspace mode (CI sets GOFLAGS=-mod=vendor which conflicts with go.work) +fmt vet test test-unit test-e2e e2e-test-vault run update-vendor update-dep: GOFLAGS= + +.PHONY: e2e-test-vault e2e-test-vault: - go test \ + `@go` test -C $(PROJECT_ROOT)/test \ -timeout $(E2E_TIMEOUT) \ -count 1 \ -v \ -p 1 \ -tags e2e \ - ./test/e2e \ + ./e2e \ -ginkgo.v \ + -ginkgo.trace \ -ginkgo.show-node-events \ -ginkgo.label-filter="$(E2E_GINKGO_LABEL_FILTER)"Note: Update line 182 to include
e2e-test-vaultin the GOFLAGS override list.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 219 - 230, The new e2e-test-vault Make target is inconsistent with test-e2e; update e2e-test-vault to match: ensure it's covered by the GOFLAGS override (add e2e-test-vault to the existing GOFLAGS= override that prevents go.work conflicts), prefix the go test invocation with @ to suppress echo, add the -ginkgo.trace flag to the ginkgo flags, and run the tests from the same directory style as test-e2e (use -C $(PROJECT_ROOT)/test ./e2e rather than ./test/e2e) while keeping the same flags (-timeout $(E2E_TIMEOUT) -count 1 -v -p 1 -tags e2e -ginkgo.v -ginkgo.show-node-events -ginkgo.label-filter="$(E2E_GINKGO_LABEL_FILTER)").
🤖 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/e2e_test.go`:
- Around line 762-785: The AfterEach cleanup currently removes Vault
namespace/networkpolicy and also deletes the global ExternalSecretsConfig
"cluster"; change this block to run in AfterAll (replace AfterEach with
AfterAll) so cleanup runs once, and remove the deletion of the shared
ExternalSecretsConfig "cluster" (do not call safeDelete for
ExternalSecretsConfig "cluster"); if you need to revert Vault-specific config
changes, instead restore the ExternalSecretsConfig to its prior state (or delete
only Vault-scoped resources) using the same safeDelete helper and referencing
vaultNamespace, networkpolicy name "allow-to-vault-test", and the
ExternalSecretsConfig resource only when scoped to Vault-specific changes.
- Around line 1044-1069: In createVaultTokenSecret, don’t assume any Get() error
means “not found”: use the Kubernetes API error helper
(apierrors.IsNotFound(err)) to distinguish a missing secret from other errors;
keep the existing update path when err == nil, call Create only when
apierrors.IsNotFound(err), and for any other non-nil error return it immediately
(adjust imports to include k8s.io/apimachinery/pkg/api/errors as apierrors).
In `@test/e2e/testdata/vault/externalsecretsconfig.yaml`:
- Around line 10-14: The network policy for ExternalSecretsCoreController is
overly permissive because the egress entry `- {}` (in the policy named
`allow-external-secrets-egress`) allows all outbound traffic; update the
`networkPolicies` for componentName `ExternalSecretsCoreController` to either
remove this empty egress rule if redundant with `vault-networkpolicy.yaml` or
replace it with explicit egress rules (e.g., allow TCP/UDP port 53 for DNS and
TCP port 8200 to the Vault namespace) so only required destinations/ports are
permitted.
---
Nitpick comments:
In `@Makefile`:
- Around line 219-230: The new e2e-test-vault Make target is inconsistent with
test-e2e; update e2e-test-vault to match: ensure it's covered by the GOFLAGS
override (add e2e-test-vault to the existing GOFLAGS= override that prevents
go.work conflicts), prefix the go test invocation with @ to suppress echo, add
the -ginkgo.trace flag to the ginkgo flags, and run the tests from the same
directory style as test-e2e (use -C $(PROJECT_ROOT)/test ./e2e rather than
./test/e2e) while keeping the same flags (-timeout $(E2E_TIMEOUT) -count 1 -v -p
1 -tags e2e -ginkgo.v -ginkgo.show-node-events
-ginkgo.label-filter="$(E2E_GINKGO_LABEL_FILTER)").
In `@test/e2e/e2e_test.go`:
- Around line 726-727: Remove the unused constants clusterSecretStoreFile and
externalSecretFile from the top-level declarations in the test file; they are
never referenced and duplicate the local paths that are used inside the test
function (where the actual file paths are defined around lines ~790-792). Locate
and delete the constant definitions named clusterSecretStoreFile and
externalSecretFile so only the locally scoped variables remain.
- Around line 1031-1037: The test builds a shell command via fmt.Sprintf and sh
-c which allows shell metacharacters in secretname/key/value to break behavior;
update the exec.Command invocation (the cmd := exec.Command(...) that currently
uses "sh", "-c" and fmt.Sprintf("vault kv put secret/%s %s=\"%s\"")) to call
vault kv put with separate arguments instead of a shell string—e.g., remove
"sh", "-c" and pass "vault","kv","put", fmt.Sprintf("secret/%s", secretname),
fmt.Sprintf("%s=%s", key, value) (or construct the key=value arg safely) so that
exec.Command handles argument quoting and no shell interpolation occurs.
🪄 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: cf609882-7f2b-4bc5-a693-4dddaa3125dc
📒 Files selected for processing (7)
Makefiletest/e2e/e2e_test.gotest/e2e/testdata/vault/cluster_secret_store.yamltest/e2e/testdata/vault/external_secret.yamltest/e2e/testdata/vault/externalsecretsconfig.yamltest/e2e/testdata/vault/vault-networkpolicy.yamltest/e2e/testdata/vault/vault.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/e2e/e2e_test.go (3)
725-731: Remove unused constants.
clusterSecretStoreFileandexternalSecretFileare declared but never used. The test instead defines local variablesvaultSecretStoreFileandvaultExternalSecretFileat lines 784-785.Suggested fix
Context("Vault Secret Manager", Label("Platform:Vault"), func() { const ( - clusterSecretStoreFile = "testdata/vault/secret_store.yaml" - externalSecretFile = "testdata/vault/external_secret.yaml" - vaultSecretName = "foo" - vaultSecretKey = "my-value" - vaultSecretValue = "bar" + vaultSecretName = "foo" + vaultSecretKey = "my-value" + vaultSecretValue = "bar" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 725 - 731, Remove the unused constants clusterSecretStoreFile and externalSecretFile from the constants block (they are shadowed by local variables vaultSecretStoreFile and vaultExternalSecretFile later in the test), leaving only the actually used constants vaultSecretName, vaultSecretKey, and vaultSecretValue; update the constants declaration near the top of the test in e2e_test.go so there are no unused variables left.
874-874: Remove unusedloaderparameter.The
loaderparameter is passed toapplyVaultbut never used within the function.Suggested fix
-func applyVault(ctx context.Context, loader utils.DynamicResourceLoader) error { +func applyVault(ctx context.Context) error {And update the call site at line 735:
- Expect(applyVault(ctx, loader)).To(Succeed()) + Expect(applyVault(ctx)).To(Succeed())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` at line 874, The applyVault function signature includes an unused parameter loader; remove the unused parameter from the applyVault declaration (change func applyVault(ctx context.Context, loader utils.DynamicResourceLoader) to func applyVault(ctx context.Context)) and update all call sites that pass a loader to call applyVault with only the ctx argument (i.e., remove the extra argument where applyVault is invoked) so the function and its callers remain consistent.
1004-1014: Consider more granular error handling inenableKVEngine.The
|| trueat line 1007 masks all errors, including authentication failures or Vault unavailability. While this is intentional for idempotent KV engine enabling, consider checking only for the "already enabled" error.Alternative approach
cmd := exec.Command( "oc", "exec", "-n", vaultNamespace, podName, "--", "sh", "-c", fmt.Sprintf( - "vault status && vault login %s && vault secrets enable -path=secret kv-v2 || true", + "vault status && vault login %s && (vault secrets enable -path=secret kv-v2 2>&1 || echo 'KV engine may already be enabled')", token, ), )Alternatively, check the exit code explicitly and only ignore the "path is already in use" error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/e2e_test.go` around lines 1004 - 1014, The current command uses "|| true" which masks all errors; in the enableKVEngine logic replace the blanket ignore with targeted handling: run the same exec.Command via utils.Run (the existing cmd, podName, vaultNamespace, token) but if it returns an error inspect the output/error string or exit code and only suppress the error when it indicates the KV engine is already enabled (e.g., message like "path is already in use" or equivalent); for any other error (authentication failure, vault unavailable, etc.) propagate/return the error so failures are not silently ignored.
🤖 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/e2e_test.go`:
- Around line 725-731: Remove the unused constants clusterSecretStoreFile and
externalSecretFile from the constants block (they are shadowed by local
variables vaultSecretStoreFile and vaultExternalSecretFile later in the test),
leaving only the actually used constants vaultSecretName, vaultSecretKey, and
vaultSecretValue; update the constants declaration near the top of the test in
e2e_test.go so there are no unused variables left.
- Line 874: The applyVault function signature includes an unused parameter
loader; remove the unused parameter from the applyVault declaration (change func
applyVault(ctx context.Context, loader utils.DynamicResourceLoader) to func
applyVault(ctx context.Context)) and update all call sites that pass a loader to
call applyVault with only the ctx argument (i.e., remove the extra argument
where applyVault is invoked) so the function and its callers remain consistent.
- Around line 1004-1014: The current command uses "|| true" which masks all
errors; in the enableKVEngine logic replace the blanket ignore with targeted
handling: run the same exec.Command via utils.Run (the existing cmd, podName,
vaultNamespace, token) but if it returns an error inspect the output/error
string or exit code and only suppress the error when it indicates the KV engine
is already enabled (e.g., message like "path is already in use" or equivalent);
for any other error (authentication failure, vault unavailable, etc.)
propagate/return the error so failures are not silently ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92efc958-9b30-428c-b949-6e886bc8d253
📒 Files selected for processing (1)
test/e2e/e2e_test.go
Author
|
/retitle MULTIARCH-5550: add vault test for ppc64le |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@raja-0940: This pull request references MULTIARCH-5550 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 epic to target either version "4.22." or "openshift-4.22.", but it targets "openshift-4.21" 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 (1)
test/e2e/testdata/vault/vault.yaml (1)
53-84: Consider adding resource requests/limits.The container lacks resource requests and limits. While acceptable for e2e tests, defining them can prevent resource contention and improve scheduling predictability in shared test clusters.
💡 Optional: Add resource constraints
ports: - containerPort: 8200 securityContext: allowPrivilegeEscalation: false capabilities: drop: - ALL + resources: + requests: + cpu: 100m + memory: 256Mi + limits: + cpu: 500m + memory: 512Mi volumeMounts:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/vault/vault.yaml` around lines 53 - 84, The vault container spec (container name "vault") is missing resources; add a resources block under the container to set requests and limits (e.g., resources.requests.cpu, resources.requests.memory and resources.limits.cpu, resources.limits.memory) so the "vault" container has both minimal guaranteed resources and caps; update the same container entry that contains command/args/readinessProbe/volumeMounts to include these fields with appropriate values for the e2e environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 219-230: The Makefile target e2e-test-vault is missing from the
target-specific GOFLAGS reset list, causing exported GOFLAGS (e.g. -mod=vendor)
to be applied and break workspace mode; update the GOFLAGS override list (the
target-specific GOFLAGS= reset block) to include e2e-test-vault so that the
e2e-test-vault target uses a clean GOFLAGS environment when invoking go test.
- Line 230: Remove the extraneous double quotes around E2E_GINKGO_LABEL_FILTER
so the ginkgo.label-filter uses the already-quoted variable (match the pattern
used by test-e2e for E2E_GINKGO_LABEL_FILTER), and update the GOFLAGS reset list
to include the e2e-test-vault target so it does not inherit CI's -mod=vendor
(add e2e-test-vault to the list where GOFLAGS is cleared/reset).
In `@test/e2e/testdata/vault/vault.yaml`:
- Around line 1-5: The YAML file defining a Namespace (kind: Namespace,
metadata.name: vault-test) uses incorrect line endings; convert the file
test/e2e/testdata/vault/vault.yaml to use Unix-style LF line endings (\n)
throughout (ensure no CRLF sequences), then re-save/commit the file so CI and
parsers read the manifest correctly.
---
Nitpick comments:
In `@test/e2e/testdata/vault/vault.yaml`:
- Around line 53-84: The vault container spec (container name "vault") is
missing resources; add a resources block under the container to set requests and
limits (e.g., resources.requests.cpu, resources.requests.memory and
resources.limits.cpu, resources.limits.memory) so the "vault" container has both
minimal guaranteed resources and caps; update the same container entry that
contains command/args/readinessProbe/volumeMounts to include these fields with
appropriate values for the e2e environment.
🪄 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: 12c742d1-a59f-434a-b8d0-27caf760dca9
📒 Files selected for processing (7)
Makefiletest/e2e/e2e_test.gotest/e2e/testdata/vault/cluster_secret_store.yamltest/e2e/testdata/vault/external_secret.yamltest/e2e/testdata/vault/externalsecretsconfig.yamltest/e2e/testdata/vault/vault-networkpolicy.yamltest/e2e/testdata/vault/vault.yaml
✅ Files skipped from review due to trivial changes (3)
- test/e2e/testdata/vault/external_secret.yaml
- test/e2e/testdata/vault/externalsecretsconfig.yaml
- test/e2e/testdata/vault/vault-networkpolicy.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/e2e/testdata/vault/cluster_secret_store.yaml
- test/e2e/e2e_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/e2e/testdata/vault/vault.yaml (1)
1-5:⚠️ Potential issue | 🟡 MinorFix newline character format.
Static analysis indicates the file uses incorrect line endings (CRLF). Convert to Unix-style line endings (
\n) for CI compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/vault/vault.yaml` around lines 1 - 5, The YAML file defining the Namespace (look for the block with "apiVersion: v1", "kind: Namespace", and "name: vault-test") uses CRLF line endings and must be converted to Unix-style LF; open that vault.yaml, normalize line endings to "\n" (e.g., use your editor's EOL conversion, run dos2unix, or git's core.autocrlf settings), verify there are no trailing CR (\r) characters, and recommit the file.
🤖 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/testdata/vault/vault.yaml`:
- Around line 1-5: The YAML file defining the Namespace (look for the block with
"apiVersion: v1", "kind: Namespace", and "name: vault-test") uses CRLF line
endings and must be converted to Unix-style LF; open that vault.yaml, normalize
line endings to "\n" (e.g., use your editor's EOL conversion, run dos2unix, or
git's core.autocrlf settings), verify there are no trailing CR (\r) characters,
and recommit the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 58ce074d-3ad7-4f54-bfdf-f62ae51dcb00
📒 Files selected for processing (4)
Makefiletest/e2e/e2e_test.gotest/e2e/testdata/vault/externalsecretsconfig.yamltest/e2e/testdata/vault/vault.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/testdata/vault/externalsecretsconfig.yaml
Author
|
/retitle MULTIARCH-5550: add vault test for ppc64le |
Author
|
/test verify |
prb112
reviewed
Apr 8, 2026
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/e2e/testdata/vault/vault.yaml (1)
1-115:⚠️ Potential issue | 🟡 MinorRe-save this manifest with LF line endings.
YAMLlint is still reporting CRLF here, so CI will keep failing until the file is saved with Unix newlines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/testdata/vault/vault.yaml` around lines 1 - 115, The manifest uses CRLF endings; re-save vault.yaml with Unix LF line endings so YAML lint passes (affects resources like the Namespace named "vault-test", Deployment/ServiceAccount/Deployment named "vault", and ConfigMap "vault-config"); open the file in your editor or run a tool (e.g., convert line endings or dos2unix) to normalize to LF, commit the updated file, and ensure your local git autocrlf setting won't reintroduce CRLF.
🤖 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/e2e_test.go`:
- Around line 918-924: The test currently prints the raw Vault command outputs
(initOut from utils.Run(initCmd) and similarly login output) via
By(fmt.Sprintf(...)), leaking unseal keys and tokens; change the test to NOT
print raw command output to logs/artifacts—keep capturing initOut/loginOut for
error handling but replace the By(fmt.Sprintf(...)) calls with sanitized status
messages like "Vault operator init completed" and "Vault login completed", and
if you must log details, redact sensitive fields from initOut/loginOut before
logging or only log non-sensitive status lines; ensure functions/variables
initOut, initCmd, and the By(...) calls are updated accordingly.
- Around line 762-767: The test suite currently spawns oc subprocesses with
exec.Command which can hang; change calls in functions that accept a context
(setupVault, enableKVEngine, createVaultTestSecret) to use
exec.CommandContext(ctx, ...) so they respect the caller's timeout, and update
all cleanup invocations to create a short deadline context (context.WithTimeout)
and call a modified safeDelete(ctx, cmd) that uses exec.CommandContext
internally; specifically update safeDelete to accept a context.Context and run
the oc delete/apply/exec commands via exec.CommandContext, and replace the
listed direct exec.Command usages (around the safeDelete calls and the lines for
setupVault/enableKVEngine/createVaultTestSecret) to use the context-aware
variants so subprocesses are bounded by deadlines.
---
Duplicate comments:
In `@test/e2e/testdata/vault/vault.yaml`:
- Around line 1-115: The manifest uses CRLF endings; re-save vault.yaml with
Unix LF line endings so YAML lint passes (affects resources like the Namespace
named "vault-test", Deployment/ServiceAccount/Deployment named "vault", and
ConfigMap "vault-config"); open the file in your editor or run a tool (e.g.,
convert line endings or dos2unix) to normalize to LF, commit the updated file,
and ensure your local git autocrlf setting won't reintroduce CRLF.
🪄 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: 16992a33-62aa-4ee9-93a4-e0710accdc67
📒 Files selected for processing (3)
Makefiletest/e2e/e2e_test.gotest/e2e/testdata/vault/vault.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: prb112, raja-0940 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 |
Author
|
@swghosh Could you please review this PR? |
|
/retest-required |
|
Hi @swghosh - our tests are passing on IBM Power. We'd appreciate a merge so we can start testing it. Thank you, Paul |
Contributor
hey @prb112 cc: @bharath-b-rh as he is leading the ESO efforts now. |
|
Thank you @mytreya-rh Hi @bharath-b-rh happy to take feedback, many thanks in advance, Paul |
Contributor
|
Thank you @prb112 ! I will take a look tomorrow or early next week. |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
Contributor
There was a problem hiding this comment.
Please update the PR description with more details to help with the reviews.
|
@raja-0940: This pull request references MULTIARCH-5550 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.21" 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. |
prb112
reviewed
May 20, 2026
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
raja-0940
force-pushed
the
eso-e2e-clean
branch
3 times, most recently
from
bdf52eb to
c904975
Compare
Author
|
Hi @bharath-b-rh, I have resolved all the PR review comments. Could you please review them again? Thanks |
Contributor
|
Hi @raja-0940 , Thank you for addressing the comments! I will take a look at the changes on Thursday. |
|
/retest-required |
|
Hey @bharath-b-rh looking at the retest, I think this is saying the go.mod is missing a dependency? I think we've got it all covered already, any advice. Thank you, Paul p.s. Also happy to make changes based on review comments. cc: @raja-0940 |
bharath-b-rh
left a comment
bharath-b-rh
left a comment
Contributor
There was a problem hiding this comment.
For the verify CI failure, please use make update-vendor and include the changes in repo root and test dir go mods too.
| var arch string | ||
|
|
||
| // Try multiple sources for architecture information | ||
| if a, ok := node.Labels["kubernetes.io/arch"]; ok && a != "" { |
Contributor
There was a problem hiding this comment.
nit: I am not well versed with OpenShift CI infra. But are there any chances of multi-arch data plane clusters, in which case this could fail I think.
If we want to enhance the handling, what we could do is, check the node arch where the operator is scheduled and deploy the vault pod too on the same node by using podAffinity. WDYT?
prb112
reviewed
Jun 15, 2026
| metadata: | ||
| labels: | ||
| app: vault | ||
| spec: |
raja-0940
force-pushed
the
eso-e2e-clean
branch
3 times, most recently
from
07af3ac to
be8537f
Compare
prb112
reviewed
Jun 19, 2026
| - key: kubernetes.io/arch | ||
| operator: In | ||
| values: | ||
| - {{VAULT_ARCH}} |
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
raja-0940
force-pushed
the
eso-e2e-clean
branch
2 times, most recently
from
b14581a to
fadf332
Compare
Author
|
/retest-required |
Author
|
/test verify |
raja-0940
force-pushed
the
eso-e2e-clean
branch
3 times, most recently
from
edc8d9a to
f57a0e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/testdata/vault/externalsecretsconfig.yaml (1)
10-19: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider scoping egress destinations for defense-in-depth.
The first egress rule permits ports 6443 (K8s API), 443 (HTTPS), and 5353 (DNS) to ANY destination without a
toselector. While the ESO controller legitimately requires K8s API and DNS access, scoping these destinations would follow defense-in-depth principles.For a test fixture, the current pragmatic approach is acceptable. If tightening is desired, consider restricting:
- Ports 6443/443 to the
kubernetes.defaultservice or API server endpoints- Port 5353 to DNS service pods (e.g.,
kube-dnsoropenshift-dnsnamespaces)🤖 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/externalsecretsconfig.yaml` around lines 10 - 19, The egress rules in the network policy (ports 6443, 443, and 5353) currently lack destination scoping with a `to` selector, allowing traffic to any destination. To apply defense-in-depth principles, add a `to` selector for each egress rule that specifies the legitimate destination endpoints: restrict TCP ports 6443 and 443 to the kubernetes.default service or API server endpoints, and restrict UDP/TCP port 5353 to DNS service pods in the kube-dns or openshift-dns namespaces. Note that this is optional for a test fixture, but implementing it would improve the security posture of the configuration.
🤖 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/testdata/vault/externalsecretsconfig.yaml`:
- Around line 10-22: The externalsecretsconfig.yaml NetworkPolicy has a
conflicting port 8200 TCP rule in the first unrestricted egress section that
permits traffic to any destination, which overrides the subsequent
namespace-scoped egress rule (lines 23-29) that attempts to restrict port 8200
traffic to the vault-test namespace. Remove the port 8200 entry (the TCP rule on
port 8200 at lines 20-21) from the first egress block to eliminate the conflict
and allow the namespace-scoped rule to take effect, since NetworkPolicy rules
are evaluated with OR semantics where the broader permission takes precedence.
---
Nitpick comments:
In `@test/e2e/testdata/vault/externalsecretsconfig.yaml`:
- Around line 10-19: The egress rules in the network policy (ports 6443, 443,
and 5353) currently lack destination scoping with a `to` selector, allowing
traffic to any destination. To apply defense-in-depth principles, add a `to`
selector for each egress rule that specifies the legitimate destination
endpoints: restrict TCP ports 6443 and 443 to the kubernetes.default service or
API server endpoints, and restrict UDP/TCP port 5353 to DNS service pods in the
kube-dns or openshift-dns namespaces. Note that this is optional for a test
fixture, but implementing it would improve the security posture of the
configuration.
🪄 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: 0fd11aec-d5e7-440a-aed9-a8f37de77f94
⛔ Files ignored due to path filters (72)
go.work.sumis excluded by!**/*.sumtest/go.sumis excluded by!**/*.sumvendor/github.com/gorilla/websocket/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/AUTHORSis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/compression.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/conn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/join.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/json.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/mask_safe.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/prepared.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gorilla/websocket/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/MAINTAINERSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/connection.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/handlers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/priority.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/PATENTSis excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/dictionary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/read.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/spdy/write.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/stream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/moby/spdystream/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/flowrate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/io.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/mxk/go-flowrate/flowrate/util.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/client.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/internal/socks/socks.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/direct.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/per_host.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/proxy.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/net/proxy/socks5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/httpstream/spdy/upgrade.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/dial.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/transport.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/apimachinery/third_party/forked/golang/netutil/addr.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/doc.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/errorstream.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/fallback.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/reader.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/remotecommand.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/resize.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v1.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v2.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v3.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v4.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/v5.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/tools/remotecommand/websocket.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/spdy/spdy.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/transport/websocket/roundtripper.gois excluded by!**/vendor/**,!vendor/**vendor/k8s.io/client-go/util/exec/exec.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (8)
test/e2e/e2e_test.gotest/e2e/testdata/vault/external_secret.yamltest/e2e/testdata/vault/externalsecretsconfig.yamltest/e2e/testdata/vault/secret_store.yamltest/e2e/testdata/vault/vault.yamltest/go.modtest/utils/dynamic_resources.gotest/utils/kube_client.go
🚧 Files skipped from review as they are similar to previous changes (7)
- test/e2e/testdata/vault/secret_store.yaml
- test/e2e/testdata/vault/external_secret.yaml
- test/e2e/testdata/vault/vault.yaml
- test/utils/dynamic_resources.go
- test/go.mod
- test/e2e/e2e_test.go
- test/utils/kube_client.go
Signed-off-by: Rajakumar Battula <rbattula@redhat.com>
|
@raja-0940: 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. |
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
6 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
Adds e2e test suite for vault with IBM power.
Description
Platform:Vault - ESO CR-based flow:
ExternalSecretsConfigBeforeAllClusterSecretStoreExternalSecretAfterAllTesting
Summary by CodeRabbit
Tests
Chores