tools: pdctl support query region direct#10273
tools: pdctl support query region direct#10273bufferflies wants to merge 2 commits intotikv:masterfrom
Conversation
Signed-off-by: tongjian <1045931706@qq.com>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
[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 |
📝 WalkthroughWalkthroughIntroduces a "direct" flag for pd-ctl region commands to enable forwarding requests to PD followers with X-Forwarded-For headers, alongside failpoint injection for testing region query rejection on leaders. Changes
Sequence DiagramsequenceDiagram
participant Client as pd-ctl Client
participant PD as PD Follower
participant Leader as PD Leader
alt Without --direct flag
Client->>PD: Region request (no direct flag)
PD->>Leader: Forward to leader
Note over Leader: RejectGetRegionByIDWhenAccessLeader<br/>failpoint enabled → returns 404
Leader-->>PD: 404 Not Found
PD-->>Client: 404 Not Found
else With --direct flag
Client->>Client: Build header with X-Forwarded-For<br/>& PDAllowFollowerHandleHeader
Client->>PD: Region request (direct header set)
Note over PD: PDAllowFollowerHandleHeader=true<br/>processes locally
PD-->>Client: Region info
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
ti-chi-bot
bot
added
the
size/L
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #10273 +/- ##
==========================================
+ Coverage 78.80% 78.84% +0.03%
==========================================
Files 523 527 +4
Lines 70529 70937 +408
==========================================
+ Hits 55580 55929 +349
- Misses 10955 11000 +45
- Partials 3994 4008 +14
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tools/pd-ctl/tests/region/region_test.go (1)
561-564: Consider strengthening the follower assertion.The assertion accepts either region data (
"id":100) OR"TiKV cluster not bootstrapped". This fallback suggests potential test flakiness - if followers are properly handling the request, they should consistently return region data since the cluster was bootstrapped and the region heartbeat was processed.If the "not bootstrapped" case is a known race condition, consider adding a brief wait or retry logic to ensure cluster state propagation before querying followers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/region/region_test.go` around lines 561 - 564, The current test reads output from tests.ExecuteCommand(cmd, "-u", serverAddr, "region", "100", "--direct") and accepts either "\"id\":100" or "TiKV cluster not bootstrapped", which masks flakiness; modify the test to retry the ExecuteCommand call (or add a short wait loop) until the response contains "\"id\":100" or a timeout is reached, so followers are given time to observe the bootstrapped/heartbeat state before asserting, and remove the permissive OR branch once retries/timeouts are exhausted; reference tests.ExecuteCommand and the region query parameters ("region", "100", "--direct") when implementing the wait/retry.tools/pd-ctl/tests/global_test.go (1)
130-142: Confusing test behavior:--direct=falsestill adds headers.The comment states the direct flag "does not control whether to add X-Forwarded-For header," but this seems counterintuitive. The test expects
--direct=falseto still add headers becausebuildDirectHeaderchecksChanged("direct")rather than the flag's actual value.If this is intentional behavior, consider clarifying the flag's purpose in its help text. If
--direct=falseshould disable header addition, the condition inbuildDirectHeadershould check the flag's boolean value, not just whether it was changed.♻️ If the intent is that --direct=false should NOT add headers
In
global.go:- if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") { + if direct, _ := cmd.Flags().GetBool("direct"); direct {And update this test to expect empty headers for
--direct=false.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/pd-ctl/tests/global_test.go` around lines 130 - 142, The test observes that --direct=false still causes X-Forwarded-For to be added because buildDirectHeader only checks whether the flag was changed (Changed("direct")) rather than its boolean value; change buildDirectHeader to read the actual boolean value of the "direct" flag (e.g., via the command's FlagSet/GetBool or the flags API used in your CLI) and only add the X-Forwarded-For header when that boolean is true, and update the test (the ExecuteCommand call with "--direct=false") to expect no forwarded header when the flag is false.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/pd-ctl/pdctl/command/global.go`:
- Around line 193-205: The buildDirectHeader function uses
cmd.Flags().Lookup("direct") which only sees local flags; change it to use
cmd.Flag("direct") so inherited persistent flags are detected (check the
returned *pflag.Flag for nil and use its .Changed property). Update the
conditional that currently references flag := cmd.Flags().Lookup("direct") to
obtain the flag via cmd.Flag("direct") and keep the rest of the logic (iterating
getEndpoints(cmd), adding apiutil.XForwardedForHeader, and setting
apiutil.PDAllowFollowerHandleHeader) unchanged.
---
Nitpick comments:
In `@tools/pd-ctl/tests/global_test.go`:
- Around line 130-142: The test observes that --direct=false still causes
X-Forwarded-For to be added because buildDirectHeader only checks whether the
flag was changed (Changed("direct")) rather than its boolean value; change
buildDirectHeader to read the actual boolean value of the "direct" flag (e.g.,
via the command's FlagSet/GetBool or the flags API used in your CLI) and only
add the X-Forwarded-For header when that boolean is true, and update the test
(the ExecuteCommand call with "--direct=false") to expect no forwarded header
when the flag is false.
In `@tools/pd-ctl/tests/region/region_test.go`:
- Around line 561-564: The current test reads output from
tests.ExecuteCommand(cmd, "-u", serverAddr, "region", "100", "--direct") and
accepts either "\"id\":100" or "TiKV cluster not bootstrapped", which masks
flakiness; modify the test to retry the ExecuteCommand call (or add a short wait
loop) until the response contains "\"id\":100" or a timeout is reached, so
followers are given time to observe the bootstrapped/heartbeat state before
asserting, and remove the permissive OR branch once retries/timeouts are
exhausted; reference tests.ExecuteCommand and the region query parameters
("region", "100", "--direct") when implementing the wait/retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13618ab6-2a5d-4acc-bc09-ff35235fc6b2
📒 Files selected for processing (5)
server/api/region.gotools/pd-ctl/pdctl/command/global.gotools/pd-ctl/pdctl/command/region_command.gotools/pd-ctl/tests/global_test.gotools/pd-ctl/tests/region/region_test.go
| func buildDirectHeader(cmd *cobra.Command, customHeader http.Header) http.Header { | ||
| header := customHeader.Clone() | ||
| if header == nil { | ||
| header = http.Header{} | ||
| } | ||
| if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") { | ||
| for _, endpoint := range getEndpoints(cmd) { | ||
| header.Add(apiutil.XForwardedForHeader, endpoint) | ||
| } | ||
| header.Set(apiutil.PDAllowFollowerHandleHeader, "true") | ||
| } | ||
| return header | ||
| } |
There was a problem hiding this comment.
Use cmd.Flag() instead of cmd.Flags().Lookup() to find inherited persistent flags.
cmd.Flags().Lookup("direct") only searches local flags on the current command. Since --direct is defined as a PersistentFlag on the region command, subcommands won't find it with this lookup. Use cmd.Flag("direct") which searches both local and inherited flags.
🐛 Proposed fix
func buildDirectHeader(cmd *cobra.Command, customHeader http.Header) http.Header {
header := customHeader.Clone()
if header == nil {
header = http.Header{}
}
- if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") {
+ if flag := cmd.Flag("direct"); flag != nil && flag.Changed {
for _, endpoint := range getEndpoints(cmd) {
header.Add(apiutil.XForwardedForHeader, endpoint)
}
header.Set(apiutil.PDAllowFollowerHandleHeader, "true")
}
return header
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/pd-ctl/pdctl/command/global.go` around lines 193 - 205, The
buildDirectHeader function uses cmd.Flags().Lookup("direct") which only sees
local flags; change it to use cmd.Flag("direct") so inherited persistent flags
are detected (check the returned *pflag.Flag for nil and use its .Changed
property). Update the conditional that currently references flag :=
cmd.Flags().Lookup("direct") to obtain the flag via cmd.Flag("direct") and keep
the rest of the logic (iterating getEndpoints(cmd), adding
apiutil.XForwardedForHeader, and setting apiutil.PDAllowFollowerHandleHeader)
unchanged.
|
@bufferflies: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| if header == nil { | ||
| header = http.Header{} | ||
| } | ||
| if flag := cmd.Flags().Lookup("direct"); flag != nil && cmd.Flags().Changed("direct") { |
There was a problem hiding this comment.
cmd.Flags().Changed("direct") is also true for --direct=false, so this will still enable the direct path when the flag is explicitly set to false.
| for _, endpoint := range getEndpoints(cmd) { | ||
| header.Add(apiutil.XForwardedForHeader, endpoint) | ||
| } |
There was a problem hiding this comment.
We should remove it, PD-Allow-follower-handle is enough.
We should not set X-Forwarded-For to PD endpoints. PD treats X-Forwarded-For as the caller/source IP, so adding PD addresses here changes the request metadata unexpectedly.
For example, with -u pd1:2379,pd2:2379,pd3:2379, the request logic may still try all endpoints, but the server-side parsing of X-Forwarded-For will effectively see the first value and record pd1:2379 as the source, which is incorrect.
2 participants
What problem does this PR solve?
Issue Number: Close #10327
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
Release Notes
New Features
--directflag to the region command in pd-ctl, enabling X-Forwarded-For header support for direct PD control requests.Tests