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

Add --read-only for read only container rootfs #10093

Merged
merged 1 commit into from Jan 14, 2015

Conversation

Projects
None yet
10 participants
@crosbymichael
Contributor

crosbymichael commented Jan 14, 2015

Add a --read-only flag to allow the container's root filesystem to be
mounted as read only. This can be used in combination with volumes to
force a container's process to only write to locations that will be
persisted. This is useful in many cases where the admin controls where
they would like developers to write files and error on any other
locations.

Closes #7923
Closes #8752

Signed-off-by: Michael Crosby crosbymichael@gmail.com

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 14, 2015

Contributor

@crosbymichael Tests not compiling :)

Contributor

LK4D4 commented Jan 14, 2015

@crosbymichael Tests not compiling :)

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@LK4D4 i don't know what you are talking about....

Contributor

crosbymichael commented Jan 14, 2015

@LK4D4 i don't know what you are talking about....

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Jan 14, 2015

Member

+1 for this even without --tmpfs 👍

:hurtrealbad:

Member

tianon commented Jan 14, 2015

+1 for this even without --tmpfs 👍

:hurtrealbad:

Show outdated Hide outdated docs/man/docker-run.1.md
**--readonly**=*true*|*false*
Mount the container's root filesystem as readonly.
By default a container will have it's root filesystem writable allowing processes

This comment has been minimized.

@mrunalp

mrunalp Jan 14, 2015

Contributor

typo: it's --> its

@mrunalp

mrunalp Jan 14, 2015

Contributor

typo: it's --> its

Show outdated Hide outdated docs/man/docker-run.1.md
By default a container will have it's root filesystem writable allowing processes
to write files anywhere. By specifying the `--readonly` flag the container will have
it's root filesystem mounted as readonly prohibiting any writes.

This comment has been minimized.

@mrunalp

mrunalp Jan 14, 2015

Contributor

Same here.

@mrunalp

mrunalp Jan 14, 2015

Contributor

Same here.

@mrunalp

This comment has been minimized.

Show comment
Hide comment
@mrunalp

mrunalp Jan 14, 2015

Contributor

👍

Contributor

mrunalp commented Jan 14, 2015

👍

@jessfraz

This comment has been minimized.

Show comment
Hide comment
@jessfraz

jessfraz Jan 14, 2015

Contributor

cooooollll the diff is so smalll! LGTM

Contributor

jessfraz commented Jan 14, 2015

cooooollll the diff is so smalll! LGTM

Show outdated Hide outdated docs/sources/reference/commandline/cli.md
@@ -1681,6 +1683,13 @@ will automatically create this directory on the host for you. In the
example above, Docker will create the `/doesnt/exist`
folder before starting your container.
$ sudo docker run -v /icanwrite --readonly busybox touch /icanwrite here

This comment has been minimized.

@tiborvass

tiborvass Jan 14, 2015

Collaborator

Minor nit, but docker run --readonly -v /icanwrite ... could be better, as "icanwrite readonly" sounds weird.

@tiborvass

tiborvass Jan 14, 2015

Collaborator

Minor nit, but docker run --readonly -v /icanwrite ... could be better, as "icanwrite readonly" sounds weird.

This comment has been minimized.

@SvenDowideit

SvenDowideit Jan 14, 2015

Contributor

i agree - putting the flag you're talking about first also gives it emphasis.

@SvenDowideit

SvenDowideit Jan 14, 2015

Contributor

i agree - putting the flag you're talking about first also gives it emphasis.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Jan 14, 2015

Collaborator

Should we add a small warning somewhere explaining that tmpfs mountpoints are still writeable?

Collaborator

tiborvass commented Jan 14, 2015

Should we add a small warning somewhere explaining that tmpfs mountpoints are still writeable?

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

Any mounts are still writable, if they are mounted as rw

Contributor

crosbymichael commented Jan 14, 2015

Any mounts are still writable, if they are mounted as rw

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Jan 14, 2015

Collaborator

Oh right, it's only the rootfs that's readonly, my bad. Code LGTM

Collaborator

tiborvass commented Jan 14, 2015

Oh right, it's only the rootfs that's readonly, my bad. Code LGTM

Show outdated Hide outdated docs/sources/reference/api/docker_remote_api_v1.17.md
@@ -323,6 +326,7 @@ Return low-level information on the container `id`
"NetworkMode": "bridge",
"PortBindings": {},
"Privileged": false,
"ReadonlyRootfs": false,

