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.

Sign up for GitHub

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

Jump to bottom

feat(chart): add "encrypted" and custom storageclass parameters #9007

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(chart): add "encrypted" and custom storageclass parameters #9007

wants to merge 1 commit into from

Conversation

fllaca
Copy link

@fllaca fllaca commented Jul 15, 2024 •
edited
Loading

Allow setting the encrypted parameter plus any custom parameter in the Longhorn default StorageClass, to allow volume encryption as documented in Setting up Kubernetes Secrets and StorageClasses. Example Helm values file:

persistence:
  encrypted: "true"
  extraParameters:
    # custom storage class settings
    csi.storage.k8s.io/provisioner-secret-name: "longhorn-crypto"
    csi.storage.k8s.io/provisioner-secret-namespace: "longhorn-system"
    csi.storage.k8s.io/node-publish-secret-name: "longhorn-crypto"
    csi.storage.k8s.io/node-publish-secret-namespace: "longhorn-system"
    csi.storage.k8s.io/node-stage-secret-name: "longhorn-crypto"
    csi.storage.k8s.io/node-stage-secret-namespace: "longhorn-system"

@fllaca fllaca requested a review from a team as a code owner July 15, 2024 17:56
@derekbit
Copy link
Member

@fllaca
Thanks for your contribution.
I think we need to add the missing parameters such as encrypted rather than extraParameters.

@derekbit
Copy link
Member

derekbit commented Jul 16, 2024 •
edited
Loading

@mantissahz Can you help follow up with the issue? Thank you.

@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch from 354d178 to f0d49bb Compare July 16, 2024 16:08
@fllaca fllaca changed the title feat(chart): add custom storageclass parameters feat(chart): add "encrypted" and custom storageclass parameters Jul 16, 2024
@fllaca
Copy link
Author

fllaca commented Jul 16, 2024

hi @derekbit @mantissahz , thank you for your review! I've added the persistence.encrypted parameter. I've kept persistence.extraParameters though for the csi.storage.k8s.io/* parameters, to make the chart flexible on how to set them (I couldn't think on a better interface for them, please let me know any idea). The extraParameters field also gives flexibility to support other usecases without requiring modifications to the chart.

derekbit
derekbit reviewed Jul 17, 2024
View reviewed changes
chart/README.md Outdated Show resolved Hide resolved
@derekbit derekbit requested a review from mantissahz July 18, 2024 07:39
@mantissahz
Copy link
Contributor

Hi @fllaca,
Could you elaborate on why you removed the items like defaultDiskSelector?

@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch from f0d49bb to 22f1370 Compare July 18, 2024 08:15
@fllaca
Copy link
Author

fllaca commented Jul 18, 2024

Hi @fllaca, Could you elaborate on why you removed the items like defaultDiskSelector?

@mantissahz Fixed! I unavertedly removed those lines while rearranging the order of the new parameters I was introducing. Thanks!

@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch 2 times, most recently from dde0904 to 2d5ebf2 Compare July 20, 2024 15:22
@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch from 2d5ebf2 to 4e165dd Compare August 10, 2024 10:26
@fllaca
Copy link
Author

fllaca commented Aug 10, 2024 •
edited
Loading

hi @derekbit @mantissahz , just following up 🙏 , trying to get this PR over the finish line if you think this little feature in the helm chart is worth it. thanks!!

@mantissahz mantissahz requested a review from derekbit August 12, 2024 03:50
innobead
innobead requested changes Aug 18, 2024
View reviewed changes
chart/templates/storageclass.yaml
@@ -54,3 +54,9 @@ data:
{{- if .Values.persistence.disableRevisionCounter }}
disableRevisionCounter: "{{ .Values.persistence.disableRevisionCounter }}"
{{- end }}
{{- if .Values.persistence.encrypted }}
Copy link
Member

Choose a reason for hiding this comment

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

Suggest below making the variables self-explanatory.

      {{- if .Values.persistence.encrypted }}
      encrypted: {{ .Values.persistence.encrypted }}
      {{- if .Values.persistence.encryptionParameters }}
      {{ .Values.persistence.encryptionParameters | toYaml | nindent 6 }}
      {{- end }}
      {{- end }}

Copy link
Author

Choose a reason for hiding this comment

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

Done! thanks!

@innobead
Copy link
Member

@longhorn/doc needs to review the README before merging as usual.

cc @derekbit

@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch 2 times, most recently from e474f89 to 562afd4 Compare August 24, 2024 10:16
@fllaca
feat(chart): add encrypted and encryptionParameters to storagecla…
429cdc8 
…ss options

Signed-off-by: Fernando Llaca <ferllarom@gmail.com>
@fllaca fllaca force-pushed the feat/custom-storageclass-parameters branch from 562afd4 to 429cdc8 Compare August 24, 2024 10:20
@mantissahz mantissahz requested a review from innobead August 26, 2024 01:48
mantissahz
mantissahz approved these changes Aug 26, 2024
View reviewed changes
Copy link
Contributor

@mantissahz mantissahz left a comment

Choose a reason for hiding this comment

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

LGTM, @fllaca Could you please solve the pending conversations?

And README.md needs to be reviewed, cc @longhorn/doc.

chart/templates/storageclass.yaml Outdated Show resolved Hide resolved
chart/templates/storageclass.yaml Show resolved Hide resolved
chart/values.yaml Outdated
@@ -172,6 +172,10 @@ persistence:
selector: ""
# -- Setting that allows you to enable automatic snapshot removal during filesystem trim for a Longhorn StorageClass. (Options: "ignored", "enabled", "disabled")
removeSnapshotsDuringFilesystemTrim: ignored
# -- Setting that allows you to specify extra parameters for the default Longhorn StorageClass.
# It must be a map of string to string. (Example: `{"encrypted": "true" }`)
extraParameters: {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @fllaca,
If this extraParameters is only used for the encrypted, can we change it to an independent part like backingImage

persistence:
  encrypted:
    enabled: false
    csiParmeters: {}
    ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this naming acceptable to you, @fllaca?

chart/README.md Outdated Show resolved Hide resolved
@mergify Mergify
Copy link

mergify bot commented Sep 6, 2024

This pull request is now in conflict. Could you fix it @fllaca? 🙏

@derekbit
Copy link
Member

Hello @fllaca
Will you proceed with the PR?

@mantissahz mantissahz mentioned this pull request Oct 15, 2024
Open
@mergify Mergify
Copy link

mergify bot commented Dec 14, 2024

This pull request is now in conflict. Could you fix it @fllaca? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantissahz mantissahz mantissahz approved these changes

@derekbit derekbit Awaiting requested review from derekbit

@innobead innobead Awaiting requested review from innobead

Requested changes must be addressed to merge this pull request.

Assignees

@fllaca fllaca

Labels
None yet
Projects
Longhorn Sprint
Status: New Issues
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fllaca @derekbit @mantissahz @innobead