Repository navigation
OSAC-1675: Add runner user setup and fix machine-init for github-runner - #98
Conversation
|
@eliorerz: This pull request references OSAC-1675 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. |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (5)
WalkthroughAdds a ChangesRunner Bootstrap and Vault Automation
Estimated code review effort: 4 (Complex) | ~60 minutes New CI Machine Setup Runbook
Estimated code review effort: 1 (Trivial) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant vault-migrate-secrets.sh
participant SourceVault
participant DestinationVault
Operator->>vault-migrate-secrets.sh: run with src-ssh, dst-ssh, init jsons
vault-migrate-secrets.sh->>SourceVault: vault status -format=json
vault-migrate-secrets.sh->>DestinationVault: vault status -format=json
vault-migrate-secrets.sh->>SourceVault: vault kv list -format=json (recursive)
SourceVault-->>vault-migrate-secrets.sh: leaf key paths
loop each secret key
vault-migrate-secrets.sh->>SourceVault: vault kv get -format=json
SourceVault-->>vault-migrate-secrets.sh: .data.data payload
vault-migrate-secrets.sh->>DestinationVault: vault kv put key -
DestinationVault-->>vault-migrate-secrets.sh: write result
end
vault-migrate-secrets.sh-->>Operator: migrated/failed summary
Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@scripts/machine-init.sh`:
- Around line 480-485: The runner-user verification in the `id "${RUNNER_USER}"`
block only checks that the user exists, so it misses required setup contract
failures. Update the runner-user validation in `machine-init.sh` to also verify
`libvirt` group membership and passwordless sudo for `${RUNNER_USER}`, and
ensure each missing requirement increments `failures` just like the existing
“user not found” path. Keep the checks and failure handling together so the
summary reflects any contract violation.
- Around line 142-144: The libvirt group membership step can fail because
`usermod -aG libvirt` assumes the group already exists; update the
`machine-init.sh` runner setup to ensure `libvirt` is created before calling
`usermod`, or make the add-to-group logic explicitly depend on the
package/service step that creates it. Use the `RUNNER_USER` setup block with the
`id -nG ... | grep -qw libvirt` check to locate this change and keep the
existing echo flow intact.
- Around line 328-330: The cluster-tool setup in the runner-home configuration
block can expand RUNNER_USER before that user is guaranteed to exist, which may
create invalid paths and later fail during ownership changes. Update the script
around the RUNNER_HOME and CT_CONFIG_DIR setup to either call create_runner_user
first or explicitly fail early with a clear message if the runner user is
missing, so the cluster-tool step never proceeds against a non-existent home
directory.
- Around line 354-356: The ownership step in machine-init.sh is too broad
because the chown in the data directory setup can recursively touch an arbitrary
DATA_PATH from config. Update the directory creation block around mkdir and
chown so only the directories this script creates are chowned, and add
validation in the same flow to reject unsafe DATA_PATH roots before ownership
changes. Use the existing DATA_PATH and RUNNER_USER handling in the machine-init
script to keep the fix localized and avoid recursive ownership of the full
configured path.
In `@vault/scripts/vault-migrate-secrets.sh`:
- Line 161: The vault-migrate-secrets.sh file was auto-modified by
end-of-file-fixer because it is missing the required trailing newline. Update
the script so the file ends with a single EOF newline, then re-run pre-commit
and commit that newline-only change; use the vault-migrate-secrets.sh script as
the target for the fix.
- Around line 157-161: The vault migration script currently prints the failure
count in its completion summary but still exits successfully even when some
secrets fail to migrate. Update the end of vault-migrate-secrets.sh so the main
migration flow returns a non-zero status whenever failed is greater than zero,
keeping the existing summary output in place. Use the script’s final
completion/exit logic near the Migration complete message and the
migrated/failed counters to ensure automation can detect partial migration
failures.
- Around line 43-50: The SSH helpers src_vault and dst_vault are interpolating
sensitive values and user-controlled paths directly into remote shell strings,
which exposes tokens and makes the commands injectable. Update src_vault,
dst_vault, and the call sites that use KV_MOUNT and key so they pass arguments
safely as quoted positional parameters rather than string-expanded fragments,
keeping SRC_TOKEN and DST_TOKEN out of the command text and preventing crafted
mount/key values from altering the remote command.
- Line 59: The secret listing logic in src_vault within vault-migrate-secrets.sh
is swallowing all failures by using a blanket return 0 after kv list, which can
hide real Vault or transport errors. Update the list_output handling so only the
explicit empty/not-found case is treated as “no secrets,” and let other non-zero
exits propagate so the caller can fail appropriately; keep the fix localized
around the kv list invocation and the surrounding migration flow that consumes
list_output.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7055f95d-b710-49b4-9d7f-16a0dada48dc
📒 Files selected for processing (3)
scripts/machine-init.shvault/scripts/vault-migrate-secrets.shvault/scripts/vault-setup.sh
| # Run a vault command on the source host. | ||
| src_vault() { | ||
| ssh "$SRC_SSH" "VAULT_ADDR=http://127.0.0.1:8200 VAULT_TOKEN='${SRC_TOKEN}' vault $*" | ||
| } | ||
|
|
||
| # Run a vault command on the destination host. | ||
| dst_vault() { | ||
| ssh "$DST_SSH" "VAULT_ADDR=http://127.0.0.1:8200 VAULT_TOKEN='${DST_TOKEN}' vault $*" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the target file and inspect the relevant sections with line numbers.
git ls-files 'vault/scripts/vault-migrate-secrets.sh'
wc -l vault/scripts/vault-migrate-secrets.sh
cat -n vault/scripts/vault-migrate-secrets.sh | sed -n '1,220p'Repository: osac-project/osac-test-infra
Length of output: 6397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect shell usage patterns and the referenced line ranges in the script.
rg -n "src_vault|dst_vault|KV_MOUNT|SRC_TOKEN|DST_TOKEN|ssh |vault " vault/scripts/vault-migrate-secrets.shRepository: osac-project/osac-test-infra
Length of output: 1661
Avoid interpolating tokens and secret paths into SSH command strings. SRC_TOKEN, DST_TOKEN, KV_MOUNT, and key are expanded into the remote shell at lines 45, 50, 59, and 136-147, which exposes root tokens in process args and lets crafted mount/key values break or inject the remote command. Pass fixed commands with quoted positional arguments instead.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 45-45: Note that, unescaped, this expands on the client side.
(SC2029)
[info] 50-50: Note that, unescaped, this expands on the client side.
(SC2029)
🤖 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 `@vault/scripts/vault-migrate-secrets.sh` around lines 43 - 50, The SSH helpers
src_vault and dst_vault are interpolating sensitive values and user-controlled
paths directly into remote shell strings, which exposes tokens and makes the
commands injectable. Update src_vault, dst_vault, and the call sites that use
KV_MOUNT and key so they pass arguments safely as quoted positional parameters
rather than string-expanded fragments, keeping SRC_TOKEN and DST_TOKEN out of
the command text and preventing crafted mount/key values from altering the
remote command.
Sources: Coding guidelines, Linters/SAST tools
| local path="$1" | ||
| local list_output key | ||
|
|
||
| list_output=$(src_vault "kv list -format=json '${KV_MOUNT}/${path}'" 2>/dev/null) || return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant section with line numbers.
git ls-files vault/scripts/vault-migrate-secrets.sh
wc -l vault/scripts/vault-migrate-secrets.sh
sed -n '1,140p' vault/scripts/vault-migrate-secrets.sh | cat -nRepository: osac-project/osac-test-infra
Length of output: 5641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the remainder of the script around key enumeration and error handling.
sed -n '116,160p' vault/scripts/vault-migrate-secrets.sh | cat -n
# Search for any explicit handling of list failures or "empty/not-found" cases.
rg -n "list_recursive|kv list|not found|empty|return 0|return 1" vault/scripts/vault-migrate-secrets.shRepository: osac-project/osac-test-infra
Length of output: 1995
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '116,160p' vault/scripts/vault-migrate-secrets.sh | cat -n
echo '---'
rg -n "list_recursive|kv list|not found|empty|return 0|return 1" vault/scripts/vault-migrate-secrets.shRepository: osac-project/osac-test-infra
Length of output: 1999
Handle Vault list failures separately vault/scripts/vault-migrate-secrets.sh:59
|| return 0 turns permission errors, missing mounts, SSH failures, and Vault CLI errors into “no secrets”, so the migration can silently skip data and still report success. Only suppress the explicit empty/not-found case; otherwise propagate a non-zero exit.
🤖 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 `@vault/scripts/vault-migrate-secrets.sh` at line 59, The secret listing logic
in src_vault within vault-migrate-secrets.sh is swallowing all failures by using
a blanket return 0 after kv list, which can hide real Vault or transport errors.
Update the list_output handling so only the explicit empty/not-found case is
treated as “no secrets,” and let other non-zero exits propagate so the caller
can fail appropriately; keep the fix localized around the kv list invocation and
the surrounding migration flow that consumes list_output.
|
💀 CI Triage: Root cause: Helm upgrade fails because the new osac-operator chart uses valueFrom for OSAC_AAP_TOKEN, which conflicts with the literal value in the snapshot's deployment. Explanation: PR #328 in osac-installer updated the CI Helm values to use Evidence: Suggestion: Fix the upgrade path in the osac-operator Helm chart by explicitly setting Prow job | Build For deeper investigation, use the |
42eb839 to
0db4153
Compare
4b301c2 to
5ea5c38
Compare
|
💀 CI Triage: Root cause: Helm upgrade fails during cluster boot because strategic merge patch merges the new 'valueFrom' field into the existing 'osac-operator' deployment's 'OSAC_AAP_TOKEN' env var without clearing the old 'value' field, resulting in an invalid Kubernetes deployment spec. Explanation: Following the merge of osac-installer PR #328 and PR #329, the CI Helm values (values/vmaas-ci/values.yaml) were updated to configure 'operator.aap.tokenSecret' to read the AAP token from a secret. This causes the new Helm chart to render the 'OSAC_AAP_TOKEN' env var using 'valueFrom.secretKeyRef'. However, the pre-built cluster snapshot (vmaas-helm) contains an older deployment of 'osac-operator' where 'OSAC_AAP_TOKEN' is defined as a literal 'value: ""'. During the boot step's refresh phase, 'refresh-after-snapshot.py' runs 'helm upgrade', which attempts to apply a strategic merge patch to the existing deployment. Because strategic merge patch does not automatically unset the existing 'value' field, the merged env var ends up with both 'value' and 'valueFrom' set, which is rejected by the Kubernetes API server. Evidence: Suggestion: To fix this, either: (1) Update the osac-operator Helm chart deployment template (charts/operator/templates/deployment.yaml) to explicitly set 'value: null' when 'tokenSecret' is used so that strategic merge patch clears the existing 'value' field; (2) Update 'refresh-after-snapshot.py' in osac-installer to delete the existing 'osac-operator' deployment before running 'helm upgrade' to force a clean recreation; or (3) Rebuild and publish new cluster snapshot flavors with the updated Helm chart. Prow job | Build For deeper investigation, use the |
5ea5c38 to
fa4c44d
Compare
fa4c44d to
05c4b84
Compare
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 `@scripts/machine-init.sh`:
- Around line 410-413: The symlink setup in machine-init.sh is deleting the
existing root-owned cluster-tool config before migrating its contents, which can
lose state on reruns. Update the initialization flow around the ROOT_CT_DIR
replacement to first detect and copy any existing config into the runner-owned
CT_CONFIG_DIR, preserving state.json, keys, and server data, and only then
remove the old directory and create the symlink. Keep the change localized to
the config migration block that currently runs rm -rf, mkdir -p /root/.config,
and ln -sfn.
In `@vault/scripts/vault-setup.sh`:
- Around line 232-237: The AppRole setup for osac-e2e leaves the SecretID
effectively unlimited because secret_id_num_uses and secret_id_ttl are set to
zero. Update the vault write auth/approle/role/osac-e2e configuration to bound
SecretID reuse and lifetime, using the existing vault-setup.sh role definition
so the SecretID can’t be reused indefinitely from disk.
- Around line 240-249: The AppRole credential setup in vault-setup.sh writes
ROLE_ID and SECRET_ID before tightening permissions, leaving a window where
existing files or the APPROLE_DIR may be readable. Update the
APPROLE_DIR/role-id/secret-id handling so vault setup for osac-e2e creates and
secures the directory and files before any write of the SecretID, using the same
APPROLE_DIR, ROLE_ID, and SECRET_ID flow while ensuring restrictive permissions
are applied first.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 86c57386-5ca4-4c43-ab6d-f3c2e13b4a8f
📒 Files selected for processing (3)
scripts/machine-init.shvault/scripts/vault-migrate-secrets.shvault/scripts/vault-setup.sh
| # Remove any pre-existing directory so the symlink can be created | ||
| rm -rf "${ROOT_CT_DIR}" | ||
| mkdir -p /root/.config | ||
| ln -sfn "${CT_CONFIG_DIR}" "${ROOT_CT_DIR}" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Migrate existing root config before replacing it.
On reruns from the old root-based layout, Line 411 deletes /root/.config/cluster-tool before preserving state.json, keys, or server data. Copy the existing config into the runner-owned directory before replacing it with the symlink.
Proposed fix
- # Remove any pre-existing directory so the symlink can be created
- rm -rf "${ROOT_CT_DIR}"
+ # Preserve config from the old root-owned layout before replacing it.
+ if [[ -d "${ROOT_CT_DIR}" ]]; then
+ cp -a "${ROOT_CT_DIR}/." "${CT_CONFIG_DIR}/"
+ chown -R "${RUNNER_USER}:${RUNNER_USER}" "${CT_CONFIG_DIR}"
+ elif [[ -e "${ROOT_CT_DIR}" ]]; then
+ echo "ERROR: ${ROOT_CT_DIR} exists and is not a directory or symlink" >&2
+ return 1
+ fi
+ rm -rf "${ROOT_CT_DIR}"
mkdir -p /root/.config
ln -sfn "${CT_CONFIG_DIR}" "${ROOT_CT_DIR}"🤖 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 `@scripts/machine-init.sh` around lines 410 - 413, The symlink setup in
machine-init.sh is deleting the existing root-owned cluster-tool config before
migrating its contents, which can lose state on reruns. Update the
initialization flow around the ROOT_CT_DIR replacement to first detect and copy
any existing config into the runner-owned CT_CONFIG_DIR, preserving state.json,
keys, and server data, and only then remove the old directory and create the
symlink. Keep the change localized to the config migration block that currently
runs rm -rf, mkdir -p /root/.config, and ln -sfn.
| vault write auth/approle/role/osac-e2e \ | ||
| token_policies="osac-e2e" \ | ||
| token_ttl=10m \ | ||
| token_max_ttl=30m \ | ||
| secret_id_num_uses=0 \ | ||
| secret_id_ttl=0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Bound the AppRole SecretID lifetime.
secret_id_num_uses=0 and secret_id_ttl=0 make the on-disk SecretID reusable indefinitely; a host read can keep minting tokens despite the short token TTL.
Proposed fix
+APPROLE_SECRET_ID_NUM_USES="${APPROLE_SECRET_ID_NUM_USES:-100}"
+APPROLE_SECRET_ID_TTL="${APPROLE_SECRET_ID_TTL:-24h}"
+
vault write auth/approle/role/osac-e2e \
token_policies="osac-e2e" \
token_ttl=10m \
token_max_ttl=30m \
- secret_id_num_uses=0 \
- secret_id_ttl=0
+ secret_id_num_uses="${APPROLE_SECRET_ID_NUM_USES}" \
+ secret_id_ttl="${APPROLE_SECRET_ID_TTL}"📝 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.
| vault write auth/approle/role/osac-e2e \ | |
| token_policies="osac-e2e" \ | |
| token_ttl=10m \ | |
| token_max_ttl=30m \ | |
| secret_id_num_uses=0 \ | |
| secret_id_ttl=0 | |
| APPROLE_SECRET_ID_NUM_USES="${APPROLE_SECRET_ID_NUM_USES:-100}" | |
| APPROLE_SECRET_ID_TTL="${APPROLE_SECRET_ID_TTL:-24h}" | |
| vault write auth/approle/role/osac-e2e \ | |
| token_policies="osac-e2e" \ | |
| token_ttl=10m \ | |
| token_max_ttl=30m \ | |
| secret_id_num_uses="${APPROLE_SECRET_ID_NUM_USES}" \ | |
| secret_id_ttl="${APPROLE_SECRET_ID_TTL}" |
🤖 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 `@vault/scripts/vault-setup.sh` around lines 232 - 237, The AppRole setup for
osac-e2e leaves the SecretID effectively unlimited because secret_id_num_uses
and secret_id_ttl are set to zero. Update the vault write
auth/approle/role/osac-e2e configuration to bound SecretID reuse and lifetime,
using the existing vault-setup.sh role definition so the SecretID can’t be
reused indefinitely from disk.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/machine-init.sh (1)
151-157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate the actual passwordless sudo contract.
A stale
/etc/sudoers.d/${RUNNER_USER}can exist but not grantNOPASSWD, and verification still passes. Always install/validate the intended sudoers rule and verify with a non-interactive sudo check.Proposed fix
- if [[ ! -f "${SUDOERS_FILE}" ]]; then - echo "${RUNNER_USER} ALL=(ALL) NOPASSWD: ALL" > "${SUDOERS_FILE}" - chmod 0440 "${SUDOERS_FILE}" - echo " Passwordless sudo configured." - fi + SUDOERS_TMP=$(mktemp) + printf '%s ALL=(ALL) NOPASSWD: ALL\n' "${RUNNER_USER}" > "${SUDOERS_TMP}" + visudo -cf "${SUDOERS_TMP}" >/dev/null + install -m 0440 "${SUDOERS_TMP}" "${SUDOERS_FILE}" + rm -f "${SUDOERS_TMP}" + echo " Passwordless sudo configured."- if [[ ! -f "/etc/sudoers.d/${RUNNER_USER}" ]]; then - printf " %-20s FAILED — sudoers file missing\n" "${RUNNER_USER}:" + if ! sudo -n -u "${RUNNER_USER}" sudo -n true 2>/dev/null; then + printf " %-20s FAILED — passwordless sudo unavailable\n" "${RUNNER_USER}:" (( failures++ )) || true fiAlso applies to: 511-513
🤖 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 `@scripts/machine-init.sh` around lines 151 - 157, The passwordless sudo setup in machine-init.sh only checks whether SUDOERS_FILE exists, so a stale sudoers entry can bypass the intended NOPASSWD rule. Update the logic around the RUNNER_USER sudoers file to always ensure the exact "ALL=(ALL) NOPASSWD: ALL" rule is installed or replaced, then validate it with a non-interactive sudo check before continuing. Use the existing SUDOERS_FILE and RUNNER_USER flow so the fix applies consistently wherever this block is used.
♻️ Duplicate comments (1)
scripts/machine-init.sh (1)
362-365: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winReject unsafe
DATA_PATHbefore privileged ownership changes.
DATA_PATHcan come from${CT_CONFIG_DIR}/config; if it is/,/etc,/root, or a symlinked system path, this root-run script creates/chowns host directories. Normalize and reject unsafe roots beforemkdir/chown.Proposed fix
# Create data directories and ensure runner user owns them + DATA_PATH=$(realpath -m -- "${DATA_PATH}") + case "${DATA_PATH}" in + /|/etc|/etc/*|/usr|/usr/*|/bin|/bin/*|/sbin|/sbin/*|/boot|/boot/*|/root|/root/*) + echo "ERROR: Refusing unsafe DATA_PATH: ${DATA_PATH}" >&2 + exit 1 + ;; + esac mkdir -p "${DATA_PATH}/flavors" "${DATA_PATH}/overlays" "${DATA_PATH}/tmp" "${DATA_PATH}/containers/storage"🤖 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 `@scripts/machine-init.sh` around lines 362 - 365, The data-directory setup in machine-init.sh should validate DATA_PATH before the mkdir/chown block to avoid changing ownership of unsafe host locations. Normalize the value loaded from config, reject root or sensitive system paths like /, /etc, /root, and symlinked targets, then only proceed with the existing directory creation and ownership changes in the data path setup section. Use the DATA_PATH handling and the create-data-directories/chown logic to place the safeguard immediately before the privileged filesystem operations.
🤖 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 `@docs/new-ci-machine-setup.md`:
- Around line 141-165: The verification steps using cluster-tool are still
missing the prerequisite symlink/setup, so move the github-runner cluster-tool
config link before the “Verify” section and before any pull/boot/destroy
commands. Update the new-ci-machine-setup guide around the cluster-tool
instructions so /root/.config/cluster-tool points to the runner-owned config
before verification begins, using the existing cluster-tool
setup/troubleshooting steps as the reference point.
- Around line 55-61: The setup steps use a relative repo path that the
`github-runner` user cannot access after switching users, so update the
instructions around the `su - github-runner` and `vault-setup.sh` steps to use a
shared or absolute checkout location that the runner account can reach. Adjust
the earlier clone/check-out command and the subsequent `cd` in the setup flow so
they reference the same accessible path instead of a root-owned directory.
In `@scripts/machine-init.sh`:
- Around line 407-409: The symlink check in the machine-init flow is too
permissive: `ROOT_CT_DIR` is treated as valid whenever it exists as a link, even
if it is broken or points to an outdated target. Update the
`scripts/machine-init.sh` logic around the `ROOT_CT_DIR` handling to verify the
symlink target is still correct and repair/recreate it when it is stale, not
only when it is missing; keep the fix localized to the existing `if [[ -L
"${ROOT_CT_DIR}" ]]` block and related root config setup.
---
Outside diff comments:
In `@scripts/machine-init.sh`:
- Around line 151-157: The passwordless sudo setup in machine-init.sh only
checks whether SUDOERS_FILE exists, so a stale sudoers entry can bypass the
intended NOPASSWD rule. Update the logic around the RUNNER_USER sudoers file to
always ensure the exact "ALL=(ALL) NOPASSWD: ALL" rule is installed or replaced,
then validate it with a non-interactive sudo check before continuing. Use the
existing SUDOERS_FILE and RUNNER_USER flow so the fix applies consistently
wherever this block is used.
---
Duplicate comments:
In `@scripts/machine-init.sh`:
- Around line 362-365: The data-directory setup in machine-init.sh should
validate DATA_PATH before the mkdir/chown block to avoid changing ownership of
unsafe host locations. Normalize the value loaded from config, reject root or
sensitive system paths like /, /etc, /root, and symlinked targets, then only
proceed with the existing directory creation and ownership changes in the data
path setup section. Use the DATA_PATH handling and the
create-data-directories/chown logic to place the safeguard immediately before
the privileged filesystem operations.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 77e3d1b3-1825-4597-8ae1-8a91fd368556
📒 Files selected for processing (5)
.github/workflows/e2e-full-install.yml.github/workflows/e2e.ymldocs/new-ci-machine-setup.mdscripts/machine-init.shvault/scripts/vault-migrate-secrets.sh
| Switch to the `github-runner` user and run the Vault setup script: | ||
|
|
||
| ```bash | ||
| su - github-runner | ||
| cd osac-test-infra | ||
| ./vault/scripts/vault-setup.sh | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use a repo path the runner user can actually reach.
After su - github-runner, cd osac-test-infra will fail if the checkout was cloned as root in step 1, because that repo lives under /root. Move the checkout to a shared location or use an absolute path before switching users.
Suggested fix
-SSH into the new machine as root and clone the repo:
+SSH into the new machine and clone the repo into a shared location:
```bash
-git clone https://github.com/osac-project/osac-test-infra.git
-cd osac-test-infra
+git clone https://github.com/osac-project/osac-test-infra.git /opt/osac-test-infra
+cd /opt/osac-test-infra
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @docs/new-ci-machine-setup.md around lines 55 - 61, The setup steps use a
relative repo path that the github-runner user cannot access after switching
users, so update the instructions around the su - github-runner and
vault-setup.sh steps to use a shared or absolute checkout location that the
runner account can reach. Adjust the earlier clone/check-out command and the
subsequent cd in the setup flow so they reference the same accessible path
instead of a root-owned directory.
</details>
<!-- fingerprinting:phantom:triton:quartz -->
<!-- cr-indicator-types:potential_issue -->
<!-- cr-comment:v1:3d549036dbaa80696b3c5fec -->
<!-- This is an auto-generated comment by CodeRabbit -->
| As `github-runner`, pull the required cluster flavors: | ||
|
|
||
| ```bash | ||
| su - github-runner | ||
| sudo python3 /usr/local/bin/cluster-tool pull quay.io/rh-ee-ovishlit/cluster-flavors:vmaas-helm | ||
| ``` | ||
|
|
||
| This creates the `state.json` file that the e2e workflow checks during its preflight. | ||
|
|
||
| ### 8. Verify | ||
|
|
||
| ```bash | ||
| # Check runner services are active | ||
| sudo systemctl status 'actions.runner.osac-project-*' | ||
|
|
||
| # Check runner logs | ||
| sudo journalctl -u 'actions.runner.osac-project-*' -f | ||
|
|
||
| # Verify in GitHub UI | ||
| # https://github.com/organizations/osac-project/settings/actions/runners | ||
|
|
||
| # Test a cluster boot | ||
| sudo python3 /usr/local/bin/cluster-tool boot --flavor vmaas-helm --name test-1 | ||
| KUBECONFIG=~/.kube/test-1.kubeconfig oc get nodes | ||
| sudo python3 /usr/local/bin/cluster-tool destroy test-1 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Move the cluster-tool symlink prerequisite before verification.
The sudo python3 /usr/local/bin/cluster-tool pull/boot checks in the verification section still need /root/.config/cluster-tool to point at the runner-owned config. Right now that setup only appears later in troubleshooting, so a fresh machine can fail before the reader reaches the fix.
Suggested fix
+sudo mkdir -p /root/.config
+sudo ln -sfn /home/github-runner/.config/cluster-tool /root/.config/cluster-toolAlso applies to: 184-195
🤖 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 `@docs/new-ci-machine-setup.md` around lines 141 - 165, The verification steps
using cluster-tool are still missing the prerequisite symlink/setup, so move the
github-runner cluster-tool config link before the “Verify” section and before
any pull/boot/destroy commands. Update the new-ci-machine-setup guide around the
cluster-tool instructions so /root/.config/cluster-tool points to the
runner-owned config before verification begins, using the existing cluster-tool
setup/troubleshooting steps as the reference point.
| if [[ -L "${ROOT_CT_DIR}" ]]; then | ||
| echo " Symlink ${ROOT_CT_DIR} already exists." | ||
| else |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Repair stale root symlinks, not just missing ones.
Line 407 treats any symlink as valid. A broken or old /root/.config/cluster-tool symlink will keep workflows reading the wrong config path.
Proposed fix
ROOT_CT_DIR="/root/.config/cluster-tool"
if [[ -L "${ROOT_CT_DIR}" ]]; then
- echo " Symlink ${ROOT_CT_DIR} already exists."
+ CURRENT_TARGET=$(readlink -f -- "${ROOT_CT_DIR}" 2>/dev/null || true)
+ DESIRED_TARGET=$(readlink -f -- "${CT_CONFIG_DIR}")
+ if [[ "${CURRENT_TARGET}" == "${DESIRED_TARGET}" ]]; then
+ echo " Symlink ${ROOT_CT_DIR} already exists."
+ else
+ ln -sfn "${CT_CONFIG_DIR}" "${ROOT_CT_DIR}"
+ echo " Repaired symlink ${ROOT_CT_DIR} -> ${CT_CONFIG_DIR}"
+ fi
else📝 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.
| if [[ -L "${ROOT_CT_DIR}" ]]; then | |
| echo " Symlink ${ROOT_CT_DIR} already exists." | |
| else | |
| if [[ -L "${ROOT_CT_DIR}" ]]; then | |
| CURRENT_TARGET=$(readlink -f -- "${ROOT_CT_DIR}" 2>/dev/null || true) | |
| DESIRED_TARGET=$(readlink -f -- "${CT_CONFIG_DIR}") | |
| if [[ "${CURRENT_TARGET}" == "${DESIRED_TARGET}" ]]; then | |
| echo " Symlink ${ROOT_CT_DIR} already exists." | |
| else | |
| ln -sfn "${CT_CONFIG_DIR}" "${ROOT_CT_DIR}" | |
| echo " Repaired symlink ${ROOT_CT_DIR} -> ${CT_CONFIG_DIR}" | |
| fi | |
| else |
🤖 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 `@scripts/machine-init.sh` around lines 407 - 409, The symlink check in the
machine-init flow is too permissive: `ROOT_CT_DIR` is treated as valid whenever
it exists as a link, even if it is broken or points to an outdated target.
Update the `scripts/machine-init.sh` logic around the `ROOT_CT_DIR` handling to
verify the symlink target is still correct and repair/recreate it when it is
stale, not only when it is missing; keep the fix localized to the existing `if
[[ -L "${ROOT_CT_DIR}" ]]` block and related root config setup.
01005b4 to
c869647
Compare
c869647 to
f2a3698
Compare
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 @.github/workflows/e2e-vmaas.yml:
- Around line 243-245: The haproxy cleanup step in the e2e-vmaas workflow
mutates shared haproxy state without any serialization, which can race with
other concurrent jobs. Update the cleanup-haproxy invocation to run under the
same locking scheme used elsewhere in this job, such as FLAVOR_LOCK or a
dedicated haproxy lock, so it cannot overlap with boot or other config-writing
steps. Keep the existing cleanup-haproxy command but wrap it in the lock logic
used by the other shared-state mutations in this workflow.
In `@docs/new-ci-machine-setup.md`:
- Around line 77-83: The migration step is missing the required repo checkout
context for vault-migrate-secrets.sh, so update the instructions to explicitly
tell the reader to run it from the repository root (or use the absolute path to
the checked-out script). Keep the example around
vault/scripts/vault-migrate-secrets.sh but make the needed checkout location
clear so the source-host and new-host commands work regardless of the current
directory.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: c8160a1e-d4d9-4213-b0ed-bd6792808676
📒 Files selected for processing (5)
.github/workflows/e2e-vmaas.ymldocs/new-ci-machine-setup.mdscripts/machine-init.shvault/scripts/vault-migrate-secrets.shvault/scripts/vault-setup.sh
| # Remove haproxy entries for VMs that no longer exist | ||
| sudo python3 /usr/local/bin/cluster-tool cleanup-haproxy 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded haproxy cleanup races with concurrent jobs on the shared runner.
This step mutates the shared /etc/haproxy/haproxy.cfg outside of any flock, while every other shared-state mutation in this same job (flavor pull/delete/boot) is deliberately guarded by FLAVOR_LOCK to serialize concurrent jobs on osac-ci. If cleanup-haproxy runs concurrently with another job's boot (which also touches haproxy config for its own clone), the cleanup could race with that write — removing/corrupting a backend entry for a VM that's mid-creation, or clobbering config while another process is writing it.
Consider serializing this with the existing lock (or a dedicated haproxy lock) rather than leaving it unguarded:
🔒 Suggested fix
- # Remove haproxy entries for VMs that no longer exist
- sudo python3 /usr/local/bin/cluster-tool cleanup-haproxy 2>/dev/null || true
+ # Remove haproxy entries for VMs that no longer exist. Serialize with
+ # other jobs since this mutates the shared haproxy config.
+ exec 9>"${FLAVOR_LOCK}"
+ flock --exclusive 9
+ sudo python3 /usr/local/bin/cluster-tool cleanup-haproxy 9>&- 2>/dev/null || true
+ exec 9>&-📝 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.
| # Remove haproxy entries for VMs that no longer exist | |
| sudo python3 /usr/local/bin/cluster-tool cleanup-haproxy 2>/dev/null || true | |
| # Remove haproxy entries for VMs that no longer exist. Serialize with | |
| # other jobs since this mutates the shared haproxy config. | |
| exec 9>"${FLAVOR_LOCK}" | |
| flock --exclusive 9 | |
| sudo python3 /usr/local/bin/cluster-tool cleanup-haproxy 9>&- 2>/dev/null || true | |
| exec 9>&- |
🤖 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 @.github/workflows/e2e-vmaas.yml around lines 243 - 245, The haproxy cleanup
step in the e2e-vmaas workflow mutates shared haproxy state without any
serialization, which can race with other concurrent jobs. Update the
cleanup-haproxy invocation to run under the same locking scheme used elsewhere
in this job, such as FLAVOR_LOCK or a dedicated haproxy lock, so it cannot
overlap with boot or other config-writing steps. Keep the existing
cleanup-haproxy command but wrap it in the lock logic used by the other
shared-state mutations in this workflow.
| Run from a machine that can SSH into both the source and destination hosts: | ||
|
|
||
| ```bash | ||
| ./vault/scripts/vault-migrate-secrets.sh \ | ||
| github-runner@<source-host> /path/to/source-vault-init.json \ | ||
| github-runner@<new-host> /path/to/new-vault-init.json | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Call out the required checkout location for the migration script.
./vault/scripts/vault-migrate-secrets.sh only works from this repo checkout, but the step never says to cd into it or use an absolute path. As written, this will fail on any host that is not already in the cloned tree.
🤖 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 `@docs/new-ci-machine-setup.md` around lines 77 - 83, The migration step is
missing the required repo checkout context for vault-migrate-secrets.sh, so
update the instructions to explicitly tell the reader to run it from the
repository root (or use the absolute path to the checked-out script). Keep the
example around vault/scripts/vault-migrate-secrets.sh but make the needed
checkout location clear so the source-host and new-host commands work regardless
of the current directory.
ygalblum
left a comment
ygalblum
left a comment
There was a problem hiding this comment.
/lgtm
/approve
I'll just say that an Ansible playbook would probably have been simpler than shell scripts
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz, ygalblum 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 |
…ault migrate script - Add runner-user step to machine-init.sh: creates github-runner user with libvirt group, passwordless sudo, and SSH authorized_keys - Fix cluster-tool config to target runner user's home instead of root's, resolving "No server specified" error when running as github-runner - Fix vault-setup.sh rootless Podman chown using podman unshare - Add vault-migrate-secrets.sh for copying KV v2 secrets between Vaults
Chown ~/.config itself (not just subdirs) so podman doesn't reject the parent directory when running as github-runner.
The e2e workflow uses AppRole (role-id + secret-id) to authenticate with Vault. Add phase 10 to enable AppRole auth, create the osac-e2e role, and write credentials to ~/.vault-server/.approle/.
The e2e workflow runs cluster-tool via sudo, which looks for config under /root/.config/cluster-tool/. Symlink it to the runner user's config so both sudo and non-sudo invocations work.
Remove orphaned haproxy entries (for VMs that no longer exist) before each boot to prevent config drift from killed/crashed jobs.
Set OOMScoreAdjust=-1000 on sshd so the machine stays reachable via SSH even when VMs or runners exhaust memory.
f2a3698 to
aedf907
Compare
|
New changes are detected. LGTM label has been removed. |
|
/retest |
|
No failed workflow runs found for this PR at commit |
This branch was previously deployed
4 participants
Summary
runner-userstep tomachine-init.sh: createsgithub-runneruser with libvirt group, passwordless sudo, and SSH authorized_keys from root~github-runnerinstead of~root, resolving "No server specified" errorvault-setup.shrootless Podman chown usingpodman unsharevault-migrate-secrets.shfor copying KV v2 secrets between Vault instances over SSHTest plan
sudo ./scripts/machine-init.sh runner-useron a new CI server and verify user creationsudo ./scripts/machine-init.sh cluster-tooland verify config lands under/home/github-runner/.config/cluster-tool/github-runner, runcluster-tool pulland confirm no "No server specified" errorvault-setup.shas non-root and verifypodman unshare chownsucceedsvault-migrate-secrets.shbetween two Vault instancesSummary by CodeRabbit
New Features
Bug Fixes