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

Ensure that the runtime mounts RO volumes read-only #58720

Merged
merged 1 commit into from Feb 2, 2018

Conversation

@joelsmith
Copy link
Contributor

commented Jan 23, 2018

What this PR does / why we need it:

This change is part of the fix to address CVE-2017-1002102 (#60814).

This change makes it so that containers cannot write to secret, configMap, downwardAPI and projected volumes since the runtime will now mount them read-only. This change makes things less confusing for a user since any attempt to update a secret volume will result in an error rather than a successful change followed by a revert by the kubelet when the volume next syncs.

It also adds a feature gate ReadOnlyAPIDataVolumes to a provide a way to disable the new behavior in 1.10, but for 1.11, the new behavior will become non-optional.

Also, E2E tests for downwardAPI and projected volumes are updated to mount the volumes somewhere other than /etc.

Which issue(s) this PR fixes
Fixes #58719

Fixes #60814 for master / 1.10

Release note:

Changes secret, configMap, downwardAPI and projected volumes to mount read-only, instead of allowing applications to write data and then reverting it automatically. Until version 1.11, setting the feature gate ReadOnlyAPIDataVolumes=false will preserve the old behavior.
@joelsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2018

/unassign @dims
/unassign @yujuhong
/assign @jsafrane
/assign @derekwaynecarr
/kind bug
/sig storage
/sig node

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Jan 24, 2018

/approve

@joelsmith joelsmith force-pushed the joelsmith:ro-vol branch from a89d4e6 to 0184d60 Jan 24, 2018

@k8s-ci-robot k8s-ci-robot added size/M and removed size/XS approved labels Jan 24, 2018

@joelsmith joelsmith force-pushed the joelsmith:ro-vol branch from 0184d60 to 4fd49c9 Jan 24, 2018

@joelsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

/retest

@jsafrane

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

I am afraid this is behavior change. Volumes that were not read-only are read-only now. As you can see in e2e test updates, it can break existing applications.

IMO it is the right way to go, however I am not sure a small release note is enough.

@saad-ali, what do you think?

@joelsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

/retest

@joelsmith joelsmith force-pushed the joelsmith:ro-vol branch from 4fd49c9 to 450e0ee Jan 25, 2018

@joelsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

Due to the issues raised by @jsafrane we should probably hold this PR until we get a 👍 from @smarterclayton and/or @saad-ali .

/hold

@@ -56,8 +56,7 @@ spec:
timeoutSeconds: 30
volumes:
- name: kubernetes-dashboard-certs
secret:
secretName: kubernetes-dashboard-certs
emptyDir: {}

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 25, 2018

Member

please open an issue for the dashboard to stop trying to write to this location and to simply use in-memory certs in the absence of mounted certs

This comment has been minimized.

Copy link
@liggitt

liggitt Jan 25, 2018

Member

fyi, opened kubernetes/dashboard#2795 to keep the dashboard from writing to this location

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 1, 2018

Member

Would like a dashboard release cut and bumped rather than switching this to emptydir

This comment has been minimized.

Copy link
@liggitt

liggitt Feb 4, 2018

Member

@joelsmith this is still outstanding

This comment has been minimized.

Copy link
@joelsmith

joelsmith Feb 5, 2018

Author Contributor

@liggitt I have opened this issue:
#59354
and this PR:
#59355
I will contact @floreks about a new release of Dashboard.

@liggitt

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

I am afraid this is behavior change. Volumes that were not read-only are read-only now. As you can see in e2e test updates, it can break existing applications.

IMO it is the right way to go, however I am not sure a small release note is enough.

Applications that relied on persisting data in these locations were already broken, since the volume sync would remove that data at a later point in time. A release note seems sufficient to me (updated the release note with additional context)

@liggitt

This comment has been minimized.

Copy link
Member

commented Jan 25, 2018

/retest

@joelsmith

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2018

The pull-kubernetes-e2e-gke is not marked "Required" and seems to be failing for everyone right now.

rolanddb added a commit to Eneco/charts that referenced this pull request Apr 9, 2018

[stable/minio] Fix configmap issue, add ingress resource and general …
…refactor (helm#4281)

* Rename manifests to align with best practices

* Refactor minio chart

- add ingress resource
- consolidate svc resource to support all deployment modes
- update labels and selectors to align with helm best practices
- general cleanup to align with helm best practices/patterns observed in `helm create`
- update values, README and _helpers accordingly
- bump image tag
- bump chart version

* Fix issue caused by ConfigMaps now being mounted ReadOnly

Tested on:
k8s 1.8.10 and 1.9.6

Related:
kubernetes/kubernetes#58720

Fixes:
helm#4272

* Bump chart version to 1.0.0

rolanddb added a commit to Eneco/charts that referenced this pull request Apr 9, 2018

add an InitContainer to the Deployment to copy the files from the Con…
…figMap to an EmptyDir (helm#4271)

* Update deployment.yaml

ConfigMaps are mounted read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The Grafana Chart uses a ConfigMap to provision the config- and dashboard directories. Grafana tries to create/modify files in these directories, which is not allowed anymore. This PR adds an busybox initContainer to the Deployment that copies the files from the ConfigMap to a new emptyDir, similar to helm#4169. Fixes helm#4267.

* bump Chart version

ichtar pushed a commit to Bestmile/charts that referenced this pull request May 15, 2018

add initContainer to copy config from ConfigMap (helm#4169)
* Update statefulset.yaml

ConfigMaps are read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The rabbitmq-ha Chart uses a ConfigMap to provision /etc/rabbitmq. The official RabbitMQ docker image modifies these files in its docker-entrypoint.sh. This PR adds an initContainer to the StatefulSet to copy the configs from the ConfigMap to a new emptyDir volume.

* Update Chart.yaml

Bump version

* Update statefulset.yaml

ichtar pushed a commit to Bestmile/charts that referenced this pull request May 15, 2018

[stable/minio] Fix configmap issue, add ingress resource and general …
…refactor (helm#4281)

* Rename manifests to align with best practices

* Refactor minio chart

- add ingress resource
- consolidate svc resource to support all deployment modes
- update labels and selectors to align with helm best practices
- general cleanup to align with helm best practices/patterns observed in `helm create`
- update values, README and _helpers accordingly
- bump image tag
- bump chart version

* Fix issue caused by ConfigMaps now being mounted ReadOnly

Tested on:
k8s 1.8.10 and 1.9.6

Related:
kubernetes/kubernetes#58720

Fixes:
helm#4272

* Bump chart version to 1.0.0

ichtar pushed a commit to Bestmile/charts that referenced this pull request May 15, 2018

add an InitContainer to the Deployment to copy the files from the Con…
…figMap to an EmptyDir (helm#4271)

* Update deployment.yaml

ConfigMaps are mounted read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The Grafana Chart uses a ConfigMap to provision the config- and dashboard directories. Grafana tries to create/modify files in these directories, which is not allowed anymore. This PR adds an busybox initContainer to the Deployment that copies the files from the ConfigMap to a new emptyDir, similar to helm#4169. Fixes helm#4267.

* bump Chart version

theinrichs added a commit to theinrichs/gitlab that referenced this pull request May 25, 2018

Remove chown of secrets and configmaps
See kubernetes/kubernetes#58720
The chown breaks the gitlab startup in k8s 1.9 and is not necessary.

theinrichs added a commit to inovex/gitlab that referenced this pull request May 25, 2018

Remove chown of secrets and configmaps
See kubernetes/kubernetes#58720
The chown breaks the gitlab startup in k8s 1.9 and is not necessary.

@jstriebel jstriebel referenced this pull request Jul 19, 2018

Closed

Update Changelog 1.9 #66374

@ludwikbukowski

This comment has been minimized.

Copy link

commented Sep 5, 2018

How can I create executable files with configMap now?

voron added a commit to arilot/charts that referenced this pull request Sep 5, 2018

add initContainer to copy config from ConfigMap (helm#4169)
* Update statefulset.yaml

ConfigMaps are read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The rabbitmq-ha Chart uses a ConfigMap to provision /etc/rabbitmq. The official RabbitMQ docker image modifies these files in its docker-entrypoint.sh. This PR adds an initContainer to the StatefulSet to copy the configs from the ConfigMap to a new emptyDir volume.

* Update Chart.yaml

Bump version

* Update statefulset.yaml

Signed-off-by: voron <av@arilot.com>

voron added a commit to arilot/charts that referenced this pull request Sep 5, 2018

[stable/minio] Fix configmap issue, add ingress resource and general …
…refactor (helm#4281)

* Rename manifests to align with best practices

* Refactor minio chart

- add ingress resource
- consolidate svc resource to support all deployment modes
- update labels and selectors to align with helm best practices
- general cleanup to align with helm best practices/patterns observed in `helm create`
- update values, README and _helpers accordingly
- bump image tag
- bump chart version

* Fix issue caused by ConfigMaps now being mounted ReadOnly

Tested on:
k8s 1.8.10 and 1.9.6

Related:
kubernetes/kubernetes#58720

Fixes:
helm#4272

* Bump chart version to 1.0.0

Signed-off-by: voron <av@arilot.com>

voron added a commit to arilot/charts that referenced this pull request Sep 5, 2018

add an InitContainer to the Deployment to copy the files from the Con…
…figMap to an EmptyDir (helm#4271)

* Update deployment.yaml

ConfigMaps are mounted read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The Grafana Chart uses a ConfigMap to provision the config- and dashboard directories. Grafana tries to create/modify files in these directories, which is not allowed anymore. This PR adds an busybox initContainer to the Deployment that copies the files from the ConfigMap to a new emptyDir, similar to helm#4169. Fixes helm#4267.

* bump Chart version

Signed-off-by: voron <av@arilot.com>

manics pushed a commit to ome/minio-helm-chart that referenced this pull request Oct 8, 2018

[stable/minio] Fix configmap issue, add ingress resource and general …
…refactor (#4281)

* Rename manifests to align with best practices

* Refactor minio chart

- add ingress resource
- consolidate svc resource to support all deployment modes
- update labels and selectors to align with helm best practices
- general cleanup to align with helm best practices/patterns observed in `helm create`
- update values, README and _helpers accordingly
- bump image tag
- bump chart version

* Fix issue caused by ConfigMaps now being mounted ReadOnly

Tested on:
k8s 1.8.10 and 1.9.6

Related:
kubernetes/kubernetes#58720

Fixes:
helm/charts#4272

* Bump chart version to 1.0.0

gochist pushed a commit to gochist/charts that referenced this pull request Mar 27, 2019

add initContainer to copy config from ConfigMap (helm#4169)
* Update statefulset.yaml

ConfigMaps are read-only since Kubernetes 1.9.4 (kubernetes/kubernetes#58720). The rabbitmq-ha Chart uses a ConfigMap to provision /etc/rabbitmq. The official RabbitMQ docker image modifies these files in its docker-entrypoint.sh. This PR adds an initContainer to the StatefulSet to copy the configs from the ConfigMap to a new emptyDir volume.

* Update Chart.yaml

Bump version

* Update statefulset.yaml

@oneonestar oneonestar referenced this pull request Apr 9, 2019

Merged

Add Presto Dockerfile. #590

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.