Quadlet - treat paths starting with systemd specifiers as absolute #17930

ygalblum · 2023-03-26T09:11:32Z

If a path (Yaml, ConfigMap, EnvFile) starts with a systemd path specifier, treat the path as absolute
Add tests - unit, e2e and bats

Resolves: #17906

Does this PR introduce a user-facing change?

Yes

Quadlet - treat paths starting with systemd specifiers as absolute

openshift-ci · 2023-03-26T09:11:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ygalblum

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~~ [ygalblum]

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

rhatdan · 2023-03-26T11:05:54Z

LGTM

pkg/systemd/quadlet/quadlet.go

test/system/252-quadlet.bats

vrothberg

LGTM

test/system/252-quadlet.bats

vrothberg · 2023-03-27T09:22:45Z

pkg/systemd/quadlet/quadlet.go

@@ -969,7 +969,7 @@ func addNetworks(quadletUnitFile *parser.UnitFile, groupName string, serviceUnit
 }

 func getAbsolutePath(quadletUnitFile *parser.UnitFile, filePath string) (string, error) {
-	if !filepath.IsAbs(filePath) {
+	if filePath[0] != '%' && !filepath.IsAbs(filePath) {


Absolutely non-blocking: A comment on the % special casing may help future readers.

But don't bother repushing for it.

Following other comments, I changes the implementation to use a function and added an explanation on top of it. PTAL

edsantiago · 2023-03-27T11:35:25Z

test/system/252-quadlet.bats

+    local tmp_path=$(mktemp -d --tmpdir quadlet.volume.XXXXXX)
+    local tmp_dir=$(basename "$tmp_path")


I'm having a hard time understanding the point of doing this instead of using $PODMAN_TMPDIR

podman/test/system/helpers.bash

Lines 91 to 96 in 30619bb

# Argh. Although BATS provides $BATS_TMPDIR, it's just /tmp!

# That's bloody worthless. Let's make our own, in which subtests

# can write whatever they like and trust that it'll be deleted

# on cleanup.

# TODO: do this outside of setup, so it carries across tests?

PODMAN_TMPDIR=$(mktemp -d --tmpdir=${BATS_TMPDIR:-/tmp} podman_bats.XXXXXX)

I'm also finding it impossible to understand the systemd.unit(5) documentation for %T:

This is either /tmp or the path "$TMPDIR", "$TEMP" or "$TMP" are set to.

...because there is no indication of the process space in which those envariables are checked.

Anyhow. Please use $PODMAN_TMPDIR or provide an explanation as to why that's not possible.

According to the line you've added, the base directory for PODMAN_TMPDIR can be overridden by setting BATS_TMPDIR. So, if the latter is set, the directory will not be created under /tmp which will cause systemd to point to a non-existing path. For this reason I wanted to ensure the location of the directory. But, if it is never really set, I can use PODMAN_TMPDIR
Having said that, the idea behind the test is to check the support for the systemd specifiers. I chose to use %T, but I don't mind using a different one (https://www.freedesktop.org/software/systemd/man/systemd.unit.html#Specifiers), if you think it makes more sense.
Please let me know what you think

From bats(7):

$BATS_TMPDIR is the base temporary directory used by bats to create its temporary files / directories. (default: $TMPDIR. If $TMPDIR is not set, /tmp is used.)

...but in your current implementation, you use mktemp --tmpdir, which:

...if DIR is not specified, use $TMPDIR if set, else /tmp.

...so as best I can tell, that's transitively equivalent to $PODMAN_TMPDIR... unless I'm missing something...

(and again, it still isn't clear to me what %T expands to. Presumably /tmp in root-systemd, but I have no clue what happens in user-systemd, and I think it's kind of important to understand and document that)

OK, so I'll switch to using PODMAN_DIR and update the rest of the path accordingly.
As for %T, since for other specifiers the documentation provides information about root and user, I understand that it is the same for both. In addition, the tests pass, so it is probably true.

edsantiago · 2023-03-27T11:38:58Z

test/system/252-quadlet.bats

+    local file_name=hello.txt
+    local file_content="hello"


Why not use random_string here too? That eliminates a (small, unlikely) number of false negatives.

local file_name="f$(random_string 10).txt" local file_content="data_$(random_string 15)"

Sure, great idea. I'll update it as part of whatever we decide on the previous comment

Luap99 · 2023-03-27T16:11:18Z

pkg/systemd/quadlet/quadlet.go

@@ -969,7 +969,7 @@ func addNetworks(quadletUnitFile *parser.UnitFile, groupName string, serviceUnit
 }

 func getAbsolutePath(quadletUnitFile *parser.UnitFile, filePath string) (string, error) {
-	if !filepath.IsAbs(filePath) {
+	if filePath[0] != '%' && !filepath.IsAbs(filePath) {


Not sure if it is possible to set the path to an empty string but if it is this will cause a panic, you should check the len(filePath) > 0 before to avoid this.

Added in the new startsWithSystemdSpecifier function

eriksjolund · 2023-03-27T18:55:49Z

pkg/systemd/quadlet/quadlet.go

@@ -969,7 +969,7 @@ func addNetworks(quadletUnitFile *parser.UnitFile, groupName string, serviceUnit
 }

 func getAbsolutePath(quadletUnitFile *parser.UnitFile, filePath string) (string, error) {
-	if !filepath.IsAbs(filePath) {
+	if filePath[0] != '%' && !filepath.IsAbs(filePath) {


Shouldn't there be a special case for %% ?

$ man systemd.unit | grep "Single percent" │"%%" │ Single percent sign │ Use "%%" in place of "%" to specify a single │

Edit 1:
Ah, it quickly gets a bit complicated.

%U is the UID and would never start with a slash.

%h is the home directory and would always start with a slash.

Maybe there are also variants where it's not possible to conclude whether the expanded string starts with a slash or not.

Should the implementation go try to analyse all the different variants? Maybe it's not worth it. I don't know.

Addressed in the startsWithSystemdSpecifier function

If a path (Yaml, ConfigMap, EnvFile) starts with a systemd path specifier, treat the path as absolute Add tests - unit, e2e and bats Signed-off-by: Ygal Blum <[email protected]>

vrothberg

LGTM
Restarted the flaked job.

vrothberg · 2023-03-28T13:14:23Z

@eriksjolund @Luap99 please take another look

Luap99

LGTM

vrothberg · 2023-03-28T14:13:56Z

/lgtm

openshift-ci bot added the release-note label Mar 26, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 26, 2023

ygalblum force-pushed the quadlet-systemd-specifiers branch from 384c968 to 401f083 Compare March 26, 2023 09:11

ygalblum force-pushed the quadlet-systemd-specifiers branch from 401f083 to 1624f81 Compare March 26, 2023 12:38

vrothberg reviewed Mar 27, 2023

View reviewed changes

pkg/systemd/quadlet/quadlet.go Outdated Show resolved Hide resolved

vrothberg reviewed Mar 27, 2023

View reviewed changes

test/system/252-quadlet.bats Outdated Show resolved Hide resolved

ygalblum force-pushed the quadlet-systemd-specifiers branch from 1624f81 to 30f7432 Compare March 27, 2023 09:16

vrothberg reviewed Mar 27, 2023

View reviewed changes

edsantiago reviewed Mar 27, 2023

View reviewed changes

Luap99 reviewed Mar 27, 2023

View reviewed changes

eriksjolund reviewed Mar 27, 2023

View reviewed changes

ygalblum force-pushed the quadlet-systemd-specifiers branch from 30f7432 to 1b6190c Compare March 28, 2023 07:16

Quadlet - treat paths starting with systemd specifiers as absolute

da96ff6

If a path (Yaml, ConfigMap, EnvFile) starts with a systemd path specifier, treat the path as absolute Add tests - unit, e2e and bats Signed-off-by: Ygal Blum <[email protected]>

ygalblum force-pushed the quadlet-systemd-specifiers branch from 1b6190c to da96ff6 Compare March 28, 2023 07:30

vrothberg reviewed Mar 28, 2023

View reviewed changes

Luap99 reviewed Mar 28, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Mar 28, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2023

openshift-merge-robot merged commit 365131e into containers:main Mar 28, 2023

ygalblum deleted the quadlet-systemd-specifiers branch March 28, 2023 15:03

Luap99 mentioned this pull request Jul 5, 2023

.container using EnvironmentFile no longer starts in 4.5.1 #19125

Closed

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 Sep 4, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quadlet - treat paths starting with systemd specifiers as absolute #17930

Quadlet - treat paths starting with systemd specifiers as absolute #17930

ygalblum commented Mar 26, 2023

openshift-ci bot commented Mar 26, 2023

rhatdan commented Mar 26, 2023

vrothberg left a comment

vrothberg Mar 27, 2023

ygalblum Mar 28, 2023 •

edited

Loading

edsantiago Mar 27, 2023

ygalblum Mar 27, 2023

edsantiago Mar 27, 2023

ygalblum Mar 27, 2023

ygalblum Mar 28, 2023

edsantiago Mar 27, 2023

ygalblum Mar 27, 2023

ygalblum Mar 28, 2023

Luap99 Mar 27, 2023

ygalblum Mar 28, 2023

eriksjolund Mar 27, 2023 •

edited

Loading

ygalblum Mar 28, 2023

vrothberg left a comment

vrothberg commented Mar 28, 2023

Luap99 left a comment

vrothberg commented Mar 28, 2023

		local tmp_path=$(mktemp -d --tmpdir quadlet.volume.XXXXXX)
		local tmp_dir=$(basename "$tmp_path")

	# Argh. Although BATS provides $BATS_TMPDIR, it's just /tmp!
	# That's bloody worthless. Let's make our own, in which subtests
	# can write whatever they like and trust that it'll be deleted
	# on cleanup.
	# TODO: do this outside of setup, so it carries across tests?
	PODMAN_TMPDIR=$(mktemp -d --tmpdir=${BATS_TMPDIR:-/tmp} podman_bats.XXXXXX)

Quadlet - treat paths starting with systemd specifiers as absolute #17930

Quadlet - treat paths starting with systemd specifiers as absolute #17930

Conversation

ygalblum commented Mar 26, 2023

Does this PR introduce a user-facing change?

openshift-ci bot commented Mar 26, 2023

rhatdan commented Mar 26, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygalblum Mar 28, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eriksjolund Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg commented Mar 28, 2023

Luap99 left a comment

Choose a reason for hiding this comment

vrothberg commented Mar 28, 2023

ygalblum Mar 28, 2023 •

edited

Loading

eriksjolund Mar 27, 2023 •

edited

Loading