Fix handling of --read-only-tmpfs flag #20235

rhatdan · 2023-10-02T20:59:08Z

Fixes: #20225

Does this PR introduce a user-facing change?

podman run --read-only-tmpfs=true|false handled correctly when run with --read-only flag

edsantiago · 2023-10-02T21:25:15Z

test/system/030-run.bats

@@ -1120,10 +1120,12 @@ EOF
    CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch /testrw
    CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm $IMAGE touch /tmp/testrw
    for dir in /tmp /var/tmp /dev /dev/shm /run; do
-        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman 1 run --rm --read-only-tmpfs=false $IMAGE touch $dir/testro


These are such confusing options. I don't have the brainpower to review right now, so just two quick points:

for those who care, this is a breaking change

for completeness, should we check all permutations of --read-only with --read-only-tmpfs?

The breaking change would have been to change the --read-only-tmpfs to --read-write-tmpf, which it is called internally. The issue is that if the user actually sets the constant it does it backwards, While if the user never sets it the default is to have read-write-tmpfs.

Bottom line is the bug --read-only-tmpfs=false Was turning on read-only-tmpfs while --read-only-tmfs=true was turning off read-only-tmpfs.

flouthoc

I think man page must mention relation between these two flags.

giuseppe

LGTM

openshift-ci · 2023-10-03T07:54:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, rhatdan

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:

~~OWNERS~~ [giuseppe,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago

Even with fresh morning brain, this is still way too confusing for me, so I'm not blocking but can you please check my comments?

Also, what @flouthoc said. The man page, IMO, only makes things worse as it is.

edsantiago · 2023-10-03T11:18:03Z

cmd/podman/common/create.go

@@ -383,8 +383,7 @@ func DefineCreateFlags(cmd *cobra.Command, cf *entities.ContainerCreateOptions,
 			"read-only", podmanConfig.ContainersConfDefaultsRO.Containers.ReadOnly,
 			"Make containers root filesystem read-only",
 		)
-		createFlags.BoolVar(
-			&cf.ReadWriteTmpFS,
+		createFlags.Bool(
 			"read-only-tmpfs", cf.ReadWriteTmpFS,
 			"When running containers in read-only mode mount a read-write tmpfs on /run, /tmp and /var/tmp",


I realize this is not part of your PR, but this description seems absolutely wrong and misleading. It seems to apply to the variable name, not the option name. (Also, missing comma between "mode" and "mount").

edsantiago · 2023-10-03T11:19:22Z

test/system/030-run.bats

        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch $dir/testro
+        CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false --read-only-tmpfs=true $IMAGE touch $dir/testro


This seems wrong to me? If I ask for read-only-tmpfs, I want a read-only tmpfs.

I added the table to the docs. --read-only=true is required in order to add the --read-only-tmpfs=true.

mheon · 2023-10-03T15:57:25Z

Code LGTM, but I'll refrain from merging until Ed's comments are answered

Luap99

This is straight up breaking as it inverts the meaning of that option, as confusing as the name might be the doc is clear the setting --read-only-tmpfs mounts rw.
We cannot just break every user of that option just because the name is confusing.

rubiksdot · 2023-10-05T04:03:34Z

My understanding of how this works from past behaviour of podman (pre-4.7) and the names of the flags is:

--read-only	--tmpfs-read-only	result for fs/tmpfs
true	true	ro/ro
true	false	ro/rw
false	true	rw/rw
false	false	rw/rw

The current 4.7 behaviour the results of the first two lines are swapped.

The description of the --tmpfs-read-only is confusing. Something like this may be clearer:

"Given --read-only being true should default tmpfs mounts (/tmp, /run, /dev, etc) also be read-only? (default: false)"

Currently all my ansible is broken under 4.7 as it relied on previous behaviour.

(from memory the default pre-4.7 was to make the default tmpfs mounts r/w)

Luap99 · 2023-10-05T09:42:26Z

My understanding of how this works from past behaviour of podman (pre-4.7) and the names of the flags is:
--read-only --tmpfs-read-only result for fs/tmpfs
true true ro/ro
true false ro/rw
false true rw/rw
false false rw/rw

The current 4.7 behaviour the results of the first two lines are swapped.

The description of the --tmpfs-read-only is confusing. Something like this may be clearer:

"Given --read-only being true should default tmpfs mounts (/tmp, /run, /dev, etc) also be read-only? (default: false)"

Currently all my ansible is broken under 4.7 as it relied on previous behaviour.

(from memory the default pre-4.7 was to make the default tmpfs mounts r/w)

There is no --tmpfs-read-only it is called --read-only-tmpfs.

I see no difference between main, 4.7.0 or 4.6.2 or 4.3.1 (prior to 338b283) when checking /tmp mount points.

Your problem seems to be special to the /dev mount
Prior /dev was always mounted rw, regardless of --read-only-tmpfs true or false, now it uses the options which seems to changed intentionally by 22a8b68. So as far as I can tell that change is your problem.

As confusing as the current name might be we should not break the behaviour as this will just break every users of this option so this PR is incorrect IMO.

rhatdan · 2023-10-05T10:53:55Z

Ok the current code looks correct.

podman run --help | grep read-only-tmpfs
      --read-only-tmpfs                          When running containers in read-only mode mount a read-write tmpfs on /run, /tmp and /var/tmp (default true)
tmp $ podman run --read-only alpine touch /tmp/dan
tmp $ podman run --read-only --read-only-tmpfs=true alpine touch /tmp/dan
tmp $ podman run --read-only --read-only-tmpfs=false alpine touch /tmp/dan
touch: /tmp/dan: Read-only file system

The table should be:

--read-only	--tmpfs-read-only	result for fs/tmpfs
true	true	ro/rw
true	false	ro/ro
false	true	rw/IMAGE
false	false	rw/IMAGE

In True|True additional tmpfs are mounted on /tmp, /run, and /var/tmp
In True|False /dev and /dev/shm are marked Read/Only and no tmpfs are mounted on /tmp/run and /var/tmp, meaning they are read-only by default.

In all other cases, the /dev and /dev/shm are read/write and the /tmp, /run, and /var/tmp are not modified.

packit-as-a-service · 2023-10-11T20:17:03Z

Ephemeral COPR build failed. @containers/packit-build please check.

packit-as-a-service · 2023-10-11T20:38:09Z

Cockpit tests failed for commit b5f4d9bb623d61df9ea713eb219c7190ab724761. @martinpitt, @jelly, @mvollmer please check.

rhatdan · 2023-10-12T11:14:33Z

@Luap99 @edsantiago @vrothberg I think this is fixed now PTAL.

Luap99 · 2023-10-12T11:21:29Z

cmd/podman/containers/create.go

+	if cmd.Flags().Changed("read-only-tmpfs") {
+		cliVals.ReadWriteTmpFS, err = flags.GetBool("read-only-tmpfs")
+		if err != nil {
+			return err
+		}
+	}


This change seems unnecessary, you can just keep the flag definition as BoolVar().

Luap99 · 2023-10-12T11:34:12Z

docs/source/markdown/options/read-only-tmpfs.md

+When --read-only=True and --read-only-tmpfs=True additional tmpfs are mounted on
+the /tmp, /run, and /var/tmp directories.
+
+When --read-only=True and --read-only-tmpfs=False /dev and /dev/shm are marked
+Read/Only and no tmpfs are mounted on /tmp, /run and /var/tmp. The directories
+are exposed from the underlying image, meaning they are read-only by default.
+
+In all other cases where --read-only=false the /dev and /dev/shm are read/write and the /tmp, /run, and /var/tmp are not mounted over by default.


It seems to be common style to put command and flags into bold blocks in all man pages, i.e. **--read-only**
I also find it weird to use the upper case True/False, I think we generally stick to the lowercase versions everywhere.

And lastly the last sentence doesn't look right grammatically to me. I am not a native speaker of course but without extra context I don't think I am able to understand this.

packit-as-a-service · 2023-10-12T14:01:56Z

Cockpit tests failed for commit 0b09cf8862a2d8c0c4969ab2f77913d58e34fe10. @martinpitt, @jelly, @mvollmer please check.

packit-as-a-service · 2023-10-12T17:14:44Z

Cockpit tests failed for commit 0cdc2f867bf570e60e521566a69a03db6a345b92. @martinpitt, @jelly, @mvollmer please check.

docs/source/markdown/options/read-only-tmpfs.md

rhatdan · 2023-10-13T18:03:18Z

@TomSweeneyRedHat @Luap99 PTAL

Luap99 · 2023-10-16T08:59:55Z

Can you please squash the second commit.

Fixes: containers#20225 Signed-off-by: Daniel J Walsh <[email protected]>

TomSweeneyRedHat · 2023-10-17T18:40:08Z

Post merge, but LGTM

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 2, 2023

edsantiago reviewed Oct 2, 2023

View reviewed changes

flouthoc reviewed Oct 3, 2023

View reviewed changes

giuseppe approved these changes Oct 3, 2023

View reviewed changes

edsantiago reviewed Oct 3, 2023

View reviewed changes

rhatdan force-pushed the read-only branch 5 times, most recently from 72a128d to baaf5af Compare October 3, 2023 19:20

Luap99 requested changes Oct 4, 2023

View reviewed changes

rhatdan force-pushed the read-only branch 4 times, most recently from 8e2b8f7 to b5f4d9b Compare October 11, 2023 20:13

Luap99 reviewed Oct 12, 2023

View reviewed changes

rhatdan force-pushed the read-only branch from b5f4d9b to 0b09cf8 Compare October 12, 2023 13:29

rhatdan force-pushed the read-only branch from 0b09cf8 to 0cdc2f8 Compare October 12, 2023 16:47

TomSweeneyRedHat reviewed Oct 12, 2023

View reviewed changes

docs/source/markdown/options/read-only-tmpfs.md Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Oct 12, 2023

View reviewed changes

docs/source/markdown/options/read-only-tmpfs.md Outdated Show resolved Hide resolved

Fix handling of --read-only-tmpfs flag

c860408

Fixes: containers#20225 Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the read-only branch from 53f0b39 to c860408 Compare October 16, 2023 18:19

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2023

openshift-ci bot merged commit acc7a94 into containers:main Oct 16, 2023
98 checks passed

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 16, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of --read-only-tmpfs flag #20235

Fix handling of --read-only-tmpfs flag #20235

rhatdan commented Oct 2, 2023

edsantiago Oct 2, 2023

rhatdan Oct 2, 2023

rhatdan Oct 2, 2023

flouthoc left a comment

giuseppe left a comment

openshift-ci bot commented Oct 3, 2023

edsantiago left a comment

edsantiago Oct 3, 2023

edsantiago Oct 3, 2023

rhatdan Oct 3, 2023

mheon commented Oct 3, 2023

Luap99 left a comment

rubiksdot commented Oct 5, 2023

Luap99 commented Oct 5, 2023

rhatdan commented Oct 5, 2023

packit-as-a-service bot commented Oct 11, 2023

packit-as-a-service bot commented Oct 11, 2023

rhatdan commented Oct 12, 2023

Luap99 Oct 12, 2023

Luap99 Oct 12, 2023

packit-as-a-service bot commented Oct 12, 2023

packit-as-a-service bot commented Oct 12, 2023

rhatdan commented Oct 13, 2023

Luap99 commented Oct 16, 2023

TomSweeneyRedHat commented Oct 17, 2023

		CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false $IMAGE touch $dir/testro
		CONTAINERS_CONF_OVERRIDE="$containersconf" run_podman run --rm --read-only=false --read-only-tmpfs=true $IMAGE touch $dir/testro

Fix handling of --read-only-tmpfs flag #20235

Fix handling of --read-only-tmpfs flag #20235

Conversation

rhatdan commented Oct 2, 2023

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc left a comment

Choose a reason for hiding this comment

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 3, 2023

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mheon commented Oct 3, 2023

Luap99 left a comment

Choose a reason for hiding this comment

rubiksdot commented Oct 5, 2023

Luap99 commented Oct 5, 2023

rhatdan commented Oct 5, 2023

packit-as-a-service bot commented Oct 11, 2023

packit-as-a-service bot commented Oct 11, 2023

rhatdan commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packit-as-a-service bot commented Oct 12, 2023

packit-as-a-service bot commented Oct 12, 2023

rhatdan commented Oct 13, 2023

Luap99 commented Oct 16, 2023

TomSweeneyRedHat commented Oct 17, 2023