From 22f5d4ed041e53dc83ccbdac82163dd10edd84e7 Mon Sep 17 00:00:00 2001 From: James Telfer <792299+jamestelfer@users.noreply.github.com> Date: Mon, 6 Jul 2026 15:17:48 +1000 Subject: [PATCH 1/2] feat(config): add config command to view and set broker configuration Add `config path`, `config list`, and `config set` subcommands so operators can locate, inspect, and modify the host-side configuration file. Previously the file had only a read path and was never created automatically, so operators had to author it by hand. Extend pkg/config with a Set write path that creates the file and its parent directory when absent, preserves other keys, clears a key on an empty value, and validates values before writing. Share validation and decoding between Load and Set, and keep validation errors path-free so Set does not misleadingly reference the file for a value typed on the CLI. Deduplicate the CLI writer resolution shared with the doctor command. --- cmd/imds-broker/config.go | 118 ++++++++++++++++++++++++++ cmd/imds-broker/doctor.go | 5 +- cmd/imds-broker/main.go | 1 + cmd/imds-broker/main_test.go | 55 +++++++++++++ pkg/config/config.go | 155 ++++++++++++++++++++++++++--------- pkg/config/config_test.go | 76 +++++++++++++++++ 6 files changed, 369 insertions(+), 41 deletions(-) create mode 100644 cmd/imds-broker/config.go diff --git a/cmd/imds-broker/config.go b/cmd/imds-broker/config.go new file mode 100644 index 0000000..bd0846b --- /dev/null +++ b/cmd/imds-broker/config.go @@ -0,0 +1,118 @@ +package main + +import ( + "context" + "fmt" + "io" + "os" + "strings" + + "github.com/urfave/cli/v3" + + brokerconfig "github.com/jamestelfer/imds-broker/pkg/config" +) + +// configCommand defines the host-side configuration command. It reads and +// writes the broker configuration file directly; it makes no AWS calls and +// starts no servers. The configuration file is host-controlled, not an +// agent-reachable interface: see the project README sandbox model. +func configCommand() *cli.Command { + return &cli.Command{ + Name: "config", + Usage: "Inspect and modify the host-side broker configuration file", + Commands: []*cli.Command{ + configPathCommand(), + configListCommand(), + configSetCommand(), + }, + } +} + +// commandWriter returns the root command writer, defaulting to stdout. +func commandWriter(cmd *cli.Command) io.Writer { + if w := cmd.Root().Writer; w != nil { + return w + } + return os.Stdout +} + +func configPathCommand() *cli.Command { + return &cli.Command{ + Name: "path", + Usage: "Print the configuration file location", + Action: func(ctx context.Context, cmd *cli.Command) error { + path, err := brokerconfig.ResolvePath(ctx) + if err != nil { + return fmt.Errorf("config path: %w", err) + } + _, err = io.WriteString(commandWriter(cmd), path+"\n") + return err + }, + } +} + +func configListCommand() *cli.Command { + return &cli.Command{ + Name: "list", + Aliases: []string{"show"}, + Usage: "List the current configuration values", + Action: func(ctx context.Context, cmd *cli.Command) error { + cfg, err := brokerconfig.Load(ctx) + if err != nil { + return fmt.Errorf("config list: %w", err) + } + + fileState := "not found (using built-in defaults)" + if cfg.Found { + fileState = "found" + } + var b strings.Builder + fmt.Fprintf(&b, "path: %s\n", cfg.Path) + fmt.Fprintf(&b, "file: %s\n", fileState) + fmt.Fprintf(&b, "%s: %s\n", brokerconfig.KeyProfileFilter, valueOrUnset(cfg.ProfileFilter)) + fmt.Fprintf(&b, "%s: %s\n", brokerconfig.KeyRegion, valueOrUnset(cfg.Region)) + fmt.Fprintf(&b, "%s: %s\n", brokerconfig.KeyLogLevel, valueOrUnset(cfg.LogLevel)) + _, err = io.WriteString(commandWriter(cmd), b.String()) + return err + }, + } +} + +func configSetCommand() *cli.Command { + return &cli.Command{ + Name: "set", + Usage: "Set a configuration value (creates the file if absent)", + ArgsUsage: " ", + Description: "Valid keys: " + brokerconfig.KeyProfileFilter + ", " + + brokerconfig.KeyRegion + ", " + brokerconfig.KeyLogLevel + + ". An empty value clears the key.", + Action: func(ctx context.Context, cmd *cli.Command) error { + args := cmd.Args() + if args.Len() != 2 { + return fmt.Errorf("config set: expected , got %d argument(s)", args.Len()) + } + key, value := args.Get(0), args.Get(1) + + cfg, err := brokerconfig.Set(ctx, key, value) + if err != nil { + return fmt.Errorf("config set: %w", err) + } + + msg := fmt.Sprintf("set %s = %s in %s\n", key, value, cfg.Path) + if value == "" { + msg = fmt.Sprintf("cleared %s in %s\n", key, cfg.Path) + } + _, err = io.WriteString(commandWriter(cmd), msg) + return err + }, + } +} + +// valueOrUnset renders an absent configuration value distinctly from an empty +// string set on disk. +func valueOrUnset(v string) string { + if v == "" { + return "(unset; built-in default applies)" + } + return v +} diff --git a/cmd/imds-broker/doctor.go b/cmd/imds-broker/doctor.go index 4861d34..a618e05 100644 --- a/cmd/imds-broker/doctor.go +++ b/cmd/imds-broker/doctor.go @@ -24,10 +24,7 @@ func doctorCommand() *cli.Command { profileFilterFlag(), }, Action: func(ctx context.Context, cmd *cli.Command) error { - w := cmd.Root().Writer - if w == nil { - w = os.Stdout - } + w := commandWriter(cmd) cfg, err := brokerconfig.Load(ctx) if err != nil { diff --git a/cmd/imds-broker/main.go b/cmd/imds-broker/main.go index 503fac6..e38d7f7 100644 --- a/cmd/imds-broker/main.go +++ b/cmd/imds-broker/main.go @@ -40,6 +40,7 @@ func main() { serveCommand(), profilesCommand(), mcpCommand(), + configCommand(), doctorCommand(), versionCommand(), }, diff --git a/cmd/imds-broker/main_test.go b/cmd/imds-broker/main_test.go index 6d80f1f..1dc0848 100644 --- a/cmd/imds-broker/main_test.go +++ b/cmd/imds-broker/main_test.go @@ -164,6 +164,61 @@ func TestEffectiveRegion_ConfigDefaultAndFlagOverride(t *testing.T) { func(c *cli.Command) { assert.Equal(t, "us-east-1", effectiveRegion(c, cfg)) }) } +// runConfigCmd runs the top-level config command with args, capturing stdout. +func runConfigCmd(t *testing.T, args ...string) (string, error) { + t.Helper() + var buf bytes.Buffer + cmd := &cli.Command{ + Name: "imds-broker", + Writer: &buf, + Commands: []*cli.Command{configCommand()}, + } + err := cmd.Run(context.Background(), append([]string{"imds-broker"}, args...)) + return buf.String(), err +} + +func TestConfigPath_PrintsResolvedLocation(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + out, err := runConfigCmd(t, "config", "path") + require.NoError(t, err) + assert.Contains(t, out, filepath.Join(dir, brokerconfig.RelPath)) +} + +func TestConfigList_ReportsUnsetWhenAbsent(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + out, err := runConfigCmd(t, "config", "list") + require.NoError(t, err) + assert.Contains(t, out, "file: not found") + assert.Contains(t, out, "profile-filter: (unset") +} + +func TestConfigSet_WritesThenListShows(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + out, err := runConfigCmd(t, "config", "set", "region", "ap-southeast-2") + require.NoError(t, err) + assert.Contains(t, out, "set region = ap-southeast-2") + + out, err = runConfigCmd(t, "config", "list") + require.NoError(t, err) + assert.Contains(t, out, "file: found") + assert.Contains(t, out, "region: ap-southeast-2") +} + +func TestConfigSet_RejectsWrongArgCount(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + _, err := runConfigCmd(t, "config", "set", "region") + require.Error(t, err) + assert.Contains(t, err.Error(), "expected ") +} + func TestEffectiveLogLevel_Precedence(t *testing.T) { withConfig := &brokerconfig.Config{LogLevel: "debug"} noConfig := &brokerconfig.Config{} diff --git a/pkg/config/config.go b/pkg/config/config.go index 63beb66..2163076 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -18,6 +18,7 @@ import ( "os" "path/filepath" "regexp" + "strings" "gopkg.in/yaml.v3" ) @@ -25,6 +26,16 @@ import ( // RelPath is the configuration path relative to the XDG config base directory. const RelPath = "imds-broker/config.yaml" +// Configuration key names accepted by Set and reported by List. +const ( + KeyProfileFilter = "profile-filter" + KeyRegion = "region" + KeyLogLevel = "log-level" +) + +// Keys lists the settable configuration keys in file order. +var Keys = []string{KeyProfileFilter, KeyRegion, KeyLogLevel} + // Config holds the effective host-side broker configuration. Empty string // values for ProfileFilter, Region, and LogLevel mean the key was absent and // the relevant built-in default applies. @@ -42,11 +53,60 @@ type Config struct { } // fileSchema mirrors the supported YAML keys. Strict decoding rejects any -// other key. +// other key. omitempty keeps cleared keys out of the written file. type fileSchema struct { - ProfileFilter string `yaml:"profile-filter"` - Region string `yaml:"region"` - LogLevel string `yaml:"log-level"` + ProfileFilter string `yaml:"profile-filter,omitempty"` + Region string `yaml:"region,omitempty"` + LogLevel string `yaml:"log-level,omitempty"` +} + +// validate checks a decoded schema against the value rules Load and Set share. +// Errors are path-free; callers add file context where the value originates +// from a file (Load) but not where it originates from the caller (Set). +func validate(schema fileSchema) error { + if schema.ProfileFilter != "" { + if _, err := regexp.Compile(schema.ProfileFilter); err != nil { + return fmt.Errorf("invalid profile-filter regex %q: %w", schema.ProfileFilter, err) + } + } + if schema.LogLevel != "" { + var lvl slog.Level + if err := lvl.UnmarshalText([]byte(schema.LogLevel)); err != nil { + return fmt.Errorf("invalid log-level %q: %w", schema.LogLevel, err) + } + } + return nil +} + +// decodeFile reads and strictly decodes the configuration file at path. A +// missing file yields a zero schema with found=false. A present but malformed, +// unknown-key, or multi-document file fails. +func decodeFile(path string) (schema fileSchema, found bool, err error) { + data, err := os.ReadFile(path) //nolint:gosec // path is host-controlled, not agent-controlled + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return fileSchema{}, false, nil + } + return fileSchema{}, false, fmt.Errorf("read config %q: %w", path, err) + } + + dec := yaml.NewDecoder(bytes.NewReader(data)) + dec.KnownFields(true) + // An empty file decodes to io.EOF; treat it as built-in defaults. + if err := dec.Decode(&schema); err != nil && !errors.Is(err, io.EOF) { + return fileSchema{}, false, fmt.Errorf("parse config %q: %w", path, err) + } + // Reject multi-document files. A single Decode reads only the first + // document, so additional documents would be silently ignored, including + // any unknown keys. Fail closed: a multi-document config is an operator + // mistake. + if err := dec.Decode(new(fileSchema)); !errors.Is(err, io.EOF) { + if err != nil { + return fileSchema{}, false, fmt.Errorf("parse config %q: %w", path, err) + } + return fileSchema{}, false, fmt.Errorf("parse config %q: multiple YAML documents are not supported", path) + } + return schema, true, nil } // ResolvePath returns the configuration file path. It uses XDG_CONFIG_HOME when @@ -77,43 +137,17 @@ func Load(ctx context.Context) (*Config, error) { cfg := &Config{Path: path} - data, err := os.ReadFile(path) //nolint:gosec // path is host-controlled, not agent-controlled + schema, found, err := decodeFile(path) if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return cfg, nil - } - return nil, fmt.Errorf("read config %q: %w", path, err) - } - cfg.Found = true - - var schema fileSchema - dec := yaml.NewDecoder(bytes.NewReader(data)) - dec.KnownFields(true) - // An empty file decodes to io.EOF; treat it as built-in defaults. - if err := dec.Decode(&schema); err != nil && !errors.Is(err, io.EOF) { - return nil, fmt.Errorf("parse config %q: %w", path, err) + return nil, err } - // Reject multi-document files. A single Decode reads only the first - // document, so additional documents would be silently ignored, including - // any unknown keys. Fail closed: a multi-document config is an operator - // mistake. - if err := dec.Decode(new(fileSchema)); !errors.Is(err, io.EOF) { - if err != nil { - return nil, fmt.Errorf("parse config %q: %w", path, err) - } - return nil, fmt.Errorf("parse config %q: multiple YAML documents are not supported", path) + if !found { + return cfg, nil } + cfg.Found = true - if schema.ProfileFilter != "" { - if _, err := regexp.Compile(schema.ProfileFilter); err != nil { - return nil, fmt.Errorf("invalid profile-filter regex %q in %q: %w", schema.ProfileFilter, path, err) - } - } - if schema.LogLevel != "" { - var lvl slog.Level - if err := lvl.UnmarshalText([]byte(schema.LogLevel)); err != nil { - return nil, fmt.Errorf("invalid log-level %q in %q: %w", schema.LogLevel, path, err) - } + if err := validate(schema); err != nil { + return nil, fmt.Errorf("config %q: %w", path, err) } cfg.ProfileFilter = schema.ProfileFilter @@ -121,3 +155,50 @@ func Load(ctx context.Context) (*Config, error) { cfg.LogLevel = schema.LogLevel return cfg, nil } + +// Set updates a single configuration key on disk and returns the reloaded +// configuration. It creates the file and its parent directory if absent and +// preserves other keys. An empty value clears the key. Setting an existing but +// malformed file fails rather than overwriting operator content. +// +// This command is host-side and writes a host-controlled path. It is not an +// agent-reachable interface: see the package doc and README sandbox model. +func Set(ctx context.Context, key, value string) (*Config, error) { + path, err := ResolvePath(ctx) + if err != nil { + return nil, err + } + + schema, _, err := decodeFile(path) + if err != nil { + return nil, err + } + + switch key { + case KeyProfileFilter: + schema.ProfileFilter = value + case KeyRegion: + schema.Region = value + case KeyLogLevel: + schema.LogLevel = value + default: + return nil, fmt.Errorf("unknown configuration key %q; valid keys: %s", key, strings.Join(Keys, ", ")) + } + + if err := validate(schema); err != nil { + return nil, err + } + + data, err := yaml.Marshal(schema) + if err != nil { + return nil, fmt.Errorf("marshal config: %w", err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return nil, fmt.Errorf("create config dir: %w", err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + return nil, fmt.Errorf("write config %q: %w", path, err) + } + + return Load(ctx) +} diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index f1479d9..06781c9 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -109,6 +109,82 @@ func TestLoad_InvalidLogLevelFails(t *testing.T) { assert.Contains(t, err.Error(), "log-level") } +func TestSet_CreatesFileWhenAbsent(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + cfg, err := Set(context.Background(), KeyProfileFilter, ".*ViewOnly.*") + require.NoError(t, err) + assert.True(t, cfg.Found) + assert.Equal(t, ".*ViewOnly.*", cfg.ProfileFilter) + + path := filepath.Join(dir, RelPath) + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) +} + +func TestSet_PreservesOtherKeys(t *testing.T) { + writeConfig(t, "profile-filter: \".*ViewOnly.*\"\nregion: \"ap-southeast-2\"\n") + + cfg, err := Set(context.Background(), KeyLogLevel, "debug") + require.NoError(t, err) + assert.Equal(t, ".*ViewOnly.*", cfg.ProfileFilter) + assert.Equal(t, "ap-southeast-2", cfg.Region) + assert.Equal(t, "debug", cfg.LogLevel) +} + +func TestSet_EmptyValueClearsKey(t *testing.T) { + writeConfig(t, "profile-filter: \".*ViewOnly.*\"\nregion: \"ap-southeast-2\"\n") + + cfg, err := Set(context.Background(), KeyProfileFilter, "") + require.NoError(t, err) + assert.Empty(t, cfg.ProfileFilter) + assert.Equal(t, "ap-southeast-2", cfg.Region) +} + +func TestSet_UnknownKeyFails(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + _, err := Set(context.Background(), "bogus", "x") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown configuration key") +} + +func TestSet_InvalidValueFails(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + _, err := Set(context.Background(), KeyProfileFilter, "[invalid") + require.Error(t, err) + assert.Contains(t, err.Error(), "profile-filter") + + _, err = Set(context.Background(), KeyLogLevel, "verbose") + require.Error(t, err) + assert.Contains(t, err.Error(), "log-level") +} + +func TestSet_MalformedExistingFileFails(t *testing.T) { + writeConfig(t, "profile-filter: \"x\nregion: [unterminated\n") + + _, err := Set(context.Background(), KeyRegion, "ap-southeast-2") + require.Error(t, err) +} + +func TestSet_RoundTripsThroughLoad(t *testing.T) { + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + _, err := Set(context.Background(), KeyRegion, "ap-southeast-2") + require.NoError(t, err) + + cfg, err := Load(context.Background()) + require.NoError(t, err) + assert.Equal(t, "ap-southeast-2", cfg.Region) + assert.Empty(t, cfg.ProfileFilter) +} + func TestLoad_UnreadableFileFails(t *testing.T) { if os.Geteuid() == 0 { t.Skip("root bypasses file permissions") From ca01d8c04fe29268f57dffbe3dd8e133bde75dc9 Mon Sep 17 00:00:00 2001 From: James Telfer <792299+jamestelfer@users.noreply.github.com> Date: Mon, 6 Jul 2026 16:57:07 +1000 Subject: [PATCH 2/2] refactor(config): derive set usage from Keys and write atomically Build the `config set` valid-keys usage text from brokerconfig.Keys so it cannot drift from the canonical list. Replace the direct os.WriteFile in Set with a temp-file write plus rename so an interrupted write cannot truncate the operator's existing config. --- cmd/imds-broker/config.go | 3 +-- pkg/config/config.go | 24 +++++++++++++++++++++--- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/cmd/imds-broker/config.go b/cmd/imds-broker/config.go index bd0846b..6422dfa 100644 --- a/cmd/imds-broker/config.go +++ b/cmd/imds-broker/config.go @@ -83,8 +83,7 @@ func configSetCommand() *cli.Command { Name: "set", Usage: "Set a configuration value (creates the file if absent)", ArgsUsage: " ", - Description: "Valid keys: " + brokerconfig.KeyProfileFilter + ", " + - brokerconfig.KeyRegion + ", " + brokerconfig.KeyLogLevel + + Description: "Valid keys: " + strings.Join(brokerconfig.Keys, ", ") + ". An empty value clears the key.", Action: func(ctx context.Context, cmd *cli.Command) error { args := cmd.Args() diff --git a/pkg/config/config.go b/pkg/config/config.go index 2163076..f27d246 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -193,11 +193,29 @@ func Set(ctx context.Context, key, value string) (*Config, error) { if err != nil { return nil, fmt.Errorf("marshal config: %w", err) } - if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o700); err != nil { return nil, fmt.Errorf("create config dir: %w", err) } - if err := os.WriteFile(path, data, 0o600); err != nil { - return nil, fmt.Errorf("write config %q: %w", path, err) + + // Write to a temp file in the same directory, then rename over the target. + // Rename is atomic on the same filesystem, so the existing config is only + // replaced on a fully written file. os.CreateTemp creates the file 0o600. + tmp, err := os.CreateTemp(dir, ".config-*.yaml.tmp") + if err != nil { + return nil, fmt.Errorf("create temp config: %w", err) + } + tmpName := tmp.Name() + defer func() { _ = os.Remove(tmpName) }() // no-op once the rename succeeds + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return nil, fmt.Errorf("write temp config: %w", err) + } + if err := tmp.Close(); err != nil { + return nil, fmt.Errorf("close temp config: %w", err) + } + if err := os.Rename(tmpName, path); err != nil { + return nil, fmt.Errorf("replace config %q: %w", path, err) } return Load(ctx)