This comment has been minimized.

@LK4D4

LK4D4 Jan 14, 2015

Contributor

weird formatting

@LK4D4

LK4D4 Jan 14, 2015

Contributor

weird formatting

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 14, 2015

Contributor

I have no idea where, but should be in "what's new"

Contributor

LK4D4 commented Jan 14, 2015

I have no idea where, but should be in "what's new"

Show outdated Hide outdated docs/sources/reference/commandline/cli.md
a container writes files. The `--readonly` flag mounts the container's root
filesystem as read only prohibiting writes to locations other than the
specified volumes for the container.

This comment has been minimized.

@SvenDowideit

SvenDowideit Jan 14, 2015

Contributor

I'd flip the first sentence to be more like

To control where a container can write files, you can combine the--readonlyflag with volumes of volume containers

Starting a discussion about readonly with Volumes made me wonder if it was out of place.

no matter tho, easy for us to discuss post-merge.

@SvenDowideit

SvenDowideit Jan 14, 2015

Contributor

I'd flip the first sentence to be more like

To control where a container can write files, you can combine the--readonlyflag with volumes of volume containers

Starting a discussion about readonly with Volumes made me wonder if it was out of place.

no matter tho, easy for us to discuss post-merge.

This comment has been minimized.

@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@fredlf ! ! ! ^^^ I'm trying to apply what you told us but @SvenDowideit is saying you are wrong or I did it wrong.

@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@fredlf ! ! ! ^^^ I'm trying to apply what you told us but @SvenDowideit is saying you are wrong or I did it wrong.

This comment has been minimized.

@fredlf

fredlf Jan 15, 2015

Contributor

Heh, this is one of those cases where we can see the fact that writing is not code. There's no right or wrong, here, it's a question of what we most want to emphasize. @crosbymichael 's original sentence emphasizes the idea of "control over where a container writes". @SvenDowideit 's rewrite places reader emphasis on the idea of "combining the --readonly flag with volumes." Only the writer knows what he actually wanted to emphasize. But @SvenDowideit's response as a reader (another term for RET is reader-response criticism), gives an important clue: he did not have any context for mentally processing the concept of "volumes" when it was introduced. So, let's give the reader the context they need and expect: "The --readonly flag can be used in combination with volumes to control where a container writes files."

@fredlf

fredlf Jan 15, 2015

Contributor

Heh, this is one of those cases where we can see the fact that writing is not code. There's no right or wrong, here, it's a question of what we most want to emphasize. @crosbymichael 's original sentence emphasizes the idea of "control over where a container writes". @SvenDowideit 's rewrite places reader emphasis on the idea of "combining the --readonly flag with volumes." Only the writer knows what he actually wanted to emphasize. But @SvenDowideit's response as a reader (another term for RET is reader-response criticism), gives an important clue: he did not have any context for mentally processing the concept of "volumes" when it was introduced. So, let's give the reader the context they need and expect: "The --readonly flag can be used in combination with volumes to control where a container writes files."

@SvenDowideit

This comment has been minimized.

Show comment
Hide comment
@SvenDowideit

SvenDowideit Jan 14, 2015

Contributor

This is a very cool featurette! Docs LGTM @fredlf @jamtur01

Need to create a working example showing how its useful tho

Contributor

SvenDowideit commented Jan 14, 2015

This is a very cool featurette! Docs LGTM @fredlf @jamtur01

Need to create a working example showing how its useful tho

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Jan 14, 2015

Contributor

I think readonly should probably be read-only both in option and text. It's definitely read-only when used in a sentence.

Contributor

jamtur01 commented Jan 14, 2015

I think readonly should probably be read-only both in option and text. It's definitely read-only when used in a sentence.

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Jan 14, 2015

Contributor

Otherwise LGTM

Contributor

jamtur01 commented Jan 14, 2015

Otherwise LGTM

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@jamtur01 you mean --read-only?

Contributor

crosbymichael commented Jan 14, 2015

@jamtur01 you mean --read-only?

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Jan 14, 2015

Contributor

Yes - though I'm open to be told I'm wrong as an option - but definitely the docs should say "read-only" - readonly is wrong.

Contributor

jamtur01 commented Jan 14, 2015

Yes - though I'm open to be told I'm wrong as an option - but definitely the docs should say "read-only" - readonly is wrong.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@tianon what do you think? --readonly or --read-only for a cli flag?

Contributor

crosbymichael commented Jan 14, 2015

@tianon what do you think? --readonly or --read-only for a cli flag?

@tianon

This comment has been minimized.

Show comment
Hide comment
@tianon

tianon Jan 14, 2015

Member

I think --read-only for consistency, even though I like --readonly.

from man mount:

       -r, --read-only
              Mount the filesystem read-only. A synonym is -o ro.

              Note  that,  depending  on the filesystem type, state and kernel
              behavior, the system may still write to the device. For example,
              ext3 or ext4 will replay its journal if the filesystem is dirty.
              To prevent this kind of write access, you may want to mount ext3
              or  ext4  filesystem  with  "ro,noload" mount options or set the
              block device to read-only mode, see command blockdev(8).
Member

tianon commented Jan 14, 2015

I think --read-only for consistency, even though I like --readonly.

from man mount:

       -r, --read-only
              Mount the filesystem read-only. A synonym is -o ro.

              Note  that,  depending  on the filesystem type, state and kernel
              behavior, the system may still write to the device. For example,
              ext3 or ext4 will replay its journal if the filesystem is dirty.
              To prevent this kind of write access, you may want to mount ext3
              or  ext4  filesystem  with  "ro,noload" mount options or set the
              block device to read-only mode, see command blockdev(8).
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@tianon thanks, consistency wins, i'll make the change in docs and code.

Contributor

crosbymichael commented Jan 14, 2015

@tianon thanks, consistency wins, i'll make the change in docs and code.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@jamtur01 made the changes and changed the flag name to be consistent with other tools.

Contributor

crosbymichael commented Jan 14, 2015

@jamtur01 made the changes and changed the flag name to be consistent with other tools.

@jamtur01

This comment has been minimized.

Show comment
Hide comment
@jamtur01

jamtur01 Jan 14, 2015

Contributor

LGTM

Contributor

jamtur01 commented Jan 14, 2015

LGTM

@crosbymichael crosbymichael changed the title from Add --readonly for read only container rootfs to Add --read-only for read only container rootfs Jan 14, 2015

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 14, 2015

Contributor

Okay, I was ignored again :(
I found What's new section for you @crosbymichael : docs/sources/reference/api/docker_remote_api.md.

Contributor

LK4D4 commented Jan 14, 2015

Okay, I was ignored again :(
I found What's new section for you @crosbymichael : docs/sources/reference/api/docker_remote_api.md.

@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@LK4D4 I saw your comment, I just don't know what to say about adding a new field to an api object, it looks to be more about endpoints.

Contributor

crosbymichael commented Jan 14, 2015

@LK4D4 I saw your comment, I just don't know what to say about adding a new field to an api object, it looks to be more about endpoints.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 14, 2015

Contributor

@crosbymichael Now it is possible to mount container rootfs as read-only!!!!!! or something like this. This is new feature in api, this should be there.
ping @SvenDowideit maybe you know how to write it better.

Contributor

LK4D4 commented Jan 14, 2015

@crosbymichael Now it is possible to mount container rootfs as read-only!!!!!! or something like this. This is new feature in api, this should be there.
ping @SvenDowideit maybe you know how to write it better.

Add --readonly for read only container rootfs
Add a --readonly flag to allow the container's root filesystem to be
mounted as readonly.  This can be used in combination with volumes to
force a container's process to only write to locations that will be
persisted.  This is useful in many cases where the admin controls where
they would like developers to write files and error on any other
locations.

Closes #7923
Closes #8752

Signed-off-by: Michael Crosby <crosbymichael@gmail.com>
@crosbymichael

This comment has been minimized.

Show comment
Hide comment
@crosbymichael

crosbymichael Jan 14, 2015

Contributor

@LK4D4 added

Contributor

crosbymichael commented Jan 14, 2015

@LK4D4 added

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 14, 2015

Contributor

LGTM

Contributor

LK4D4 commented Jan 14, 2015

LGTM

LK4D4 added a commit that referenced this pull request Jan 14, 2015

Merge pull request #10093 from crosbymichael/readonly-containers
Add --read-only for read only container rootfs

@LK4D4 LK4D4 merged commit 95c0f07 into moby:master Jan 14, 2015

1 check passed

default The build succeeded on drone.io
Details

@crosbymichael crosbymichael deleted the crosbymichael:readonly-containers branch Jan 14, 2015

@noisy

This comment has been minimized.

Show comment
Hide comment
@noisy

noisy commented Feb 16, 2015

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment