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(csi/providersDir): add configurable providersDir #603

Merged

Conversation

eyenx
Copy link
Contributor

@eyenx eyenx commented Aug 26, 2021

  • Adds a configurable providersDir path for the Daemonset

Background: with kubernetes-sigs/secrets-store-csi-driver#409 the providersDir path has been made configurable for secrets-store-csi-driver, as not every single kubernetes distribution out there uses the same path.

This is why we should also allow vault-helm to set providersDir accordingly. Default will remain /etc/kubernetes/secrets-store-csi-providers

Signed-off-by: Toni Tauro toni.tauro@adfinis.com

Signed-off-by: Toni Tauro <toni.tauro@adfinis.com>
@eyenx
Copy link
Contributor Author

eyenx commented Sep 6, 2021

@jasonodonnell Can you please review this PR?

We can't upgrade the vault integration on a cluster due to it being using differente providersDir paths.

@benashz benashz self-assigned this Sep 10, 2021
@benashz benashz self-requested a review September 10, 2021 14:56
Chart.yaml Outdated Show resolved Hide resolved
values.yaml Show resolved Hide resolved
eyenx and others added 3 commits September 14, 2021 21:16
Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
Signed-off-by: Toni Tauro <toni.tauro@adfinis.com>
Signed-off-by: Toni Tauro <toni.tauro@adfinis.com>
@eyenx
Copy link
Contributor Author

eyenx commented Sep 15, 2021

Thanks for the review

Please re-review @benashz

@eyenx eyenx requested a review from benashz September 15, 2021 15:09
@benashz
Copy link
Contributor

benashz commented Sep 15, 2021

Thanks @eyenx . I think all that is missing now are some unit tests. I think the following patch should suffice:

diff --git a/test/unit/csi-daemonset.bats b/test/unit/csi-daemonset.bats
index c546d0a..5cfd8a7 100644
--- a/test/unit/csi-daemonset.bats
+++ b/test/unit/csi-daemonset.bats
@@ -315,6 +315,68 @@ load _helpers
   [ "${actual}" = "{}" ]
 }
 
+@test "csi/daemonset: csi providersDir default" {
+  cd `chart_dir`
+
+  # Test that it defines it
+  local object=$(helm template \
+      --show-only templates/csi-daemonset.yaml  \
+      --set 'csi.enabled=true' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.volumes[] | select(.name == "providervol")' | tee /dev/stderr)
+
+  local actual=$(echo $object |
+      yq -r '.hostPath.path' | tee /dev/stderr)
+  [ "${actual}" = "/etc/kubernetes/secrets-store-csi-providers" ]
+}
+
+@test "csi/daemonset: csi kubeletRootDir default" {
+  cd `chart_dir`
+
+  # Test that it defines it
+  local object=$(helm template \
+      --show-only templates/csi-daemonset.yaml  \
+      --set 'csi.enabled=true' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.volumes[] | select(.name == "mountpoint-dir")' | tee /dev/stderr)
+
+  local actual=$(echo $object |
+      yq -r '.hostPath.path' | tee /dev/stderr)
+  [ "${actual}" = "/var/lib/kubelet/pods" ]
+}
+
+@test "csi/daemonset: csi providersDir override " {
+  cd `chart_dir`
+
+  # Test that it defines it
+  local object=$(helm template \
+      --show-only templates/csi-daemonset.yaml  \
+      --set 'csi.enabled=true' \
+      --set 'csi.daemonSet.providersDir=/alt/csi-prov-dir' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.volumes[] | select(.name == "providervol")' | tee /dev/stderr)
+
+  local actual=$(echo $object |
+      yq -r '.hostPath.path' | tee /dev/stderr)
+  [ "${actual}" = "/alt/csi-prov-dir" ]
+}
+
+@test "csi/daemonset: csi kubeletRootDir override" {
+  cd `chart_dir`
+
+  # Test that it defines it
+  local object=$(helm template \
+      --show-only templates/csi-daemonset.yaml  \
+      --set 'csi.enabled=true' \
+      --set 'csi.daemonSet.kubeletRootDir=/alt/kubelet-root' \
+      . | tee /dev/stderr |
+      yq -r '.spec.template.spec.volumes[] | select(.name == "mountpoint-dir")' | tee /dev/stderr)
+
+  local actual=$(echo $object |
+      yq -r '.hostPath.path' | tee /dev/stderr)
+  [ "${actual}" = "/alt/kubelet-root/pods" ]
+}
+
 #--------------------------------------------------------------------
 # volumeMounts
$ make test-unit UNIT_TESTS_FILTER='.+csi.+Dir.+' 
1..4
ok 1 csi/daemonset: csi providersDir default
ok 2 csi/daemonset: csi kubeletRootDir default
ok 3 csi/daemonset: csi providersDir override 
ok 4 csi/daemonset: csi kubeletRootDir override

Signed-off-by: Toni Tauro <toni.tauro@adfinis.com>
@eyenx
Copy link
Contributor Author

eyenx commented Sep 15, 2021

thanks @benashz, added your suggested changes in b2af383

Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

LGTM!

Thank you for your contribution to HashiCorp!

@eyenx
Copy link
Contributor Author

eyenx commented Sep 15, 2021

My pleasure

@benashz benashz merged commit 23e0348 into hashicorp:master Sep 15, 2021
@tvoran tvoran mentioned this pull request Sep 16, 2021
illegalnumbers pushed a commit to streamnative/vault-helm that referenced this pull request Mar 17, 2022
*  add configurable values for providersDir and kubeletRootDir

Signed-off-by: Toni Tauro <toni.tauro@adfinis.com>

Co-authored-by: Ben Ash <32777270+benashz@users.noreply.github.com>
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.

2 participants