Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libpod: pass entire environment to conmon #20148

Merged
merged 1 commit into from
Sep 27, 2023
Merged

libpod: pass entire environment to conmon #20148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 18 additions & 55 deletions libpod/oci_conmon_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import (
"github.com/containers/podman/v4/pkg/specgenutil"
"github.com/containers/podman/v4/pkg/util"
"github.com/containers/podman/v4/utils"
"github.com/containers/storage/pkg/homedir"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -1042,11 +1041,6 @@ func (r *ConmonOCIRuntime) getLogTag(ctr *Container) (string, error) {
func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *ContainerCheckpointOptions) (int64, error) {
var stderrBuf bytes.Buffer

runtimeDir, err := util.GetRuntimeDir()
if err != nil {
return 0, err
}

parentSyncPipe, childSyncPipe, err := newPipe()
if err != nil {
return 0, fmt.Errorf("creating socket pair: %w", err)
Expand Down Expand Up @@ -1189,7 +1183,10 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
}

// 0, 1 and 2 are stdin, stdout and stderr
conmonEnv := r.configureConmonEnv(runtimeDir)
conmonEnv, err := r.configureConmonEnv()
if err != nil {
return 0, fmt.Errorf("configuring conmon env: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baude something for 5.0.


var filesToClose []*os.File
if preserveFDs > 0 {
Expand Down Expand Up @@ -1251,7 +1248,7 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
if restoreOptions != nil {
runtimeRestoreStarted = time.Now()
}
err = startCommand(cmd, ctr)
err = cmd.Start()

// regardless of whether we errored or not, we no longer need the children pipes
childSyncPipe.Close()
Expand Down Expand Up @@ -1311,38 +1308,23 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
}

// configureConmonEnv gets the environment values to add to conmon's exec struct
// TODO this may want to be less hardcoded/more configurable in the future
func (r *ConmonOCIRuntime) configureConmonEnv(runtimeDir string) []string {
var env []string
for _, e := range os.Environ() {
if strings.HasPrefix(e, "LC_") {
env = append(env, e)
}
if strings.HasPrefix(e, "LANG=") {
env = append(env, e)
func (r *ConmonOCIRuntime) configureConmonEnv() ([]string, error) {
env := os.Environ()
res := make([]string, 0, len(env))
for _, v := range env {
if strings.HasPrefix(v, "NOTIFY_SOCKET=") {
// The NOTIFY_SOCKET must not leak into the environment.
continue
}
res = append(res, v)
}
if path, ok := os.LookupEnv("PATH"); ok {
env = append(env, fmt.Sprintf("PATH=%s", path))
}
if conf, ok := os.LookupEnv("CONTAINERS_CONF"); ok {
env = append(env, fmt.Sprintf("CONTAINERS_CONF=%s", conf))
}
if conf, ok := os.LookupEnv("CONTAINERS_CONF_OVERRIDE"); ok {
env = append(env, fmt.Sprintf("CONTAINERS_CONF_OVERRIDE=%s", conf))
}
if conf, ok := os.LookupEnv("CONTAINERS_HELPER_BINARY_DIR"); ok {
env = append(env, fmt.Sprintf("CONTAINERS_HELPER_BINARY_DIR=%s", conf))
}
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 != "" {
env = append(env, fmt.Sprintf("HOME=%s", home))
runtimeDir, err := util.GetRuntimeDir()
if err != nil {
return nil, err
}

return env
res = append(res, "XDG_RUNTIME_DIR="+runtimeDir)
return res, nil
}

// sharedConmonArgs takes common arguments for exec and create/restore and formats them for the conmon CLI
Expand Down Expand Up @@ -1422,25 +1404,6 @@ func (r *ConmonOCIRuntime) sharedConmonArgs(ctr *Container, cuuid, bundlePath, p
return args
}

func startCommand(cmd *exec.Cmd, ctr *Container) error {
// Make sure to unset the NOTIFY_SOCKET and reset it afterwards if needed.
switch ctr.config.SdNotifyMode {
case define.SdNotifyModeContainer, define.SdNotifyModeIgnore:
if prev := os.Getenv("NOTIFY_SOCKET"); prev != "" {
if err := os.Unsetenv("NOTIFY_SOCKET"); err != nil {
logrus.Warnf("Error unsetting NOTIFY_SOCKET %v", err)
}
defer func() {
if err := os.Setenv("NOTIFY_SOCKET", prev); err != nil {
logrus.Errorf("Resetting NOTIFY_SOCKET=%s", prev)
}
}()
}
}

return cmd.Start()
}

// newPipe creates a unix socket pair for communication.
// Returns two files - first is parent, second is child.
func newPipe() (*os.File, *os.File, error) {
Expand Down
13 changes: 5 additions & 8 deletions libpod/oci_conmon_exec_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/containers/podman/v4/libpod/define"
"github.com/containers/podman/v4/pkg/errorhandling"
"github.com/containers/podman/v4/pkg/lookup"
"github.com/containers/podman/v4/pkg/util"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/sirupsen/logrus"
"golang.org/x/sys/unix"
Expand Down Expand Up @@ -374,11 +373,6 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
}
}()

runtimeDir, err := util.GetRuntimeDir()
if err != nil {
return nil, nil, err
}

finalEnv := make([]string, 0, len(options.Env))
for k, v := range options.Env {
finalEnv = append(finalEnv, fmt.Sprintf("%s=%s", k, v))
Expand Down Expand Up @@ -438,7 +432,10 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
// }
// }

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

var filesToClose []*os.File
if options.PreserveFDs > 0 {
Expand All @@ -461,7 +458,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
Setpgid: true,
}

err = startCommand(execCmd, c)
err = execCmd.Start()

// We don't need children pipes on the parent side
errorhandling.CloseQuiet(childSyncPipe)
Expand Down
3 changes: 2 additions & 1 deletion test/system/030-run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,7 +1288,7 @@ search | $IMAGE |
done < <(parse_table "$tests")
}

@test "podman --syslog passed to conmon" {
@test "podman --syslog and environment passed to conmon" {
skip_if_remote "--syslog is not supported for remote clients"
skip_if_journald_unavailable

Expand All @@ -1298,6 +1298,7 @@ search | $IMAGE |
run_podman container inspect $cid --format "{{ .State.ConmonPid }}"
conmon_pid="$output"
is "$(< /proc/$conmon_pid/cmdline)" ".*--exit-command-arg--syslog.*" "conmon's exit-command has --syslog set"
assert "$(< /proc/$conmon_pid/environ)" =~ "BATS_TEST_TMPDIR" "entire env is passed down to conmon (incl. BATS variables)"

run_podman rm -f -t0 $cid
}
Expand Down