server: validate store addresses on PutStore#11039
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
[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 |
ti-chi-bot
Bot
added
the
size/L
📝 WalkthroughWalkthroughPutStore now validates store addresses before persisting metadata, probing configured hosts with timeout and cancellation handling while allowing ChangesStore address validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PutStore
participant validateStoreAddress
participant dialAddress
participant StoreEndpoint
PutStore->>validateStoreAddress: validate configured store addresses
validateStoreAddress->>dialAddress: parse and probe each unique address
dialAddress->>StoreEndpoint: send ICMP probe
StoreEndpoint-->>dialAddress: probe result
dialAddress-->>validateStoreAddress: validation result
validateStoreAddress-->>PutStore: persist metadata or return INVALID_VALUE
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/grpc_service.go`:
- Around line 831-845: Update validateStoreAddress to validate the store’s
Address, StatusAddress, and PeerAddress directly in that order instead of
collecting them in a map. When dialAddress fails, return or propagate the
associated endpoint address so callers can identify the exact failing field in
the INVALID_VALUE message, while preserving the existing successful-validation
behavior.
- Around line 809-824: Update the PutStore validation flow, including
validateStoreAddress and dialAddress, to reject unsupported schemes and enforce
the trusted allowlist for 127.0.0.1 and other local-only addresses before any
TCP probe. Apply this validation consistently to metapb.Store Address,
StatusAddress, and PeerAddress, while preserving the existing mock:// handling
and only dialing approved non-mock endpoints.
In `@tests/server/cluster/cluster_test.go`:
- Around line 2242-2244: Update the deferred cleanup for listener in the test to
assert the error returned by listener.Close(), matching the existing
unreachableListener.Close() cleanup pattern and avoiding a discarded error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 929e3953-98ba-4dfe-88ee-adfacc026f07
📒 Files selected for processing (2)
server/grpc_service.gotests/server/cluster/cluster_test.go
Signed-off-by: tongjian <1045931706@qq.com>
102072c to
de5496d
Compare
|
|
||
| // TiKV puts the store before listening on its store address, so PD can only | ||
| // validate the configured address by checking whether it is dialable. | ||
| if err := validateStoreAddress(ctx, store); err != nil { |
There was a problem hiding this comment.
TiKV calls PutStore before its gRPC server is built and bound. This dial therefore returns connection refused, PD responds with INVALID_VALUE, and TiKV aborts before it can start listening. Please avoid requiring address reachability during PutStore, or coordinate a two-phase startup protocol.
Signed-off-by: bufferflies <1045931706@qq.com>
|
@bufferflies: The following tests 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/grpc_service.go`:
- Around line 819-850: Update server/grpc_service.go in the dialAddress
validation flow (around probing.NewPinger) to validate connectivity to the
configured host and port rather than only issuing ICMP against the stripped
host, while preserving context cancellation and error propagation. Update
tests/server/cluster/cluster_test.go at lines 2254-2260 so the live-host
negative case targets a closed store endpoint port; both sites require changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d9908822-f7cb-4d14-9e31-0b209c4336ea
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modserver/grpc_service.gotests/server/cluster/cluster_test.go
| host, _, err := net.SplitHostPort(hostPort) | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
|
|
||
| pinger, err := probing.NewPinger(host) | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| pinger.Count = 1 | ||
| pinger.Timeout = defaultGRPCDialTimeout | ||
| pinger.SetPrivileged(false) | ||
|
|
||
| done := make(chan error, 1) | ||
| go func() { | ||
| done <- pinger.Run() | ||
| }() | ||
|
|
||
| select { | ||
| case <-ctx.Done(): | ||
| pinger.Stop() | ||
| return errors.WithStack(ctx.Err()) | ||
| case err := <-done: | ||
| if err != nil { | ||
| return errors.WithStack(err) | ||
| } | ||
| } | ||
|
|
||
| stats := pinger.Statistics() | ||
| if stats.PacketsRecv == 0 { | ||
| return errors.Errorf("no ICMP reply from %s", host) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)grpc_service\.go$|(^|/)cluster_test\.go$'
echo "== grpc_service relevant lines =="
sed -n '760,870p' server/grpc_service.go 2>/dev/null || true
echo "== cluster test relevant lines =="
sed -n '2230,2275p' tests/server/cluster/cluster_test.go 2>/dev/null || true
echo "== ping/store validation usages =="
rg -n "SplitHostPort|NewPinger|PacketsRecv|unreachable-host|putStore|Address:" server tests -SRepository: tikv/pd
Length of output: 18065
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dialAddress callers =="
rg -n "dialAddress\\(|validateStoreAddress\\(" server/server.go server -S
echo "== context around validate/caller =="
file=$(rg -l "func validateStoreAddress" server)
echo "$file"
wc -l "$file"
sed -n '800,870p' "$file"
rg -n "validateStoreAddress|dialAddress" server -S
echo "== probe dependency availability =="
rg -n "github.com/probing-probe/ping|probing|NewPinger" go.mod go.sum server tests -SRepository: tikv/pd
Length of output: 3406
🌐 Web query:
github.com/prometheus-community/pro-bing NewPinger Count Timeout Statistics PacketsRecv Run behavior ICMP ignored interfaces documentation
💡 Result:
The pro-bing library is a Go package for performing ICMP echo (ping) operations [1][2]. Below is a summary of its behavior and configuration based on official source code and documentation. NewPinger and Run Behavior The NewPinger(host string) function initializes a Pinger struct for a specific host [3][4]. The Run method initiates the probing loop [4]. The pinger operates by sending ICMP Echo Request packets and waiting for responses [2]. It runs until a specified count is reached, a timeout occurs, or the operation is externally interrupted [5]. Count and Timeout - Count: The Count field determines how many packets the pinger should send before stopping. If Count is not set (default -1), the pinger operates indefinitely until interrupted [5]. - Timeout: The Timeout field specifies a duration after which the pinger will stop, regardless of the number of packets sent or received [5]. Statistics The Statistics struct tracks the result of the operation, including [5]: - PacketsSent: Total count of packets sent. - PacketsRecv: Total count of successful responses received. - PacketsRecvDuplicates: Count of duplicate responses received. - PacketLoss: Calculated as a percentage: ((PacketsSent - PacketsRecv) / PacketsSent) * 100. Statistics can be retrieved during or after the run [5]. ICMP and Interface Management - ICMP: The library uses standard ICMP echo requests and replies [2]. It provides callbacks like OnRecv for successful responses, OnDuplicateRecv for duplicates, and OnFinish when the operation completes [2][3]. - Interfaces: The InterfaceName field in the Pinger struct allows users to bind the probe to a specific network interface [3][4]. The library internally maps the interface name to an index and sets the necessary control messages (e.g., IfIndex) on the packet connection to ensure traffic uses the specified interface [6][4]. Documentation Detailed documentation for the library is generated and available via pkg.go.dev [1][2]. For practical usage patterns, the library's repository includes a comprehensive ping utility example in the cmd/ping/ping.go file [3][7].
Citations:
- 1: https://github.com/prometheus-community/pro-bing?tab=readme-ov-file
- 2: https://pkg.go.dev/github.com/prometheus-community/pro-bing
- 3: https://github.com/prometheus-community/pro-bing/blob/main/cmd/ping/ping.go
- 4: https://github.com/prometheus-community/pro-bing/blob/59a8668c/ping.go
- 5: https://github.com/prometheus-community/pro-bing/blob/main/ping.go
- 6: https://github.com/prometheus-community/pro-bing/blob/main/packetconn.go
- 7: https://github.com/prometheus-community/pro-bing/blob/59a8668c/README.md
Validate the configured endpoint, not just host reachability.
dialAddress() strips the port after splitting hostPort and then runs ICMP to the host only via probing.NewPinger(host), so a live host with a closed store endpoint will be accepted while only unreachable hosts are rejected.
- Change
server/grpc_service.govalidation to check the actual endpoint before storing the address, or update the spec/docs to make host-level ICMP validation explicit. - Extend the live-host negative case in
tests/server/cluster/cluster_test.goto use the closed port, not just an unreachable host.
📍 Affects 2 files
server/grpc_service.go#L819-L850(this comment)tests/server/cluster/cluster_test.go#L2254-L2260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/grpc_service.go` around lines 819 - 850, Update server/grpc_service.go
in the dialAddress validation flow (around probing.NewPinger) to validate
connectivity to the configured host and port rather than only issuing ICMP
against the stripped host, while preserving context cancellation and error
propagation. Update tests/server/cluster/cluster_test.go at lines 2254-2260 so
the live-host negative case targets a closed store endpoint port; both sites
require changes.
2 participants
What problem does this PR solve?
Issue Number: Close #11038
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
Release note
Summary by CodeRabbit