server: restrict GlobalConfig to its namespace - #11076
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughGlobal configuration store, load, and watch requests now validate paths and names. Valid paths normalize to ChangesGlobal configuration validation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
ab9950f to
ef3c52b
Compare
rleungx
changed the title
ef3c52b to
3170cdd
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #11076 +/- ##
==========================================
+ Coverage 79.23% 79.32% +0.08%
==========================================
Files 540 541 +1
Lines 76010 76504 +494
==========================================
+ Hits 60226 60686 +460
- Misses 11533 11542 +9
- Partials 4251 4276 +25
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Signed-off-by: Ryan Leung <rleungx@gmail.com>
3170cdd to
60f6b19
Compare
ti-chi-bot
Bot
added
size/XL
Signed-off-by: Ryan Leung <rleungx@gmail.com>
3fc90a5 to
9823c66
Compare
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
do-not-merge/needs-triage-completed
and removed
do-not-merge/needs-linked-issue
labels
Signed-off-by: Ryan Leung <rleungx@gmail.com>
ti-chi-bot
Bot
added
needs-cherry-pick-release-8.5
|
/retest |
| return globalConfigPath, nil | ||
| } | ||
| if !strings.HasPrefix(configPath, globalConfigPath) && | ||
| !strings.HasPrefix(configPath, resourceGroupControllerPath) { |
There was a problem hiding this comment.
[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.
| // `globalConfigPath`. | ||
| globalConfigPath = "/global/config/" | ||
| // cloud-storage-engine stores its resource controller configs under this | ||
| // prefix through the GlobalConfig API. |
There was a problem hiding this comment.
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.
| globalConfigPath = "/global/config/" | ||
| // cloud-storage-engine stores its resource controller configs under this | ||
| // prefix through the GlobalConfig API. | ||
| resourceGroupControllerPath = "resource_group/controller" |
There was a problem hiding this comment.
[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.
| Kind: pdpb.EventType_PUT, | ||
| Name: siblingKey, | ||
| Payload: []byte("other"), | ||
| }}, res.Items) |
There was a problem hiding this comment.
Could you clarify whether returning raw-prefix siblings from an exact resource_group/controller Load is an intentionally supported API compatibility contract?
The old implementation did use WithPrefix(), so retaining that range behavior may be reasonable independently of current callers. However, both cloud-storage-engine and tikv-one select only configs.get("resource_group/controller") and do not consume sibling entries.
If preserving the generic prefix-query semantics is intentional, this assertion is useful, but please document that contract. Otherwise, an exact-item assertion would match all observed consumers. This question is separate from whether a sibling value should be accepted directly as ConfigPath.
| Payload: []byte("1"), | ||
| }}, | ||
| }) | ||
| re.NoError(err) |
There was a problem hiding this comment.
These operations were possible before this PR because GlobalConfig accepted arbitrary ConfigPath values, so the question here is which legacy behavior should remain in the new allowlist.
I found no inspected caller that stores controller configuration or performs a named Load under this path through GlobalConfig. Do we intentionally consider controller Store and named Load part of the supported compatibility contract? If so, please identify or document that caller. Otherwise, could this section assert InvalidArgument for those operations while keeping only the exact Names == nil Load exception?
| err = suite.server.WatchGlobalConfig(&pdpb.WatchGlobalConfigRequest{ | ||
| ConfigPath: resourceGroupControllerPath, | ||
| }, testReceiver{re: re, ctx: watchCtx}) | ||
| re.NotEqual(codes.InvalidArgument, status.Code(err)) |
There was a problem hiding this comment.
If the purpose here is only to verify that the controller path is authorized for Watch, canceling the context first is a practical way to avoid blocking. However, this does not establish a watch or verify any event, so it provides no coverage for the actual Watch behavior.
Do we intentionally need to retain controller-path Watch support? I found no such caller in the inspected consumers. If it is supported, could we either add a live put/watch assertion or make the validation-only intent explicit in the test name/comment? If it is not required, this should instead expect InvalidArgument.
Signed-off-by: Ryan Leung <rleungx@gmail.com>
|
/retest |
|
The PR body appears to be stale after The current code:
Could you update the What is changed, test coverage, and Side effects sections accordingly? In particular, the statements saying that the controller prefix is allowed for Store/Load/Watch and that cloud-storage-engine stores controller configs through GlobalConfig no longer match the code. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
2 participants
What problem does this PR solve?
Issue Number: close #11079
The GlobalConfig gRPC endpoints trust the client-provided
ConfigPathand use it as an etcd key or prefix. A caller that can access PD gRPC can therefore makeStoreGlobalConfig,LoadGlobalConfig, orWatchGlobalConfigoperate outside the intended global-config namespace. Usingpath.Joinfor etcd keys also interprets otherwise literalNamecomponents such as.., which changes the requested key.What is changed and how does it work?
ConfigPathand/global/configto/global/config/.ConfigPathvalues only when their literal byte prefix is/global/config/or the legacy cloud-storage-engine prefixresource_group/controller; etcd treats.,.., duplicate separators, and backslashes as ordinary key bytes.ConfigPath/Nameby string concatenation instead ofpath.Join. An emptyNamecontinues to addressConfigPathitself, so the default path maps it to/global/config./global/config/authorization boundary to Store, Load, and Watch.resource_group/controllerprefix to Store, Load, and Watch through the same validation path.Existing TiDB and TiCDC callers use an empty path and remain compatible. cloud-storage-engine and tikv-one retain their existing
resource_group/controllerprefix Load behavior.Check List
Tests
go test -tags without_dashboard ./client -run '^TestGlobalConfigTestSuite$' -count=1Side effects
ConfigPathvalues outside/global/config/andresource_group/controllernow receiveInvalidArgument.path.Join.Related changes
Release note