fix flashing to internal registry#86
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds file-based OCI registry credential handling: CLI writes registry auth JSON to a temp file and displays its path; flash script forwards credentials by creating a temp auth file on the exporter and uses it for flashing; fixes a variable shadowing bug in the image build controller. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as caib CLI
participant Exporter as Exporter host
participant Registry as OCI Registry
CLI->>CLI: build OCI auth JSON (username: serviceaccount, token)
CLI->>Exporter: stream auth JSON via stdin -> write /tmp/.oci-auth/config.json
CLI->>Exporter: set REGISTRY_AUTH_FILE=/tmp/.oci-auth/config.json
CLI->>Exporter: run FLASH_CMD
Exporter->>Registry: authenticate using REGISTRY_AUTH_FILE and push/flash image
Exporter-->>CLI: return FLASH_EXIT (success/failure)
CLI->>Exporter: remove /tmp/.oci-auth directory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/common/tasks/scripts/flash_image.sh (1)
88-100:⚠️ Potential issue | 🟡 Minor
FLASH_EXIT=$?is dead code underset -e/pipefail.With
set -euo pipefail(line 2), ifjmp shellon line 80 or line 92 returns non-zero, the script exits immediately —FLASH_EXIT=$?is never reached. The error message on lines 97-99 will never be printed on failure.The script still behaves correctly in practice (the
cleanuptrap fires and releases the lease, and the non-zero exit propagates), but the explicit error handling is unreachable.If you want the explicit error message and
FLASH_SUCCESStracking to work:Proposed fix
# shellcheck disable=SC2086 - echo "${AUTH_JSON}" | jmp shell ${JMP_SHELL_ARGS} -- sh -c " + echo "${AUTH_JSON}" | jmp shell ${JMP_SHELL_ARGS} -- sh -c " mkdir -p /tmp/.oci-auth && \ cat > /tmp/.oci-auth/config.json && \ REGISTRY_AUTH_FILE=/tmp/.oci-auth/config.json ${FLASH_CMD}; \ EXIT_CODE=\$?; \ rm -rf /tmp/.oci-auth; \ exit \$EXIT_CODE - " - FLASH_EXIT=$? + " && FLASH_EXIT=0 || FLASH_EXIT=$? else # No credentials, run flash command directly # shellcheck disable=SC2086 - jmp shell ${JMP_SHELL_ARGS} -- ${FLASH_CMD} - FLASH_EXIT=$? + jmp shell ${JMP_SHELL_ARGS} -- ${FLASH_CMD} && FLASH_EXIT=0 || FLASH_EXIT=$? fi
🤖 Fix all issues with AI agents
In `@cmd/caib/main.go`:
- Around line 727-735: The temp credentials file written by
writeRegistryCredentialsFile (credsFile) contains a short-lived sensitive token
but is never removed; update the success branch that handles credsFile to either
register cleanup (e.g., schedule removal of credsFile on process exit or defer
os.Remove(credsFile) when appropriate) or, if the file must persist for
follow-up commands, at minimum print a clear one-line warning telling the user
to securely delete credsFile when finished and how long the token is valid; also
ensure writeRegistryCredentialsFile creates the file with restrictive
permissions (owner-only) to minimize exposure.
- Around line 249-272: The writeRegistryCredentialsFile function currently
embeds the token into JSON via fmt.Sprintf which can produce invalid JSON if the
token contains special chars; change the implementation to build the credentials
object and serialize it with encoding/json (e.g., json.Marshal on a struct or
map like {"username":"serviceaccount","token": token}) and write the marshaled
bytes to the temp file, preserving the existing error handling and permission
setting (Chmod); also update or remove the stale comment that mentions
extractRegistryCredentials so it accurately describes
writeRegistryCredentialsFile.
bennyz
force-pushed
the
fix-flashing-internal-registry
branch
2 times, most recently
from
8e03d21 to
23cef6d
Compare
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)
cmd/caib/main.go (1)
1997-2008:⚠️ Potential issue | 🟡 Minor
parseLeaseDurationsilently swallows malformed input.If the user passes
--lease "abc:def:ghi",fmt.Sscanffails silently and all components default to0, returning a zerotime.Duration. This could cause unexpected behavior downstream (e.g., a zero-duration lease or a very short flash timeout inwaitForFlashCompletionat line 2064).Consider validating the parsed values or at least logging a warning.
Suggested fix
func parseLeaseDuration(duration string) time.Duration { parts := strings.Split(duration, ":") if len(parts) != 3 { + fmt.Fprintf(os.Stderr, "Warning: invalid lease duration %q, using default 1h\n", duration) return time.Hour // Default 1 hour } - var hours, mins, secs int - _, _ = fmt.Sscanf(parts[0], "%d", &hours) - _, _ = fmt.Sscanf(parts[1], "%d", &mins) - _, _ = fmt.Sscanf(parts[2], "%d", &secs) - return time.Duration(hours)*time.Hour + time.Duration(mins)*time.Minute + time.Duration(secs)*time.Second + hours, err1 := strconv.Atoi(parts[0]) + mins, err2 := strconv.Atoi(parts[1]) + secs, err3 := strconv.Atoi(parts[2]) + if err1 != nil || err2 != nil || err3 != nil { + fmt.Fprintf(os.Stderr, "Warning: invalid lease duration %q, using default 1h\n", duration) + return time.Hour + } + return time.Duration(hours)*time.Hour + time.Duration(mins)*time.Minute + time.Duration(secs)*time.Second }
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 88-93: set -e causes the script to exit before assigning
FLASH_EXIT when jmp shell fails; wrap the jmp shell invocation in a temporary
errexit-disable so the exit code is captured and the error handling runs.
Specifically, before calling jmp shell (the branches that run "jmp shell
${JMP_SHELL_ARGS} -- ${FLASH_CMD}"), disable errexit (e.g. set +e), run jmp
shell, capture its exit into FLASH_EXIT, then restore errexit (set -e) so
subsequent logic (the error message and cleanup) executes as intended.
- Around line 74-76: AUTH_JSON is built by naive string interpolation and can
break if OCI_USERNAME or OCI_PASSWORD contain JSON-special characters; update
the construction around REGISTRY_HOST/AUTH_B64/AUTH_JSON to safely emit JSON by
either (a) using jq to create the object (e.g., pass OCI_USERNAME and
OCI_PASSWORD to jq to produce {"auths":{REGISTRY_HOST:{"auth":...}}}) or (b)
using a heredoc that injects the base64-auth value via a single safe command
substitution (mirroring the pattern used in build_image.sh/build_builder.sh) so
keys are not interpolated into a single quoted JSON string; modify the code that
sets AUTH_B64 and AUTH_JSON (and the related use of OCI_USERNAME/OCI_PASSWORD)
to adopt one of these safe methods.
23cef6d to
6bb0ae9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/common/tasks/scripts/flash_image.sh`:
- Around line 77-80: The flash_image.sh script sets AUTH_JSON using jq
(AUTH_JSON variable) which isn't available in the jumpstarter container; either
add jq to the flash task image build so jq is present at runtime, or replace the
jq invocation with a shell-safe JSON construction that inserts REGISTRY_HOST and
AUTH_B64 directly into the string (e.g., build '{"auths":{ "<REGISTRY_HOST>":
{"auth":"<AUTH_B64>"}}}') while properly quoting/escaping values; update the
code that references AUTH_JSON accordingly so the credential forwarding path
works without jq.
fix passing credentials, and put caib credentials in a file instead of showing in the output Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
fix-flashing-internal-registry
branch
from
6bb0ae9 to
d9b7469
Compare
1 participant
fix passing credentials, and put caib credentials in a file instead of showing in the output
Summary by CodeRabbit