Extend CacheRuntime phase 2.2: support dataset secret mount options#5810
Conversation
Signed-off-by: xliuqq <xlzq1992@gmail.com>
![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 refactors the handling of encryption options in CacheRuntime by mounting secrets as volumes instead of using raw values. It removes the EncryptOption dependency from the CacheRuntimeClass CRD and updates the engine logic to map secret keys to container paths. Review feedback identifies several critical safety issues, specifically potential nil pointer dereferences when accessing the Topology field or the Containers slice, and suggests optimizations for the secret volume transformation logic to reduce redundant processing.
| func (e *CacheEngine) transformEncryptOptionsToComponentVolumes(dataset *datav1alpha1.Dataset, component *common.CacheRuntimeComponentValue) { | ||
| if component == nil || !component.Enabled { | ||
| return | ||
| } | ||
|
|
||
| for _, m := range dataset.Spec.Mounts { | ||
| if common.IsFluidNativeScheme(m.MountPoint) { | ||
| continue | ||
| } | ||
| for _, encryptOpt := range append(dataset.Spec.SharedEncryptOptions, m.EncryptOptions...) { | ||
| secretName := encryptOpt.ValueFrom.SecretKeyRef.Name | ||
|
|
||
| volName := getSecretVolumeName(secretName) | ||
| volumeToAdd := corev1.Volume{ | ||
| Name: volName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: secretName, | ||
| }, | ||
| }, | ||
| } | ||
| component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume( | ||
| component.PodTemplateSpec.Spec.Volumes, volumeToAdd) | ||
|
|
||
| volumeMountToAdd := corev1.VolumeMount{ | ||
| Name: volName, | ||
| ReadOnly: true, | ||
| MountPath: getSecretMountPath(secretName), | ||
| } | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts( | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation has a few issues:
- Efficiency: It performs redundant processing of
SharedEncryptOptionsfor every mount and creates multiple slice allocations viaappendinside the loop. It also callsAppendOrOverrideVolumeandAppendOrOverrideVolumeMountsmore often than necessary. - Safety: It accesses
component.PodTemplateSpec.Spec.Containers[0]without checking if theContainersslice is empty, which could lead to a runtime panic if aCacheRuntimeClassis defined with an empty pod template.
I suggest refactoring the logic to collect unique secret names first and adding a safety check for the containers slice.
func (e *CacheEngine) transformEncryptOptionsToComponentVolumes(dataset *datav1alpha1.Dataset, component *common.CacheRuntimeComponentValue) {
if component == nil || !component.Enabled || len(component.PodTemplateSpec.Spec.Containers) == 0 {
return
}
// Collect unique secret names to avoid redundant processing and allocations
secretNames := make(map[string]struct{})
hasNonNativeMount := false
for _, m := range dataset.Spec.Mounts {
if common.IsFluidNativeScheme(m.MountPoint) {
continue
}
hasNonNativeMount = true
for _, encryptOpt := range m.EncryptOptions {
if name := encryptOpt.ValueFrom.SecretKeyRef.Name; name != "" {
secretNames[name] = struct{}{}
}
}
}
// If no non-native mounts exist, we don't need to mount any secrets (including shared ones)
if !hasNonNativeMount {
return
}
// Add shared encrypt options
for _, encryptOpt := range dataset.Spec.SharedEncryptOptions {
if name := encryptOpt.ValueFrom.SecretKeyRef.Name; name != "" {
secretNames[name] = struct{}{}
}
}
for secretName := range secretNames {
volName := getSecretVolumeName(secretName)
volumeToAdd := corev1.Volume{
Name: volName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: secretName,
},
},
}
component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume(
component.PodTemplateSpec.Spec.Volumes, volumeToAdd)
volumeMountToAdd := corev1.VolumeMount{
Name: volName,
ReadOnly: true,
MountPath: getSecretMountPath(secretName),
}
// Mount to the first container (main engine container)
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts(
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd)
}
}| } | ||
|
|
||
| // dataset mount after runtime ready to ensure master pod is ready for executing commands. | ||
| if runtimeValue.Master.Enabled && runtimeClass.Topology.Master.ExecutionEntries != nil { |
There was a problem hiding this comment.
Potential nil pointer dereference. runtimeClass.Topology is an optional field in the CacheRuntimeClass spec. If it is not defined, accessing runtimeClass.Topology.Master will cause a panic. A nil check for Topology should be added.
| if runtimeValue.Master.Enabled && runtimeClass.Topology.Master.ExecutionEntries != nil { | |
| if runtimeValue.Master.Enabled && runtimeClass.Topology != nil && runtimeClass.Topology.Master.ExecutionEntries != nil { |
| func (e *CacheEngine) transformMaster(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | ||
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | ||
| // TODO: these two field both indicate Master enabled or not, should be combined into one field. | ||
| if runtimeClass.Topology.Master == nil || runtime.Spec.Master.Disabled { |
There was a problem hiding this comment.
Potential nil pointer dereference. runtimeClass.Topology is optional and could be nil. Accessing runtimeClass.Topology.Master without checking Topology first is unsafe.
| func (e *CacheEngine) transformMaster(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | |
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | |
| // TODO: these two field both indicate Master enabled or not, should be combined into one field. | |
| if runtimeClass.Topology.Master == nil || runtime.Spec.Master.Disabled { | |
| func (e *CacheEngine) transformMaster(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | |
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | |
| // TODO: these two field both indicate Master enabled or not, should be combined into one field. | |
| if runtimeClass.Topology == nil || runtimeClass.Topology.Master == nil || runtime.Spec.Master.Disabled { |
| func (e *CacheEngine) transformWorker(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | ||
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | ||
|
|
||
| if runtimeClass.Topology.Worker == nil || runtime.Spec.Worker.Disabled { |
There was a problem hiding this comment.
Potential nil pointer dereference. runtimeClass.Topology is optional and could be nil. Accessing runtimeClass.Topology.Worker without checking Topology first is unsafe.
| func (e *CacheEngine) transformWorker(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | |
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | |
| if runtimeClass.Topology.Worker == nil || runtime.Spec.Worker.Disabled { | |
| func (e *CacheEngine) transformWorker(dataset *datav1alpha1.Dataset, runtime *datav1alpha1.CacheRuntime, runtimeClass *datav1alpha1.CacheRuntimeClass, | |
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | |
| if runtimeClass.Topology == nil || runtimeClass.Topology.Worker == nil || runtime.Spec.Worker.Disabled { |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5810 +/- ##
==========================================
+ Coverage 58.17% 59.10% +0.92%
==========================================
Files 478 480 +2
Lines 32485 32511 +26
==========================================
+ Hits 18899 19215 +316
+ Misses 12042 11746 -296
- Partials 1544 1550 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: xliuqq <xlzq1992@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for Dataset secret-backed mount options in the generic CacheRuntime integration by mounting referenced Kubernetes Secrets into component pods and emitting secret file paths (instead of plaintext values) into the runtime config JSON.
Changes:
- Extend runtime config generation to output
mounts[].encryptOptionsas a map of option name → secret file path. - Mount referenced Secrets into CacheRuntime master/worker pods under
/etc/fluid/secrets/<secretName>. - Update Curvine e2e manifests/scripts and integration docs to use secret-mounted credentials.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/gha-e2e/curvine/mount.yaml | Updates e2e mount script to read credentials from secret-mounted files via encryptOptions. |
| test/gha-e2e/curvine/dataset.yaml | Switches e2e dataset to use encryptOptions.valueFrom.secretKeyRef and adds a Secret manifest. |
| pkg/ddc/cache/engine/util.go | Adds helpers for secret volume naming and mount/file path generation. |
| pkg/ddc/cache/engine/ufs.go | Adjusts PrepareUFS signature to accept MountUFS entry directly. |
| pkg/ddc/cache/engine/transform_worker.go | Mounts Dataset encrypt-option secrets into worker pod volumes/mounts. |
| pkg/ddc/cache/engine/transform_volumes_test.go | Adds unit tests for secret volume/mount transformation. |
| pkg/ddc/cache/engine/transform_volumes.go | Implements secret volume/mount injection for encrypt options. |
| pkg/ddc/cache/engine/transform_master.go | Mounts Dataset encrypt-option secrets into master pod volumes/mounts. |
| pkg/ddc/cache/engine/transform.go | Threads dataset into master/worker transforms. |
| pkg/ddc/cache/engine/status.go | Import ordering cleanup (no functional change). |
| pkg/ddc/cache/engine/setup.go | Defers mount execution until runtime is ready; avoids nil ExecutionEntries access. |
| pkg/ddc/cache/engine/dataset.go | Changes mount option generation to return both options and encryptOptions (paths). |
| pkg/ddc/cache/engine/cm.go | Writes EncryptOptions into runtime config JSON. |
| pkg/ddc/cache/engine/cache_engine_suite_test.go | Adds ginkgo suite bootstrap for cache engine tests. |
| pkg/common/cacheruntime.go | Extends MountConfig to include EncryptOptions in runtime config JSON. |
| docs/zh/dev/generic_cache_runtime_integration.md | Updates integration documentation to mention encryptOptions in runtime config. |
| docs/en/dev/generic_cache_runtime_integration.md | Updates integration documentation to mention encryptOptions in runtime config. |
| config/crd/bases/data.fluid.io_cacheruntimeclasses.yaml | Removes dependencies.encryptOption from CacheRuntimeClass CRD schema. |
| charts/fluid/fluid/crds/data.fluid.io_cacheruntimeclasses.yaml | Mirrors CRD schema change in Helm chart CRDs. |
| api/v1alpha1/zz_generated.deepcopy.go | Updates deepcopy generation (incl. DataOperationSpecs slice). |
| api/v1alpha1/openapi_generated.go | Removes OpenAPI schema for EncryptOptionComponentDependency. |
| api/v1alpha1/cacheruntimeclass_types.go | Removes EncryptOptionComponentDependency and dependencies.encryptOption from API types. |
Comments suppressed due to low confidence (3)
api/v1alpha1/cacheruntimeclass_types.go:96
RuntimeComponentDependencies.encryptOptionwas removed from the API. Even if it was previously unused, removing a field from a served version is a breaking change for users who already have CacheRuntimeClass manifests containing it (they will fail validation on re-apply/upgrade). Consider keeping the field for backward compatibility (deprecated/no-op), or introducing a versioned migration strategy before removing it.
// RuntimeComponentDependencies defines the dependencies required by a CacheRuntime component
type RuntimeComponentDependencies struct {
// ExtraResources specifies the usage of extra resources such as ConfigMaps
// +optional
ExtraResources *ExtraResourcesComponentDependency `json:"extraResources,omitempty"`
}
config/crd/bases/data.fluid.io_cacheruntimeclasses.yaml:87
- The CRD schema removes
spec.topology.*.dependencies.encryptOption. This is an API-breaking change for any existing CacheRuntimeClass resources/manifests that include this field (they will be rejected by validation on upgrade). Consider retaining the field (even as deprecated/no-op) to preserve backward compatibility, or clearly versioning the CRD change.
properties:
client:
properties:
dependencies:
properties:
extraResources:
properties:
configMaps:
items:
properties:
mountPath:
type: string
name:
type: string
type: object
type: array
type: object
type: object
charts/fluid/fluid/crds/data.fluid.io_cacheruntimeclasses.yaml:87
- The Helm chart CRD also removes
spec.topology.*.dependencies.encryptOption, which will break upgrades for clusters that already have CacheRuntimeClass manifests using this field. Consider keeping the field (deprecated/no-op) or versioning the CRD to avoid rejecting existing resources during helm upgrade.
properties:
client:
properties:
dependencies:
properties:
extraResources:
properties:
configMaps:
items:
properties:
mountPath:
type: string
name:
type: string
type: object
type: array
type: object
type: object
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| secretName := encryptOpt.ValueFrom.SecretKeyRef.Name | ||
|
|
There was a problem hiding this comment.
transformEncryptOptionsToComponentVolumes assumes every EncryptOption has a non-empty valueFrom.secretKeyRef.name, but the Dataset CRD allows valueFrom/secretKeyRef to be omitted. When it’s empty, this will generate an invalid volume name (cache-mount-secret-) and attempt to mount a secret with an empty name. Please add a guard (and ideally surface a validation error earlier) before creating the Volume/VolumeMount.
| secretName := encryptOpt.ValueFrom.SecretKeyRef.Name | |
| if encryptOpt.ValueFrom == nil || encryptOpt.ValueFrom.SecretKeyRef == nil || encryptOpt.ValueFrom.SecretKeyRef.Name == "" { | |
| continue | |
| } | |
| secretName := encryptOpt.ValueFrom.SecretKeyRef.Name |
| // getSecretVolumeName generates the volume name for a secret mount | ||
| func getSecretVolumeName(secretName string) string { | ||
| return fmt.Sprintf("cache-mount-secret-%s", secretName) | ||
| } |
There was a problem hiding this comment.
getSecretVolumeName directly embeds secretName into the pod volume name. Kubernetes volume names are limited to 63 characters, while Secret names can be much longer, so long secret names will cause pod validation failures. Consider truncating and appending a hash (or otherwise normalizing) to ensure the generated volume name always satisfies DNS label length constraints.
| "mountPoint": "s3://test", | ||
| "options": { | ||
| "access": "minioadmin", | ||
| "endpoint_url": "http://minio:9000", | ||
| "path_style": "true", | ||
| "region_name": "us-east-1", | ||
| "secret": "minioadmin" | ||
| }, | ||
| "encryptOptions": { | ||
| "access-key": "/etc/fluid/secrets/minio-secret/access-key", | ||
| "secret-key": "/etc/fluid/secrets/minio-secret/secret-key" | ||
| }, |
There was a problem hiding this comment.
The JSON example still shows secrets (access/secret) embedded directly under options, which contradicts the new behavior of moving secret-backed values into encryptOptions as file paths. Please update the example so sensitive values are not shown in options, and ensure the encryptOptions keys match the Dataset EncryptOption.name (e.g., access/secret in the curvine e2e example).
| for _, item := range encryptOpts { | ||
| sRef := item.ValueFrom.SecretKeyRef | ||
| secret, err := kubeclient.GetSecret(e.Client, sRef.Name, e.namespace) | ||
| if err != nil { | ||
| e.Log.Error(err, "get secret by mount encrypt options failed", "name", item.Name) | ||
| return mOptions, err | ||
| } | ||
|
|
||
| e.Log.Info("get value from secret", "mount name", name, "secret key", sRef.Key) | ||
| // Construct the secret mount path in the container | ||
| // The secret will be mounted at /etc/fluid/secrets/<secret-name>/<key> | ||
| secretPath := getSecretFilePath(sRef.Name, sRef.Key) | ||
|
|
||
| v := secret.Data[sRef.Key] | ||
| mOptions[item.Name] = string(v) | ||
| // Store in map: key is option name, value is secret path | ||
| existingEncryptOpts[item.Name] = secretPath | ||
| } |
There was a problem hiding this comment.
collectEncryptOptions currently constructs file paths even when valueFrom.secretKeyRef is omitted (allowed by the CRD) or when secretKeyRef.key is empty, producing invalid paths like /etc/fluid/secrets// and later invalid secret volumes. Consider validating that secretKeyRef.name and secretKeyRef.key are non-empty and returning an error that includes the mount/option name to make misconfigurations actionable. Also consider rejecting duplicate EncryptOption.name entries instead of silently overwriting entries in the map.
| "options": { | ||
| "access": "minioadmin", | ||
| "endpoint_url": "http://minio:9000", | ||
| "path_style": "true", | ||
| "region_name": "us-east-1", | ||
| "secret": "minioadmin" | ||
| }, | ||
| "encryptOptions": { | ||
| "access-key": "/etc/fluid/secrets/minio-secret/access-key", | ||
| "secret-key": "/etc/fluid/secrets/minio-secret/secret-key" | ||
| }, |
There was a problem hiding this comment.
文档中的 JSON 示例仍在 options 里直接展示 access/secret 等敏感值，但本 PR 的实现是把密钥通过 encryptOptions 以文件路径形式提供（避免明文出现在配置中）。建议同步更新示例：移除 options 中的敏感值，并确保 encryptOptions 的 key 与 Dataset EncryptOption.name 一致。
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of encrypted mount options in the CacheRuntime by mounting secrets as volumes into component pods instead of embedding secret values directly into the configuration. Key changes include updating the MountConfig structure to store secret file paths, refactoring the CacheEngine to handle volume transformations for both Master and Worker components, and updating CRDs and documentation accordingly. Feedback was provided regarding the new transformEncryptOptionsToComponentVolumes function, specifically pointing out a potential runtime panic when accessing the containers slice and suggesting a more efficient way to process shared and mount-specific encrypt options to avoid redundant operations.
| func (e *CacheEngine) transformEncryptOptionsToComponentVolumes(dataset *datav1alpha1.Dataset, component *common.CacheRuntimeComponentValue) { | ||
| if component == nil || !component.Enabled { | ||
| return | ||
| } | ||
|
|
||
| for _, m := range dataset.Spec.Mounts { | ||
| if common.IsFluidNativeScheme(m.MountPoint) { | ||
| continue | ||
| } | ||
| for _, encryptOpt := range append(dataset.Spec.SharedEncryptOptions, m.EncryptOptions...) { | ||
| secretName := encryptOpt.ValueFrom.SecretKeyRef.Name | ||
|
|
||
| volName := getSecretVolumeName(secretName) | ||
| volumeToAdd := corev1.Volume{ | ||
| Name: volName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: secretName, | ||
| }, | ||
| }, | ||
| } | ||
| component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume( | ||
| component.PodTemplateSpec.Spec.Volumes, volumeToAdd) | ||
|
|
||
| volumeMountToAdd := corev1.VolumeMount{ | ||
| Name: volName, | ||
| ReadOnly: true, | ||
| MountPath: getSecretMountPath(secretName), | ||
| } | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts( | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of transformEncryptOptionsToComponentVolumes has several issues:
- Potential Panic: It accesses
component.PodTemplateSpec.Spec.Containers[0]without checking if theContainersslice is empty. This could lead to a runtime panic if the component is misconfigured. - Performance Inefficiency: It performs a slice
appendoperation inside a nested loop (dataset.Spec.Mounts), which leads to unnecessary memory allocations for every mount. - Logic Inconsistency:
SharedEncryptOptionsare only processed if there is at least one non-native mount. IfSharedEncryptOptionsare intended to be global for the dataset, they should be processed independently of the specific mounts. - Redundant Processing:
SharedEncryptOptionsare re-processed for every mount in the loop, even thoughutils.AppendOrOverrideVolumehandles the duplication.
func (e *CacheEngine) transformEncryptOptionsToComponentVolumes(dataset *datav1alpha1.Dataset, component *common.CacheRuntimeComponentValue) {
if component == nil || !component.Enabled || len(component.PodTemplateSpec.Spec.Containers) == 0 {
return
}
// Helper to add secret volume and mount to the component
addSecret := func(secretName string) {
volName := getSecretVolumeName(secretName)
volumeToAdd := corev1.Volume{
Name: volName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: secretName,
},
},
}
component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume(
component.PodTemplateSpec.Spec.Volumes, volumeToAdd)
volumeMountToAdd := corev1.VolumeMount{
Name: volName,
ReadOnly: true,
MountPath: getSecretMountPath(secretName),
}
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts(
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd)
}
// 1. Process shared encrypt options once
for _, encryptOpt := range dataset.Spec.SharedEncryptOptions {
addSecret(encryptOpt.ValueFrom.SecretKeyRef.Name)
}
// 2. Process mount-specific encrypt options
for _, m := range dataset.Spec.Mounts {
if common.IsFluidNativeScheme(m.MountPoint) {
continue
}
for _, encryptOpt := range m.EncryptOptions {
addSecret(encryptOpt.ValueFrom.SecretKeyRef.Name)
}
}
}Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of encrypted mount options in the CacheRuntime by transitioning from direct secret fetching to mounting secrets as volumes within component pods. The changes involve removing the EncryptOptionComponentDependency type, updating CRD schemas, and implementing logic to transform dataset encryption options into pod volumes and mounts. Documentation and E2E tests have been updated to reflect these architectural changes. The review feedback suggests optimizing the volume transformation process to improve efficiency and simplifying internal methods by removing redundant error returns.
| // Helper to add secret volume and mount to the component | ||
| addSecret := func(secretName string) { | ||
| volName := getSecretVolumeName(secretName) | ||
| volumeToAdd := corev1.Volume{ | ||
| Name: volName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: secretName, | ||
| }, | ||
| }, | ||
| } | ||
| component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume( | ||
| component.PodTemplateSpec.Spec.Volumes, volumeToAdd) | ||
|
|
||
| volumeMountToAdd := corev1.VolumeMount{ | ||
| Name: volName, | ||
| ReadOnly: true, | ||
| MountPath: getSecretMountPath(secretName), | ||
| } | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts( | ||
| component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd) | ||
| } | ||
|
|
||
| // 1. Process shared encrypt options once | ||
| for _, encryptOpt := range dataset.Spec.SharedEncryptOptions { | ||
| addSecret(encryptOpt.ValueFrom.SecretKeyRef.Name) | ||
| } | ||
|
|
||
| // 2. Process mount-specific encrypt options, override shared options | ||
| for _, m := range dataset.Spec.Mounts { | ||
| if common.IsFluidNativeScheme(m.MountPoint) { | ||
| continue | ||
| } | ||
| for _, encryptOpt := range m.EncryptOptions { | ||
| addSecret(encryptOpt.ValueFrom.SecretKeyRef.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of transformEncryptOptionsToComponentVolumes is inefficient because it calls utils.AppendOrOverrideVolume and utils.AppendOrOverrideVolumeMounts for every EncryptOption across all mounts. Since these utility functions typically iterate through the existing volumes/mounts to check for duplicates, this results in O(N^2) complexity relative to the number of mounts and secrets. It is better to collect the unique secret names first and then add the volumes and mounts once per unique secret.
// 1. Collect unique secret names to avoid redundant volume/mount additions
secretNames := make(map[string]struct{})
for _, encryptOpt := range dataset.Spec.SharedEncryptOptions {
secretNames[encryptOpt.ValueFrom.SecretKeyRef.Name] = struct{}{}
}
for _, m := range dataset.Spec.Mounts {
if common.IsFluidNativeScheme(m.MountPoint) {
continue
}
for _, encryptOpt := range m.EncryptOptions {
secretNames[encryptOpt.ValueFrom.SecretKeyRef.Name] = struct{}{}
}
}
// 2. Add secret volumes and mounts to the component
for secretName := range secretNames {
volName := getSecretVolumeName(secretName)
volumeToAdd := corev1.Volume{
Name: volName,
VolumeSource: corev1.VolumeSource{
Secret: &corev1.SecretVolumeSource{
SecretName: secretName,
},
},
}
component.PodTemplateSpec.Spec.Volumes = utils.AppendOrOverrideVolume(
component.PodTemplateSpec.Spec.Volumes, volumeToAdd)
volumeMountToAdd := corev1.VolumeMount{
Name: volName,
ReadOnly: true,
MountPath: getSecretMountPath(secretName),
}
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts = utils.AppendOrOverrideVolumeMounts(
component.PodTemplateSpec.Spec.Containers[0].VolumeMounts, volumeMountToAdd)
}| func (e *CacheEngine) generateDatasetMountOptions(m *datav1alpha1.Mount, sharedEncryptOptions []datav1alpha1.EncryptOption, | ||
| sharedOptions map[string]string) (map[string]string, error) { | ||
| sharedOptions map[string]string) (map[string]string, map[string]string, error) { | ||
|
|
||
| // initialize mount options | ||
| // initialize mount options, mount options will overwrite shared options. | ||
| mOptions := map[string]string{} | ||
| for k, v := range sharedOptions { | ||
| mOptions[k] = v | ||
| } | ||
|
|
||
| for key, value := range m.Options { | ||
| mOptions[key] = value | ||
| } | ||
|
|
||
| // if encryptOptions have the same key with options, it will overwrite the corresponding value | ||
| // collect encrypt options, mount options will overwrite shared options. | ||
| encryptOptions := map[string]string{} | ||
| var err error | ||
| mOptions, err = e.genEncryptOptions(sharedEncryptOptions, mOptions, m.Name) | ||
| err = e.collectEncryptOptions(sharedEncryptOptions, encryptOptions) | ||
| if err != nil { | ||
| return mOptions, err | ||
| return mOptions, encryptOptions, err | ||
| } | ||
|
|
||
| // gen public encryptOptions | ||
| mOptions, err = e.genEncryptOptions(m.EncryptOptions, mOptions, m.Name) | ||
| err = e.collectEncryptOptions(m.EncryptOptions, encryptOptions) | ||
| if err != nil { | ||
| return mOptions, err | ||
| return mOptions, encryptOptions, err | ||
| } | ||
|
|
||
| return mOptions, nil | ||
| return mOptions, encryptOptions, nil | ||
| } | ||
|
|
||
| func (e *CacheEngine) genEncryptOptions(EncryptOptions []datav1alpha1.EncryptOption, mOptions map[string]string, name string) (map[string]string, error) { | ||
| for _, item := range EncryptOptions { | ||
| if _, ok := mOptions[item.Name]; ok { | ||
| err := fmt.Errorf("the option %s is set more than one times, please double check the dataset's option and encryptOptions", item.Name) | ||
| return mOptions, err | ||
| } | ||
| func (e *CacheEngine) collectEncryptOptions(encryptOpts []datav1alpha1.EncryptOption, existingEncryptOpts map[string]string) error { | ||
|
|
||
| securityutil.UpdateSensitiveKey(item.Name) | ||
| for _, item := range encryptOpts { | ||
| sRef := item.ValueFrom.SecretKeyRef | ||
| secret, err := kubeclient.GetSecret(e.Client, sRef.Name, e.namespace) | ||
| if err != nil { | ||
| e.Log.Error(err, "get secret by mount encrypt options failed", "name", item.Name) | ||
| return mOptions, err | ||
| } | ||
|
|
||
| e.Log.Info("get value from secret", "mount name", name, "secret key", sRef.Key) | ||
| // Construct the secret mount path in the container | ||
| // The secret will be mounted at /etc/fluid/secrets/<secret-name>/<key> | ||
| secretPath := getSecretFilePath(sRef.Name, sRef.Key) | ||
|
|
||
| v := secret.Data[sRef.Key] | ||
| mOptions[item.Name] = string(v) | ||
| // Store in map: key is option name, value is secret path | ||
| existingEncryptOpts[item.Name] = secretPath | ||
| } | ||
|
|
||
| return mOptions, nil | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The functions generateDatasetMountOptions and collectEncryptOptions no longer perform any operations that can fail (like fetching secrets from the API server). Consequently, they always return a nil error. Removing the error return simplifies the code and eliminates unnecessary error checks in the callers.
func (e *CacheEngine) generateDatasetMountOptions(m *datav1alpha1.Mount, sharedEncryptOptions []datav1alpha1.EncryptOption,
sharedOptions map[string]string) (map[string]string, map[string]string) {
// initialize mount options, mount options will overwrite shared options.
mOptions := map[string]string{}
for k, v := range sharedOptions {
mOptions[k] = v
}
for key, value := range m.Options {
mOptions[key] = value
}
// collect encrypt options, mount options will overwrite shared options.
encryptOptions := map[string]string{}
e.collectEncryptOptions(sharedEncryptOptions, encryptOptions)
e.collectEncryptOptions(m.EncryptOptions, encryptOptions)
return mOptions, encryptOptions
}
func (e *CacheEngine) collectEncryptOptions(encryptOpts []datav1alpha1.EncryptOption, existingEncryptOpts map[string]string) {
for _, item := range encryptOpts {
sRef := item.ValueFrom.SecretKeyRef
// Construct the secret mount path in the container
// The secret will be mounted at /etc/fluid/secrets/<secret-name>/<key>
secretPath := getSecretFilePath(sRef.Name, sRef.Key)
// Store in map: key is option name, value is secret path
existingEncryptOpts[item.Name] = secretPath
}
}| options, encryptOptions, err := e.generateDatasetMountOptions(&m, dataset.Spec.SharedEncryptOptions, dataset.Spec.SharedOptions) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| mountCg.Options = options | ||
| mountCg.EncryptOptions = encryptOptions |
There was a problem hiding this comment.
Update the caller to match the simplified signature of generateDatasetMountOptions (removing the error return).
options, encryptOptions := e.generateDatasetMountOptions(&m, dataset.Spec.SharedEncryptOptions, dataset.Spec.SharedOptions)
mountCg.Options = options
mountCg.EncryptOptions = encryptOptionsThere was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 1. Process shared encrypt options once | ||
| for _, encryptOpt := range dataset.Spec.SharedEncryptOptions { | ||
| addSecret(encryptOpt.ValueFrom.SecretKeyRef.Name) | ||
| } |
There was a problem hiding this comment.
transformEncryptOptionsToComponentVolumes assumes every encrypt option has a non-empty secretKeyRef.name. If it’s empty (allowed by the current API struct tags), this will add a Secret volume with SecretName: "" and a potentially invalid volume/mount name, causing pod validation failures. Add a guard (and ideally log/record an error) to skip or fail fast when secretName is empty.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
| } | ||
|
|
||
| // getSecretVolumeName generates the volume name for a secret mount | ||
| func getSecretVolumeName(secretName string) string { |
There was a problem hiding this comment.
The cache-mount-secret- prefix is 19 chars. K8s Secret names can be up to 253 chars, so the resulting volume name could blow past the 63-char DNS-1123 label limit and fail pod creation. Worth adding a truncate+hash fallback for long secret names.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of encrypted options in CacheRuntime by mounting secrets as volumes into component pods instead of embedding their values in the configuration. Key changes include updates to API types, CRD schemas, and the transformation logic that maps dataset EncryptOptions to Kubernetes volumes and mounts for Master and Worker components. Additionally, utility functions were introduced to generate DNS-compliant volume names. Review feedback suggests extending this secret mounting logic to the Client component, adding validation for empty secret names during volume creation, and fixing a potential DNS-1035 violation in the volume naming utility when handling empty inputs.
| if runtimeClass.Topology.Client == nil || runtime.Spec.Client.Disabled { | ||
| value.Client.Enabled = false | ||
| value.Client = &common.CacheRuntimeComponentValue{Enabled: false} | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The transformEncryptOptionsToComponentVolumes function is called for both Master and Worker components, but it appears to be missing for the Client component. If a client is enabled and needs to access the underlying file system (UFS) using encrypted options, the corresponding secrets must be mounted in the client pod as well.
| } | ||
|
|
||
| // Helper to add secret volume and mount to the component | ||
| addSecret := func(secretName string) { |
There was a problem hiding this comment.
It is recommended to add a check for an empty secretName here. While higher-level validation might catch this, ensuring robustness in this helper prevents the creation of invalid Kubernetes volumes (e.g., with an empty SecretName) if an invalid EncryptOption somehow reaches this stage.
addSecret := func(secretName string) {
if secretName == "" {
return
}| func getSecretVolumeName(name string) string { | ||
| fullName := fmt.Sprintf("%s%s", secretVolumeNamePrefix, name) | ||
| // check volume name length | ||
| if len(fullName) <= validation.DNS1035LabelMaxLength { | ||
| return fullName | ||
| } |
There was a problem hiding this comment.
The current implementation of getSecretVolumeName returns cache-mnt-secret- if the input name is empty. According to Kubernetes DNS-1035 label requirements, a label must start and end with an alphanumeric character. A trailing hyphen makes the volume name invalid. Although collectEncryptOptions validates for empty names, this utility should be robust enough to return a valid DNS label or handle empty inputs gracefully.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sRef := item.ValueFrom.SecretKeyRef | ||
| secret, err := kubeclient.GetSecret(e.Client, sRef.Name, e.namespace) | ||
| if err != nil { | ||
| e.Log.Error(err, "get secret by mount encrypt options failed", "name", item.Name) | ||
| return mOptions, err | ||
| if sRef.Name == "" || sRef.Key == "" { | ||
| return fmt.Errorf("encryptOption %s has empty secretKeyRef name or key", item.Name) | ||
| } |
| @@ -26,7 +26,7 @@ func (e *CacheEngine) transformClient(runtime *datav1alpha1.CacheRuntime, runtim | |||
| config *CacheRuntimeComponentCommonConfig, value *common.CacheRuntimeValue) error { | |||
|
|
|||
| if runtimeClass.Topology.Client == nil || runtime.Spec.Client.Disabled { | |||
| value.Client.Enabled = false | |||
| value.Client = &common.CacheRuntimeComponentValue{Enabled: false} | |||
| return nil | |||
| } | |||
| It("should handle empty secret name", func() { | ||
| secretName := "" | ||
| result := getSecretVolumeName(secretName) | ||
| expected := secretVolumeNamePrefix | ||
|
|
||
| Expect(result).To(Equal(expected)) | ||
| Expect(len(result)).To(BeNumerically("<=", validation.DNS1035LabelMaxLength)) | ||
| }) |
| // getSecretVolumeName generates the volume name for a secret mount | ||
| func getSecretVolumeName(name string) string { | ||
| fullName := fmt.Sprintf("%s%s", secretVolumeNamePrefix, name) | ||
| // check volume name length | ||
| if len(fullName) <= validation.DNS1035LabelMaxLength { | ||
| return fullName | ||
| } | ||
|
|
||
| // Case 2: Long name - truncate + hash (fallback) | ||
| // Step 1: Truncate secret to 36 chars | ||
| truncatedName := name | ||
| if len(truncatedName) > truncatedSecretMaxLength { | ||
| truncatedName = truncatedName[:truncatedSecretMaxLength] | ||
| } | ||
|
|
||
| // Step 2: Generate 8-char SHA-256 hash of the ORIGINAL secret name (prevents collisions) | ||
| hash := sha256.Sum256([]byte(name)) | ||
| shortHash := hex.EncodeToString(hash[:])[:hashSuffixLength] | ||
|
|
||
| // Step 3: Combine to exact 63 chars | ||
| volumeName := secretVolumeNamePrefix + truncatedName + shortHash | ||
|
|
||
| return volumeName |
| // Helper to add secret volume and mount to the component | ||
| addSecret := func(secretName string) { | ||
| volName := getSecretVolumeName(secretName) | ||
| volumeToAdd := corev1.Volume{ | ||
| Name: volName, | ||
| VolumeSource: corev1.VolumeSource{ | ||
| Secret: &corev1.SecretVolumeSource{ | ||
| SecretName: secretName, | ||
| }, |
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Found 4 must-fix nil dereference / guard issues that need to be addressed before merge.
setup.go:89—runtimeClass.Topologydereference without nil guard (fragile implicit guarantee from transform path)transform_master.go:27—runtimeClass.Topology.Masterdereference can panic when Topology is nil (reconcile loop crash)transform_worker.go:27— same nil dereference risk forruntimeClass.Topology.Workertransform_volumes.go:34—addSecretclosure lacks empty secretName guard; separate path from cm.go validation chain
All 4 points have concrete fix suggestions in the inline comments below.
Signed-off-by: xliuqq <xlzq1992@gmail.com>
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Design suggestion: Client component secret mount should be configurable per runtime implementation.
Different runtime implementations have different needs — JuiceFS Client (FUSE pod) reads secret files directly to mount underlying storage, while Alluxio Client only does local cache reads and doesn't need secrets. Hardcoding secret mount for all Clients would unnecessarily expand Pod secret access scope.
Consider adding a toggle in RuntimeComponentDependencies so runtime integrators can control this via CacheRuntimeClass YAML rather than Fluid core code:
type RuntimeComponentDependencies struct {
// SecretMount controls whether dataset encrypt-option secrets are mounted into this component's pod.
// Defaults to true for Master/Worker, false for Client unless explicitly enabled.
// +optional
SecretMount *SecretMountComponentDependency `json:"secretMount,omitempty"`
ExtraResources *ExtraResourcesComponentDependency `json:"extraResources,omitempty"`
}
type SecretMountComponentDependency struct {
// +optional
Enabled bool `json:"enabled,omitempty"`
}This approach:
- Avoids mounting unused secrets into Client pods that don't need them (security: smaller secret access surface)
- Lets runtime integrators control behavior through CacheRuntimeClass, not hardcoded logic
- Keeps current behavior as default (Client = no mount) for backward compatibility
- New runtime implementations don't need Fluid core code changes
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
Design suggestion: Client component secret mount should be configurable per runtime implementation.
pkg/ddc/cache/engine/transform_client.go:50 — No call to transformEncryptOptionsToComponentVolumes here. Before adding one, consider whether this should be gated by a CacheRuntimeClass configuration toggle. Different runtimes have different needs — JuiceFS Client (FUSE pod) needs secret file access to mount underlying storage, while Alluxio Client only does local cache reads and does not need secrets. Hardcoding secret mount for all Clients would unnecessarily expand Pod secret access scope.
api/v1alpha1/cacheruntimeclass_types.go:91 — Since EncryptOptionComponentDependency was removed here, this is a good place to introduce the replacement toggle. Consider adding a SecretMount *SecretMountComponentDependency field in RuntimeComponentDependencies so each component (Master/Worker/Client) can independently control whether dataset secrets are mounted into its pod:
type RuntimeComponentDependencies struct {
// SecretMount controls whether dataset encrypt-option secrets are mounted into this component pod.
// Defaults to true for Master/Worker, false for Client unless explicitly enabled.
// +optional
SecretMount *SecretMountComponentDependency `json:"secretMount,omitempty"`
ExtraResources *ExtraResourcesComponentDependency `json:"extraResources,omitempty"`
}
type SecretMountComponentDependency struct {
// +optional
Enabled bool `json:"enabled,omitempty"`
}Benefits of this approach:
- Avoids mounting unused secrets into Client pods that do not need them (security: smaller secret access surface)
- Lets runtime integrators control behavior through CacheRuntimeClass YAML, not hardcoded logic
- Keeps current behavior as default (Client = no mount) for backward compatibility
- New runtime implementations do not need Fluid core code changes
…ion. Signed-off-by: xliuqq <xlzq1992@gmail.com>
Signed-off-by: xliuqq <xlzq1992@gmail.com>
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang 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 |
Ⅰ. Describe what this PR does
support dataset secret mount options
Ⅱ. Does this pull request fix one issue?
part of #5412
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
existed e2e test
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews