Fix/serialize volume policy - #267
Merged
Merged
Conversation
9686684 added `volumes` to the contract's parser and to the struct, but `TargetConfigContract` has a hand-written `Serialize` that builds an intermediate struct field by field — and it did not know about the new one. So a declaration parsed correctly, then vanished on the way out. That path is `stacker submit`: the contract is serialized into the submit body. The declaration never reached the registry, and the bake would have reset the volume it was meant to keep. Nothing surfaced — the submit succeeded, the stored contract merely had an empty service block where the volumes should have been. Caught by querying the database after a real resubmit of stackpilot. Every existing test read a contract and asserted on the parsed result; none serialized one back. The new test round-trips through JSON and checks both directions, which is what the submit path actually does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The serialization defect fixed in the previous commit reached production
because every existing test parsed a contract and asserted on the result.
None serialized one back, so a kind missing from the hand-written
`Serialize` looked fine everywhere.
Two paths serialize it, and both were losing the declaration:
- `submit.rs:130` — the marketplace registry, covered by the round-trip
test added with the fix
- `stacker_client.rs:4102` — `stacker sync`, writing the contract onto
every app of a project. Covered here
A third reads it: `compose_env_keys` in `deploy.rs`. It takes fields and
must ignore the rest — a volume name is not an environment variable, and
parameterizing one would leave the compose asking for a value nothing
supplies. Covered here too.
Both new serialization tests were verified against the bug: with the fix
reverted they fail, with it applied they pass.
Two boundaries the audit surfaced also get tests. A legacy
`required`/`optional`/`secret` contract must round-trip untouched and must
not grow an empty `volumes:` key. And a service declaring only volumes and
no fields must survive serialization rather than collapsing to `{}` —
which is precisely the shape the production database showed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The worst of these would have published an image in a worse state than
before any of this work started, so they are grouped rather than split.
**The sanitize step could run against nothing and report success.**
Moving `FinalizeContext` to a parsed contract meant the flat protected-key
set was re-derived by serializing it back — and `Serialize` moves a
default-shaped generated secret out of `fields` into the legacy `secret:`
list, which `protected_keys_from_contract` did not read. For the most
ordinary contract there is, the set came out empty. Empty is worse than
useless: `resolve_non_contract_references` then treats every `${VAR}` as
unmanaged and restores its literal value, undoing the deploy-time
parameterization and writing the author's passwords back into the baked
compose — while the bake prints "Sanitized". Fixed twice over: the set is
now passed in rather than re-derived, and the function reads both shapes.
**An unparseable contract silently became an empty one.** `bake.rs` did
`from_value(...).ok().unwrap_or_default()`, while the gate that is meant
to catch this was evaluated against the raw JSON — so the gate passed and
finalize sanitized against nothing. Reachable: every contract type denies
unknown fields, so a template using a newer kind fails wholesale on an
older binary. It now aborts.
**The keep-list matched more than it claimed.** The comment promised that
keeping `ollama` would not also keep `not-ollama-backup`; the generated
pattern `*[-_]ollama[-_]*` matched exactly that. The test asserted only
that the literal `*ollama*` was absent, so it passed against the bug.
Names now come from the contract and name compose volumes directly, so
the fuzzy patterns are gone — exact match, and the test checks the name
it names.
Also: two services declaring one volume with different policies is
refused rather than silently resolved toward `fixed`; `display` no longer
vanishes when a field collapses into the legacy `secret:` shorthand,
which was dropping the password-input hint on every `stacker sync`; and
`VolumePolicy`'s deserializer matches `Mutability` exhaustively, so a
fifth variant is a compile error instead of a runtime panic on user input.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
No description provided.