Vault ci#79255
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
WalkthroughThe PR adds a new TLS observer e2e test to OpenShift origin CI configurations, restructures periodic TLS scanner job definitions, and parametrizes Vault deployment names across etcd-encryption step registry commands and references by introducing a configurable ChangesTLS Observed Configuration E2E Tests
TLS Scanner Periodic Job Restructuring
Vault Helm Release Name Parametrization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gangwgr The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
[REHEARSALNOTIFIER]
A total of 46 jobs have been affected by this change. The above listing is non-exhaustive and limited to 25 jobs. A full list of affected jobs can be found here Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-configure/etcd-encryption-vault-configure-commands.sh`:
- Line 23: Quote the pod reference variable in each oc exec invocation to
prevent word splitting/globbing; replace unquoted ${VAULT_RELEASE_NAME}-0 with a
quoted form like "${VAULT_RELEASE_NAME}-0" in every oc exec occurrence (the
lines currently using oc exec ${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" --
\ and the six other similar invocations at lines noted in the review) so all
seven oc exec commands consistently use the quoted pod name.
- Line 88: The script currently prints the sensitive AppRole credential ROLE_ID
via the echo line (echo " - ROLE_ID: ${ROLE_ID}"); remove that echo (or replace
it with a non-sensitive message such as " - ROLE_ID: redacted" or " - ROLE_ID:
(set)") so the ROLE_ID value is not written to CI logs; locate the echo that
references the ROLE_ID variable in the
etcd-encryption-vault-configure-commands.sh and delete or redact that single log
line to prevent leaking authentication credentials.
In
`@ci-operator/step-registry/etcd-encryption/vault-install/etcd-encryption-vault-install-commands.sh`:
- Line 75: The oc wait command uses an unquoted variable substitution
pod/${VAULT_RELEASE_NAME}-0 which can be subject to word splitting or globbing;
update the command that builds the pod name (the oc wait invocation referencing
VAULT_RELEASE_NAME and VAULT_NAMESPACE) to quote the pod argument (e.g.
"pod/${VAULT_RELEASE_NAME}-0") so the entire pod name is treated as a single
token while keeping the existing -n "${VAULT_NAMESPACE}" and --timeout options
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6d97d5a9-ca4a-41a2-954c-6e3d8ea31678
⛔ Files ignored due to path filters (2)
ci-operator/jobs/openshift/origin/openshift-origin-main-presubmits.yamlis excluded by!ci-operator/jobs/**ci-operator/jobs/openshift/origin/openshift-origin-release-4.22-presubmits.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (8)
ci-operator/config/openshift/origin/openshift-origin-main.yamlci-operator/config/openshift/origin/openshift-origin-release-4.22.yamlci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yamlci-operator/config/openshift/tls-scanner/openshift-tls-scanner-release-4.22.yamlci-operator/step-registry/etcd-encryption/vault-configure/etcd-encryption-vault-configure-commands.shci-operator/step-registry/etcd-encryption/vault-configure/etcd-encryption-vault-configure-ref.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
💤 Files with no reviewable changes (2)
- ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-main.yaml
- ci-operator/config/openshift/tls-scanner/openshift-tls-scanner-release-4.22.yaml
| # Enable transit secret engine | ||
| echo "Enabling transit secret engine..." | ||
| oc exec vault-0 -n "${VAULT_NAMESPACE}" -- \ | ||
| oc exec ${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" -- \ |
There was a problem hiding this comment.
Quote variables to prevent word splitting.
Multiple oc exec commands use unquoted ${VAULT_RELEASE_NAME}-0 pod references. While Helm release names typically don't contain spaces, defensive coding practices require quoting variables to prevent potential word splitting or globbing issues.
🛡️ Proposed fix
-oc exec ${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" -- \
+oc exec "${VAULT_RELEASE_NAME}-0" -n "${VAULT_NAMESPACE}" -- \
env VAULT_TOKEN="${ROOT_TOKEN}" vault secrets enable -path=transit transitApply the same quoting pattern to all seven oc exec invocations at lines 23, 28, 33, 38, 56, 64, and 66.
Also applies to: 28-28, 33-33, 38-38, 56-56, 64-64, 66-66
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 23-23: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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-configure/etcd-encryption-vault-configure-commands.sh`
at line 23, Quote the pod reference variable in each oc exec invocation to
prevent word splitting/globbing; replace unquoted ${VAULT_RELEASE_NAME}-0 with a
quoted form like "${VAULT_RELEASE_NAME}-0" in every oc exec occurrence (the
lines currently using oc exec ${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" --
\ and the six other similar invocations at lines noted in the review) so all
seven oc exec commands consistently use the quoted pod name.
| echo " - Vault Service: ${VAULT_RELEASE_NAME}.${VAULT_NAMESPACE}.svc:8200" | ||
| echo " - Credentials Secret: vault-credentials (namespace: ${VAULT_NAMESPACE})" | ||
| echo " - Transit Key: ${VAULT_KMS_KEY_NAME}" | ||
| echo " - ROLE_ID: ${ROLE_ID}" |
There was a problem hiding this comment.
Remove ROLE_ID from log output.
The AppRole ROLE_ID is part of the authentication credentials and should not be logged to CI outputs. While it cannot authenticate alone (requires the SECRET_ID), exposing authentication components in logs increases the attack surface. As per coding guidelines, step registry command scripts must protect sensitive information from leaking into CI logs.
🔒 Proposed fix
echo "Summary:"
echo " - Vault Service: ${VAULT_RELEASE_NAME}.${VAULT_NAMESPACE}.svc:8200"
echo " - Credentials Secret: vault-credentials (namespace: ${VAULT_NAMESPACE})"
echo " - Transit Key: ${VAULT_KMS_KEY_NAME}"
-echo " - ROLE_ID: ${ROLE_ID}"
echo ""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo " - ROLE_ID: ${ROLE_ID}" | |
| echo "Summary:" | |
| echo " - Vault Service: ${VAULT_RELEASE_NAME}.${VAULT_NAMESPACE}.svc:8200" | |
| echo " - Credentials Secret: vault-credentials (namespace: ${VAULT_NAMESPACE})" | |
| echo " - Transit Key: ${VAULT_KMS_KEY_NAME}" | |
| echo "" |
🤖 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-configure/etcd-encryption-vault-configure-commands.sh`
at line 88, The script currently prints the sensitive AppRole credential ROLE_ID
via the echo line (echo " - ROLE_ID: ${ROLE_ID}"); remove that echo (or replace
it with a non-sensitive message such as " - ROLE_ID: redacted" or " - ROLE_ID:
(set)") so the ROLE_ID value is not written to CI logs; locate the echo that
references the ROLE_ID variable in the
etcd-encryption-vault-configure-commands.sh and delete or redact that single log
line to prevent leaking authentication credentials.
| echo "Waiting for Vault pod to be ready..." | ||
| oc wait --for=condition=ready pod/vault-0 -n "${VAULT_NAMESPACE}" --timeout=5m | ||
| echo "Waiting for Vault pod to be ready..." | ||
| oc wait --for=condition=ready pod/${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" --timeout=5m |
There was a problem hiding this comment.
Quote variable to prevent word splitting.
While Helm release names typically don't contain spaces, defensive coding practices require quoting variables in command arguments to prevent potential word splitting or globbing issues.
🛡️ Proposed fix
-oc wait --for=condition=ready pod/${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" --timeout=5m
+oc wait --for=condition=ready "pod/${VAULT_RELEASE_NAME}-0" -n "${VAULT_NAMESPACE}" --timeout=5m📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc wait --for=condition=ready pod/${VAULT_RELEASE_NAME}-0 -n "${VAULT_NAMESPACE}" --timeout=5m | |
| oc wait --for=condition=ready "pod/${VAULT_RELEASE_NAME}-0" -n "${VAULT_NAMESPACE}" --timeout=5m |
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 75-75: Double quote to prevent globbing and word splitting.
(SC2086)
🤖 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 75, The oc wait command uses an unquoted variable substitution
pod/${VAULT_RELEASE_NAME}-0 which can be subject to word splitting or globbing;
update the command that builds the pod name (the oc wait invocation referencing
VAULT_RELEASE_NAME and VAULT_NAMESPACE) to quote the pod argument (e.g.
"pod/${VAULT_RELEASE_NAME}-0") so the entire pod name is treated as a single
token while keeping the existing -n "${VAULT_NAMESPACE}" and --timeout options
unchanged.
1 participant
Summary
This PR adds support for parameterized Vault deployments in OpenShift CI and introduces new TLS testing configuration.
Vault Deployment Flexibility
The primary changes make the Vault installation and configuration steps in the etcd-encryption CI suite more flexible by introducing a
VAULT_RELEASE_NAMEparameter. Previously, Vault was hardcoded to use the release namevault, which prevented running multiple Vault instances simultaneously. Changes include:VAULT_RELEASE_NAME(default:"vault") to control the Helm release name, allowing deployment of multiple Vault instances with different names (e.g.,vault-1,vault-2)vault-0references with${VAULT_RELEASE_NAME}-0TLS Testing Configuration
Added new CI test jobs for TLS testing in OpenShift origin:
openshift-origin-mainandopenshift-origin-release-4.22configurations that runs TLS observed configuration testing on the openshift-org-aws cluster profile with resource watching enabledTLS Scanner Configuration Updates
Updated the tls-scanner CI configurations to restructure periodic test definitions:
reporter_configblocks from tls-scanner configurations