Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

added security context to template if using persisted volumes #6428

Closed
wants to merge 3 commits into from

Conversation

goshlanguage
Copy link
Contributor

What this PR does / why we need it:
This chart is broken when using a PVC with updated versions of Grafana.
Grafana now runs as a non privileged user, with uid 472.

This PR ensures that if we're using a persistent volume, that it has the right access and perms to write to disk.

Which issue this PR fixes
fixes grafana-docker #167
grafana/grafana-docker#167 (comment)

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 2, 2018
@obeyler
Copy link
Contributor

obeyler commented Jul 5, 2018

Question Why do you use 472 as fsGroup ?

@goshlanguage
Copy link
Contributor Author

472 is the uid of the grafana user the upstream docker image creates and uses to run grafana in the container.

Without this, grafana doesn't have access to persistent volumes, and will failed due to insufficient permissions.

@obeyler
Copy link
Contributor

obeyler commented Jul 5, 2018

Ok I'm not an expert in rbac.psp so may I do a mistake but have you think about people who use rbac.pspEnable: false ? in this case it may not work isn't it ?

@goshlanguage
Copy link
Contributor Author

I'm not very familiar with pod security policies, but this PR addresses the linux systems permissions issue in the issue linked to above specifically that stems from the grafana team updating their image.

If this won't work for those disabling psp, I'd love to know about it, and what a possible work around might be.

@antodoms
Copy link

antodoms commented Jul 9, 2018

@RyanHartje I am getting this issue when trying to create Grafana with a persistence turned on in the values.yaml.

@goshlanguage
Copy link
Contributor Author

@antodoms I'm sorry, I don't quite understand. Do you mean that you're also getting issues when enabling persistence with the current Grafana charts, or do you mean that you receive an error with my changes in the Grafana chart when enabling persistent volumes?

Thank you for your help.

@stale
Copy link

stale bot commented Aug 18, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

@stale stale bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 18, 2018
@mattfarina mattfarina added the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Aug 27, 2018
@stale stale bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 27, 2018
@goshlanguage
Copy link
Contributor Author

I've been getting a lot of github notifications for this issue where I filed it in Grafana.

Here's a walkthrough of the problem this PR seeks to solve (copied from an issue filed in Grafana).

The helm chart has broken linux permissions when mounting a volume for persistence.

Within the docker image for grafana, we create the user grafana, with a uid of 472:
https://github.com/grafana/grafana/blob/master/Dockerfile#L40
https://github.com/grafana/grafana/blob/master/Dockerfile#L61

We then give this user parts of the filesystem ownership here:
https://github.com/grafana/grafana/blob/master/Dockerfile#L69

The issue being reported is when helm mounts a volume over /var/lib/grafana, the ownership is changed to the default (root:root).

Here's a walk through of issue replication and identification.

Steps:

Start Minikube

▶ minikube start
Starting local Kubernetes v1.10.0 cluster...
Starting VM...
Downloading Minikube ISO
 160.27 MB / 160.27 MB [==================================] 100.00% 0s
Getting VM IP address...
Moving files into cluster...
Setting up certs...
Connecting to cluster...
Setting up kubeconfig...
Starting cluster components...
Kubectl is now configured to use the cluster.
Loading cached images from config file.

Init Helm

▶ helm init
$HELM_HOME has been configured at /Users/ryan/.helm.

Tiller (the Helm server-side component) has been installed into your Kubernetes Cluster.

Please note: by default, Tiller is deployed with an insecure 'allow unauthenticated users' policy.
For more information on securing your installation see: https://docs.helm.sh/using_helm/#securing-your-helm-installation
Happy Helming!

Modify Grafana values to persist data

