From 60f6b1911702b3e90531492ae131b75d70c39c67 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 11:34:58 +0800 Subject: [PATCH 1/6] server: restrict GlobalConfig to its namespace Signed-off-by: Ryan Leung --- server/grpc_service.go | 65 ++++-- .../integrations/client/global_config_test.go | 198 +++++++++++++++--- 2 files changed, 216 insertions(+), 47 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index 87e1c3c97b9..5736054ec46 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -30,6 +30,8 @@ import ( clientv3 "go.etcd.io/etcd/client/v3" "go.uber.org/zap" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/pingcap/errors" "github.com/pingcap/failpoint" @@ -2379,6 +2381,31 @@ func (*GrpcServer) GetDCLocationInfo(_ context.Context, _ *pdpb.GetDCLocationInf // for CDC compatibility, we need to initialize config path to `globalConfigPath` const globalConfigPath = "/global/config/" +func normalizeGlobalConfigPath(configPath string) (string, error) { + if configPath == "" { + return globalConfigPath, nil + } + cleanedPath := path.Clean(configPath) + rootPath := strings.TrimSuffix(globalConfigPath, "/") + isCanonical := configPath == cleanedPath || configPath == cleanedPath+"/" + isInGlobalConfig := cleanedPath == rootPath || strings.HasPrefix(cleanedPath, globalConfigPath) + if strings.Contains(configPath, `\`) || !isCanonical || !isInGlobalConfig { + return "", status.Errorf(codes.InvalidArgument, "global config path %q is not allowed", configPath) + } + return cleanedPath + "/", nil +} + +func resolveGlobalConfigKey(configPath, name string) (string, error) { + if name == "" { + return "", status.Errorf(codes.InvalidArgument, "invalid global config name %q", name) + } + key := path.Join(configPath, name) + if !strings.HasPrefix(key, globalConfigPath) { + return "", status.Errorf(codes.InvalidArgument, "global config name %q resolves outside the allowed path", name) + } + return key, nil +} + // StoreGlobalConfig store global config into etcd by transaction // Since item value needs to support marshal of different struct types, // it should be set to `Payload bytes` instead of `Value string` @@ -2393,13 +2420,16 @@ func (s *GrpcServer) StoreGlobalConfig(_ context.Context, request *pdpb.StoreGlo if done != nil { defer done() } - configPath := request.GetConfigPath() - if configPath == "" { - configPath = globalConfigPath + configPath, err := normalizeGlobalConfigPath(request.GetConfigPath()) + if err != nil { + return nil, err } ops := make([]clientv3.Op, len(request.Changes)) for i, item := range request.Changes { - name := path.Join(configPath, item.GetName()) + key, err := resolveGlobalConfigKey(configPath, item.GetName()) + if err != nil { + return nil, err + } switch item.GetKind() { case pdpb.EventType_PUT: // For CDC compatibility, we need to check the Value field firstly. @@ -2407,9 +2437,9 @@ func (s *GrpcServer) StoreGlobalConfig(_ context.Context, request *pdpb.StoreGlo if value == "" { value = string(item.GetPayload()) } - ops[i] = clientv3.OpPut(name, value) + ops[i] = clientv3.OpPut(key, value) case pdpb.EventType_DELETE: - ops[i] = clientv3.OpDelete(name) + ops[i] = clientv3.OpDelete(key) } } res, err := @@ -2437,16 +2467,23 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo if done != nil { defer done() } - configPath := request.GetConfigPath() - if configPath == "" { - configPath = globalConfigPath + configPath, err := normalizeGlobalConfigPath(request.GetConfigPath()) + if err != nil { + return nil, err + } + keys := make([]string, len(request.GetNames())) + for i, name := range request.GetNames() { + keys[i], err = resolveGlobalConfigKey(configPath, name) + if err != nil { + return nil, err + } } // Since item value needs to support marshal of different struct types, // it should be set to `Payload bytes` instead of `Value string`. if request.Names != nil { res := make([]*pdpb.GlobalConfigItem, len(request.Names)) for i, name := range request.Names { - r, err := s.client.Get(ctx, path.Join(configPath, name)) + r, err := s.client.Get(ctx, keys[i]) if err != nil { res[i] = &pdpb.GlobalConfigItem{Name: name, Error: &pdpb.Error{Type: pdpb.ErrorType_UNKNOWN, Message: err.Error()}} } else if len(r.Kvs) == 0 { @@ -2483,12 +2520,12 @@ func (s *GrpcServer) WatchGlobalConfig(req *pdpb.WatchGlobalConfigRequest, serve if done != nil { defer done() } + configPath, err := normalizeGlobalConfigPath(req.GetConfigPath()) + if err != nil { + return err + } ctx, cancel := context.WithCancel(server.Context()) defer cancel() - configPath := req.GetConfigPath() - if configPath == "" { - configPath = globalConfigPath - } revision := req.GetRevision() // If the revision is compacted, will meet required revision has been compacted error. // - If required revision < CompactRevision, we need to reload all configs to avoid losing data. diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index 2d3ef7707bc..280870eb534 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -25,6 +25,8 @@ import ( "github.com/stretchr/testify/suite" "go.uber.org/zap" "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "github.com/pingcap/kvproto/pkg/pdpb" "github.com/pingcap/log" @@ -137,40 +139,185 @@ func (suite *globalConfigTestSuite) TestLoadWithoutConfigPath() { re.Equal([]byte("1"), res.Items[0].Payload) } -func (suite *globalConfigTestSuite) TestLoadOtherConfigPath() { +func (suite *globalConfigTestSuite) TestRejectInvalidConfigPath() { re := suite.Require() + invalidPaths := []string{ + "OtherConfigPath", + "/tmp/codex-repro/", + "/global/config/../pd/", + "/global/config/child/../../pd/", + "/global/config/./child/", + "/global/config//child/", + "/global/config//", + "/global/configuration/", + `/global/config/child\secret/`, + } + for _, configPath := range invalidPaths { + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + ConfigPath: configPath, + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "source_id", + Payload: []byte("1"), + }}, + }) + re.Equal(codes.InvalidArgument, status.Code(err), configPath) + + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + Names: []string{"source_id"}, + ConfigPath: configPath, + }) + re.Equal(codes.InvalidArgument, status.Code(err), configPath) + + err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ + ConfigPath: configPath, + }, testReceiver{re: re, ctx: suite.server.Context()}) + re.Equal(codes.InvalidArgument, status.Code(err), configPath) + } +} + +func (suite *globalConfigTestSuite) TestNestedConfigPath() { + re := suite.Require() + nestedPath := "/global/config/tidb" + nestedKey := nestedPath + "/source_id" + siblingKey := "/global/config/tidb-other/source_id" defer func() { - for i := range 3 { - _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(strconv.Itoa(i))) + for _, key := range []string{nestedKey, siblingKey} { + _, err := suite.server.GetClient().Delete(suite.server.Context(), key) re.NoError(err) } }() - for i := range 3 { - _, err := suite.server.GetClient().Put(suite.server.Context(), path.Join("OtherConfigPath", strconv.Itoa(i)), strconv.Itoa(i)) + + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + ConfigPath: nestedPath, + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "source_id", + Payload: []byte("1"), + }}, + }) + re.NoError(err) + _, err = suite.server.GetClient().Put(suite.server.Context(), siblingKey, "2") + re.NoError(err) + + res, err := suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + ConfigPath: nestedPath + "/", + }) + re.NoError(err) + re.Equal([]*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: nestedKey, + Payload: []byte("1"), + }}, res.Items) + + res, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + Names: []string{"source_id"}, + ConfigPath: nestedPath + "/", + }) + re.NoError(err) + re.Equal([]*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "source_id", + Payload: []byte("1"), + }}, res.Items) +} + +func (suite *globalConfigTestSuite) TestRejectInvalidConfigName() { + re := suite.Require() + invalidNames := []string{"", ".", "..", "nested/..", "../source_id", "nested/../../pd"} + for _, name := range invalidNames { + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: name, + Payload: []byte("1"), + }}, + }) + re.Equal(codes.InvalidArgument, status.Code(err), name) + + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + Names: []string{name}, + }) + re.Equal(codes.InvalidArgument, status.Code(err), name) + } + + validName := "valid-before-invalid" + defer func() { + _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(validName)) re.NoError(err) + }() + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + Changes: []*pdpb.GlobalConfigItem{ + {Kind: pdpb.EventType_PUT, Name: validName, Payload: []byte("1")}, + {Kind: pdpb.EventType_PUT, Name: "../../pd/key", Payload: []byte("2")}, + }, + }) + re.Equal(codes.InvalidArgument, status.Code(err)) + res, err := suite.server.GetClient().Get(suite.server.Context(), getEtcdPath(validName)) + re.NoError(err) + re.Empty(res.Kvs) +} + +func (suite *globalConfigTestSuite) TestCompatibleConfigName() { + re := suite.Require() + names := []string{ + "nested/source_id", + "nested/../source_id", + `nested\source_id`, + "source id", + "source:id", + "source..id", + "配置", + "/absolute", } + defer func() { + for _, name := range names { + _, err := suite.server.GetClient().Delete(suite.server.Context(), path.Join(globalConfigPath, name)) + re.NoError(err) + } + }() + + changes := make([]*pdpb.GlobalConfigItem, 0, len(names)) + for _, name := range names { + changes = append(changes, &pdpb.GlobalConfigItem{ + Kind: pdpb.EventType_PUT, + Name: name, + Payload: []byte(name), + }) + } + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + Changes: changes, + }) + re.NoError(err) + res, err := suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ - Names: []string{"0", "1"}, - ConfigPath: "OtherConfigPath", + Names: names, }) re.NoError(err) - re.Len(res.Items, 2) + re.Len(res.Items, len(names)) for i, item := range res.Items { - re.Equal(&pdpb.GlobalConfigItem{Kind: pdpb.EventType_PUT, Name: strconv.Itoa(i), Payload: []byte(strconv.Itoa(i))}, item) + re.Equal(names[i], item.Name) + re.Equal([]byte(names[i]), item.Payload) + + expectedKey := path.Join(globalConfigPath, names[i]) + getRes, err := suite.server.GetClient().Get(suite.server.Context(), expectedKey) + re.NoError(err) + re.Len(getRes.Kvs, 1) + re.Equal(expectedKey, string(getRes.Kvs[0].Key)) } } func (suite *globalConfigTestSuite) TestLoadAndStore() { re := suite.Require() defer func() { - for range 3 { - _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath("test")) + for i := range 3 { + _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(strconv.Itoa(i))) re.NoError(err) } }() changes := []*pdpb.GlobalConfigItem{{Kind: pdpb.EventType_PUT, Name: "0", Payload: []byte("0")}, {Kind: pdpb.EventType_PUT, Name: "1", Payload: []byte("1")}, {Kind: pdpb.EventType_PUT, Name: "2", Payload: []byte("2")}} _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ - ConfigPath: globalConfigPath, + ConfigPath: "/global/config", Changes: changes, }) re.NoError(err) @@ -187,15 +334,14 @@ func (suite *globalConfigTestSuite) TestLoadAndStore() { func (suite *globalConfigTestSuite) TestStore() { re := suite.Require() defer func() { - for range 3 { - _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath("test")) + for i := range 3 { + _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(strconv.Itoa(i))) re.NoError(err) } }() changes := []*pdpb.GlobalConfigItem{{Kind: pdpb.EventType_PUT, Name: "0", Payload: []byte("0")}, {Kind: pdpb.EventType_PUT, Name: "1", Payload: []byte("1")}, {Kind: pdpb.EventType_PUT, Name: "2", Payload: []byte("2")}} _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ - ConfigPath: globalConfigPath, - Changes: changes, + Changes: changes, }) re.NoError(err) for i := range 3 { @@ -273,24 +419,10 @@ func (suite *globalConfigTestSuite) TestClientLoadWithoutConfigPath() { re.Equal(pd.GlobalConfigItem{EventType: pdpb.EventType_PUT, Name: "source_id", PayLoad: []byte("1"), Value: "1"}, res[0]) } -func (suite *globalConfigTestSuite) TestClientLoadOtherConfigPath() { +func (suite *globalConfigTestSuite) TestClientRejectOtherConfigPath() { re := suite.Require() - defer func() { - for i := range 3 { - _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(strconv.Itoa(i))) - re.NoError(err) - } - }() - for i := range 3 { - _, err := suite.server.GetClient().Put(suite.server.Context(), path.Join("OtherConfigPath", strconv.Itoa(i)), strconv.Itoa(i)) - re.NoError(err) - } - res, _, err := suite.client.LoadGlobalConfig(suite.server.Context(), []string{"0", "1"}, "OtherConfigPath") - re.NoError(err) - re.Len(res, 2) - for i, item := range res { - re.Equal(pd.GlobalConfigItem{EventType: pdpb.EventType_PUT, Name: strconv.Itoa(i), PayLoad: []byte(strconv.Itoa(i)), Value: strconv.Itoa(i)}, item) - } + _, _, err := suite.client.LoadGlobalConfig(suite.server.Context(), []string{"source_id"}, "OtherConfigPath") + re.Equal(codes.InvalidArgument, status.Code(err)) } func (suite *globalConfigTestSuite) TestClientStore() { From 9823c661095d9b9aa0cb6ad01e220b36dae7deb4 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 15:05:52 +0800 Subject: [PATCH 2/6] server: preserve GlobalConfig key semantics Signed-off-by: Ryan Leung --- server/grpc_service.go | 45 ++++-- .../integrations/client/global_config_test.go | 140 ++++++++++++++---- 2 files changed, 142 insertions(+), 43 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index 5736054ec46..b14fa28da88 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "io" - "path" "runtime" "runtime/trace" "strconv" @@ -2378,32 +2377,37 @@ func (*GrpcServer) GetDCLocationInfo(_ context.Context, _ *pdpb.GetDCLocationInf }, nil } -// for CDC compatibility, we need to initialize config path to `globalConfigPath` -const globalConfigPath = "/global/config/" +const ( + // For CDC compatibility, we need to initialize an empty config path to + // `globalConfigPath`. + globalConfigPath = "/global/config/" + // cloud-storage-engine loads the resource controller config from this exact + // key through LoadGlobalConfig. + resourceGroupControllerPath = "resource_group/controller" +) func normalizeGlobalConfigPath(configPath string) (string, error) { if configPath == "" { return globalConfigPath, nil } - cleanedPath := path.Clean(configPath) rootPath := strings.TrimSuffix(globalConfigPath, "/") - isCanonical := configPath == cleanedPath || configPath == cleanedPath+"/" - isInGlobalConfig := cleanedPath == rootPath || strings.HasPrefix(cleanedPath, globalConfigPath) - if strings.Contains(configPath, `\`) || !isCanonical || !isInGlobalConfig { + if configPath == rootPath { + return globalConfigPath, nil + } + if !strings.HasPrefix(configPath, globalConfigPath) { return "", status.Errorf(codes.InvalidArgument, "global config path %q is not allowed", configPath) } - return cleanedPath + "/", nil + return configPath, nil } func resolveGlobalConfigKey(configPath, name string) (string, error) { if name == "" { return "", status.Errorf(codes.InvalidArgument, "invalid global config name %q", name) } - key := path.Join(configPath, name) - if !strings.HasPrefix(key, globalConfigPath) { - return "", status.Errorf(codes.InvalidArgument, "global config name %q resolves outside the allowed path", name) + if strings.HasSuffix(configPath, "/") { + return configPath + name, nil } - return key, nil + return configPath + "/" + name, nil } // StoreGlobalConfig store global config into etcd by transaction @@ -2467,9 +2471,13 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo if done != nil { defer done() } - configPath, err := normalizeGlobalConfigPath(request.GetConfigPath()) - if err != nil { - return nil, err + isResourceGroupControllerPath := request.Names == nil && request.GetConfigPath() == resourceGroupControllerPath + configPath := request.GetConfigPath() + if !isResourceGroupControllerPath { + configPath, err = normalizeGlobalConfigPath(configPath) + if err != nil { + return nil, err + } } keys := make([]string, len(request.GetNames())) for i, name := range request.GetNames() { @@ -2495,7 +2503,12 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo } return &pdpb.LoadGlobalConfigResponse{Items: res}, nil } - r, err := s.client.Get(ctx, configPath, clientv3.WithPrefix()) + var r *clientv3.GetResponse + if isResourceGroupControllerPath { + r, err = s.client.Get(ctx, configPath) + } else { + r, err = s.client.Get(ctx, configPath, clientv3.WithPrefix()) + } if err != nil { return &pdpb.LoadGlobalConfigResponse{}, err } diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index 280870eb534..377e95525af 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -16,7 +16,6 @@ package client_test import ( "context" - "path" "strconv" "testing" "time" @@ -38,7 +37,10 @@ import ( "github.com/tikv/pd/tests" ) -const globalConfigPath = "/global/config/" +const ( + globalConfigPath = "/global/config/" + resourceGroupControllerPath = "resource_group/controller" +) type testReceiver struct { re *require.Assertions @@ -144,13 +146,8 @@ func (suite *globalConfigTestSuite) TestRejectInvalidConfigPath() { invalidPaths := []string{ "OtherConfigPath", "/tmp/codex-repro/", - "/global/config/../pd/", - "/global/config/child/../../pd/", - "/global/config/./child/", - "/global/config//child/", - "/global/config//", "/global/configuration/", - `/global/config/child\secret/`, + "/", } for _, configPath := range invalidPaths { _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ @@ -169,6 +166,11 @@ func (suite *globalConfigTestSuite) TestRejectInvalidConfigPath() { }) re.Equal(codes.InvalidArgument, status.Code(err), configPath) + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + ConfigPath: configPath, + }) + re.Equal(codes.InvalidArgument, status.Code(err), configPath) + err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ ConfigPath: configPath, }, testReceiver{re: re, ctx: suite.server.Context()}) @@ -176,6 +178,88 @@ func (suite *globalConfigTestSuite) TestRejectInvalidConfigPath() { } } +func (suite *globalConfigTestSuite) TestLiteralConfigPath() { + re := suite.Require() + configPaths := []string{ + "/global/config/../pd/", + "/global/config/child/../../pd/", + "/global/config/./child/", + "/global/config//child/", + "/global/config//", + `/global/config/child\secret/`, + } + defer func() { + for _, configPath := range configPaths { + _, err := suite.server.GetClient().Delete(suite.server.Context(), configPath+"source_id") + re.NoError(err) + } + }() + + for _, configPath := range configPaths { + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + ConfigPath: configPath, + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "source_id", + Payload: []byte(configPath), + }}, + }) + re.NoError(err, configPath) + + res, err := suite.server.GetClient().Get(suite.server.Context(), configPath+"source_id") + re.NoError(err, configPath) + re.Len(res.Kvs, 1, configPath) + re.Equal([]byte(configPath), res.Kvs[0].Value, configPath) + } +} + +func (suite *globalConfigTestSuite) TestLoadResourceGroupControllerConfig() { + re := suite.Require() + siblingKey := resourceGroupControllerPath + "-other" + defer func() { + for _, key := range []string{resourceGroupControllerPath, siblingKey} { + _, err := suite.server.GetClient().Delete(suite.server.Context(), key) + re.NoError(err) + } + }() + + _, err := suite.server.GetClient().Put(suite.server.Context(), resourceGroupControllerPath, "controller") + re.NoError(err) + _, err = suite.server.GetClient().Put(suite.server.Context(), siblingKey, "other") + re.NoError(err) + + res, err := suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + ConfigPath: resourceGroupControllerPath, + }) + re.NoError(err) + re.Equal([]*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: resourceGroupControllerPath, + Payload: []byte("controller"), + }}, res.Items) + + _, err = suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + ConfigPath: resourceGroupControllerPath, + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "settings", + Payload: []byte("1"), + }}, + }) + re.Equal(codes.InvalidArgument, status.Code(err)) + + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + Names: []string{"settings"}, + ConfigPath: resourceGroupControllerPath, + }) + re.Equal(codes.InvalidArgument, status.Code(err)) + + err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ + ConfigPath: resourceGroupControllerPath, + }, testReceiver{re: re, ctx: suite.server.Context()}) + re.Equal(codes.InvalidArgument, status.Code(err)) +} + func (suite *globalConfigTestSuite) TestNestedConfigPath() { re := suite.Require() nestedPath := "/global/config/tidb" @@ -224,32 +308,29 @@ func (suite *globalConfigTestSuite) TestNestedConfigPath() { func (suite *globalConfigTestSuite) TestRejectInvalidConfigName() { re := suite.Require() - invalidNames := []string{"", ".", "..", "nested/..", "../source_id", "nested/../../pd"} - for _, name := range invalidNames { - _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ - Changes: []*pdpb.GlobalConfigItem{{ - Kind: pdpb.EventType_PUT, - Name: name, - Payload: []byte("1"), - }}, - }) - re.Equal(codes.InvalidArgument, status.Code(err), name) + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + Changes: []*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "", + Payload: []byte("1"), + }}, + }) + re.Equal(codes.InvalidArgument, status.Code(err)) - _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ - Names: []string{name}, - }) - re.Equal(codes.InvalidArgument, status.Code(err), name) - } + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + Names: []string{""}, + }) + re.Equal(codes.InvalidArgument, status.Code(err)) validName := "valid-before-invalid" defer func() { _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(validName)) re.NoError(err) }() - _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ + _, err = suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ Changes: []*pdpb.GlobalConfigItem{ {Kind: pdpb.EventType_PUT, Name: validName, Payload: []byte("1")}, - {Kind: pdpb.EventType_PUT, Name: "../../pd/key", Payload: []byte("2")}, + {Kind: pdpb.EventType_PUT, Name: "", Payload: []byte("2")}, }, }) re.Equal(codes.InvalidArgument, status.Code(err)) @@ -261,8 +342,13 @@ func (suite *globalConfigTestSuite) TestRejectInvalidConfigName() { func (suite *globalConfigTestSuite) TestCompatibleConfigName() { re := suite.Require() names := []string{ + ".", + "..", "nested/source_id", + "nested/..", "nested/../source_id", + "../source_id", + "nested/../../pd", `nested\source_id`, "source id", "source:id", @@ -272,7 +358,7 @@ func (suite *globalConfigTestSuite) TestCompatibleConfigName() { } defer func() { for _, name := range names { - _, err := suite.server.GetClient().Delete(suite.server.Context(), path.Join(globalConfigPath, name)) + _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(name)) re.NoError(err) } }() @@ -299,7 +385,7 @@ func (suite *globalConfigTestSuite) TestCompatibleConfigName() { re.Equal(names[i], item.Name) re.Equal([]byte(names[i]), item.Payload) - expectedKey := path.Join(globalConfigPath, names[i]) + expectedKey := getEtcdPath(names[i]) getRes, err := suite.server.GetClient().Get(suite.server.Context(), expectedKey) re.NoError(err) re.Len(getRes.Kvs, 1) From 623e00fbfcc99b6938feff78df68d4129b9d104e Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 16:24:39 +0800 Subject: [PATCH 3/6] server: preserve GlobalConfig compatibility Signed-off-by: Ryan Leung --- server/grpc_service.go | 29 ++++--------- .../integrations/client/global_config_test.go | 43 ++++++++++--------- 2 files changed, 32 insertions(+), 40 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index b14fa28da88..4b5f4441c35 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -2381,8 +2381,8 @@ const ( // For CDC compatibility, we need to initialize an empty config path to // `globalConfigPath`. globalConfigPath = "/global/config/" - // cloud-storage-engine loads the resource controller config from this exact - // key through LoadGlobalConfig. + // cloud-storage-engine loads the resource controller configs from this + // prefix through LoadGlobalConfig. resourceGroupControllerPath = "resource_group/controller" ) @@ -2400,14 +2400,14 @@ func normalizeGlobalConfigPath(configPath string) (string, error) { return configPath, nil } -func resolveGlobalConfigKey(configPath, name string) (string, error) { +func resolveGlobalConfigKey(configPath, name string) string { if name == "" { - return "", status.Errorf(codes.InvalidArgument, "invalid global config name %q", name) + return strings.TrimSuffix(configPath, "/") } if strings.HasSuffix(configPath, "/") { - return configPath + name, nil + return configPath + name } - return configPath + "/" + name, nil + return configPath + "/" + name } // StoreGlobalConfig store global config into etcd by transaction @@ -2430,10 +2430,7 @@ func (s *GrpcServer) StoreGlobalConfig(_ context.Context, request *pdpb.StoreGlo } ops := make([]clientv3.Op, len(request.Changes)) for i, item := range request.Changes { - key, err := resolveGlobalConfigKey(configPath, item.GetName()) - if err != nil { - return nil, err - } + key := resolveGlobalConfigKey(configPath, item.GetName()) switch item.GetKind() { case pdpb.EventType_PUT: // For CDC compatibility, we need to check the Value field firstly. @@ -2481,10 +2478,7 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo } keys := make([]string, len(request.GetNames())) for i, name := range request.GetNames() { - keys[i], err = resolveGlobalConfigKey(configPath, name) - if err != nil { - return nil, err - } + keys[i] = resolveGlobalConfigKey(configPath, name) } // Since item value needs to support marshal of different struct types, // it should be set to `Payload bytes` instead of `Value string`. @@ -2503,12 +2497,7 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo } return &pdpb.LoadGlobalConfigResponse{Items: res}, nil } - var r *clientv3.GetResponse - if isResourceGroupControllerPath { - r, err = s.client.Get(ctx, configPath) - } else { - r, err = s.client.Get(ctx, configPath, clientv3.WithPrefix()) - } + r, err := s.client.Get(ctx, configPath, clientv3.WithPrefix()) if err != nil { return &pdpb.LoadGlobalConfigResponse{}, err } diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index 377e95525af..ec4258cb64f 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -236,6 +236,10 @@ func (suite *globalConfigTestSuite) TestLoadResourceGroupControllerConfig() { Kind: pdpb.EventType_PUT, Name: resourceGroupControllerPath, Payload: []byte("controller"), + }, { + Kind: pdpb.EventType_PUT, + Name: siblingKey, + Payload: []byte("other"), }}, res.Items) _, err = suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ @@ -306,37 +310,36 @@ func (suite *globalConfigTestSuite) TestNestedConfigPath() { }}, res.Items) } -func (suite *globalConfigTestSuite) TestRejectInvalidConfigName() { +func (suite *globalConfigTestSuite) TestEmptyConfigName() { re := suite.Require() + rootPath := "/global/config" + defer func() { + _, err := suite.server.GetClient().Delete(suite.server.Context(), rootPath) + re.NoError(err) + }() + _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ Changes: []*pdpb.GlobalConfigItem{{ Kind: pdpb.EventType_PUT, Name: "", - Payload: []byte("1"), + Payload: []byte("root"), }}, }) - re.Equal(codes.InvalidArgument, status.Code(err)) + re.NoError(err) + getRes, err := suite.server.GetClient().Get(suite.server.Context(), rootPath) + re.NoError(err) + re.Len(getRes.Kvs, 1) + re.Equal([]byte("root"), getRes.Kvs[0].Value) - _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + res, err := suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ Names: []string{""}, }) - re.Equal(codes.InvalidArgument, status.Code(err)) - - validName := "valid-before-invalid" - defer func() { - _, err := suite.server.GetClient().Delete(suite.server.Context(), getEtcdPath(validName)) - re.NoError(err) - }() - _, err = suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ - Changes: []*pdpb.GlobalConfigItem{ - {Kind: pdpb.EventType_PUT, Name: validName, Payload: []byte("1")}, - {Kind: pdpb.EventType_PUT, Name: "", Payload: []byte("2")}, - }, - }) - re.Equal(codes.InvalidArgument, status.Code(err)) - res, err := suite.server.GetClient().Get(suite.server.Context(), getEtcdPath(validName)) re.NoError(err) - re.Empty(res.Kvs) + re.Equal([]*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "", + Payload: []byte("root"), + }}, res.Items) } func (suite *globalConfigTestSuite) TestCompatibleConfigName() { From 5fa6a32ce5720d27203886ff116d76fa9bae85e5 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 16:36:22 +0800 Subject: [PATCH 4/6] server: preserve resource controller operations Signed-off-by: Ryan Leung --- server/grpc_service.go | 17 ++++++-------- .../integrations/client/global_config_test.go | 22 +++++++++++++------ 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index 4b5f4441c35..53dab26098b 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -2381,8 +2381,8 @@ const ( // For CDC compatibility, we need to initialize an empty config path to // `globalConfigPath`. globalConfigPath = "/global/config/" - // cloud-storage-engine loads the resource controller configs from this - // prefix through LoadGlobalConfig. + // cloud-storage-engine stores its resource controller configs under this + // prefix through the GlobalConfig API. resourceGroupControllerPath = "resource_group/controller" ) @@ -2394,7 +2394,8 @@ func normalizeGlobalConfigPath(configPath string) (string, error) { if configPath == rootPath { return globalConfigPath, nil } - if !strings.HasPrefix(configPath, globalConfigPath) { + if !strings.HasPrefix(configPath, globalConfigPath) && + !strings.HasPrefix(configPath, resourceGroupControllerPath) { return "", status.Errorf(codes.InvalidArgument, "global config path %q is not allowed", configPath) } return configPath, nil @@ -2468,13 +2469,9 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo if done != nil { defer done() } - isResourceGroupControllerPath := request.Names == nil && request.GetConfigPath() == resourceGroupControllerPath - configPath := request.GetConfigPath() - if !isResourceGroupControllerPath { - configPath, err = normalizeGlobalConfigPath(configPath) - if err != nil { - return nil, err - } + configPath, err := normalizeGlobalConfigPath(request.GetConfigPath()) + if err != nil { + return nil, err } keys := make([]string, len(request.GetNames())) for i, name := range request.GetNames() { diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index ec4258cb64f..162daa06a59 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -213,11 +213,12 @@ func (suite *globalConfigTestSuite) TestLiteralConfigPath() { } } -func (suite *globalConfigTestSuite) TestLoadResourceGroupControllerConfig() { +func (suite *globalConfigTestSuite) TestResourceGroupControllerConfig() { re := suite.Require() siblingKey := resourceGroupControllerPath + "-other" + settingsKey := resourceGroupControllerPath + "/settings" defer func() { - for _, key := range []string{resourceGroupControllerPath, siblingKey} { + for _, key := range []string{resourceGroupControllerPath, siblingKey, settingsKey} { _, err := suite.server.GetClient().Delete(suite.server.Context(), key) re.NoError(err) } @@ -250,18 +251,25 @@ func (suite *globalConfigTestSuite) TestLoadResourceGroupControllerConfig() { Payload: []byte("1"), }}, }) - re.Equal(codes.InvalidArgument, status.Code(err)) + re.NoError(err) - _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + res, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ Names: []string{"settings"}, ConfigPath: resourceGroupControllerPath, }) - re.Equal(codes.InvalidArgument, status.Code(err)) + re.NoError(err) + re.Equal([]*pdpb.GlobalConfigItem{{ + Kind: pdpb.EventType_PUT, + Name: "settings", + Payload: []byte("1"), + }}, res.Items) + watchCtx, cancel := context.WithCancel(suite.server.Context()) + cancel() err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ ConfigPath: resourceGroupControllerPath, - }, testReceiver{re: re, ctx: suite.server.Context()}) - re.Equal(codes.InvalidArgument, status.Code(err)) + }, testReceiver{re: re, ctx: watchCtx}) + re.NoError(err) } func (suite *globalConfigTestSuite) TestNestedConfigPath() { From ce15cba642705df88f1b56e7f1c5f9844ed39d37 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 17:19:36 +0800 Subject: [PATCH 5/6] server: simplify GlobalConfig compatibility checks Signed-off-by: Ryan Leung --- server/grpc_service.go | 6 +----- tests/integrations/client/global_config_test.go | 2 +- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index 53dab26098b..0b193092838 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -2473,16 +2473,12 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo if err != nil { return nil, err } - keys := make([]string, len(request.GetNames())) - for i, name := range request.GetNames() { - keys[i] = resolveGlobalConfigKey(configPath, name) - } // Since item value needs to support marshal of different struct types, // it should be set to `Payload bytes` instead of `Value string`. if request.Names != nil { res := make([]*pdpb.GlobalConfigItem, len(request.Names)) for i, name := range request.Names { - r, err := s.client.Get(ctx, keys[i]) + r, err := s.client.Get(ctx, resolveGlobalConfigKey(configPath, name)) if err != nil { res[i] = &pdpb.GlobalConfigItem{Name: name, Error: &pdpb.Error{Type: pdpb.ErrorType_UNKNOWN, Message: err.Error()}} } else if len(r.Kvs) == 0 { diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index 162daa06a59..185ef27a5dd 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -269,7 +269,7 @@ func (suite *globalConfigTestSuite) TestResourceGroupControllerConfig() { err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ ConfigPath: resourceGroupControllerPath, }, testReceiver{re: re, ctx: watchCtx}) - re.NoError(err) + re.NotEqual(codes.InvalidArgument, status.Code(err)) } func (suite *globalConfigTestSuite) TestNestedConfigPath() { From 2d2943d833f33247a9d2adbf34f609391b651a87 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Fri, 31 Jul 2026 18:53:44 +0800 Subject: [PATCH 6/6] server: narrow controller config compatibility Signed-off-by: Ryan Leung --- server/grpc_service.go | 26 +++++----- .../integrations/client/global_config_test.go | 51 +++++++++---------- 2 files changed, 37 insertions(+), 40 deletions(-) diff --git a/server/grpc_service.go b/server/grpc_service.go index 0b193092838..2361c3af005 100644 --- a/server/grpc_service.go +++ b/server/grpc_service.go @@ -2377,14 +2377,9 @@ func (*GrpcServer) GetDCLocationInfo(_ context.Context, _ *pdpb.GetDCLocationInf }, nil } -const ( - // For CDC compatibility, we need to initialize an empty config path to - // `globalConfigPath`. - globalConfigPath = "/global/config/" - // cloud-storage-engine stores its resource controller configs under this - // prefix through the GlobalConfig API. - resourceGroupControllerPath = "resource_group/controller" -) +// For CDC compatibility, we need to initialize an empty config path to +// `globalConfigPath`. +const globalConfigPath = "/global/config/" func normalizeGlobalConfigPath(configPath string) (string, error) { if configPath == "" { @@ -2394,8 +2389,7 @@ func normalizeGlobalConfigPath(configPath string) (string, error) { if configPath == rootPath { return globalConfigPath, nil } - if !strings.HasPrefix(configPath, globalConfigPath) && - !strings.HasPrefix(configPath, resourceGroupControllerPath) { + if !strings.HasPrefix(configPath, globalConfigPath) { return "", status.Errorf(codes.InvalidArgument, "global config path %q is not allowed", configPath) } return configPath, nil @@ -2469,9 +2463,15 @@ func (s *GrpcServer) LoadGlobalConfig(ctx context.Context, request *pdpb.LoadGlo if done != nil { defer done() } - configPath, err := normalizeGlobalConfigPath(request.GetConfigPath()) - if err != nil { - return nil, err + configPath := request.GetConfigPath() + // Keep the legacy prefix-load behavior used by cloud-storage-engine, but + // only for the exact controller path. Other operations and direct sibling + // paths must remain within the global config namespace. + if request.Names != nil || configPath != keypath.ControllerConfigPath() { + configPath, err = normalizeGlobalConfigPath(configPath) + if err != nil { + return nil, err + } } // Since item value needs to support marshal of different struct types, // it should be set to `Payload bytes` instead of `Value string`. diff --git a/tests/integrations/client/global_config_test.go b/tests/integrations/client/global_config_test.go index 185ef27a5dd..89e5c3d0a78 100644 --- a/tests/integrations/client/global_config_test.go +++ b/tests/integrations/client/global_config_test.go @@ -32,15 +32,13 @@ import ( pd "github.com/tikv/pd/client" "github.com/tikv/pd/client/pkg/caller" + "github.com/tikv/pd/pkg/utils/keypath" "github.com/tikv/pd/pkg/utils/syncutil" "github.com/tikv/pd/server" "github.com/tikv/pd/tests" ) -const ( - globalConfigPath = "/global/config/" - resourceGroupControllerPath = "resource_group/controller" -) +const globalConfigPath = "/global/config/" type testReceiver struct { re *require.Assertions @@ -148,13 +146,16 @@ func (suite *globalConfigTestSuite) TestRejectInvalidConfigPath() { "/tmp/codex-repro/", "/global/configuration/", "/", + keypath.ControllerConfigPath() + "-other", + keypath.ControllerConfigPath() + "s", + keypath.ControllerConfigPath() + "/child", } for _, configPath := range invalidPaths { _, err := suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ ConfigPath: configPath, Changes: []*pdpb.GlobalConfigItem{{ Kind: pdpb.EventType_PUT, - Name: "source_id", + Name: "", Payload: []byte("1"), }}, }) @@ -213,29 +214,32 @@ func (suite *globalConfigTestSuite) TestLiteralConfigPath() { } } -func (suite *globalConfigTestSuite) TestResourceGroupControllerConfig() { +func (suite *globalConfigTestSuite) TestResourceGroupControllerPrefixLoadCompatibility() { re := suite.Require() - siblingKey := resourceGroupControllerPath + "-other" - settingsKey := resourceGroupControllerPath + "/settings" + controllerPath := keypath.ControllerConfigPath() + siblingKey := controllerPath + "-other" defer func() { - for _, key := range []string{resourceGroupControllerPath, siblingKey, settingsKey} { + for _, key := range []string{controllerPath, siblingKey} { _, err := suite.server.GetClient().Delete(suite.server.Context(), key) re.NoError(err) } }() - _, err := suite.server.GetClient().Put(suite.server.Context(), resourceGroupControllerPath, "controller") + _, err := suite.server.GetClient().Put(suite.server.Context(), controllerPath, "controller") re.NoError(err) _, err = suite.server.GetClient().Put(suite.server.Context(), siblingKey, "other") re.NoError(err) + // LoadGlobalConfig without Names is a prefix query. Keep that legacy result + // behavior for the exact controller path while rejecting sibling paths as + // direct requests. res, err := suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ - ConfigPath: resourceGroupControllerPath, + ConfigPath: controllerPath, }) re.NoError(err) re.Equal([]*pdpb.GlobalConfigItem{{ Kind: pdpb.EventType_PUT, - Name: resourceGroupControllerPath, + Name: controllerPath, Payload: []byte("controller"), }, { Kind: pdpb.EventType_PUT, @@ -244,32 +248,25 @@ func (suite *globalConfigTestSuite) TestResourceGroupControllerConfig() { }}, res.Items) _, err = suite.server.StoreGlobalConfig(suite.server.Context(), &pdpb.StoreGlobalConfigRequest{ - ConfigPath: resourceGroupControllerPath, + ConfigPath: controllerPath, Changes: []*pdpb.GlobalConfigItem{{ Kind: pdpb.EventType_PUT, Name: "settings", Payload: []byte("1"), }}, }) - re.NoError(err) + re.Equal(codes.InvalidArgument, status.Code(err)) - res, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ + _, err = suite.server.LoadGlobalConfig(suite.server.Context(), &pdpb.LoadGlobalConfigRequest{ Names: []string{"settings"}, - ConfigPath: resourceGroupControllerPath, + ConfigPath: controllerPath, }) - re.NoError(err) - re.Equal([]*pdpb.GlobalConfigItem{{ - Kind: pdpb.EventType_PUT, - Name: "settings", - Payload: []byte("1"), - }}, res.Items) + re.Equal(codes.InvalidArgument, status.Code(err)) - watchCtx, cancel := context.WithCancel(suite.server.Context()) - cancel() err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ - ConfigPath: resourceGroupControllerPath, - }, testReceiver{re: re, ctx: watchCtx}) - re.NotEqual(codes.InvalidArgument, status.Code(err)) + ConfigPath: controllerPath, + }, testReceiver{re: re, ctx: suite.server.Context()}) + re.Equal(codes.InvalidArgument, status.Code(err)) } func (suite *globalConfigTestSuite) TestNestedConfigPath() {