Add psp rbac helm #279

cnellis101 · 2020-11-12T20:27:01Z

Is this a bug fix or adding new feature?
New feature.

What is this PR about? / Why do we need it?
This was needed in my setup when our cluster required ServiceAccounts and a PodSecurityPolicy to allow access due to our restricted default PSP.

Added:

Pod security policy which grants the appropriate permissions needed by the container such as root filesystem access, volumes, and host ports.
ServiceAccount to tie the PSP and the Role.
RBAC with a Role for accessing the PSP and a RoleBinding for the Role, PSP, and SA.

Configuration:

I've left RBAC, PSP, and SA creation disabled for backwards compatibility. To use these kinds, you must enable them in values.yaml.

What testing is done?
Currently using in our own cluster.

$ helm upgrade --install aws-efs-csi-driver -f values.yaml . --tiller-namespace kube-system --namespace kube-system
Release "aws-efs-csi-driver" has been upgraded.
LAST DEPLOYED: Thu Nov 12 15:23:32 2020
NAMESPACE: kube-system
STATUS: DEPLOYED

RESOURCES:
==> v1/DaemonSet
NAME          DESIRED  CURRENT  READY  UP-TO-DATE  AVAILABLE  NODE SELECTOR  AGE
efs-csi-node  3        3        3      3           3          <none>         3h19m

==> v1/Pod(related)
NAME                READY  STATUS   RESTARTS  AGE
efs-csi-node-579f7  3/3    Running  0         3h19m
efs-csi-node-dzvrp  3/3    Running  0         3h19m
efs-csi-node-shdc5  3/3    Running  0         3h19m

==> v1/Role
NAME                AGE
aws-efs-csi-driver  3h19m

==> v1/RoleBinding
NAME                AGE
aws-efs-csi-driver  3h19m

==> v1/ServiceAccount
NAME                SECRETS  AGE
aws-efs-csi-driver  1        3h19m

==> v1beta1/PodSecurityPolicy
NAME                PRIV  CAPS      SELINUX   RUNASUSER  FSGROUP   SUPGROUP  READONLYROOTFS  VOLUMES
aws-efs-csi-driver  true  RunAsAny  RunAsAny  RunAsAny   RunAsAny  false     hostPath,secret


NOTES:
To verify that aws-efs-csi-driver has started, run:

    kubectl get pod -n kube-system -l "app.kubernetes.io/name=aws-efs-csi-driver,app.kubernetes.io/instance=aws-efs-csi-driver"

k8s-ci-robot · 2020-11-12T20:27:05Z

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 · 2020-11-12T20:27:09Z

Welcome @quizy101!

It looks like this is your first PR to kubernetes-sigs/aws-efs-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-efs-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 · 2020-11-12T20:27:09Z

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

dan-vaughan · 2020-12-07T15:40:07Z

Hi all, what's the status of this PR?

Skaronator · 2020-12-11T08:17:51Z

helm/templates/serviceaccount.yaml

+{{- if .Values.serviceAccount.create -}}
+apiVersion: v1
+kind: ServiceAccount
+metadata:


namespace seems to be missing?

Ah actually no. Helm just includes it if the value is empty.

wongma7 · 2020-12-14T21:30:34Z

/ok-to-test

cnellis101 · 2020-12-14T23:25:38Z

/retest

nxf5025 · 2020-12-18T15:57:28Z

@quizy101 - Any objection to pulling in the changes from this PR? - #288

wongma7 · 2020-12-22T19:06:51Z

/assign

k8s-ci-robot · 2020-12-22T20:30:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: quizy101
To complete the pull request process, please assign wongma7 after the PR has been reviewed.
You can assign the PR to them by writing /assign @wongma7 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

helm/templates/daemonset.yaml

krmichel · 2021-01-01T23:32:35Z

helm/Chart.yaml

 description: A Helm chart for AWS EFS CSI Driver
-version: 0.3.0
+name: aws-efs-csi-driver
+version: 0.3.1


Since values that used to work in the templates are no longer being used the version should go to 0.4.0 if the project isn't ready to move to 1.0.0

I agree on changing to 0.4.0, it is a lot of changes, I don't think I can make the call to 1.0.0.

krmichel · 2021-01-01T23:38:12Z

helm/templates/_helpers.tpl

+Create the name of the service account to use
+*/}}
+{{- define "aws-efs-csi-driver.serviceAccountName" -}}
+{{- if .Values.serviceAccount.create -}}


I believe this could just be default (ternary .Values.serviceAccount.create (include "aws-efs-csi-driver.fullname" .) "default") .Values.serviceAccount.name but I haven't tested it

Looks like ternary is a helm3 supported function but not in helm2. Since the chart api version is still set to v1, it would cause some issues to change the chart to be only helm3 supported. As it is right now, the chart is compatible with helm2 and helm3.

Helm 2 is no longer supported, even for security patches. Helm is strongly recommending migrating charts to helm 3 might be worth considering. If the owners weigh in on the namespace part maybe they would have an opinion about switching to only supporting helm 3

I'm conflicted on how to proceed between helm2 and helm3. There are several parts that I am trying to address from the original chart that definitely help those that are still on helm2. I would propose that with this PR we get it to a stable helm2 supported version and maybe as a v1 the switch to helm3 happens going forward?

I'm open to whichever the maintainers would like to go with this but my original need was to get PSP and RBAC enabled on the chart. I'm also ok with doing the helm3 cutover PR later.

I do not think it is important to continue supporting helm 2. I agree with you @quizy101 that it can be left as a follow-up PR/discussion though: if you don't need to take advantage of helm 3 features to achieve the goals of this PR then let's leave it for later.

If I understand correctly, we'll simply need to flip chart version to v2 to signal to users that our chart will only support helm v3 from then on.

To be totally clear, since #292 just merged, there is only one chart version in existence: 0.1.0. I want to next release a 0.2.0 with various fixes over the last few monnthsp, including this one if we can get it merged soon. Then 0.3.0 or 1.0.0 could become the helm3 cutoff.

krmichel · 2021-01-02T00:17:57Z

helm/templates/daemonset.yaml

@@ -3,7 +3,9 @@ kind: DaemonSet
 apiVersion: apps/v1
 metadata:
  name: efs-csi-node
-  namespace: kube-system
+  namespace: {{ .Values.namespace }}


I am not sure about this project but in general you either hard code a namespace because it is required to run in a specific namespace or you leave it out completely and allow the user to decide which namespace is goes in based on the namespace flag with the helm command. I would generally not expect to have to set a value to control the namespace. Maybe an owner would weigh in on whether to leave it hardcoded or remove it in favor of typical helm behavior.

@d-nishi @leakingtapan @justinsb @jsafrane @wongma7 @nckturner @jqmichael

I had trouble with this, originally I had removed the hardcoded namespace because I felt that it would be up to the individual who installed the driver where they would like it to do. The problem is that once I was OK to test, the tests assume the chart is installed in kube-system, so without a default namespace set, it installs in the default namespace and the tests fail. Wasn't sure what to do so I went with a middle approach.

I haven't looked at these tests much but I think at least this helm install would need to change to have --namespace kube-system added to it if the hard coded namespace were removed.

aws-efs-csi-driver/tester/e2e-test-config.yaml

Line 30 in eed29e4

helm install aws-efs-csi-driver \

Thanks for pointing that out, I changed the install to include the namespace and went back to my original commit with using the namespace from the release. e2e works perfectly now and this is the behavior that I would expect.

it seems subject to a lot of debate. helm/helm#5465 I don't mind whether we hardcode the namespace or do this Release.namespace thing, as long as the helm install in the test script AND the README work.

helm/templates/daemonset.yaml

krmichel · 2021-01-02T22:24:47Z

helm/templates/daemonset.yaml

      tolerations:
-        - operator: Exists


Sorry, I didn't notice this before but with this being a daemonset it should probably have a default set of tolerations that will work in many cases. This toleration is one that tolerates any taint that exists. In the ebs project they kept this as a default with a boolean value to turn it on an off.

I see the relevant PRs for that. I give way too much credit to the admin for how they want to configure their tolerations. I can move - operator: Exists into values.yaml as the default for the chart, then the admin can configure it how they want later if they need to. This should keep the default behavior without the need of a bool.

wongma7 · 2021-01-06T21:47:20Z

helm/values.yaml


 imagePullSecrets: []
 nameOverride: ""
 fullnameOverride: ""
+priorityClassName: system-node-critical


now that there is a controller component (the deployment in controller.yaml), probably need to prefix this with nodePriorityClassName and also have `controllerPrijorityClassName?

wongma7 · 2021-01-06T21:52:49Z

helm/Chart.yaml

 description: A Helm chart for AWS EFS CSI Driver
-version: 0.3.0
+name: aws-efs-csi-driver
+version: 0.4.0


leave as 0.1.0 for now, since #292 merged and the only chart in the index is 0.1.0. After #291 merges, I will try to bump the version and include this and everything else between 0.1.0 and now.

wongma7 · 2021-01-06T21:55:02Z

helm/values.yaml


 tolerations: []

 affinity: {}

-node:


Let's keep this, continuing my comment above, since there is a controller component now we are going to need to separate node values from controller values.

ref:
https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/charts/aws-ebs-csi-driver/values.yaml#L86

wongma7 · 2021-01-06T21:56:20Z

helm/values.yaml

+  # If not set and create is true, a name is generated using the fullname template
+  name: ""
+
+rbac:


this should be named less ambiguously, it means the psp-related rbac, not the other rbac resopurces that are necessary for the driver to run

wongma7 · 2021-01-06T21:58:30Z

The merging of #274 has caused some conflict for this PR.

I DON'T expect you to add all the same values you did for the node daemonset to the new controller deployment, but PR needs rebase and we now have to disambiguate values based on whehter they will touch the node daemonset or the controller deployment

thanks, I'll try to get this in for the next helm release

k8s-ci-robot · 2021-01-06T21:58:37Z

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

ccucui · 2021-02-10T19:12:26Z

Hey folks, any news solving these conflicts?

aquam8 · 2021-05-10T17:12:31Z

Thank you @quizy101 for this very valuable PR! It would be so cool if you could review the changes and resolve the conflicts so the reviewers can push this forward, thank you very much!

cnellis101 · 2021-06-03T20:01:15Z

There are a few conflicting files at this point so it will take a moment to sort out but I would love to get this completed. Thanks @aquam8 for the poke.

k8s-triage-robot · 2021-09-01T20:19:14Z

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

k8s-triage-robot · 2021-10-01T21:02:20Z

The Kubernetes project currently lacks enough active 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 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 rotten

k8s-triage-robot · 2021-10-31T21:08:05Z

The Kubernetes project currently lacks enough active 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:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

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

/close

k8s-ci-robot · 2021-10-31T21:08:15Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active 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:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

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

/close

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.

Add PSP/RBAC to helm chart

6ea8008

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 12, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 12, 2020

k8s-ci-robot requested review from jqmichael and leakingtapan November 12, 2020 20:27

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 12, 2020

add newline at EOF

738a706

cnellis101 force-pushed the add-psp-rbac-helm branch from 492fd25 to 738a706 Compare November 12, 2020 20:35

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 12, 2020

This was referenced Nov 12, 2020

Very dirty Helm chart #243

Closed

Helm support for node affinity #234

Closed

Skaronator suggested changes Dec 11, 2020

View reviewed changes

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 Dec 14, 2020

Template namespace and priorityClassName

01c8a8d

Fixed namespace value

45de588

k8s-ci-robot assigned wongma7 Dec 22, 2020

Merge branch 'master' into add-psp-rbac-helm

dd65109

krmichel reviewed Jan 1, 2021

View reviewed changes

helm/templates/daemonset.yaml Show resolved Hide resolved

krmichel reviewed Jan 1, 2021

View reviewed changes

helm/templates/daemonset.yaml Show resolved Hide resolved

krmichel reviewed Jan 1, 2021

View reviewed changes

krmichel reviewed Jan 2, 2021

View reviewed changes

helm/templates/daemonset.yaml Show resolved Hide resolved

krmichel reviewed Jan 2, 2021

View reviewed changes

helm/templates/daemonset.yaml Show resolved Hide resolved

krmichel reviewed Jan 2, 2021

View reviewed changes

helm/templates/daemonset.yaml Outdated Show resolved Hide resolved

cnellis101 added 2 commits January 1, 2021 21:29

Addresses feedback

0df2bc0

Fix namespace

733a277

krmichel reviewed Jan 2, 2021

View reviewed changes

wongma7 reviewed Jan 6, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 6, 2021

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 1, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 1, 2021

k8s-ci-robot closed this Oct 31, 2021

Add psp rbac helm #279

Add psp rbac helm #279

Conversation

cnellis101 commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020

k8s-ci-robot commented Nov 12, 2020

dan-vaughan commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongma7 commented Dec 14, 2020

cnellis101 commented Dec 14, 2020

nxf5025 commented Dec 18, 2020

wongma7 commented Dec 22, 2020

k8s-ci-robot commented Dec 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wongma7 commented Jan 6, 2021

k8s-ci-robot commented Jan 6, 2021

ccucui commented Feb 10, 2021

aquam8 commented May 10, 2021

cnellis101 commented Jun 3, 2021

k8s-triage-robot commented Sep 1, 2021

k8s-triage-robot commented Oct 1, 2021

k8s-triage-robot commented Oct 31, 2021

k8s-ci-robot commented Oct 31, 2021