-
Notifications
You must be signed in to change notification settings - Fork 302
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(values): add kubernetesRootDir #409
feat(values): add kubernetesRootDir #409
Conversation
Welcome @eyenx! |
Hi @eyenx. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @ritazh |
Thanks for the PR @eyenx! |
There was a problem hiding this 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 PR @eyenx.
- As @ritazh pointed out all the changes are required to be made in manifest_staging/charts folder.
- I think it would be better to have the whole providers-dir configurable instead of just the kubernetes dir. So we can add
providersDir
and default is/etc/kubernetes/secrets-store-csi-providers
for Linux andC:\k\secrets-store-csi-providers
for Windows. I would love to hear what others think about this. - Please also add the new configurable values to the configuration table
Signed-off-by: Toni Tauro <[email protected]>
dd9e0ee
to
d7dcd14
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eyenx Could you also update the values here:
- https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/master/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver.yaml#L55
- https://github.com/kubernetes-sigs/secrets-store-csi-driver/blob/master/manifest_staging/charts/secrets-store-csi-driver/templates/secrets-store-csi-driver-windows.yaml#L53
Those lines aren't really relevant for this change, as the |
a.e. the registration Volume's |
/ok-to-test |
#418 already opened for |
I think this needs to become a requirement for supported providers actually. Currently the GCP provider install has hardcoded And I think the azure plugin would need to be updated to accept this configuration here: https://github.com/Azure/secrets-store-csi-driver-provider-azure/blob/50aaa7b66843a2d58bc4895dacc01000d5b541d3/manifest_staging/charts/csi-secrets-store-provider-azure/templates/provider-azure-installer.yaml#L60-L63 |
It's the same for all providers. But I think we should add a note before next release that if this value is changed, then also make sure to update the volume in the provider manifests. WDYT? I think with helm if the driver is packaged as part of provider, then it'll be handled. |
/test pull-secrets-store-csi-driver-e2e-gcp |
@aramase ah that sounds good to me, realizing now that it is just a deploy configuration yaml parameter and not something that provider code implementations really need to be aware of. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eyenx, ritazh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
Adds
providersDir
to thelinux
andwindows
values to be able to configure thehostPath
mountpoint of theDaemonset
.Not all Kubernetes Distributions set their
kubernetesRootDir
under/etc/kubernetes
(example: Nutanix Karbon).Special notes for your reviewer:
First PR in this repo.