Worktree spike binary preflight#295
Merged
Merged
Conversation
Running `make start` without spire-server/spire-agent on PATH failed several steps in with a bare "spire-server: command not found" from the agent-token script, after already building the SPIKE binaries and starting the SPIRE server. Add a SPIRE preflight to start.sh, right after the domain check, that fails fast with a clear message: which binaries are missing, how to build and install them (./hack/bare-metal/build/build-spire.sh), how to add an existing install to PATH, and how to verify. It is gated by the same SPIKE_SKIP_* flags as the steps that use each binary, so external-SPIRE workflows that skip those steps are not blocked. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
Wrap ./hack/bare-metal/build/build-spike.sh's SPIRE counterpart in a make target, matching how the rest of the bare-metal workflow is driven (`make start`, `make build`, `make bootstrap`). Point the start.sh SPIRE preflight message at `make build-spire` instead of the raw script path so the guidance uses the conventional entry point. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
build-spire.sh cloned SPIRE into ./spire, moved the built binaries to /usr/local/bin, and left the 45MB clone behind. Since ./spire was not gitignored, it showed up as untracked and broke `make audit` (the `gofmt -l .` step scans the SPIRE source). Clone into a temporary directory instead and remove it on exit via a trap, so the working tree stays clean even if the build fails. Add `set -euo pipefail` for fail-fast behavior, and gitignore /spire/ as a belt-and-suspenders guard against a repo-root clone from older runs or a manual git clone. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
start.sh invoked spike and demo as bare PATH commands with no check, so a missing binary surfaced as a raw "spike: command not found" halfway through startup, after SPIRE and the Keepers were already running. The only existing check ran when SPIKE_SKIP_SPIKE_BUILD was set, and it did not cover demo. Replace it with a single check_spike_binaries preflight that runs right after the build step (on a fresh clone the binaries do not exist until the build produces them) and verifies all five binaries (spike, nexus, keeper, bootstrap, demo) are on PATH. On failure it exits before any SPIRE or SPIKE process starts, with concrete guidance: `make build`, a copy-pasteable export PATH line, and a verify command. Also update the closing hint to run 'spike' instead of the stale './spike'. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
The binary names (spike, nexus, keeper, bootstrap, demo) are generic enough to collide with tools a developer already has on PATH, and the docs recommend appending the repo's bin directory to PATH, so an existing lookalike wins silently. The preflight then reported green while the run was guaranteed to break later: the SPIRE entries pin the exact binary path and hash (unix:path and unix:sha256 selectors), so a lookalike cannot obtain an identity and the failure surfaces as cryptic attestation errors. Teach check_spike_binaries to also verify that each name resolves to the binary in ./bin (via the same-file test, so symlinks are fine). On mismatch it fails fast, naming the conflicting paths and suggesting a prepend-style export PATH fix. When SPIKE_SKIP_REGISTER_ENTRIES is set, the entries may legitimately point elsewhere, so the mismatch downgrades to a warning and the run continues. Keeping PATH-based invocation is deliberate: having the binaries on PATH is the user-facing convenience, and the harness sharing the same resolution keeps one consistent story. This check only makes the conflict case loud instead of silent. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
Policies use the name as their sole identifier since #292, and the ID field of PolicyReadRequest and PolicyDeleteRequest carries the policy name; the route handlers already treat it that way. The request guards, however, still called net.RespondErrOnBadPolicyID, which enforces UUID format, so every `spike policy get <name>` and `spike policy delete <name>` was rejected with "400 Bad Request" before reaching its handler. Validate with net.RespondErrOnBadName instead, the same validator the put guard applies to policy names on create. Input validation stays in place; only the format it enforces changes. The SDK-side field rename (ID to Name) is tracked in issue #250. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
Policies are keyed by name and no longer have IDs since #292, but the human formatters for `spike policy list` and `spike policy get` still printed an "ID:" line, now always empty. Remove the line from both, update the format tests accordingly, and rewrite the stale doc example of `spike policy list`, which showed fields the list endpoint never returns. JSON and YAML outputs still contain an empty "id" field because the SDK's PolicyListItem and Policy types carry it; that goes away with the SDK field rename tracked in issue #250. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.com>
The policy checks in start.sh grepped for output formats that no longer exist: the tabular "ID NAME" header (gone since #274 standardized the CLI output) and "Permissions:" lines, which `spike policy list` has not printed since the block format landed. The checks failed on every run and were masked by the debug-mode "continuing anyway" path. Match the current contract instead: expect one "Name: <policy>" block per policy from `spike policy list`, and verify permissions through `spike policy get`, which is where permissions are shown and which also exercises the get-by-name path end to end. On failure, dump the list and get outputs to make the next format drift easy to diagnose. Spec: TBD Signed-off-by: Volkan Özçelik <volkan.ozcelik@broadcom.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
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.
What started as "fail fast when SPIRE binaries are missing" turned into
a chain of finds, each surfaced by the previous fix letting
make startrun further. The branch hardens the bare-metal startup path end to end
and fixes a real regression in Nexus that made
spike policy getandspike policy deleteunusable.The
make startpreflight (bare-metal harness)a70c80b,861c86c,526ee14):start.shnowchecks for
spire-server/spire-agenton PATH up front and pointsat the new
make build-spiretarget instead of failing mid-run witha bare "command not found".
build-spire.shclones into a tempdirectory and cleans up after itself (the leftover
./spirecloneused to break
make audit).d683ce8): the same treatment forspike,nexus,keeper,bootstrap, anddemo. The check runs rightafter the build step (on a fresh clone the binaries do not exist
earlier) and fails with concrete guidance:
make build, acopy-pasteable
export PATHline, and a verify command.314ae6e): the binary names are generic enoughto collide with tools already on PATH, and the docs recommend
appending the repo's
binto PATH, so an existing lookalike winssilently. Because the SPIRE entries pin the exact binary path and
hash (
unix:path,unix:sha256), a shadowed binary cannot obtain anidentity and the run fails later in confusing ways. The preflight now
verifies each name resolves to
./bin/<name>(same-file test, sosymlinks are fine) and fails fast naming the conflicting paths. When
SPIKE_SKIP_REGISTER_ENTRIESis set the entries may legitimatelypoint elsewhere, so the mismatch downgrades to a warning.
Keeping PATH-based invocation is deliberate: binaries on PATH are the
user-facing convenience, and the harness sharing that resolution keeps
one consistent story. The preflight only makes the failure modes loud.
The policy regression (Nexus)
0971260— since #292 policies are keyed solely by name, and theIDfield of
PolicyReadRequest/PolicyDeleteRequestcarries the policyname (the route handlers already treat it that way). The request
guards, however, still validated that field with
net.RespondErrOnBadPolicyID, which enforces UUID format, so everyspike policy get <name>andspike policy delete <name>came back"400 Bad Request" before reaching its handler. The guards now use
net.RespondErrOnBadName, the same validator the put guard applies oncreate. Validation stays in force; only the enforced format changes.
Cleanups the regression hunt surfaced
837e0db(pilot): the human formatters forspike policy listandspike policy getprinted a vestigialID:line, always empty since feat(policy): use name as the sole policy identifier #292. Removed, tests updated, and the stalelistdoc examplerewritten (it showed fields the list endpoint never returns).
7795eea(harness): the policy checks instart.shgrepped foroutput formats that predate both fix: Standardize output format options across all CLI commands #274 and feat(policy): use name as the sole policy identifier #292 (tabular
ID NAMEheader,
Permissions:lines thatlistnever prints). They failedon every run, masked by the debug-mode "continuing anyway" path. The
validation now expects
Name: <policy>blocks fromlistandverifies permissions through
spike policy get, which also exercisesthe fixed get-by-name path end to end.
Testing
make auditandmake testpass on every commit.missing binaries, shadowed binaries (fatal and warn variants), and
clean pass.
environment (
Error: Invalid request.) before the fix.Follow-ups (not in this PR)
IDfield toNamein policy request/response types; the empty"id"in JSON/YAML output goesaway with it. - The sqlite state tests use the real
~/.spike/data/spike.dbanddelete it, which breaks a concurrently running bare-metal environment (and vice versa). They should use
t.TempDir().