Repository navigation
Remove e2e-vault-secrets.yml and point caller to e2e.yml - #74
Merged
Merged
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eliorerz 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 |
|
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:
WalkthroughVault authentication in the E2E test infrastructure is switched from GitHub OIDC/JWT to Vault AppRole. ChangesJWT to AppRole migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 |
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 20:26
982caa1 to
1361e87
Compare
eliorerz
changed the title
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 20:38
1361e87 to
f65014a
Compare
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 20:41
f65014a to
1e5cad2
Compare
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 20:51
1e5cad2 to
9a5555f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-vault-secrets.yml (1)
102-119: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueQuote
FILTER_ARGSto handle special characters safely.If
TEST_FILTERcontains spaces or shell metacharacters, the unquoted expansion could cause unexpected behavior. While pytest's-ktypically uses simple patterns, quoting is safer.Proposed fix
- pytest ${TEST_ARGS} ${FILTER_ARGS} \ + pytest ${TEST_ARGS} "${FILTER_ARGS}" \🤖 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-vault-secrets.yml around lines 102 - 119, The FILTER_ARGS variable is passed unquoted to the pytest command in the podman run container, which could cause issues if TEST_FILTER contains spaces or special characters. Quote the FILTER_ARGS variable expansion in the pytest invocation to ensure it is safely passed as a single argument, even when it contains special characters or whitespace. Change the pytest command line where FILTER_ARGS is used to wrap it with double quotes like "${FILTER_ARGS}".
🤖 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-vault-secrets.yml:
- Around line 42-43: The actions/checkout action in the Checkout repository step
is pinned to a version tag (v4) instead of a full commit SHA, which violates
security guidelines. Replace the `@v4` reference with the full commit SHA for
the corresponding version of actions/checkout. Additionally, add a `with`
section to the step that sets `persist-credentials: false` to prevent Git
credentials from being stored and reduce the attack surface after checkout
completes.
In `@vault/scripts/vault-setup.sh`:
- Around line 191-196: The vault write command configuring the osac-e2e role
with secret_id_ttl="0" means the secret ID never expires, creating a persistent
security risk if compromised. Add a comment above or inline with the vault write
auth/approle/role/osac-e2e command block that explains this setting is
intentional for persistent runner credentials and documents the security
trade-off of indefinite secret ID validity, clarifying why the non-expiring
credential is necessary despite the associated risk.
---
Outside diff comments:
In @.github/workflows/e2e-vault-secrets.yml:
- Around line 102-119: The FILTER_ARGS variable is passed unquoted to the pytest
command in the podman run container, which could cause issues if TEST_FILTER
contains spaces or special characters. Quote the FILTER_ARGS variable expansion
in the pytest invocation to ensure it is safely passed as a single argument,
even when it contains special characters or whitespace. Change the pytest
command line where FILTER_ARGS is used to wrap it with double quotes like
"${FILTER_ARGS}".
🪄 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: ee8beb14-0f03-4298-9828-f6258f358d43
📒 Files selected for processing (4)
.github/workflows/e2e-test.yml.github/workflows/e2e-vault-secrets.ymlvault/scripts/vault-health-check.shvault/scripts/vault-setup.sh
Comment on lines
+191
to
+196
| vault write auth/approle/role/osac-e2e \ | ||
| token_policies="osac-e2e" \ | ||
| token_ttl="10m" \ | ||
| token_max_ttl="30m" \ | ||
| secret_id_ttl="0" \ | ||
| token_num_uses=0 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
secret_id_ttl="0" means the secret_id never expires.
This is presumably intentional for a persistent runner credential, but it does mean a compromised secret_id remains valid indefinitely. Consider documenting this trade-off or implementing periodic rotation.
🤖 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 191 - 196, The vault write command
configuring the osac-e2e role with secret_id_ttl="0" means the secret ID never
expires, creating a persistent security risk if compromised. Add a comment above
or inline with the vault write auth/approle/role/osac-e2e command block that
explains this setting is intentional for persistent runner credentials and
documents the security trade-off of indefinite secret ID validity, clarifying
why the non-expiring credential is necessary despite the associated risk.
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 21:05
9a5555f to
801d792
Compare
eliorerz
changed the title
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 22:51
801d792 to
63b7f45
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-vault-secrets.yml:
- Around line 155-158: After setting the KUBECONFIG_PATH variable using the
CLONE_NAME, add a guard condition to verify that the kubeconfig file actually
exists at the specified path before proceeding with the test execution. If the
file does not exist, the workflow should exit with an appropriate error message.
This will prevent confusing mount errors during the podman run command and
provide clear feedback if the kubeconfig path is incorrect or the boot step
fails silently.
In `@vault/scripts/vault-setup.sh`:
- Around line 205-211: The credential setup in vault-setup.sh lacks atomicity
between the role-id and secret-id operations. If the vault read command for
role-id succeeds but the vault write command for secret-id fails, the state
becomes inconsistent with only a role-id file present. Add error handling after
each vault command to check for failures and exit if either operation fails, or
alternatively write both credentials to temporary files first, verify both
succeeded, then atomically move them to the APPROLE_DIR. This ensures the
directory either has both credentials or neither, preventing partial failures.
🪄 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: a0d94250-50b3-47c8-8f8d-3b284c25c2db
📒 Files selected for processing (4)
.github/workflows/e2e-test.yml.github/workflows/e2e-vault-secrets.ymlvault/scripts/vault-health-check.shvault/scripts/vault-setup.sh
Comment on lines
+205
to
+211
| vault read -format=json auth/approle/role/osac-e2e/role-id \ | ||
| | jq -r '.data.role_id' > "${APPROLE_DIR}/role-id" | ||
| chmod 600 "${APPROLE_DIR}/role-id" | ||
|
|
||
| vault write -format=json -f auth/approle/role/osac-e2e/secret-id \ | ||
| | jq -r '.data.secret_id' > "${APPROLE_DIR}/secret-id" | ||
| chmod 600 "${APPROLE_DIR}/secret-id" |
There was a problem hiding this comment.
Credential storage lacks atomicity - partial failure leaves inconsistent state.
If vault read ... role-id succeeds but vault write ... secret-id fails, you'll have a role-id file but no secret-id, causing downstream auth failures. Consider writing to temp files first, then moving atomically, or adding error handling.
Proposed fix with error handling
-vault read -format=json auth/approle/role/osac-e2e/role-id \
- | jq -r '.data.role_id' > "${APPROLE_DIR}/role-id"
-chmod 600 "${APPROLE_DIR}/role-id"
-
-vault write -format=json -f auth/approle/role/osac-e2e/secret-id \
- | jq -r '.data.secret_id' > "${APPROLE_DIR}/secret-id"
-chmod 600 "${APPROLE_DIR}/secret-id"
+ROLE_ID=$(vault read -format=json auth/approle/role/osac-e2e/role-id | jq -r '.data.role_id')
+SECRET_ID=$(vault write -format=json -f auth/approle/role/osac-e2e/secret-id | jq -r '.data.secret_id')
+
+printf '%s' "${ROLE_ID}" > "${APPROLE_DIR}/role-id"
+chmod 600 "${APPROLE_DIR}/role-id"
+
+printf '%s' "${SECRET_ID}" > "${APPROLE_DIR}/secret-id"
+chmod 600 "${APPROLE_DIR}/secret-id"🤖 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 205 - 211, The credential setup in
vault-setup.sh lacks atomicity between the role-id and secret-id operations. If
the vault read command for role-id succeeds but the vault write command for
secret-id fails, the state becomes inconsistent with only a role-id file
present. Add error handling after each vault command to check for failures and
exit if either operation fails, or alternatively write both credentials to
temporary files first, verify both succeeded, then atomically move them to the
APPROLE_DIR. This ensures the directory either has both credentials or neither,
preventing partial failures.
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 18, 2026 23:10
63b7f45 to
e4cf341
Compare
eliorerz
changed the title
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 21, 2026 04:32
214dff1 to
bffb0eb
Compare
Contributor
|
/retest |
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 21, 2026 16:13
bffb0eb to
5ef821b
Compare
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 21, 2026 16:16
5ef821b to
8c45896
Compare
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 21, 2026 16:39
8c45896 to
1087fba
Compare
The reusable workflow was added as e2e.yml in PR osac-project#76. Delete the old e2e-vault-secrets.yml and update e2e-test.yml to reference it.
eliorerz
force-pushed
the
fix-e2e-permissions
branch
from
June 21, 2026 17:06
1087fba to
afae15d
Compare
eliorerz
changed the title
This was referenced Jun 23, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.github/workflows/e2e-vault-secrets.yml(superseded bye2e.ymlfrom PR OSAC-1568: Add reusable e2e workflow #76)e2e-test.ymlto referencee2e.yml