importinto: require S3-like auth for nextgen import#68231
Conversation
ti-chi-bot
Bot
added
the
release-note
|
@D3Hunter I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
ti-chi-bot
Bot
added
sig/planner
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR enforces explicit S3-like storage authentication in NextGen IMPORT INTO operations under Security Enhanced Mode. It introduces S3 credential constants, centralizes query parameter key normalization, changes planner validation to require access-key+secret-access-key or role-arn while rejecting explicit external IDs, and updates unit/integration tests including sem-compat adjustments. ChangesS3 Authentication Enforcement for NextGen IMPORT INTO
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Command failed 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 |
D3Hunter
changed the title
|
/cherry-pick release-nextgen-20251011 |
|
@D3Hunter: once the present PR merges, I will cherry-pick it on top of release-nextgen-20251011/release-nextgen-202603 in the new PR and assign it to you. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/planner/core/planbuilder_test.go (1)
1147-1157: ⚡ Quick win"No-error" block is missing cross-backend credential combinations.
The slice validates S3+AK/SK and OSS+role-ARN (in both dash and underscore forms), but omits:
oss://bucket?access-key=ak&secret-access-key=sk(and the_form) — OSS with AK/SKs3://bucket?role-arn=arn(and the_form) — S3 with role ARNSince the PR intent is "shared S3-like query keys for access key, secret access key, and role ARN" (applicable to both backends), a regression where
checkNextGenS3PathWithSemonly accepts role ARN for OSS or only accepts AK/SK for S3 would be invisible to this test.🧪 Suggested additions
for _, str := range []string{ "s3://bucket?access-key=ak&secret-access-key=sk", "s3://bucket?access_key=ak&secret_access_key=sk", + "s3://bucket?role-arn=arn", + "s3://bucket?role_arn=arn", "oss://bucket?role-arn=arn", "oss://bucket?role_arn=arn", + "oss://bucket?access-key=ak&secret-access-key=sk", + "oss://bucket?access_key=ak&secret_access_key=sk", } {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/planbuilder_test.go` around lines 1147 - 1157, The test's "no-error" cases currently only cover S3 with AK/SK and OSS with role-ARN; add the missing cross-backend credential combinations so checkNextGenS3PathWithSem is validated for all forms: include "oss://bucket?access-key=ak&secret-access-key=sk" and "oss://bucket?access_key=ak&secret_access_key=sk" (OSS with AK/SK) and "s3://bucket?role-arn=arn" and "s3://bucket?role_arn=arn" (S3 with role ARN) in the same test loop to ensure the shared query-key behavior is enforced for both backends.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/planner/core/planbuilder_test.go`:
- Around line 1147-1157: The test's "no-error" cases currently only cover S3
with AK/SK and OSS with role-ARN; add the missing cross-backend credential
combinations so checkNextGenS3PathWithSem is validated for all forms: include
"oss://bucket?access-key=ak&secret-access-key=sk" and
"oss://bucket?access_key=ak&secret_access_key=sk" (OSS with AK/SK) and
"s3://bucket?role-arn=arn" and "s3://bucket?role_arn=arn" (S3 with role ARN) in
the same test loop to ensure the shared query-key behavior is enforced for both
backends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6e6e6f9-590f-41db-b137-1920456d7112
📒 Files selected for processing (5)
pkg/executor/import_into_test.gopkg/objstore/parse.gopkg/objstore/s3like/store.gopkg/planner/core/planbuilder.gopkg/planner/core/planbuilder_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68231 +/- ##
================================================
- Coverage 77.7304% 77.0293% -0.7012%
================================================
Files 1990 1972 -18
Lines 551907 552793 +886
================================================
- Hits 429000 425813 -3187
- Misses 121987 126977 +4990
+ Partials 920 3 -917
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, hawkingrei, joechenrh 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 |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
@D3Hunter: new pull request created to branch DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
5 participants
What problem does this PR solve?
Issue Number: close #68226
Problem Summary:
In NextGen security enhanced mode,
IMPORT INTOaccepted S3-like storage URIs without explicit user-provided credentials. That allowed the object-store client to fall back to TiDB node-role credentials, which weakens the expected boundary for user-specified import sources.What changed and how does it work?
This PR requires explicit authentication for S3-like
IMPORT INTOsources when NextGen and SEM are enabled.Check List
Tests
Unit tests:
./tools/check/failpoint-go-test.sh pkg/planner/core -tags=intest,deadlock,nextgen -run TestProcessNextGenS3Path -count=1./tools/check/failpoint-go-test.sh pkg/executor -tags=intest,deadlock,nextgen -run TestNextGenS3ExternalID -count=1make lintSide effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Bug Fixes
Tests