fix: support multi-mount OSS secret projections in jindo#5778
fix: support multi-mount OSS secret projections in jindo#5778CAICAIIs wants to merge 19 commits into
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 63.56% 63.35% -0.21%
==========================================
Files 479 480 +1
Lines 33276 33554 +278
==========================================
+ Hits 21151 21258 +107
- Misses 10445 10615 +170
- Partials 1680 1681 +1 ☔ View full report in Codecov by Harness. 🚀 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
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
|
done , thx your review |
…ount-secret-projection Signed-off-by: CAICAIIs <3360776475@qq.com> # Conflicts: # .github/scripts/build-all-images.sh
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
| dnsServer string | ||
| } | ||
|
|
||
| const ( |
There was a problem hiding this comment.
Nit: these constants and all helper functions below (buildBucketSecretURI, validateSecretKeyRef, getSecretDataValue, appendSecretProjection, setBucketSecretProviderProperties, buildBucketPropertyKey, extractSingleOSSEndpoint, parseOSSMountBucket) are identical to their counterparts in pkg/ddc/jindocache/transform.go (only the bucket prefix constant differs).
Consider extracting the shared logic into a common package (e.g. pkg/ddc/jindo/) with the prefix as a parameter to reduce duplication and future maintenance burden.
| e.Log.Error(err, "invalid encryptOption secret reference", "key", key, "mountPoint", mount.MountPoint) | ||
| return err | ||
| } | ||
| if mountType == "oss" && ossBucketName != "" { |
There was a problem hiding this comment.
Does the bug #4255 affect all mountType 's3', 'oss', 'cos' etc. ? If so, why not fix other mountType ?
There was a problem hiding this comment.
Thanks for pointing this out.
The issue title is broad, but the reproducer in #4255 and my validation are both for multiple oss:// mounts with different fs.oss.* Secret refs. I agree the existing non-OSS paths may have a similar single-secret/global-property limitation, because they still use the existing value.Secret / SecretKey / SecretValue flow or global <runtime>.<mountType>.accessKey* properties.
I did not include S3/COS/OBS in this PR because I have not validated their bucket-scoped credential semantics, and their provider/property keys may differ from the OSS bucket-scoped provider config used here. To keep this PR focused and reviewable, I scoped it to the reported OSS case and updated the PR description to say it partially addresses #4255 for OSS mounts. I can track S3/COS/OBS multi-mount credentials in a follow-up issue/PR.
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
|
|
This PR has existing CHANGES_REQUESTED status from @cheyang. The key outstanding items from prior reviews include: (1) extract duplicated OSS secret-projection helpers into a shared package, (2) fix error handling that uses break instead of return on secret fetch failure, (3) fix Helm template YAML indentation for projected volumes, and (4) clarify the inline credential persistence behavior when secretMountSupport=false. Please address these items and push an update. |
…ount-secret-projection Signed-off-by: CAICAIIs <3360776475@qq.com> # Conflicts: # .github/scripts/build-all-images.sh # test/gha-e2e/curvine/test.sh
Signed-off-by: CAICAIIs <3360776475@qq.com>
Signed-off-by: CAICAIIs <3360776475@qq.com>
|
done, can you have a another look? |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Code Review Summary
I reviewed the full scope of this PR (core transform logic, shared helpers, Helm templates, unit tests, and e2e infrastructure). This is a well-structured fix for a real problem — when multiple OSS mounts with different AK/SK pairs are configured in a Dataset, only the last one would take effect. The bucket-scoped projected secret approach is the right solution.
What looks good
-
Shared helper package (
pkg/ddc/jindo/oss_secret.go): Extracting common logic (bucket parsing, secret projection management, property key builders, validation) into a shared package addresses the earlier feedback about code duplication and improves maintainability. -
Projection deduplication and conflict detection in
AppendSecretProjection: Same-bucket-same-source correctly deduplicates, while conflicting sources for the same path correctly error out. -
Improved error handling: The previous code used
breakon secret retrieval failures (silently continuing), now properly returns errors. This is a meaningful correctness fix. -
Bug fix in jindofsx:
value.SecretKey = keywas incorrectly assigning the option name instead ofsecretKeyRef.Key. Fixed. -
Security: When
secretMountSupport=true, credentials never land in ConfigMaps. WhensecretMountSupport=false, a plaintext warning log is emitted per the earlier reviewer request. No credential values are logged. -
Test coverage: Comprehensive unit tests covering multi-mount projections, deduplication, conflict detection, inline credential compatibility, missing secrets, and the new fuse arg behavior. The OSS emulator-based e2e test validates that per-bucket credential isolation actually works end-to-end.
-
Chart versions bumped to 1.0.5 as previously requested.
Minor observations (non-blocking)
-
-ono_symlinksuppression for multi-mount: The change to only add-ono_symlinkwhenlen(dataset.Spec.Mounts) <= 1is correct — multi-mount requires symlink support for namespace directory resolution. Worth noting in release notes since it changes fuse behavior for all multi-mount datasets, not just those using bucket-scoped projections. -
ExtractSingleOSSEndpointnaming: The function returns the endpoint only when all configured buckets share the same endpoint (for global fallback). The name "single" could be read as "first" — something likeExtractUniformOSSEndpointmight be slightly clearer, but this is purely a naming preference and not blocking. -
OSS emulator auth: The emulator validates AccessKeyId but not the signature/secret. This is sufficient for e2e credential-routing verification but doesn't test that the actual secret value is correctly projected. The unit tests cover that aspect.
Previous review feedback status
All feedback from maintainers has been addressed:
- Constants extracted (cheyang, Apr 17) — done
- Shared package created (cheyang, Jun 1) — done
- Chart version bumped (cheyang, Apr 10) — done
- Plaintext credential warning added (cheyang, Jun 1) — done
- OSS-only scope clarified (xliuqq, Jun 1) — acknowledged, follow-up for S3/COS/OBS noted in PR description
The PR appears ready for maintainer re-approval.
Ⅰ. Describe what this PR does
This PR addresses multi-mount OSS secret handling for Jindo when different
oss://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 OSS Secret-ref path to bucket-scoped secret projections, so each OSS bucket gets its own projected secret path and provider configuration.The fix covers the OSS paths in both
jindocacheandjindofsx, and updates the related Helm templates for master / worker / fuse.Ⅱ. Does this pull request fix one issue?
Partially addresses #4255 for OSS mounts.
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added and updated unit 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
secretMountSupport=true, credentials are projected through bucket-scoped Secret paths. Inline mount options and thesecretMountSupport=falsefallback still write credentials into ConfigMap-backed properties for compatibility; those plaintext paths now emit warning logs.