Skip to content

server: restrict GlobalConfig to its namespace - #11075

Open
rleungx wants to merge 6 commits into
tikv:release-8.5from
rleungx:codex/fix-global-config-path-release-8.5
Open

server: restrict GlobalConfig to its namespace#11075
rleungx wants to merge 6 commits into
tikv:release-8.5from
rleungx:codex/fix-global-config-path-release-8.5

Conversation

@rleungx

@rleungx rleungx commented Jul 31, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: ref #11079

The GlobalConfig gRPC endpoints trust the client-provided ConfigPath and use it as an etcd key or prefix. A caller that can access PD gRPC can therefore make StoreGlobalConfig, LoadGlobalConfig, or WatchGlobalConfig operate outside the intended global-config namespace. Using path.Join for etcd keys also interprets otherwise literal Name components such as .., which changes the requested key.

What is changed and how does it work?

  • Map an empty ConfigPath and /global/config to /global/config/.
  • Allow other ConfigPath values only when their literal byte prefix is /global/config/ or the legacy cloud-storage-engine prefix resource_group/controller; etcd treats ., .., duplicate separators, and backslashes as ordinary key bytes.
  • Build ConfigPath/Name by string concatenation instead of path.Join. An empty Name continues to address ConfigPath itself, so the default path maps it to /global/config.
  • Apply the /global/config/ authorization boundary to Store, Load, and Watch.
  • Preserve cloud-storage-engine compatibility by applying its resource_group/controller prefix to Store, Load, and Watch through the same validation path.
  • Add integration coverage for literal etcd path/name semantics, invalid paths, empty names, and resource-controller Store, Load, and Watch operations.

Existing TiDB and TiCDC callers use an empty path and remain compatible. cloud-storage-engine and tikv-one retain their existing resource_group/controller prefix Load behavior.

Check List

Tests

  • Integration test:
    • go test -tags without_dashboard ./client -run '^TestGlobalConfigTestSuite$' -count=1

Side effects

  • Breaking backward compatibility: arbitrary ConfigPath values outside /global/config/ and resource_group/controller now receive InvalidArgument.
  • Path-like non-empty names are treated as literal etcd key bytes instead of being normalized by path.Join.

Release note

Restrict GlobalConfig gRPC access to authorized etcd keys while preserving existing TiDB, TiCDC, and resource-controller callers.

@ti-chi-bot ti-chi-bot Bot added do-not-merge/cherry-pick-not-approved release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jul 31, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign cabinfeverb for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 364bd71a-0359-4230-a99a-a82559cec218

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 31, 2026
@rleungx
rleungx force-pushed the codex/fix-global-config-path-release-8.5 branch from ba839b4 to 078e722 Compare July 31, 2026 03:36
@rleungx
rleungx force-pushed the codex/fix-global-config-path-release-8.5 branch from 078e722 to 7d789be Compare July 31, 2026 04:26
@rleungx rleungx changed the title server: restrict GlobalConfig to the default namespace server: restrict GlobalConfig to its namespace Jul 31, 2026
@rleungx
rleungx force-pushed the codex/fix-global-config-path-release-8.5 branch from 7d789be to 01c2f84 Compare July 31, 2026 04:52
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.43590% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 78.30%. Comparing base (f4e9417) to head (01c2f84).
⚠️ Report is 5 commits behind head on release-8.5.

Additional details and impacted files
@@               Coverage Diff               @@
##           release-8.5   #11075      +/-   ##
===============================================
- Coverage        78.39%   78.30%   -0.09%     
===============================================
  Files              499      500       +1     
  Lines            68100    68153      +53     
===============================================
- Hits             53385    53370      -15     
- Misses           10844    10896      +52     
- Partials          3871     3887      +16     
Flag Coverage Δ
unittests 78.30% <97.43%> (-0.09%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
rleungx force-pushed the codex/fix-global-config-path-release-8.5 branch from 01c2f84 to 4e97971 Compare July 31, 2026 05:27
@ti-chi-bot ti-chi-bot Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 31, 2026
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@rleungx
rleungx force-pushed the codex/fix-global-config-path-release-8.5 branch from 41a24bd to bee0b99 Compare July 31, 2026 07:09
rleungx added 2 commits July 31, 2026 16:24
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
rleungx added 2 commits July 31, 2026 17:19
Signed-off-by: Ryan Leung <rleungx@gmail.com>
Signed-off-by: Ryan Leung <rleungx@gmail.com>
@ti-chi-bot ti-chi-bot Bot added cherry-pick-approved Cherry pick PR approved by release team. and removed do-not-merge/cherry-pick-not-approved labels Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cherry-pick-approved Cherry pick PR approved by release team. dco-signoff: yes Indicates the PR's author has signed the dco. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant