Repository navigation
fix(githttp): keep the hook's store errors off the pusher's screen and give them to the operator - #742
Conversation
…d give them to the operator git relays a hook's stderr to the pusher, and a store error names the database host, user and name, or the S3 endpoint and bucket. #724 dropped the error from the hook's eventing log lines, so when only the hook's connection failed, nobody saw the cause. - serve creates a 0600 hook-errors file in each push's private 0700 state directory and passes it to the hooks as OPENSTORAGE_GIT_HOOK_ERRORS (stripped from inherited env). After receive-pack ends, serve logs each record (at most 100 per push), and the file goes with the directory. - githttp.NewHookLogHandler, the hook's slog handler, writes each record to stderr without its error values and the whole record to that file. Without the file, as outside a push, the error values are dropped. It does not follow a symlink, and serve refuses a file that is already there. - The eventing log lines carry err again, and the disabled line says "could record", because it also refuses updates to repositories without an upstream link. - ADR-33's amendment says where the operator finds the cause, and ADR-51's fifth fact gets an amendment: hook eventing no longer fails open for intent. Change-Id: Ie29ff45a426eba49c10d225e5ac0d86d7ee8f07a Signed-off-by: Thomas Kosiewski <tk@coder.com>
…and strip OPENSTORAGE_GIT_HOOK_ERRORS from inherited env in the env test Change-Id: I49e0132d6cbaa730c2100a0e057b1363c677be63 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ic654bfd4c4d7d0fce5f66a16af57ba70e2fddd79 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…rors file Change-Id: Ic64086156cfecfbddec72d974bf5fd74a0a54c17 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I49ebf65007cb08e171a79aefe7505f97ad44d5ef Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I2deb671afa9fe25e00966c825b4745fd9bf541c9 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5efd2c928
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
serve logged at most 100 records, but the hooks still wrote every record to disk: a push of many refs while the store is down writes one record per update hook. A hook now drops each record once the file holds 1 MiB, and serve logs that the hooks dropped some. Change-Id: Ib0c11e52aaec70ba877c636f59e6d5f40b4d61bd Signed-off-by: Thomas Kosiewski <tk@coder.com>
… push Change-Id: Id3441ae1171e7711098e7bfff1dc514482bef80d Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…om the pusher Round 3 of review on the hook-errors file: - The reservation-retire step after a failed upload printed its store error to stderr. It now logs it to the hook-errors file only. - The hook's River client logged to stdout, River's default, and River logs a failed insert notification with the database error as a string. jobs.NewRiverInsertOnly now takes a logger, and the hook passes one that writes only to the hook-errors file. - runHook, the hook command, builds the hook logger, makes it the default logger (so the store packages' own slog lines lose their error values on stderr too) and runs the hook. main calls it, and a test calls it. - The hook's log handler also drops "err" and "error" attributes of any kind, and it checks for an error before it resolves a LogValuer. - url.Parse's error quotes the URL, password included. The metadata, ref, S3-WAL and blob store opens now report "malformed URL" without it. - Tests pin the handler's level, the file's 0600 mode, the refusal of a directory or FIFO, the 100-record and 1 MiB limits, records longer than the scanner's default, and the cause in the format-check and BeforeCAS records. - Docs: ADR-51's amendment, missed-step note and mixed-version row no longer say that hook eventing fails open. The deploy guide states the limits and their log lines, and that a serve crash mid-push leaves the private directory behind. Change-Id: I64afdfdc41991ed5cfca2e6ef4e621bc499f684c Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I9682ff988b323706b65f280fd14f8bb17f1dfb47 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Iea8d06eb2e00260fda265cc73e481211d5ef80a1 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…iter fixes Change-Id: I513339610fa57a429f93075ab169a2c6f1efc1d0 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I606f615270b6a4912faee6b03b9c9165a480ba15 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…eservation retire out of preReceive Change-Id: Ia00a8f58ee207dc6e8b44eff09512a63780402cf Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I6473609d2d792f8f8db3da406c1b087d7ff445f8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e04fdaea6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…p check - A record longer than 64 KiB is cut and ends in "[record cut]": an S3 error can carry a whole response body, and serve's line reader stopped at a line over 1 MiB, losing that record and every later one. - A mutex serializes the size check and the append across one hook's goroutines (a local-mode hook delivers webhooks concurrently), so the file overshoots its 1 MiB cap by at most one record. Change-Id: I1f18847a3b8d59fbf9da585766fba5211d9f1762 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: I0d6cf458141ef09c6c62aa61a54004a79a07bd89 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ie77b15133d117367478ae862711f6a4fec0823f3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
… run Change-Id: I527b729cbbf38524b1129f85ea6b4eacf7fca8d2 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
check.sh --record: LaneSchedule.cfg PASS (3535660 distinct states) OtherWriters.cfg PASS (56705945 distinct states) Liveness.cfg PASS (348735 distinct states) NoLeaseAssumption.cfg fails as expected RESULT: PASS Change-Id: I684e7e45d28a1b9e5579bb46fd9bfdc200d7e8b7 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Why
git relays a hook's stderr and its error to the pusher. A store error names the database host, user and name, or the S3 endpoint and bucket. #724 kept those errors off the hook's eventing log lines, so when only the hook's connection to the store failed, the pusher saw the failed step and nobody saw the cause. The operator needs the cause, and the pusher must see no part of the store's connection string.
This PR is the first of two. It gives the hooks a channel that only serve reads, and it moves every error value that the hook's logger writes into that channel. The follow-up PR changes the hook's top-level error printer to show the pusher only our own refusals, so that a store error in a returned hook error reaches the operator and not the pusher.
Fix
hook-errors(mode 0600) in the push's private 0700 state directory, the one that already holds the verify-sig state. It passes the path to the hooks asOPENSTORAGE_GIT_HOOK_ERRORS, andpushHookEnvstrips any inherited value. The hooks stop writing once the file holds 1 MiB, and they cut a record longer than 64 KiB (an S3 error can carry a whole response body), so serve can read every line. A mutex serializes the size check and the append across a hook's loggers, so the file holds at most one record past the cap. After receive-pack ends, serve logs each record asreceive-pack <repo>: hook: <record>, at most 100 per push, then a count of the rest and a line when the hooks dropped records. Then the directory and the file go with the request.githttp.NewHookLogHandlerwrites each record to stderr without its error values and writes the whole record to the hook-errors file. It drops error values in attributes,Withattributes and groups, an error that is aLogValuer, aLogValuerthat resolves to an error, and anyerrorerrorattribute, a string included (some libraries logerr.Error()). Without the file, as for a hook that runs outside a push, the error values are dropped. They never fall back to stderr.runHook, the hook command thatmaincalls, builds the hook logger and makes it the process's default logger, so the events decorator'spush event lostandsync poke lostlines, the eventing setup lines and the store packages' own slog lines show the pusher no store error. The hook's River client wrote River's lines to stdout, River's default, and River logs a failed insert notification with the database error as a string:jobs.NewRiverInsertOnlynow takes a logger, and the hook passes one that writes only to the hook-errors file. The reservation-retire error after a failed upload went to stderr, and it now goes to the hook-errors file only. The other direct writes on the hook's paths print fixed text: two test-mode crash knobs and verify-sig'sgpg binary not found.url.Parse's error quotes the URL, password included, and its reason can quote part of it. The metadata, ref, S3-WAL and blob store opens now reportparse <store> URL: malformed URL (not shown: it can hold a password). serve refuses such a URL at start, but the hook no longer depends on that.BeforeCAS's refusal log carryerragain, so the operator sees why the hook's eventing failed.O_EXCL, so it refuses one that is already there. The hook writes only to an existing regular file and does not follow a symlink, and serve reads only a regular file. In the 0700 directory no other user can swap the entry.refusing only updates that could record forward intent, because it also refuses updates to repositories without an upstream link.TMPDIR, as it does for the verify-sig state today. serve does not remove such directories when it starts: the temporary directory can be shared, and another serve on the host can have a push in flight. The production guide says so.Threat model
The credential is the hook's store connection string (PostgreSQL URL, SQLite path, S3-WAL endpoint and bucket).
Tests
TestHookLogHandlerKeepsErrorsFromThePusher(internal/githttp): a canary error value in an attribute, aWithattribute, a group, a stringerrattribute, an errorLogValuerand aLogValuerthat resolves to an error appears 0 times on stderr and 8 times in the hook-errors file and serve's log.TestHookLogHandlerHonorsItsLevel,TestHookErrorsFileLimitsAndMode(the 100 and 1 MiB limits, mode 0600, a 200 KiB record logged whole) andTestHookErrorsFileRefusesNonRegularFiles(a directory or FIFO is neither written nor read, and nothing blocks).TestHookErrorsFileCutsLongRecords: a 256 KiB record is cut to 64 KiB with[record cut], and serve logs the record after it.TestHookErrorsFileCapHoldsAcrossGoroutines: 50 loggers that write at once past the cap leave at most one record past it.TestRetireUnuploadedKeepsItsErrorOffStderr: after a failed upload, a retire that fails with a canary store error writes nothing to stderr, and the hook-errors file has the cause.TestHookErrorsFileFailsClosed: an unset variable drops the error values. A symlink is neither written through nor read, and serve refuses to create the file over one.TestLogHookErrorsBoundsOnePush: 105 records log as 100 lines plus5 more records not logged.TestHookErrorsFileBoundsItsSize: 2 MiB of records through the hook's handler leave a file of 1 MiB plus at most one record, and serve logs that the hooks dropped some.TestServeLogsHookErrors: a push through an in-process serve whose pre-receive hook logs a canary error. The pusher's output shows the line 0 times with the canary, serve's log has the canary, and the file is gone after the request.TestHookEventingRefusesIntentWithoutEventing(cmd/openstorage) now also checks that the hook-errors records name the canary host, user and database (PostgreSQL) or path (SQLite) and never the password.TestHookEventingRefusesUnreadableLocalSchedulechecks that the format-check andBeforeCASrecords name the canary endpoint.TestRunHookLogsWithTheHookLogger:runHookwith a canary store logs the setup line and a store package's own slog line to stderr without the canary, and the hook-errors file has both.TestMalformedStoreURLHidesItsPassword: a PostgreSQL URL that does not parse keeps its canary password out of the hook's records, and out of the errors of every store open (metadata, ref, S3-WAL, blob, file).TestRiverInsertOnlyLogsToItsLogger(internal/jobs) andTestHookRiverLogsOnlyToHookErrors(cmd/openstorage): apg_notifythat fails with a canary (a function ahead ofpg_catalogon the connection's search path) makes River log its notify failure. The canary appears 0 times on stdout and stderr, and it appears in the client's logger, which for the hook is the hook-errors file.TestPushHookEnvStripsBothPrefixescovers the new variable.Thirty-nine source mutations each turn at least one of these tests red. They cover the handler (error values, groups,
Withattributes, stringerrattributes,LogValuers and their resolution on stderr; its level; no write to the file; a stderr fallback), the file (a writer or reader that follows a symlink or accepts a directory or FIFO; noO_EXCL; mode 0644; no record cap or a different one; no size cap; no dropped-records line; no scanner limit; no record cut; no mutex around the cap), serve (not passing the path or not logging it; the variable not stripped), the hook's writers (the retire error on stderr; River without a logger; the hook's River client on the default logger;runHookwith a plain handler or without the default logger;slogForHookwith a plain handler), the cause (the setup, format-check andBeforeCASlines withouterr), the URL opens (metadata, ref, S3-WAL and file errors that quote the URL), and the old wording.Not covered by a test (the follow-up PR adds tests for the code-level items, except the first):
runHook(ctx, os.Args[2:], os.Stderr)call incmd/openstorage/main.go: a plain handler orio.Discardthere passes every test. The follow-up runs the realopenstorage hookcommand with a failing store.errorattribute key: the tests pin onlyerr.retireUnuploaded's call site inpreReceiveand its guards,runHooksetting the default logger beforewireHookEventing,slogForHook's Warn level, and the group prefix on the operator's records.The
verify-lane-schedulemodel does not describe hook logging or the per-push files.wireHookEventing,Server.handleServiceRPC,runHook,newHookRiverandpreReceivechanged, socheck.sh --recordrecorded the new sync commit. Theverify-orphan-sweepmodel leaves quota reservations out, and the reservation retire moved out ofpreReceive's publication block intoretireUnuploadedunchanged, so itscheck.sh --recordrecorded the new sync commit too.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh