CNTRLPLANE-3364: utility for vault deployer#2189
Closed
sandeepknd wants to merge 1 commit intoopenshift:masterfrom
Closed
CNTRLPLANE-3364: utility for vault deployer#2189sandeepknd wants to merge 1 commit intoopenshift:masterfrom
sandeepknd wants to merge 1 commit 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:
WalkthroughAdds a Vault Enterprise Kubernetes test deployer and accompanying tests: deploys Vault into a namespace, reads/creates license secret, initializes and unseals Vault, configures transit KMS and AppRole auth, stores AppRole credentials, exposes getters and Cleanup, and includes unit and integration tests. Changes
Sequence DiagramsequenceDiagram
actor TestHarness as Test Harness
participant K8sAPI as Kubernetes API
participant VaultPod as Vault Pod
participant VaultCLI as Vault CLI (in Pod)
TestHarness->>K8sAPI: Apply shared manifests & deployment YAML
K8sAPI->>VaultPod: Create Pod, Service, RBAC, PVC/emptyDir
TestHarness->>K8sAPI: Poll Pod status until Running+Ready
K8sAPI-->>TestHarness: Pod Running+Ready
TestHarness->>VaultCLI: Exec `vault operator init -format=json`
VaultCLI-->>TestHarness: Return root_token + unseal_keys
TestHarness->>VaultCLI: Exec `vault operator unseal` (use threshold keys)
VaultCLI-->>TestHarness: Vault unsealed
TestHarness->>VaultCLI: Enable transit, create key, enable AppRole, write policy, create role
VaultCLI-->>TestHarness: Return role_id & secret_id
TestHarness->>K8sAPI: Create/update secrets (vault-root-token, vault-license, approle creds)
TestHarness->>K8sAPI: Delete namespace (Cleanup)
K8sAPI-->>TestHarness: Namespace and resources removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 inconclusive)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
|
Skipping CI for Draft Pull Request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
test/library/encryption/kms/GUIDE.md (1)
65-65: Add language specifiers to fenced code blocks.Multiple code blocks are missing language specifiers, which affects syntax highlighting and violates markdownlint rules. Add appropriate language identifiers (e.g.,
text,plaintext, or specific languages).Affected lines: 65, 117, 161, 190, 220, 232, 242, 261, 290.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` at line 65, Several fenced code blocks in GUIDE.md lack language specifiers (affecting syntax highlighting and markdownlint). Edit the affected fenced code blocks (those around the examples referenced) and add appropriate language identifiers such as text, plaintext, or the specific language (e.g., bash, json) as appropriate; search for triple-backtick fences in GUIDE.md (examples near the noted sections) and update each opening ``` to include the correct specifier to satisfy markdownlint and enable highlighting.test/library/encryption/kms/vault_kube_kms_deployer.go (3)
330-372: Consider removing or documenting unusedapplyManifestsfunction.The
applyManifestsfunction appears unused since deployment now goes through Helm (installVaultWithHelm). If this is intentional dead code for future use, add a comment explaining its purpose. Otherwise, consider removing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 330 - 372, The applyManifests method on VaultDeployer appears to be dead/unused now that installVaultWithHelm handles deployment; either remove the entire applyManifests implementation (and any references to vaultSharedManifestFiles, vaultDeploymentManifestFile, wrapAssetWithTemplateData) to avoid stale code, or add a concise doc comment above func (d *VaultDeployer) applyManifests explaining why it is retained (e.g., "kept for tests/legacy/manual K8s manifest application; not used by installVaultWithHelm") and mark it as intentional dead code so reviewers understand its purpose.
152-153: Replace arbitrary sleep with proper readiness check.The
time.Sleep(5 * time.Second)is a code smell. Consider polling Vault's health endpoint until it's ready to accept commands.♻️ Suggested improvement
- // Wait a bit for Vault to be ready to accept commands - time.Sleep(5 * time.Second) + // Wait for Vault to be ready to accept commands + if err := d.waitForVaultReady(ctx); err != nil { + return fmt.Errorf("Vault not ready to accept commands: %w", err) + }Add a new method that polls the Vault status endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 152 - 153, Replace the arbitrary time.Sleep(5 * time.Second) with a proper readiness poll: implement a new helper method (e.g., waitForVaultReady or pollVaultHealth) that repeatedly queries Vault's health endpoint (GET /v1/sys/health) with a timeout and backoff, checks the response status/body for readiness (initialized and not sealed / not standby or HTTP 200), and returns an error on timeout; then call this helper from where time.Sleep is currently used (replace time.Sleep with waitForVaultReady) so the deployer waits deterministically for Vault to accept commands.
711-714: Remove unusedescapeForShellfunction.The
escapeForShellfunction is defined but never used in the codebase.-// escapeForShell escapes a string for safe use in shell commands -func escapeForShell(s string) string { - return strings.ReplaceAll(s, "'", "'\\''") -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 711 - 714, Remove the unused helper function escapeForShell by deleting its entire definition (the escapeForShell(s string) function) and also remove any now-unused imports (e.g., "strings") that were only required for that function so the package still builds cleanly; if you prefer to keep similar behavior, replace any ad-hoc shell-escaping usages with a single shared utility and ensure it's actually referenced, otherwise simply remove the function and tidy imports.test/library/encryption/kms/vault_kube_kms_deployer_test.go (1)
524-532: Consider using exponential backoff for namespace deletion polling.The fixed 1-second sleep interval with 30 iterations is acceptable but could benefit from using
wait.PollUntilContextTimeoutfor consistency with other polling patterns in this codebase.♻️ Suggested improvement
err = wait.PollUntilContextTimeout(ctx, 1*time.Second, 30*time.Second, true, func(ctx context.Context) (bool, error) { _, err := kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) if apierrors.IsNotFound(err) { return true, nil } return false, nil }) if err != nil { t.Error("✗ Namespace should be deleted after cleanup") } else { t.Logf("✓ Namespace %s successfully deleted", testNamespace) }This requires importing
apierrors "k8s.io/apimachinery/pkg/api/errors"and"k8s.io/apimachinery/pkg/util/wait".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 524 - 532, Replace the manual for-loop polling in the test that checks namespace deletion with Kubernetes' wait.PollUntilContextTimeout for consistency: instead of the loop around kubeClient.CoreV1().Namespaces().Get and time.Sleep, call wait.PollUntilContextTimeout with a 1s interval and 30s timeout, use kubeClient.CoreV1().Namespaces().Get inside the predicate and return true when apierrors.IsNotFound(err), otherwise false; ensure you import "k8s.io/apimachinery/pkg/util/wait" and apierrors "k8s.io/apimachinery/pkg/api/errors" and update the test assertion to fail on a returned error from PollUntilContextTimeout and log success when it returns nil.
🤖 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/library/encryption/kms/GUIDE.md`:
- Line 79: Replace hardcoded user-specific absolute paths in the GUIDE.md
instructions (e.g., the "cd
/home/skundu/automation/library-go/test/library/encryption/kms" command and
similar occurrences referenced at lines 146, 156, 185, and 209) with either
relative paths (like "cd ../../test/library/encryption/kms") or a generic
placeholder (like "cd
/path/to/automation/library-go/test/library/encryption/kms") so the guide is
portable; update each matching "cd" or path string in the document accordingly
and ensure any example commands use the placeholder or relative path
consistently.
- Line 34: The Docker image tag in the GUIDE.md snippet references a
non-existent Vault Enterprise version
"docker.io/hashicorp/vault-enterprise:2.0.0-ent"; update that tag to an existing
release (e.g., replace "docker.io/hashicorp/vault-enterprise:2.0.0-ent" with
"docker.io/hashicorp/vault-enterprise:1.21.4+ent") so the documentation uses a
valid Vault Enterprise image.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 580-592: Replace the custom contains and findSubstring helpers
with the stdlib strings.Contains: remove the contains and findSubstring
functions and add import "strings"; then update the two call sites that
currently call contains(contentStr, "emptyDir") and contains(contentStr,
"persistentVolumeClaim") to use strings.Contains(contentStr, "...") instead so
the code uses the standard, tested implementation.
- Around line 358-360: The test currently prints sensitive AppRole credentials
(roleID and secretID) via t.Logf; instead redact or abbreviate these values
before logging (e.g., show only first/last N chars or replace with a fixed mask)
and/or remove the logs entirely. Update the t.Logf calls that reference roleID
and secretID in vault_kube_kms_deployer_test.go to use a redaction helper or
inline substring masking so the logs no longer emit full credentials while
preserving enough info for debugging.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 441-448: The current branch in the error path of initializeVault
returns hard-coded dummy credentials when d.isVaultInitialized(ctx, podName) is
true which will break subsequent unsealVault and configureVaultForKMS calls;
change this behavior to fail fast instead: when isVaultInitialized(ctx, podName)
returns true, return a non-nil error describing "Vault already initialized" (do
not return "EXISTING_ROOT_TOKEN" or dummy keys), so callers (e.g., unsealVault,
configureVaultForKMS) will not attempt to use invalid credentials; update any
callers or tests expecting the dummy return to handle the error path
accordingly.
- Around line 302-307: Replace the hardcoded temp path with a unique temp file
created via os.CreateTemp and write valuesContent into it (instead of using the
literal "/tmp/vault-values.yaml"); update uses of valuesFile to use the returned
*os.File (valuesFile.Name() where the path string is needed) and ensure you
still defer os.Remove(valuesFile.Name()) and close the file; look for the
valuesFile variable and the code that writes valuesContent to update the write,
close, and remove logic accordingly.
- Around line 493-505: The unseal loop hardcodes a threshold of 3; replace that
with the real threshold returned by Vault init (or a central constant) so
changes don't break unsealing: obtain the threshold value from the init result
used to populate unsealKeys (e.g., use initResp.Threshold or the struct that
produced unsealKeys) and loop for j := 0; j < threshold && j < len(unsealKeys);
j++ when calling d.execInPod; also validate threshold <= len(unsealKeys) and
return a clear error if not.
- Around line 172-173: Remove the line that logs the Vault root token to test
output: stop calling d.t.Logf("Vault root token: %s", rootToken). Keep or adjust
the address log (d.t.Logf("Vault address: http://%s.%s.svc:8200",
WellKnownVaultServiceName, d.config.Namespace)) if needed, and instead store or
pass rootToken via a secure test secret/variable or return it from the deployer
object without printing; reference the rootToken identifier and the d.t.Logf
call in vault_kube_kms_deployer.go when making the change.
- Around line 284-287: The current image tag parsing using strings.Split and
index arithmetic is fragile; replace that expression with logic that finds the
last ':' only if it appears after the last '/' so ports (registry:port) are
ignored and images without tags default to "latest": compute lastSlash :=
strings.LastIndex(d.config.Image, "/"), lastColon :=
strings.LastIndex(d.config.Image, ":"), then if lastColon > lastSlash set tag :=
d.config.Image[lastColon+1:] and base := d.config.Image[:lastColon] else set tag
:= "latest" and base := d.config.Image; use these base and tag variables instead
of the existing strings.Split/TrimSuffix expression to build the YAML values
(referencing d.config.Image and the current split/trim expression in the diff).
---
Nitpick comments:
In `@test/library/encryption/kms/GUIDE.md`:
- Line 65: Several fenced code blocks in GUIDE.md lack language specifiers
(affecting syntax highlighting and markdownlint). Edit the affected fenced code
blocks (those around the examples referenced) and add appropriate language
identifiers such as text, plaintext, or the specific language (e.g., bash, json)
as appropriate; search for triple-backtick fences in GUIDE.md (examples near the
noted sections) and update each opening ``` to include the correct specifier to
satisfy markdownlint and enable highlighting.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 524-532: Replace the manual for-loop polling in the test that
checks namespace deletion with Kubernetes' wait.PollUntilContextTimeout for
consistency: instead of the loop around kubeClient.CoreV1().Namespaces().Get and
time.Sleep, call wait.PollUntilContextTimeout with a 1s interval and 30s
timeout, use kubeClient.CoreV1().Namespaces().Get inside the predicate and
return true when apierrors.IsNotFound(err), otherwise false; ensure you import
"k8s.io/apimachinery/pkg/util/wait" and apierrors
"k8s.io/apimachinery/pkg/api/errors" and update the test assertion to fail on a
returned error from PollUntilContextTimeout and log success when it returns nil.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 330-372: The applyManifests method on VaultDeployer appears to be
dead/unused now that installVaultWithHelm handles deployment; either remove the
entire applyManifests implementation (and any references to
vaultSharedManifestFiles, vaultDeploymentManifestFile,
wrapAssetWithTemplateData) to avoid stale code, or add a concise doc comment
above func (d *VaultDeployer) applyManifests explaining why it is retained
(e.g., "kept for tests/legacy/manual K8s manifest application; not used by
installVaultWithHelm") and mark it as intentional dead code so reviewers
understand its purpose.
- Around line 152-153: Replace the arbitrary time.Sleep(5 * time.Second) with a
proper readiness poll: implement a new helper method (e.g., waitForVaultReady or
pollVaultHealth) that repeatedly queries Vault's health endpoint (GET
/v1/sys/health) with a timeout and backoff, checks the response status/body for
readiness (initialized and not sealed / not standby or HTTP 200), and returns an
error on timeout; then call this helper from where time.Sleep is currently used
(replace time.Sleep with waitForVaultReady) so the deployer waits
deterministically for Vault to accept commands.
- Around line 711-714: Remove the unused helper function escapeForShell by
deleting its entire definition (the escapeForShell(s string) function) and also
remove any now-unused imports (e.g., "strings") that were only required for that
function so the package still builds cleanly; if you prefer to keep similar
behavior, replace any ad-hoc shell-escaping usages with a single shared utility
and ensure it's actually referenced, otherwise simply remove the function and
tidy imports.
🪄 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: 4c97b637-7eae-4794-9ec6-8a265f46c606
📒 Files selected for processing (3)
test/library/encryption/kms/GUIDE.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
sandeepknd
force-pushed
the
deploy_vault
branch
from
e60a6cd to
2adf31b
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (7)
test/library/encryption/kms/GUIDE.md (1)
78-79:⚠️ Potential issue | 🟡 MinorReplace user-specific absolute paths with portable placeholders.
These paths are environment-specific and make the guide hard to follow for others.
Also applies to: 145-146, 155-156, 184-185, 208-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 78 - 79, The guide contains user-specific absolute paths like "/home/skundu/automation/library-go/test/library/encryption/kms" (and the other occurrences at the noted locations) which should be replaced with portable placeholders; update the GUIDE.md entries to use a repo-root or environment variable placeholder (e.g., <REPO_ROOT> or $PROJECT_ROOT or relative paths like ./test/library/encryption/kms or ~/) and add a brief note explaining how to substitute the placeholder for local users so the instructions are environment-agnostic.test/library/encryption/kms/vault_kube_kms_deployer.go (5)
286-287:⚠️ Potential issue | 🟠 MajorImage repo/tag parsing is brittle for registries with ports and untagged images.
The current split/trim approach can build invalid repo/tag combinations.
🔧 Safer parsing using last ':' after last '/'
+ lastSlash := strings.LastIndex(d.config.Image, "/") + lastColon := strings.LastIndex(d.config.Image, ":") + imageRepo := d.config.Image + imageTag := "latest" + if lastColon > lastSlash { + imageRepo = d.config.Image[:lastColon] + imageTag = d.config.Image[lastColon+1:] + } ... -`, strings.TrimSuffix(d.config.Image, ":"+strings.Split(d.config.Image, ":")[len(strings.Split(d.config.Image, ":"))-1]), - strings.Split(d.config.Image, ":")[len(strings.Split(d.config.Image, ":"))-1]) +`, imageRepo, imageTag)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 286 - 287, The image repo/tag assembly is brittle where the code uses strings.Split on d.config.Image; replace that logic with parsing that finds the last ':' only if it occurs after the last '/' (i.e., use strings.LastIndex and strings.LastIndexByte or similar) to separate repo and tag safely for images with registry ports or untagged images; update the expression that builds the repo and tag from d.config.Image accordingly (refer to the use of d.config.Image in vault_kube_kms_deployer.go and the surrounding repo/tag assembly) so that if no tag is present you treat the whole string as the repo and if a ':' is part of the registry host (before the last '/') you do not split there.
443-446:⚠️ Potential issue | 🔴 CriticalFail fast when Vault is already initialized instead of returning fake credentials.
Returning
"EXISTING_ROOT_TOKEN"and dummy keys makes downstream unseal/config steps fail with misleading errors.🔧 Suggested fix
if d.isVaultInitialized(ctx, podName) { - d.t.Logf("Vault already initialized, skipping initialization") - return "EXISTING_ROOT_TOKEN", []string{"KEY1", "KEY2", "KEY3"}, nil + return "", nil, fmt.Errorf("vault already initialized; existing credentials must be provided") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 443 - 446, The current branch in the function that checks d.isVaultInitialized(ctx, podName) must not return fake credentials ("EXISTING_ROOT_TOKEN", ["KEY1",...]) — instead fail fast by returning an explicit error so callers know Vault is already initialized; modify the block in the method that calls d.isVaultInitialized to return a non-nil error (e.g., ErrVaultAlreadyInitialized or fmt.Errorf with context including podName) and no fake token/keys, so unseal/config callers receive a clear failure instead of misleading dummy values.
302-307:⚠️ Potential issue | 🟠 MajorUse a unique temp values file to avoid parallel-test collisions.
Line 303 uses a fixed
/tmp/vault-values.yaml; concurrent runs can overwrite each other.🔧 Use `os.CreateTemp` and pass `valuesFile.Name()`
- valuesFile := "/tmp/vault-values.yaml" - if err := os.WriteFile(valuesFile, []byte(valuesContent), 0644); err != nil { + valuesFile, err := os.CreateTemp("", "vault-values-*.yaml") + if err != nil { + return fmt.Errorf("failed to create values file: %w", err) + } + defer os.Remove(valuesFile.Name()) + if _, err := valuesFile.WriteString(valuesContent); err != nil { + valuesFile.Close() return fmt.Errorf("failed to write values file: %w", err) } - defer os.Remove(valuesFile) + if err := valuesFile.Close(); err != nil { + return fmt.Errorf("failed to close values file: %w", err) + } ... - "--values", valuesFile, + "--values", valuesFile.Name(),Also applies to: 313-313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 302 - 307, The code currently uses a fixed filename string for valuesFile ("/tmp/vault-values.yaml") which can collide in parallel tests; replace that with a unique temp file created via os.CreateTemp (e.g., f, err := os.CreateTemp("", "vault-values-*.yaml")), then use f.Name() instead of the hardcoded path when writing valuesContent (or write directly to f), close the file (f.Close()) and defer os.Remove(f.Name()) to cleanup; also apply the same change to the other occurrence around valuesFile usage (the later os.Remove call) so both places reference the temp file name returned by CreateTemp.
172-173:⚠️ Potential issue | 🔴 CriticalRemove root token from test logs.
Line 172 logs full admin credentials to CI/test output. This is a direct secret leak.
🔧 Minimal fix
d.t.Logf("HashiCorp Vault Enterprise deployed and configured successfully!") -d.t.Logf("Vault root token: %s", rootToken) d.t.Logf("Vault address: http://%s.%s.svc:8200", WellKnownVaultServiceName, d.config.Namespace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 172 - 173, The test currently logs the Vault root token via d.t.Logf("Vault root token: %s", rootToken) which exposes secrets; remove that log or replace it with a non-secret indicator (e.g., log that a root token was generated without printing its value or print a redacted/hashed form). Update the code around the d.t.Logf calls (the lines referencing rootToken, WellKnownVaultServiceName, and d.config.Namespace) so only non-sensitive info is emitted (keep the Vault address log if needed, but do not print rootToken).
493-500:⚠️ Potential issue | 🟠 MajorUnseal loop hardcodes threshold and can panic on short key slices.
Line 494 assumes exactly 3 keys and directly indexes
unsealKeys[j].🔧 Guard key length and centralize threshold
+const vaultKeyThreshold = 3 ... - for j := 0; j < 3; j++ { + if len(unsealKeys) < vaultKeyThreshold { + return fmt.Errorf("insufficient unseal keys: got %d, need %d", len(unsealKeys), vaultKeyThreshold) + } + for j := 0; j < vaultKeyThreshold; j++ {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 493 - 500, The unseal loop currently hardcodes 3 and indexes unsealKeys[j], which can panic; introduce a central unsealThreshold variable/constant (e.g., unsealThreshold := 3), validate that len(unsealKeys) >= unsealThreshold before the loop and return/log an error if not, and iterate using for j := 0; j < unsealThreshold; j++ so the unsealCmd construction that references unsealKeys[j] is safe; alternatively compute n := min(len(unsealKeys), unsealThreshold) and loop to n if you prefer graceful partial unseal.test/library/encryption/kms/vault_kube_kms_deployer_test.go (1)
358-360:⚠️ Potential issue | 🟠 MajorDo not print full AppRole credentials in test logs.
Line 359 and Line 360 expose reusable credentials in CI logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 358 - 360, The test currently logs full AppRole credentials using t.Logf with roleID and secretID which exposes reusable secrets in CI; change the test to stop printing them (remove or comment out the two t.Logf lines that reference roleID and secretID) or replace them with non-sensitive placeholders/masked output (e.g., show only last 4 chars) and, if needed, write full values to a secure test artifact or secret store instead; update references in the test function where roleID/secretID are logged to use the masked form or no logging.
🧹 Nitpick comments (1)
test/library/encryption/kms/vault_kube_kms_deployer.go (1)
380-393: Readiness gate should check container readiness, not just Pod phase.Returning success on
Runningcan race with init/config commands and increase flakes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 380 - 393, The readiness check currently returns success when pod.Status.Phase == corev1.PodRunning which can race with container init/config steps; update the wait callback used in wait.PollUntilContextTimeout to verify container readiness instead of just Pod phase by inspecting the Pod's readiness condition or container statuses (e.g., look for ConditionType "Ready" with Status == corev1.ConditionTrue or ensure all entries in pod.Status.ContainerStatuses have Ready == true); modify the block that fetches the Pod (the code using d.kubeClient.CoreV1().Pods(...).Get and the return evaluating pod.Status.Phase) to return success only when the Pod is truly ready (use pod.Status.Conditions or pod.Status.ContainerStatuses for the check) while preserving the existing NotFound handling and logging for podName and namespace.
🤖 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/library/encryption/kms/GUIDE.md`:
- Line 65: The markdown file contains multiple fenced code blocks missing
language identifiers (MD040); update each triple-backtick fence to include an
appropriate language token (e.g., ```bash, ```text, ```go, or ```yaml) so the
linter stops flagging them; search for the backtick fences in GUIDE.md and
replace each opening ``` with the correct fenced language for the snippet
content (examples: shell commands -> bash, config -> yaml, plain output -> text,
Go snippets -> go).
- Around line 172-175: Update the GUIDE.md examples (currently showing
real-looking secrets in the vault_kube_kms_deployer_test.go excerpts) to redact
sensitive values — replace literal ROLE_ID, SECRET_ID and any root token strings
with placeholders like <REDACTED_ROLE_ID>, <REDACTED_SECRET_ID>,
<REDACTED_TOKEN> and change the example output blocks accordingly; additionally
add a brief explicit note next to the examples discouraging any plaintext
secret/token logging and instructing readers to use secure secret stores or
masked logging for AppRole credentials and Vault tokens.
- Around line 229-230: The guide's integration test invocation is inconsistent:
it tells readers to run with "-args -integration" while actual gating checks the
environment variable INTEGRATION_TEST=true; update the GUIDE.md text to either
instruct users to export/set INTEGRATION_TEST=true (e.g., export
INTEGRATION_TEST=true && go test ./... -run Test... ) or change the test gating
to honor the "-args -integration" flag; reference and update the occurrences
that mention "-args -integration" and the gating check INTEGRATION_TEST=true so
both the docs and the test gating are consistent.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 525-529: The namespace-delete loop incorrectly treats any error
from kubeClient.CoreV1().Namespaces().Get as successful deletion; change the
check inside the loop (the block around kubeClient.CoreV1().Namespaces().Get and
the deleted boolean) to only set deleted = true and break when
apierrors.IsNotFound(err) is true, otherwise continue retrying on
transient/errors (or log/record the non-notfound error) and sleep briefly
between attempts; import k8s.io/apimachinery/pkg/api/errors as apierrors and
update the logic in the test function surrounding that loop so non-NotFound
errors are not considered successful deletions.
- Around line 328-345: The test currently ignores failures from the oc exec
command and JSON parsing because it only proceeds when err == nil and otherwise
does nothing, which can hide failures; update the block that reads statusOutput
and unmarshals into vaultStatus so that if err from the oc exec call is non-nil
you call t.Fatalf (or t.Fatalf-like) with the error and raw statusOutput, and if
json.Unmarshal returns an error you also fail the test with a clear message
including the unmarshal error and the raw output; use the existing symbols
statusOutput, vaultStatus, and the surrounding error check (the if err == nil)
to locate the code and replace the silent-skipping behavior with explicit
t.Fatalf/t.Errorf on oc exec or JSON parse failures while keeping the subsequent
initialized/sealed assertions.
- Around line 274-278: The test currently calls t.Fatalf after
deployer.Deploy(ctx), which can prevent the later explicit cleanup call from
running; register the cleanup early with t.Cleanup so it always executes on test
termination (even after FailNow). Move the cleanup logic currently executed at
Line 366 into a t.Cleanup(func(){ ... }) registered before calling
deployer.Deploy(ctx) (and likewise replace the other end-of-test explicit
cleanup at 364-368 with t.Cleanup), ensuring the cleanup closure references the
same resources/namespaces and uses the existing deployer/teardown helpers.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 648-649: The test currently logs full command and arguments via
d.t.Logf("Executing in pod %s/%s: %v", d.config.Namespace, podName, command)
which can leak sensitive materials (unseal keys, root tokens, AppRole secrets);
change these logs to avoid printing raw command contents by either removing the
command from the log, replacing it with a non-sensitive summary (e.g.,
"executing vault-related command"), or pass the command through a sanitizer/mask
function that redacts tokens/keys before logging; update all similar usages (the
other d.t.Logf calls around the pod execution path that reference podName,
d.config.Namespace, and command) so no sensitive payloads are emitted.
- Around line 108-113: The constructor NewVaultDeployer currently dereferences
config without checking for nil; update it to guard against a nil *VaultConfig
by returning an error or creating a default VaultConfig when config == nil
before using fields like config.Namespace, ensuring VaultDeployer is constructed
safely; reference the NewVaultDeployer function and the VaultConfig and
VaultDeployer types and set defaults (e.g., WellKnownVaultNamespace) only after
verifying config is non-nil.
---
Duplicate comments:
In `@test/library/encryption/kms/GUIDE.md`:
- Around line 78-79: The guide contains user-specific absolute paths like
"/home/skundu/automation/library-go/test/library/encryption/kms" (and the other
occurrences at the noted locations) which should be replaced with portable
placeholders; update the GUIDE.md entries to use a repo-root or environment
variable placeholder (e.g., <REPO_ROOT> or $PROJECT_ROOT or relative paths like
./test/library/encryption/kms or ~/) and add a brief note explaining how to
substitute the placeholder for local users so the instructions are
environment-agnostic.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 358-360: The test currently logs full AppRole credentials using
t.Logf with roleID and secretID which exposes reusable secrets in CI; change the
test to stop printing them (remove or comment out the two t.Logf lines that
reference roleID and secretID) or replace them with non-sensitive
placeholders/masked output (e.g., show only last 4 chars) and, if needed, write
full values to a secure test artifact or secret store instead; update references
in the test function where roleID/secretID are logged to use the masked form or
no logging.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 286-287: The image repo/tag assembly is brittle where the code
uses strings.Split on d.config.Image; replace that logic with parsing that finds
the last ':' only if it occurs after the last '/' (i.e., use strings.LastIndex
and strings.LastIndexByte or similar) to separate repo and tag safely for images
with registry ports or untagged images; update the expression that builds the
repo and tag from d.config.Image accordingly (refer to the use of d.config.Image
in vault_kube_kms_deployer.go and the surrounding repo/tag assembly) so that if
no tag is present you treat the whole string as the repo and if a ':' is part of
the registry host (before the last '/') you do not split there.
- Around line 443-446: The current branch in the function that checks
d.isVaultInitialized(ctx, podName) must not return fake credentials
("EXISTING_ROOT_TOKEN", ["KEY1",...]) — instead fail fast by returning an
explicit error so callers know Vault is already initialized; modify the block in
the method that calls d.isVaultInitialized to return a non-nil error (e.g.,
ErrVaultAlreadyInitialized or fmt.Errorf with context including podName) and no
fake token/keys, so unseal/config callers receive a clear failure instead of
misleading dummy values.
- Around line 302-307: The code currently uses a fixed filename string for
valuesFile ("/tmp/vault-values.yaml") which can collide in parallel tests;
replace that with a unique temp file created via os.CreateTemp (e.g., f, err :=
os.CreateTemp("", "vault-values-*.yaml")), then use f.Name() instead of the
hardcoded path when writing valuesContent (or write directly to f), close the
file (f.Close()) and defer os.Remove(f.Name()) to cleanup; also apply the same
change to the other occurrence around valuesFile usage (the later os.Remove
call) so both places reference the temp file name returned by CreateTemp.
- Around line 172-173: The test currently logs the Vault root token via
d.t.Logf("Vault root token: %s", rootToken) which exposes secrets; remove that
log or replace it with a non-secret indicator (e.g., log that a root token was
generated without printing its value or print a redacted/hashed form). Update
the code around the d.t.Logf calls (the lines referencing rootToken,
WellKnownVaultServiceName, and d.config.Namespace) so only non-sensitive info is
emitted (keep the Vault address log if needed, but do not print rootToken).
- Around line 493-500: The unseal loop currently hardcodes 3 and indexes
unsealKeys[j], which can panic; introduce a central unsealThreshold
variable/constant (e.g., unsealThreshold := 3), validate that len(unsealKeys) >=
unsealThreshold before the loop and return/log an error if not, and iterate
using for j := 0; j < unsealThreshold; j++ so the unsealCmd construction that
references unsealKeys[j] is safe; alternatively compute n :=
min(len(unsealKeys), unsealThreshold) and loop to n if you prefer graceful
partial unseal.
---
Nitpick comments:
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 380-393: The readiness check currently returns success when
pod.Status.Phase == corev1.PodRunning which can race with container init/config
steps; update the wait callback used in wait.PollUntilContextTimeout to verify
container readiness instead of just Pod phase by inspecting the Pod's readiness
condition or container statuses (e.g., look for ConditionType "Ready" with
Status == corev1.ConditionTrue or ensure all entries in
pod.Status.ContainerStatuses have Ready == true); modify the block that fetches
the Pod (the code using d.kubeClient.CoreV1().Pods(...).Get and the return
evaluating pod.Status.Phase) to return success only when the Pod is truly ready
(use pod.Status.Conditions or pod.Status.ContainerStatuses for the check) while
preserving the existing NotFound handling and logging for podName and namespace.
🪄 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: fa265f17-afd0-4ce3-9a35-b60c3175a479
📒 Files selected for processing (3)
test/library/encryption/kms/GUIDE.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
sandeepknd
force-pushed
the
deploy_vault
branch
3 times, most recently
from
4d952ac to
a19ea1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (12)
test/library/encryption/kms/GUIDE.md (3)
77-79:⚠️ Potential issue | 🟡 MinorReplace user-specific absolute paths with portable placeholders.
These
/home/skundu/...examples are only valid on one workstation and make the guide hard to follow elsewhere.Also applies to: 143-147, 155-158, 191-194, 215-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 77 - 79, Replace user-specific absolute paths like '/home/skundu/automation/library-go/test/library/encryption/kms' in GUIDE.md with portable placeholders (e.g., $REPO_ROOT or ~/path/to/repo) so examples work on any machine; update all occurrences including the other noted locations (around the ranges referenced: 143-147, 155-158, 191-194, 215-218) and ensure the placeholder is documented once near the top of the guide so readers know what to substitute.
65-71:⚠️ Potential issue | 🟡 MinorAdd language identifiers to the fenced code blocks.
Markdown lint is already flagging these fences with MD040.
Also applies to: 116-122, 167-187, 198-211, 228-230, 240-242, 250-252, 269-271, 298-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 65 - 71, The GUIDE.md contains multiple fenced code blocks (for example the file-tree block listing vault_kube_kms_deployer.go, vault_kube_kms_deployer_test.go, vault.hclic, GUIDE.md) that lack language identifiers and trigger MD040; update each fenced block in GUIDE.md to include an appropriate language tag (e.g., ```text or ```bash for file trees/commands, ```yaml or ```json for config snippets, ```sh for shell commands) so the markdown linter stops flagging them.
179-181:⚠️ Potential issue | 🟠 MajorDo not normalize plaintext secret logging in the guide.
The sample output and usage examples still print full
ROLE_ID/SECRET_ID, and the security section explicitly says those credentials are logged. That encourages exactly the behavior we should avoid in CI and local troubleshooting.Also applies to: 363-365, 485-488
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 179 - 181, The guide currently prints full plaintext credentials in its sample output (the blocks that list "ROLE_ID" and "SECRET_ID" with example UUIDs); replace those concrete UUIDs with non-sensitive placeholders or masked values (e.g., "<REDACTED_ROLE_ID>" or "f4ce0bd3-****-****-****-804d01246a4f") and update all similar sample blocks (other occurrences that list ROLE_ID/SECRET_ID) so examples no longer show full secrets but still illustrate format; ensure the text describing that credentials are logged is updated to recommend redaction and not show real values.test/library/encryption/kms/vault_kube_kms_deployer.go (5)
32-35:⚠️ Potential issue | 🟠 MajorVerify the default Vault image tag exists.
If
docker.io/hashicorp/vault-enterprise:2.0.0-entis not a published tag, every deployment will fail withImagePullBackOff. Please pinWellKnownVaultImageto a real Vault Enterprise release and keep the guide/examples aligned with it.Does the container image tag `docker.io/hashicorp/vault-enterprise:2.0.0-ent` exist, and what Vault Enterprise image tags are currently published by HashiCorp?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 32 - 35, The WellKnownVaultImage constant currently points to docker.io/hashicorp/vault-enterprise:2.0.0-ent which may not be a published tag causing ImagePullBackOff; verify available HashiCorp Vault Enterprise tags and update the WellKnownVaultImage constant to a confirmed, published release (e.g., a specific enterprise version tag), and update any accompanying guide/examples to match that confirmed tag so deployments use a real image; locate and change the WellKnownVaultImage symbol in vault_kube_kms_deployer.go and ensure any documentation or tests referencing the old tag are updated accordingly.
108-123:⚠️ Potential issue | 🟠 MajorGuard
configbefore applying defaults.
NewVaultDeployer(..., nil)will panic here before the test gets a usable failure.🔧 Proposed fix
func NewVaultDeployer(t testing.TB, kubeClient kubernetes.Interface, restConfig *rest.Config, config *VaultConfig) *VaultDeployer { t.Helper() + if config == nil { + config = &VaultConfig{} + } // Set defaults if config.Namespace == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 108 - 123, The NewVaultDeployer function should guard against a nil config before applying defaults: check if the config parameter is nil at the start of NewVaultDeployer and call t.Fatalf (or t.Fatal) with a clear error message (e.g., "config is nil") to fail the test early; then proceed with the existing default assignments for config.Namespace, config.Image, config.Replicas, and config.Mode. This ensures NewVaultDeployer will not panic when called with nil and provides a useful test failure.
301-306:⚠️ Potential issue | 🟡 MinorUse a unique temp file for Helm values.
A fixed
/tmp/vault-values.yamlwill collide across parallel runs and can make one test install another test's values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 301 - 306, The code writes Helm values to a fixed path valuesFile := "/tmp/vault-values.yaml" which can collide in parallel runs; replace this with a unique temp file (e.g., use os.CreateTemp or ioutil.TempFile to create a file like "vault-values-*.yaml"), write valuesContent to that temp file (using the temp file's Name()), and defer removal using the temp file's Name() (ensure defer os.Remove(tempName) so cleanup targets the created file). Keep the same error handling around writing (wrap and return fmt.Errorf on failure).
439-445:⚠️ Potential issue | 🔴 CriticalFail fast when Vault is already initialized.
Returning dummy credentials lets the flow continue into
unsealVaultandconfigureVaultForKMSwith invalid auth, which turns a recoverable precondition into a confusing downstream failure.🐛 Proposed fix
if err != nil { // Check if already initialized if d.isVaultInitialized(ctx, podName) { - d.t.Logf("Vault already initialized, skipping initialization") - return "EXISTING_ROOT_TOKEN", []string{"KEY1", "KEY2", "KEY3"}, nil + return "", nil, fmt.Errorf("Vault is already initialized and existing credentials cannot be recovered") } return "", nil, fmt.Errorf("failed to initialize Vault: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 439 - 445, The code currently returns dummy credentials when d.isVaultInitialized(ctx, podName) is true, which lets unsealVault and configureVaultForKMS proceed with invalid auth; instead modify the initialization branch in the function calling d.execInPod so that when d.isVaultInitialized(ctx, podName) returns true it fails fast by returning an error (e.g., wrapped with context like "vault already initialized") rather than "EXISTING_ROOT_TOKEN" and dummy keys; update callers of this function to handle the error path (so unsealVault and configureVaultForKMS are not invoked with invalid credentials) and keep references to d.execInPod and d.isVaultInitialized to locate and change the logic.
614-630:⚠️ Potential issue | 🔴 CriticalStop printing Vault secrets to the test log.
This still emits AppRole credentials directly and logs raw command output from init/unseal/auth flows. Those logs can contain the root token, unseal keys, role IDs, and secret IDs.
Also applies to: 646-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 614 - 630, The test currently prints sensitive Vault credentials (AppRole Role ID and Secret ID) to the test log; stop emitting secrets by removing or redacting the d.t.Logf calls that print roleID and secretID and ensure d.execInPodWithEnv is not logging raw command output; store the values in d.config.AppRoleRoleID and d.config.AppRoleSecretID but never log them (or log only a masked version like "<redacted>" or first/last 4 chars), and apply the same change to the other secret-emitting blocks referenced around the 646-694 region so no raw Vault tokens, unseal keys, role IDs, or secret IDs are written to logs.test/library/encryption/kms/vault_kube_kms_deployer_test.go (4)
325-345:⚠️ Potential issue | 🟠 MajorMake the status check fail when
oc execor JSON parsing fails.Right now both failure modes are silently ignored, so the test can report success without ever proving Vault was initialized or unsealed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 325 - 345, The status check currently ignores failures from exec.CommandContext(statusCmd).Output() and json.Unmarshal(statusOutput, &vaultStatus); update the block around initCheckCmd/statusCmd/statusOutput so that if statusCmd.Output() returns an error you call t.Fatalf (or t.Errorf+return) with the exec error, and if json.Unmarshal fails you also fail the test with the parse error; keep the subsequent checks on vaultStatus.Initialized and vaultStatus.Sealed but ensure they run only after successful output and unmarshal so the test cannot report success when oc exec or JSON parsing failed.
522-526:⚠️ Potential issue | 🟡 MinorOnly treat
NotFoundas successful namespace deletion.This loop currently marks any API error as success, so a transient read failure can make the cleanup test pass while the namespace still exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 522 - 526, The loop currently treats any error from kubeClient.CoreV1().Namespaces().Get as a successful deletion; change it to only mark deleted when the error is a NotFound error by using apierrors.IsNotFound(err) (k8s.io/apimachinery/pkg/api/errors.IsNotFound) and break in that case, while for other non-nil errors continue retrying (or log/record transient errors) so transient read failures don't cause a false-positive deletion; update the logic around the Get call and ensure the appropriate apierrors import is added.
358-360:⚠️ Potential issue | 🟠 MajorRedact the AppRole credentials in test output.
These values end up in CI logs and are usable until cleanup revokes access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 358 - 360, The test currently prints sensitive AppRole credentials via t.Logf using the variables roleID and secretID; instead redact or truncate these values before logging to avoid exposing secrets in CI logs. Modify the t.Logf calls that reference roleID and secretID to either log a constant like "[REDACTED]" or a safely truncated form (e.g., first/last 4 chars with asterisks) so the variables roleID and secretID are never emitted in full to test output.
271-278:⚠️ Potential issue | 🟠 MajorRegister cleanup before deployment so failures do not leak namespaces.
If anything after
Deploycallst.Fatalf, this test leaves the namespace behind. If the intent is to preserve successful deployments for manual inspection, at.Cleanupthat only runs on failure would keep that behavior without leaking failed runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 271 - 278, Register a t.Cleanup before calling deployer.Deploy so namespaces are removed on test failure: create the ctx/cancel as before, then call t.Cleanup(func(){ if t.Failed() { /* call deployer cleanup method, e.g., deployer.Delete(ctx) or deployer.Teardown(ctx) */ } }); then proceed to t.Log and if err := deployer.Deploy(ctx) { t.Fatalf(...) } so a failed test won't leak the namespace.
🤖 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/library/encryption/kms/GUIDE.md`:
- Around line 36-50: The guide incorrectly asserts compatibility with vanilla
Kubernetes but the implementation shells out to oc and binds OpenShift
privileged SCC; update the steps mentioning "Kubernetes Cluster (OpenShift or
vanilla Kubernetes)", the command examples referencing kubectl, and the "oc CLI"
section to scope the guide explicitly to OpenShift only (or else remove any
claims/examples that reference kubectl or generic Kubernetes). Ensure the text
and examples consistently require oc and note the SCC requirement so readers
know OpenShift is mandatory.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 247-285: The Helm values string built in valuesContent ignores the
caller-provided VaultConfig fields; update the generator in
vault_kube_kms_deployer.go to use d.config (VaultConfig) values instead of
hardcoded ones: set dataStorage.size from d.config.StorageSize,
dataStorage.storageClass from d.config.StorageClass (use null when empty), set
standalone.enabled when d.config.Mode == "standalone" and ha.enabled when
d.config.Mode == "ha"/"replicated", and set server.replicas from
d.config.Replicas; keep the existing image repository/tag logic using
d.config.Image but ensure the other fields (size, storageClass,
standalone.enabled, ha.enabled, replicas) are interpolated from d.config so
Deploy respects the public VaultConfig.
- Around line 659-673: The code builds envPrefix by concatenating raw env values
into a shell command (envPrefix, fullCmd) which allows shell metacharacters to
break the command; fix it by properly shell-quoting or escaping each env value
before adding to envPrefix (replace the current envPrefix += fmt.Sprintf("%s=%s
", e.Name, e.Value) with a call that escapes e.Value for POSIX sh), keep using
shellJoin for the command and preserve the heredoc logic for stdin, then pass
the safe fullCmd to ocArgs ("sh", "-c", fullCmd).
---
Duplicate comments:
In `@test/library/encryption/kms/GUIDE.md`:
- Around line 77-79: Replace user-specific absolute paths like
'/home/skundu/automation/library-go/test/library/encryption/kms' in GUIDE.md
with portable placeholders (e.g., $REPO_ROOT or ~/path/to/repo) so examples work
on any machine; update all occurrences including the other noted locations
(around the ranges referenced: 143-147, 155-158, 191-194, 215-218) and ensure
the placeholder is documented once near the top of the guide so readers know
what to substitute.
- Around line 65-71: The GUIDE.md contains multiple fenced code blocks (for
example the file-tree block listing vault_kube_kms_deployer.go,
vault_kube_kms_deployer_test.go, vault.hclic, GUIDE.md) that lack language
identifiers and trigger MD040; update each fenced block in GUIDE.md to include
an appropriate language tag (e.g., ```text or ```bash for file trees/commands,
```yaml or ```json for config snippets, ```sh for shell commands) so the
markdown linter stops flagging them.
- Around line 179-181: The guide currently prints full plaintext credentials in
its sample output (the blocks that list "ROLE_ID" and "SECRET_ID" with example
UUIDs); replace those concrete UUIDs with non-sensitive placeholders or masked
values (e.g., "<REDACTED_ROLE_ID>" or "f4ce0bd3-****-****-****-804d01246a4f")
and update all similar sample blocks (other occurrences that list
ROLE_ID/SECRET_ID) so examples no longer show full secrets but still illustrate
format; ensure the text describing that credentials are logged is updated to
recommend redaction and not show real values.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 325-345: The status check currently ignores failures from
exec.CommandContext(statusCmd).Output() and json.Unmarshal(statusOutput,
&vaultStatus); update the block around initCheckCmd/statusCmd/statusOutput so
that if statusCmd.Output() returns an error you call t.Fatalf (or
t.Errorf+return) with the exec error, and if json.Unmarshal fails you also fail
the test with the parse error; keep the subsequent checks on
vaultStatus.Initialized and vaultStatus.Sealed but ensure they run only after
successful output and unmarshal so the test cannot report success when oc exec
or JSON parsing failed.
- Around line 522-526: The loop currently treats any error from
kubeClient.CoreV1().Namespaces().Get as a successful deletion; change it to only
mark deleted when the error is a NotFound error by using
apierrors.IsNotFound(err) (k8s.io/apimachinery/pkg/api/errors.IsNotFound) and
break in that case, while for other non-nil errors continue retrying (or
log/record transient errors) so transient read failures don't cause a
false-positive deletion; update the logic around the Get call and ensure the
appropriate apierrors import is added.
- Around line 358-360: The test currently prints sensitive AppRole credentials
via t.Logf using the variables roleID and secretID; instead redact or truncate
these values before logging to avoid exposing secrets in CI logs. Modify the
t.Logf calls that reference roleID and secretID to either log a constant like
"[REDACTED]" or a safely truncated form (e.g., first/last 4 chars with
asterisks) so the variables roleID and secretID are never emitted in full to
test output.
- Around line 271-278: Register a t.Cleanup before calling deployer.Deploy so
namespaces are removed on test failure: create the ctx/cancel as before, then
call t.Cleanup(func(){ if t.Failed() { /* call deployer cleanup method, e.g.,
deployer.Delete(ctx) or deployer.Teardown(ctx) */ } }); then proceed to t.Log
and if err := deployer.Deploy(ctx) { t.Fatalf(...) } so a failed test won't leak
the namespace.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 32-35: The WellKnownVaultImage constant currently points to
docker.io/hashicorp/vault-enterprise:2.0.0-ent which may not be a published tag
causing ImagePullBackOff; verify available HashiCorp Vault Enterprise tags and
update the WellKnownVaultImage constant to a confirmed, published release (e.g.,
a specific enterprise version tag), and update any accompanying guide/examples
to match that confirmed tag so deployments use a real image; locate and change
the WellKnownVaultImage symbol in vault_kube_kms_deployer.go and ensure any
documentation or tests referencing the old tag are updated accordingly.
- Around line 108-123: The NewVaultDeployer function should guard against a nil
config before applying defaults: check if the config parameter is nil at the
start of NewVaultDeployer and call t.Fatalf (or t.Fatal) with a clear error
message (e.g., "config is nil") to fail the test early; then proceed with the
existing default assignments for config.Namespace, config.Image,
config.Replicas, and config.Mode. This ensures NewVaultDeployer will not panic
when called with nil and provides a useful test failure.
- Around line 301-306: The code writes Helm values to a fixed path valuesFile :=
"/tmp/vault-values.yaml" which can collide in parallel runs; replace this with a
unique temp file (e.g., use os.CreateTemp or ioutil.TempFile to create a file
like "vault-values-*.yaml"), write valuesContent to that temp file (using the
temp file's Name()), and defer removal using the temp file's Name() (ensure
defer os.Remove(tempName) so cleanup targets the created file). Keep the same
error handling around writing (wrap and return fmt.Errorf on failure).
- Around line 439-445: The code currently returns dummy credentials when
d.isVaultInitialized(ctx, podName) is true, which lets unsealVault and
configureVaultForKMS proceed with invalid auth; instead modify the
initialization branch in the function calling d.execInPod so that when
d.isVaultInitialized(ctx, podName) returns true it fails fast by returning an
error (e.g., wrapped with context like "vault already initialized") rather than
"EXISTING_ROOT_TOKEN" and dummy keys; update callers of this function to handle
the error path (so unsealVault and configureVaultForKMS are not invoked with
invalid credentials) and keep references to d.execInPod and d.isVaultInitialized
to locate and change the logic.
- Around line 614-630: The test currently prints sensitive Vault credentials
(AppRole Role ID and Secret ID) to the test log; stop emitting secrets by
removing or redacting the d.t.Logf calls that print roleID and secretID and
ensure d.execInPodWithEnv is not logging raw command output; store the values in
d.config.AppRoleRoleID and d.config.AppRoleSecretID but never log them (or log
only a masked version like "<redacted>" or first/last 4 chars), and apply the
same change to the other secret-emitting blocks referenced around the 646-694
region so no raw Vault tokens, unseal keys, role IDs, or secret IDs are written
to logs.
🪄 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: e393d53d-7da5-4f53-ac9a-0a1bbe81ab11
📒 Files selected for processing (3)
test/library/encryption/kms/GUIDE.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (16)
test/library/encryption/kms/vault_kube_kms_deployer.go (10)
108-113:⚠️ Potential issue | 🔴 CriticalGuard
configbefore dereferencing in constructor.Line 112 dereferences
configunconditionally;NewVaultDeployer(..., nil)will panic.🔧 Suggested fix
func NewVaultDeployer(t testing.TB, kubeClient kubernetes.Interface, restConfig *rest.Config, config *VaultConfig) *VaultDeployer { t.Helper() + if config == nil { + config = &VaultConfig{} + } // Set defaults if config.Namespace == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 108 - 113, The constructor NewVaultDeployer dereferences config without guarding for nil; add a nil-check at the start of the function (e.g., if config == nil { config = &VaultConfig{} }) before accessing config.Namespace so calling NewVaultDeployer(..., nil) does not panic, then proceed to set defaults and return the *VaultDeployer as before.
301-306:⚠️ Potential issue | 🟡 MinorUse a unique temp file for Helm values.
Lines 302-306 use a fixed
/tmp/vault-values.yaml, which can race in parallel test runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 301 - 306, The code writes Helm values to a fixed path in valuesFile which can race in parallel tests; replace the fixed "/tmp/vault-values.yaml" usage with a unique temp file created via os.CreateTemp (or ioutil.TempFile), write valuesContent into that temp file (using the returned *os.File or calling os.WriteFile on temp.Name()), close the file if necessary, and defer removal using defer os.Remove(temp.Name()); update references to valuesFile to use the temp file name and preserve the intended file permissions when writing.
492-499:⚠️ Potential issue | 🔴 CriticalUnseal loop can panic and is coupled to a magic threshold.
Line 498 indexes
unsealKeys[j]withj < 3and no length guard. If fewer keys are returned, this panics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 492 - 499, The unseal loop in vault_kube_kms_deployer.go indexes unsealKeys with a hardcoded limit (for j := 0; j < 3) which can panic if fewer keys are returned; change it to compute a threshold = min(3, len(unsealKeys)) (or read a vaultThreshold constant) and iterate for j := 0; j < threshold; j++; if len(unsealKeys) < 3 return an explicit error (or fail the test) before the loop so the code doesn't panic and the failure is clear; update references to unsealKeys and unsealCmd accordingly.
647-648:⚠️ Potential issue | 🔴 CriticalCommand/request logging leaks sensitive Vault material.
These logs can print init outputs, unseal flows, and secret-bearing command context to CI logs.
Also applies to: 689-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 647 - 648, The test currently logs full command and output (d.t.Logf("Executing in pod %s/%s: %v", d.config.Namespace, podName, command)), which can leak Vault init/unseal outputs and secrets; change these calls to avoid printing command or output contents—e.g. log only non-sensitive context (d.config.Namespace and podName) or a generic message like "Executing command in pod %s/%s" and remove or redact the 'command' and any captured output; apply the same change to the similar logging calls around the other occurrence (lines 689-694) so no secret-bearing command context is written to CI logs.
615-616:⚠️ Potential issue | 🟠 MajorDo not log AppRole credentials in plaintext.
Lines 615 and 630 expose
role_idandsecret_idin test logs.Also applies to: 629-630
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 615 - 616, Remove plaintext logging of AppRole credentials: stop calling d.t.Logf with the raw roleID and secretID values and either omit those logs entirely or print only redacted forms (e.g., show length or last 4 chars with the rest replaced by asterisks). Update the places that currently log roleID and secretID (calls that pass roleID and secretID to d.t.Logf) to use redaction or remove the log entries so credentials are not exposed in test output.
441-445:⚠️ Potential issue | 🔴 CriticalFail fast when Vault is already initialized; do not return dummy credentials.
Returning
"EXISTING_ROOT_TOKEN"and fake keys causes downstream auth/unseal/configuration failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 441 - 445, The current branch returns dummy credentials when d.isVaultInitialized(ctx, podName) is true, which breaks downstream auth/unseal/config; change the early-exit to fail fast by returning a clear error instead of "EXISTING_ROOT_TOKEN" and fake keys: replace the return of dummy values with an explicit error (e.g., fmt.Errorf or errors.New) signaling "vault already initialized" from the same block that calls d.isVaultInitialized; if real credentials can be retrieved, call the appropriate retrieval method instead of fabricating tokens and ensure callers of this function handle the error path.
659-663:⚠️ Potential issue | 🟠 MajorShell command composition injects unescaped env values.
Line 662 concatenates raw env values into
sh -c; metacharacters can alter command execution.🔒 Suggested fix
for _, e := range env { - envPrefix += fmt.Sprintf("%s=%s ", e.Name, e.Value) + envPrefix += fmt.Sprintf("%s='%s' ", e.Name, escapeForShell(e.Value)) }Also applies to: 668-673
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 659 - 663, The code builds a shell command by concatenating raw env values into envPrefix (using env, stdin, envPrefix) and passing it to sh -c, which allows shell metacharacter injection; instead, stop composing a shell string with unescaped values — either run the command directly via exec.Command with cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", e.Name, e.Value)...) and pass arguments as separate params, or if a shell is mandatory, properly escape/quote each value (e.g., strconv.Quote or a reliable shell-escape library) before concatenation; apply the same fix to the other similar block around lines 668-673.
247-257:⚠️ Potential issue | 🟠 MajorHelm values ignore public
VaultConfigfields.The generated YAML hardcodes storage/mode behavior, so
Replicas,Mode,StorageClass, andStorageSizeare not honored.Also applies to: 271-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 247 - 257, The Helm values string in valuesContent currently hardcodes replicas, mode, storage size and storageClass so VaultConfig fields are ignored; update the code that builds valuesContent to interpolate VaultConfig.Replicas, VaultConfig.Mode, VaultConfig.StorageSize and VaultConfig.StorageClass (handle empty StorageClass -> null) instead of hardcoded "2Gi", "null", and fixed accessMode/replica values, and mirror the same change in the other valuesContent block later in the file so Replicas/Mode/StorageClass/StorageSize are honored when rendering Helm values.
34-34:⚠️ Potential issue | 🟠 MajorVerify
WellKnownVaultImageresolves to a published Vault Enterprise image tag.Line 34 hardcodes a tag that appears problematic and can break default deployments at pull time.
What published tags exist for docker.io/hashicorp/vault-enterprise, and does the tag `2.0.0-ent` exist?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` at line 34, WellKnownVaultImage is hardcoded to a likely-nonexistent enterprise tag ("2.0.0-ent"); verify available published tags for docker.io/hashicorp/vault-enterprise and update WellKnownVaultImage in vault_kube_kms_deployer.go to a valid published tag (or switch to a supported OSS image like docker.io/hashicorp/vault:X.Y.Z) and/or make it configurable via an environment variable so tests won’t break at pull time; specifically, check the registry for the exact tag name and replace the literal in the WellKnownVaultImage constant or add logic to read VAULT_IMAGE from env before falling back to the constant.
285-286:⚠️ Potential issue | 🟠 MajorImage repo/tag extraction is brittle for registry-port and no-tag images.
Current
strings.Split+ index logic can derive wrong repo/tag values and produce invalid Helm values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 285 - 286, The current repo/tag extraction using repeated strings.Split on d.config.Image is brittle (fails for registry:port and no-tag images); replace it with robust parsing: compute lastSlash := strings.LastIndex(d.config.Image, "/") and lastColon := strings.LastIndex(d.config.Image, ":") and treat a tag as present only if lastColon > lastSlash; then set tag := "" if no tag else d.config.Image[lastColon+1:] and repo := d.config.Image[:lastColon] (or repo := d.config.Image if no tag); alternatively import and use docker/distribution/reference.ParseNormalizedNamed to get name and tag—update the expression building the Helm values (the strings.TrimSuffix/strings.Split usage) to use these repo and tag variables instead of the current repeated strings.Split logic.test/library/encryption/kms/vault_kube_kms_deployer_test.go (3)
329-346:⚠️ Potential issue | 🟠 MajorDo not silently skip Vault status verification on command/parse failure.
If
oc execfails (or JSON parsing fails), this block does nothing and the test may pass without validating initialized/unsealed state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 329 - 346, The test currently ignores failures from the oc exec call and JSON parsing (statusOutput and json.Unmarshal into vaultStatus), which can let the test pass without validating Vault state; update the block to explicitly fail the test when err != nil or when json.Unmarshal returns an error (use t.Fatalf or t.Errorf) and include the underlying error and raw statusOutput in the message, and keep the existing initialized/unsealed checks using vaultStatus when parsing succeeds.
359-361:⚠️ Potential issue | 🟡 MinorAvoid logging full AppRole credentials.
Lines 360-361 print raw secret material to test logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 359 - 361, The test currently prints raw AppRole secrets via t.Logf using roleID and secretID; remove or redact these secrets to avoid leaking credentials. Replace the two t.Logf lines that output roleID and secretID with either no logging or a safe redaction (e.g., show only first/last few chars or a hash) so that the variables roleID and secretID are never logged in full by the test.
546-549:⚠️ Potential issue | 🟡 MinorNamespace deletion check should only succeed on NotFound.
Any error currently flips
deleted=true; transient API/RBAC errors can be misreported as successful cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 546 - 549, The current namespace deletion loop sets deleted = true for any Get error; change it to only mark deleted when the error is a NotFound using k8s.io/apimachinery/pkg/api/errors.IsNotFound(err). Specifically, in the polling that calls kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}), set deleted = true and break only when errors.IsNotFound(err); if err == nil keep polling (namespace still exists); for any other error do not flip deleted (optionally log or return the error) so transient API/RBAC errors aren't treated as successful deletion.test/library/encryption/kms/GUIDE.md (3)
215-217:⚠️ Potential issue | 🟠 MajorDocumentation still encourages plaintext credential logging.
Examples and security notes should redact or mask
ROLE_ID/SECRET_IDand explicitly discourage logging secrets.Also applies to: 507-510, 644-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 215 - 217, The docs currently show plaintext AppRole credentials (ROLE_ID and SECRET_ID in the vault_kube_kms_deployer_test.go examples) and implicitly encourage logging them; update the GUIDE.md examples (and the sample lines referencing vault_kube_kms_deployer_test.go) to redact or mask those values (e.g., replace with <REDACTED> or partial masks) and add a brief explicit note advising never to log secrets or store them in plaintext, referencing the symbols ROLE_ID and SECRET_ID so reviewers can find and remove any remaining cleartext occurrences (also apply the change to the other mentioned occurrences around the same file).
36-41:⚠️ Potential issue | 🟠 MajorScope prerequisites to OpenShift (or remove vanilla-K8s claim).
This guide says vanilla Kubernetes is supported, but the deployer/test flow depends on
ocand OpenShift-specific SCC/RBAC behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 36 - 41, The guide currently claims vanilla Kubernetes support but uses OpenShift-specific commands and security constructs; either restrict the prerequisites to OpenShift or remove the vanilla-K8s claim and document differences. Update the checklist and commands (replace or remove the "kubectl version" line and keep only "oc version"), add a clear note in the prerequisites explaining that the deploy/test flow relies on OpenShift SCC and RBAC behavior (mentioning SCC/RBAC) and that vanilla Kubernetes will require alternative steps/configs, or provide those alternative steps if you want to keep vanilla-K8s support.
78-79:⚠️ Potential issue | 🟡 MinorReplace user-specific absolute paths with portable placeholders.
These hardcoded local paths are not reusable across environments.
Also applies to: 170-171, 188-189, 284-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/GUIDE.md` around lines 78 - 79, Replace the hardcoded user-specific absolute path "/home/skundu/automation/library-go/test/library/encryption/kms" in the GUIDE.md examples with a portable placeholder (e.g. <project-root> or ${REPO_ROOT}) and update the other duplicate occurrences noted (around the other cd examples) so instructions work across environments; ensure the placeholder is documented at the top of the guide so readers know how to substitute their local repo path.
🤖 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/library/encryption/kms/GUIDE.md`:
- Around line 344-349: The troubleshooting paragraph in GUIDE.md is
contradictory: it presents an ImagePullBackOff for
"hashicorp/vault-enterprise:2.0.0-ent" then states that the same tag is correct.
Fix by making the example consistent—either show an actually incorrect failing
image (e.g., a truncated or wrong registry) and then provide the correct image
"docker.io/hashicorp/vault-enterprise:2.0.0-ent", or explicitly explain that the
error can occur even when the deployer uses the correct image and instruct
readers to verify cluster access to docker.io; update the lines referencing
"hashicorp/vault-enterprise:2.0.0-ent" and
"docker.io/hashicorp/vault-enterprise:2.0.0-ent" accordingly.
---
Duplicate comments:
In `@test/library/encryption/kms/GUIDE.md`:
- Around line 215-217: The docs currently show plaintext AppRole credentials
(ROLE_ID and SECRET_ID in the vault_kube_kms_deployer_test.go examples) and
implicitly encourage logging them; update the GUIDE.md examples (and the sample
lines referencing vault_kube_kms_deployer_test.go) to redact or mask those
values (e.g., replace with <REDACTED> or partial masks) and add a brief explicit
note advising never to log secrets or store them in plaintext, referencing the
symbols ROLE_ID and SECRET_ID so reviewers can find and remove any remaining
cleartext occurrences (also apply the change to the other mentioned occurrences
around the same file).
- Around line 36-41: The guide currently claims vanilla Kubernetes support but
uses OpenShift-specific commands and security constructs; either restrict the
prerequisites to OpenShift or remove the vanilla-K8s claim and document
differences. Update the checklist and commands (replace or remove the "kubectl
version" line and keep only "oc version"), add a clear note in the prerequisites
explaining that the deploy/test flow relies on OpenShift SCC and RBAC behavior
(mentioning SCC/RBAC) and that vanilla Kubernetes will require alternative
steps/configs, or provide those alternative steps if you want to keep
vanilla-K8s support.
- Around line 78-79: Replace the hardcoded user-specific absolute path
"/home/skundu/automation/library-go/test/library/encryption/kms" in the GUIDE.md
examples with a portable placeholder (e.g. <project-root> or ${REPO_ROOT}) and
update the other duplicate occurrences noted (around the other cd examples) so
instructions work across environments; ensure the placeholder is documented at
the top of the guide so readers know how to substitute their local repo path.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 329-346: The test currently ignores failures from the oc exec call
and JSON parsing (statusOutput and json.Unmarshal into vaultStatus), which can
let the test pass without validating Vault state; update the block to explicitly
fail the test when err != nil or when json.Unmarshal returns an error (use
t.Fatalf or t.Errorf) and include the underlying error and raw statusOutput in
the message, and keep the existing initialized/unsealed checks using vaultStatus
when parsing succeeds.
- Around line 359-361: The test currently prints raw AppRole secrets via t.Logf
using roleID and secretID; remove or redact these secrets to avoid leaking
credentials. Replace the two t.Logf lines that output roleID and secretID with
either no logging or a safe redaction (e.g., show only first/last few chars or a
hash) so that the variables roleID and secretID are never logged in full by the
test.
- Around line 546-549: The current namespace deletion loop sets deleted = true
for any Get error; change it to only mark deleted when the error is a NotFound
using k8s.io/apimachinery/pkg/api/errors.IsNotFound(err). Specifically, in the
polling that calls kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace,
metav1.GetOptions{}), set deleted = true and break only when
errors.IsNotFound(err); if err == nil keep polling (namespace still exists); for
any other error do not flip deleted (optionally log or return the error) so
transient API/RBAC errors aren't treated as successful deletion.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 108-113: The constructor NewVaultDeployer dereferences config
without guarding for nil; add a nil-check at the start of the function (e.g., if
config == nil { config = &VaultConfig{} }) before accessing config.Namespace so
calling NewVaultDeployer(..., nil) does not panic, then proceed to set defaults
and return the *VaultDeployer as before.
- Around line 301-306: The code writes Helm values to a fixed path in valuesFile
which can race in parallel tests; replace the fixed "/tmp/vault-values.yaml"
usage with a unique temp file created via os.CreateTemp (or ioutil.TempFile),
write valuesContent into that temp file (using the returned *os.File or calling
os.WriteFile on temp.Name()), close the file if necessary, and defer removal
using defer os.Remove(temp.Name()); update references to valuesFile to use the
temp file name and preserve the intended file permissions when writing.
- Around line 492-499: The unseal loop in vault_kube_kms_deployer.go indexes
unsealKeys with a hardcoded limit (for j := 0; j < 3) which can panic if fewer
keys are returned; change it to compute a threshold = min(3, len(unsealKeys))
(or read a vaultThreshold constant) and iterate for j := 0; j < threshold; j++;
if len(unsealKeys) < 3 return an explicit error (or fail the test) before the
loop so the code doesn't panic and the failure is clear; update references to
unsealKeys and unsealCmd accordingly.
- Around line 647-648: The test currently logs full command and output
(d.t.Logf("Executing in pod %s/%s: %v", d.config.Namespace, podName, command)),
which can leak Vault init/unseal outputs and secrets; change these calls to
avoid printing command or output contents—e.g. log only non-sensitive context
(d.config.Namespace and podName) or a generic message like "Executing command in
pod %s/%s" and remove or redact the 'command' and any captured output; apply the
same change to the similar logging calls around the other occurrence (lines
689-694) so no secret-bearing command context is written to CI logs.
- Around line 615-616: Remove plaintext logging of AppRole credentials: stop
calling d.t.Logf with the raw roleID and secretID values and either omit those
logs entirely or print only redacted forms (e.g., show length or last 4 chars
with the rest replaced by asterisks). Update the places that currently log
roleID and secretID (calls that pass roleID and secretID to d.t.Logf) to use
redaction or remove the log entries so credentials are not exposed in test
output.
- Around line 441-445: The current branch returns dummy credentials when
d.isVaultInitialized(ctx, podName) is true, which breaks downstream
auth/unseal/config; change the early-exit to fail fast by returning a clear
error instead of "EXISTING_ROOT_TOKEN" and fake keys: replace the return of
dummy values with an explicit error (e.g., fmt.Errorf or errors.New) signaling
"vault already initialized" from the same block that calls d.isVaultInitialized;
if real credentials can be retrieved, call the appropriate retrieval method
instead of fabricating tokens and ensure callers of this function handle the
error path.
- Around line 659-663: The code builds a shell command by concatenating raw env
values into envPrefix (using env, stdin, envPrefix) and passing it to sh -c,
which allows shell metacharacter injection; instead, stop composing a shell
string with unescaped values — either run the command directly via exec.Command
with cmd.Env = append(os.Environ(), fmt.Sprintf("%s=%s", e.Name, e.Value)...)
and pass arguments as separate params, or if a shell is mandatory, properly
escape/quote each value (e.g., strconv.Quote or a reliable shell-escape library)
before concatenation; apply the same fix to the other similar block around lines
668-673.
- Around line 247-257: The Helm values string in valuesContent currently
hardcodes replicas, mode, storage size and storageClass so VaultConfig fields
are ignored; update the code that builds valuesContent to interpolate
VaultConfig.Replicas, VaultConfig.Mode, VaultConfig.StorageSize and
VaultConfig.StorageClass (handle empty StorageClass -> null) instead of
hardcoded "2Gi", "null", and fixed accessMode/replica values, and mirror the
same change in the other valuesContent block later in the file so
Replicas/Mode/StorageClass/StorageSize are honored when rendering Helm values.
- Line 34: WellKnownVaultImage is hardcoded to a likely-nonexistent enterprise
tag ("2.0.0-ent"); verify available published tags for
docker.io/hashicorp/vault-enterprise and update WellKnownVaultImage in
vault_kube_kms_deployer.go to a valid published tag (or switch to a supported
OSS image like docker.io/hashicorp/vault:X.Y.Z) and/or make it configurable via
an environment variable so tests won’t break at pull time; specifically, check
the registry for the exact tag name and replace the literal in the
WellKnownVaultImage constant or add logic to read VAULT_IMAGE from env before
falling back to the constant.
- Around line 285-286: The current repo/tag extraction using repeated
strings.Split on d.config.Image is brittle (fails for registry:port and no-tag
images); replace it with robust parsing: compute lastSlash :=
strings.LastIndex(d.config.Image, "/") and lastColon :=
strings.LastIndex(d.config.Image, ":") and treat a tag as present only if
lastColon > lastSlash; then set tag := "" if no tag else
d.config.Image[lastColon+1:] and repo := d.config.Image[:lastColon] (or repo :=
d.config.Image if no tag); alternatively import and use
docker/distribution/reference.ParseNormalizedNamed to get name and tag—update
the expression building the Helm values (the strings.TrimSuffix/strings.Split
usage) to use these repo and tag variables instead of the current repeated
strings.Split logic.
🪄 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: f264afb8-e053-440b-b3d4-13c7242581d2
📒 Files selected for processing (3)
test/library/encryption/kms/GUIDE.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
sandeepknd
force-pushed
the
deploy_vault
branch
2 times, most recently
from
374fd2d to
be568d8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (13)
test/library/encryption/kms/vault_kube_kms_deployer.go (9)
301-307:⚠️ Potential issue | 🟠 MajorUse a unique temp values file to avoid cross-test collisions.
Line 302 uses a fixed path (
/tmp/vault-values.yaml), which is race-prone in parallel runs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 301 - 307, The temp values file uses a fixed path ("valuesFile" variable) which can collide in parallel tests; update the code that writes values (the block creating and writing valuesFile) to use a unique temp file (e.g., via os.CreateTemp / ioutil.TempFile) so each test gets its own filename, write the valuesContent to that file, ensure you close the file handle before using it, keep the defer os.Remove on the generated filename, and preserve the permission semantics and error wrapping in the existing function (referencing the valuesFile variable and the surrounding write/check/error-return logic).
647-647:⚠️ Potential issue | 🔴 CriticalCommand and output logging may leak Vault secrets.
Raw command/output logging can expose root tokens, unseal keys, and secret IDs.
Also applies to: 689-694
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` at line 647, The test currently logs raw commands and outputs (d.t.Logf("Executing in pod %s/%s: %v", d.config.Namespace, podName, command)) which may leak Vault secrets; update the logging in the pod execution helper (the call site using d.t.Logf with podName, d.config.Namespace and command) to either remove raw command/output logging or redact sensitive tokens/keys before logging (e.g., replace patterns like "vault token", "unseal", "root", "secret_id" or known env var values with "<REDACTED>"); apply the same change to the other occurrences around lines referenced (the other d.t.Logf usages at the pod execution helper and the block at 689-694) so no raw Vault secrets are emitted to test logs.
615-630:⚠️ Potential issue | 🟠 MajorDo not log AppRole credentials in plain text.
Line 615 and Line 630 print
role_idandsecret_iddirectly to test logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 615 - 630, The test currently logs AppRole credentials (roleID and secretID) via d.t.Logf and stores secretID in d.config.AppRoleSecretID; remove or redact those plain-text logs to avoid leaking secrets. Replace the d.t.Logf("AppRole Role ID: %s", roleID) and d.t.Logf("AppRole Secret ID: %s", secretID) calls with either no logging or a redacted/truncated form (e.g., only show last 4 chars or "[REDACTED]") and keep storing the secret in d.config.AppRoleSecretID as needed; ensure any reference to secret-id generated via execInPodWithEnv remains non-printed and that error logs do not include the secret value.
108-113:⚠️ Potential issue | 🟠 MajorGuard against nil
configbefore applying defaults.Line 112 dereferences
configdirectly;NewVaultDeployer(..., nil)will panic.Suggested fix
func NewVaultDeployer(t testing.TB, kubeClient kubernetes.Interface, restConfig *rest.Config, config *VaultConfig) *VaultDeployer { t.Helper() + if config == nil { + config = &VaultConfig{} + } // Set defaults if config.Namespace == "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 108 - 113, NewVaultDeployer currently dereferences config without nil-check; update NewVaultDeployer to guard against a nil *VaultConfig by checking if config == nil and, if so, initializing it to an empty VaultConfig (e.g., config = &VaultConfig{}) before applying defaults like WellKnownVaultNamespace and continuing to construct and return the *VaultDeployer; reference NewVaultDeployer, VaultConfig, WellKnownVaultNamespace, and VaultDeployer when locating and changing the code.
247-257:⚠️ Potential issue | 🟠 MajorHelm values ignore
VaultConfigfields.
StorageSize,StorageClass,Mode, andReplicasare exposed inVaultConfigbut hardcoded in values (size: 2Gi,storageClass: null,standalone.enabled: true,ha.enabled: false).Also applies to: 271-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 247 - 257, The Helm values template stored in valuesContent currently hardcodes storage size, storageClass, accessMode and HA/standalone flags; update the template generation to inject the VaultConfig fields (StorageSize, StorageClass, Mode, Replicas) instead of literals: use VaultConfig.StorageSize for size, VaultConfig.StorageClass (or null when empty) for storageClass, VaultConfig.Mode (or VaultConfig.AccessMode) for accessMode, and map VaultConfig.Replicas to enable HA vs standalone (set standalone.enabled when Replicas==1, ha.enabled when Replicas>1) so valuesContent reflects the VaultConfig passed into the deployer function.
442-445:⚠️ Potential issue | 🔴 CriticalFail fast when Vault is already initialized.
Returning
"EXISTING_ROOT_TOKEN"and dummy keys causes downstream auth/unseal failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 442 - 445, The current branch in d.isVaultInitialized(ctx, podName) returns a fake root token and dummy keys which causes downstream auth/unseal failures; instead fail fast by returning no token/keys and a clear error. Modify the block that calls d.isVaultInitialized(ctx, podName) so it logs that Vault is already initialized and then returns an error (e.g., fmt.Errorf including podName/context) with empty token and nil keys rather than "EXISTING_ROOT_TOKEN" and the fake ["KEY1","KEY2","KEY3"] values.
493-499:⚠️ Potential issue | 🟠 MajorUnseal loop can panic on short key lists.
Line 498 indexes
unsealKeys[j]with a hardcodedj < 3and no length validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 493 - 499, The unseal loop in vault_kube_kms_deployer.go indexes unsealKeys with a hardcoded j < 3 and can panic if unsealKeys has fewer than 3 elements; change the loop in the block that builds unsealCmd (the for j := 0; j < 3; j++ { ... unsealKeys[j] }) to bound j by the actual slice length (e.g., for j := 0; j < 3 && j < len(unsealKeys); j++ or iterate using for j := range unsealKeys and stop after 3) and add a nil/length check before the loop to avoid dereferencing an empty slice.
659-673:⚠️ Potential issue | 🟠 MajorQuote env values before composing
sh -c.Line 662 injects raw env values into a shell command; metacharacters can alter execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 659 - 673, Env values are being injected raw into envPrefix before composing the sh -c command, which allows metacharacters to alter execution; update the loop that builds envPrefix (the envPrefix variable populated from env slice) to shell-quote each env value (and/or use strconv.Quote or a dedicated shellEscape helper) so you emit NAME='value' (with proper escaping of any embedded single quotes) instead of raw values, and keep using shellJoin and the stdin heredoc logic as-is before appending to ocArgs ("sh", "-c", fullCmd).
285-286:⚠️ Potential issue | 🟡 MinorImage repository/tag split is fragile.
The split/trim logic breaks for registries with ports and images without explicit tags.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 285 - 286, The image split logic using strings.Split and strings.TrimSuffix is fragile for registries with ports and images without tags; replace it with a safe split that finds the last '/' and the last ':' and only treats the part after ':' as a tag if that colon occurs after the last '/', otherwise treat the whole string as the repository and use a default tag (e.g., "latest") or no tag as appropriate. Locate code referencing d.config.Image and the current strings.Split/TrimSuffix usage, compute lastSlash := strings.LastIndex(image, "/") and lastColon := strings.LastIndex(image, ":"), then if lastColon > lastSlash set repo = image[:lastColon] and tag = image[lastColon+1:] else set repo = image and tag = "latest" (or nil per project conventions), and use those repo/tag values instead of the current split logic.test/library/encryption/kms/vault_kube_kms_deployer_test.go (3)
546-549:⚠️ Potential issue | 🟠 MajorNamespace deletion check should only treat NotFound as success.
Any API error currently flips
deleted=true, which can mask transient failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 546 - 549, The loop that checks namespace deletion currently sets deleted = true for any error returned by kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}); change it to treat success only when the error is a NotFound; i.e., import k8s.io/apimachinery/pkg/api/errors (apierrors) and replace the unconditional error branch with a conditional that sets deleted = true only if apierrors.IsNotFound(err), otherwise propagate or record the unexpected error (fail the test or continue retrying) so transient API errors don't incorrectly mark the namespace as deleted; update references to kubeClient.CoreV1().Namespaces().Get, deleted, and testNamespace accordingly.
329-346:⚠️ Potential issue | 🟠 MajorDo not silently skip Vault status verification failures.
If
oc execor JSON parsing fails, the test currently continues and can pass falsely.Suggested fix
statusOutput, err := statusCmd.Output() -if err == nil { - var vaultStatus struct { - Initialized bool `json:"initialized"` - Sealed bool `json:"sealed"` - } - if err := json.Unmarshal(statusOutput, &vaultStatus); err == nil { - ... - } -} +if err != nil { + t.Fatalf("Failed to run vault status: %v; output=%s", err, string(statusOutput)) +} +var vaultStatus struct { + Initialized bool `json:"initialized"` + Sealed bool `json:"sealed"` +} +if err := json.Unmarshal(statusOutput, &vaultStatus); err != nil { + t.Fatalf("Failed to parse vault status JSON: %v; output=%s", err, string(statusOutput)) +} +...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 329 - 346, The current check silently ignores failures from the `oc exec` call (when `err != nil`) and from `json.Unmarshal(statusOutput, &vaultStatus)`; update the block that references `statusOutput` and `vaultStatus` so that any `err` from the exec call results in a test failure (e.g., `t.Fatalf` or `t.Fatalf("...: %v", err)`) and any JSON unmarshal error likewise fails the test and logs `statusOutput` for debugging; keep the subsequent initialized/sealed assertions but ensure they run only after successful unmarshal so failures are reported rather than skipped.
359-361:⚠️ Potential issue | 🟡 MinorAvoid logging full AppRole credentials.
Line 360 and Line 361 expose sensitive values in CI logs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 359 - 361, The test currently logs full AppRole credentials via t.Logf for roleID and secretID which exposes sensitive data; remove or redact these full values in the test (vault_kube_kms_deployer_test.go) by replacing the t.Logf calls that reference roleID and secretID with either non-sensitive messages (e.g., "AppRole credentials saved to CI secret store") or masked output (e.g., show only first/last N characters of roleID/secretID) and ensure any storage is done via secure CI secrets/artifacts rather than test logs; update the t.Logf calls that mention ROLE_ID and SECRET_ID accordingly.test/library/encryption/kms/deploy_vault_readme.md (1)
199-200:⚠️ Potential issue | 🟠 MajorDocumentation still promotes printing sensitive credentials.
The guide says credentials are output and shows full sample
ROLE_ID/SECRET_ID; this normalizes secret leakage in logs.Also applies to: 216-217, 644-645
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/deploy_vault_readme.md` around lines 199 - 200, The README currently prints and shows full AppRole credentials (ROLE_ID and SECRET_ID) and instructs outputting them for inspection; remove or redact any example literal ROLE_ID/SECRET_ID values and stop recommending printing secrets to logs or console. Update the "Outputs AppRole credentials" and any sample blocks that display ROLE_ID/SECRET_ID to instead instruct users to store credentials in a secure secret store or CI/CD secret variable, show only masked placeholders (e.g., <REDACTED_ROLE_ID>), and replace guidance to "Leaves deployment running" with a note about cleaning up or using ephemeral, access-controlled inspection methods. Ensure all occurrences of ROLE_ID/SECRET_ID in the document are changed to masked placeholders and guidance points to secure storage and safe handling.
🧹 Nitpick comments (1)
test/library/encryption/kms/deploy_vault_readme.md (1)
78-79: Avoid hardcoded machine-specific absolute paths in docs.These paths are not portable and will fail for most contributors.
Also applies to: 170-171, 188-189, 284-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/deploy_vault_readme.md` around lines 78 - 79, Replace hardcoded absolute paths like the snippet "cd /home/skundu/automation/library-go/test/library/encryption/kms" with a portable alternative (e.g., a relative path from the repo root such as "cd ../../..../test/library/encryption/kms", a placeholder like "<repo-root>/test/library/encryption/kms", or an environment variable like "$REPO_ROOT/test/library/encryption/kms"); update all other occurrences of the same pattern in the document so examples are portable for other contributors and add a brief note telling users how to set the placeholder or navigate from the repository root.
🤖 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/library/encryption/kms/deploy_vault_readme.md`:
- Line 70: The project tree in the README mistakenly references "GUIDE.md"
instead of this file's actual name; update the reference string "GUIDE.md" to
"deploy_vault_readme.md" (or rename the target file to match if intended) so the
project structure accurately reflects the current filename; search for the
literal "GUIDE.md" in this document and replace it with "deploy_vault_readme.md"
and verify links or TOC entries that use the old name are updated accordingly.
- Around line 65-71: The markdown contains unlabelled fenced code blocks (e.g.,
the project tree and several example blocks) that trigger markdownlint MD040;
update each triple-backtick block in the README/GUIDE markdown so it includes an
appropriate language tag (for example ```text for plain output, ```bash for
shell snippets, ```yaml for manifests, or ```log for logs) — search for the
unlabeled ``` blocks (including the project tree listing that shows
vault_kube_kms_deployer.go, vault_kube_kms_deployer_test.go, vault.hclic,
GUIDE.md) and add the correct language identifier to each fence.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 565-569: The test currently treats any error from
kubeClient.RbacV1().ClusterRoleBindings().Get(ctx, clusterRoleBindingName,
metav1.GetOptions{}) as "deleted or doesn't exist"; change the logic to
distinguish a NotFound error from other API errors by using
apierrors.IsNotFound(err) (import k8s.io/apimachinery/pkg/api/errors as
apierrors), log the "deleted/doesn't exist" message only when IsNotFound(err) is
true, and for non-nil/non-NotFound errors log the actual error and fail the test
(t.Fatalf or t.Fatalf-like) so RBAC/API failures are surfaced; keep references
to clusterRoleBindingName and ctx in the updated handling.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 389-392: The wait closure currently returns true when
pod.Status.Phase == corev1.PodRunning which is insufficient; change the
readiness check to ensure the Pod is Running and has a PodReady condition with
Status == corev1.ConditionTrue by inspecting pod.Status.Conditions (i.e., keep
corev1.PodRunning but also verify the condition of type corev1.PodReady is
ConditionTrue) before returning true so commands only run after the container is
ready.
---
Duplicate comments:
In `@test/library/encryption/kms/deploy_vault_readme.md`:
- Around line 199-200: The README currently prints and shows full AppRole
credentials (ROLE_ID and SECRET_ID) and instructs outputting them for
inspection; remove or redact any example literal ROLE_ID/SECRET_ID values and
stop recommending printing secrets to logs or console. Update the "Outputs
AppRole credentials" and any sample blocks that display ROLE_ID/SECRET_ID to
instead instruct users to store credentials in a secure secret store or CI/CD
secret variable, show only masked placeholders (e.g., <REDACTED_ROLE_ID>), and
replace guidance to "Leaves deployment running" with a note about cleaning up or
using ephemeral, access-controlled inspection methods. Ensure all occurrences of
ROLE_ID/SECRET_ID in the document are changed to masked placeholders and
guidance points to secure storage and safe handling.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 546-549: The loop that checks namespace deletion currently sets
deleted = true for any error returned by
kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{});
change it to treat success only when the error is a NotFound; i.e., import
k8s.io/apimachinery/pkg/api/errors (apierrors) and replace the unconditional
error branch with a conditional that sets deleted = true only if
apierrors.IsNotFound(err), otherwise propagate or record the unexpected error
(fail the test or continue retrying) so transient API errors don't incorrectly
mark the namespace as deleted; update references to
kubeClient.CoreV1().Namespaces().Get, deleted, and testNamespace accordingly.
- Around line 329-346: The current check silently ignores failures from the `oc
exec` call (when `err != nil`) and from `json.Unmarshal(statusOutput,
&vaultStatus)`; update the block that references `statusOutput` and
`vaultStatus` so that any `err` from the exec call results in a test failure
(e.g., `t.Fatalf` or `t.Fatalf("...: %v", err)`) and any JSON unmarshal error
likewise fails the test and logs `statusOutput` for debugging; keep the
subsequent initialized/sealed assertions but ensure they run only after
successful unmarshal so failures are reported rather than skipped.
- Around line 359-361: The test currently logs full AppRole credentials via
t.Logf for roleID and secretID which exposes sensitive data; remove or redact
these full values in the test (vault_kube_kms_deployer_test.go) by replacing the
t.Logf calls that reference roleID and secretID with either non-sensitive
messages (e.g., "AppRole credentials saved to CI secret store") or masked output
(e.g., show only first/last N characters of roleID/secretID) and ensure any
storage is done via secure CI secrets/artifacts rather than test logs; update
the t.Logf calls that mention ROLE_ID and SECRET_ID accordingly.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 301-307: The temp values file uses a fixed path ("valuesFile"
variable) which can collide in parallel tests; update the code that writes
values (the block creating and writing valuesFile) to use a unique temp file
(e.g., via os.CreateTemp / ioutil.TempFile) so each test gets its own filename,
write the valuesContent to that file, ensure you close the file handle before
using it, keep the defer os.Remove on the generated filename, and preserve the
permission semantics and error wrapping in the existing function (referencing
the valuesFile variable and the surrounding write/check/error-return logic).
- Line 647: The test currently logs raw commands and outputs
(d.t.Logf("Executing in pod %s/%s: %v", d.config.Namespace, podName, command))
which may leak Vault secrets; update the logging in the pod execution helper
(the call site using d.t.Logf with podName, d.config.Namespace and command) to
either remove raw command/output logging or redact sensitive tokens/keys before
logging (e.g., replace patterns like "vault token", "unseal", "root",
"secret_id" or known env var values with "<REDACTED>"); apply the same change to
the other occurrences around lines referenced (the other d.t.Logf usages at the
pod execution helper and the block at 689-694) so no raw Vault secrets are
emitted to test logs.
- Around line 615-630: The test currently logs AppRole credentials (roleID and
secretID) via d.t.Logf and stores secretID in d.config.AppRoleSecretID; remove
or redact those plain-text logs to avoid leaking secrets. Replace the
d.t.Logf("AppRole Role ID: %s", roleID) and d.t.Logf("AppRole Secret ID: %s",
secretID) calls with either no logging or a redacted/truncated form (e.g., only
show last 4 chars or "[REDACTED]") and keep storing the secret in
d.config.AppRoleSecretID as needed; ensure any reference to secret-id generated
via execInPodWithEnv remains non-printed and that error logs do not include the
secret value.
- Around line 108-113: NewVaultDeployer currently dereferences config without
nil-check; update NewVaultDeployer to guard against a nil *VaultConfig by
checking if config == nil and, if so, initializing it to an empty VaultConfig
(e.g., config = &VaultConfig{}) before applying defaults like
WellKnownVaultNamespace and continuing to construct and return the
*VaultDeployer; reference NewVaultDeployer, VaultConfig,
WellKnownVaultNamespace, and VaultDeployer when locating and changing the code.
- Around line 247-257: The Helm values template stored in valuesContent
currently hardcodes storage size, storageClass, accessMode and HA/standalone
flags; update the template generation to inject the VaultConfig fields
(StorageSize, StorageClass, Mode, Replicas) instead of literals: use
VaultConfig.StorageSize for size, VaultConfig.StorageClass (or null when empty)
for storageClass, VaultConfig.Mode (or VaultConfig.AccessMode) for accessMode,
and map VaultConfig.Replicas to enable HA vs standalone (set standalone.enabled
when Replicas==1, ha.enabled when Replicas>1) so valuesContent reflects the
VaultConfig passed into the deployer function.
- Around line 442-445: The current branch in d.isVaultInitialized(ctx, podName)
returns a fake root token and dummy keys which causes downstream auth/unseal
failures; instead fail fast by returning no token/keys and a clear error. Modify
the block that calls d.isVaultInitialized(ctx, podName) so it logs that Vault is
already initialized and then returns an error (e.g., fmt.Errorf including
podName/context) with empty token and nil keys rather than "EXISTING_ROOT_TOKEN"
and the fake ["KEY1","KEY2","KEY3"] values.
- Around line 493-499: The unseal loop in vault_kube_kms_deployer.go indexes
unsealKeys with a hardcoded j < 3 and can panic if unsealKeys has fewer than 3
elements; change the loop in the block that builds unsealCmd (the for j := 0; j
< 3; j++ { ... unsealKeys[j] }) to bound j by the actual slice length (e.g., for
j := 0; j < 3 && j < len(unsealKeys); j++ or iterate using for j := range
unsealKeys and stop after 3) and add a nil/length check before the loop to avoid
dereferencing an empty slice.
- Around line 659-673: Env values are being injected raw into envPrefix before
composing the sh -c command, which allows metacharacters to alter execution;
update the loop that builds envPrefix (the envPrefix variable populated from env
slice) to shell-quote each env value (and/or use strconv.Quote or a dedicated
shellEscape helper) so you emit NAME='value' (with proper escaping of any
embedded single quotes) instead of raw values, and keep using shellJoin and the
stdin heredoc logic as-is before appending to ocArgs ("sh", "-c", fullCmd).
- Around line 285-286: The image split logic using strings.Split and
strings.TrimSuffix is fragile for registries with ports and images without tags;
replace it with a safe split that finds the last '/' and the last ':' and only
treats the part after ':' as a tag if that colon occurs after the last '/',
otherwise treat the whole string as the repository and use a default tag (e.g.,
"latest") or no tag as appropriate. Locate code referencing d.config.Image and
the current strings.Split/TrimSuffix usage, compute lastSlash :=
strings.LastIndex(image, "/") and lastColon := strings.LastIndex(image, ":"),
then if lastColon > lastSlash set repo = image[:lastColon] and tag =
image[lastColon+1:] else set repo = image and tag = "latest" (or nil per project
conventions), and use those repo/tag values instead of the current split logic.
---
Nitpick comments:
In `@test/library/encryption/kms/deploy_vault_readme.md`:
- Around line 78-79: Replace hardcoded absolute paths like the snippet "cd
/home/skundu/automation/library-go/test/library/encryption/kms" with a portable
alternative (e.g., a relative path from the repo root such as "cd
../../..../test/library/encryption/kms", a placeholder like
"<repo-root>/test/library/encryption/kms", or an environment variable like
"$REPO_ROOT/test/library/encryption/kms"); update all other occurrences of the
same pattern in the document so examples are portable for other contributors and
add a brief note telling users how to set the placeholder or navigate from the
repository root.
🪄 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: 3880f5ca-6c72-4c8e-ad3a-36eb0ad7842e
📒 Files selected for processing (3)
test/library/encryption/kms/deploy_vault_readme.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
sandeepknd
force-pushed
the
deploy_vault
branch
from
be568d8 to
02fd41b
Compare
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
Contributor
|
try to use helic file content because we stored content not file in secret i |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
test/library/encryption/kms/vault_kube_kms_deployer_test.go (1)
526-529:⚠️ Potential issue | 🟡 MinorDifferentiate
NotFoundfrom other API errors during namespace checks.Both checks currently treat any error as “missing/deleted”, which can hide transient API/RBAC failures.
✅ Proposed fix
- ns, err := kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) - if err != nil { - t.Logf(" ⚠ Namespace %s not found (may have been deleted already)", testNamespace) - continue - } + ns, err := kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + t.Logf(" ⚠ Namespace %s not found (may have been deleted already)", testNamespace) + continue + } + t.Errorf(" ✗ Failed to query namespace %s: %v", testNamespace, err) + continue + } ... - _, err = kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) - if err != nil { - deleted = true - break - } + _, err = kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + deleted = true + break + } + t.Logf(" ... namespace check transient error: %v", err) + }Also applies to: 567-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` around lines 526 - 529, When calling kubeClient.CoreV1().Namespaces().Get(ctx, testNamespace, metav1.GetOptions{}) in the test, don't treat every error as "namespace missing"; instead use the k8s API error helper (k8s.io/apimachinery/pkg/api/errors). Replace the unconditional continue branch with a conditional: if apierrors.IsNotFound(err) then t.Logf(... and continue) else fail the test or surface the error (e.g., t.Fatalf or t.Errorf) so transient API/RBAC errors are not hidden; apply the same change to the other occurrence that calls Namespaces().Get for testNamespace.test/library/encryption/kms/vault_kube_kms_deployer.go (1)
283-289:⚠️ Potential issue | 🟠 Major
DeploymentModecurrently has no effect in Helm values.Both branches set
standalone.enabled=trueandha.enabled=false, soDeploymentModeCloudbehaves the same as local in this path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 283 - 289, The Helm values for Vault deployment ignore d.config.Mode because standaloneEnabled and haEnabled are set to the same values in both branches; update the logic in vault_kube_kms_deployer.go where standaloneEnabled and haEnabled are defined so that when d.config.Mode == DeploymentModeCloud you set standaloneEnabled="false" and haEnabled="true" (or otherwise enable HA for cloud), while keeping the local/default branch as standaloneEnabled="true" and haEnabled="false"; ensure you reference the DeploymentModeCloud constant and the variables standaloneEnabled/haEnabled used later when rendering Helm values.
🤖 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/library/encryption/kms/deploy_vault_readme.md`:
- Around line 521-522: The README's default replicas value is incorrect; update
the documentation to match the code which uses DefaultVaultReplicas (3) as the
default in NewVaultDeployer. Change the comment/value for the Replicas field in
the doc from "default: 1" to "default: 3" (or explicitly reference
DefaultVaultReplicas) and ensure the description for Replicas and Mode still
references the types DeploymentMode and the NewVaultDeployer behavior so docs
reflect the actual default used by NewVaultDeployer.
- Around line 216-217: The sample output in deploy_vault_readme.md exposes full
credentials for ROLE_ID and SECRET_ID; update the sample lines that show
vault_kube_kms_deployer_test.go output so these values are redacted or truncated
(e.g. ROLE_ID: f4ce0bd3-2d**-****-****-****d01246a4f or ROLE_ID: f4ce0bd3...a4f,
SECRET_ID: 58fe4d86...124c or SECRET_ID: ********-****-****-****-************),
keeping the surrounding context intact and only replacing the secret string
tokens ROLE_ID and SECRET_ID in the example output.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 681-683: Current test code logs AppRole credentials in plaintext
(roleID/secretID) via d.t.Logf while assigning to
d.config.AppRoleRoleID/d.config.AppRoleSecretID; remove or redact those logs so
sensitive values are not emitted. Locate the uses of roleID and secretID and
either delete the d.t.Logf lines or replace the logged values with a constant
mask (e.g., "[REDACTED]") or a length-only hint, ensuring assignments to
d.config.AppRoleRoleID and d.config.AppRoleSecretID remain but no plaintext
credentials are written to logs.
---
Duplicate comments:
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 526-529: When calling kubeClient.CoreV1().Namespaces().Get(ctx,
testNamespace, metav1.GetOptions{}) in the test, don't treat every error as
"namespace missing"; instead use the k8s API error helper
(k8s.io/apimachinery/pkg/api/errors). Replace the unconditional continue branch
with a conditional: if apierrors.IsNotFound(err) then t.Logf(... and continue)
else fail the test or surface the error (e.g., t.Fatalf or t.Errorf) so
transient API/RBAC errors are not hidden; apply the same change to the other
occurrence that calls Namespaces().Get for testNamespace.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 283-289: The Helm values for Vault deployment ignore d.config.Mode
because standaloneEnabled and haEnabled are set to the same values in both
branches; update the logic in vault_kube_kms_deployer.go where standaloneEnabled
and haEnabled are defined so that when d.config.Mode == DeploymentModeCloud you
set standaloneEnabled="false" and haEnabled="true" (or otherwise enable HA for
cloud), while keeping the local/default branch as standaloneEnabled="true" and
haEnabled="false"; ensure you reference the DeploymentModeCloud constant and the
variables standaloneEnabled/haEnabled used later when rendering Helm values.
🪄 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: 2c1ec926-d63d-4cd4-90bd-3e915b53dee9
📒 Files selected for processing (3)
test/library/encryption/kms/deploy_vault_readme.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
gangwgr
reviewed
Apr 29, 2026
sandeepknd
force-pushed
the
deploy_vault
branch
2 times, most recently
from
493671b to
c6d30db
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
test/library/encryption/kms/vault_kube_kms_deployer.go (3)
170-171: Avoid arbitrarytime.Sleep; prefer polling for readiness.Using
time.Sleep(5 * time.Second)introduces a race condition where Vault may not be ready to accept commands, or wastes time if it's ready sooner. Consider polling Vault's status endpoint instead.♻️ Suggested approach
- // Wait a bit for Vault to be ready to accept commands - time.Sleep(5 * time.Second) + // Wait for Vault to be ready to accept commands + if err := d.waitForVaultReady(ctx); err != nil { + return fmt.Errorf("Vault not ready to accept commands: %w", err) + }Implement
waitForVaultReadyto pollvault statusuntil it returns without connection errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 170 - 171, Replace the arbitrary time.Sleep(5 * time.Second) by implementing and calling waitForVaultReady that polls Vault's status via the "vault status" command (or HTTP status endpoint) until it reports ready or a timeout elapses; use a context with deadline, retry loop with short backoff, treat transient connection errors as retryable, and return an error if timeout is reached. Remove the Sleep call and invoke waitForVaultReady from the same deployment flow (where time.Sleep is used) so the deployer only proceeds when vault status indicates readiness.
543-546: Differentiate "already exists" errors from other failures.Currently, all errors from enabling transit engine are logged and ignored. This could mask permission errors, network failures, or other critical issues. Consider checking for specific "already enabled" error patterns.
♻️ Suggested approach
if _, _, err := d.execInPodWithEnv(ctx, podName, "vault", enableTransitCmd, env); err != nil { - // Ignore error if already enabled - d.t.Logf("Transit engine may already be enabled: %v", err) + // Only ignore "path is already in use" errors + if !strings.Contains(err.Error(), "path is already in use") { + return fmt.Errorf("failed to enable transit engine: %w", err) + } + d.t.Logf("Transit engine already enabled, continuing") }This same pattern applies to Lines 556-558 (transit key) and Lines 569-571 (AppRole auth).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 543 - 546, The current code calls d.execInPodWithEnv(ctx, podName, "vault", enableTransitCmd, env) and unconditionally ignores any error by logging "Transit engine may already be enabled"; change this to inspect the returned err and only swallow it when it matches the Vault "already enabled"/"already exists" pattern (e.g., contains "already enabled" or the known API error string); for any other error, propagate or fail the test (return the err or call d.t.Fatalf/d.t.Errorf) so permission/network/etc. failures are not masked. Apply the same fix for the transit key and AppRole auth calls (the other execInPodWithEnv usages noted in the comment) so each checks error content before deciding to ignore versus fail.
767-778:shellJoindoesn't escape all shell metacharacters.The function only handles spaces and single quotes. Characters like
$,`,\,",!, etc. could still cause shell injection or unexpected behavior in edge cases.♻️ Consider a more robust approach
For test utilities, the current implementation may be sufficient, but consider using
%qformatting or always single-quoting all arguments:func shellJoin(args []string) string { quoted := make([]string, len(args)) for i, arg := range args { - if strings.Contains(arg, " ") || strings.Contains(arg, "'") { - quoted[i] = fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\\''")) - } else { - quoted[i] = arg - } + // Always quote to be safe + quoted[i] = fmt.Sprintf("'%s'", strings.ReplaceAll(arg, "'", "'\\''")) } return strings.Join(quoted, " ") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer.go` around lines 767 - 778, shellJoin currently only handles spaces and single quotes and can still allow shell metacharacters to cause injection; replace its ad-hoc logic in the shellJoin function with a robust quoting approach such as using strconv.Quote (or fmt.Sprintf("%q")) on each argument to produce a safely quoted representation and then strings.Join the results, ensuring all characters like $, `, \, ", !, etc. are escaped consistently instead of only handling spaces and single quotes.test/library/encryption/kms/vault_kube_kms_deployer_test.go (1)
113-113: Minor: Use slice literal with capacity for efficiency.
append(vaultSharedManifestFiles, vaultDeploymentManifestFile)creates a new slice. For clarity and slight efficiency, consider pre-allocating.♻️ Suggested improvement
- allManifests := append(vaultSharedManifestFiles, vaultDeploymentManifestFile) + allManifests := make([]string, 0, len(vaultSharedManifestFiles)+1) + allManifests = append(allManifests, vaultSharedManifestFiles...) + allManifests = append(allManifests, vaultDeploymentManifestFile)Or simply:
allManifests := append([]string{}, vaultSharedManifestFiles...) allManifests = append(allManifests, vaultDeploymentManifestFile)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go` at line 113, Replace the direct append call that allocates a new slice for allManifests by preallocating capacity and copying the shared files first: create a slice with make([]string, 0, len(vaultSharedManifestFiles)+1), append vaultSharedManifestFiles... into it, then append vaultDeploymentManifestFile; this targets the allManifests assignment that currently uses append(vaultSharedManifestFiles, vaultDeploymentManifestFile) to avoid an unnecessary allocation.
🤖 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/library/encryption/kms/deploy_vault_readme.md`:
- Around line 77-79: Replace hardcoded, user-specific absolute paths in
deploy_vault_readme.md (e.g., the cd command instances that use
"/home/skundu/automation/library-go/...") with portable alternatives; update the
occurrences referenced in the diff (the cd lines around the current examples and
the similar lines at the other two locations) to use relative paths like "cd
test/library/encryption/kms" or a placeholder such as
"${REPO_ROOT}/test/library/encryption/kms" so the README is not tied to a
specific user's home directory.
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Around line 516-522: The test is checking for a StatefulSet but the deployer
actually creates a Deployment; update the check in the test (where
kubeClient.AppsV1().StatefulSets(testNamespace).Get(...) is used) to query the
Deployments API instead
(kubeClient.AppsV1().Deployments(testNamespace).Get(...)) for the "vault"
resource name (testNamespace and "vault" stay the same) so the test inspects the
correct resource created by the deployer (see vault_kube_kms_deployer.go where
the Deployment is created).
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 974-979: The template references a PVC named "vault-data" in cloud
mode but no PVC manifest is provided; add a new manifest entry "vault_pvc.yaml"
to the vaultSharedManifestFiles slice and add a corresponding template entry in
getVaultManifestTemplate that emits a PersistentVolumeClaim named "vault-data"
when .Mode == "cloud" (include .Namespace, optional .StorageClass, and
.StorageSize in the spec); this ensures the claim referenced by the Deployment's
persistentVolumeClaim: claimName: vault-data exists in cloud mode.
---
Nitpick comments:
In `@test/library/encryption/kms/vault_kube_kms_deployer_test.go`:
- Line 113: Replace the direct append call that allocates a new slice for
allManifests by preallocating capacity and copying the shared files first:
create a slice with make([]string, 0, len(vaultSharedManifestFiles)+1), append
vaultSharedManifestFiles... into it, then append vaultDeploymentManifestFile;
this targets the allManifests assignment that currently uses
append(vaultSharedManifestFiles, vaultDeploymentManifestFile) to avoid an
unnecessary allocation.
In `@test/library/encryption/kms/vault_kube_kms_deployer.go`:
- Around line 170-171: Replace the arbitrary time.Sleep(5 * time.Second) by
implementing and calling waitForVaultReady that polls Vault's status via the
"vault status" command (or HTTP status endpoint) until it reports ready or a
timeout elapses; use a context with deadline, retry loop with short backoff,
treat transient connection errors as retryable, and return an error if timeout
is reached. Remove the Sleep call and invoke waitForVaultReady from the same
deployment flow (where time.Sleep is used) so the deployer only proceeds when
vault status indicates readiness.
- Around line 543-546: The current code calls d.execInPodWithEnv(ctx, podName,
"vault", enableTransitCmd, env) and unconditionally ignores any error by logging
"Transit engine may already be enabled"; change this to inspect the returned err
and only swallow it when it matches the Vault "already enabled"/"already exists"
pattern (e.g., contains "already enabled" or the known API error string); for
any other error, propagate or fail the test (return the err or call
d.t.Fatalf/d.t.Errorf) so permission/network/etc. failures are not masked. Apply
the same fix for the transit key and AppRole auth calls (the other
execInPodWithEnv usages noted in the comment) so each checks error content
before deciding to ignore versus fail.
- Around line 767-778: shellJoin currently only handles spaces and single quotes
and can still allow shell metacharacters to cause injection; replace its ad-hoc
logic in the shellJoin function with a robust quoting approach such as using
strconv.Quote (or fmt.Sprintf("%q")) on each argument to produce a safely quoted
representation and then strings.Join the results, ensuring all characters like
$, `, \, ", !, etc. are escaped consistently instead of only handling spaces and
single quotes.
🪄 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: 2b37e46d-6193-4001-8292-2703029069d1
📒 Files selected for processing (3)
test/library/encryption/kms/deploy_vault_readme.mdtest/library/encryption/kms/vault_kube_kms_deployer.gotest/library/encryption/kms/vault_kube_kms_deployer_test.go
ardaguclu
reviewed
May 4, 2026
| // Run: | ||
| // | ||
| // INTEGRATION_TEST=true VAULT_LICENSE="$(cat vault.hclic)" go test -v -timeout 15m -run TestVaultDeployerIntegration ./test/library/encryption/kms/... | ||
| func TestVaultDeployerIntegration(t *testing.T) { |
Member
There was a problem hiding this comment.
Do we really need to have a unit test for a test deployment package?
Author
There was a problem hiding this comment.
Would you suggest to remove the vault_kube_kms_deployer_test.go file itself ?
sandeepknd
force-pushed
the
deploy_vault
branch
5 times, most recently
from
eff7879 to
953b3b7
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ardaguclu
reviewed
May 5, 2026
| sleep 2 | ||
| done | ||
|
|
||
| if vault status 2>&1 | grep -q 'Initialized.*true'; then |
Member
There was a problem hiding this comment.
In line:32, we almost do the same. In line:32, we wait for the Vault to be initialized. But after executing the same command here, we decide that Vault initialized and skip setup. I'm not sure I understand the difference between two?
Author
There was a problem hiding this comment.
This is mainly for handling the re-deployment or restart of vault pod or trying to re-initialize an already-initialized Vault pod. The condition check appears to be redundant; actually the issue arises due to two sidecar containers (vault binary and vault-script) being launched simultaneously. In situations when the vault-setup script starts executing the vault commands (eg: check the vault status) even before the vault binary is installed, it would fail.
So, here either the vault-script needs to explicitly wait for few seconds (use sleep hardcoded time in seconds)
before executing any vault command
or
need to wait/poll and check if vault is responding - initialized field might show up but status might still be empty
or
initialized field might show up with status true/false.
@ardaguclu ,
please share your thoughts.
| fi | ||
|
|
||
| echo "Initializing Vault..." | ||
| INIT_OUTPUT=$(vault operator init \ |
Member
There was a problem hiding this comment.
Why just not;
Suggested change
| INIT_OUTPUT=$(vault operator init \ | |
| INIT_OUTPUT=$(vault operator init -key-shares=1 -key-threshold=1 -format=json |
| exit 1 | ||
| } | ||
|
|
||
| UNSEAL_KEY=$(echo "$INIT_OUTPUT" | tr -d ' \n\t' | sed -n 's/.*"unseal_keys_b64":\["\([^"]*\)".*/\1/p') |
Member
There was a problem hiding this comment.
We init keys using -format=json. Why can't we use jq instead of this?
Author
There was a problem hiding this comment.
neither 'jq' nor 'oc' comes pre-installed with hasicorp vault image.
~ $
~ $ jq
/bin/sh: jq: not found
~ $ oc
/bin/sh: oc: not found
~ $
| } | ||
|
|
||
| UNSEAL_KEY=$(echo "$INIT_OUTPUT" | tr -d ' \n\t' | sed -n 's/.*"unseal_keys_b64":\["\([^"]*\)".*/\1/p') | ||
| ROOT_TOKEN=$(echo "$INIT_OUTPUT" | tr -d ' \n\t' | sed -n 's/.*"root_token":"\([^"]*\)".*/\1/p') |
| fi | ||
|
|
||
| echo "Unsealing Vault..." | ||
| vault operator unseal "$UNSEAL_KEY" |
Member
There was a problem hiding this comment.
Suggested change
| vault operator unseal "$UNSEAL_KEY" | |
| vault operator unseal "${UNSEAL_KEY}" |
|
|
||
| echo "Creating vault-credentials secret via K8s API..." | ||
| SA_TOKEN=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token) | ||
| K8S_API="https://${KUBERNETES_SERVICE_HOST}:${KUBERNETES_SERVICE_PORT}" |
Member
There was a problem hiding this comment.
Wouldn't be simpler to install oc and use it (i.e. oc create secret)?
| } | ||
| EOSECRET | ||
|
|
||
| wget --header="Authorization: Bearer ${SA_TOKEN}" \ |
Member
There was a problem hiding this comment.
I think we can install oc in vault image. That would save us multiple lines of code.
| // Run: | ||
| // | ||
| // INTEGRATION_TEST=true VAULT_LICENSE="$(cat vault.hclic)" go test -v -timeout 15m -run TestVaultDeployerIntegration ./test/library/encryption/kms/... | ||
| func TestVaultDeployerIntegration(t *testing.T) { |
tjungblu
reviewed
May 5, 2026
Comment on lines
+58
to
+67
| - name: vault-setup | ||
| image: {{ .Image }} | ||
| imagePullPolicy: IfNotPresent | ||
| command: ["/bin/sh", "/vault/config/setup.sh"] | ||
| env: | ||
| - name: VAULT_ADDR | ||
| value: "http://127.0.0.1:8200" | ||
| volumeMounts: | ||
| - name: config | ||
| mountPath: /vault/config |
Contributor
There was a problem hiding this comment.
why not make it an init container? avoids the sleep statements in your setup script too
Author
There was a problem hiding this comment.
Currently, we are having two containers - vault binary (installation with enterprise licesnse) and the vault-setup.
Initconatiner will get completed even before the main container starts.
We need to keep the vault binary container running and configuring the vault (vault-setup.sh) can be done post that.
Contributor
There was a problem hiding this comment.
I suggest you go checkout how the helm chart works:
https://github.com/hashicorp/vault-helm/tree/main
because that doesn't need any of that, you just point the license to it and it is deployed, I don't see any operator code necessary to setup any of this, but like I mentioned earlier, haven't been using this in a long time.
having a sidecar container that is dependent on another container is something we should avoid and also the sleep statements are an indicator to me that this isn't quite right ;-)
also from a common sense perspective: you setup something, then you run it. Why would you run something and then setup?
Author
There was a problem hiding this comment.
"also from a common sense perspective: you setup something, then you run it. Why would you run something and then setup?"
I think its a bit confusing here :)
we are
installing vault and then configuring it.
"having a sidecar container that is dependent on another container is something we should avoid and also the sleep statements are an indicator to me that this isn't quite right ;-)"
maybe @ardaguclu would like your inputs here as well.
Author
There was a problem hiding this comment.
@gangwgr
#2189 (comment),
so the init and unseal part can be executed under the init container.
How about the vault main binary ? How is it executed ?
Will it continue to get executed as the main container in the vault pod.
So, once again comes the chicken egg situation, the init container will try to configure the vault even before the vault binary is installed.
Is my understanding correct ?
Contributor
There was a problem hiding this comment.
Do we need to have this in library-go btw? seems a post-install script in the release repo would be good enough. Seems there are some helm installs there already, too:
https://github.com/search?q=repo%3Aopenshift%2Frelease+%22helm+install%22&type=code
maybe the more relevant question for this would be: do we need to deploy those per-testcase, do we need multiple instances in different namespaces (likely with migrations?).
Contributor
There was a problem hiding this comment.
that's a good question, Yes I prefer in release repo there it is easy to handle.
@p0lyn0mial can we move to release repo or keep it here?
Author
There was a problem hiding this comment.
@tjungblu ,
#2189 (comment)
your thoughts on this;
even if we deploy through helm, eventually both the vault server (binary) and the setup script needs to be executed synchronously (server at first then configuration script). How to achieve this polling/waiting without sleep ?
Even if run both in the same container, we still need to wait/poll for the vault server to be ready, to be able to configure it.
#2189 (comment)
even if we run it by a job,
vault container → starts, takes ~2-5s to be ready
Job (separate pod): vault-setup → polls until vault service ready. polling is still needed to synchronize.
Just trying to understand. :)
Contributor
There was a problem hiding this comment.
that's a good question, Yes I prefer in release repo there it is easy to handle.
@p0lyn0mial can we move to release repo or keep it here?
If we could create a reusable custom workflow that works across multiple jobs, then we could consider it.
In general, though, I would prefer to implement it in go.
-
The test cases are much more explicit and easier to understand when reading the code.
It is also easier to modify. -
Writing validation functions would be simpler as well especially since I’m pretty sure we’ll need code to verify the status of the Vault service.
-
It would also make it easier to support Vault to Vault migration scenarios, which would require multiple Vault instances.
-
In the future we will also have a function that will force a key rotation.
Does that make sense?
Member
|
/retitle CNTRLPLANE-3364: utility for vault deployer |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sandeepknd: This pull request references CNTRLPLANE-3364 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. |
p0lyn0mial
reviewed
May 6, 2026
| } | ||
|
|
||
| // NewVaultDeployer creates a new VaultDeployer with the given configuration. | ||
| func NewVaultDeployer(t testing.TB, kubeClient kubernetes.Interface, config *VaultConfig) *VaultDeployer { |
Contributor
There was a problem hiding this comment.
what happens when two independent tests provide the same configuration ? will the service be installed only once ?
p0lyn0mial
reviewed
May 6, 2026
| // VaultDeployer manages the deployment and lifecycle of HashiCorp Vault Enterprise | ||
| // for KMS encryption testing. All init/unseal/configure logic runs inside the pod | ||
| // as an init container; the Go deployer only applies manifests and reads results. | ||
| type VaultDeployer struct { |
Contributor
There was a problem hiding this comment.
what is the purpose of this struct ? do we need it ?
Contributor
There was a problem hiding this comment.
if not then maybe we could do something similar to
?
Contributor
|
@sandeepknd: 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. |
p0lyn0mial
reviewed
May 6, 2026
|
|
||
| // readCredentials reads the vault-credentials Secret created by the init | ||
| // container and populates the VaultConfig with AppRole credentials. | ||
| func (d *VaultDeployer) readCredentials(ctx context.Context) error { |
Contributor
There was a problem hiding this comment.
what is the purpose of this function ?
p0lyn0mial
reviewed
May 6, 2026
| namespace: {{ .Namespace }} | ||
| data: | ||
| vault.hcl: | | ||
| cluster_addr = "http://POD_IP:8201" |
Contributor
There was a problem hiding this comment.
have we tried installing the vault without setting this option ?
p0lyn0mial
reviewed
May 6, 2026
| data: | ||
| vault.hcl: | | ||
| cluster_addr = "http://POD_IP:8201" | ||
| api_addr = "http://POD_IP:8200" |
Contributor
There was a problem hiding this comment.
have we tried installing the vault without setting this option ?
p0lyn0mial
reviewed
May 6, 2026
|
|
||
| disable_mlock = true | ||
|
|
||
| setup.sh: | |
Contributor
There was a problem hiding this comment.
Is there an official HashiCorp Vault go client we could use for setup ?
Contributor
There was a problem hiding this comment.
experimented a little bit at https://github.com/p0lyn0mial/library-go/commits/deploy_vault-review/
especially p0lyn0mial@fefae70
Contributor
There was a problem hiding this comment.
the port-forward hack is what we already use for talking with etcd
xref:
Contributor
There was a problem hiding this comment.
In the future we could use the client to force a key rotation, client.Logical().WriteWithContext(ctx, "transit/keys/kubernetes-encryption-key/rotate", nil)
Contributor
There was a problem hiding this comment.
the drawback is that we pull in a new dep just for testing ....
Contributor
There was a problem hiding this comment.
yeah I raised this on slack already, because the client makes this a lot easier, but the additional test dependency sucks
Contributor
|
The |
Contributor
|
I am okay with splitting this PR into smaller ones if that's easier. |
Author
|
Raised a new PR openshift/release#78947 for the CI implementation. |
Author
|
closing this PR since the other one is the working one as per the most recent implementation approac. |
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
7 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.
Introduces a Kubernetes test deployer for local HashiCorp Vault Enterprise deployment, init/unseal, transit AppRole KMS configuration, unit and optional integration tests.
Summary by CodeRabbit