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

Support systemd socket activation #429

Merged

Conversation

charliemirabile
Copy link
Contributor

Fixes: #428

This is my first time writing go code, so go easy on me :)

I tried to break this down into a clear set of commits, so the actual logic I want to implement is easy to follow separate from the necessary refactoring I did. Happy to squash this down into one commit though if that is the etiquette around here.

Before editing this code, it will be helpful to factor it out first.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
The environment variable contained hardcoded values of 3 and 4 for the file
descriptors. If more file desciptors will be shared the pipes might no longer
be file descriptors 3 and 4.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
since (an unknown amount) more values will be added to cmd.ExtraFiles, making
a dynamic array and slotting the two values in to specific indices (and then
referencing those indices in the environment variable) will make things more
flexible.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
introduce a variable to hold a count of files from systemd. this value is always zero for now,
but will come to hold the count of how many file descriptors we inherited from systemd

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
…ent variable

If this environemnt variable is set and the value is an integer n, we should arrange to
have the child inherit a copy of our file descriptors 3, 4, ... 3 + n.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
…mespace

The child will inheret the file descriptors from the parent, but it needs to
pass them along to its child, the actual target process within the environment.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
@AkihiroSuda
Copy link
Member

Thanks, how to test this?

@AkihiroSuda AkihiroSuda added this to the v2.0.3 milestone Mar 29, 2024
@charliemirabile
Copy link
Contributor Author

charliemirabile commented Mar 29, 2024

In order to test, systemd provides a program systemd-socket-activate that can be used to simulate starting a service with socket activation. I used the docker daemon as an example.

I needed to modify the script that is used to spawn it with rootless kit (the default provided for installing dockerd rootless) by adding the line export LISTEN_PID=$$ right above the line that actually execs dockerd exec "$dockerd" "$@" and then I could invoke it with the systemd program as follows: systemd-socket-activate -l /tmp/docker.sock /path/to/dockerd-rootless.sh -H fd://.

The systemd program will create /tmp/docker.sock and be listening on it waiting for a connection. I can then attempt to connect to it from a different terminal with docker (with DOCKER_HOST=unix:///tmp/docker.sock) or with the following curl command curl -H "Content-Type: application/json" --unix-socket /tmp/docker.sock http://localhost/_ping.

Assuming all is well you will the the daemon startup messages in the original terminal and after a short delay, you will see the expected output of the docker command or the successful ping reponse OK.

@AkihiroSuda
Copy link
Member

Thanks, is it testable without depending on Docker?
Can we have a test script like https://github.com/rootless-containers/rootlesskit/blob/v2.0.2/hack/integration-evacuate-cgroup2.sh#L4 ?

@charliemirabile
Copy link
Contributor Author

charliemirabile commented Mar 29, 2024

yes, you could write a script like this:

#!/bin/sh
read -r REQUEST
if [ "$(printf 'GET /hello HTTP/1.1\r\n')" = "$REQUEST" ]
then
    printf 'HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\nHello\n'
else
    printf 'HTTP/1.1 400 Bad Request\r\nContent-Length: 5\r\n\r\nBad!\n'
fi

and then do systemd-socket-activate -l /tmp/test.sock socat ACCEPT-FD:3 EXEC:./test.sh,nofork and then verify that you get Hello back from curl --unix-socket /tmp/test.sock https://localhost/hello

@charliemirabile
Copy link
Contributor Author

Ok, I have a working testing script, but I am struggling to add it to the existing integration testing framework.

diff --git a/hack/integration-systemd-socket.sh b/hack/integration-systemd-socket.sh
new file mode 100755
index 0000000..1129a72
--- /dev/null
+++ b/hack/integration-systemd-socket.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+set -e
+if [ -z "$EXECED" ]
+then
+       systemd-socket-activate -E EXECED=1 -l /tmp/activate.sock socat ACCEPT-FD:3 EXEC:"rootlesskit $0",nofork 2>/dev/null &
+       OUTPUT="$(curl --unix-socket /tmp/activate.sock http://localhost/hello 2>/dev/null)"
+       [ "$(printf 'Hello\n' )" = "$OUTPUT" ] || exit 1
+else
+       [ "$LISTEN_FDS" = "1" ] || exit 1
+       read -r REQUEST
+       if [ "$(printf 'GET /hello HTTP/1.1\r\n')" = "$REQUEST" ]
+       then
+           printf 'HTTP/1.1 200 OK\r\nContent-Length: 6\r\n\r\nHello\n'
+       else
+           printf 'HTTP/1.1 400 Bad Request\r\nContent-Length: 5\r\n\r\nBad!\n'
+       fi
+fi

I need a newer version of socat (v1.8.0 instead of v.1.7.4) which is available in ubuntu 24.04 but not 22.04. I feel like bumping the ubuntu version used in the Dockerfile across the board would probably be outside of the scope of this PR (especially because it isn't trivial to do). Is it ok for me to just add the testing script without adding it to the CI, or should I make a separate PR and try to bump the ubuntu version and then come back to this?

@AkihiroSuda
Copy link
Member

Integration to CI can be worked out later.
https://github.com/rootless-containers/rootlesskit/blob/v2.0.2/hack/integration-evacuate-cgroup2.sh isn’t integrated to CI either, as the current Dockerfile lacks systemd

This test is not currently run in CI, but it can be run locally.

Signed-off-by: charliemirabile <46761267+charliemirabile@users.noreply.github.com>
@charliemirabile
Copy link
Contributor Author

Alright, just pushed one more commit with the script

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 2356582 into rootless-containers:master Mar 30, 2024
5 checks passed
@charliemirabile charliemirabile deleted the systemd_activation branch March 30, 2024 19:15
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.

Sockets from systemd activation not inherited by child
2 participants