feat(gateway): add config set overrides by matthewgrossman · Pull Request #2350 · NVIDIA/OpenShell

matthewgrossman · 2026-07-17T23:13:28Z

Summary

Add a generic, atomic openshell-gateway config set command and expose repeatable configuration overrides through the local Docker gateway task.

Related Issue

N/A — extracted from the config-management work in #2137 following the engineering discussion.

Changes

add repeatable dotted KEY=VALUE assignments for gateway TOML updates
preserve comments, validate the resulting gateway schema, and replace the config atomically
add mise run gateway:docker -- --set KEY=VALUE passthrough after generated config creation
document the command and local development workflow
keep multi-file config merging out of scope

Testing

mise run pre-commit passes
Unit tests added/updated
E2E tests added/updated (not applicable; no sandbox infrastructure behavior changed)

Checklist

Follows Conventional Commits
Commits are signed off (DCO)

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

copy-pr-bot · 2026-07-17T23:13:31Z

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

github-actions · 2026-07-17T23:15:42Z

🌿 Preview your docs: https://nvidia-preview-pr-2350.docs.buildwithfern.com/openshell

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

pimlock · 2026-07-20T20:11:16Z

+                }
+                Ok(component.to_string())
+            })
+            .collect::<std::result::Result<Vec<_>, _>>()?;


I think it makes sense to only support flat keys, i.e. without arrays (e.g. foo.bar[0].baz).

We don't have many things that use arrays, but now have interceptors and middlewares that are arrays. So editing these would be a manual config edit still.

I think that's okay for now? I can just document that for now I guess

Yeah, that makes sense.
I was thinking for more complex config edits, it would be nice to be able to do it in the editor, e.g. config edit or config set --edit, or similar.Not for this PR, just food for thought.

I kept array element addressing out of scope and documented that arrays must be replaced as complete values in config set --help, the man page, and the gateway configuration docs.

I also changed assignments to use Cargo-style TOML syntax rather than adding an OpenShell-specific path language. An interactive config edit command sounds like a useful follow-up for more complex edits, but I’m leaving it out of this PR.

pimlock · 2026-07-20T20:19:24Z

+
+#[derive(Debug, Subcommand)]
+enum ConfigCommand {
+    /// Update fields in the gateway TOML configuration.


Worth mentioning the config is updated in place? I think the name set is quite clear already, but maybe worth making sure?

I'm wondering if there is anything that could go wrong, e.g. someone overrides something that is hard to recover? Looking at other tools, it doesn't seem a concern they deal with.

Updated config set --help to say that the selected gateway TOML file is validated and atomically replaced. I used “atomically replaced” instead of “updated in place” because the implementation writes a temporary file and renames it over the destination.

pimlock · 2026-07-20T20:22:47Z

+
+#[derive(Debug, Args)]
+pub struct ConfigArgs {
+    #[command(subcommand)]


Do we need to add some properties here, e.g. the help_template, after_help, etc. to keep the help output consistent?

I checked the existing CLI conventions. The custom help templates are used by the user-facing openshell CLI, while openshell-gateway, including generate-certs, currently uses Clap’s default formatting. I kept the default formatting for consistency within this binary and added the relevant atomic-replacement and array behavior to the generated long help.

elezar · 2026-07-21T11:14:56Z

+
+#[derive(Clone, Debug)]
+struct Assignment {
+    key: Vec<String>,


Probably a nit, but in the toml context, I would argue that key is the string representation root.table.table2.setting, whereas the path is the Vec representation (split by .).

Done. The parsed component vector is now named Assignment::path.

elezar · 2026-07-21T11:36:30Z

+    }
+}
+
+fn parse_assignment_value(raw: &str) -> Item {


Could we add focused tests for RHS parsing and use toml_edit’s value parser directly here?

Proposed parser shape:

fn parse_assignment_value(raw: &str) -> Result<Item, String> { if let Ok(item) = raw.parse::<Item>() { return Ok(item); } if raw.contains(['\n', '\r']) { return Err("invalid assignment value: expected a single TOML value or an unquoted string".into()); } Ok(value(raw)) }

Proposed test cases:

openshell.gateway.log_level="""debug\ntrace""" is accepted as a multiline basic string.

openshell.gateway.log_level='''debug\\n\ntrace''' is accepted as a multiline literal string.

openshell.gateway.log_level=42\nother = true is rejected as an invalid value fragment.

openshell.gateway.log_level=42\nother is rejected as a multiline unquoted value.

That would pin the intended boundary between “single TOML value”, “single-line bare string fallback”, and invalid multiline input, while avoiding the current fake-document parse.

Addressed by removing the fake-document RHS parser and parsing the complete argument as exactly one TOML assignment. This reuses toml_edit for both the key and value grammar and deliberately removes the custom unquoted-string fallback, matching Cargo-style configuration overrides.

Added coverage for multiline basic and literal strings, multiple assignments, unquoted strings, and table-header fragments. Trailing assignments are now rejected instead of being silently discarded.

elezar · 2026-07-21T11:38:50Z

+        .ok_or_else(|| miette::miette!("gateway config key '{key}' must be a TOML table"))
+}
+
+fn write_atomically(path: &Path, contents: &[u8]) -> Result<()> {


This follows the same basic tempfile + persist pattern we already use elsewhere, but we now have a couple of local atomic file writers. Could we either reuse/extract a small shared helper, or add tests here for the metadata we care about? In particular, the current implementation preserves mode bits but not uid/gid on Unix, which can matter for managed config files.

Good catch. The latest commit does not address the uid/gid case: the writer currently preserves mode bits, but the replacement file is owned by the user running the command.

I think this thread should remain open while I add a Unix mode-preservation test and decide whether to preserve uid/gid or explicitly limit the supported guarantee to caller-owned configuration files.

elezar · 2026-07-21T11:46:11Z

+            format!("invalid assignment '{input}': expected a dotted KEY=VALUE argument")
+        })?;
+
+        let key = raw_key


