cmdline: add support for loading config from a local device#2230
cmdline: add support for loading config from a local device#2230atd9876 wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for loading Ignition configurations from a local device on bare metal platforms via the ignition.config.device and ignition.config.path kernel parameters. The implementation includes documentation updates, a refactored command-line parser, and logic to mount the specified device and read the configuration file. Review feedback identifies a potential logic error where returning configErrors.ErrEmpty might cause a fatal failure instead of a fallback when a device is missing, and a path traversal vulnerability when accessing the configuration file on the mounted device.
bab4a01 to
a13e77b
Compare
prestist
left a comment
prestist
left a comment
There was a problem hiding this comment.
This is looking good overall, thank you for working on this! Some small comments.
I think we could use some integration tests, I would add a blackbox test (in tests/positive/) that sets up a labeled disk image with a config file and boots with ignition.config.device=LABEL ignition.config.path=/config.ign, wdyt?
prestist
left a comment
prestist
left a comment
There was a problem hiding this comment.
Just two comments, Im not sure how I feel about them. What are your thoughts?
|
Thanks for the updates in A couple of things I'd like addressed before we merge:
Lastly the "Require release note" CI check is failing -- can you add a release note? |
|
I will fix those issues. Re the
|
9a073d5 to
0861cdd
Compare
|
@atd9876 Okay from my perspective this is lgtm, just need to rebase and resolve any commits to get the CI working :) |
0861cdd to
06a1302
Compare
Follow the existing pattern used by SystemConfigDir and other distro functions, allowing KernelCmdlinePath to be overridden via the IGNITION_KERNEL_CMDLINE_PATH environment variable. This enables blackbox tests to inject a fake kernel cmdline. Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
Add support for loading Ignition config from a labeled block device via the ignition.config.device and ignition.config.path kernel command-line arguments. Both must be specified together. Error handling: - Return hard error if only one of device/path is specified - Return hard error if the device does not appear within 30s timeout - Return hard error if the config file is not found on the device Includes unit tests for parseCmdline and blackbox integration tests covering both positive (successful fetch) and negative (missing device, missing file, incomplete options) scenarios. Signed-off-by: Andrew Dodds <andrew.dodds@sap.com>
06a1302 to
feb4f08
Compare
Binary size report (
|
| Size | |
|---|---|
Base (main) |
21MiB |
| PR (#2230) | 21MiB |
| Delta | +16KiB (0.07%) |
📝 WalkthroughWalkthroughIgnition’s cmdline provider now supports configuration files from labeled local devices using ChangesKernel Command-Line Device Configuration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KernelCmdline
participant CmdlineProvider
participant LabeledDevice
participant utilParseConfig
KernelCmdline->>CmdlineProvider: provide ignition.config.device and ignition.config.path
CmdlineProvider->>LabeledDevice: wait for label and mount read-only
LabeledDevice-->>CmdlineProvider: return config file contents
CmdlineProvider->>utilParseConfig: parse configuration contents
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 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 `@docs/supported-platforms.md`:
- Line 24: Update the Bare Metal configuration documentation to state that when
ignition.config.url and the ignition.config.device/path pair are all supplied,
ignition.config.url takes precedence and the device/path configuration is
ignored.
In `@internal/providers/cmdline/cmdline.go`:
- Around line 178-221: Update tryMounting so the entire device retrieval flow,
including mounting and reading the config, is retried until the context expires.
Use exec.CommandContext for the mount command so a hanging mount is canceled by
ctx, and retry transient mount or os.ReadFile failures rather than returning
immediately; preserve successful reads and the existing not-found behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Enterprise
Run ID: becb678f-b985-4ac4-8a91-cc0c49628aca
📒 Files selected for processing (9)
docs/release-notes.mddocs/supported-platforms.mdinternal/distro/distro.gointernal/providers/cmdline/cmdline.gointernal/providers/cmdline/cmdline_test.gotests/blackbox_test.gotests/negative/cmdline/cmdline.gotests/positive/cmdline/cmdline.gotests/registry/registry.go
| * [IBM Cloud] (`ibmcloud`) - Ignition will read its configuration from the instance userdata. Cloud SSH keys are handled separately. | ||
| * [KubeVirt] (`kubevirt`) - Ignition will read its configuration from the instance userdata via `cloudInitConfigDrive` or `cloudInitNoCloud`. Cloud SSH keys are handled separately. | ||
| * Bare Metal (`metal`) - Use the `ignition.config.url` kernel parameter to provide a URL to the configuration. The URL can use the `http://`, `https://`, `tftp://`, `s3://`, `arn:`, or `gs://` schemes to specify a remote config. | ||
| * Bare Metal (`metal`) - Use the `ignition.config.url` kernel parameter to provide a URL to the configuration. The URL can use the `http://`, `https://`, `tftp://`, `s3://`, `arn:`, or `gs://` schemes to specify a remote config. Alternatively, use `ignition.config.device` (a disk-by-label name, e.g. `CONFIG`) and `ignition.config.path` (the path to the config file on that device, e.g. `/ignition/config.ign`) to load the configuration from a locally attached device. Both parameters must be provided together. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document URL precedence when both sources are supplied.
The provider gives ignition.config.url precedence and ignores the device/path pair when all are present, as implemented in internal/providers/cmdline/cmdline.go:77-80. State this explicitly so operators do not assume the device configuration will be used.
🤖 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/supported-platforms.md` at line 24, Update the Bare Metal configuration
documentation to state that when ignition.config.url and the
ignition.config.device/path pair are all supplied, ignition.config.url takes
precedence and the device/path configuration is ignored.
| func tryMounting(logger *log.Logger, ctx context.Context, opts *cmdlineOpts) ([]byte, error) { | ||
| device := filepath.Join(distro.DiskByLabelDir(), opts.DeviceLabel) | ||
| for !fileExists(device) { | ||
| logger.Debug("disk (%q) not found. Waiting...", device) | ||
| select { | ||
| case <-time.After(time.Second): | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| } | ||
| } | ||
|
|
||
| if len(parts) == 2 { | ||
| url = parts[1] | ||
| logger.Debug("creating temporary mount point") | ||
| mnt, err := os.MkdirTemp("", "ignition-config") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create temp directory: %v", err) | ||
| } | ||
| defer func() { | ||
| if err := os.Remove(mnt); err != nil { | ||
| logger.Err("failed to remove temporary mount point %q: %v", mnt, err) | ||
| } | ||
| }() | ||
|
|
||
| cmd := exec.Command(distro.MountCmd(), "-o", "ro", "-t", "auto", device, mnt) | ||
| if _, err := logger.LogCmd(cmd, "mounting disk"); err != nil { | ||
| return nil, err | ||
| } | ||
| defer func() { | ||
| _ = logger.LogOp( | ||
| func() error { | ||
| return ut.UmountPath(mnt) | ||
| }, | ||
| "unmounting %q at %q", device, mnt, | ||
| ) | ||
| }() | ||
|
|
||
| configPath := filepath.Join(mnt, filepath.Clean(filepath.Join("/", opts.UserDataPath))) | ||
| if !fileExists(configPath) { | ||
| logger.Debug("config file %q not found on device %q", opts.UserDataPath, opts.DeviceLabel) | ||
| return nil, nil | ||
| } | ||
|
|
||
| contents, err := os.ReadFile(configPath) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Bound and retry the complete device retrieval operation.
The context only governs the device-existence loop. mount can hang indefinitely because it uses exec.Command, and transient mount/read failures immediately abort instead of retrying. Use exec.CommandContext and retry the mount/read attempt until the context expires.
As per coding guidelines, “Platform providers must retry config fetches.”
🤖 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 `@internal/providers/cmdline/cmdline.go` around lines 178 - 221, Update
tryMounting so the entire device retrieval flow, including mounting and reading
the config, is retried until the context expires. Use exec.CommandContext for
the mount command so a hanging mount is canceled by ctx, and retry transient
mount or os.ReadFile failures rather than returning immediately; preserve
successful reads and the existing not-found behavior.
Source: Coding guidelines
There was a problem hiding this comment.
The exec.CommandContext point is valid -- if mount hangs (e.g. unresponsive block device), the 30s context only covers the device-existence loop, not the mount itself. Switching to exec.CommandContext(ctx, ...) at line 200 is a small change and would prevent an indefinite hang.
@atd9876 I think this would be a worthwhile fix, everything else is minor. If you want I can do this so we can get this merged.
3 participants
Summary
Add support for loading Ignition configuration from a local device
specified via kernel command-line flags
ignition.config.deviceandignition.config.path. This enables bare-metal and air-gappedenvironments to provide Ignition configs on a labeled disk partition
(e.g., a config drive) without requiring network access.
Acknowledgments
The initial device-based config loading implementation (commit 6c250bc)
was authored by Jan Larwig (@tuunit). This PR builds on that work with
tests, documentation, and robustness improvements.
Changes
parseCmdlinenow recognizesignition.config.deviceandignition.config.pathflags.fetchConfiguses both to mount the labeled device read-only andread the config file at the specified path.
tryMountingfunction: Waits for the labeled device to appear(with a 30-second timeout), mounts it read-only into a temporary
directory, reads the config, and unmounts on return.
parseCmdline:strings.Fieldsinstead ofstrings.Splitto correctlyhandle tabs, multiple spaces, and trailing newlines.
urlvariable toparsedURLto avoid shadowingthe
net/urlimport.errors.Is(err, context.DeadlineExceeded)instead of==to correctly match wrapped errors.
partial flags, empty flags, invalid URLs, precedence,
fileExists,and error handling in test helpers.
Fixes #2207
Summary by CodeRabbit
New Features
Bug Fixes
Documentation