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.

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

pass LISTEN_* environment into container #11316

Merged
merged 1 commit into from
Aug 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 12 additions & 0 deletions libpod/container_internal_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -773,6 +773,18 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
}
}

// Pass down the LISTEN_* environment (see #10443).
for _, key := range []string{"LISTEN_PID", "LISTEN_FDS", "LISTEN_FDNAMES"} {
if val, ok := os.LookupEnv(key); ok {
// Force the PID to `1` since we cannot rely on (all
// versions of) all runtimes to do it for us.
if key == "LISTEN_PID" {
val = "1"
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the user passed --init, then the main pid will be 2. Who is supposed to rewrite that. For sure catatonit (currently used on e.g. fedora) doesn't do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@alexlarsson This is been fixed in underlying runtime via containers/crun#721 if catatonit sets LISTEN_PID=2 crun will adhere it. We can remove this explicit LISTEN_PID=1 once latest crun is released and vendor-ed into the Podman CI.

However following edge case still remains if OCI runtime is runc this is only fixed for crun. cc @giuseppe

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point, @ alexlarsson. I think Podman should set it since we should not (and to a certain degree cannot) rely on the runtimes or inits to do this work.

Mind opening a PR against Podman?

Copy link
Contributor

Choose a reason for hiding this comment

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

ftr, @giuseppe is fixing catatonit here: openSUSE/catatonit#15

}
g.AddProcessEnv(key, val)
}
}

return g.Config, nil
}

Expand Down
3 changes: 1 addition & 2 deletions libpod/oci_conmon_exec_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
// }
// }

conmonEnv, extraFiles := r.configureConmonEnv(c, runtimeDir)
conmonEnv := r.configureConmonEnv(c, runtimeDir)

var filesToClose []*os.File
if options.PreserveFDs > 0 {
Expand All @@ -456,7 +456,6 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
execCmd.Env = append(execCmd.Env, conmonEnv...)

execCmd.ExtraFiles = append(execCmd.ExtraFiles, childSyncPipe, childStartPipe, childAttachPipe)
execCmd.ExtraFiles = append(execCmd.ExtraFiles, extraFiles...)
execCmd.Dir = c.execBundlePath(sessionID)
execCmd.SysProcAttr = &syscall.SysProcAttr{
Setpgid: true,
Expand Down
44 changes: 22 additions & 22 deletions libpod/oci_conmon_linux.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (
"github.com/containers/podman/v3/utils"
"github.com/containers/storage/pkg/homedir"
pmount "github.com/containers/storage/pkg/mount"
"github.com/coreos/go-systemd/v22/activation"
"github.com/coreos/go-systemd/v22/daemon"
spec "github.com/opencontainers/runtime-spec/specs-go"
"github.com/opencontainers/selinux/go-selinux"
Expand Down Expand Up @@ -66,7 +65,6 @@ type ConmonOCIRuntime struct {
supportsJSON bool
supportsKVM bool
supportsNoCgroups bool
sdNotify bool
enableKeyring bool
}

Expand Down Expand Up @@ -105,7 +103,6 @@ func newConmonOCIRuntime(name string, paths []string, conmonPath string, runtime
runtime.logSizeMax = runtimeCfg.Containers.LogSizeMax
runtime.noPivot = runtimeCfg.Engine.NoPivotRoot
runtime.reservePorts = runtimeCfg.Engine.EnablePortReservation
runtime.sdNotify = runtimeCfg.Engine.SDNotify
runtime.enableKeyring = runtimeCfg.Containers.EnableKeyring

// TODO: probe OCI runtime for feature and enable automatically if
Expand Down Expand Up @@ -1050,8 +1047,22 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
}
}

if ctr.config.PreserveFDs > 0 {
args = append(args, formatRuntimeOpts("--preserve-fds", fmt.Sprintf("%d", ctr.config.PreserveFDs))...)
// Pass down the LISTEN_* environment (see #10443).
preserveFDs := ctr.config.PreserveFDs
if val := os.Getenv("LISTEN_FDS"); val != "" {
if ctr.config.PreserveFDs > 0 {
logrus.Warnf("Ignoring LISTEN_FDS to preserve custom user-specified FDs")
} else {
fds, err := strconv.Atoi(val)
if err != nil {
return fmt.Errorf("converting LISTEN_FDS=%s: %w", val, err)
}
preserveFDs = uint(fds)
}
}

if preserveFDs > 0 {
args = append(args, formatRuntimeOpts("--preserve-fds", fmt.Sprintf("%d", preserveFDs))...)
}

if restoreOptions != nil {
Expand Down Expand Up @@ -1104,11 +1115,11 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
}

// 0, 1 and 2 are stdin, stdout and stderr
conmonEnv, envFiles := r.configureConmonEnv(ctr, runtimeDir)
conmonEnv := r.configureConmonEnv(ctr, runtimeDir)

var filesToClose []*os.File
if ctr.config.PreserveFDs > 0 {
for fd := 3; fd < int(3+ctr.config.PreserveFDs); fd++ {
if preserveFDs > 0 {
for fd := 3; fd < int(3+preserveFDs); fd++ {
f := os.NewFile(uintptr(fd), fmt.Sprintf("fd-%d", fd))
filesToClose = append(filesToClose, f)
cmd.ExtraFiles = append(cmd.ExtraFiles, f)
Expand All @@ -1118,10 +1129,9 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
cmd.Env = r.conmonEnv
// we don't want to step on users fds they asked to preserve
// Since 0-2 are used for stdio, start the fds we pass in at preserveFDs+3
cmd.Env = append(cmd.Env, fmt.Sprintf("_OCI_SYNCPIPE=%d", ctr.config.PreserveFDs+3), fmt.Sprintf("_OCI_STARTPIPE=%d", ctr.config.PreserveFDs+4))
cmd.Env = append(cmd.Env, fmt.Sprintf("_OCI_SYNCPIPE=%d", preserveFDs+3), fmt.Sprintf("_OCI_STARTPIPE=%d", preserveFDs+4))
cmd.Env = append(cmd.Env, conmonEnv...)
cmd.ExtraFiles = append(cmd.ExtraFiles, childSyncPipe, childStartPipe)
cmd.ExtraFiles = append(cmd.ExtraFiles, envFiles...)

if r.reservePorts && !rootless.IsRootless() && !ctr.config.NetMode.IsSlirp4netns() {
ports, err := bindPorts(ctr.config.PortMappings)
Expand Down Expand Up @@ -1225,7 +1235,7 @@ 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(ctr *Container, runtimeDir string) ([]string, []*os.File) {
func (r *ConmonOCIRuntime) configureConmonEnv(ctr *Container, runtimeDir string) []string {
var env []string
for _, e := range os.Environ() {
if strings.HasPrefix(e, "LC_") {
Expand All @@ -1240,17 +1250,7 @@ func (r *ConmonOCIRuntime) configureConmonEnv(ctr *Container, runtimeDir string)
env = append(env, fmt.Sprintf("HOME=%s", home))
}

extraFiles := make([]*os.File, 0)
if !r.sdNotify {
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
if listenfds, ok := os.LookupEnv("LISTEN_FDS"); ok {
env = append(env, fmt.Sprintf("LISTEN_FDS=%s", listenfds), "LISTEN_PID=1")
fds := activation.Files(false)
extraFiles = append(extraFiles, fds...)
}
} else {
logrus.Debug("disabling SD notify")
}
return env, extraFiles
return env
}

// sharedConmonArgs takes common arguments for exec and create/restore and formats them for the conmon CLI
Expand Down
42 changes: 42 additions & 0 deletions test/system/250-systemd.bats
Original file line number Diff line number Diff line change
Expand Up @@ -136,4 +136,46 @@ function service_cleanup() {
service_cleanup
}

function set_listen_env() {
export LISTEN_PID="100" LISTEN_FDS="1" LISTEN_FDNAMES="listen_fdnames"
}

function unset_listen_env() {
unset LISTEN_PID LISTEN_FDS LISTEN_FDNAMES
}

function check_listen_env() {
local stdenv="$1"
local context="$2"
if is_remote; then
is "$output" "$stdenv" "LISTEN Environment did not pass: $context"
else
is "$output" "$stdenv
LISTEN_PID=1
LISTEN_FDS=1
LISTEN_FDNAMES=listen_fdnames" "LISTEN Environment passed: $context"
fi
}

@test "podman pass LISTEN environment " {
# Note that `--hostname=host1` makes sure that all containers have the same
# environment.
run_podman run --hostname=host1 --rm $IMAGE printenv
stdenv=$output

# podman run
set_listen_env
run_podman run --hostname=host1 --rm $IMAGE printenv
unset_listen_env
check_listen_env "$stdenv" "podman run"

# podman start
run_podman create --hostname=host1 --rm $IMAGE printenv
cid="$output"
set_listen_env
run_podman start --attach $cid
unset_listen_env
check_listen_env "$stdenv" "podman start"
}

# vim: filetype=sh