flashers: add support for passing OCI credentials to fls#211
Conversation
📝 WalkthroughWalkthroughAdds OCI credential support to the flasher client: new CLI options/env vars for OCI username/password, credential validation, propagation of credentials to FLS via environment or temporary credential file, redaction of sensitive outputs, and tests and README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Client as FlasherClient.flash
participant Validator as _validate_oci_credentials
participant Performer as _perform_flash_operation
participant EnvBuilder as _fls_oci_auth_env / credential file setup
participant FLS as FLS (on DUT)
participant Console as Console/Redactor
User->>Client: flash(path, oci_username?, oci_password?)
Client->>Validator: validate credentials
Validator-->>Client: normalized creds or error
Client->>Performer: start flash operation (with creds)
Performer->>EnvBuilder: build env prefix or write cred file (if OCI)
EnvBuilder-->>Performer: env string / creds file path
Performer->>FLS: invoke FLS with env prefix / cred file
FLS-->>Console: progress output
Console->>Performer: redacted progress/status
Performer-->>Client: result
Client-->>User: operation complete / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
122-130:⚠️ Potential issue | 🔴 CriticalBug:
elif oci_username:breaks the URL-handling control flow for non-OCI paths.The new
elif oci_username:branch is inserted into theif/elif/elsechain that determines the download strategy (should_download_to_httpd,image_url). When a user accidentally supplies--oci-usernamewith an HTTP URL, this branch fires, clears the credentials, but skips the HTTP URL handling that would normally setimage_urlandshould_download_to_httpd = False. The result:image_urlstays""and the code falls through to the storage-transfer path with no operator or URL configured, causing a runtime failure.The credential-clearing logic should be a separate, standalone check that doesn't participate in the download-strategy chain.
🐛 Proposed fix: decouple credential check from download-strategy chain
if path.startswith("oci://"): # OCI URLs are always passed directly to fls image_url = path should_download_to_httpd = False - elif oci_username: - self.logger.warning("OCI credentials provided for non-OCI image path; ignoring credentials") - oci_username = None - oci_password = None - elif path.startswith(("http://", "https://")) and not force_exporter_http: + elif path.startswith(("http://", "https://")) and not force_exporter_http: # the flasher image can handle the http(s) from a remote directly, unless target is isolated image_url = path should_download_to_httpd = False else: # use the exporter's http server for the flasher image, we should download it first if operator is None: if path.startswith(("http://", "https://")) and bearer_token: parsed = urlparse(path) self.logger.info(f"Using Bearer token authentication for {parsed.netloc}") original_http_url = path operator = Operator( "http", root="/", endpoint=f"{parsed.scheme}://{parsed.netloc}", token=bearer_token ) operator_scheme = "http" path = Path(parsed.path) else: path, operator, operator_scheme = operator_for_path(path) image_url = self.http.get_url() + "/" + path.name + + if not path.startswith("oci://") and oci_username: + self.logger.warning("OCI credentials provided for non-OCI image path; ignoring credentials") + oci_username = None + oci_password = None
596-607:⚠️ Potential issue | 🟡 MinorBroad
except Exceptionre-wraps theFlashRetryableErrorraised for panics.If
"panicked at"is detected at line 598–599, the raisedFlashRetryableErroris immediately caught by theexcept Exceptionhandler at line 605, which wraps it in a newFlashRetryableErrorwith a generic message, losing the specific panic context.🐛 Proposed fix: catch FlashRetryableError before the generic handler, or restructure
except pexpect.EOF as err: # End of file - connection closed self.logger.error("Console connection closed unexpectedly") raise FlashRetryableError("Console connection closed during flash operation") from err + except FlashRetryableError: + raise except Exception as err: self.logger.error(f"Error monitoring FLS progress: {err}") raise FlashRetryableError(f"Error monitoring FLS progress: {err}") from err
🤖 Fix all issues with AI agents
In
`@python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py`:
- Around line 558-564: The flash command currently injects
FLS_REGISTRY_USERNAME/PASSWORD inline (built in _fls_oci_auth_env and flash_cmd)
and is sent via console.sendline, risking credential leakage to
console.logfile_read and _monitor_fls_progress prints; fix by removing inline
credentials: create a temp cred file on the target with strict perms (via
console.sendline: write file with username/password, chmod 600), change
_fls_oci_auth_env to source that file (e.g., "set -o allexport; source
/tmp/fls_creds; set +o allexport") so flash_cmd contains no cleartext creds, and
ensure any debug/log output that prints flash_cmd or fls_oci_auth_env (including
_monitor_fls_progress and any places setting console.logfile_read) redacts
sensitive values (replace username/password with "***") before logging.
Currently OCI credentials weren't passed, so it was not possible to work with private registries. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
oci-creds-flashers
branch
from
b4088bc to
a378c3e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (2)
628-638:⚠️ Potential issue | 🟠 Major
FlashRetryableErrorraised on panic is caught by the genericexcept Exceptionand double-wrapped.When
"panicked at"is detected on line 629, the raisedFlashRetryableErrorfalls throughexcept pexpect.EOFand is caught byexcept Exceptionon line 636, which wraps it in a secondFlashRetryableError. This distorts the error message for the retry/categorisation logic upstream.Proposed fix — re-raise `FlashRetryableError` before the generic handler
except pexpect.EOF as err: # End of file - connection closed self.logger.error("Console connection closed unexpectedly") raise FlashRetryableError("Console connection closed during flash operation") from err + except FlashRetryableError: + raise except Exception as err: self.logger.error(f"Error monitoring FLS progress: {err}") raise FlashRetryableError(f"Error monitoring FLS progress: {err}") from err
640-645:⚠️ Potential issue | 🟡 MinorMissing error handling for exit-code parsing after FLS completes.
Line 643 can raise
IndexError(empty output) orValueError(non-numeric line) without a guard. Compare with the defensive handling in_download_fls_binary(lines 526-530) which wraps the same pattern in atry/except.Proposed fix — add try/except like _download_fls_binary
console.sendline("echo $?") console.expect(prompt, timeout=EXPECT_TIMEOUT_DEFAULT) - exit_code = int(console.before.decode(errors="ignore").strip().splitlines()[-1]) + try: + lines = console.before.decode(errors="ignore").strip().splitlines() + exit_code = int(lines[-1]) if lines else -1 + except (IndexError, ValueError) as e: + raise FlashRetryableError("FLS flash operation failed, could not parse exit code") from e if exit_code != 0: raise FlashRetryableError(f"FLS flash operation failed, exit code: {exit_code}")
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client_test.py (1)
128-170: Positional-index access incapture_performis fragile.The mock captures
args[2],args[3],args[13],args[14]— if_perform_flash_operation's parameter order changes, this test will silently assert wrong values instead of failing clearly. Consider using keyword arguments or named indices.Example: using a dict keyed by parameter name
- def capture_perform(*args): - captured["image_url"] = args[2] - captured["should_download_to_httpd"] = args[3] - captured["oci_username"] = args[13] - captured["oci_password"] = args[14] + def capture_perform( + partition, path, image_url, should_download_to_httpd, + storage_thread, error_queue, cacert_file, insecure_tls, + headers, bearer_token, method, fls_version, fls_binary_url, + oci_username, oci_password, + ): + captured["image_url"] = image_url + captured["should_download_to_httpd"] = should_download_to_httpd + captured["oci_username"] = oci_username + captured["oci_password"] = oci_password
| if "panicked at" in current_output: | ||
| raise FlashRetryableError(f"FLS panicked: {current_output}") | ||
| if "panicked at" in raw_output: | ||
| raise FlashRetryableError(f"FLS panicked: {self._redact_sensitive_values(raw_output)}") |
|
@mangelajo sorry i forgot to mark it draft, i just wanted preliminary coderabbit comments, i still need to try this out... |
2 participants
Currently OCI credentials weren't passed, so it was not possible to work with private registries.
Summary by CodeRabbit
New Features
Documentation
Tests