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(values): add kubernetesRootDir #409

Merged

Conversation

eyenx
Copy link
Contributor

@eyenx eyenx commented Dec 30, 2020

What this PR does / why we need it:

Adds providersDir to the linux and windows values to be able to configure the hostPath mountpoint of the Daemonset.

Not all Kubernetes Distributions set their kubernetesRootDir under /etc/kubernetes (example: Nutanix Karbon).

Special notes for your reviewer:

First PR in this repo.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 30, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @eyenx!

It looks like this is your first PR to kubernetes-sigs/secrets-store-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/secrets-store-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 30, 2020
@k8s-ci-robot k8s-ci-robot requested review from ritazh and tam7t December 30, 2020 10:50
@eyenx
Copy link
Contributor Author

eyenx commented Jan 4, 2021

/assign @ritazh

@ritazh
Copy link
Member

ritazh commented Jan 4, 2021

Thanks for the PR @eyenx!
Can you please make these changes in the yamls in manifest_staging/charts where we host the staging charts? All the chart changes will then be promoted into the released charts folder with the next release. Thanks!

Copy link
Member

@aramase aramase 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 PR @eyenx.

  1. As @ritazh pointed out all the changes are required to be made in manifest_staging/charts folder.
  2. 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 and C:\k\secrets-store-csi-providers for Windows. I would love to hear what others think about this.
  3. Please also add the new configurable values to the configuration table

@eyenx eyenx force-pushed the feature/add-etc-kubernetes-path branch from dd9e0ee to d7dcd14 Compare January 5, 2021 07:17
@eyenx
Copy link
Contributor Author

eyenx commented Jan 5, 2021

@ritazh @aramase as requested, made my changes in the manifest_staging folder and switched to using providersDir instead of kubernetesRootDir and added the docs in the README.md

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

@eyenx
Copy link
Contributor Author

eyenx commented Jan 5, 2021

Those lines aren't really relevant for this change, as the mountPath is the path which is valid inside the pod: L103 And that can stay the same in my opinion.

@eyenx
Copy link
Contributor Author

eyenx commented Jan 5, 2021

a.e. the registration Volume's mountPath is also hardcoded: L38

@aramase
Copy link
Member

aramase commented Jan 5, 2021

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2021
@aramase
Copy link
Member

aramase commented Jan 5, 2021

#418 already opened for pull-secrets-store-csi-driver-e2e-gcp failures.

@tam7t
Copy link
Contributor

tam7t commented Jan 5, 2021

I think this needs to become a requirement for supported providers actually. Currently the GCP provider install has hardcoded providervol set to /etc/kubernetes/secrets-store-csi-providers.

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

@aramase
Copy link
Member

aramase commented Jan 5, 2021

I think this needs to become a requirement for supported providers actually. Currently the GCP provider install has hardcoded providervol set to /etc/kubernetes/secrets-store-csi-providers.

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.

@aramase
Copy link
Member

aramase commented Jan 6, 2021

/test pull-secrets-store-csi-driver-e2e-gcp

@tam7t
Copy link
Contributor

tam7t commented Jan 7, 2021

@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.

Copy link
Member

@aramase aramase left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021
@ritazh
Copy link
Member

ritazh commented Jan 8, 2021

/approve

@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit e2108be into kubernetes-sigs:master Jan 8, 2021
@eyenx eyenx deleted the feature/add-etc-kubernetes-path branch January 9, 2021 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants