libpod: pass entire environment to conmon #20148

vrothberg · 2023-09-26T07:45:06Z

Pass the entire environment to conmon instead of selectively enabling only specific variables. The main reasoning is to make sure that conmon and the podman-cleanup callback process operate in the exact same environment than the initial podman process. Some configuration files may be passed via environment variables. Podman not passing those down to conmon has led to subtle and hard to debug issues in the past, so passing all down will avoid such kinds of issues in the future.

Does this PR introduce a user-facing change?

None

Luap99 · 2023-09-26T08:12:02Z

libpod/oci_conmon_common.go

 	env = append(env, fmt.Sprintf("XDG_RUNTIME_DIR=%s", runtimeDir))
-	env = append(env, fmt.Sprintf("_CONTAINERS_USERNS_CONFIGURED=%s", os.Getenv("_CONTAINERS_USERNS_CONFIGURED")))
-	env = append(env, fmt.Sprintf("_CONTAINERS_ROOTLESS_UID=%s", os.Getenv("_CONTAINERS_ROOTLESS_UID")))
-	home := homedir.Get()
-	if home != "" {
+	if home := homedir.Get(); home != "" {
 		env = append(env, fmt.Sprintf("HOME=%s", home))


libpod always sets XDG_RUNTIME_DIR so I don't think we need that at all and can leave the env just nil so that the golang API just passes down the env as is

packit-as-a-service · 2023-09-26T12:01:17Z

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

packit-as-a-service · 2023-09-26T12:26:11Z

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

martinpitt · 2023-09-26T13:24:36Z

@vrothberg : That breakage of podman run -d --name test-sh-system --stop-timeout 0 quay.io/jitesoft/alpinesh, is that on your radar? (also seems to happen with busybox) Or do you need help with reproducing that regression? Thanks!

Luap99 · 2023-09-26T13:42:12Z

libpod/oci_conmon_common.go

+		if strings.HasPrefix(v, "NOTIFY_SOCKET=") {
+			// The NOTIFY_SOCKET cannot leak into the environment.
+			continue


I think you can remove the startCommand() function workaround with this change

Looking at the test failure I think we also need to unset XDG_RUNTIME_DIR when run as root, the problem is that when you login with ssh for example we also get XDG_RUNTIME_DIR=/run/user/0 as root so this makes crun store its files under a different location thus failing to find it.

vrothberg · 2023-09-26T14:27:38Z

@vrothberg : That breakage of podman run -d --name test-sh-system --stop-timeout 0 quay.io/jitesoft/alpinesh, is that on your radar? (also seems to happen with busybox) Or do you need help with reproducing that regression? Thanks!

Thanks for checking, Martin! I will take a look

Pass the _entire_ environment to conmon instead of selectively enabling only specific variables. The main reasoning is to make sure that conmon and the podman-cleanup callback process operate in the exact same environment than the initial podman process. Some configuration files may be passed via environment variables. Podman not passing those down to conmon has led to subtle and hard to debug issues in the past, so passing all down will avoid such kinds of issues in the future. Signed-off-by: Valentin Rothberg <[email protected]>

mheon · 2023-09-26T15:53:22Z

Code changes LGTM

edsantiago · 2023-09-26T19:34:12Z

libpod/oci_conmon_common.go

+	conmonEnv, err := r.configureConmonEnv()
+	if err != nil {
+		return 0, fmt.Errorf("configuring conmon env: %w", err)
+	}


LGTM. I've been trying my best to find a way to break this, like setting IFS or other nasties, and can't find any problems.

One suggestion for cleanup: if you scroll down a few lines, to 1200, cmd.Env = r.conmonEnv, that seems to be a NOP. r.conmonEnv is set above to .Engine.ConmonEnvVars, which in turn is defined in

podman/libpod/options.go

Lines 191 to 192 in 94f47d6

// WithConmonEnv specifies the environment variable list for the conmon process.

func WithConmonEnv(environment []string) RuntimeOption {

... which in turn does not seem to be invoked from anywhere. It dates back to the 2017 crio import, and I think that's all dead code. A future maintainer might appreciate not having to waste time chasing that wild goose.

Great catch! Ough ... I really don't like how we threw containers.conf over the wall and years after still find (on a weekly basis) that many options are not tested at all :( Rushed things turn into ghosts 👻

@edsantiago, I made a note to fix this in another PR. I think it's time to sit down and check which containers.conf options/fields are actually used and then decide case-by-case what to do with them.

@baude something for 5.0.

TomSweeneyRedHat · 2023-09-26T21:29:47Z

Changes LGTM
The rootles host boltdb test failed in a way that I've not seen before. I didn't restart.

vrothberg · 2023-09-27T07:55:46Z

@Luap99 PTAL

Luap99

/lgtm

openshift-ci · 2023-09-27T09:07:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, vrothberg

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~~ [Luap99,vrothberg]

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

openshift-ci bot added release-note-none do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 26, 2023

vrothberg marked this pull request as ready for review September 26, 2023 07:45

openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 26, 2023

vrothberg force-pushed the conmon-env branch from 76922b2 to cb6bebf Compare September 26, 2023 07:45

Luap99 reviewed Sep 26, 2023

View reviewed changes

vrothberg force-pushed the conmon-env branch 2 times, most recently from c02cd14 to c2934cc Compare September 26, 2023 11:40

Luap99 reviewed Sep 26, 2023

View reviewed changes

vrothberg force-pushed the conmon-env branch from c2934cc to 7ade972 Compare September 26, 2023 14:50

edsantiago reviewed Sep 26, 2023

View reviewed changes

Luap99 approved these changes Sep 27, 2023

View reviewed changes

openshift-ci bot assigned Luap99 Sep 27, 2023

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

openshift-merge-robot merged commit 35824b6 into containers:main Sep 27, 2023
97 checks passed

vrothberg deleted the conmon-env branch September 27, 2023 09:11

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 Dec 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libpod: pass entire environment to conmon #20148

libpod: pass entire environment to conmon #20148

vrothberg commented Sep 26, 2023

Luap99 Sep 26, 2023

packit-as-a-service bot commented Sep 26, 2023

packit-as-a-service bot commented Sep 26, 2023

martinpitt commented Sep 26, 2023

Luap99 Sep 26, 2023

vrothberg commented Sep 26, 2023

mheon commented Sep 26, 2023

edsantiago Sep 26, 2023

vrothberg Sep 27, 2023

vrothberg Sep 27, 2023

vrothberg Sep 27, 2023

TomSweeneyRedHat commented Sep 26, 2023

vrothberg commented Sep 27, 2023

Luap99 left a comment

openshift-ci bot commented Sep 27, 2023

	// WithConmonEnv specifies the environment variable list for the conmon process.
	func WithConmonEnv(environment []string) RuntimeOption {

libpod: pass entire environment to conmon #20148

libpod: pass entire environment to conmon #20148

Conversation

vrothberg commented Sep 26, 2023

Does this PR introduce a user-facing change?

Luap99 Sep 26, 2023

Choose a reason for hiding this comment

packit-as-a-service bot commented Sep 26, 2023

packit-as-a-service bot commented Sep 26, 2023

martinpitt commented Sep 26, 2023

Luap99 Sep 26, 2023

Choose a reason for hiding this comment

vrothberg commented Sep 26, 2023

mheon commented Sep 26, 2023

edsantiago Sep 26, 2023

Choose a reason for hiding this comment

vrothberg Sep 27, 2023

Choose a reason for hiding this comment

vrothberg Sep 27, 2023

Choose a reason for hiding this comment

vrothberg Sep 27, 2023

Choose a reason for hiding this comment

TomSweeneyRedHat commented Sep 26, 2023

vrothberg commented Sep 27, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 27, 2023