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

Systemd journal remote and related #189277

Merged
merged 10 commits into from
Dec 10, 2023

Conversation

minijackson
Copy link
Member

@minijackson minijackson commented Sep 1, 2022

Description of changes

This adds NixOS services for systemd's journal remote/upload/gateway features, which allow for uploading the journal over the network.

GnuTLS was added in the systemd closure if withRemote is true. Without it, no certificate checking would be done.

The added tests work on master, but I didn't test them when based off of staging, due to the number of builds.

The closure size of systemd is not increased, because libmicrohttpd already depends on GnuTLS.

Note that this only adds "push"-style journal uploads. I think the pull mode might need one service for each pull, and I wanted to keep it simple at first.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@minijackson minijackson requested a review from a team as a code owner September 1, 2022 14:08
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 1, 2022
@ofborg ofborg bot requested review from kloenk, Mic92 and flokli September 1, 2022 14:18
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ labels Sep 1, 2022
@minijackson
Copy link
Member Author

As per the comment, I removed the "static" generation of certificates for the NixOS tests, and with some slight tweaking, managed to generate them in the testScripts

@ofborg ofborg bot added 10.rebuild-darwin: 101-500 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Apr 25, 2023
@SuperSandro2000 SuperSandro2000 removed their request for review May 3, 2023 22:20
@Skarlett
Copy link

added on my fork & synced occasionally

@flokli
Copy link
Contributor

flokli commented Jun 17, 2023

It seems i didn't see this so far, apologies for the lack of response.

For this to be merged, the section in the release notes would need to be moved to the next release.

I'm also not sure if gnutls is still required for more recent versions of systemd, can you check if it's still needed?

@RaitoBezarius
Copy link
Member

@minijackson Do you mind if I take this over and bring it to completion?

@RaitoBezarius
Copy link
Member

This is needed to exploit multi-node journal centralization in netdata.

@RaitoBezarius
Copy link
Member

I'm also not sure if gnutls is still required for more recent versions of systemd, can you check if it's still needed?

Yes it does, gateway test will fail otherwise with:

client # [    8.166503] systemd-journal-gatewayd[1017]: /run/secrets/client/key.pem has 0644 mode that is too permissive, please adjust the ownership and access mode.
client # [    8.167762] systemd-journal-gatewayd[1017]: Option --trust= is not available.
client # [    8.171842] systemd[1]: systemd-journal-gatewayd.service: Main process exited, code=exited, status=1/FAILURE
client # [    8.172825] systemd[1]: systemd-journal-gatewayd.service: Failed with result 'exit-code'.
client # [    8.181715] systemd[1]: Started Journal Gateway Service.
client # [    8.222356] systemd-journal-gatewayd[1021]: /run/secrets/client/key.pem has 0644 mode that is too permissive, please adjust the ownership and access mode.

@RaitoBezarius
Copy link
Member

Resolved the conflicts, moved the RL, retested the introduced tests, locally. Ready to push on this PR or open a new PR if needed.

@minijackson
Copy link
Member Author

@RaitoBezarius sure, go ahead! Sorry, I having been able to finish this, I have so much to do these days…

@RaitoBezarius
Copy link
Member

@RaitoBezarius sure, go ahead! Sorry, I having been able to finish this, I have so much to do these days…

Thank you, done!

@flokli
Copy link
Contributor

flokli commented Dec 9, 2023

Cross-linking to my comment at ae896e0#commitcomment-133828003:

I asked about the maintenance state of it in https://www.mail-archive.com/systemd-devel@lists.freedesktop.org/msg45126.html, upstream is hesitant to drop it, but the whole HTTP stack definitely is very untested. It's still using gnutls, and systemd plans to unify on openssl.

I'm glad we now have VM tests to provide some regression testing downstream in NixOS at least, thank you for that!

flokli referenced this pull request Dec 9, 2023
This disabled systemd-journal-upload and systemd-journal-remote.

We didn't install the unit files anyways, so this was probably not used
at all, and currently fails to build due to libmicrohttpd and systemd
code being incompatible:

```
../src/journal-remote/journal-remote-main.c: In function ‘setup_microhttpd_server’:
../src/journal-remote/journal-remote-main.c:431:38: error: passing argument 5 of ‘MHD_start_daemon’ from incompatible pointer type [-Werror=incompatible-pointer-types]
```
@RaitoBezarius
Copy link
Member

Cross-linking to my comment at ae896e0#commitcomment-133828003:

I asked about the maintenance state of it in mail-archive.com/systemd-devel@lists.freedesktop.org/msg45126.html, upstream is hesitant to drop it, but the whole HTTP stack definitely is very untested. It's still using gnutls, and systemd plans to unify on openssl.

I'm glad we now have VM tests to provide some regression testing downstream in NixOS at least, thank you for that!

I can answer easily that systemd journald is going to very well maintained as it's now part of the core product of netdata and they have been contributing many fixes and changes: https://learn.netdata.cloud/docs/logs/systemd-journal/passive-journal-centralization-with-encryption-using-self-signed-certificates etc.

@flokli
Copy link
Contributor

flokli commented Dec 9, 2023

Yes, this was no objection to merging this PR. I just wanted to make sure the threads are linked. If you intend to maintain this in NixOS, go ahead :-D Please add yourself as a maintainer for that NixOS module though ;-)

@RaitoBezarius
Copy link
Member

Yes, this was no objection to merging this PR. I just wanted to make sure the threads are linked. If you intend to maintain this in NixOS, go ahead :-D Please add yourself as a maintainer for that NixOS module though ;-)

Done.

@RaitoBezarius RaitoBezarius merged commit 9faaff8 into NixOS:staging Dec 10, 2023
21 of 22 checks passed
@minijackson minijackson deleted the systemd-journal-upload branch December 10, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: systemd 8.has: changelog 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants