Cleanup and refactor helm chart

[AtkinsChang]

What is this PR about? / Why do we need it?

base on #1013

• update RBAC from upstream
• unify style (ex. default "foo" .Value.foo vs .Value.foo | default "foo")
• unify socket-dir mount path

Controller

• add options:
  • controller.podSecurityContext
  • controller.securityContext
• use system-cluster-critical as default value of priorityClassName
• prevent from deploying csi-snapshotter related resources if not supported

Node

• add options:
  • node.podSecurityContext
  • node.securityContext
  • node.additionalArgs (this option is already in controller)
• set ImagePullPolicy according to image.pullPolicy

Sidecar

• add sidecars.<name>.image.pullPolicy option for images

@krmichel is it ok to rename any resource under deploy/kubernetes?

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

[k8s-ci-robot]

Welcome @AtkinsChang!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-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/aws-ebs-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]

Hi @AtkinsChang. 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]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: AtkinsChang
To complete the pull request process, please assign leakingtapan after the PR has been reviewed.
You can assign the PR to them by writing /assign @leakingtapan in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[AtkinsChang]

I signed it

[krmichel]

I am not very familiar with kustomize but I believe the files can be renamed as long as the kustomization.yaml is updated correctly to reflect the changes. The makefile for this project has a target called generate-kustomize that generates the files in the deploy/kubernetes/base directory and should be run when changes to the chart have been made. If the kustomize files are going to be renamed then the make target needs to be updated to reflect that. This project would like the kustomize files to match names with the helm templates that create them. I don't know that this chart is big enough to warrant splitting the templates into sub-directories, but am also not opposed to it. @wongma7 do you have any thoughts on organizing the templates into sub-directories by function? If we are going to do that I would like to see the moving/organizing of the templates done as its own PR and to ensure that the files all show as moved in git. The combination of moving files around, renaming a lot of the resources, and the new editions is, in my opinion, too much to deal with in a single PR. I also don't know a whole lot about the driver itself. @wongma7, @ayberk, @AndyXiangLi, or @vdhanan do any of you know if the RBAC should be synced as indicated with the comments in the changes like this should be sync from upstream? Instead of truncating the name and fullname named templates at 40 characters, which is based on the length of fairly arbitrary strings, I think it would be better to have a suffix name helper that combined the name/fullname with a provided suffix and then truncates based on a specific suffix. This could either be done by adding the suffix and then truncating or by doing some math based on suffix length and truncating the name before adding the suffix. Since it seems like the suffix is the more descriptive part, in this case, doing the latter is probably the better option. I just don't want to truncate information when we don't need to.

Please, unless one of the owners says that they don't want the templates reorganized, split this into two PRs. One for the template rename/reorg and another for the resource renaming/adding additional functionality/image version updates.

[AtkinsChang]

@krmichel
maybe I should refine my question more accurately:
should the kustomize related files in deploy/kubernetes/base keep the compatibility? if not then just update the Makefile, if yes then can be handled via {{- if eq .Release.Name "kustomize" }}

do any of you know if the RBAC should be synced as indicated with the comments in the changes like this should be sync from upstream?

Some RBAC are used by the external-attacher, external-provisioner, external-resizer, external-snapshotter.
For example, kubernetes-csi/external-attacher#146 update the deprecated csinodeinfos but clusterrole-attacher.yaml is not update. It is lucky because some people update clusterrole-provisioner.yaml so the external-attacher sidecar JUST WORKS.
I think we should leave these untouched and keep it sync when the sidecar version is updated.
Put the part related to this plugin to another ClusterRole (for example nodes API introduce in #907)

I known this is a big PR and will split this. But I'd like to hear which to do and which not so I open a draft PR. Thanks for advice.

[krmichel]

The majority of what you are adding as new options has no default values and since you are using with blocks I don't think you will need any if to prevent them from doing anything undesirable in the kustomize manifests. You will need to make sure that the templates that you are adding API checks to have --api-versions 'snapshot.storage.k8s.io/v1' added in the makefile to ensure the manifests are generated correctly. Let me know if you need anything else before splitting this PR.

[krmichel]

This was set to always before so that is probably what I would set the default to.

[AtkinsChang]

I think Always is wrong value with fixed image version, and taking the risk of Docker Hub API rate limiting

[krmichel]

I don't believe that k8s.gcr.io is docker hub. I think it is google cloud. Also, I believe, if there is already a local copy of the image and the digest for the image hasn't changed it won't pull but instead reuse the local cached images since it hasn't changed.

[AtkinsChang]

Ok if you think it is reasonable to have csi-resizer using Always as default image pull policy and all other containers using IfNotPresent then I will do it

[krmichel]

Are these paths different on different nodes? Should this be configurable?

[AtkinsChang]

This can be set to any path, but this and the volume mount path should be same.

[krmichel]

What I mean is that on one windows system could this be a different path than on another? Should the user be able to specify/override this value?

[krmichel]

I would split the PR before making any more updates.

[AtkinsChang]

#1013 is what you talk about

[krmichel]

#1013 is what you talk about

Sort of. I was wanting to split the renaming of the resources inside of the templates from moving/renaming the templates files themselves. Doing them together, based on git figuring out what is a rename based on content similarity, causes some of the files to appear to be deleted and then new files added since the resource renaming, inside the templates, causes them to no longer be similar enough to be a rename. The first PR would just be moving/renaming files and then fixing the paths in the Makefile to ensure git tracks all the renames.

[k8s-ci-robot]

@AtkinsChang: PR needs rebase.

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-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale