Add option --unsetenv to remove default environment variables #12100

rhatdan · 2021-10-26T16:46:29Z

Podman adds a few environment variables by default, and
currently there is no way to get rid of them from your container.
This option will allow you to specify which defaults you don't
want.

--unsetenv-all will remove all default environment variables.

Default environment variables can come from podman builtin,
containers.conf or from the container image.

Fixes: #11836

Signed-off-by: Daniel J Walsh [email protected]

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

openshift-ci · 2021-10-26T16:46:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

rhatdan · 2021-10-26T16:47:41Z

Fixes: #11836

mheon · 2021-10-26T18:38:42Z

docs/source/markdown/podman-create.1.md

@@ -1037,6 +1037,12 @@ Remote connections use local containers.conf for defaults
 Set the umask inside the container. Defaults to `0022`.
 Remote connections use local containers.conf for defaults

+#### **--unsetenv**=*env*
+
+Unset default environment variables. These can be configured via buildin,


"Unset default environment variables for the container. Default environment variables include variables provided natively by Podman, environment variables configured by the image, and environment variables from containers.conf"

mheon · 2021-10-26T18:39:46Z

pkg/specgen/specgen.go

@@ -194,6 +194,9 @@ type ContainerBasicConfig struct {
 	// The execution domain system allows Linux to provide limited support
 	// for binaries compiled under other UNIX-like operating systems.
 	Personality *spec.LinuxPersonality `json:"personality,omitempty"`
+	// Unsetenv is a unset default environment variables from the image or from buildin
+	// Optional.
+	Unsetenv []string `json:"unsetenv,omitempty"`


Please rename to UnsetEnv

mheon · 2021-10-26T18:40:19Z

pkg/specgen/generate/oci.go

@@ -332,6 +331,7 @@ func SpecGenToOCI(ctx context.Context, s *specgen.SpecGenerator, rt *libpod.Runt

 	BlockAccessToKernelFilesystems(s.Privileged, s.PidNS.IsHost(), s.Mask, s.Unmask, &g)

+	g.ClearProcessEnv()


This seems dangerous, you're clearing all the default environment variables?

All the defaults set in runtime-tools, Which sets TERM and PATH. We set both of those and currently override what runtime-tools sets.

mheon · 2021-10-26T18:41:09Z

pkg/specgen/generate/container.go

@@ -88,9 +89,6 @@ func CompleteSpec(ctx context.Context, r *libpod.Runtime, s *specgen.SpecGenerat
 	if err != nil {
 		return nil, errors.Wrap(err, "error parsing fields in containers.conf")
 	}
-	if defaultEnvs["container"] == "" {
-		defaultEnvs["container"] = "podman"


Where is this getting set now? You've removed every instance of container=podman I can find.

It is set in pkg/env/DefaultEnvVariables()

rhatdan · 2021-10-26T18:59:18Z

Fixes: #11566

edsantiago

minor quibbles

edsantiago · 2021-10-26T19:23:06Z

test/system/030-run.bats

+@test "podman run defaultenv" {
+    run_podman run --rm $IMAGE printenv
+    is "$output" ".*TERM=xterm" "output matches TERM"
+    is "$output" ".*container=podman" "output matches TERM"


Looks suspiciously like copy-paste inaccuracy (line 745 too).

edsantiago · 2021-10-26T19:26:28Z

test/system/030-run.bats

+
+    run_podman run --unsetenv=TERM --rm $IMAGE printenv
+    is "$output" ".*container=podman" "output matches TERM"
+    run grep TERM <(echo "$output")


Suggestion: <<<"$output" (three anglebrackets) instead of <(...)

edsantiago · 2021-10-26T19:28:07Z

test/system/030-run.bats

+
+    run_podman run --unsetenv=all --rm $IMAGE /bin/printenv
+    run grep TERM <(echo "$output")
+    is "$output" "" "unwanted TERM environment variable"


(very minor nit): Suggestion here and below: "unwanted ... environment variable despite --unsetenv=all"

docs/source/markdown/podman-run.1.md

TomSweeneyRedHat · 2021-10-26T20:18:59Z

I don't know if it's possible, as it looks like you're clearing all, but it would be nice to get a confirmation from the user for each variable unset unless they also pass in the --force option. It would also be nice to have a list command to see the set environment variables.

I don't think either is required for this PR, just thought I'd throw the suggestions out there for rumination.

eriksjolund · 2021-10-26T20:36:54Z

I think it is better two have two command-line options,
instead of using the special value "all" because "all"
is also a valid environment variable name.

Using two command-line options, for instance

--unsetenv
--unsetenv-all

will make it more straight-forward to describe the command-line options
to somebody and also to document them. Using a valid value from the value space to
have a special meaning might lead to other software having to handle this special case.

A very hypothetical scenario:

If you are writing some other software interacting with
Podman, you might need to write

pseudo code:
if user_input_env_name == "all" then print error "Podman does not support unsetting the environment variable 'all' "

I see that the special value style has been used before:

--group-add=group|keep-groups

--network=mode

Special-value style used in documentation

Example 1: --network

See
https://docs.podman.io/en/latest/markdown/podman-run.1.html#network-mode-net

When listing the valid mode values: bridge, none, container:id, network, ns:path, ...
it's not obvious that network need to be replaced by the network name. When I read the line

network: Connect to a user-defined network, multiple networks should be comma-separated.

I often have to read the line twice before realizing that I need to substitute network with the network name.

Example 2: --env

Documentation of --env is a bit complicated due to the special value-style
https://docs.podman.io/en/latest/markdown/podman-run.1.html#env-e-env

umohnani8 · 2021-11-15T17:28:08Z

LGTM
@mheon @TomSweeneyRedHat PTAL

mheon · 2021-11-15T19:13:10Z

libpod/define/container_inspect.go

@@ -30,6 +30,8 @@ type InspectContainerConfig struct {
 	StdinOnce bool `json:"StdinOnce"`
 	// Container environment variables
 	Env []string `json:"Env"`
+	// Unset container environment variables
+	UnsetEnv []string `json:"UnsetEnv"`


Can you make this OmitEmpty?

Also - where's this getting set? I don't see any changes to container_inspect.go in libpod

I removed it, since we only want to see the actual Env in the container. Unsetenv and UnsetenvAll have already done their thing.

Podman adds a few environment variables by default, and currently there is no way to get rid of them from your container. This option will allow you to specify which defaults you don't want. --unsetenv-all will remove all default environment variables. Default environment variables can come from podman builtin, containers.conf or from the container image. Fixes: containers#11836 Signed-off-by: Daniel J Walsh <[email protected]>

edsantiago

LGTM. Just a few minor nits.

edsantiago · 2021-11-15T20:28:13Z

test/system/250-systemd.bats

@@ -174,10 +174,14 @@ function check_listen_env() {
    if is_remote; then
 	is "$output" "$stdenv" "LISTEN Environment did not pass: $context"
    else
-	is "$output" "$stdenv
+	out=$(for o in $output; do echo $o; done| sort)


If for any reason you need to resubmit, could I suggest this instead?

out=$(sort <<<"$output")

Reason: if there's ever an environment variable whose value includes a space (unlikely), the for loop will print each space-separated token on a new line.

edsantiago · 2021-11-15T20:28:34Z

test/system/250-systemd.bats

+       echo "<$out>"
+       echo "<$std>"


You probably don't need these, since the is will display them on failure.

TomSweeneyRedHat · 2021-11-15T20:33:51Z

LGTM
assuming happy tests. @mheon?

edsantiago · 2021-11-15T21:04:32Z

Flake is #12273, restarting.

edsantiago · 2021-11-15T22:05:02Z

Yet another new flake, filed #12306 and restarted.

mheon · 2021-11-15T22:06:01Z

/lgtm
/hold

edsantiago · 2021-11-16T15:26:27Z

/hold cancel

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

rhatdan force-pushed the env branch from 272169d to ee4643d Compare October 26, 2021 16:47

mheon reviewed Oct 26, 2021

View reviewed changes

edsantiago reviewed Oct 26, 2021

View reviewed changes

TomSweeneyRedHat reviewed Oct 26, 2021

View reviewed changes

docs/source/markdown/podman-run.1.md Show resolved Hide resolved

rhatdan force-pushed the env branch 3 times, most recently from 8efbe5b to 59e6f0e Compare October 27, 2021 18:15

rhatdan force-pushed the env branch 4 times, most recently from 0eee79a to 2ec1531 Compare November 12, 2021 18:30

mheon reviewed Nov 15, 2021

View reviewed changes

rhatdan force-pushed the env branch from 2ec1531 to 44d1618 Compare November 15, 2021 20:10

edsantiago reviewed Nov 15, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2021

openshift-ci bot assigned mheon Nov 15, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 15, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2021

openshift-merge-robot merged commit f031bd2 into containers:main Nov 16, 2021

edsantiago mentioned this pull request Nov 16, 2021

CNI: failed to set bridge addr: "cni-podman1" already has an IP address different from 10.25.31.1/24 #12306

Closed

edsantiago mentioned this pull request Nov 30, 2021

podman network rm: ... cni plugin bridge failed #12459

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 22, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option --unsetenv to remove default environment variables #12100

Add option --unsetenv to remove default environment variables #12100

rhatdan commented Oct 26, 2021 •

edited

Loading

openshift-ci bot commented Oct 26, 2021

rhatdan commented Oct 26, 2021

mheon Oct 26, 2021

mheon Oct 26, 2021

mheon Oct 26, 2021

rhatdan Oct 26, 2021

mheon Oct 26, 2021

rhatdan Oct 26, 2021

rhatdan commented Oct 26, 2021

edsantiago left a comment

edsantiago Oct 26, 2021

edsantiago Oct 26, 2021

edsantiago Oct 26, 2021

TomSweeneyRedHat commented Oct 26, 2021

eriksjolund commented Oct 26, 2021 •

edited

Loading

umohnani8 commented Nov 15, 2021

mheon Nov 15, 2021

mheon Nov 15, 2021

rhatdan Nov 15, 2021

edsantiago left a comment

edsantiago Nov 15, 2021

edsantiago Nov 15, 2021

TomSweeneyRedHat commented Nov 15, 2021

edsantiago commented Nov 15, 2021

edsantiago commented Nov 15, 2021

mheon commented Nov 15, 2021

edsantiago commented Nov 16, 2021

		@@ -332,6 +331,7 @@ func SpecGenToOCI(ctx context.Context, s specgen.SpecGenerator, rt libpod.Runt

		BlockAccessToKernelFilesystems(s.Privileged, s.PidNS.IsHost(), s.Mask, s.Unmask, &g)

		g.ClearProcessEnv()

Add option --unsetenv to remove default environment variables #12100

Add option --unsetenv to remove default environment variables #12100

Conversation

rhatdan commented Oct 26, 2021 • edited Loading

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

openshift-ci bot commented Oct 26, 2021

rhatdan commented Oct 26, 2021

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

rhatdan commented Oct 26, 2021

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

TomSweeneyRedHat commented Oct 26, 2021

eriksjolund commented Oct 26, 2021 • edited Loading

Special-value style used in documentation

Example 1: --network

Example 2: --env

umohnani8 commented Nov 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

TomSweeneyRedHat commented Nov 15, 2021

edsantiago commented Nov 15, 2021

edsantiago commented Nov 15, 2021

mheon commented Nov 15, 2021

edsantiago commented Nov 16, 2021

rhatdan commented Oct 26, 2021 •

edited

Loading

eriksjolund commented Oct 26, 2021 •

edited

Loading