CNTRLPLANE-3364: ci implementation for vault deployment#78947
CNTRLPLANE-3364: ci implementation for vault deployment#78947openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
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. |
|
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 new ci-operator step and script to install/configure Vault Enterprise via Helm for etcd-encryption KMS e2e, adds OWNERS/metadata for the step, and wires the step into the ChangesVault Installation for KMS E2E Testing
Sequence Diagram(s)sequenceDiagram
autonumber
participant CI as CI Job
participant Script as Vault Install Script
participant Helm as Helm
participant K8s as Kubernetes API
participant VaultPod as Vault Pod
participant E2E as e2e Test Job
CI->>Script: run etcd-encryption-vault-install
Script->>Helm: add repo and install Vault chart
Helm->>K8s: create namespace and resources
K8s->>VaultPod: schedule Vault pods
Script->>VaultPod: wait ready and run status
Script->>VaultPod: init/unseal, enable transit, create key and AppRole
Script->>K8s: create vault-credentials secret
CI->>E2E: start openshift-e2e-test after vault-install
E2E->>VaultPod: use credentials to run KMS tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 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 |
|
@sandeepknd, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
| readOnly: true | ||
|
|
||
| injector: | ||
| enabled: false |
There was a problem hiding this comment.
by default, it is enbled.
It denotes the Helm chart would deploy:
- vault-agent-injector Deployment (separate pod)
- MutatingWebhookConfiguration (webhook server)
- Service for the webhook
- Additional RBAC resources
we don't need any of these.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/etcd-encryption/vault-install/vault-install-commands.sh`:
- Around line 15-31: The preflight uses helm (helm list and helm -n checks)
before ensuring Helm is present, so guard or install Helm first: ensure command
-v helm is checked and, if missing, perform the Helm bootstrap (the block that
sets HELM_VERSION and downloads helm) before any use of helm, or wrap the
installed-release check (the helm list / INSTALLED_VERSION extraction lines)
with a command -v helm guard; update the logic around VAULT_NAMESPACE/helm list
and the INSTALLED_VERSION extraction to only run after Helm is confirmed
available.
- Around line 176-253: The current if uses a piped `oc exec ... vault status |
grep -q 'Initialized.*false'` which treats any `oc exec` failure as "already
initialized"; change it to first run the status command into a variable or
capture its exit code (e.g., STATUS_OUT=$(oc exec vault-0 -n
"${VAULT_NAMESPACE}" -- vault status 2>&1) || { echo "vault status failed:
$STATUS_OUT"; exit 1; }) and then test the content with grep (grep -q
'Initialized.*false' <<<"$STATUS_OUT") so you fail fast on `oc exec` failures
and only branch on the parsed status. Use the same VAULT_NAMESPACE/vault-0
references from the existing check.
- Around line 31-37: The script currently downloads Helm with curl (the curl
command that writes /tmp/helm.tar.gz using HELM_VERSION) and extracts it without
integrity checks; update the flow to also download the corresponding SHA256
checksum (from get.helm.sh for the same HELM_VERSION) and verify the archive
before extraction — e.g., fetch the .sha256 (or checksums file), validate
/tmp/helm.tar.gz with sha256sum -c (or compare computed sha256sum) and exit
non‑zero on mismatch, then proceed to tar -xzf, mv, chmod, etc.; ensure the
verification step occurs between the curl download and the tar extraction so a
failed checksum prevents execution of the downloaded Helm binary.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/vault-install-ref.yaml`:
- Line 40: Update the documentation entry that currently states "Creates
vault-license secret from VAULT_LICENSE env var (if provided)" to reflect the
actual input mechanism: the step reads the license from a mounted file at
/var/run/vault-license/license rather than from the VAULT_LICENSE environment
variable; reference the actual file path (/var/run/vault-license/license) and
the secret name vault-license in the docs so users mount the secret correctly
for the vault-install step (vault-install-ref.yaml).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f2e8d297-f13d-42be-a402-dfe9cba705d8
📒 Files selected for processing (3)
ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yamlci-operator/step-registry/etcd-encryption/vault-install/vault-install-commands.shci-operator/step-registry/etcd-encryption/vault-install/vault-install-ref.yaml
| curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz" -o /tmp/helm.tar.gz | ||
| tar -xzf /tmp/helm.tar.gz -C /tmp | ||
| mkdir -p /tmp/bin | ||
| mv /tmp/linux-amd64/helm /tmp/bin/helm | ||
| chmod +x /tmp/bin/helm | ||
| export PATH="/tmp/bin:$PATH" | ||
| rm -rf /tmp/helm.tar.gz /tmp/linux-amd64 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "ci-operator/step-registry/etcd-encryption/vault-install/vault-install-commands.sh" | sed -n '25,45p'Repository: openshift/release
Length of output: 769
🌐 Web query:
Helm download integrity verification SHA256 checksum get.helm.sh
💡 Result:
To verify the integrity of Helm binaries downloaded from get.helm.sh using SHA256 checksums, follow these steps based on official documentation and practices [1][2]. 1. Download the binary from get.helm.sh, e.g., for Linux amd64 latest version (check https://github.com/helm/helm/releases for current version, e.g., v3.20.2 as of April 2026 [3][4]): curl -LO https://get.helm.sh/helm-v3.20.2-linux-amd64.tar.gz [5][6] 2. Download the corresponding SHA256 checksum file: curl -LO https://get.helm.sh/helm-v3.20.2-linux-amd64.tar.gz.sha256 [5][6] 3. Verify the checksum (the.sha256 file contains the raw hex digest): sha256sum -c helm-v3.20.2-linux-amd64.tar.gz.sha256sum # or use.sha256sum file if preferred [7] Or manually: grep equivalent: computed=$(sha256sum helm-v3.20.2-linux-amd64.tar.gz | cut -d' ' -f1); expected=$(cat helm-v3.20.2-linux-amd64.tar.gz.sha256); if [ "$computed" = "$expected" ]; then echo OK; else echo FAIL; fi [1][5] The official install scripts (get-helm-3 or get-helm-4) automatically download from get.helm.sh and verify the SHA256 checksum before unpacking [5][6][2]. Example: curl -fsSL -o get_helm.sh https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 chmod 700 get_helm.sh./get_helm.sh For full supply chain security, also verify GPG signatures (.asc files from GitHub releases) using Helm's KEYS file [8][9][10]. Note: Chart provenance verification uses separate 'helm verify' for.prov files, not binary downloads [11][12].
Citations:
- 1: https://helm.sh/docs/intro/install/
- 2: https://helm.sh/docs/intro/install
- 3: https://github.com/helm/helm/releases/tag/v3.20.2
- 4: https://github.com/helm/helm/releases
- 5: https://raw.githubusercontent.com/helm/helm/main/scripts/get
- 6: https://github.com/helm/helm/blob/main/scripts/get-helm-4
- 7: helm/helm@8e9c62b
- 8: Binary releases content trust helm/helm#7838
- 9: Add GPG signature verification to install script helm/helm#7944
- 10: GPG signature verification fails for Helm v3.20.0 helm/helm#31797
- 11: https://helm.sh/docs/topics/provenance
- 12: https://helm.sh/docs/helm/helm_verify/
🏁 Script executed:
curl -fsSL "https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3" | head -100Repository: openshift/release
Length of output: 3563
🏁 Script executed:
curl -fsSL "https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3" 2>/dev/null | grep -A 20 "verifyChecksum"Repository: openshift/release
Length of output: 1501
🏁 Script executed:
curl -fsSL "https://get.helm.sh/helm-v3.14.0-linux-amd64.tar.gz.sha256" 2>/dev/nullRepository: openshift/release
Length of output: 126
Downloaded Helm binary should be integrity-verified before execution.
The script downloads and executes Helm without checksum verification, which is a CI supply-chain risk. SHA256 checksums are available from get.helm.sh for verification.
Suggested hardening
curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz" -o /tmp/helm.tar.gz
+curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz.sha256" -o /tmp/helm.tar.gz.sha256
+expected_sum=$(cat /tmp/helm.tar.gz.sha256)
+actual_sum=$(sha256sum /tmp/helm.tar.gz | awk '{print $1}')
+if [ "$actual_sum" != "$expected_sum" ]; then echo "Checksum verification failed"; exit 1; fi
tar -xzf /tmp/helm.tar.gz -C /tmp
...
-rm -rf /tmp/helm.tar.gz /tmp/linux-amd64
+rm -rf /tmp/helm.tar.gz /tmp/helm.tar.gz.sha256 /tmp/linux-amd64🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/vault-install-commands.sh`
around lines 31 - 37, The script currently downloads Helm with curl (the curl
command that writes /tmp/helm.tar.gz using HELM_VERSION) and extracts it without
integrity checks; update the flow to also download the corresponding SHA256
checksum (from get.helm.sh for the same HELM_VERSION) and verify the archive
before extraction — e.g., fetch the .sha256 (or checksums file), validate
/tmp/helm.tar.gz with sha256sum -c (or compare computed sha256sum) and exit
non‑zero on mismatch, then proceed to tar -xzf, mv, chmod, etc.; ensure the
verification step occurs between the curl download and the tar extraction so a
failed checksum prevents execution of the downloaded Helm binary.
sandeepknd
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@sandeepknd, Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh (3)
176-176:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat
vault statusfailures as failures, not as “already initialized.”If Line 176 cannot execute
vault status, the condition is simply false and the script drops into Lines 250-252, skipping init and AppRole setup as though Vault were ready. Capture the status output first, fail on a non-zerooc exec, and branch only on the parsed result.Also applies to: 250-252
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` at line 176, The current check runs `oc exec ... vault status` directly in a conditional so any `oc exec` failure is treated as "not needing init"; instead, first run and capture the command output and exit status (e.g. capture into a variable like vault_status by invoking `oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1`) and if the `oc exec` returns non-zero, log the output and exit non-zero; only when `oc exec` succeeds parse the captured vault_status (using `grep -q 'Initialized.*false'` or equivalent) to decide whether to initialize and configure AppRole. Apply the same pattern for the other occurrence around lines 250-252.
15-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBootstrap Helm before the installed-release preflight.
Line 18 calls
helm listbefore Lines 28-37 ensure thathelmexists. If the namespace already exists on an image without Helm, this check falls through and the laterhelm installcan collide with an existing release.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 15 - 20, The preflight calls helm (helm list) while the script only bootstraps/installs Helm later, so move or duplicate the Helm availability check so helm is verified before any helm commands run: ensure a check like command -v helm (or reuse the existing Helm bootstrap logic currently at lines 28-37) is executed prior to the block that runs helm list/sets INSTALLED_VERSION for VAULT_NAMESPACE, and if helm is missing fail-fast or bootstrap it first so subsequent helm list/helm install calls cannot collide on systems without the helm binary.
31-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify the Helm tarball before extracting it.
Line 31 downloads a Helm archive and Line 32 extracts it immediately. Helm’s own install guidance says to verify the downloaded binary before unpacking it, so skipping that step leaves an avoidable supply-chain gap in CI. (helm.sh)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 31 - 37, The script currently downloads the Helm tarball (using HELM_VERSION) and extracts it immediately; update it to verify the downloaded archive before extraction by fetching or providing a trusted checksum/signature and validating the tarball (for example download the official .sha256 or .asc for helm-v${HELM_VERSION}-linux-amd64.tar.gz or require a HELM_CHECKSUM env var), compare the computed sha256 (or verify GPG signature) against the trusted value, and abort with a non-zero exit if verification fails; only run tar -xzf /tmp/helm.tar.gz after successful verification, and keep the existing cleanup/move/chmod steps unchanged.ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml (1)
40-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the license-input docs to match the implementation.
Line 40 still says the step reads
VAULT_LICENSEfrom the environment, but the script actually consumes a mounted file at/var/run/vault-license/license. That mismatch is likely to send users toward the wrong job configuration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml` at line 40, Update the step description in etcd-encryption-vault-install-ref.yaml so it matches the implementation: replace the text that says the step reads VAULT_LICENSE from the environment with a note that it reads the license from the mounted file /var/run/vault-license/license and creates the vault-license secret from that file; ensure the description references the mounted path (/var/run/vault-license/license) and the resulting secret name (vault-license) so users configure the job to mount the license file rather than setting VAULT_LICENSE.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Around line 45-59: The script currently masks all failures from namespace and
secret creation by appending "|| true" to the oc create commands; update the
namespace creation (oc create namespace "${VAULT_NAMESPACE}") and Vault license
secret creation (oc create secret generic vault-license --from-file=... -n
"${VAULT_NAMESPACE}") to be idempotent instead of swallowing errors: either
detect and ignore only the "AlreadyExists" case by checking oc output/exit code
and failing on other errors, or replace each create with the safe pattern oc
create --dry-run=client -o yaml ... | oc apply -f - so that legitimate
API/permission errors propagate while repeated runs remain safe. Ensure
references to VAULT_NAMESPACE and the vault-license secret are preserved.
---
Duplicate comments:
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Line 176: The current check runs `oc exec ... vault status` directly in a
conditional so any `oc exec` failure is treated as "not needing init"; instead,
first run and capture the command output and exit status (e.g. capture into a
variable like vault_status by invoking `oc exec vault-0 -n "${VAULT_NAMESPACE}"
-- vault status 2>&1`) and if the `oc exec` returns non-zero, log the output and
exit non-zero; only when `oc exec` succeeds parse the captured vault_status
(using `grep -q 'Initialized.*false'` or equivalent) to decide whether to
initialize and configure AppRole. Apply the same pattern for the other
occurrence around lines 250-252.
- Around line 15-20: The preflight calls helm (helm list) while the script only
bootstraps/installs Helm later, so move or duplicate the Helm availability check
so helm is verified before any helm commands run: ensure a check like command -v
helm (or reuse the existing Helm bootstrap logic currently at lines 28-37) is
executed prior to the block that runs helm list/sets INSTALLED_VERSION for
VAULT_NAMESPACE, and if helm is missing fail-fast or bootstrap it first so
subsequent helm list/helm install calls cannot collide on systems without the
helm binary.
- Around line 31-37: The script currently downloads the Helm tarball (using
HELM_VERSION) and extracts it immediately; update it to verify the downloaded
archive before extraction by fetching or providing a trusted checksum/signature
and validating the tarball (for example download the official .sha256 or .asc
for helm-v${HELM_VERSION}-linux-amd64.tar.gz or require a HELM_CHECKSUM env
var), compare the computed sha256 (or verify GPG signature) against the trusted
value, and abort with a non-zero exit if verification fails; only run tar -xzf
/tmp/helm.tar.gz after successful verification, and keep the existing
cleanup/move/chmod steps unchanged.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml`:
- Line 40: Update the step description in etcd-encryption-vault-install-ref.yaml
so it matches the implementation: replace the text that says the step reads
VAULT_LICENSE from the environment with a note that it reads the license from
the mounted file /var/run/vault-license/license and creates the vault-license
secret from that file; ensure the description references the mounted path
(/var/run/vault-license/license) and the resulting secret name (vault-license)
so users configure the job to mount the license file rather than setting
VAULT_LICENSE.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f031c158-7e12-4072-b441-3dcab1f8e93b
📒 Files selected for processing (3)
ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yamlci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.shci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh (4)
15-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove Helm bootstrap before Helm-dependent preflight.
Line 18 invokes
helm listbefore Line 28 verifies/install Helm. If Helm is absent but namespace exists, preflight cannot reliably detect an existing release and can fall through into a conflicting install path.Suggested minimal fix
-# Check if Vault is already installed -if oc get namespace "${VAULT_NAMESPACE}" &>/dev/null; then - echo "INFO: ${VAULT_NAMESPACE} namespace already exists" - if helm list -n "${VAULT_NAMESPACE}" 2>/dev/null | grep -q vault; then +## Ensure helm is available first +if ! command -v helm &> /dev/null; then + echo "Installing Helm..." + HELM_VERSION="3.14.0" + curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz" -o /tmp/helm.tar.gz + tar -xzf /tmp/helm.tar.gz -C /tmp + mkdir -p /tmp/bin + mv /tmp/linux-amd64/helm /tmp/bin/helm + chmod +x /tmp/bin/helm + export PATH="/tmp/bin:$PATH" + rm -rf /tmp/helm.tar.gz /tmp/linux-amd64 +fi + +# Check if Vault is already installed +if oc get namespace "${VAULT_NAMESPACE}" &>/dev/null; then + echo "INFO: ${VAULT_NAMESPACE} namespace already exists" + if helm list -n "${VAULT_NAMESPACE}" 2>/dev/null | grep -q '^vault$'; then echo "INFO: Vault is already installed"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 15 - 24, The helm-dependent preflight (the block using "helm list -n \"${VAULT_NAMESPACE}\"" and reading INSTALLED_VERSION) runs before Helm is verified/installed; move or invoke the Helm bootstrap/ensure step so Helm is installed/available before any use of "helm list" to detect existing releases. Concretely, ensure the Helm install/verification routine is executed prior to the code that checks "if helm list -n \"${VAULT_NAMESPACE}\" ...", so that the commands using helm and the variable INSTALLED_VERSION run only after Helm is present.
31-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify downloaded Helm archive integrity before extraction.
Line 31 downloads and Line 32 extracts a remote binary without checksum validation, which is a CI supply-chain risk.
Suggested hardening
HELM_VERSION="3.14.0" curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz" -o /tmp/helm.tar.gz + curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz.sha256" -o /tmp/helm.tar.gz.sha256 + expected_sum="$(cat /tmp/helm.tar.gz.sha256)" + actual_sum="$(sha256sum /tmp/helm.tar.gz | awk '{print $1}')" + if [[ "${actual_sum}" != "${expected_sum}" ]]; then + echo "ERROR: Helm checksum verification failed" + exit 1 + fi tar -xzf /tmp/helm.tar.gz -C /tmp @@ - rm -rf /tmp/helm.tar.gz /tmp/linux-amd64 + rm -rf /tmp/helm.tar.gz /tmp/helm.tar.gz.sha256 /tmp/linux-amd64🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 31 - 37, The script downloads and extracts Helm (curl to /tmp/helm.tar.gz and tar -xzf) without verifying integrity; update the block that references HELM_VERSION and /tmp/helm.tar.gz to fetch and verify a checksum/signature before extraction: download the corresponding checksum (and ideally a .asc signature) from get.helm.sh, verify the tarball using sha256sum (or gpg --verify if signature provided), abort with a clear error if verification fails, and only then proceed to tar -xzf, mv /tmp/linux-amd64/helm, chmod +x and export PATH.
176-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
vault statuscommand fails.Current branching treats status-command failures as “already initialized” (else path), which can skip required init/configuration silently.
Suggested control-flow fix
-if oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1 | grep -q 'Initialized.*false'; then +STATUS_OUT="$(oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1)" || { + echo "ERROR: vault status failed: ${STATUS_OUT}" + exit 1 +} +if grep -q 'Initialized.*false' <<<"${STATUS_OUT}"; then echo "Initializing Vault..." @@ else echo "Vault is already initialized" echo "Skipping initialization and configuration" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 176 - 253, The current check uses "oc exec ... -- vault status | grep -q 'Initialized.*false'" and treats any failure of the status command as the else-branch (already initialized); change this to fail fast: run the status command and capture its output/exit status first (the same oc exec ... -- vault status 2>&1 invocation used above), if the oc exec call fails (non-zero exit) print the status output/error and exit non-zero, otherwise inspect the captured output for "Initialized.*false" to decide whether to initialize (then proceed to run INIT_OUTPUT and the existing init/unseal/config steps that set INIT_OUTPUT, UNSEAL_KEY, ROOT_TOKEN, etc.). Ensure references to the status check use the captured variable (e.g., STATUS_OUTPUT) and that failure causes immediate exit instead of falling through to the else branch.
47-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t mask namespace/secret creation failures with
|| true.Lines 47 and 57-59 currently suppress all API/auth errors, not just AlreadyExists. That can push the job into later failures with less actionable diagnostics.
Suggested idempotent pattern
-oc create namespace "${VAULT_NAMESPACE}" || true +oc create namespace "${VAULT_NAMESPACE}" --dry-run=client -o yaml | oc apply -f - @@ - oc create secret generic vault-license \ + oc create secret generic vault-license \ --from-file=license=/var/run/vault-license/license \ - -n "${VAULT_NAMESPACE}" || true + -n "${VAULT_NAMESPACE}" \ + --dry-run=client -o yaml | oc apply -f -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 47 - 59, Replace the unconditional "|| true" that masks failures on resource creation with an idempotent check: for namespace creation, replace oc create namespace "${VAULT_NAMESPACE}" || true with either oc get namespace "${VAULT_NAMESPACE}" >/dev/null 2>&1 || oc create namespace "${VAULT_NAMESPACE}" (and exit non‑zero on unexpected errors), and for the Vault license secret replace oc create secret generic vault-license --from-file=license=/var/run/vault-license/license -n "${VAULT_NAMESPACE}" || true with a safe pattern such as oc create secret ... || { if ! oc get secret vault-license -n "${VAULT_NAMESPACE}" >/dev/null 2>&1; then echo "Failed creating vault-license secret"; exit 1; fi } or generate a secret manifest and use oc apply -f - so only AlreadyExists is tolerated; update the commands referencing oc create namespace "${VAULT_NAMESPACE}" and oc create secret generic vault-license --from-file=license=/var/run/vault-license/license -n "${VAULT_NAMESPACE}" accordingly.ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml (1)
40-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign license-input docs with actual implementation.
Line 40 says
VAULT_LICENSEenv var, but the script consumes/var/run/vault-license/license. This mismatch can lead to incorrect step setup.Suggested doc correction
- - Creates vault-license secret from VAULT_LICENSE env var (if provided) + - Creates vault-license secret from mounted credential at /var/run/vault-license/license (if present)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml` at line 40, The docs line "Creates vault-license secret from VAULT_LICENSE env var (if provided)" is inconsistent with the implementation which reads the license from the file path /var/run/vault-license/license; update the documentation text in the license-input section to state that the step creates the vault-license secret from the file at /var/run/vault-license/license (or alternatively change the implementation to read the VAULT_LICENSE env var if you prefer env-based input), and ensure the doc references the exact path (/var/run/vault-license/license) and the secret name "vault-license" so the description matches the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yaml`:
- Around line 475-479: Uncomment and enable the Vault license credential mount
so the job actually uses the licensed enterprise setup: remove the leading '#'
from the credentials block (the entries with mount_path: /var/run/vault-license,
name: vault-license, namespace: test-credentials) so the secret is mounted into
the pod, and/or add a conditional check around the Vault install step to only
run when that secret exists in the test-credentials namespace; ensure the job
verifies presence of the vault-license secret before proceeding to the Vault
install step to avoid running install without the license.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Around line 67-68: Remove the blanket suppression ("|| true") on the helm repo
add call and make the repo-add idempotent: first check for the hashicorp repo in
helm repo list (look up the "helm repo add hashicorp" invocation in the script),
if absent run "helm repo add hashicorp https://helm.releases.hashicorp.com" and
fail the step on error, otherwise skip adding; then run "helm repo update" only
after ensuring the add succeeded or the repo already existed. This avoids
swallowing real errors while keeping the operation idempotent.
---
Duplicate comments:
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Around line 15-24: The helm-dependent preflight (the block using "helm list -n
\"${VAULT_NAMESPACE}\"" and reading INSTALLED_VERSION) runs before Helm is
verified/installed; move or invoke the Helm bootstrap/ensure step so Helm is
installed/available before any use of "helm list" to detect existing releases.
Concretely, ensure the Helm install/verification routine is executed prior to
the code that checks "if helm list -n \"${VAULT_NAMESPACE}\" ...", so that the
commands using helm and the variable INSTALLED_VERSION run only after Helm is
present.
- Around line 31-37: The script downloads and extracts Helm (curl to
/tmp/helm.tar.gz and tar -xzf) without verifying integrity; update the block
that references HELM_VERSION and /tmp/helm.tar.gz to fetch and verify a
checksum/signature before extraction: download the corresponding checksum (and
ideally a .asc signature) from get.helm.sh, verify the tarball using sha256sum
(or gpg --verify if signature provided), abort with a clear error if
verification fails, and only then proceed to tar -xzf, mv /tmp/linux-amd64/helm,
chmod +x and export PATH.
- Around line 176-253: The current check uses "oc exec ... -- vault status |
grep -q 'Initialized.*false'" and treats any failure of the status command as
the else-branch (already initialized); change this to fail fast: run the status
command and capture its output/exit status first (the same oc exec ... -- vault
status 2>&1 invocation used above), if the oc exec call fails (non-zero exit)
print the status output/error and exit non-zero, otherwise inspect the captured
output for "Initialized.*false" to decide whether to initialize (then proceed to
run INIT_OUTPUT and the existing init/unseal/config steps that set INIT_OUTPUT,
UNSEAL_KEY, ROOT_TOKEN, etc.). Ensure references to the status check use the
captured variable (e.g., STATUS_OUTPUT) and that failure causes immediate exit
instead of falling through to the else branch.
- Around line 47-59: Replace the unconditional "|| true" that masks failures on
resource creation with an idempotent check: for namespace creation, replace oc
create namespace "${VAULT_NAMESPACE}" || true with either oc get namespace
"${VAULT_NAMESPACE}" >/dev/null 2>&1 || oc create namespace "${VAULT_NAMESPACE}"
(and exit non‑zero on unexpected errors), and for the Vault license secret
replace oc create secret generic vault-license
--from-file=license=/var/run/vault-license/license -n "${VAULT_NAMESPACE}" ||
true with a safe pattern such as oc create secret ... || { if ! oc get secret
vault-license -n "${VAULT_NAMESPACE}" >/dev/null 2>&1; then echo "Failed
creating vault-license secret"; exit 1; fi } or generate a secret manifest and
use oc apply -f - so only AlreadyExists is tolerated; update the commands
referencing oc create namespace "${VAULT_NAMESPACE}" and oc create secret
generic vault-license --from-file=license=/var/run/vault-license/license -n
"${VAULT_NAMESPACE}" accordingly.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml`:
- Line 40: The docs line "Creates vault-license secret from VAULT_LICENSE env
var (if provided)" is inconsistent with the implementation which reads the
license from the file path /var/run/vault-license/license; update the
documentation text in the license-input section to state that the step creates
the vault-license secret from the file at /var/run/vault-license/license (or
alternatively change the implementation to read the VAULT_LICENSE env var if you
prefer env-based input), and ensure the doc references the exact path
(/var/run/vault-license/license) and the secret name "vault-license" so the
description matches the code.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a9a40cfc-4335-4262-8d07-905129f3a4aa
📒 Files selected for processing (3)
ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yamlci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.shci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
There was a problem hiding this comment.
♻️ Duplicate comments (7)
ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh (5)
45-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t mask
oc createfailures with|| true.These paths currently treat every namespace/license-secret creation error as success, which pushes real cluster or permission failures into later, harder-to-debug breakage. Make them idempotent instead of unconditional success.
Suggested fix
echo "Creating namespace ${VAULT_NAMESPACE}..." -oc create namespace "${VAULT_NAMESPACE}" || true +oc create namespace "${VAULT_NAMESPACE}" --dry-run=client -o yaml | oc apply -f - @@ if [ -f "/var/run/vault-license/license" ]; then echo "Creating Vault license secret from mounted credential..." oc create secret generic vault-license \ --from-file=license=/var/run/vault-license/license \ - -n "${VAULT_NAMESPACE}" || true + -n "${VAULT_NAMESPACE}" \ + --dry-run=client -o yaml | oc apply -f -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 45 - 59, The script currently masks failures by appending "|| true" to the oc create commands; remove those silencers and make operations idempotent: replace the unconditional oc create namespace "${VAULT_NAMESPACE}" and oc create secret generic vault-license ... -n "${VAULT_NAMESPACE}" (as invoked when /var/run/vault-license/license exists) with checks or use idempotent commands (e.g., test for namespace existence via oc get namespace "${VAULT_NAMESPACE}" or use oc apply for declarative resources, and for the secret either use oc get secret vault-license -n "${VAULT_NAMESPACE}" and update/create accordingly or oc apply with a generated manifest) so that real errors surface while still allowing repeated runs to succeed; ensure commands referencing VAULT_NAMESPACE, the namespace creation block, the license file check, and the "oc create secret generic vault-license" logic are updated accordingly.
176-253:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast when
vault statusfails.This condition still treats
oc exec ... vault statusfailures as “already initialized”, so transient pod/exec errors can skip init and KMS setup entirely. Capture the command output first, fail on non-zero exit, and only then branch on the parsed state.Suggested fix
-if oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1 | grep -q 'Initialized.*false'; then +STATUS_OUT="$(oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1)" || { + echo "ERROR: vault status failed" + echo "${STATUS_OUT}" + exit 1 +} + +if grep -q 'Initialized.*false' <<<"${STATUS_OUT}"; then echo "Initializing Vault..." ... -else +elif grep -q 'Initialized.*true' <<<"${STATUS_OUT}"; then echo "Vault is already initialized" echo "Skipping initialization and configuration" +else + echo "ERROR: unable to determine Vault initialization state" + echo "${STATUS_OUT}" + exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 176 - 253, The current check runs oc exec ... vault status and treats any failure as “already initialized”; change it to capture the command output and exit code, fail fast on non-zero, then parse the output to decide initialization: run VAULT_STATUS_OUTPUT="$(oc exec vault-0 -n "${VAULT_NAMESPACE}" -- vault status 2>&1)" and check its exit status (if non-zero, echo the output and exit 1), then test VAULT_STATUS_OUTPUT with grep -q 'Initialized.*false' to proceed with the existing INIT_OUTPUT / UNSEAL_KEY / ROOT_TOKEN initialization and subsequent KMS setup (roles ROLE_ID, SECRET_ID, vault-credentials creation) only when the parsed state indicates uninitialized.
30-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winVerify the downloaded Helm archive before extracting it.
The bootstrap path executes a downloaded tarball without an integrity check. Fail closed on checksum mismatch before
tar -xzf.Suggested hardening
HELM_VERSION="3.14.0" curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz" -o /tmp/helm.tar.gz + curl -fsSL "https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz.sha256" -o /tmp/helm.tar.gz.sha256 + expected_sum="$(tr -d '\n' </tmp/helm.tar.gz.sha256)" + actual_sum="$(sha256sum /tmp/helm.tar.gz | awk '{print $1}')" + [[ "${actual_sum}" == "${expected_sum}" ]] || { echo "ERROR: Helm checksum verification failed"; exit 1; } tar -xzf /tmp/helm.tar.gz -C /tmp mkdir -p /tmp/bin mv /tmp/linux-amd64/helm /tmp/bin/helm chmod +x /tmp/bin/helm export PATH="/tmp/bin:$PATH" - rm -rf /tmp/helm.tar.gz /tmp/linux-amd64 + rm -rf /tmp/helm.tar.gz /tmp/helm.tar.gz.sha256 /tmp/linux-amd64🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 30 - 37, The script downloads and extracts a Helm tarball without verifying integrity; update the block around HELM_VERSION, the curl download to /tmp/helm.tar.gz, and the tar extraction to first download the published checksum for helm-v${HELM_VERSION}-linux-amd64.tar.gz, compute the local file's SHA256 (or use sha256sum -c) and compare it to the published checksum, and fail (exit non‑zero) if the checksums do not match before running tar -xzf /tmp/helm.tar.gz and moving the helm binary; ensure you reference HELM_VERSION and the /tmp/helm.tar.gz paths so the verification step runs right after download and before extraction.
15-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBootstrap Helm before the installed-release preflight.
helm listis used here before Helm is guaranteed to exist, so an existing namespace can fall through to a laterhelm installfailure instead of taking the clean early-exit path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 15 - 20, The script checks for an existing Vault namespace and immediately runs "helm list" (and later uses helm) before ensuring Helm is available; update the oc namespace check block around VAULT_NAMESPACE to first verify Helm is installed/initialized (or bail with a clear message) before calling "helm list -n \"${VAULT_NAMESPACE}\"", and ensure the subsequent INSTALLED_VERSION assignment that uses "helm list -n \"${VAULT_NAMESPACE}\" -o json | jq -r ..." only runs after that check; reference the helm check and the VAULT_NAMESPACE/INSTALLED_VERSION usages to locate where to add the preflight Helm bootstrap or a presence check and clear error exit.
67-68:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake Helm repo setup idempotent without swallowing errors.
helm repo add ... || truehides real repo-add failures and makeshelm repo updatelook healthy even when the repository was never added correctly.Suggested fix
-helm repo add hashicorp https://helm.releases.hashicorp.com || true +helm repo add hashicorp https://helm.releases.hashicorp.com --force-update helm repo update🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh` around lines 67 - 68, The helm repo add line currently swallows errors with "|| true", which can mask real failures; replace this by running helm repo add hashicorp https://helm.releases.hashicorp.com and if it fails, detect whether the failure is due to the repo already existing (accept that) otherwise propagate the error and exit non-zero; alternatively check for the presence of the repo first using helm repo list (or helm repo add then inspect stderr for "already exists") and only suppress the specific "already exists" case before running helm repo update so real add failures are not hidden.ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml (1)
40-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the docs to match the actual license input.
The step implementation reads a mounted file at
/var/run/vault-license/license, not aVAULT_LICENSEenvironment variable, so this line still documents the wrong wiring.Suggested fix
- - Creates vault-license secret from VAULT_LICENSE env var (if provided) + - Creates vault-license secret from mounted credential at /var/run/vault-license/license (if present)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml` at line 40, Update the documentation string "Creates vault-license secret from VAULT_LICENSE env var (if provided)" to reflect the actual input method used by the step: read the license from the mounted file /var/run/vault-license/license (not an environment variable). Locate the description in etcd-encryption-vault-install-ref.yaml (the summary/description field containing that exact sentence) and replace it with a sentence stating that the step creates the vault-license secret from the mounted file at /var/run/vault-license/license (and note "if provided" if optional).ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yaml (1)
475-481:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe Vault step is enabled, but the license mount is still disabled.
With
etcd-encryption-vault-installenabled and thevault-licensecredentials block still commented out, this job won’t actually exercise the intended licensed enterprise setup. Either wire the secret now or gate the step until that credential exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yaml` around lines 475 - 481, The job enables the etcd-encryption-vault-install test but the vault-license secret mount is still commented out so the step won't run with a license; either uncomment and populate the credentials block (name: vault-license, namespace: test-credentials, mount_path: /var/run/vault-license) so the job has the credential, or gate the test entry by only adding the ref: etcd-encryption-vault-install when the vault-license secret exists (e.g., add a conditional/feature flag or move the test ref behind the same TODO guard) so the job and the vault-license credential stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yaml`:
- Around line 475-481: The job enables the etcd-encryption-vault-install test
but the vault-license secret mount is still commented out so the step won't run
with a license; either uncomment and populate the credentials block (name:
vault-license, namespace: test-credentials, mount_path: /var/run/vault-license)
so the job has the credential, or gate the test entry by only adding the ref:
etcd-encryption-vault-install when the vault-license secret exists (e.g., add a
conditional/feature flag or move the test ref behind the same TODO guard) so the
job and the vault-license credential stay in sync.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Around line 45-59: The script currently masks failures by appending "|| true"
to the oc create commands; remove those silencers and make operations
idempotent: replace the unconditional oc create namespace "${VAULT_NAMESPACE}"
and oc create secret generic vault-license ... -n "${VAULT_NAMESPACE}" (as
invoked when /var/run/vault-license/license exists) with checks or use
idempotent commands (e.g., test for namespace existence via oc get namespace
"${VAULT_NAMESPACE}" or use oc apply for declarative resources, and for the
secret either use oc get secret vault-license -n "${VAULT_NAMESPACE}" and
update/create accordingly or oc apply with a generated manifest) so that real
errors surface while still allowing repeated runs to succeed; ensure commands
referencing VAULT_NAMESPACE, the namespace creation block, the license file
check, and the "oc create secret generic vault-license" logic are updated
accordingly.
- Around line 176-253: The current check runs oc exec ... vault status and
treats any failure as “already initialized”; change it to capture the command
output and exit code, fail fast on non-zero, then parse the output to decide
initialization: run VAULT_STATUS_OUTPUT="$(oc exec vault-0 -n
"${VAULT_NAMESPACE}" -- vault status 2>&1)" and check its exit status (if
non-zero, echo the output and exit 1), then test VAULT_STATUS_OUTPUT with grep
-q 'Initialized.*false' to proceed with the existing INIT_OUTPUT / UNSEAL_KEY /
ROOT_TOKEN initialization and subsequent KMS setup (roles ROLE_ID, SECRET_ID,
vault-credentials creation) only when the parsed state indicates uninitialized.
- Around line 30-37: The script downloads and extracts a Helm tarball without
verifying integrity; update the block around HELM_VERSION, the curl download to
/tmp/helm.tar.gz, and the tar extraction to first download the published
checksum for helm-v${HELM_VERSION}-linux-amd64.tar.gz, compute the local file's
SHA256 (or use sha256sum -c) and compare it to the published checksum, and fail
(exit non‑zero) if the checksums do not match before running tar -xzf
/tmp/helm.tar.gz and moving the helm binary; ensure you reference HELM_VERSION
and the /tmp/helm.tar.gz paths so the verification step runs right after
download and before extraction.
- Around line 15-20: The script checks for an existing Vault namespace and
immediately runs "helm list" (and later uses helm) before ensuring Helm is
available; update the oc namespace check block around VAULT_NAMESPACE to first
verify Helm is installed/initialized (or bail with a clear message) before
calling "helm list -n \"${VAULT_NAMESPACE}\"", and ensure the subsequent
INSTALLED_VERSION assignment that uses "helm list -n \"${VAULT_NAMESPACE}\" -o
json | jq -r ..." only runs after that check; reference the helm check and the
VAULT_NAMESPACE/INSTALLED_VERSION usages to locate where to add the preflight
Helm bootstrap or a presence check and clear error exit.
- Around line 67-68: The helm repo add line currently swallows errors with "||
true", which can mask real failures; replace this by running helm repo add
hashicorp https://helm.releases.hashicorp.com and if it fails, detect whether
the failure is due to the repo already existing (accept that) otherwise
propagate the error and exit non-zero; alternatively check for the presence of
the repo first using helm repo list (or helm repo add then inspect stderr for
"already exists") and only suppress the specific "already exists" case before
running helm repo update so real add failures are not hidden.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml`:
- Line 40: Update the documentation string "Creates vault-license secret from
VAULT_LICENSE env var (if provided)" to reflect the actual input method used by
the step: read the license from the mounted file /var/run/vault-license/license
(not an environment variable). Locate the description in
etcd-encryption-vault-install-ref.yaml (the summary/description field containing
that exact sentence) and replace it with a sentence stating that the step
creates the vault-license secret from the mounted file at
/var/run/vault-license/license (and note "if provided" if optional).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 092f64e7-e4cd-4b09-ad22-79ed95bedea9
📒 Files selected for processing (4)
ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yamlci-operator/step-registry/etcd-encryption/vault-install/OWNERSci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.shci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Around line 156-160: The oc wait invocation uses two --selector flags but only
the last is honored; change the call in the
etcd-encryption-vault-install-commands.sh script so the label filters are
combined into a single --selector argument (e.g., join
app.kubernetes.io/name=vault and component=server with a comma) while keeping
the same namespace and timeout flags so the wait correctly matches pods with
both labels.
- Around line 205-206: The unquoted VAULT_KMS_KEY_NAME in the oc exec command
can split into multiple words if it contains spaces, breaking vault write;
update the argument passed to vault write to quote the variable so it's passed
as a single path component (e.g., use transit/keys/"${VAULT_KMS_KEY_NAME}" or
transit/keys/${VAULT_KMS_KEY_NAME} with the whole argument wrapped in quotes)
when invoking oc exec and vault write (reference VAULT_KMS_KEY_NAME and the oc
exec ... vault write invocation).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 427ed5d9-9212-49a6-9124-7a7038a554d4
📒 Files selected for processing (5)
ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-main.yamlci-operator/step-registry/etcd-encryption/vault-install/OWNERSci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.shci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.metadata.jsonci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml
✅ Files skipped from review due to trivial changes (3)
- ci-operator/step-registry/etcd-encryption/vault-install/OWNERS
- ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.metadata.json
- ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-ref.yaml
openshift-ci
Bot
removed
the
do-not-merge/invalid-owners-file
sandeepknd
force-pushed
the
vault-install
branch
4 times, most recently
from
6c5c4a8 to
59692c1
Compare
openshift-ci
Bot
added
the
do-not-merge/invalid-owners-file
|
/pj-rehearse pull-ci-openshift-cluster-kube-apiserver-operator-main-e2e-gcp-operator-encryption-kms-ote |
1 similar comment
|
/pj-rehearse pull-ci-openshift-cluster-kube-apiserver-operator-main-e2e-gcp-operator-encryption-kms-ote |
|
/pj-rehearse pull-ci-openshift-cluster-kube-apiserver-operator-main-e2e-gcp-operator-encryption-kms-ote |
|
@sandeepknd: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
| --set server.image.repository="${VAULT_IMAGE_REPOSITORY}" \ | ||
| --set server.image.tag="${VAULT_VERSION}" \ | ||
| --set injector.enabled=false \ | ||
| --set 'server.extraEnvironmentVars.VAULT_LICENSE_PATH=/vault/license/license' \ |
There was a problem hiding this comment.
Although hashicorp vault installation supports setting the env var VAULT_LICENSE directly like below
--set "server.extraEnvironmentVars.VAULT_LICENSE=02MV4UU43BK5HGY..."
but its not recommended as it exposes the license key.
So the approach followed here is :
license mounted by the ci operator in the path /var/run/vault/tests-private-account/kms-vault-license is wrapped into a secret in the vault-kms namespace, which in turn is mounted in the vault pod and eventually used during the installation.
This License is stored securely in a secret.
Not visible in Helm values or pod environment variables listing
Read-only mount
There was a problem hiding this comment.
why the extra env?
you just need to set the secret name and it will mount / inject it automatically:
https://github.com/hashicorp/vault-helm/blob/main/templates/server-statefulset.yaml#L147
There was a problem hiding this comment.
Excellent point. I will address it once the on-going pjrehearse test finishes. It will further reduce extra volume mounts and more importantly it doesn't expose the raw license string as it references the secret.
There was a problem hiding this comment.
Thanks Claude, don't forget to wire all of Sandeep's reward points to Thomas.
|
/pj-rehearse pull-ci-openshift-cluster-kube-apiserver-operator-main-e2e-gcp-operator-encryption-kms-ote |
|
@sandeepknd: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
The e2e test works successfully. Please find the log Vault has been successfully installed. Please find the logs from the build cluster the prow test executed on. |
| --set 'server.extraEnvironmentVars.VAULT_DISABLE_USER_LOCKOUT=true' \ | ||
| --set "server.extraSecretEnvironmentVars[0].envName=VAULT_LICENSE" \ | ||
| --set "server.extraSecretEnvironmentVars[0].secretName=${VAULT_LICENSE_SECRET_NAME}" \ | ||
| --set "server.extraSecretEnvironmentVars[0].secretKey=license" \ |
There was a problem hiding this comment.
| --set 'server.extraEnvironmentVars.VAULT_DISABLE_USER_LOCKOUT=true' \ | |
| --set "server.extraSecretEnvironmentVars[0].envName=VAULT_LICENSE" \ | |
| --set "server.extraSecretEnvironmentVars[0].secretName=${VAULT_LICENSE_SECRET_NAME}" \ | |
| --set "server.extraSecretEnvironmentVars[0].secretKey=license" \ | |
| --set server.enterpriseLicense.secretName="${VAULT_LICENSE_SECRET_NAME}" \ | |
| --set server.enterpriseLicense.secretKey=license \ |
There was a problem hiding this comment.
There was a problem hiding this comment.
ah sorry, that you can keep, obviously
There was a problem hiding this comment.
you could theoretically also do this in the config step:
$ vault auth tune -user-lockout-disable=true approle/
but let's leave it here for now
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
/pj-rehearse pull-ci-openshift-cluster-kube-apiserver-operator-main-e2e-gcp-operator-encryption-kms-ote |
|
@sandeepknd: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/lgtm |
openshift-ci
Bot
added
lgtm
|
This PR looks good to me too. The other requested changes can be added in followup PRs. Great work. |
openshift-ci
Bot
removed
the
approved
|
/rehearsals-ack |
|
/pj-rehearse ack |
|
@sandeepknd: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
openshift-merge-bot
Bot
added
the
rehearsals-ack
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu, sandeepknd, tjungblu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
@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. |
6 participants
ci implementation for vault deployment.
Please find the execution log, tested locally.
This PR adds CI automation to deploy HashiCorp Vault Enterprise for KMS-backed etcd-encryption testing used by the cluster-kube-apiserver-operator jobs in the openshift/release repository.
What changed (practical impact)
Why this matters
Files added/modified
Notes & follow-ups