From de8a8e0874bed9b5252565bda77aaf25ea57b150 Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 11 May 2023 20:28:38 +0200 Subject: [PATCH 1/7] feat: Add support to specify file permissions for pvc hostpaths Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/config.go | 104 ++++++++++++++++-- .../app/provisioner_hostpath.go | 2 +- docs/quickstart.md | 15 ++- docs/tutorials/hostpath/filepermissions.md | 54 +++++++++ 4 files changed, 161 insertions(+), 14 deletions(-) create mode 100644 docs/tutorials/hostpath/filepermissions.md diff --git a/cmd/provisioner-localpv/app/config.go b/cmd/provisioner-localpv/app/config.go index 40012452..cf816bd5 100644 --- a/cmd/provisioner-localpv/app/config.go +++ b/cmd/provisioner-localpv/app/config.go @@ -130,6 +130,26 @@ const ( KeyQuotaSoftLimit = "softLimitGrace" KeyQuotaHardLimit = "hardLimitGrace" + + // FilePermissions allows to define the default directory mode + // Exemple StorageClass snippet: + // - name: FilePermissions + // enabled: true + // data: + // UID: 1000 + // GID: 1000 + // mode: g+s + // This is the cas-template key for all file permission 'data' keys + KeyFilePermissions = "FilePermissions" + + // FsUID defines the user owner of the shared directory + KeyFsUID = "UID" + + // FsGID defines the group owner of the shared directory + KeyFsGID = "GID" + + // FSMode defines the file permission mode of the shared directory + KeyFsMode = "mode" ) const ( @@ -170,15 +190,21 @@ func (p *Provisioner) GetVolumeConfig(ctx context.Context, pvName string, pvc *c } //TODO : extract and merge the cas volume config from pvc - //This block can be added once validation checks are added + // This block can be added once validation checks are added // as to the type of config that can be passed via PVC - //pvcCASConfigStr := pvc.ObjectMeta.Annotations[string(mconfig.CASConfigKey)] - //if len(strings.TrimSpace(pvcCASConfigStr)) != 0 { - // pvcCASConfig, err := cast.UnMarshallToConfig(pvcCASConfigStr) - // if err == nil { - // pvConfig = cast.MergeConfig(pvcCASConfig, pvConfig) - // } - //} + pvcCASConfigStr := pvc.ObjectMeta.Annotations[string(mconfig.CASConfigKey)] + klog.V(4).Infof("PVC %v has config:%v", pvc.Name, pvcCASConfigStr) + if len(strings.TrimSpace(pvcCASConfigStr)) != 0 { + pvcCASConfig, err := cast.UnMarshallToConfig(pvcCASConfigStr) + if err == nil { + pvConfig = cast.MergeConfig(pvcCASConfig, pvConfig) + } else { + return nil, errors.Wrapf(err, "failed to get config: invalid config {%v}"+ + " in pvc {%v} in namespace {%v}", + pvcCASConfigStr, pvc.Name, pvc.Namespace, + ) + } + } pvConfigMap, err := cast.ConfigToMap(pvConfig) if err != nil { @@ -342,6 +368,68 @@ func (c *VolumeConfig) IsExt4QuotaEnabled() bool { return enableExt4QuotaBool } +func (c *VolumeConfig) IsPermissionEnabled() bool { + permissionEnabled := c.getEnabled(KeyFilePermissions) + permissionEnabled = strings.TrimSpace(permissionEnabled) + + permissionEnabledQuotaBool, err := strconv.ParseBool(permissionEnabled) + //Default case + // this means that we have hit either of the two cases below: + // i. The value was something other than a straightforward + // true or false + // ii. The value was empty + if err != nil { + return false + } + + return permissionEnabledQuotaBool +} + +// GetFsGID fetches the group owner's ID from +// PVC annotation, if specified +// NOT YET USED +func (c *VolumeConfig) GetFsGID() string { + if c.IsPermissionEnabled() { + configData := c.getData(KeyFilePermissions) + if configData != nil { + if val, p := configData[KeyFsGID]; p { + return strings.TrimSpace(val) + } + } + } + return "" +} + +// GetFsGID fetches the user owner's ID from +// PVC annotation, if specified +// NOT YET USED +func (c *VolumeConfig) GetFsUID() string { + if c.IsPermissionEnabled() { + configData := c.getData(KeyFilePermissions) + if configData != nil { + if val, p := configData[KeyFsUID]; p { + return strings.TrimSpace(val) + } + } + } + return "" +} + +// GetFsMode fetches the file mode from PVC +// or StorageClass annotation, if specified +func (c *VolumeConfig) GetFsMode() string { + if c.IsPermissionEnabled() { + configData := c.getData(KeyFilePermissions) + if configData != nil { + if val, p := configData[KeyFsMode]; p { + return strings.TrimSpace(val) + } + } + } + //Keep the original default mode + return "0777" +} + // getValue is a utility function to extract the value // of the `key` from the ConfigMap object - which is // map[string]interface{map[string][string]} diff --git a/cmd/provisioner-localpv/app/provisioner_hostpath.go b/cmd/provisioner-localpv/app/provisioner_hostpath.go index c1b88af4..19cd4aa1 100644 --- a/cmd/provisioner-localpv/app/provisioner_hostpath.go +++ b/cmd/provisioner-localpv/app/provisioner_hostpath.go @@ -77,7 +77,7 @@ func (p *Provisioner) ProvisionHostPath(ctx context.Context, opts pvController.P klog.Infof("Creating volume %v at node with labels {%v}, path:%v,ImagePullSecrets:%v", name, nodeAffinityLabels, path, imagePullSecrets) //Before using the path for local PV, make sure it is created. - initCmdsForPath := []string{"mkdir", "-m", "0777", "-p"} + initCmdsForPath := []string{"mkdir", "-m", volumeConfig.GetFsMode(), "-p"} podOpts := &HelperPodOptions{ cmdsForPath: initCmdsForPath, name: name, diff --git a/docs/quickstart.md b/docs/quickstart.md index e0a4157b..ab37f7bc 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -2,7 +2,7 @@ ## Prerequisites -A Kubernetes cluster with Kubernetes v1.16 or above. +A Kubernetes cluster with Kubernetes v1.16 or above. For more platform-specific installation instructions, [click here](./installation/platforms/). @@ -13,12 +13,12 @@ Install OpenEBS Dynamic LocalPV Provisioner using the openebs helm chart. Sample #helm repo update helm install openebs openebs/openebs -n openebs --create-namespace ``` - +
Click here for configuration options. - 1. Install OpenEBS Dynamic LocalPV Provisioner without NDM. - + 1. Install OpenEBS Dynamic LocalPV Provisioner without NDM. + You may choose to exclude the NDM subchart from installation if... - you want to only use OpenEBS LocalPV Hostpath - you already have NDM installed. Check if NDM pods exist with the command `kubectl get pods -n openebs -l 'openebs.io/component-name in (ndm, ndm-operator)'` @@ -35,7 +35,7 @@ helm install openebs openebs/openebs -n openebs --create-namespace \ --set ndmOperator.enabled=false \ --set localprovisioner.deviceClass.enabled=false ``` - 3. Install OpenEBS Dynamic LocalPV Provisioner with a custom hostpath directory. + 3. Install OpenEBS Dynamic LocalPV Provisioner with a custom hostpath directory. This will change the `BasePath` value for the 'openebs-hostpath' StorageClass. ```console helm install openebs openebs/openebs -n openebs --create-namespace \ @@ -92,6 +92,11 @@ You can provision LocalPV hostpath StorageType volumes dynamically using the def # hostpath directory #- name: BasePath # value: "/var/openebs/local" + #Use this to set a specific mode for directory creation + #- name: FilePermissions + # enabled: true + # data: + # mode: "0770" provisioner: openebs.io/local reclaimPolicy: Delete #It is necessary to have volumeBindingMode as WaitForFirstConsumer diff --git a/docs/tutorials/hostpath/filepermissions.md b/docs/tutorials/hostpath/filepermissions.md new file mode 100644 index 00000000..74efe211 --- /dev/null +++ b/docs/tutorials/hostpath/filepermissions.md @@ -0,0 +1,54 @@ +# File permission tuning + +Hostpath LocalPV will by default create folder with the following rights: `0777`. In some usecases, these rights are too wide and should be reduced. +As an important point, when using hostpath the underlying PV will be a localpath whichs allows kubelet to chown the folder based on the [fsGroup](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods)) + +We allow to reduce this filepermission using : + +```yaml + #This is a custom StorageClass template + apiVersion: storage.k8s.io/v1 + kind: StorageClass + metadata: + name: custom-hostpath + annotations: + openebs.io/cas-type: local + cas.openebs.io/config: | + - name: StorageType + value: "hostpath" + - name: BasePath + value: "/var/openebs/local" + - name: FilePermissions + enabled: true + data: + mode: "0770" + provisioner: openebs.io/local + reclaimPolicy: Delete + #It is necessary to have volumeBindingMode as WaitForFirstConsumer + volumeBindingMode: WaitForFirstConsumer +``` + +With such configuration the folder will be crated with `0770` rights for all the PVC using this storage class. + +The same configuration is available at PVC level to have a more fined grained configuration capability (overrding the Storage class configuration level): + +```yaml +kind: PersistentVolumeClaim +apiVersion: v1 +metadata: + name: localpv-vol + annotations: + cas.openebs.io/config: | + - name: FilePermissions + enabled: true + data: + mode: "0770" +spec: + #Change this name if you are using a custom StorageClass + storageClassName: openebs-hostpath + accessModes: ["ReadWriteOnce"] + resources: + requests: + #Set capacity here + storage: 5Gi +``` From 0fae619b10607e656b78fb9647589c08ae9b6136 Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 10:40:41 +0100 Subject: [PATCH 2/7] Update cmd/provisioner-localpv/app/config.go Co-authored-by: Niladri Halder Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/provisioner-localpv/app/config.go b/cmd/provisioner-localpv/app/config.go index cf816bd5..b06454ec 100644 --- a/cmd/provisioner-localpv/app/config.go +++ b/cmd/provisioner-localpv/app/config.go @@ -197,7 +197,7 @@ func (p *Provisioner) GetVolumeConfig(ctx context.Context, pvName string, pvc *c if len(strings.TrimSpace(pvcCASConfigStr)) != 0 { pvcCASConfig, err := cast.UnMarshallToConfig(pvcCASConfigStr) if err == nil { - pvConfig = cast.MergeConfig(pvcCASConfig, pvConfig) + pvConfig = cast.MergeConfig(pvConfig, pvcCASConfig) } else { return nil, errors.Wrapf(err, "failed to get config: invalid config {%v}"+ " in pvc {%v} in namespace {%v}", From 876a7dbd1048c5f4b05da12c9fa1783c4d5b0ae5 Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 10:40:59 +0100 Subject: [PATCH 3/7] Update docs/tutorials/hostpath/filepermissions.md Co-authored-by: Niladri Halder Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- docs/tutorials/hostpath/filepermissions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/tutorials/hostpath/filepermissions.md b/docs/tutorials/hostpath/filepermissions.md index 74efe211..214adc05 100644 --- a/docs/tutorials/hostpath/filepermissions.md +++ b/docs/tutorials/hostpath/filepermissions.md @@ -3,7 +3,7 @@ Hostpath LocalPV will by default create folder with the following rights: `0777`. In some usecases, these rights are too wide and should be reduced. As an important point, when using hostpath the underlying PV will be a localpath whichs allows kubelet to chown the folder based on the [fsGroup](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods)) -We allow to reduce this filepermission using : +We allow to set file permissions using: ```yaml #This is a custom StorageClass template From aaea577d15fb1b38748c4efbd05d33f8caaae81d Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 10:41:41 +0100 Subject: [PATCH 4/7] Update cmd/provisioner-localpv/app/provisioner_hostpath.go Co-authored-by: Niladri Halder Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/provisioner_hostpath.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/cmd/provisioner-localpv/app/provisioner_hostpath.go b/cmd/provisioner-localpv/app/provisioner_hostpath.go index 19cd4aa1..a21d16bd 100644 --- a/cmd/provisioner-localpv/app/provisioner_hostpath.go +++ b/cmd/provisioner-localpv/app/provisioner_hostpath.go @@ -77,7 +77,12 @@ func (p *Provisioner) ProvisionHostPath(ctx context.Context, opts pvController.P klog.Infof("Creating volume %v at node with labels {%v}, path:%v,ImagePullSecrets:%v", name, nodeAffinityLabels, path, imagePullSecrets) //Before using the path for local PV, make sure it is created. - initCmdsForPath := []string{"mkdir", "-m", volumeConfig.GetFsMode(), "-p"} + fsMode := volumeConfig.GetFsMode() + // Set default value if FilePermissions mode is not specified. + if len(fsMode) == 0 { + fsMode = "0777" + } + initCmdsForPath := []string{"mkdir", "-m", fsMode, "-p"} podOpts := &HelperPodOptions{ cmdsForPath: initCmdsForPath, name: name, From af59c82b9d349d4f6c8fb036dff38a18eb185dd0 Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 10:47:27 +0100 Subject: [PATCH 5/7] Update cmd/provisioner-localpv/app/config.go Co-authored-by: Niladri Halder Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/provisioner-localpv/app/config.go b/cmd/provisioner-localpv/app/config.go index b06454ec..34a6b402 100644 --- a/cmd/provisioner-localpv/app/config.go +++ b/cmd/provisioner-localpv/app/config.go @@ -427,7 +427,7 @@ func (c *VolumeConfig) GetFsMode() string { } } //Keep the original default mode - return "0777" + return "" } // getValue is a utility function to extract the value From b5a53acad42e06f5a7b68178d3873c8c54c0b903 Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 10:51:23 +0100 Subject: [PATCH 6/7] review Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/config.go | 36 --------------------- cmd/provisioner-localpv/app/config_test.go | 37 ++++++++++++++++++++++ docs/tutorials/hostpath/filepermissions.md | 4 +-- 3 files changed, 39 insertions(+), 38 deletions(-) diff --git a/cmd/provisioner-localpv/app/config.go b/cmd/provisioner-localpv/app/config.go index 34a6b402..948bd153 100644 --- a/cmd/provisioner-localpv/app/config.go +++ b/cmd/provisioner-localpv/app/config.go @@ -142,12 +142,6 @@ const ( // This is the cas-template key for all file permission 'data' keys KeyFilePermissions = "FilePermissions" - // FsUID defines the user owner of the shared directory - KeyFsUID = "UID" - - // FsGID defines the group owner of the shared directory - KeyFsGID = "GID" - // FSMode defines the file permission mode of the shared directory KeyFsMode = "mode" ) @@ -385,36 +379,6 @@ func (c *VolumeConfig) IsPermissionEnabled() bool { return permissionEnabledQuotaBool } -// GetFsGID fetches the group owner's ID from -// PVC annotation, if specified -// NOT YET USED -func (c *VolumeConfig) GetFsGID() string { - if c.IsPermissionEnabled() { - configData := c.getData(KeyFilePermissions) - if configData != nil { - if val, p := configData[KeyFsGID]; p { - return strings.TrimSpace(val) - } - } - } - return "" -} - -// GetFsGID fetches the user owner's ID from -// PVC annotation, if specified -// NOT YET USED -func (c *VolumeConfig) GetFsUID() string { - if c.IsPermissionEnabled() { - configData := c.getData(KeyFilePermissions) - if configData != nil { - if val, p := configData[KeyFsUID]; p { - return strings.TrimSpace(val) - } - } - } - return "" -} - // GetFsMode fetches the file mode from PVC // or StorageClass annotation, if specified func (c *VolumeConfig) GetFsMode() string { diff --git a/cmd/provisioner-localpv/app/config_test.go b/cmd/provisioner-localpv/app/config_test.go index 477a3fcd..ee42937f 100644 --- a/cmd/provisioner-localpv/app/config_test.go +++ b/cmd/provisioner-localpv/app/config_test.go @@ -105,6 +105,43 @@ func TestDataConfigToMap(t *testing.T) { } } +func TestPermissionConfigToMap(t *testing.T) { + hostpathConfig := mconfig.Config{Name: "StorageType", Value: "hostpath"} + permissionConfig := mconfig.Config{Name: "FilePermissions", Enabled: "true", + Data: map[string]string{ + "mode": "0750", + }, + } + + testCases := map[string]struct { + config []mconfig.Config + expectedValue map[string]interface{} + }{ + "nil 'Data' map": { + config: []mconfig.Config{hostpathConfig, permissionConfig}, + expectedValue: map[string]interface{}{ + "FilePermissions": map[string]string{ + "mode": "0750", + }, + }, + }, + } + + for k, v := range testCases { + v := v + k := k + t.Run(k, func(t *testing.T) { + actualValue, err := dataConfigToMap(v.config) + if err != nil { + t.Errorf("expected error to be nil, but got %v", err) + } + if !reflect.DeepEqual(actualValue, v.expectedValue) { + t.Errorf("expected %v, but got %v", v.expectedValue, actualValue) + } + }) + } +} + func Test_listConfigToMap(t *testing.T) { tests := map[string]struct { pvConfig []mconfig.Config diff --git a/docs/tutorials/hostpath/filepermissions.md b/docs/tutorials/hostpath/filepermissions.md index 214adc05..1e9d1982 100644 --- a/docs/tutorials/hostpath/filepermissions.md +++ b/docs/tutorials/hostpath/filepermissions.md @@ -3,7 +3,7 @@ Hostpath LocalPV will by default create folder with the following rights: `0777`. In some usecases, these rights are too wide and should be reduced. As an important point, when using hostpath the underlying PV will be a localpath whichs allows kubelet to chown the folder based on the [fsGroup](https://kubernetes.io/docs/tasks/configure-pod-container/security-context/#configure-volume-permission-and-ownership-change-policy-for-pods)) -We allow to set file permissions using: +We allow to set file permissions using: ```yaml #This is a custom StorageClass template @@ -30,7 +30,7 @@ We allow to set file permissions using: With such configuration the folder will be crated with `0770` rights for all the PVC using this storage class. -The same configuration is available at PVC level to have a more fined grained configuration capability (overrding the Storage class configuration level): +The same configuration is available at PVC level to have a more fined grained configuration capability (the Storage class configuration will always win against PVC one): ```yaml kind: PersistentVolumeClaim From f19ed4d1572452b0df19533c149ad2b192205b2d Mon Sep 17 00:00:00 2001 From: sushiMix <53741704+sushiMix@users.noreply.github.com> Date: Thu, 4 Jan 2024 11:10:23 +0100 Subject: [PATCH 7/7] remove enabled flag Signed-off-by: sushiMix <53741704+sushiMix@users.noreply.github.com> --- cmd/provisioner-localpv/app/config.go | 28 ++++------------------ cmd/provisioner-localpv/app/config_test.go | 2 +- docs/quickstart.md | 1 - docs/tutorials/hostpath/filepermissions.md | 2 -- 4 files changed, 5 insertions(+), 28 deletions(-) diff --git a/cmd/provisioner-localpv/app/config.go b/cmd/provisioner-localpv/app/config.go index 948bd153..ebf2ee9d 100644 --- a/cmd/provisioner-localpv/app/config.go +++ b/cmd/provisioner-localpv/app/config.go @@ -134,7 +134,6 @@ const ( // FilePermissions allows to define the default directory mode // Exemple StorageClass snippet: // - name: FilePermissions - // enabled: true // data: // UID: 1000 // GID: 1000 @@ -362,32 +361,13 @@ func (c *VolumeConfig) IsExt4QuotaEnabled() bool { return enableExt4QuotaBool } -func (c *VolumeConfig) IsPermissionEnabled() bool { - permissionEnabled := c.getEnabled(KeyFilePermissions) - permissionEnabled = strings.TrimSpace(permissionEnabled) - - permissionEnabledQuotaBool, err := strconv.ParseBool(permissionEnabled) - //Default case - // this means that we have hit either of the two cases below: - // i. The value was something other than a straightforward - // true or false - // ii. The value was empty - if err != nil { - return false - } - - return permissionEnabledQuotaBool -} - // GetFsMode fetches the file mode from PVC // or StorageClass annotation, if specified func (c *VolumeConfig) GetFsMode() string { - if c.IsPermissionEnabled() { - configData := c.getData(KeyFilePermissions) - if configData != nil { - if val, p := configData[KeyFsMode]; p { - return strings.TrimSpace(val) - } + configData := c.getData(KeyFilePermissions) + if configData != nil { + if val, p := configData[KeyFsMode]; p { + return strings.TrimSpace(val) } } //Keep the original default mode diff --git a/cmd/provisioner-localpv/app/config_test.go b/cmd/provisioner-localpv/app/config_test.go index ee42937f..64ae37e1 100644 --- a/cmd/provisioner-localpv/app/config_test.go +++ b/cmd/provisioner-localpv/app/config_test.go @@ -107,7 +107,7 @@ func TestDataConfigToMap(t *testing.T) { func TestPermissionConfigToMap(t *testing.T) { hostpathConfig := mconfig.Config{Name: "StorageType", Value: "hostpath"} - permissionConfig := mconfig.Config{Name: "FilePermissions", Enabled: "true", + permissionConfig := mconfig.Config{Name: "FilePermissions", Data: map[string]string{ "mode": "0750", }, diff --git a/docs/quickstart.md b/docs/quickstart.md index ab37f7bc..1ecb5ee5 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -94,7 +94,6 @@ You can provision LocalPV hostpath StorageType volumes dynamically using the def # value: "/var/openebs/local" #Use this to set a specific mode for directory creation #- name: FilePermissions - # enabled: true # data: # mode: "0770" provisioner: openebs.io/local diff --git a/docs/tutorials/hostpath/filepermissions.md b/docs/tutorials/hostpath/filepermissions.md index 1e9d1982..c9e38aa7 100644 --- a/docs/tutorials/hostpath/filepermissions.md +++ b/docs/tutorials/hostpath/filepermissions.md @@ -19,7 +19,6 @@ We allow to set file permissions using: - name: BasePath value: "/var/openebs/local" - name: FilePermissions - enabled: true data: mode: "0770" provisioner: openebs.io/local @@ -40,7 +39,6 @@ metadata: annotations: cas.openebs.io/config: | - name: FilePermissions - enabled: true data: mode: "0770" spec: