stage1: implement read-only rootfs #2612

tmrts · 2016-05-11T16:51:59Z

Using the Pod manifest readOnlyRootFS option mounts the rootfs of the app
as read-only using systemd-exec unit option ReadOnlyDirectories.

Fixes #2487

tmrts · 2016-05-11T16:52:44Z

stage1/init/common/pod.go

@@ -720,10 +724,10 @@ func writeEnvFile(p *stage1commontypes.Pod, env types.Environment, appName types

 // PodToSystemd creates the appropriate systemd service unit files for
 // all the constituent apps of the Pod
-func PodToSystemd(p *stage1commontypes.Pod, interactive bool, flavor string, privateUsers string) error {
+func PodToSystemd(p *stage1commontypes.Pod, interactive bool, flavor string, privateUsers string, rootFsIsReadOnly bool) error {


Needs functional tests

tmrts · 2016-05-11T16:56:56Z

cc @iaguis @jonboulle @alban

s-urbaniak · 2016-05-12T07:42:13Z

stage1/init/common/pod.go

@@ -386,7 +386,7 @@ func findBinPath(p *stage1commontypes.Pod, appName types.ACName, app types.App,
 }

 // appToSystemd transforms the provided RuntimeApp+ImageManifest into systemd units
-func appToSystemd(p *stage1commontypes.Pod, ra *schema.RuntimeApp, interactive bool, flavor string, privateUsers string) error {
+func appToSystemd(p *stage1commontypes.Pod, ra *schema.RuntimeApp, interactive bool, flavor string, privateUsers string, rootFsIsReadOnly bool) error {


we should really think about refactoring this function, it becomes bigger and bigger, maybe in "functional options" style.

Please open a PR for functional options and assign it to me 👍

@s-urbaniak Thank you :)

jonboulle · 2016-05-12T08:08:54Z

This needs to be per-app, not per-pod, e.g. https://github.com/coreos/rkt/blob/623fc3140681aeff1f353717a4943e2538769c51/rkt/run.go#L96

tmrts · 2016-05-12T10:38:20Z

Depends on appc/spec#603

@jonboulle PTAL

iaguis · 2016-05-12T12:58:44Z

stage1/init/common/pod.go

@@ -465,6 +465,10 @@ func appToSystemd(p *stage1commontypes.Pod, ra *schema.RuntimeApp, interactive b
 		unit.NewUnitOption("Service", "CapabilityBoundingSet", strings.Join(capabilitiesStr, " ")),
 	}

+	if ra.ReadOnlyRootFS {
+		opts = append(opts, unit.NewUnitOption("Service", "ReadOnlyDirectory", "/"))


Isn't it ReadOnlyDirectories?

Indeed, spelling mistake.

I also wonder if we need to add ReadWriteDirectories= for the RW volumes when we have a read-only rootfs.

More tests to add in tests/rkt_volume_test.go?

iaguis · 2016-05-12T12:59:31Z

Shouldn't we expose a CLI option too?

jonboulle · 2016-05-12T13:01:06Z

yeah we should

On Thu, May 12, 2016 at 2:59 PM, Iago López Galeiras <
notifications@github.com> wrote:

Shouldn't we expose a CLI option too?

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#2612 (comment)

jonboulle · 2016-05-12T13:04:25Z

Basically we need appReadOnlyRootFS
https://github.com/coreos/rkt/blob/master/rkt/cli_apps.go#L309

On Thu, May 12, 2016 at 3:01 PM, Tamer TAS notifications@github.com wrote:

Shouldn't we expose a CLI option too?

cc @jonboulle https://github.com/jonboulle

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2612 (comment)

jonboulle · 2016-05-12T13:04:46Z

Actually, read only flag is NOT blocking for this, because rktnetes always generates pod manifests anyway.

jonboulle · 2016-05-12T13:05:00Z

So we can do that in a follow-up.

Sorry about all the github spam, tra la la la

alban · 2016-05-12T13:05:37Z

Does it work fine with volumes? E.g. a read-write "host" volume + this new rootfs-read-only option. Does the volume stay read-write as expected?

iaguis · 2016-05-12T13:07:02Z

Does it work fine with volumes? E.g. a read-write "host" volume + this new rootfs-read-only option. Does the volume stay read-write as expected?

We probably need to add the target mountpoints to ReadWriteDirectories (if RW)...

tmrts · 2016-05-12T13:34:54Z

@iaguis @alban I'll add RW volumes to ReadWriteDirectories

alban · 2016-05-13T09:37:41Z

tests/rkt_run_pod_manifest_test.go

@@ -367,6 +598,32 @@ func TestPodManifest(t *testing.T) {
 			"",
 		},
 		{
+			// Simple read after write with volume mounted in a read-only rootfs, no apps in pod manifest.


I don't understand this one. There is no mount point, so the volume is not mounted and the test writes directly in the read-only rootfs. Why does it pass?

ReadOnlyRootFS is not set 😄

Nevermind my previous comment, you're correct

alban · 2016-05-13T09:40:42Z

Can we have a summary somewhere of the tests added?

Action	Where	rootfs RO?	ACI manifest RO?	pod manifest RO?	Outcome expected
Read	Rootfs	RO	-	RO	Success
Write+Read	Empty volume	RO	RW	RW	Success
Stat	Empty volume	RO	RW	RW	Success
Write+Read	Host volume	RO	RW	RW	Success
Write+Read	Host volume	RO	RO	RW	Fail
Write+Read	Host volume	RO	RW	RO	Fail
Write+Read	Host volume	RO	RW	unspecified	Success
?
?

alban · 2016-05-13T09:49:52Z

Documentation/subcommands/run.md

+| `--stage1-hash` | none | Image hash (ex. `--stage1-hash=sha512-dedce9f5ea50`) | A hash of a stage1 image. The image must exist in the store. |
+| `--stage1-name` | none | Image name (ex. `--stage1-name=coreos.com/rkt/stage1-coreos`) | A name of a stage1 image. Will perform a discovery if the image is not in the store. |
+| `--stage1-path` | none | Absolute or relative path | A path to a stage1 image. |
+| `--stage1-url` | none | URL with protocol | A URL to a stage1 image. HTTP/HTTPS/File/Docker URLs are supported. |


If you sort this, you should also sort the same in Documentation/subcommands/prepare.md

This could be done in a follow-up PR if you prefer.

I sorted this because this PR was originally implementing runtime flags, I'll submit another PR for sorting docs

Using the Pod manifest `readOnlyRootFS` option mounts the rootfs of the app as read-only using systemd-exec unit option `ReadOnlyDirectories`. Uses `ReadWriteDirectories` systemd-exec unit option for mounting read-write volumes. Fixes rkt#2487

Tests shouldn't fail completely when a case fails

iaguis · 2016-05-13T11:24:57Z

@tmrts has some connection problems, moving to #2624

tmrts reviewed May 11, 2016
View reviewed changes

tmrts force-pushed the implement/read-only-rootfs branch from 31d150e to 8b8fb81 Compare May 11, 2016 17:01

tmrts added this to the v1.6.0 milestone May 12, 2016

tmrts added component/stage1 needs/review needs/tests required-by/rktnetes priority/P1 labels May 12, 2016

s-urbaniak reviewed May 12, 2016
View reviewed changes

jonboulle added needs/some-changes priority/P0 and removed needs/review priority/P1 labels May 12, 2016

tmrts force-pushed the implement/read-only-rootfs branch from 8b8fb81 to 0f304fc Compare May 12, 2016 10:35

tmrts changed the title ~~Implement read-only rootfs user flag~~ stage1: implement read-only rootfs May 12, 2016

tmrts assigned jonboulle May 12, 2016

tmrts added needs/review depends-on/appc do not merge and removed needs/some-changes labels May 12, 2016

tmrts force-pushed the implement/read-only-rootfs branch from 0f304fc to 2918d04 Compare May 12, 2016 11:54

iaguis reviewed May 12, 2016
View reviewed changes

tmrts force-pushed the implement/read-only-rootfs branch 2 times, most recently from 087d17d to 3fca517 Compare May 13, 2016 07:49

docs: sort subcommand descriptions

b55c7d7

tmrts force-pushed the implement/read-only-rootfs branch 2 times, most recently from 2301627 to 499da86 Compare May 13, 2016 08:25

tmrts removed depends-on/appc do not merge needs/tests labels May 13, 2016

tmrts force-pushed the implement/read-only-rootfs branch from 499da86 to 9fb0614 Compare May 13, 2016 09:20

alban reviewed May 13, 2016
View reviewed changes

tmrts added 2 commits May 13, 2016 13:30

stage1: implement read-only rootfs

a644d42

Using the Pod manifest `readOnlyRootFS` option mounts the rootfs of the app as read-only using systemd-exec unit option `ReadOnlyDirectories`. Uses `ReadWriteDirectories` systemd-exec unit option for mounting read-write volumes. Fixes rkt#2487

tests/rkt-run-pod-manifest: contain test failures

2526677

Tests shouldn't fail completely when a case fails

tmrts force-pushed the implement/read-only-rootfs branch from 9fb0614 to 2526677 Compare May 13, 2016 10:32

iaguis mentioned this pull request May 13, 2016

stage1: implement read-only rootfs #2624

Merged

iaguis closed this May 13, 2016

alban mentioned this pull request May 13, 2016

doc: ReadWriteDirectories/ReadOnlyDirectories not relative to RootDirectory systemd/systemd#3248

Closed

tmrts deleted the implement/read-only-rootfs branch May 14, 2016 06:38

tmrts restored the implement/read-only-rootfs branch May 23, 2016 10:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stage1: implement read-only rootfs #2612

stage1: implement read-only rootfs #2612

tmrts commented May 11, 2016 •

edited

tmrts May 11, 2016

tmrts commented May 11, 2016 •

edited

s-urbaniak May 12, 2016

tmrts May 12, 2016 •

edited

s-urbaniak May 12, 2016

tmrts May 12, 2016

jonboulle commented May 12, 2016

tmrts commented May 12, 2016 •

edited

iaguis May 12, 2016

alban May 12, 2016

tmrts May 12, 2016

iaguis commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

alban commented May 12, 2016

iaguis commented May 12, 2016

tmrts commented May 12, 2016

alban May 13, 2016

tmrts May 13, 2016

tmrts May 13, 2016

alban commented May 13, 2016

alban May 13, 2016

alban May 13, 2016

tmrts May 13, 2016

iaguis commented May 13, 2016

stage1: implement read-only rootfs #2612

stage1: implement read-only rootfs #2612

Conversation

tmrts commented May 11, 2016 • edited

Choose a reason for hiding this comment

tmrts commented May 11, 2016 • edited

Choose a reason for hiding this comment

tmrts May 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonboulle commented May 12, 2016

tmrts commented May 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iaguis commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

jonboulle commented May 12, 2016

alban commented May 12, 2016

iaguis commented May 12, 2016

tmrts commented May 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alban commented May 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iaguis commented May 13, 2016

tmrts commented May 11, 2016 •

edited

tmrts commented May 11, 2016 •

edited

tmrts May 12, 2016 •

edited

tmrts commented May 12, 2016 •

edited