Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

docker-images: explicitly create storage folders in Dockerfile… #7991

Merged
merged 1 commit into from
Jan 24, 2020

Conversation

ggilmore
Copy link
Contributor

@ggilmore ggilmore commented Jan 23, 2020

Part of #7829

See also https://github.com/sourcegraph/infrastructure/pull/1792

Many of our docker images write to external volumes for caches, data storage, etc. The file system permissions are identical from both the container's and host's POV.

For example, let's say you mount a folder (/data) into a container and on the host, only uid 11 has r/w access to it. If the container is running as some other unprivileged user (say uid 22) then the container process can't access the folder. This is the source of all the permission issues/container crashes in #7829.

There are multiple ways to mount volumes in Docker:

  • When using bind mounts, what we currently tell people to do by default, if the host folder that's being mounted has mismatched file permissions you're forced to run chmod commands as a workaround. This is hacky, brittle, and might not be possible to run in every environment.

  • When using 'named' volumes, the method that's recommended in the official documentation, the volume's initial contents and permissions are propagated from the container. However, this only applies if the path in the container that the volume is being attached to existed beforehand (otherwise, the volume will be attached with root permissions).

This PR ensures that all of the storage paths in our docker images are created ahead of time in their respective Dockerfiles with permissions that are accessible to our "sourcegraph" user.

There was one complication with https://github.com/sourcegraph/sourcegraph/tree/master/docker-images/prometheus. The upstream image that we inherit from explicitly declares a VOLUME in the Dockerfile with permissions that are only writeable via their default "nobody" user. It is not possible to change the permissions on this volume with a Dockerfile that inherits from this image. (Note that using explicit VOLUME's in Dockerfiles has been a long-standing complaint of many base images: docker-library/postgres#404, moby/moby#3465, moby/moby#3465). The workaround used here is to effectively copy the upstream Dockerfile's assets into our own Dockerfile that deliberately omits that VOLUME directive. We'll need to ensure that our Dockerfile is kept in sync whenever we need to upgrade the Prometheus version, but https://github.com/prometheus/prometheus/commits/master/Dockerfile doesn't change all that often in practice.

--- Test plan

I'm pretty confident our users won't need to do any manual migration due to this change. (Our k8s configuration still runs all our images as root, and anyone still using bind mounts shouldn't have to do anything since the existing host folder's permissions still override anything that's set in the container).

However, just to be sure, we'll need to test a fresh kubernetes deployment, the kubernetes upgrade path, and a fresh docker-compose deployment( note that https://github.com/sourcegraph/deploy-sourcegraph-docker/tree/permission depends on changes from this PR). This change doesn't affect sourcegraph/server.

@ggilmore ggilmore changed the title Volumes docker-images: explicitly create storage folders in Dockerfile with "sourcegraph" accessible permissions Jan 23, 2020
@ggilmore ggilmore marked this pull request as ready for review January 23, 2020 20:22
@ggilmore ggilmore requested a review from a team January 23, 2020 20:22
Copy link
Contributor

@uwedeportivo uwedeportivo left a comment

Choose a reason for hiding this comment

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

@ggilmore walked me through these changes yesterday, and his description of this PR is super nice.

i don't have concerns about the prometheus image changes.

LGTM

Copy link
Member

@emidoots emidoots left a comment

Choose a reason for hiding this comment

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

I didn't review heavily, but I see this is effectively what me and @ggilmore have been discussing actively over Slack, so LGTM.

Copy link
Member

@beyang beyang left a comment

Choose a reason for hiding this comment

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

LGTM. @ggilmore please own testing these in a k8s cluster before merging (or immediately after merging if that saves time).

@ggilmore
Copy link
Contributor Author

@uwedeportivo We'll need to release a new version of sourcegraph/prometheus. What should the tag be? It's currently at 2.15.2 but we're not bumping the version of prometheus itself. 2.15.2-1?

@uwedeportivo
Copy link
Contributor

actually sourcegraph/prometheus versioning is decoupled from prometheus/prometheus versioning
it's at 10.0.6 (version.sh in docker-images/prometheus). so 10.0.7 ?

@ggilmore
Copy link
Contributor Author

ggilmore commented Jan 24, 2020 via email

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants