secret manager fix: mount files into container for -f/--from-file#80914
Open
psalajova wants to merge 1 commit into
Open
secret manager fix: mount files into container for -f/--from-file#80914psalajova wants to merge 1 commit into
psalajova wants to merge 1 commit into
Conversation
openshift-merge-bot
Bot
added
the
rehearsals-ack
Contributor
|
[REHEARSALNOTIFIER] Note: If this PR includes changes to step registry files ( |
Contributor
Walkthrough
Changes--from-file host-path mounting in secret-manager.sh
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: psalajova 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@hack/secret-manager.sh`:
- Around line 43-68: The file_mount variable is being overwritten instead of
appended to in both the -f|--from-file case and the --from-file=* case, which
means only the last file mount is preserved when multiple --from-file arguments
are provided. Additionally, the container path construction using
/input/$(basename ...) creates collisions when files with identical basenames
exist in different directories. Fix this by changing the file_mount assignments
from = to += to accumulate all mounts, and modify the container path logic to
use unique identifiers (such as a counter or index) or preserve relative
directory structure to avoid collisions between files with the same basename
from different source directories.
🪄 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: 6f07ee9e-33fd-4272-a32f-bdeb09e98782
📒 Files selected for processing (1)
hack/secret-manager.sh
Comment on lines
+43
to
+68
| file_mount=() | ||
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| -f|--from-file) | ||
| if [[ -n "${2:-}" ]]; then | ||
| host_path="$(resolve_path "$2")" | ||
| if [[ -f "$host_path" ]]; then | ||
| container_path="/input/$(basename "$host_path")" | ||
| file_mount=(-v "$host_path:$container_path:ro,z") | ||
| args+=("$1" "$container_path") | ||
| else | ||
| args+=("$1" "$2") | ||
| fi | ||
| shift 2 | ||
| else | ||
| args+=("$1") | ||
| shift | ||
| fi | ||
| ;; | ||
| --from-file=*) | ||
| file_arg="${1#--from-file=}" | ||
| host_path="$(resolve_path "$file_arg")" | ||
| if [[ -f "$host_path" ]]; then | ||
| container_path="/input/$(basename "$host_path")" | ||
| file_mount=(-v "$host_path:$container_path:ro,z") | ||
| args+=("--from-file=$container_path") |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
--from-file handling drops earlier mounts and can mis-map files with duplicate basenames.
file_mount is overwritten on each match, so only the last --from-file file is mounted. Also, mapping to /input/$(basename ...) can collide across different directories, causing wrong file selection.
Suggested fix
args=()
file_mount=()
+from_file_idx=0
while [[ $# -gt 0 ]]; do
case "$1" in
-f|--from-file)
if [[ -n "${2:-}" ]]; then
host_path="$(resolve_path "$2")"
if [[ -f "$host_path" ]]; then
- container_path="/input/$(basename "$host_path")"
- file_mount=(-v "$host_path:$container_path:ro,z")
+ from_file_idx=$((from_file_idx + 1))
+ container_path="/input/${from_file_idx}-$(basename "$host_path")"
+ file_mount+=(-v "$host_path:$container_path:ro,z")
args+=("$1" "$container_path")
else
args+=("$1" "$2")
fi
shift 2
@@
--from-file=*)
file_arg="${1#--from-file=}"
host_path="$(resolve_path "$file_arg")"
if [[ -f "$host_path" ]]; then
- container_path="/input/$(basename "$host_path")"
- file_mount=(-v "$host_path:$container_path:ro,z")
+ from_file_idx=$((from_file_idx + 1))
+ container_path="/input/${from_file_idx}-$(basename "$host_path")"
+ file_mount+=(-v "$host_path:$container_path:ro,z")
args+=("--from-file=$container_path")
else
args+=("$1")
fi
shiftAlso applies to: 86-89
🤖 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 `@hack/secret-manager.sh` around lines 43 - 68, The file_mount variable is
being overwritten instead of appended to in both the -f|--from-file case and the
--from-file=* case, which means only the last file mount is preserved when
multiple --from-file arguments are provided. Additionally, the container path
construction using /input/$(basename ...) creates collisions when files with
identical basenames exist in different directories. Fix this by changing the
file_mount assignments from = to += to accumulate all mounts, and modify the
container path logic to use unique identifiers (such as a counter or index) or
preserve relative directory structure to avoid collisions between files with the
same basename from different source directories.
Contributor
|
@psalajova: 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. |
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
1 participant
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.
The
-f/--from-fileflag passes a host path to the secret-manager tool running inside a container, where the path doesn't exist. Fixed by intercepting the flag, bind-mounting the file into the container, and rewriting the argument to the container path.Summary by CodeRabbit
This PR enhances the
hack/secret-manager.shscript, which is a wrapper for running the OpenShift CI secret-manager tool in a container. It fixes an issue where the-f/--from-fileflag would fail when users pass a host file path, since that path doesn't exist inside the container environment.Changes Made
The script now preprocesses CLI arguments before invoking the container:
Path Resolution: Adds a
resolve_path()helper function that converts relative file paths to absolute paths based on the current working directory.Argument Parsing: Implements a loop to parse and intercept both
-f|--from-file <path>and--from-file=<path>argument formats.Dynamic Bind-Mounting: When a file specified in the
--from-fileflag exists on the host:/input/<basename>as read-only with SELinux context binding (:ro,z)Argument Forwarding: Changes the container execution to use the parsed and rewritten argument list (
"${args[@]}") instead of the original arguments ("$@"), ensuring only the processed arguments are forwarded to the containerized tool.This allows the secret-manager tool to work seamlessly regardless of whether it's executed on the host or in a container, properly handling file inputs from the host environment.