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

Add execution branch for socket activation to correct LISTEN_PID #449

Merged

Conversation

alopukhov
Copy link
Contributor

@alopukhov alopukhov commented Jul 5, 2024

Proposal on #448
Fixing LISTEN_PID env variable for target process if it equals to PID of [rootlless:parent] process.

Tested with dockerd. Extra $$ script is not required anymore.

systemd-socket-activate -E "XDG_RUNTIME_DIR=${XDG_RUNTIME_DIR}" -l 12312 \
./rootlesskit/rootlesskit --net=slirp4netns --slirp4netns-sandbox=true \
--slirp4netns-seccomp=true \
--disable-host-loopback \
--port-driver=slirp4netns \
--copy-up=/etc \
--copy-up=/run \
sh -c 'rm /run/docker ; exec dockerd -H fd://'

Can be demonstated.

> (timeout 10 systemd-socket-activate -l 12345 ./rootlesskit --pidns sh -c 'echo "\$\$ is $$; LISTEN_PID is $LISTEN_PID"' &); sleep 1; netcat localhost 12345
Listening on [::]:12345 as 3.
Communication attempt on fd 3.
Execing ./rootlesskit (./rootlesskit --pidns sh -c echo "\$\$ is $$; LISTEN_PID is $LISTEN_PID")
$$ is 11; LISTEN_PID is 11

Example output for v2.1.0:

(timeout 10 systemd-socket-activate -l 12345 ./rootlesskit --pidns sh -c 'echo "\$\$ is $$; LISTEN_PID is $LISTEN_PID"' &); sleep 1; netcat localhost 12345
Listening on [::]:12345 as 3.
Communication attempt on fd 3.
Execing ./rootlesskit (./rootlesskit --pidns sh -c echo "\$\$ is $$; LISTEN_PID is $LISTEN_PID")
$$ is 11; LISTEN_PID is 3868868

@alopukhov alopukhov force-pushed the better_socket_activation branch 3 times, most recently from d855821 to 3a3d6b5 Compare July 6, 2024 14:07
@AkihiroSuda AkihiroSuda added this to the v2.1.1 milestone Jul 8, 2024
@AkihiroSuda
Copy link
Member

cc @charliemirabile

cmd/rootlesskit/main.go Outdated Show resolved Hide resolved
cmd/rootlesskit/main.go Outdated Show resolved Hide resolved
@charliemirabile
Copy link
Contributor

Looks good. Thanks for picking up where I left off. I was hesitant to introduce another whole process into the already somewhat complex way that rootlesskit launches the app as part of #429 especially for a first contribution and given my lack of experience with Go.

It is unfortunate that fork and exec are so tightly coupled in Go (though this of course helps with making the language suitable for cross platform development for other projects), since in C, it would be as easy as inserting a call to setenv in the code between fork and exec whereas here we need to introduce a whole other program as a shim and successfully plumb all the relevant info through it, but your implementation looks sane.

iAmChild := os.Getenv(pipeFDEnvKey) != ""
id := "parent"
if iAmChild {
id = "child " // padded to len("parent")
} else if iAmActivationHelper {
id = "activ."
Copy link
Member

Choose a reason for hiding this comment

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

What is this dot symbol?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with activation_helper literal.

cmd/rootlesskit/main.go Outdated Show resolved Hide resolved
@AkihiroSuda
Copy link
Member

Could you squash the commits?

Signed-off-by: Andrei Lopukhov <andrewlopukhov@gmail.com>
@alopukhov alopukhov force-pushed the better_socket_activation branch from d0094f0 to ca26493 Compare July 15, 2024 21:22
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit 3cb66b7 into rootless-containers:master Jul 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants