Repository navigation
Conversation
|
🌿 Preview your docs: https://nvidia-preview-pr-5489.docs.buildwithfern.com/openshell |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. cli — No explicit risk acceptance foundFor
Review details
Advisory review; publication gates are unchanged. |
69a79e1 to
6903a84
Compare
|
|
||
| `name` assigns an operator-facing identity to the gateway installation. Set it with `[openshell.gateway].name`, `--name`, or `OPENSHELL_GATEWAY_NAME`. It defaults to `openshell`; the Helm chart defaults it to the chart fullname so all replicas in one installation share a name. Chart fullnames are only unique within their Kubernetes namespace, so set `server.name` explicitly when one collector receives telemetry from multiple namespaces or clusters. This identity is independent of client-side gateway aliases, TLS names, and `gateway_jwt.gateway_id`. | ||
|
|
||
| ## Drop-In Fragments |
There was a problem hiding this comment.
Is Drop-In fragments how this is referred to in other ecosystems?
TaylorMutch
left a comment
TaylorMutch
left a comment
There was a problem hiding this comment.
Two error-attribution bugs remain in the layered loader. Both affect the requirement in #5446 that errors name the file that caused them.
Error attribution can report an error already fixed by a fragment
attribute_to_layer returns the first invalid intermediate configuration without checking whether its error is the error in the final merged configuration:
OpenShell/crates/openshell-server/src/config_file.rs
Lines 1895 to 1902 in 6903a84
Reproduction:
# gateway.toml
[openshell]
version = 2
[openshell.gateway]
database_max_connections = "bad"# gateway.d/10-fix.toml
[openshell.gateway]
database_max_connections = 10# gateway.d/20-bad.toml
[openshell.gateway]
not_a_field = 1The final merged configuration has a valid pool size and an unknown field from 20-bad.toml. Instead, startup reports gateway.toml: invalid type: a string, expected u32. Operators are directed to fix a value that has already been overridden, and the actual failure is hidden. Attribute the error in the final merged configuration rather than returning an unrelated prefix error.
Semantic failures lose the offending fragment's path
attribute_to_layer only attaches a path to Parse errors. InvalidValue and SecretInFile retain no source path, so preflight_layered falls back to the base file:
OpenShell/crates/openshell-server/src/config_file.rs
Lines 1926 to 1930 in 6903a84
With a valid base containing [openshell] version = 2 and this fragment:
# gateway.d/20-bad.toml
[openshell.gateway]
database_max_connections = 0Preflight reports path='.../gateway.toml' category=malformed detected_version=2, although the invalid value comes from 20-bad.toml. Startup's semantic error also omits the fragment path. Before layering, the single selected file made the source unambiguous; with drop-ins, operators are sent to the wrong file. Preserve the originating path for semantic validation errors as well as parse errors.
Verification: both behaviors reproduced in an isolated probe using this commit's loader, merge, and attribution functions with a reduced schema. The full server tests could not run offline because hickory-proto v0.26.2 was not cached. The Ansible integration path was inspected but not executed.
matthewgrossman
left a comment
matthewgrossman
left a comment
There was a problem hiding this comment.
Reviewed the merged-validation design and the tmachine migration. The approach is sound; the notes below are mostly about error attribution and untested paths. Inline comments follow.
| path: path.clone(), | ||
| detail, | ||
| }, | ||
| other => other, |
There was a problem hiding this comment.
Only Parse errors get re-pathed here; every other variant (SecretInFile, semantic validation errors) falls through to other => other with no file name. Before this PR an operator with one file got a path-specific error. With several fragments, they now see e.g. a secret rejection but can't tell which fragment holds it, yet configuration.mdx says "An error names the first file that causes it." The tests only assert attribution for Parse (layered_secret_in_fragment_is_rejected checks the variant, not the file). Either attribute the other variants too or soften the docs.
| merged | ||
| .try_into() | ||
| .map_err(|error: toml::de::Error| ConfigFileError::Parse { | ||
| path: PathBuf::new(), |
There was a problem hiding this comment.
If a merged-result Parse error is never attributed to a layer (the cumulative replay in attribute_to_layer passes for every prefix), it keeps this empty PathBuf::new(), and preflight_layered forwards that path into the stable failure message, so the operator sees a blank path. Consider falling back to the base or directory path here.
| /// | ||
| /// Hidden files and entries without a `.toml` extension are ignored so editor | ||
| /// backups and package-manager leftovers never take effect. | ||
| pub fn fragment_paths(dir: &Path) -> Result<Vec<PathBuf>, ConfigFileError> { |
There was a problem hiding this comment.
Fragments are read through read_config_contents, so O_NOFOLLOW and the non-regular-file check now apply to every file in gateway.d. A symlinked fragment, which is common in /etc/*.d setups, will fail startup. That matches the base file, but the docs don't say so for fragments; worth one sentence in configuration.mdx.
| }; | ||
| anchor | ||
| .parent() | ||
| .map(|parent| parent.join("gateway.d")) |
There was a problem hiding this comment.
Before this PR, a gateway.d directory next to an explicit --config file had no effect. Now any *.toml in it is merged silently, so a leftover or unrelated directory can change gateway behavior on upgrade. This is the stated design and preflight lists merged files, but it's worth a line in the release note.
| - openshell_gateway_overrides.stat.exists | ||
| - openshell_gateway_merge_default | default('') | length > 0 | ||
|
|
||
| - name: Write a minimal base for a package with no default configuration |
There was a problem hiding this comment.
This task only runs when openshell_gateway_merge_default is set and the file is missing. I couldn't find where that variable is set from this diff. If an installer (deb, binaries) leaves it unset and no base file exists, the fragment lands with no base and the gateway fails with a missing version. The PR notes the Ansible changes were only parsed as YAML, so please run at least one packaged tmachine environment before merging.
| - Hidden files and files without a `.toml` extension are ignored. | ||
| - A fragment can omit `[openshell] version`. The merged result must declare version 2. | ||
|
|
||
| The gateway validates the merged result once, including unknown fields and values that must come from the environment. An error names the first file that causes it. The precedence is: command-line flag, then `OPENSHELL_*` environment variable, then fragments, then the base file, then the built-in default. |
There was a problem hiding this comment.
See the comment on attribute_to_layer: this sentence is only true for parse errors today.
Load `*.toml` drop-in configs from a directory and merge them over the base config in lexical order, so operators and packages can customize a gateway without editing the base file. Select the directory with `--config-dir` or `OPENSHELL_GATEWAY_CONFIG_DIR`, or let the gateway use `gateway.d` beside the base file. `--config` and `--config-dir` compose. Tables merge key by key; scalars and arrays replace; a drop-in cannot unset a key. Every file must declare `[openshell] version = 2`, but a drop-in may set only some keys of a table. The merged result is validated once, and an error names the file that sets the offending key. Errors that no single key explains name the merged configuration and its files. Unknown keys are rejected. Follow a drop-in symlink only when it resolves inside the drop-in directory, which covers Kubernetes ConfigMap mounts, and keep the base file strict. Treat a `gateway.d` with no drop-in configs and no base file as no configuration. `config preflight` loads the same merged result as startup and lists the files it merged. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Install an environment's gateway overrides as `gateway.d/90-test-overrides.toml` next to the gateway's own config instead of merging them into it with a script. Write a minimal base file when a package has no default, and have the override files declare `[openshell] version = 2`. Signed-off-by: Evan Lezar <elezar@nvidia.com>
Match the daemon flag so the same name selects the base config file in both places. Keep --path for compatibility. Signed-off-by: Evan Lezar <elezar@nvidia.com>
6903a84 to
22fb7a9
Compare
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. configuration — No explicit risk acceptance foundopenshell-gateway --config FILE and config preflight --path FILE previously read only FILE but now automatically merge adjacent gateway.d/*.toml files.
Review details
Advisory review; publication gates are unchanged. |
Summary
The gateway now merges
*.tomldrop-in configs from a directory over its base config file, so operators and test harnesses can customize a package-seededgateway.tomlwithout editing it. Test environments use the same mechanism instead of a merge script.Related Issue
Closes #5446
Changes
load_layeredinconfig_file.rs: load the base file and the drop-in configs, merge them in lexical order, and validate the merged result once. Tables merge key by key; scalars and arrays replace; a drop-in cannot unset a key.[openshell] version = 2, but let a drop-in set only some keys of a table, such as one key under[openshell.gateway.tls]. Unknown keys are rejected.serde_path_to_error. An error that no single key explains, such as a required key no file sets, names the merged configuration and lists its files. Parse errors stay free of file contents.--config-dir/OPENSHELL_GATEWAY_CONFIG_DIR. Without them, the gateway usesgateway.d/next to the base file when it exists. An explicit directory must exist. Agateway.d/with no drop-in configs and no base file counts as no configuration.--configand--config-dircompose rather than exclude each other, as in CRI-O and kubelet.config preflightload the same merged result, list the files it merged, and fail on the same errors as startup. A legacy schema version still reports its version, and a malformed version still reportsdetected_version=unavailable.--configas an alias forconfig preflight --pathto match the daemon flag. It is a separate commit.configuration.mdx, the RPM guide and the man page.gateway.d/90-test-overrides.toml, write a minimal base file when a package has no default, and removemerge-gateway-toml.pyand its tests.Not covered: the Helm ConfigMap and Secret mapping from the issue's open questions, and the e2e additional-CA overrides, which still edit the base file because
e2e/rust/tests/additional_ca.rsrewrites it in place. Implicitgateway.d/discovery applies beside any--configpath, so a drop-in directory in a shared writable location is merged; it deserves a separate look.Release Notes
Gateway configuration can be split across a base
gateway.tomland*.tomldrop-in configs ingateway.d/. Later files override earlier ones, andconfig preflightreports the merged result. Agateway.d/directory next to an existing--configfile is now merged, so remove files you do not want applied.Testing
cargo clippy -p openshell-server --all-targetsandcargo fmt --checkare clean and theopenshell-serverlib tests pass (2701 tests); the pre-commit lint hook passed on each commit.--configalias and layered preflight.Checklist