Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add support to specify file permissions for pvc hostpaths #157

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

sushiMix
Copy link

@sushiMix sushiMix commented May 11, 2023

Pull Request template

Why is this PR required? What issue does it fix?:
Fixes #95.

What this PR does?:
It allows to configure default directory mode using LocaMode entry in cas.openebs.io/config.

Does this PR require any upgrade changes?: No

If the changes in this PR are manually verified, list down the scenarios covered::

apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
  annotations:
    cas.openebs.io/config: |
      - name: StorageType
        value: "hostpath"
      - name: BasePath
        value: "/opt/data/"
      - name: HostPathMode
        value: "770"
    openebs.io/cas-type: local
  name: local-path
provisioner: openebs.io/local
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer
---
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
  name: task-pv-claim
spec:
  storageClassName: local-path
  accessModes:
    - ReadWriteOnce
  resources:
    requests:
      storage: 1Gi
---
apiVersion: v1
kind: Pod
metadata:
  name: task-pv-pod
spec:
  volumes:
    - name: task-pv-storage
      persistentVolumeClaim:
        claimName: task-pv-claim
  containers:
    - name: task-pv-container
      image: nginx
      ports:
        - containerPort: 80
          name: "http-server"
      volumeMounts:
        - mountPath: "/usr/share/nginx/html"
          name: task-pv-storage

check the file mode of the created folder (0770 instead of 0777)

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

PLEASE REMOVE BELOW INFORMATION BEFORE SUBMITTING

IMPORTANT: Please review the CONTRIBUTING.md file for detailed contributing guidelines.

@sushiMix
Copy link
Author

sushiMix commented May 11, 2023

Sorry for last PR with a bad rebase when signing.

Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @sushiMix. There is already one file-mode/file-mode API in place over at the openebs/dynamic-nfs-provisioner repo.
PR link: openebs-archive/dynamic-nfs-provisioner#125

While I don't have any issues with this implementation, it'd be useful to have a stable cas-config API across openebs storage engines. Also, the above implemetation supports file-permissions as well, which may be useful to your use-case.

@sushiMix sushiMix force-pushed the develop branch 4 times, most recently from ef56055 to 584d3e1 Compare October 5, 2023 14:07
@sushiMix
Copy link
Author

sushiMix commented Oct 5, 2023

Hello @niladrih I did a test with storage class and then pvc annotation configuration.

After I did rebase and squashed the commits.

I only support the mode changed. The user and group are left to kubelet behavior (FsGroup is supported for local volume in kubernetes). Need a bit more code change to update the command sent.

@niladrih
Copy link
Member

niladrih commented Oct 5, 2023

Great! Could you also add a documentation .md file in the docs directory with instructions on using this feature?

@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2023

Codecov Report

Merging #157 (6e61d78) into develop (a4a9273) will decrease coverage by 0.40%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           develop     #157      +/-   ##
===========================================
- Coverage    38.00%   37.61%   -0.40%     
===========================================
  Files           36       36              
  Lines         3365     3400      +35     
===========================================
  Hits          1279     1279              
- Misses        2004     2039      +35     
  Partials        82       82              
Files Coverage Δ
...md/provisioner-localpv/app/provisioner_hostpath.go 0.00% <0.00%> (ø)
cmd/provisioner-localpv/app/config.go 13.69% <0.00%> (-3.61%) ⬇️

@sushiMix
Copy link
Author

sushiMix commented Oct 5, 2023

Great! Could you also add a documentation .md file in the docs directory with instructions on using this feature?

I updated the QuickStart, do you need a more detailed documentation?

@niladrih I added a specific page.

Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the changes @sushiMix! I have left some comments.

cmd/provisioner-localpv/app/config.go Outdated Show resolved Hide resolved
cmd/provisioner-localpv/app/config.go Outdated Show resolved Hide resolved
cmd/provisioner-localpv/app/config.go Outdated Show resolved Hide resolved
cmd/provisioner-localpv/app/config.go Outdated Show resolved Hide resolved
cmd/provisioner-localpv/app/provisioner_hostpath.go Outdated Show resolved Hide resolved
docs/tutorials/hostpath/filepermissions.md Outdated Show resolved Hide resolved
docs/tutorials/hostpath/filepermissions.md Outdated Show resolved Hide resolved
Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, the enabled key is not required here. Mentioning the details in 'data' is good enough.

@sushiMix sushiMix force-pushed the develop branch 3 times, most recently from 86246dc to 815a205 Compare January 4, 2024 10:02
@sushiMix
Copy link
Author

sushiMix commented Jan 4, 2024

@niladrih Happy New Year, I had time to perform code modification.

sushiMix and others added 7 commits February 6, 2024 16:41
Co-authored-by: Niladri Halder <[email protected]>
Signed-off-by: sushiMix <[email protected]>
Co-authored-by: Niladri Halder <[email protected]>
Signed-off-by: sushiMix <[email protected]>
Co-authored-by: Niladri Halder <[email protected]>
Signed-off-by: sushiMix <[email protected]>
Signed-off-by: sushiMix <[email protected]>
Signed-off-by: sushiMix <[email protected]>
@niladrih
Copy link
Member

niladrih commented Jun 5, 2024

Hi @sushiMix. Could you add ginkgo tests to this PR to test your feature? You could use https://github.com/openebs-archive/dynamic-nfs-provisioner/blob/develop/tests/nfs_server_file_permissions_test.go as an example in this case.

Let me know if you need help with this.

Also, I am sorry I haven't been able to review this!

@avishnu avishnu added this to the v4.2 milestone Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support to specify file permissions for pvc hostpaths
4 participants