From 6f34f4ab845a2b29e1ea26781761c0463a99e508 Mon Sep 17 00:00:00 2001 From: Jan Larwig Date: Fri, 7 Feb 2025 09:39:00 +0100 Subject: [PATCH 1/4] add support for loading config from device Signed-off-by: Andrew Dodds --- internal/providers/cmdline/cmdline.go | 176 +++++++++++++++++++++----- 1 file changed, 145 insertions(+), 31 deletions(-) diff --git a/internal/providers/cmdline/cmdline.go b/internal/providers/cmdline/cmdline.go index b0d774fff..954e3cc6d 100644 --- a/internal/providers/cmdline/cmdline.go +++ b/internal/providers/cmdline/cmdline.go @@ -18,9 +18,14 @@ package cmdline import ( + "context" + "fmt" "net/url" "os" + "os/exec" + "path/filepath" "strings" + "time" "github.com/coreos/ignition/v2/config/v3_7_experimental/types" "github.com/coreos/ignition/v2/internal/distro" @@ -28,14 +33,25 @@ import ( "github.com/coreos/ignition/v2/internal/platform" "github.com/coreos/ignition/v2/internal/providers/util" "github.com/coreos/ignition/v2/internal/resource" + ut "github.com/coreos/ignition/v2/internal/util" "github.com/coreos/vcontext/report" ) +type cmdlineFlag string + const ( - cmdlineUrlFlag = "ignition.config.url" + flagUrl cmdlineFlag = "ignition.config.url" + flagDeviceLabel cmdlineFlag = "ignition.config.device" + flagUserDataPath cmdlineFlag = "ignition.config.path" ) +type cmdlineOpts struct { + Url *url.URL + UserDataPath string + DeviceLabel string +} + var ( // we are a special-cased system provider; don't register ourselves // for lookup by name @@ -46,59 +62,157 @@ var ( ) func fetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) { - url, err := readCmdline(f.Logger) + opts, err := parseCmdline(f.Logger) if err != nil { return types.Config{}, report.Report{}, err } - if url == nil { - return types.Config{}, report.Report{}, platform.ErrNoProvider + var data []byte + + if opts.Url != nil { + data, err = f.FetchToBuffer(*opts.Url, resource.FetchOptions{}) + if err != nil { + return types.Config{}, report.Report{}, err + } + + return util.ParseConfig(f.Logger, data) } - data, err := f.FetchToBuffer(*url, resource.FetchOptions{}) - if err != nil { - return types.Config{}, report.Report{}, err + if opts.UserDataPath != "" && opts.DeviceLabel != "" { + return fetchConfigFromDevice(f.Logger, opts) } - return util.ParseConfig(f.Logger, data) + return types.Config{}, report.Report{}, platform.ErrNoProvider } -func readCmdline(logger *log.Logger) (*url.URL, error) { - args, err := os.ReadFile(distro.KernelCmdlinePath()) +func parseCmdline(logger *log.Logger) (*cmdlineOpts, error) { + cmdline, err := os.ReadFile(distro.KernelCmdlinePath()) if err != nil { logger.Err("couldn't read cmdline: %v", err) return nil, err } - rawUrl := parseCmdline(args) - logger.Debug("parsed url from cmdline: %q", rawUrl) - if rawUrl == "" { - logger.Info("no config URL provided") - return nil, nil - } + opts := &cmdlineOpts{} - url, err := url.Parse(rawUrl) - if err != nil { - logger.Err("failed to parse url: %v", err) - return nil, err + for _, arg := range strings.Split(string(cmdline), " ") { + parts := strings.SplitN(strings.TrimSpace(arg), "=", 2) + if len(parts) != 2 { + continue + } + + key := cmdlineFlag(parts[0]) + value := parts[1] + + switch key { + case flagUrl: + if value == "" { + logger.Info("url flag found but no value provided") + continue + } + + url, err := url.Parse(value) + if err != nil { + logger.Err("failed to parse url: %v", err) + continue + } + opts.Url = url + case flagDeviceLabel: + if value == "" { + logger.Info("device label flag found but no value provided") + continue + } + opts.DeviceLabel = value + case flagUserDataPath: + if value == "" { + logger.Info("user data path flag found but no value provided") + continue + } + opts.DeviceLabel = value + } } - return url, err + return opts, nil } -func parseCmdline(cmdline []byte) (url string) { - for _, arg := range strings.Split(string(cmdline), " ") { - parts := strings.SplitN(strings.TrimSpace(arg), "=", 2) - key := parts[0] - - if key != cmdlineUrlFlag { - continue +func fetchConfigFromDevice(logger *log.Logger, opts *cmdlineOpts) (types.Config, report.Report, error) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + var data []byte + + dispatch := func(name string, fn func() ([]byte, error)) { + raw, err := fn() + if err != nil { + switch err { + case context.Canceled: + case context.DeadlineExceeded: + logger.Err("timed out while fetching config from %s", name) + default: + logger.Err("failed to fetch config from %s: %v", name, err) + } + return } - if len(parts) == 2 { - url = parts[1] + data = raw + cancel() + } + + go dispatch( + "load config from disk", func() ([]byte, error) { + return tryMounting(logger, ctx, opts) + }, + ) + + <-ctx.Done() + if ctx.Err() == context.DeadlineExceeded { + logger.Info("disk was not available in time. Continuing without a config...") + } + + return util.ParseConfig(logger, data) +} + +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() } } - return + 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 os.Remove(mnt) + + 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, + ) + }() + + if !fileExists(filepath.Join(mnt, opts.UserDataPath)) { + return nil, nil + } + + contents, err := os.ReadFile(filepath.Join(mnt, opts.UserDataPath)) + if err != nil { + return nil, err + } + + return contents, nil +} + +func fileExists(path string) bool { + _, err := os.Stat(path) + return (err == nil) } From 79fc035cc2b0edc7e112055f8a2f88bc86430ea2 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Fri, 24 Apr 2026 12:23:11 +0100 Subject: [PATCH 2/4] add tests and documentation for cmdline device config support Signed-off-by: Andrew Dodds --- docs/supported-platforms.md | 2 +- internal/providers/cmdline/cmdline.go | 55 +++--- internal/providers/cmdline/cmdline_test.go | 189 +++++++++++++++++++++ 3 files changed, 214 insertions(+), 32 deletions(-) create mode 100644 internal/providers/cmdline/cmdline_test.go diff --git a/docs/supported-platforms.md b/docs/supported-platforms.md index bd0e2b200..2744fff7d 100644 --- a/docs/supported-platforms.md +++ b/docs/supported-platforms.md @@ -21,7 +21,7 @@ Ignition is currently supported for the following platforms: * [Microsoft Hyper-V] (`hyperv`) - Ignition will read its configuration from the `ignition.config` key in pool 0 of the Hyper-V Data Exchange Service (KVP). Values are limited to approximately 1 KiB of text, so Ignition can also read and concatenate multiple keys named `ignition.config.0`, `ignition.config.1`, and so on. * [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. * [Nutanix] (`nutanix`) - Ignition will read its configuration from the instance userdata via config drive. Cloud SSH keys are handled separately. * [NVIDIA BlueField] (`nvidiabluefield`) - Ignition will read its configuration from the bootfifo sysfs interface from the mlxbf_bootctl platform driver. * [OpenStack] (`openstack`) - Ignition will read its configuration from the instance userdata via either metadata service or config drive. Cloud SSH keys are handled separately. diff --git a/internal/providers/cmdline/cmdline.go b/internal/providers/cmdline/cmdline.go index 954e3cc6d..7c739fed5 100644 --- a/internal/providers/cmdline/cmdline.go +++ b/internal/providers/cmdline/cmdline.go @@ -13,7 +13,8 @@ // limitations under the License. // The cmdline provider fetches a remote configuration from the URL specified -// in the kernel boot option "ignition.config.url". +// in the kernel boot option "ignition.config.url", or from a local device +// specified by "ignition.config.device" and "ignition.config.path". package cmdline @@ -27,6 +28,7 @@ import ( "strings" "time" + "github.com/coreos/ignition/v2/config/shared/errors" "github.com/coreos/ignition/v2/config/v3_7_experimental/types" "github.com/coreos/ignition/v2/internal/distro" "github.com/coreos/ignition/v2/internal/log" @@ -62,7 +64,7 @@ var ( ) func fetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) { - opts, err := parseCmdline(f.Logger) + opts, err := parseCmdline(f.Logger, distro.KernelCmdlinePath()) if err != nil { return types.Config{}, report.Report{}, err } @@ -82,11 +84,16 @@ func fetchConfig(f *resource.Fetcher) (types.Config, report.Report, error) { return fetchConfigFromDevice(f.Logger, opts) } + if opts.UserDataPath != "" || opts.DeviceLabel != "" { + f.Logger.Warning("both %q and %q must be provided together; ignoring", + string(flagDeviceLabel), string(flagUserDataPath)) + } + return types.Config{}, report.Report{}, platform.ErrNoProvider } -func parseCmdline(logger *log.Logger) (*cmdlineOpts, error) { - cmdline, err := os.ReadFile(distro.KernelCmdlinePath()) +func parseCmdline(logger *log.Logger, path string) (*cmdlineOpts, error) { + cmdline, err := os.ReadFile(path) if err != nil { logger.Err("couldn't read cmdline: %v", err) return nil, err @@ -127,7 +134,7 @@ func parseCmdline(logger *log.Logger) (*cmdlineOpts, error) { logger.Info("user data path flag found but no value provided") continue } - opts.DeviceLabel = value + opts.UserDataPath = value } } @@ -136,34 +143,19 @@ func parseCmdline(logger *log.Logger) (*cmdlineOpts, error) { func fetchConfigFromDevice(logger *log.Logger, opts *cmdlineOpts) (types.Config, report.Report, error) { ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - var data []byte - - dispatch := func(name string, fn func() ([]byte, error)) { - raw, err := fn() - if err != nil { - switch err { - case context.Canceled: - case context.DeadlineExceeded: - logger.Err("timed out while fetching config from %s", name) - default: - logger.Err("failed to fetch config from %s: %v", name, err) - } - return - } + defer cancel() - data = raw - cancel() - } - - go dispatch( - "load config from disk", func() ([]byte, error) { - return tryMounting(logger, ctx, opts) - }, - ) - - <-ctx.Done() - if ctx.Err() == context.DeadlineExceeded { + data, err := tryMounting(logger, ctx, opts) + if err == context.DeadlineExceeded { logger.Info("disk was not available in time. Continuing without a config...") + return types.Config{}, report.Report{}, errors.ErrEmpty + } + if err != nil { + return types.Config{}, report.Report{}, err + } + if data == nil { + logger.Info("config file %q not found on device. Continuing without config...", opts.UserDataPath) + return types.Config{}, report.Report{}, errors.ErrEmpty } return util.ParseConfig(logger, data) @@ -201,6 +193,7 @@ func tryMounting(logger *log.Logger, ctx context.Context, opts *cmdlineOpts) ([] }() if !fileExists(filepath.Join(mnt, opts.UserDataPath)) { + logger.Debug("config file %q not found on device %q", opts.UserDataPath, opts.DeviceLabel) return nil, nil } diff --git a/internal/providers/cmdline/cmdline_test.go b/internal/providers/cmdline/cmdline_test.go new file mode 100644 index 000000000..d946de391 --- /dev/null +++ b/internal/providers/cmdline/cmdline_test.go @@ -0,0 +1,189 @@ +// Copyright 2026 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdline + +import ( + "net/url" + "os" + "path/filepath" + "testing" + + "github.com/coreos/ignition/v2/internal/log" +) + +func newTestLogger(t *testing.T) *log.Logger { + t.Helper() + logger := log.New(false) + return &logger +} + +// writeCmdline writes the given cmdline string to a temp file and returns its path. +func writeCmdline(t *testing.T, content string) string { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "cmdline") + if err != nil { + t.Fatal(err) + } + if _, err := f.WriteString(content); err != nil { + t.Fatal(err) + } + f.Close() + return f.Name() +} + +func TestParseCmdlineURL(t *testing.T) { + cmdlinePath := writeCmdline(t, "ignition.config.url=https://example.com/config.ign console=tty0") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + expected, _ := url.Parse("https://example.com/config.ign") + if opts.Url == nil || opts.Url.String() != expected.String() { + t.Errorf("expected URL %q, got %v", expected, opts.Url) + } + if opts.DeviceLabel != "" { + t.Errorf("expected empty DeviceLabel, got %q", opts.DeviceLabel) + } + if opts.UserDataPath != "" { + t.Errorf("expected empty UserDataPath, got %q", opts.UserDataPath) + } +} + +func TestParseCmdlineDeviceAndPath(t *testing.T) { + cmdlinePath := writeCmdline(t, "ignition.config.device=CONFIG ignition.config.path=/ignition/config.ign") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.Url != nil { + t.Errorf("expected nil URL, got %v", opts.Url) + } + if opts.DeviceLabel != "CONFIG" { + t.Errorf("expected DeviceLabel %q, got %q", "CONFIG", opts.DeviceLabel) + } + if opts.UserDataPath != "/ignition/config.ign" { + t.Errorf("expected UserDataPath %q, got %q", "/ignition/config.ign", opts.UserDataPath) + } +} + +func TestParseCmdlineDeviceOnly(t *testing.T) { + cmdlinePath := writeCmdline(t, "ignition.config.device=CONFIG") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.DeviceLabel != "CONFIG" { + t.Errorf("expected DeviceLabel %q, got %q", "CONFIG", opts.DeviceLabel) + } + if opts.UserDataPath != "" { + t.Errorf("expected empty UserDataPath, got %q", opts.UserDataPath) + } +} + +func TestParseCmdlinePathOnly(t *testing.T) { + cmdlinePath := writeCmdline(t, "ignition.config.path=/ignition/config.ign") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.UserDataPath != "/ignition/config.ign" { + t.Errorf("expected UserDataPath %q, got %q", "/ignition/config.ign", opts.UserDataPath) + } + if opts.DeviceLabel != "" { + t.Errorf("expected empty DeviceLabel, got %q", opts.DeviceLabel) + } +} + +func TestParseCmdlineEmptyFlagsIgnored(t *testing.T) { + cmdlinePath := writeCmdline(t, "ignition.config.url= ignition.config.device= ignition.config.path=") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.Url != nil { + t.Errorf("expected nil URL for empty flag, got %v", opts.Url) + } + if opts.DeviceLabel != "" { + t.Errorf("expected empty DeviceLabel for empty flag, got %q", opts.DeviceLabel) + } + if opts.UserDataPath != "" { + t.Errorf("expected empty UserDataPath for empty flag, got %q", opts.UserDataPath) + } +} + +func TestParseCmdlineNoIgnitionFlags(t *testing.T) { + cmdlinePath := writeCmdline(t, "console=tty0 root=/dev/sda1 ro quiet") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.Url != nil || opts.DeviceLabel != "" || opts.UserDataPath != "" { + t.Errorf("expected all opts empty for unrelated cmdline, got %+v", opts) + } +} + +func TestParseCmdlineInvalidURL(t *testing.T) { + // A URL with a control character is unparseable; the flag should be skipped. + cmdlinePath := writeCmdline(t, "ignition.config.url=://bad url\x00") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.Url != nil { + t.Errorf("expected nil URL for invalid URL, got %v", opts.Url) + } +} + +func TestParseCmdlineURLTakesPrecedence(t *testing.T) { + // If both url and device/path are set, url wins (checked in fetchConfig). + // parseCmdline should populate all provided fields. + cmdlinePath := writeCmdline(t, "ignition.config.url=https://example.com/config.ign ignition.config.device=CONFIG ignition.config.path=/config.ign") + logger := newTestLogger(t) + opts, err := parseCmdline(logger, cmdlinePath) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if opts.Url == nil { + t.Error("expected URL to be set") + } + if opts.DeviceLabel != "CONFIG" { + t.Errorf("expected DeviceLabel %q, got %q", "CONFIG", opts.DeviceLabel) + } + if opts.UserDataPath != "/config.ign" { + t.Errorf("expected UserDataPath %q, got %q", "/config.ign", opts.UserDataPath) + } +} + +func TestFileExists(t *testing.T) { + dir := t.TempDir() + existing := filepath.Join(dir, "file.txt") + if err := os.WriteFile(existing, []byte("x"), 0600); err != nil { + t.Fatal(err) + } + + if !fileExists(existing) { + t.Errorf("expected fileExists(%q) = true", existing) + } + if fileExists(filepath.Join(dir, "nonexistent.txt")) { + t.Errorf("expected fileExists on missing file = false") + } +} From a13e77bb33944634152e18bef74eaaa6bf2a7519 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Fri, 8 May 2026 12:20:44 +0100 Subject: [PATCH 3/4] cmdline: improve robustness of parseCmdline and tests - Use strings.Fields instead of strings.Split for proper whitespace handling - Use errors.Is for wrapped error comparison (context.DeadlineExceeded) - Rename local url variable to parsedURL to avoid shadowing net/url import - Check f.Close() error in test helper - Check url.Parse error in test assertion Signed-off-by: Andrew Dodds --- internal/providers/cmdline/cmdline.go | 20 +++++++++-------- internal/providers/cmdline/cmdline_test.go | 25 ++++++---------------- 2 files changed, 18 insertions(+), 27 deletions(-) diff --git a/internal/providers/cmdline/cmdline.go b/internal/providers/cmdline/cmdline.go index 7c739fed5..fffc7bc7c 100644 --- a/internal/providers/cmdline/cmdline.go +++ b/internal/providers/cmdline/cmdline.go @@ -20,6 +20,7 @@ package cmdline import ( "context" + "errors" "fmt" "net/url" "os" @@ -28,7 +29,7 @@ import ( "strings" "time" - "github.com/coreos/ignition/v2/config/shared/errors" + configErrors "github.com/coreos/ignition/v2/config/shared/errors" "github.com/coreos/ignition/v2/config/v3_7_experimental/types" "github.com/coreos/ignition/v2/internal/distro" "github.com/coreos/ignition/v2/internal/log" @@ -101,7 +102,7 @@ func parseCmdline(logger *log.Logger, path string) (*cmdlineOpts, error) { opts := &cmdlineOpts{} - for _, arg := range strings.Split(string(cmdline), " ") { + for _, arg := range strings.Fields(string(cmdline)) { parts := strings.SplitN(strings.TrimSpace(arg), "=", 2) if len(parts) != 2 { continue @@ -117,12 +118,12 @@ func parseCmdline(logger *log.Logger, path string) (*cmdlineOpts, error) { continue } - url, err := url.Parse(value) + parsedURL, err := url.Parse(value) if err != nil { logger.Err("failed to parse url: %v", err) continue } - opts.Url = url + opts.Url = parsedURL case flagDeviceLabel: if value == "" { logger.Info("device label flag found but no value provided") @@ -146,16 +147,16 @@ func fetchConfigFromDevice(logger *log.Logger, opts *cmdlineOpts) (types.Config, defer cancel() data, err := tryMounting(logger, ctx, opts) - if err == context.DeadlineExceeded { + if errors.Is(err, context.DeadlineExceeded) { logger.Info("disk was not available in time. Continuing without a config...") - return types.Config{}, report.Report{}, errors.ErrEmpty + return types.Config{}, report.Report{}, configErrors.ErrEmpty } if err != nil { return types.Config{}, report.Report{}, err } if data == nil { logger.Info("config file %q not found on device. Continuing without config...", opts.UserDataPath) - return types.Config{}, report.Report{}, errors.ErrEmpty + return types.Config{}, report.Report{}, configErrors.ErrEmpty } return util.ParseConfig(logger, data) @@ -192,12 +193,13 @@ func tryMounting(logger *log.Logger, ctx context.Context, opts *cmdlineOpts) ([] ) }() - if !fileExists(filepath.Join(mnt, opts.UserDataPath)) { + 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(filepath.Join(mnt, opts.UserDataPath)) + contents, err := os.ReadFile(configPath) if err != nil { return nil, err } diff --git a/internal/providers/cmdline/cmdline_test.go b/internal/providers/cmdline/cmdline_test.go index d946de391..9378ba441 100644 --- a/internal/providers/cmdline/cmdline_test.go +++ b/internal/providers/cmdline/cmdline_test.go @@ -17,7 +17,6 @@ package cmdline import ( "net/url" "os" - "path/filepath" "testing" "github.com/coreos/ignition/v2/internal/log" @@ -39,7 +38,9 @@ func writeCmdline(t *testing.T, content string) string { if _, err := f.WriteString(content); err != nil { t.Fatal(err) } - f.Close() + if err := f.Close(); err != nil { + t.Fatal(err) + } return f.Name() } @@ -50,7 +51,10 @@ func TestParseCmdlineURL(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - expected, _ := url.Parse("https://example.com/config.ign") + expected, err := url.Parse("https://example.com/config.ign") + if err != nil { + t.Fatalf("failed to parse expected URL: %v", err) + } if opts.Url == nil || opts.Url.String() != expected.String() { t.Errorf("expected URL %q, got %v", expected, opts.Url) } @@ -172,18 +176,3 @@ func TestParseCmdlineURLTakesPrecedence(t *testing.T) { t.Errorf("expected UserDataPath %q, got %q", "/config.ign", opts.UserDataPath) } } - -func TestFileExists(t *testing.T) { - dir := t.TempDir() - existing := filepath.Join(dir, "file.txt") - if err := os.WriteFile(existing, []byte("x"), 0600); err != nil { - t.Fatal(err) - } - - if !fileExists(existing) { - t.Errorf("expected fileExists(%q) = true", existing) - } - if fileExists(filepath.Join(dir, "nonexistent.txt")) { - t.Errorf("expected fileExists on missing file = false") - } -} From 0dc3790a20ac487c0d67886da556d7fc7c009d11 Mon Sep 17 00:00:00 2001 From: Andrew Dodds Date: Wed, 13 May 2026 13:17:41 +0100 Subject: [PATCH 4/4] cmdline: address review feedback and add blackbox test - Return hard error on device timeout instead of silently continuing - Wrap os.Remove in logging closure for temp mount point cleanup - Add blackbox integration test for cmdline device/path config fetch Signed-off-by: Andrew Dodds --- internal/distro/distro.go | 2 +- internal/providers/cmdline/cmdline.go | 9 +- tests/blackbox_test.go | 5 +- tests/positive/cmdline/cmdline.go | 131 ++++++++++++++++++++++++++ tests/registry/registry.go | 1 + 5 files changed, 143 insertions(+), 5 deletions(-) create mode 100644 tests/positive/cmdline/cmdline.go diff --git a/internal/distro/distro.go b/internal/distro/distro.go index 201909034..a4aa8a41b 100644 --- a/internal/distro/distro.go +++ b/internal/distro/distro.go @@ -86,7 +86,7 @@ var ( func DiskByLabelDir() string { return diskByLabelDir } -func KernelCmdlinePath() string { return kernelCmdlinePath } +func KernelCmdlinePath() string { return fromEnv("KERNEL_CMDLINE_PATH", kernelCmdlinePath) } func BootIDPath() string { return bootIDPath } func SystemConfigDir() string { return fromEnv("SYSTEM_CONFIG_DIR", systemConfigDir) } diff --git a/internal/providers/cmdline/cmdline.go b/internal/providers/cmdline/cmdline.go index fffc7bc7c..ddfd20fce 100644 --- a/internal/providers/cmdline/cmdline.go +++ b/internal/providers/cmdline/cmdline.go @@ -148,8 +148,7 @@ func fetchConfigFromDevice(logger *log.Logger, opts *cmdlineOpts) (types.Config, data, err := tryMounting(logger, ctx, opts) if errors.Is(err, context.DeadlineExceeded) { - logger.Info("disk was not available in time. Continuing without a config...") - return types.Config{}, report.Report{}, configErrors.ErrEmpty + return types.Config{}, report.Report{}, fmt.Errorf("device %q did not appear within timeout", opts.DeviceLabel) } if err != nil { return types.Config{}, report.Report{}, err @@ -178,7 +177,11 @@ func tryMounting(logger *log.Logger, ctx context.Context, opts *cmdlineOpts) ([] if err != nil { return nil, fmt.Errorf("failed to create temp directory: %v", err) } - defer os.Remove(mnt) + 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 { diff --git a/tests/blackbox_test.go b/tests/blackbox_test.go index 5bcb4da8f..76ffc10df 100644 --- a/tests/blackbox_test.go +++ b/tests/blackbox_test.go @@ -291,7 +291,10 @@ func outer(t *testing.T, test types.Test, negativeTests bool) error { } // Ignition - appendEnv := test.Env + appendEnv := make([]string, len(test.Env)) + for i, env := range test.Env { + appendEnv[i] = strings.ReplaceAll(env, "$SYSTEM_CONFIG_DIR", systemConfigDir) + } appendEnv = append(appendEnv, "IGNITION_SYSTEM_CONFIG_DIR="+systemConfigDir) if !negativeTests { diff --git a/tests/positive/cmdline/cmdline.go b/tests/positive/cmdline/cmdline.go new file mode 100644 index 000000000..1ddb334d7 --- /dev/null +++ b/tests/positive/cmdline/cmdline.go @@ -0,0 +1,131 @@ +// Copyright 2026 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package cmdline + +import ( + "github.com/coreos/ignition/v2/tests/register" + "github.com/coreos/ignition/v2/tests/types" +) + +func init() { + register.Register(register.PositiveTest, FetchConfigFromDevice()) +} + +func FetchConfigFromDevice() types.Test { + name := "cmdline.device.fetch" + in := types.GetBaseDisk() + out := types.GetBaseDisk() + + // Config that will be placed on the labeled device partition. + // This is what Ignition will actually read via the cmdline provider. + deviceConfig := `{ + "ignition": { "version": "3.4.0" }, + "storage": { + "files": [{ + "path": "/foo/bar", + "contents": { "source": "data:,example%20file%0A" } + }] + } + }` + + // Config for the test framework's validation. Uses $version so the + // test is registered across spec versions. The file platform won't + // be consulted because the cmdline provider takes priority. + config := `{ + "ignition": { "version": "$version" }, + "storage": { + "files": [{ + "path": "/foo/bar", + "contents": { "source": "data:,example%20file%0A" } + }] + } + }` + configMinVersion := "3.4.0" + + // Add a second disk with a labeled partition containing the config file. + // The cmdline provider will mount this partition and read config.ign. + in = append(in, types.Disk{ + Alignment: types.IgnitionAlignment, + Partitions: types.Partitions{ + { + Label: "IGNCONFIG", + Number: 1, + Length: 65536, + FilesystemType: "ext4", + FilesystemLabel: "IGNCONFIG", + Files: []types.File{ + { + Node: types.Node{ + Name: "config.ign", + Directory: "/", + }, + Contents: deviceConfig, + }, + }, + }, + }, + }) + out = append(out, types.Disk{ + Alignment: types.IgnitionAlignment, + Partitions: types.Partitions{ + { + Label: "IGNCONFIG", + Number: 1, + Length: 65536, + FilesystemType: "ext4", + FilesystemLabel: "IGNCONFIG", + Files: []types.File{ + { + Node: types.Node{ + Name: "config.ign", + Directory: "/", + }, + Contents: deviceConfig, + }, + }, + }, + }, + }) + + out[0].Partitions.AddFiles("ROOT", []types.File{ + { + Node: types.Node{ + Name: "bar", + Directory: "foo", + }, + Contents: "example file\n", + }, + }) + + return types.Test{ + Name: name, + In: in, + Out: out, + Config: config, + ConfigMinVersion: configMinVersion, + Env: []string{ + "IGNITION_KERNEL_CMDLINE_PATH=$SYSTEM_CONFIG_DIR/cmdline", + }, + SystemDirFiles: []types.File{ + { + Node: types.Node{ + Name: "cmdline", + Directory: "/", + }, + Contents: "ignition.config.device=IGNCONFIG ignition.config.path=/config.ign", + }, + }, + } +} diff --git a/tests/registry/registry.go b/tests/registry/registry.go index dc0f01fa2..dc16ae098 100644 --- a/tests/registry/registry.go +++ b/tests/registry/registry.go @@ -25,6 +25,7 @@ import ( _ "github.com/coreos/ignition/v2/tests/negative/regression" _ "github.com/coreos/ignition/v2/tests/negative/security" _ "github.com/coreos/ignition/v2/tests/negative/timeouts" + _ "github.com/coreos/ignition/v2/tests/positive/cmdline" _ "github.com/coreos/ignition/v2/tests/positive/files" _ "github.com/coreos/ignition/v2/tests/positive/filesystems" _ "github.com/coreos/ignition/v2/tests/positive/general"