-
Notifications
You must be signed in to change notification settings - Fork 776
server: validate store addresses on PutStore #11039
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,8 @@ import ( | |
| "context" | ||
| "fmt" | ||
| "io" | ||
| "net" | ||
| "net/url" | ||
| "path" | ||
| "runtime" | ||
| "runtime/trace" | ||
|
|
@@ -27,6 +29,7 @@ import ( | |
| "sync/atomic" | ||
| "time" | ||
|
|
||
| probing "github.com/prometheus-community/pro-bing" | ||
| clientv3 "go.etcd.io/etcd/client/v3" | ||
| "go.uber.org/zap" | ||
| "google.golang.org/grpc" | ||
|
|
@@ -804,6 +807,70 @@ func checkStore(rc *cluster.RaftCluster, storeID uint64) *pdpb.Error { | |
| return nil | ||
| } | ||
|
|
||
| func dialAddress(ctx context.Context, address string) error { | ||
| if strings.HasPrefix(address, "mock://") { | ||
| return nil | ||
| } | ||
|
|
||
| hostPort := address | ||
| if u, err := url.Parse(address); err == nil && u.Host != "" { | ||
| hostPort = u.Host | ||
| } | ||
| 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) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func validateStoreAddress(ctx context.Context, store *metapb.Store) error { | ||
| addresses := make(map[string]struct{}, 2) | ||
| if address := store.GetAddress(); address != "" { | ||
| addresses[address] = struct{}{} | ||
| } | ||
| if address := store.GetStatusAddress(); address != "" { | ||
| addresses[address] = struct{}{} | ||
| } | ||
| if address := store.GetPeerAddress(); address != "" { | ||
| addresses[address] = struct{}{} | ||
| } | ||
| for address := range addresses { | ||
| if err := dialAddress(ctx, address); err != nil { | ||
| return err | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // PutStore implements gRPC PDServer. | ||
| func (s *GrpcServer) PutStore(ctx context.Context, request *pdpb.PutStoreRequest) (*pdpb.PutStoreResponse, error) { | ||
| done, err := s.rateLimitCheck() | ||
|
|
@@ -849,6 +916,15 @@ func (s *GrpcServer) PutStore(ctx context.Context, request *pdpb.PutStoreRequest | |
| }, nil | ||
| } | ||
|
|
||
| // 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 { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| return &pdpb.PutStoreResponse{ | ||
| Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_INVALID_VALUE, | ||
| fmt.Sprintf("invalid store address %s: %s", store.GetAddress(), err)), | ||
| }, nil | ||
| } | ||
|
|
||
| if err := rc.PutMetaStore(store); err != nil { | ||
| return &pdpb.PutStoreResponse{ | ||
| Header: grpcutil.WrapErrorToHeader(pdpb.ErrorType_UNKNOWN, err.Error()), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: tikv/pd
Length of output: 18065
🏁 Script executed:
Repository: 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:
Validate the configured endpoint, not just host reachability.
dialAddress()strips the port after splittinghostPortand then runs ICMP to the host only viaprobing.NewPinger(host), so a live host with a closed store endpoint will be accepted while only unreachable hosts are rejected.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.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