fix(plugin-etcd): classify etcd faults by their gRPC code, not the HTTP status - #3011
Merged
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.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.
Fixes #2994.
Root cause
EtcdHttpClient.detectApiPrefix()asked one question, "does this gateway prefix exist?", by reading the answer to a different one, "did this unauthenticatedmaintenance/statuscall succeed?". It switched on the raw HTTP status: 404 tried the next candidate, 200 accepted, 401 with a configured username accepted, and everything else threw.etcd's gateway has no HTTP status that means "you need a token". grpc-gateway's
HTTPStatusFromCodemaps bothInvalidArgument(3)andFailedPrecondition(9)onto 400, and etcd reports a missing token asInvalidArgument(3) etcdserver: user name is empty. OnceMaintenance.Statusbecame auth-gated, the probe's first call answered 400 and hit the catch-all throw, beforeconnect()ever reachedauthenticate().That gate is an era, not a version. Measured by bisecting etcd releases:
Statusis ungated through 3.5.31, gated behind any valid token from 3.5.32, gated behind therootrole on 3.6.0 through 3.6.11 (PR #14663), and relaxed back to any valid token on 3.6.12 (PR #21666). So a fix keyed on "3.6 needs root" would be wrong by 3.6.12.The same mistake ran through the rest of the client:
performRequestre-authenticated on HTTP 401 alone,authenticate()threw the raw JSON body, andwatch()never looked at the status at all.Measured against real etcd
Containers on 3.2.32, 3.3.27, 3.4.34, 3.5.17 and 3.6.1.
maintenance/statusmaintenance/statusAuthorization: Bearer <token>maintenance/statuson 3.6.1Prefix routing, which is why the probe stays:
/v3/v3beta/v3alphadocs/databases/etcd.mdxpromises etcd 3.2 and later, and 3.2 serves only/v3alpha, so hardcoding/v3would drop two documented versions. The probe is kept and rewritten instead.The fix
One fault model, owned by one type, used by every transport.
EtcdServerFaultdecodes both body shapes (3.5 emitserror,codeandmessage; 3.6 emitscodeandmessage) and classifies by the gRPC code, never the HTTP status.EtcdRequestRecoveryis the retry decision, mirroring clientv3'sshouldRefreshToken: refresh on code 16, on code 3 with a stale auth-store revision, and on code 3 with a missing token when credentials exist. Never on code 7, which clientv3 also refuses (PR #12135 was closed unmerged).EtcdGatewayRouteanswers the routing question alone. 404 is the only rejection; a JSON object body is what proves the path reached etcd.authenticate()coalesces onto oneTaskthe waySnowflakeConnection.connectIfNeededdoes. The old_isAuthenticatingflag returned without a token, so a concurrent caller retried with the stale one.etcdctl endpoint health's own check: a linearizablekv/rangeon keyhealth, counting permission denied as healthy, because reaching the RBAC check proves the quorum read served the request. Notmaintenance/status, which is admin-gated on 3.6.0 to 3.6.11, and notcluster/member/list, which answers 200 with no token at all on most releases and so cannot see a dead token.authenticate()clears the token and connects, which is whatetcdctl --userdoes.Three of the eight changes are defects in the transport being rewritten rather than parts of the reported bug, and each has its own CHANGELOG line:
watch()fed an error body to the event parser and reported zero events instead of an auth failure; the single shared cancel slot let one tab's completion clear another tab's handle; andwatch --timeoutabove 60 seconds raced the session's own 60-second request timeout and died with "The request timed out".scripts/check-etcd-auth-faults.shdiffs the classification against a live server, in the shape ofcheck-redis-command-routing.sh. It is a manual check, not a CI gate, and it says in its header which row it cannot drive.Review findings applied
A second-model review of the diff found four defects in it, all fixed here:
watch --timeout -1reachedUInt64(timeout * 1_000_000_000), a trapping conversion, and crashed the app from the command editor.--timeoutis now range-checked in the parser, with the transport clamping as a second line.Stopcancels what is actually in flight.TaskHandlenow records the request and honours it on adopt./versionfallback fixes.Codex was out of credits (
Reviewer failed to output a response, no job recorded), so the second read came fromSkill(code-review)instead.Verified
All in an isolated worktree, since this checkout is shared with other in-flight work.
verify.sh generateverify.sh buildverify.sh test(4 new suites + watch parser)verify.sh test(10 pre-existing etcd suites)verify.sh docsswiftlint --stricton the five plugin filesshellcheck --severity=warningon the new scriptverify.sh pluginsEtcdDriverPlugincompiles and linksAllPluginsdoes not go green onmainfor an unrelated reason: #3001 addedOracleCoreError.transactionLostwithout updating an exhaustive switch. #3008 fixes that and has to land first for this PR's CI to pass.No UI automation: nothing here changes a user-facing flow that XCUITest can drive. The behaviour is a network contract, and the tests cover it at the classification boundary.