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

service: Add RuntimeDirectory and LogsDirectory to unit #105

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jbergstroem
Copy link

@jbergstroem jbergstroem commented Dec 9, 2023

Caddy supports listening on a unix socket. To make it easier for users to manage said socket (including permissions), let systemd handle the creation of it.

Caddy supports listening on a unix socket. To make it easier
for users to manage said socket (including permissions),
let systemd handle the creation of it.
@francislavoie
Copy link
Member

francislavoie commented Dec 9, 2023

So if I understand correctly, this will create /run/caddy dir owned by the caddy user?

https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=

I'm realizing there's also LogsDirectory, we should probably use that instead of this stuff:

mkdir -p /var/log/caddy

@jbergstroem
Copy link
Author

So if I understand correctly, this will create /run/caddy dir owned by the caddy user?

freedesktop.org/software/systemd/man/latest/systemd.exec.html#RuntimeDirectory=

Yes.

I'm realizing there's also LogsDirectory, we should probably use that instead of this stuff:

mkdir -p /var/log/caddy

Let me have a look (test) and iterate.

@jbergstroem jbergstroem marked this pull request as draft December 9, 2023 19:34
@jbergstroem
Copy link
Author

Added the line locally and removed the log directory followed by a restart. The log directory was correctly created with 0755 permissions/caddy:caddy as owner. If we want other permissions, I can change that too.

@jbergstroem jbergstroem marked this pull request as ready for review December 9, 2023 19:52
@francislavoie francislavoie changed the title feat: create runtimedirectory on start service: Add RuntimeDirectory and LogsDirectory to unit Dec 9, 2023
Copy link
Member

@francislavoie francislavoie left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

I want to wait for @carlwgeorge's review though, he may have opinions on this.

@francislavoie francislavoie added the enhancement New feature or request label Dec 9, 2023
@jbergstroem
Copy link
Author

Ran into this issue again the other day! Any more input, review wise?

Copy link
Collaborator

@carlwgeorge carlwgeorge left a comment

Choose a reason for hiding this comment

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

I wasn't aware that the deb package was creating a log directory. The rpm doesn't do that. Either way, the default configuration writes process logs to stderr, and doesn't write requests logs at all. It also doesn't listen on a unix socket. I don't think we should be pro-actively creating directories that aren't used by default. I think these directives make more sense as local unit file overrides.

@francislavoie
Copy link
Member

We create the directory so that it's there if users want to use it for access logs. I think it's a common enough need that it's harmless to do so just to save users a step which may be surprising/confusing to require otherwise.

WDYT about the RuntimeDirectory @carlwgeorge? Same idea here, it's just a nice-to-have I think which would save a manual step for most users.

@carlwgeorge
Copy link
Collaborator

It's not surprising or confusing for users who change root to ensure the directory they point it to exists. Why would it be surprising or confusing for users to ensure (or have systemd ensure) the directory for their custom logging or unix socket exists? Sure the directive is harmless, but I'm just not convinced it's necessary in the default unit file for everyone. That combined with the simplicity of unit file overrides for adding local directives makes me lean towards no on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants