Honor all runtime_config fields over the workload API - #6214
Open
jhrozek wants to merge 2 commits into
Open
Conversation
The field-by-field copies of templates.RuntimeConfig rot as the struct grows: they were complete when written, then RuntimeEnv and BuildWith were added and each one silently stopped being carried. Clone and WithOverrides put the copy/merge logic on the type itself, so the enumeration of all four fields lives in one file next to the struct declaration instead of being reimplemented at each call site. A field can still be forgotten in Clone or WithOverrides when a new one is added, but a guard test now fails the moment RuntimeConfig's field count changes, forcing that update to happen. WithOverrides (renamed from MergedWith, base.WithOverrides(override) instead of a symmetric-sounding name that doesn't say which side wins) starts from a copy of the base struct, so an unhandled future field defaults to base-wins rather than a zero value. Clone starts the same way. Both guard against a nil receiver instead of panicking. GetDefaultRuntimeConfig now returns a value already detached from the package-global RuntimeDefaults map (via Clone internally), retiring the whole class of aliasing bugs at the source instead of requiring every caller to remember to clone what they get back. The build-constraint check (BuildWith is only supported for uvx builds) moves into the templates package as RuntimeConfig.ValidateFor, next to Validate and the defaults it needs. loadRuntimeConfig now runs every runtime config it returns - override, config-file, and default fallback alike - through ValidateFor, so the constraint can't be silently skipped on one of the three paths the way a caller-side check could be forgotten on a fourth. Also rename the build-constraint rejection message from --build-with to build_with. The check lives in pkg/ and is reachable from the REST API, the TUI and the user config file, so naming a CLI flag misleads every non-CLI caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The REST API's request type advertises all four RuntimeConfig fields (builder_image, additional_packages, build_with, runtime_env) and publishes them in swagger, but the service layer only ever copied the first two. A caller POSTing runtime_config.build_with got 201 Created and a workload built with unconstrained dependencies, silently - exactly the failure build_with exists to prevent (#6108). runtime_env was dropped the same way. Naively plumbing the two missing fields would trade that silent drop for an opaque 500: the build-constraint rejection lived deep in the imageRetriever path, and pkg/api/errors/handler.go scrubs any >=500 body down to bare status text. So this also moves where validation happens. runtimeConfigForImageBuild now merges the request onto the transport's base config with WithOverrides and validates the result with ValidateFor before it ever reaches the retriever, and that error is wrapped in retriever.ErrInvalidRunConfig, which is coded 400 and returned to the client intact. runtimeConfigFromRequest now clones the request's RuntimeConfig and normalizes it in place instead of copying it field by field, so a future field is carried automatically instead of needing a new branch. Deleted validateRuntimeConfig and isValidRuntimePackageName in favor of templates.RuntimeConfig.Validate(), which is strictly stronger: it reports every problem instead of the first, and closes a gap where ".foo"/"_foo" package names were accepted by the API but rejected at build time. The emptiness short-circuit in runtimeConfigFromRequest was itself still a hand-enumeration of all four fields, one line below the fix - a fifth field would be dropped there exactly as build_with was. Added templates.RuntimeConfig.IsEmpty() next to Clone and WithOverrides, and extended the field-count guard test to cover all three. WithOverrides also discarded the base's BuildWith unconditionally, which is fine for the CLI's static defaults (which never set it) but wrong for the API's base, which is the user's config file: a request setting only an unrelated field would silently drop a globally pinned build_with. BuildWith now falls back to the base when the override has none, matching BuilderImage's "override wins if set" rule. Carrying these fields through to responses exposed a round trip that was already broken for builder_image and additional_packages: a workload built from a protocol scheme persists the built image, not the uvx:// URI it came from, so GET returns a runtime_config that PUT then rejects with 400 as "only supported for protocol-scheme images". A client doing GET, edit, PUT could not save an existing protocol-built workload back. The update path now recognizes an inert echo - nothing to rebuild - only when the request's image, URL, and runtime_config all exactly match what is already persisted. The persisted config is threaded into BuildFullRunConfig so the echo is skipped for the retriever/build input alone: the request's runtime_config is never cleared, so the RunConfig the policy gate evaluates always carries it and a policy cannot be bypassed by echoing an unchanged config back. Anything else - a different image, a different URL, or a runtime_config that doesn't match - still returns 400, so a genuine attempt to configure a plain image, or to redirect an existing workload elsewhere, is not silently discarded. The rule is documented on the update endpoint, since swaggo drops descriptions on $ref fields and clients could not otherwise discover it. Loading the persisted state to check for an echo can itself fail, and that failure was being swallowed. A missing state file falls through to the existing protocol-scheme rejection - the workload exists, only its state file doesn't, so 400 is still the right answer - but any other load error (a corrupt file, a cancelled context) is now returned directly instead of silently disabling the echo check and producing a misleading 400 that hides the real cause. The req.URL == "" guard on WithRuntimeConfig meant an accepted echo of a remote workload's runtime_config was silently dropped from the rebuilt RunConfig even though runtimeConfigForImageBuild had already decided the request was an inert match - BuildFullRunConfig only attached the override when the request carried no URL. Removed that guard: a non-nil override here is either a protocol-scheme build (already validated above) or an accepted echo on an otherwise-rejected image/URL, and both must reach the RunConfig regardless of whether the workload is remote. The echo comparison normalized only the request's side before calling reflect.DeepEqual against the persisted value, so a config that reached storage before whitespace-trimming existed, or with a nil-vs-empty collection difference, could fail to match its own unchanged echo and be rejected as if it were a real change. Extracted the trim-and-filter logic out of runtimeConfigFromRequest into a shared normalizeRuntimeConfig helper and applied it to the persisted side of the comparison too. The state-load guard for echo detection checked the request's raw RuntimeConfig field instead of its normalized form, so a semantically empty "runtime_config": {} triggered a state read - and a failure on that read - for a request where no echo comparison was ever going to happen. Gated the LoadState call on the normalized value instead. The create endpoint's swagger annotation now documents the same protocol-scheme restriction, so callers aren't left to discover the 400 by trial and error. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
aponcedeleonch,
blkt,
rdimitrov and
reyortiz3
as code owners
August 5, 2026 16:14
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6214 +/- ##
==========================================
- Coverage 72.51% 72.47% -0.04%
==========================================
Files 739 739
Lines 76719 76732 +13
==========================================
- Hits 55629 55614 -15
- Misses 17107 17154 +47
+ Partials 3983 3964 -19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
*templates.RuntimeConfigand swagger publishes all four of its fields, but the service layer only ever copiedbuilder_imageandadditional_packages. A caller POSTingruntime_config.build_withgot201 Createdand a workload built with unconstrained dependencies — silently, which is exactly the failurebuild_withexists to prevent.runtime_envwas dropped the same way.runtime_env(Add runtime-stage environment variables to protocol Dockerfiles #5801) andbuild_with(Rename --uv-with to --build-with; reject constraints on unsupported ecosystems #6116) were added and each silently stopped being carried — the "parallel types that drift" anti-pattern from.claude/rules/go-style.md. The first commit moves copy and merge onto the type that owns the fields (Clone,WithOverrides,ValidateFor,IsEmpty); the second makes the API use them and honor all four fields.imageRetrieverpath, andpkg/api/errors/handler.goscrubs any ≥500 body to bare status text. So validation moved.runtimeConfigForImageBuildnow merges and validates before the retriever, and that error is wrapped inretriever.ErrInvalidRunConfig— coded 400 and returned intact.builder_image/additional_packages: a protocol-built workload persists the built image, so GET returned aruntime_configthat PUT rejected with 400, andGET → edit → PUTcould not save an existing workload back.Fixes #6210
Type of change
Test plan
task test)task lint-fix)Every new test was confirmed to fail against the pre-fix code before being accepted. That mattered here: an earlier iteration of the round-trip fix asserted only
HTTP 200and passed while silently erasing the workload's build configuration. Asserting a mutation's status code without asserting its effect certifies the wrong thing.Changes
pkg/container/templates/runtime_config.goClone,WithOverrides,ValidateFor,IsEmptyon the type;GetDefaultRuntimeConfigreturns a detached valuepkg/runner/protocol.gomergeRuntimeConfig/mergeEnvMapsdeleted; constraint check extractedpkg/api/v1/workload_service.govalidateRuntimeConfig/isValidRuntimePackageNamedeleted for the type's own validation;ValidateForplaced so violations return 400; inert-echo exception; sharednormalizeRuntimeConfigpkg/api/v1/workload_types.goruntime_configfield doc rewritten; response path clonespkg/api/v1/workloads.godocs/server/**_test.goDoes this introduce a user-facing change?
Yes, on the REST API:
runtime_config.build_withandruntime_envare honored instead of silently discarded. Abuild_withonnpx:///go://returns 400 with a readable message rather than being ignored or surfacing asInternal Server Error.additional_packagesno longer contains duplicates when a requested package is already a transport default..or_are now rejected at the API. They were accepted and then failed at build time — the deleted validator was missing the leading-character checkpackageNamePatternenforces.GET → edit → PUTof a protocol-built workload succeeds instead of returning 400, and the workload's build configuration is preserved rather than erased.runtime_configs.<tt>.build_withsurvives a per-requestruntime_configthat sets other fields. Previously any request-suppliedruntime_configdiscarded the global constraint, because the API layers the config file as the merge base and the merge replacedBuildWithoutright.And one CLI-visible message change: the build-constraint rejection now says
build_withrather than naming the--build-withflag, since the same message is reachable from the API, the TUI and the config file. It also gains a prefix identifying where the value came from (invalid runtime config override:/... in config file for <tt>:/... default runtime config for <tt>:).Special notes for reviewers
Why validation sits in
runtimeConfigForImageBuild, not in the builder. This placement is load-bearing and there is a comment saying so. Errors from that call are wrapped inretriever.ErrInvalidRunConfigand reach the client as a 400; the identical failure insideimageRetrieveris scrubbed toInternal Server Error. Moving the check "closer to where it's used" silently reverts the fix with tests still green.The echo exception is narrow by design. It requires an exact match of
runtime_configandimageand URL against persisted state — comparing onlyruntime_configwould let a request change the image tonginx:latestwhile echoing the old config and bypass the guard. Both operands are normalized, so a workload persisted with an untrimmedbuilder_imagestill round-trips. The request'sruntime_configis never cleared, so the policy gate always evaluates the real config; a test asserts that directly rather than relying on convention.On the "policy enforcement happens after untrusted dependencies execute" concern, if it comes up: that ordering is pre-existing and unchanged here. The build-then-policy sequence is byte-identical on the merge base, the CLI path is untouched, and a strictly stronger primitive was already API-reachable —
builder_imageis copied from the request body and validated only bynameref.ParseReference, then lands inFROM {{.RuntimeConfig.BuilderImage}}, i.e. unconstrained pre-gate build-time root execution withBuildEnvandCOPY .netrcin scope. What this PR adds is narrower:build_withis uvx-only, length-capped, and allowlisted against quotes,$,;, backslash and parens. Worth fixing at the shared choke point inretriever.ResolveMCPServerso CLI, API, TUI and the upgrade applier are covered at once — filed separately, since it is a cross-cutting change to a downstream-implemented interface.Known follow-ups, deliberately not in scope:
loadRuntimeConfigdoes not implement the three-tier precedence (flags > user config > built-ins) thatdocs/runtime-version-customization.mddocuments — it skips the config file entirely when an override is present. Pre-existing, and fixing it changes what existing setups build, so it needs its own decision.build_withandruntime_envare absent fromdocs/arch/05-runconfig-and-permissions.mdanddocs/runtime-version-customization.md. Pre-existing debt from Add runtime-stage environment variables to protocol Dockerfiles #5801 and Rename --uv-with to --build-with; reject constraints on unsupported ecosystems #6116.