-
Notifications
You must be signed in to change notification settings - Fork 98
Update systemd unit file #1218
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
base: main
Are you sure you want to change the base?
Update systemd unit file #1218
Conversation
ExecStartPre=/bin/mkdir -p ${AGENT_RUN_DIR} | ||
ExecStartPre=/bin/mkdir -p ${AGENT_LOG_DIR} | ||
|
||
ExecStartPre=/bin/mkdir -p ${AGENT_RUN_DIR} ${AGENT_LOG_DIR} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run directory
Traditionally this is used to hold a pid file for daemonizing processes and sometimes socket files for listeners. nginx-agent isn't self-daemonizing (double-fork).
Listen sockets: is there a use case of setting up a syslog listener in /run/nginx-agent/syslog.socket
or something like that?
Looking for uses of of run/nginx-agent
in the codebase I can find: https://github.com/nginx/agent/blob/main/internal/watcher/instance/instance_watcher_service.go#L31 that is used as the default for processPath
(https://github.com/nginx/agent/blob/main/internal/watcher/instance/instance_watcher_service.go#L302) -- but that's supposed to be the path for the binary so this default isn't very sound. There's a related test file entry.
This feels like an easy cross-off to me.
Logs Directory
My preference at this point is always to turn file logging off and give that to the journal that has really good built in rotation to not run the disk out of space. Not everyone loves the journal though; I don't know if you have a policy on this.
I see also that it is used as the default for opentelemetry logging (https://github.com/nginx/agent/blob/main/internal/config/defaults.go#L78). Mostly the same question -- does opentelemetry need to log to a file by default or can that go to stderr (journal) and be user-configurable to send elsewhere at need?
I could see keeping this, but maybe move this directory creation to postinstall.sh
so that if an admin is trying to limit what directories nginx-agent can access it doesn't trigger with failures on /var/log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review this with the Agent team and follow up. I can think of 1 or 2 cases where Agent might want to open a listening socket. One of them as you said is a syslog receiver
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a use-case then by all means keep it-- I'm just generally trying to shed privileges.
I don't want to get too hung up here; if you want to just move forward don't let me stop you. Here's a bunch more though:
systemd option
You can also get systemd to build these directories with RuntimeDirectory= (and LogsDirectory=). This has a small benefit that I don't need to grant this service permission to /var/run
| /run
directly that this ExecStartPre=
needs right now
E.g. on my ubuntu 24.04 system right now:
# Clear out nginx-agent trying to mkdir /var/log/nginx-agent /var/run/nginx-agent
ExecStartPre=
# otel tries to log here
LogsDirectory=nginx-agent
# not particularly used
RuntimeDirectory=nginx-agent
# store manifest information
StateDirectory=nginx-agent
#- /var/ construction
TemporaryFileSystem=/var:ro
BindPaths=/var/www
BindPaths=/var/log/nginx
# similar tmpfs construct for /etc:ro omitted
I'm blocking off nginx-agent from seeing anything else in /var
or /etc
that I'm not explicitly granting it.
E.g.
root@dpvm-h2tm:~# systemctl status nginx-agent | grep PID
Main PID: 59236 (nginx-agent)
root@dpvm-h2tm:~# cat /proc/59236/mounts | grep var
tmpfs /var tmpfs ro,nosuid,nodev,size=98148k,nr_inodes=409600,mode=755,inode64 0 0
/dev/root /var/lib/nginx-agent ext4 rw,nosuid,relatime,discard,errors=remount-ro,commit=30 0 0
/dev/root /var/log/nginx ext4 rw,nosuid,relatime,discard,errors=remount-ro,commit=30 0 0
/dev/root /var/log/nginx-agent ext4 rw,nosuid,relatime,discard,errors=remount-ro,commit=30 0 0
/dev/root /var/tmp ext4 rw,nosuid,relatime,discard,errors=remount-ro,commit=30 0 0
/dev/root /var/www ext4 rw,nosuid,relatime,discard,errors=remount-ro,commit=30 0 0
# Create a file just to demo this is a shared bind
root@dpvm-h2tm:~# touch /var/log/nginx-agent/foo
# run a shell in that mount namespace
root@dpvm-h2tm:~# nsenter --target 59236 --mount /bin/bash
root@dpvm-h2tm:/# ls -hl /var/
total 8.0K
drwxr-xr-x 3 root root 60 Aug 21 14:13 lib
drwxr-xr-x 4 root root 80 Aug 21 14:13 log
drwxrwxrwt 2 root root 4.0K Aug 21 14:13 tmp
drwxr-xr-x 2 root root 4.0K Aug 20 17:22 www
root@dpvm-h2tm:/# ls -hl /var/log/nginx-agent
total 0
-rw-r--r-- 1 root root 0 Aug 21 14:14 foo
-rw------- 1 root nginx-agent 0 Aug 21 14:13 otel.log
root@dpvm-h2tm:/# rm /var/log/nginx-agent/foo
If I remove the ExecStartPre=
that blanks out the upstream version, it fails because it can't create a directory on a read-only filesystem (the /var/ tmpfs):
root@dpvm-h2tm:~# sed -i '/ExecStartPre=/d' /etc/systemd/system/nginx-agent.service.d/fixups.conf
root@dpvm-h2tm:~# systemctl daemon-reload
root@dpvm-h2tm:~# systemctl restart nginx-agent
Job for nginx-agent.service failed because the control process exited with error code.
See "systemctl status nginx-agent.service" and "journalctl -xeu nginx-agent.service" for details.
root@dpvm-h2tm:~# systemctl status nginx-agent
● nginx-agent.service - require nginx-agent-vars.service
Loaded: loaded (/etc/systemd/system/nginx-agent.service; enabled; preset: enabled)
Drop-In: /etc/systemd/system/nginx-agent.service.d
└─fixups.conf, sandbox.conf, vars.conf
Active: activating (auto-restart) (Result: exit-code) since Thu 2025-08-21 14:28:28 UTC; 1s ago
Docs: https://github.com/nginx/agent#readme
Process: 59718 ExecStartPre=/bin/mkdir -p /var/run/nginx-agent (code=exited, status=1/FAILURE)
CPU: 18ms
Proposed changes
Added
chown
command in case Agent runs as non-root. The ExecStartPre commands run as root and will create the directories as root. If Agent needs to run as a different user it wouldn't be able to write logs.Removed StandardOutput and StandardError lines. They will now just fall back to default
journal
Removed
Environment=
as this will clear all environment variables. There are cases likeHTTP_PROXY
where we may want environment variables not cleared.Added some optional security directives:
NoNewPrivilages=true
prevents privilege escalation via setuidProtectHome=off
Disables home protection (hides /home from Agent) Users may opt to run NGINX in /home/userFixes: #1209
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTING
documentmake install-tools
and have attached any dependency changes to this pull requestREADME.md
)