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

Pass Systemd LISTEN_* environment to the container #10448

Closed
wants to merge 1 commit into from
Closed

Pass Systemd LISTEN_* environment to the container #10448

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
12 changes: 12 additions & 0 deletions libpod/container_internal_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -648,6 +648,18 @@ func (c *Container) generateSpec(ctx context.Context) (*spec.Spec, error) {
g.AddProcessEnv("HOSTNAME", hostname)
}

for _, lEnv := range []string{"LISTEN_PID", "LISTEN_FDS", "LISTEN_FDNAMES"} {
if val, ok := os.LookupEnv(lEnv); ok {
// The primary process within the container will be PID=1, so this
// value needs to be reset
if lEnv == "LISTEN_PID" {
g.AddProcessEnv(lEnv, "1")
continue
}
g.AddProcessEnv(lEnv, val)
}
}

if c.config.UTSNsCtr != "" {
if err := c.addNamespaceContainer(&g, UTSNS, c.config.UTSNsCtr, spec.UTSNamespace); err != nil {
return nil, err
Expand Down
10 changes: 10 additions & 0 deletions libpod/oci_conmon_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1055,6 +1055,16 @@ func (r *ConmonOCIRuntime) createOCIContainer(ctr *Container, restoreOptions *Co
}
}

if l, ok := os.LookupEnv("LISTEN_FDS"); ok {
listenFds, err := strconv.ParseUint(l, 10, 64)
if err != nil {
logrus.Warnf("Error LISTEN_FDS environment variable for %s not an int %v", ctr.ID(), err)
}

if uint(listenFds) > ctr.config.PreserveFDs {
ctr.config.PreserveFDs = uint(listenFds)
}
Comment on lines +1064 to +1066
Copy link
Contributor

Choose a reason for hiding this comment

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

		if uint(listenFds) > ctr.config.PreserveFDs {
			ctr.config.PreserveFDs = uint(listenFds)
		}

seems to be
(pseudo code)

ctr.config.PreserveFDs := max(listenFds, ctr.config.PreserveFDs)

I don't understand this code. Why should it be a max value?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am just guessing here, that if the user specified --preserve_fds=1 and LISTEN_FDS=2, then we need to leak at least 2 FDs into the container. But I am not really sure what should be the behaviour. Perhaps we should ignore the LISTEN_FDS and force the user to tell us how many FDS to leak.

Copy link
Contributor

@eriksjolund eriksjolund Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

At first sight runc and crun are doing the same:
See the runc example:

 LISTEN_PID=$pid_of_runc LISTEN_FDS=3 runc run --preserve-fds 5 <container>

from https://github.com/opencontainers/runc/blob/master/docs/terminals.md

The first 3 (3, 4, and 5) were passed due to LISTEN_FDS and the other 5 (6, 7, 8, 9, and 10)
were passed due to --preserve-fds

Quote: basically, it can just leave the FDs and $LISTEN_FDS untouched
https://systemd.io/CONTAINER_INTERFACE/

I guess runc and crun could have ignored LISTEN_FDS and just make it the responsibility of
the caller to specify a --preserve-fds argument that includes the LISTEN_FDS.

But that is not how it is done right now.

Copy link
Member Author

Choose a reason for hiding this comment

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

So we should do nothing with them except leak them into the OCI Runtimes. Will crun and runc rename the LISTEN_PID to pid1?

Copy link
Member

Choose a reason for hiding this comment

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

That's correct. crun and runc preserve the FDS but only runc sets LISTEN_PID=1 (Cc: @giuseppe @flouthoc).

}
if ctr.config.PreserveFDs > 0 {
args = append(args, formatRuntimeOpts("--preserve-fds", fmt.Sprintf("%d", ctr.config.PreserveFDs))...)
}
Expand Down
39 changes: 39 additions & 0 deletions test/system/250-systemd.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,4 +148,43 @@ function service_cleanup() {
service_cleanup
}

@test "podman pass run LISTEN environment " {
tmpdir=$PODMAN_TMPDIR/build-test
subdir=$tmpdir/subdir
run_podman run --hostname=host1 --rm $IMAGE printenv
std_output=$output

export LISTEN_PID="100" LISTEN_FDS="1" LISTEN_FDNAMES="listen_fdnames"
run_podman run --hostname=host1 --rm $IMAGE printenv
if is_remote; then
is "$output" "$std_output" "LISTEN Environment did not pass"
else
is "$output" "$std_output
LISTEN_PID=1
LISTEN_FDS=1
LISTEN_FDNAMES=listen_fdnames" "LISTEN Environment passed"
fi
unset LISTEN_PID LISTEN_FDS LISTEN_FDNAMES
}

@test "podman pass start LISTEN environment " {
tmpdir=$PODMAN_TMPDIR/build-test
subdir=$tmpdir/subdir
run_podman run --hostname=host1 --rm $IMAGE printenv
std_output=$output

run_podman create --name=test --hostname=host1 --rm $IMAGE printenv
export LISTEN_PID="100" LISTEN_FDS="1" LISTEN_FDNAMES="listen_fdnames"
run_podman start --attach test
if is_remote; then
is "$output" "$std_output" "LISTEN Environment did not pass"
else
is "$output" "$std_output
LISTEN_PID=1
LISTEN_FDS=1
LISTEN_FDNAMES=listen_fdnames" "LISTEN Environment passed"
fi
unset LISTEN_PID LISTEN_FDS LISTEN_FDNAMES
}

# vim: filetype=sh