fix: support multi-mount OSS secret projections in jindo#5778
fix: support multi-mount OSS secret projections in jindo#5778CAICAIIs wants to merge 1 commit intofluid-cloudnative:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @CAICAIIs. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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/test-infra repository. |
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
from
13f43d6 to
7a06f0f
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple OSS bucket credential projections in both the jindocache and jindofsx engines. It updates Helm templates to handle projected secret volumes and modifies the transformation logic to generate these projections from dataset mount options. The review feedback highlights a security inconsistency where jindofsx still persists inline credentials in plain text, suggests optimizing the grouping of secret projections to reduce pod spec verbosity, and identifies several bugs related to error messages and logging consistency.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5778 +/- ##
==========================================
- Coverage 58.51% 58.39% -0.13%
==========================================
Files 473 474 +1
Lines 32283 32618 +335
==========================================
+ Hits 18890 19046 +156
- Misses 11849 12024 +175
- Partials 1544 1548 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
from
8bd230b to
7c6841e
Compare
There was a problem hiding this comment.
Pull request overview
Fixes Jindo multi-mount OSS secret handling so that different OSS mounts (buckets) can use different AK/SK pairs by switching from a single “last-one-wins” secret to bucket-scoped projected secrets and per-bucket provider configuration.
Changes:
- Introduces bucket-scoped secret projections and per-bucket secret URIs in both
jindocacheandjindofsxtransforms. - Updates Fuse/Token properties to configure OSS credentials provider per bucket (endpoint/format/provider), avoiding persisting inline credentials.
- Updates Helm templates to mount credential volumes when either a single secret or
secretProjectionsare present, and adds projected-volume support.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/ddc/jindofsx/types.go | Adds fields to carry secret projections + bucket-to-secret URI mapping. |
| pkg/ddc/jindofsx/transform.go | Builds bucket-scoped projections/URIs and injects per-bucket provider properties (Fuse/Token). |
| pkg/ddc/jindofsx/transform_master_test.go | Adds unit tests for multi-mount OSS projections and non-persistence of inline creds. |
| pkg/ddc/jindocache/types.go | Adds fields to carry secret projections + bucket-to-secret URI mapping. |
| pkg/ddc/jindocache/transform.go | Builds bucket-scoped projections/URIs and injects per-bucket provider properties (Fuse/Token). |
| pkg/ddc/jindocache/transform_test.go | Adds unit tests for multi-mount OSS projections and non-persistence of inline creds. |
| charts/jindofsx/templates/worker/statefulset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindofsx/templates/master/statefulset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindofsx/templates/fuse/daemonset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindofsx/templates/_helpers.tpl | Adds projected secret volume rendering supporting secretProjections. |
| charts/jindocache/templates/worker/statefulset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindocache/templates/master/statefulset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindocache/templates/fuse/daemonset.yaml | Mounts secret volume when secret or secretProjections is set. |
| charts/jindocache/templates/_helpers.tpl | Adds projected secret volume rendering supporting secretProjections. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for multiple OSS bucket credentials in JindoCache and JindoFSx by utilizing Kubernetes projected volumes for secrets. Key changes include updates to Helm charts for projected volume support, new helper functions for managing secret projections, and logic in the transformation engines to handle bucket-specific credentials. Feedback highlights the need to improve error handling by returning errors immediately when secret retrieval fails and suggests optimizing performance by moving regex compilation outside of loops.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for bucket-scoped secret projections in JindoCache and JindoFSx, allowing multiple OSS mounts to use distinct credentials. The changes involve updating Helm templates to support projected volumes, refactoring the transformation logic to manage bucket-specific credential providers, and adding extensive unit tests. Feedback focuses on improving the clarity of error messages regarding credential configuration and adhering to structured logging practices by avoiding string formatting in log calls.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Adding an e2e test for the multi-mount OSS secret scenario would significantly strengthen confidence in this change. The existing Suggested e2e test: Set up two MinIO instances (or the same MinIO with different credentials) to simulate two OSS buckets with different AK/SK pairs, then create a Dataset with two OSS mounts each referencing their own This can reuse the existing MinIO-based test framework in Without an e2e test, the regression risk for this multi-mount scenario is high — a future change could reintroduce the same "last-one-wins" bug without being caught. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
4 times, most recently
from
7fea764 to
3114a48
Compare
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
from
880c89e to
66d64b8
Compare
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
6 times, most recently
from
742f6e5 to
6953964
Compare
| spec: | ||
| automountServiceAccountToken: false | ||
| containers: | ||
| - name: nginx |
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
2 times, most recently
from
659b02b to
f375bb6
Compare
Signed-off-by: CAICAIIs <3360776475@qq.com>
CAICAIIs
force-pushed
the
fix-jindo-multi-mount-secret-projection
branch
from
f375bb6 to
ccf1dcc
Compare
|
Ⅰ. Describe what this PR does
This PR fixes multi-mount OSS secret handling for Jindo when different mounts use different AK/SK pairs.
Before this change, if a
Datasetcontained multiple OSS mounts with differentencryptOptions, only the last OSS secret could effectively take effect. This PR changes the secret handling to bucket-scoped secret projections, so each OSS bucket gets its own projected secret path and provider configuration.The fix covers both
jindocacheandjindofsx, and updates the related Helm templates for master / worker / fuse.Ⅱ. Does this pull request fix one issue?
fixes #4255
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added unit tests:
TestJindoCacheEngine_transformMasterWithMultipleOSSEncryptOptionsTestJindoFSxEngine_transformMasterWithMultipleOSSEncryptOptionsTestJindoCacheEngine_transformMasterDoesNotPersistInlineOSSCredentialsThese tests cover:
Ⅳ. Describe how to verify it
Datasetwith two OSS mounts, for example:oss://caicaiis-a/->/partAoss://caicaiis-b/->/partBJindoRuntime.I also verified this with real OSS buckets:
caicaiis-acaicaiis-bAfter the fix:
/data/partAcan be read successfully/data/partBcan be read successfullyⅤ. Special notes for reviews