server: warn when the advertised status endpoint cannot identify the same TiDB#69880
server: warn when the advertised status endpoint cannot identify the same TiDB#69880OliverS929 wants to merge 3 commits into
Conversation
ti-chi-bot
Bot
added
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesTiDB now asynchronously checks whether its advertised status endpoint returns the same local DDL identity, handling HTTP, JSON, TLS, proxy, timeout, cancellation, and lifecycle cases. Server startup wires the checker, with comprehensive tests and build/test-map registration. Advertised status endpoint verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TiDBServer
participant StatusHTTPClient
participant AdvertisedInfoEndpoint
participant WarningLogger
TiDBServer->>StatusHTTPClient: GET `/info`
StatusHTTPClient->>AdvertisedInfoEndpoint: request advertised endpoint
AdvertisedInfoEndpoint-->>StatusHTTPClient: HTTP status and `ddl_id`
StatusHTTPClient-->>TiDBServer: verification result
TiDBServer->>WarningLogger: log mismatch or request failure
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔍 Starting code review for this PR... |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69880 +/- ##
================================================
- Coverage 76.3207% 73.9901% -2.3306%
================================================
Files 2041 2056 +15
Lines 559975 578447 +18472
================================================
+ Hits 427377 427994 +617
- Misses 131697 150105 +18408
+ Partials 901 348 -553
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
/test check-dev2 |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 10
- Inline comments: 9
- Summary-only findings (no inline anchor): 1
Findings (highest risk first)
🟡 [Minor] (8)
- Warning message/action text is wrong for a local client-construction failure (pkg/server/http_status.go:302, pkg/server/http_status.go:81)
- newAdvertisedStatusEndpointCheckInput takes two adjacent same-typed string params with no compiler-checked ordering (pkg/server/http_status.go:132, pkg/server/http_status.go:295)
- No comment documents that the self-check silently no-ops when
advertise-addressis unset (the default) (pkg/server/http_status.go:139, pkg/server/http_status.go:285, pkg/config/config.go:1105) - No intent comment for silently stripping proxy config and blocking redirects (pkg/server/http_status.go:161)
invalid-responsereason conflates a body-read transport failure with malformed response content (pkg/server/http_status.go:209)- Self-check response schema duplicates the ddl_id JSON contract instead of reusing serverinfo.StaticInfo (pkg/server/http_status.go:224, pkg/domain/serverinfo/info.go:56)
- Unreachable self-endpoint is reported as a topology-mismatch warning, producing false alarms in common deployments (pkg/server/http_status.go:277, pkg/server/http_status.go:263)
- Self-check identity contract (ddl.GetID vs /info ddl_id) has no end-to-end regression coverage (pkg/server/http_status.go:296, pkg/server/http_status.go:225, pkg/server/http_status_test.go)
ℹ️ [Info] (2)
- Self-check feature adds a whole new concern into an already multi-purpose file (pkg/server/http_status.go:76)
- New background-check context bypasses the existing Server-scoped cancellation convention (pkg/server/server.go:528, pkg/server/server.go:452, pkg/server/server.go:701)
Unanchored findings
ℹ️ [Info] (1)
- Self-check feature adds a whole new concern into an already multi-purpose file
- Request: Consider moving this self-check logic (and its test file) into a dedicated file, e.g.
pkg/server/advertised_status_check.go, to keephttp_status.goscoped to status-server bootstrapping and improve discoverability.
- Request: Consider moving this self-check logic (and its test file) into a dedicated file, e.g.
| client, err := newAdvertisedStatusEndpointHTTPClient(util.InternalHTTPClient(), advertisedStatusEndpointCheckTimeout) | ||
| if err != nil { | ||
| logAdvertisedStatusEndpointCheckWarning(input, advertisedStatusEndpointCheckResult{ | ||
| reason: advertisedStatusEndpointRequestFailed, |
There was a problem hiding this comment.
🟡 [Minor] Warning message/action text is wrong for a local client-construction failure
Impact
When newAdvertisedStatusEndpointHTTPClient fails (for example the internal HTTP client's Transport is not a *http.Transport), the failure is reported with reason: advertisedStatusEndpointRequestFailed, which shares the same fixed advertisedStatusEndpointWarningMessage ("advertised status endpoint does not identify this TiDB instance") and advertisedStatusEndpointWarningSuggestedAction ("check that advertise-address and status-port route directly to this TiDB instance and that no TiDB exists outside the intended topology") used for real network/identity failures.
This is a local client-construction error, not evidence of a misrouted or duplicated TiDB instance, so an operator paged by this warning would be steered toward checking cluster topology instead of the actual root cause (a broken internal HTTP client).
Scope
pkg/server/http_status.go:302—startAdvertisedStatusEndpointCheckpkg/server/http_status.go:81—advertisedStatusEndpointWarningMessage
Evidence
At pkg/server/http_status.go:302-308, a newAdvertisedStatusEndpointHTTPClient error is logged via logAdvertisedStatusEndpointCheckWarning with reason: advertisedStatusEndpointRequestFailed, reusing the same generic message/action pair that is meant for the case where the remote /info endpoint answered but did not identify this instance.
The same reused text also applies to advertisedStatusEndpointRequestFailed results coming from genuine network failures like the TLS-verification-failure case covered in TestAdvertisedStatusEndpointCheckRequestFailures, where "check topology" is equally off-target advice for a cert problem.
Change request
Give the client-construction failure its own distinct log message/reason (e.g. "failed to build HTTP client for advertised status endpoint self-check") instead of reusing advertisedStatusEndpointRequestFailed/advertisedStatusEndpointWarningSuggestedAction, or make the suggested action conditional on reason so it only points at topology duplication for missing-identity/identity-mismatch/unexpected-status.
There was a problem hiding this comment.
✅ The client-construction path now logs a dedicated client-setup-failed reason with its own remediation text, so a broken internal transport no longer sends operators chasing a topology problem.
| func newAdvertisedStatusEndpointCheckInput( | ||
| reportStatus bool, | ||
| statusListener net.Listener, | ||
| advertiseAddress string, |
There was a problem hiding this comment.
🟡 [Minor] newAdvertisedStatusEndpointCheckInput takes two adjacent same-typed string params with no compiler-checked ordering
Impact
advertiseAddress and localID sit next to each other as plain string parameters (positions 3 and 4) in newAdvertisedStatusEndpointCheckInput, and the same ordering repeats at the production call site and in every test call.
Swapping them compiles cleanly since both are string, and would feed the DDL ID into the host-building path (buildAdvertisedStatusEndpointURL) while comparing the real advertise address against the remote identity, silently misconfiguring the check for the next person who edits this call.
Scope
pkg/server/http_status.go:132—newAdvertisedStatusEndpointCheckInputpkg/server/http_status.go:295
Evidence
Signature is newAdvertisedStatusEndpointCheckInput(reportStatus bool, statusListener net.Listener, advertiseAddress string, localID string, scheme string); nothing distinguishes advertiseAddress from localID at the type level, so an argument-order mistake is not caught until the self-check silently misbehaves at runtime.
Change request
Group the semantically distinct string fields into a small named struct (or at least give advertiseAddress/localID distinct wrapper types) so a future reorder fails to compile instead of silently producing a wrong check.
There was a problem hiding this comment.
I don't think another input struct or wrapper types are necessary for this private helper with one production call site, so I kept the current signature.
There was a problem hiding this comment.
✅ Fair enough — for a private helper with a single production call site, keeping the flat signature rather than introducing a wrapper struct is a reasonable trade-off. Leaving as-is.
| localID string, | ||
| scheme string, | ||
| ) (advertisedStatusEndpointCheckInput, bool) { | ||
| if !reportStatus || statusListener == nil || advertiseAddress == "" || localID == "" { |
There was a problem hiding this comment.
🟡 [Minor] No comment documents that the self-check silently no-ops when advertise-address is unset (the default)
Impact
newAdvertisedStatusEndpointCheckInput returns ok=false with zero logging whenever advertiseAddress == "", which is the documented default value of AdvertiseAddress.
Operators who expect this new self-check to catch a duplicated or misrouted TiDB instance get silently no protection at all in the common case where advertise-address was never configured, and nothing in the code states this limitation.
Scope
pkg/server/http_status.go:139—newAdvertisedStatusEndpointCheckInputpkg/server/http_status.go:285—startAdvertisedStatusEndpointCheckpkg/config/config.go:1105
Evidence
pkg/server/http_status.go:139 folds !reportStatus, a missing listener, empty advertiseAddress, and empty localID into a single silent false return with no log line, and pkg/config/config.go:1105 sets AdvertiseAddress: "" as the default, so a stock deployment never runs this check.
The sibling GetStatusServerAddr (pkg/server/server.go:213-224) treats the same empty-AdvertiseAddress condition the same way but at least returns an explicit bool the caller inspects, whereas here the caller (startAdvertisedStatusEndpointCheck) just silently returns.
Change request
Add a short comment above the guard in newAdvertisedStatusEndpointCheckInput stating that an empty advertiseAddress intentionally disables the check (mirroring GetStatusServerAddr's behavior), and consider a one-time debug/info log noting the check was skipped so operators are not left assuming it is active by default.
There was a problem hiding this comment.
Normal tidb-server startup fills an empty advertise address before constructing the server. The remaining empty cases are direct library/test configurations and intentionally skip the check, so I did not add a startup log.
There was a problem hiding this comment.
✅ Makes sense: normal tidb-server startup fills advertise-address before the server is constructed, so the empty-value skip only touches direct library/test configs. No startup log needed.
| } | ||
| } | ||
| directTransport := baseTransport.Clone() | ||
| directTransport.Proxy = nil |
There was a problem hiding this comment.
🟡 [Minor] No intent comment for silently stripping proxy config and blocking redirects
Impact
newAdvertisedStatusEndpointHTTPClient clones the shared util.InternalHTTPClient() transport but sets Proxy = nil and forces CheckRedirect to stop-at-first-hop, diverging from the base client's normal proxy-aware, redirect-following behavior with no comment explaining why.
A future maintainer reusing or "simplifying" this helper (it is not tied to the endpoint-check name) could drop either behavior without realizing both are load-bearing for the self-check's correctness: proxy bypass ensures the request reaches the advertised host directly, and blocking redirects prevents a misrouted request from silently reporting success via a different backend.
Scope
pkg/server/http_status.go:161—newAdvertisedStatusEndpointHTTPClient
Evidence
directTransport.Proxy = nil (line 177) and the CheckRedirect override returning http.ErrUseLastResponse (lines 181-183) have no accompanying comment, even though util.InternalHTTPClient() itself is proxy-aware (http.ProxyFromEnvironment via http.DefaultTransport), so the divergence is a deliberate, non-obvious design choice rather than an accident.
Change request
Add a short comment above the directTransport.Proxy = nil / CheckRedirect block explaining that the self-check must bypass any configured proxy and must not follow redirects, since either would let a misrouted request appear to succeed.
There was a problem hiding this comment.
✅ A comment now sits above the proxy/redirect overrides spelling out that either one could let a different endpoint pass the identity check, so the intent is preserved for future readers.
| if err != nil { | ||
| result.reason = advertisedStatusEndpointInvalidResponse | ||
| var networkError net.Error | ||
| if ctx.Err() != nil || (stderrors.As(err, &networkError) && networkError.Timeout()) { |
There was a problem hiding this comment.
🟡 [Minor] invalid-response reason conflates a body-read transport failure with malformed response content
Impact
The logged reason field is meant to tell an operator whether the advertised status endpoint answered with bad content (invalid-response) or the request itself failed (request-failed), but any non-timeout io.ReadAll error while reading the body — for example a connection reset mid-response — is left labeled invalid-response even though no content was actually received to judge as malformed.
Someone triaging via the reason field would be steered toward a response-format problem when the real cause is a dropped connection.
Scope
pkg/server/http_status.go:209—checkAdvertisedStatusEndpoint
Evidence
At pkg/server/http_status.go:209-217, body, err := io.ReadAll(...) on error first sets result.reason = advertisedStatusEndpointInvalidResponse, then only re-classifies to advertisedStatusEndpointRequestFailed when ctx.Err() != nil or the error satisfies net.Error.Timeout().
A mid-stream connection reset (e.g. io.ErrUnexpectedEOF or a wrapped ECONNRESET) is neither a context error nor a Timeout() error, so it falls through and keeps the invalid-response label, unlike the oversized-body and malformed-JSON checks a few lines below that correctly reserve invalid-response for cases where a full body was actually obtained.
Change request
Treat any io.ReadAll error on the body as request-failed by default (the transport did not deliver a complete response), reserving invalid-response for the oversized-body and JSON-unmarshal checks that already run only after a full body is successfully read.
There was a problem hiding this comment.
✅ Body-read errors are now classified as request-failed, reserving invalid-response for the oversized-body and JSON-unmarshal checks that only run once a full body is in hand — a mid-stream reset lands in the right bucket.
| result.err = errors.Errorf("response body exceeds %d-byte limit", advertisedStatusEndpointResponseBodyLimit) | ||
| return result | ||
| } | ||
| var responseInfo struct { |
There was a problem hiding this comment.
🟡 [Minor] Self-check response schema duplicates the ddl_id JSON contract instead of reusing serverinfo.StaticInfo
Impact
The /info endpoint already serializes ddl_id through serverinfo.StaticInfo.ID (embedded in tikvhandler.ServerInfo), but checkAdvertisedStatusEndpoint re-declares an independent anonymous struct with its own json:"ddl_id" tag to parse the same response.
A future rename or restructuring of the source-of-truth field has no compiler or reference link to this second definition, so it can drift silently and make the self-check permanently report missing-identity without anyone noticing.
Scope
pkg/server/http_status.go:224—checkAdvertisedStatusEndpointpkg/domain/serverinfo/info.go:56—StaticInfo
Evidence
checkAdvertisedStatusEndpoint unmarshals into var responseInfo struct { DDLID string \json:"ddl_id"` }, duplicating the tag already owned by serverinfo.StaticInfo.ID (json:"ddl_id"), which is what ServerInfoHandler.ServeHTTPactually serves at/info`.
Change request
Unmarshal into (or extract just the ID field from) the existing serverinfo.StaticInfo/ServerInfo type instead of a local anonymous struct, so a future field rename only needs to change one place.
There was a problem hiding this comment.
✅ Resolved — the self-check unmarshals into serverinfo.StaticInfo directly, so the ddl_id contract has a single source of truth and can no longer drift silently.
| defer client.CloseIdleConnections() | ||
| result := checker(ctx, client, input.endpoint, input.localID) | ||
| // Cancellation means this Server.Run invocation is ending, not that the endpoint failed verification. | ||
| if ctx.Err() != nil || result.reason == "" { |
There was a problem hiding this comment.
🟡 [Minor] Unreachable self-endpoint is reported as a topology-mismatch warning, producing false alarms in common deployments
Impact
A transient or structural failure to reach the node's own advertised endpoint (reason request-failed) is logged with the same alarming warning and "check that advertise-address and status-port route directly to this TiDB instance" action as a confirmed wrong-instance mismatch.
In topologies where a node cannot reach its own advertised address (Kubernetes hairpin NAT disabled, advertise-address set to a Service/VIP not routable from the pod itself), this fires on every startup even though the topology is correct, pushing operators to chase a nonexistent misconfiguration.
Scope
pkg/server/http_status.go:277—scheduleAdvertisedStatusEndpointCheckpkg/server/http_status.go:263—logAdvertisedStatusEndpointCheckWarning
Evidence
The reporting gate only filters ctx.Err() != nil || result.reason == "", so a request-failed result flows to reporter and logAdvertisedStatusEndpointCheckWarning emits advertisedStatusEndpointWarningMessage ("does not identify this TiDB instance") plus the topology-remediation action. "Could not verify" (advertisedStatusEndpointRequestFailed) is not distinguished from "verified wrong" (advertisedStatusEndpointIdentityMismatch) at the message/severity level, and self-address unreachability is a real recurring condition, not a transient edge case.
Change request
Treat request-failed as a distinct, softer outcome from identity-mismatch: either log it at a lower level with a "could not verify advertised endpoint" message (not the definitive mismatch warning + topology action), or suppress the topology-remediation wording when the endpoint was simply unreachable, so healthy clusters that cannot hairpin to their own advertised address do not emit misconfiguration alarms.
There was a problem hiding this comment.
Kept this at warning level, but changed the message to “failed to verify” and made the action reason-specific in 1d65ac7. An unreachable endpoint is now reported as unverified rather than a confirmed topology mismatch.
| s.cfg.Status.ReportStatus, | ||
| s.statusListener, | ||
| s.cfg.AdvertiseAddress, | ||
| ddl.GetID(), |
There was a problem hiding this comment.
🟡 [Minor] Self-check identity contract (ddl.GetID vs /info ddl_id) has no end-to-end regression coverage
Impact
The whole feature depends on the invariant that the local ddl.GetID() equals the ddl_id returned by the live /info handler, but no test exercises that coupling or the real startAdvertisedStatusEndpointCheck wiring.
Every test hardcodes both sides to "local-id" and injects a fake /info body, so a future change to the /info identity field (or to which id is passed as expectedID) would silently make production emit a false "does not identify this TiDB instance" warning on every startup while all tests still pass.
Scope
pkg/server/http_status.go:296—startAdvertisedStatusEndpointCheckpkg/server/http_status.go:225—checkAdvertisedStatusEndpointpkg/server/http_status_test.go
Evidence
Production sources expectedID from s.dom.DDL().GetID(), while the /info handler serializes infosync.GetServerInfo().ID as ddl_id (both currently derive from do.ddl.GetID()). Tests only call the decomposed helpers (checkAdvertisedStatusEndpoint, scheduleAdvertisedStatusEndpointCheck) with fabricated ddl_id bodies; startAdvertisedStatusEndpointCheck and the real /info mapping are never driven, so the GetID()-equals-ddl_id invariant is unverified.
Change request
Add one integration-style test that stands up the real status server and drives startAdvertisedStatusEndpointCheck (or an equivalent seam) so a genuine identity match suppresses the warning and a real mismatch triggers exactly one warning, pinning the ddl.GetID() ⇄ /info ddl_id contract against silent drift.
There was a problem hiding this comment.
The live /info identity invariant is already covered by TestServerInfo, which asserts d.GetID() == info.ID. The focused tests and two-process manual runs cover this check without adding another heavy server fixture.
There was a problem hiding this comment.
✅ TestServerInfo already pins d.GetID() == info.ID, so the identity mapping the check relies on is guarded; combined with the focused helper tests and two-process manual runs, standing up a full server fixture just for this isn't worth the weight. Makes sense to leave as-is.
| terror.RegisterFinish() | ||
| go s.startNetworkListener(s.listener, false, errChan) | ||
| go s.startNetworkListener(s.socket, true, errChan) | ||
| advertisedStatusEndpointCheckCtx, cancelAdvertisedStatusEndpointCheck := context.WithCancel(context.Background()) |
There was a problem hiding this comment.
ℹ️ [Info] New background-check context bypasses the existing Server-scoped cancellation convention
The package already has an established convention for a startup-spawned background task: store the context.CancelFunc on the Server struct (s.authTokenCancelFunc) and cancel it centrally in closeListener() alongside the other listener/service teardown.
startAdvertisedStatusEndpointCheck's context instead lives as a local variable in Run(), canceled only via defer when Run() itself returns, so a maintainer extending Close()/closeListener() has no single place to see all background-task lifecycles tied to the server.
There was a problem hiding this comment.
The auth-token worker is long-running and joined by the Server wait group; this check is one-shot and scoped to one Run invocation. The local cancel is therefore the lifecycle owner, so I did not add a shared Server field.
There was a problem hiding this comment.
✅ Right — unlike the auth-token worker, which is long-running and joined via the Server wait group, this check is one-shot and lives entirely within a single Run invocation. Scoping its cancel to that local context keeps lifecycle ownership where it belongs, so a shared Server field would be overkill here.
|
/test pull-integration-realcluster-test-next-gen |
| advertisedStatusEndpointCheckTimeout = 5 * time.Second | ||
| advertisedStatusEndpointResponseBodyLimit = 1 << 20 | ||
| advertisedStatusEndpointWarningMessage = "failed to verify advertised status endpoint identity" | ||
| ) | ||
|
|
||
| type advertisedStatusEndpointCheckReason string | ||
|
|
||
| const ( | ||
| advertisedStatusEndpointClientSetupFailed advertisedStatusEndpointCheckReason = "client-setup-failed" |
There was a problem hiding this comment.
let's put all those advertised status endpoint checking related logic into a new pkg under pkg/server
| ddl.GetID(), | ||
| util.InternalHTTPSchema(), | ||
| ) | ||
| if !ok { |
There was a problem hiding this comment.
IMHO it's better to return (advertisedStatusEndpointCheckInput, err) for newAdvertisedStatusEndpointCheckInput so that the errors will not be silently lost here.
4 participants
What problem does this PR solve?
Issue Number: close #69688
Problem Summary:
TiDB publishes its advertised status endpoint as runtime metadata, but it does not verify whether the endpoint reaches the same TiDB process. A misconfigured endpoint can identify another TiDB instance and surface later as downstream errors without a direct diagnostic.
What changed and how does it work?
After the status and SQL listeners are initialized, TiDB schedules a one-shot background request to the advertised
/infoendpoint and compares the returnedddl_idwith the local TiDB identity.Server.Runinvocation exits.A direct self-roundtrip can be unavailable in some intentional VIP, NAT, Kubernetes Service, or load-balancer topologies. Such a topology can produce a warning; the check remains diagnostic only and does not reject startup.
Check List
Tests
Manual test:
identity-mismatchwarning was emitted with the expected local and remote identities, while SQL remained available. The same topology emitted no feature warning with the baseline binary.Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
New Features
/info, and strict validation of expected responses.Tests
Documentation