Fix config and CLI correctness/safety bugs in pilotctl#313
Merged
Conversation
config.json now writes atomically (temp file + fsync + rename) at 0600 and no longer inherits loose permissions from a pre-existing file, since it holds admin_token and other secrets. loadConfig distinguishes a missing config (silent, defaults apply) from an unreadable or corrupt one (logged). configDir honors PILOT_HOME so state can be relocated without rewriting $HOME. set-hostname/clear-hostname now report saveConfig failures instead of letting persisted config silently diverge from the running daemon. Pipe-mode stdin reads raise the bufio.Scanner buffer to 16 MiB and check scanner.Err() so long lines and read errors no longer fail silently. send-message --json --wait emits a single JSON document (the reply is folded into the envelope) so machine parsers don't choke on two concatenated documents. bench validates the size argument (rejects non-positive, NaN/Inf, and absurd values, caps at 4 GiB) before dialing. traceroute checks the conn.Write error before reading the response. parseFlags accepts flag values that begin with "-" (negative numbers, bare "-" for stdin, "-3x") via isFlagValue, while still treating "--name"/"-name" tokens as the next flag. daemon stop now discovers the daemon PID from the socket owner (lsof) when the PID file is missing, instead of punting to a manual kill. ping, traceroute, connect, and bench dials use DialAddrTimeout so a timed-out command cancels the daemon-side dial rather than leaving a dangling connection or leaking a goroutine.
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.
Fixes a batch of config-handling and CLI correctness/safety bugs in
pilotctl, almost all incmd/pilotctl/main.go.Changes
saveConfigserializes to a temp file in the config dir,chmod 0600,fsync, thenrenameoverconfig.json. A crash mid-write can no longer leave a truncated or world-readable secrets file, and pre-existing loose permissions are not preserved.loadConfigsurfaces errors — a missing config stays silent (defaults apply); a permission/I-O error or a corrupt/half-written file is logged viaslog.Warninstead of silently vanishing settings.PILOT_HOME—configDirchecksPILOT_HOMEbefore$HOME, so config/socket/PID state can be relocated.set-hostname/clear-hostnamereport save failures — a failedsaveConfignow aborts with an error instead of letting persisted config diverge from the running daemon.scanner.Err()is checked, so long lines and read errors no longer fail silently.send-message --json --waitsingle document — the reply is folded into the one JSON envelope (data.reply) instead of printing a second concatenated JSON doc that breaks machine parsers. Human (non-JSON) output is unchanged.benchsize validation — rejects non-positive,NaN/Inf, and absurd sizes, caps at 4 GiB, and validates before dialing the daemon (fail-fast).traceroutechecksconn.Write— a write failure mid-trace is recorded and breaks the loop instead of blocking inReadfor an echo that will never arrive.parseFlagsaccepts dash-leading values — newisFlagValuelets--key -1,--key -3.14,--key -(stdin), and--key -3xpass through, while--name/-nametokens are still treated as the next flag.daemon stopwithout a PID file — when the socket is live but the PID file is missing, the owning PID is discovered vialsof -t -U(same-UID only) and stopped, instead of punting to a manual kill.ping,traceroute,connect, andbenchnow useDialAddrTimeout, so a timed-out command cancels the daemon-side dial rather than leaving a dangling connection or leaking a goroutine.Tests
zz_config_safety_test.go(new): config 0600 perms + no loose-perm preservation, atomic write leaves no temp file,PILOT_HOMEprecedence + round-trip,benchbad-size rejection,send-message --json --waitsingle-document assertion.zz_parsers_test.go: updated/addedparseFlagscases for dash-leading values and "next flag is not a value".PILOT_HOMEfor hermeticity.Validation
GOWORK=off go build ./...,go vet ./cmd/pilotctl/...,gofmtclean.GOWORK=off go test -count=1 -parallel 4 ./cmd/pilotctl/...green.cli-reference-checkunaffected (no help-text changes).