Add a render variable for the manager endpoint (#79) - #81
Merged
Conversation
Three of the modules this crate's consumers package take the manager they report to as a mandatory positional argument, and no combination of the ten existing `RenderVar` variants composes it: `Hostname` is the module's own host, `Domain` is the mTLS domain alone, and `Arg` concatenates nothing, so three variants would render three arguments where the module's parser expects one. The one remaining expressible answer, a literal, bakes a single deployment's manager into a package meant to be the same bytes on every host, and a module would then verify the wrong peer's certificate. `ManagerEndpoint` closes that gap as one argv element resolved on the host. Its context field is optional because a module with no such peer has nothing to supply, and a template naming it against a `None` field is refused outright rather than rendering a default, an empty string or a placeholder: the value reaches a root-executed `ExecStart=` and decides which peer a module trusts. The crate parses none of it. The format belongs to the consuming module's own argument parser, and a second parser here would be a second definition of it, free to drift from the one that actually reads it. The producer's version and the ceiling of the accepted range move with the schema, so an older build refuses a manifest naming the variable as an unsupported format rather than as an opaque decode error. The floor does not move, because the variant is additive: every payload already published stays readable, and no release asset has to be republished. The unsigned-writer fixture follows the producer, its one changed byte being the version the writer stamps. Closes #79
Every test naming the new variable either hand-built the unit template or exercised the floor case, where the variable is absent. What none of them covered is the path the version bump was cut for: a producer's manifest whose wire spec names `manager-endpoint`, decoded through the read path with its validator, and rendered. So assert it end to end at the current version, both ways — the value substituted verbatim as the final argument when the caller supplies one, and the refusal when it does not. The floor test's context literal becomes a helper the two share, since they now differ in exactly the field under test. Part of #79
Contributor
Author
|
[Reviewer Round 1] Approved — no findings. The PR implements Issue #79 as specified:
PR linkage and test-plan hygiene are also correct. I inspected the diff and review metadata; I did not run the test suite, per instruction. |
Contributor
Author
|
[Review Verdict Round 1: APPROVED] |
Contributor
Author
Suggested squash commitTitle Body |
20 tasks
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
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.
Closes #79
Summary
RenderVargainsManagerEndpoint, serializedmanager-endpoint, so a unit template can name the manager endpoint its module is pointed at instead of a package baking one deployment's manager in as a literal. One variant rather than three, because the value is a single argv element andArgconcatenates nothing.Its rustdoc carries the four-part contract: the value is
<server_name>@<address>:<port>as one argv element;server_nameis the peer's internal-mTLS server name, a certificate identity the module verifies the presented leaf against rather than anything it resolves;addressis a numeric address and never a name, because consumers parse this into a name plus a numeric socket address;portis the peer's RPC port. That contract is documentation for the caller composing the value. Nothing in the crate splits on@, validates the port, or rejects a name in the address position — the format belongs to the consuming module's own argument parser, and a second parser here would be free to drift from the one that actually reads it.RenderContextgainsmanager_endpoint: Option<&'a str>, followingservice_account's precedent for a genuinely absent value.resolve_varrefuses a template that names the variable against aNonefield with a newRenderError::UnresolvedVariable { var }, in the shapeUnrepresentableValuealready uses; aSomevalue is substituted verbatim, subject only to the existing representability check. The variant is permitted wherever anArgis — no rule analogous toMainPid's confinement toexec_reloadwas invented, because there is no systemd behaviour to read one off.MANIFEST_FORMAT_VERSIONandMAX_MANIFEST_FORMAT_VERSIONmove 3 → 4 so an older build's refusal of a manifest naming the variable is legible asUnsupportedManifestFormatrather than an opaque decode error.MIN_MANIFEST_FORMAT_VERSIONstays at 3: the variant is additive, so every already-published payload stays readable and installable.Both fixtures that used
manager-addressas the canonical unknown variable now useno-such-variable, since a fictional name one letter from a real one is a trap for the next reader.One thing worth a reviewer's eye
assets/test-fixtures/unsigned-container/manifest.jsonchanges by one byte —"format_version":3to4. That fixture pins the unsigned writer's manifest bytes against the live writer, so a deliberate producer bump necessarily moves it; the block layout, lengths and every other byte are unchanged.Test plan
ManagerEndpointround-trips asmanager-endpointin both directions, and a template naming it decodes as one argv element.exec_startends with the variant renders the context's value as the final argument, verbatim and as one element.Nonecontext plus a template naming the variant returnsRenderError::UnresolvedVariable, and the error names the variable.Nonecontext plus a template not naming the variant renders exactly as it does today.UnrepresentableValuewithvarset to the new variant — the variant joinsHOST_RESOLVED_VARS, so the existing sweep also proves it reads its own context field.Argmay occupy, and aNonecontext refuses each of them.MIN_MANIFEST_FORMAT_VERSIONstill decodes, validates and renders its unit byte-for-byte; one aboveMAX_MANIFEST_FORMAT_VERSIONis refused withUnsupportedManifestFormat. The range test pins the ceiling to the producer and the floor no higher, with the floor relation in aconstblock so raising it past the producer is a build failure.manager-endpointdecodes through the read path with its validator and renders, both with a supplied endpoint and with none.cargo fmt --check, both clippy runs,cargo docwith-D warnings, and both test runs are green.