fix(auth): make the token envelope consumable, and stop the hourly cap rate-limiting - #184
Merged
Merged
Conversation
`ct auth token` exists to be read by another tool, and the tool it was written for reads it through Terraform's `data "external"`, which requires EVERY value in the JSON object to be a string. `environment` was the one field that could be null, and it is null in the common case — no `--env` passed. The external provider then fails with a message about JSON types that names neither this command nor the field, so the one credential helper in the system broke in the least diagnosable way available, on the default invocation. Every other field was already a string. Now this one is too.
The hourly cap was doing the rate limiting, and it is the wrong knob for it. MIN_INTERVAL_MS already caps sustained traffic at 20 handshakes a minute however hard anything loops, which is the conservative end of what a ChurchTools instance tolerates (operator estimate: 20-30 requests a minute, possibly 40). Anything the spacing permits is a rate the instance is fine with, so a cap that binds first is not protecting anything — it just fails commands that were never the problem. And it bound first by a wide margin. The gate sits in performLogin, so it covers every ct command, and there is no session cache off macOS, so on CI each invocation costs a handshake. 120/hour is an average of 2 a minute: a tenth of what the spacing already allows. 1000 sits just under the ~1200 the spacing physically permits, so it never binds on a pipeline of any plausible size while a genuine runaway still stops rather than running all day. The spacing is unchanged at 3s: a login handshake is heavier than a plain read, so the bottom of the tolerated band is the right default, and it costs CI almost nothing because a ct command usually takes longer than 3s anyway.
Two review follow-ups, both documentation; no behaviour changes. The rationale added for raising MAX_PER_HOUR claimed MIN_INTERVAL_MS caps traffic at 20 handshakes a minute "no matter how hard anything loops". The module header says the opposite 30 lines above, and it is right: the counter is read-modify-written without a lock, so parallel callers sleep the same wait together and fire in lockstep. Measured it — 10 concurrent acquires over 40 rounds produced 400 handshakes and recorded 40, under-reporting by exactly the parallelism factor. That matters for the number chosen: because the count under-reports by N, N parallel runaways reach the cap after ~MAX_PER_HOUR rounds regardless of N, so 120 -> 1000 stretches a concurrent runaway from ~6 minutes of hammering to ~50. The trade is still the right one — 120 demonstrably failed ordinary serial CI pipelines, a certain cost to every user against an uncertain one to a misconfigured few — but it is a trade, and the comment now says so instead of resting on a bound that only holds serially. Also states the property the token-envelope fix exists to create in the migration guide's contract list: every value is a string, and environment is "" rather than null on the default invocation. A provider author reading that page could not previously tell what the un-flagged envelope looks like, which is precisely the case that was broken.
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.
Two follow-ups to the credential helper from #183. Separate commits; both are small and both trace back to
ct auth token.1 —
environmentis now"", nevernullct auth tokenexists to be read by another tool, and the tool it was written for reads it through Terraform'sdata "external", which requires every value in the JSON object to be a string.environmentwas the one field that could benull— and it isnullin the common case, when no--envwas passed. The external provider then fails with a message about JSON types naming neither this command nor the field, so the one credential helper in the system broke in the least diagnosable way available, on the default invocation.Simulated both envelopes through the same all-strings check the external provider applies:
Every other field was already a string. Now this one is too. Two tests pin it, both confirmed failing against the unfixed code: one on the field, one asserting no value in the emitted object is a non-string, so the next field added cannot quietly reintroduce the trap.
auth statuskeeps its nullableenvironment— nothing parses that as an all-string envelope.This makes
eqrm/terraform-provider-churchtools#8's documented usage work without the--envworkaround.2 — hourly cap 120 → 1000
The hourly cap was doing the rate limiting, and it is the wrong knob for it.
MIN_INTERVAL_MSalready caps sustained traffic at 20 handshakes a minute however hard anything loops — the conservative end of what a ChurchTools instance tolerates (operator estimate: 20–30 requests a minute, possibly 40). Anything the spacing permits is by definition a rate the instance is fine with, so a cap that binds first is not protecting the instance; it just fails commands that were never the problem.And it bound first by a wide margin. The gate sits in
performLogin, so it covers everyctcommand, and there is no session cache off macOS — so on CI each invocation costs one handshake:MIN_INTERVAL_MS3s1000 sits just under the ~1200 the spacing physically permits, so it never binds on a pipeline of any plausible size, while a genuine runaway still stops rather than running all day.
The 3s spacing is unchanged, deliberately. It could go to 2s (30/min) or 1.5s (40/min) within the band you named, but a login handshake is heavier than a plain read, so the bottom of the tolerated band is the right default — and it costs CI almost nothing, because a
ctcommand usually takes longer than 3s on its own, so the wait has typically already elapsed by the time the next invocation asks. Easy to lower if you'd rather.Verification
npm test— 1195 passed, 5 skipped (108 files), up from 1193; the 2 new tests fail against the unfixed codenpm run typecheck,npx eslint src tests,npm run format:check,npm run build— all cleannode .github/scripts/docs-staleness.mjs— all 6 pages currentMAX_PER_HOURrather than hardcoding it, so they cover the new value as writtenDocs: the throttle section of
docs/opentofu-migration.mdnow says which of the two limits is the actual brake, and drops the claim that the hourly cap is sized for a pipeline.