Could we use toml_edit::Key::parse(raw_key) for the assignment path instead of splitting on . manually? The current parser only supports bare key components, and it will mis-split valid TOML quoted keys such as:

openshell.drivers."containerd.io".socket_path=/run/containerd/containerd.sock

With raw_key.split('.'), that becomes "containerd and io" as separate components, so there is no way to set driver tables or future config keys whose TOML key contains a dot. toml_edit::Key::parse already understands TOML dotted-key syntax and would keep this aligned with the value parsing side.

Addressed by parsing the complete KEY=VALUE argument as TOML instead of splitting the key manually. This uses toml_edit’s TOML key parser internally and supports quoted components such as openshell.drivers."containerd.io".socket_path.

Added both parser-level and end-to-end coverage for a quoted key containing a dot.

elezar · 2026-07-21T11:52:19Z

+    Ok(())
+}
+
+fn set(path: &Path, settings: &SetArgs) -> Result<()> {


nit: In the context of Go, I've often found that splitting something like this into function working on different types (Path -> io.Writer / io.Reader -> Config object) make things simpler to test without needing to go through the file system.

Assignment parsing is now isolated in Assignment::from_str and can be tested without filesystem access. I left the read-transform-validate-write flow together for now because the existing tests exercise important filesystem behavior such as atomic replacement and leaving the original untouched after validation failure.

hmm revisiting this before trying to resolve, I think I agree here and will make a quick fix

I separated config reading, in-memory transformation, validation, and atomic writing. The transformation is now testable without filesystem access, and set is just the "orchestration" function to tie all those testable bits together

elezar · 2026-07-21T11:53:54Z

+    Ok(())
+}
+
+fn set(path: &Path, settings: &SetArgs) -> Result<()> {


Question: Can set be a method (possibly apply?) on the SetArgs type? At the very least, does it make sense to reorder the arguments to make it clear that path is the output?

Renamed this to update_file to make the read-modify-write behavior explicit. I kept it as a free func so SetArgs remains a CLI input type, and retained path-first ordering because the path is both the source and destination.

elezar · 2026-07-21T11:56:06Z

+    let mut document = if original.trim().is_empty() {
+        DocumentMut::new()
+    } else {
+        original
+            .parse::<DocumentMut>()
+            .into_diagnostic()
+            .wrap_err_with(|| format!("failed to parse gateway config '{}'", path.display()))?
+    };
+
+    let openshell = ensure_table(document.as_table_mut(), "openshell")?;
+    if !openshell.contains_key("version") {
+        openshell.insert("version", value(i64::from(config_file::SCHEMA_VERSION)));
+    }


This seems like it should be common config handling (i.e. config.load()). How does OpenShell currently handle a config file that doesn't exist? Should this not generate a default config, or is this only done on parsing?

elezar · 2026-07-21T11:57:14Z

+        .unwrap_or_else(|| value(raw))
+}
+
+fn apply_assignment(document: &mut DocumentMut, assignment: &Assignment) -> Result<()> {


Maybe a function on Assignment?

Addressed by removing the standalone value parser. Assignment::from_str now parses the complete TOML assignment and produces both the logical path and value.

elezar · 2026-07-21T12:04:38Z

+    }
+
+    let rendered = document.to_string();
+    config_file::parse(&rendered, path).map_err(|err| miette::miette!("{err}"))?;


Could we add coverage for setting a field inside a nested required table on a missing config file, and either support or document the expected behavior? For example:

openshell-gateway config set openshell.gateway.tls.cert_path=/tmp/cert.pem

starts from an empty document when the config file does not exist, then creates [openshell.gateway.tls] with only cert_path. Since TlsConfig also requires key_path, final schema validation should fail before writing. That may be the right behavior, but it would be good to pin it with a test and clarify that creating nested config objects from scratch may require setting all required sibling fields in the same invocation.

Agreed. The current behavior is safe—schema validation fails before writing—but the missing-file case is not yet explicitly tested or documented.

This was not addressed in the latest commit, so I’m leaving the thread open. I plan to add a failure test proving that setting only tls.cert_path does not create the file, a success test setting both required TLS paths together, and a short documentation note.

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

feat(gateway): add config set overrides

56c14da

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

fix(gateway): preserve config update metadata

9bd284e

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

matthewgrossman marked this pull request as ready for review July 17, 2026 23:49

matthewgrossman requested review from a team, derekwaynecarr, maxamillion and mrunalp as code owners July 17, 2026 23:49

matthewgrossman requested a review from pimlock July 20, 2026 16:47

pimlock reviewed Jul 20, 2026

View reviewed changes

elezar reviewed Jul 21, 2026

View reviewed changes

matthewgrossman added 2 commits July 23, 2026 12:43

use toml syntax instead of custom syntax

3e3088f

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

code review

e3f1d9f

Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>

Uh oh!

Conversation

matthewgrossman commented Jul 17, 2026

Summary

Related Issue

Changes

Testing

Checklist

Uh oh!

copy-pr-bot Bot commented Jul 17, 2026

Uh oh!

github-actions Bot commented Jul 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewgrossman Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matthewgrossman Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

matthewgrossman Jul 23, 2026 •

edited

Loading

matthewgrossman Jul 23, 2026 •

edited

Loading