Copy default values file (https://github.com/helm/charts/blob/master/stable/grafana/values.yaml)

Modify:

 85 persistence:
 86   enabled: false
 87   # storageClassName: default
 88   # accessModes:
 89   #   - ReadWriteOnce
 90   # size: 10Gi
 91   # annotations: {}
 92   # subPath: ""
 93   # existingClaim:

to

 85 persistence:
 86   enabled: true
 87   accessModes:
 88     - ReadWriteOnce
 89   size: 10Gi

Run helm install

▶ helm install stable/grafana -f ./values.yml
NAME:   linting-umbrellabird
LAST DEPLOYED: Fri Sep  7 09:18:15 2018
NAMESPACE: default
STATUS: DEPLOYED
[truncated]

Inspect persisted volume permissions

Here's the deployment template for Grafana in it's helm chart:
https://github.com/helm/charts/blob/master/stable/grafana/templates/deployment.yaml#L256 - enables the PVC
https://github.com/helm/charts/blob/master/stable/grafana/templates/deployment.yaml#L147 -

Since we see it will be mounted at /var/lib/grafana, let's check out perms in there:

▶ kubectl exec linting-umbrellabird-grafana-fccdbc468-9wjc6 -- stat /var/lib/grafana
  File: /var/lib/grafana
  Size: 4096      	Blocks: 8          IO Block: 4096   directory
Device: 801h/2049d	Inode: 1572866     Links: 3
Access: (0777/drwxrwxrwx)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2018-09-07 13:18:15.591558589 +0000
Modify: 2018-09-07 13:18:32.858762207 +0000
Change: 2018-09-07 13:18:32.858762207 +0000
 Birth: -


▶ kubectl exec linting-umbrellabird-grafana-fccdbc468-9wjc6 -- ls -ahl /var/lib/grafana
total 400K
drwxrwxrwx 3 root    root    4.0K Sep  7 13:18 .
drwxr-xr-x 1 root    root    4.0K May 16 18:25 ..
-rw-r--r-- 1 grafana grafana 384K Sep  7 13:18 grafana.db
drwxr-xr-x 2 grafana grafana 4.0K Sep  7 13:18 plugins

What does this look like by default without persistence?
We can helm install the chart with no values file specified to get default behavior.

▶ helm install stable/grafana
NAME:   eponymous-ladybug
LAST DEPLOYED: Fri Sep  7 09:29:38 2018
NAMESPACE: default
STATUS: DEPLOYED

▶ kubectl exec eponymous-ladybug-grafana-79cf48b9db-5xfcs -- stat /var/lib/grafana
  File: /var/lib/grafana
  Size: 4096      	Blocks: 8          IO Block: 4096   directory
Device: 801h/2049d	Inode: 1057153     Links: 3
Access: (2777/drwxrwsrwx)  Uid: (    0/    root)   Gid: (  472/ grafana)
Access: 2018-09-07 13:30:09.909008138 +0000
Modify: 2018-09-07 13:29:40.667179653 +0000
Change: 2018-09-07 13:29:40.667179653 +0000
 Birth: -

▶ kubectl exec eponymous-ladybug-grafana-79cf48b9db-5xfcs -- ls -ahl /var/lib/grafana
total 400K
drwxrwsrwx 3 root    grafana 4.0K Sep  7 13:29 .
drwxr-xr-x 1 root    root    4.0K May 16 18:25 ..
-rw-r--r-- 1 grafana grafana 384K Sep  7 13:29 grafana.db
drwxr-sr-x 2 grafana grafana 4.0K Sep  7 13:29 plugins

This causes data to be unreadable by the grafana user, making a chart deployed with a persistent volume now useable since grafana 5.1.2 (guesstimate).

@mattfarina
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 7, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: RyanHartje
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: davidkarlsen

If they are not already assigned, you can assign the PR to them by writing /assign @davidkarlsen in a comment when ready.

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
Copy link
Contributor

@RyanHartje: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-charts-e2e d6e66fa link /test pull-charts-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@ey-bot ey-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label Sep 7, 2018
@goshlanguage
Copy link
Contributor Author

Looks like this is already resolved, closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants