Skip to content

server: restrict GlobalConfig to its namespace #11076

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.

Sign up for GitHub

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

Open
rleungx wants to merge 6 commits into
base: master
Choose a base branch
from
+307 −49
Open

server: restrict GlobalConfig to its namespace #11076

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
60f6b19
server: restrict GlobalConfig to its namespace
rleungx Jul 31, 2026
9823c66
server: preserve GlobalConfig key semantics
rleungx Jul 31, 2026
623e00f
server: preserve GlobalConfig compatibility
rleungx Jul 31, 2026
5fa6a32
server: preserve resource controller operations
rleungx Jul 31, 2026
ce15cba
server: simplify GlobalConfig compatibility checks
rleungx Jul 31, 2026
2d2943d
server: narrow controller config compatibility
rleungx Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 49 additions & 17 deletions server/grpc_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
"context"
"fmt"
"io"
"path"
"runtime"
"runtime/trace"
"strconv"
Expand All @@ -30,6 +29,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"
Expand Down Expand Up @@ -2376,8 +2377,39 @@ 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 stores its resource controller configs under this
// prefix through the GlobalConfig API.

@lhy1024 lhy1024 Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This compatibility comment does not match the actual consumer: cloud-storage-engine only loads the exact resource_group/controller key; it does not store controller configs through GlobalConfig.

Also, are we still expected to maintain tidbcloud/tikv-one as a supported compatibility target? Its master has not moved since 2024-12-06 and the repository is not archived. If old deployed binaries remain supported, could we document the owner and supported versions? Either way, its code has the same exact-Load-only behavior as cloud-storage-engine and does not justify broader Store/Watch or sibling-prefix access.

resourceGroupControllerPath = "resource_group/controller"

@lhy1024 lhy1024 Jul 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Please reuse keypath.ControllerConfigPath() instead of introducing another copy of this storage key. It is already the canonical path used by SaveControllerConfig, LoadControllerConfig, and the resource-group metadata watcher.

)

func normalizeGlobalConfigPath(configPath string) (string, error) {
if configPath == "" {
return globalConfigPath, nil
}
rootPath := strings.TrimSuffix(globalConfigPath, "/")
if configPath == rootPath {
return globalConfigPath, nil
}
if !strings.HasPrefix(configPath, globalConfigPath) &&
!strings.HasPrefix(configPath, resourceGroupControllerPath) {

@lhy1024 lhy1024 Jul 31, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] The current check conflates two separate concerns: which client-provided ConfigPath values are authorized, and whether an exact controller Load should retain the legacy WithPrefix() range behavior.

As written, it accepts resource_group/controller-other and resource_group/controllers as direct request paths. With an empty item name, Store/Delete can therefore mutate that exact sibling key.

I checked current cloud-storage-engine, its history, TiDB/tidb-cse, and tidbcloud/tikv-one. The observed compatibility requirement is LoadGlobalConfig with Names == nil and the exact resource_group/controller path. I found no controller Store, named Load, Watch, descendant-path, or controller-* request caller.

I am not necessarily asking to remove WithPrefix() from an exact controller Load; that legacy result behavior can be preserved separately if it is part of the supported API contract. The blocking question is why arbitrary raw-prefix values are accepted as direct ConfigPath values, and why the exception is applied to Store, named Load, and Watch. Unless there are concrete callers for those operations, please scope the exception to the exact controller Load and reject direct sibling paths.

return "", status.Errorf(codes.InvalidArgument, "global config path %q is not allowed", configPath)
}
return configPath, nil
}

func resolveGlobalConfigKey(configPath, name string) string {
if name == "" {
return strings.TrimSuffix(configPath, "/")
}
if strings.HasSuffix(configPath, "/") {
return configPath + name
}
return configPath + "/" + name
}

// StoreGlobalConfig store global config into etcd by transaction
// Since item value needs to support marshal of different struct types,
Expand All @@ -2393,23 +2425,23 @@ 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 := resolveGlobalConfigKey(configPath, item.GetName())
switch item.GetKind() {
case pdpb.EventType_PUT:
// For CDC compatibility, we need to check the Value field firstly.
value := item.GetValue()
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 :=
Expand Down Expand Up @@ -2437,16 +2469,16 @@ 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
}
// 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, 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 {
Expand Down Expand Up @@ -2483,12 +2515,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.
Expand Down
Loading
Loading