nixos/nextcloud: use LoadCredential to read secrets #367433
Conversation
github-actions
bot
added
6.topic: nixos
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
b15113c to
6587fd2
Compare
github-actions
bot
added
10.rebuild-darwin: 0
|
This should be generally well tested already so reviews are encouraged, undrafting when
|
|
Thanks! |
networkException
force-pushed
the
nextcloud-loadcredential
branch
2 times, most recently
from
6c60763 to
6fece8a
Compare
There was a problem hiding this comment.
Can you explain why you added a fixme for that?
There was a problem hiding this comment.
Maybe a note is better, I added this after removing the explicit (somewhat duplicate) variable from the nextcloud-update-db service
There was a problem hiding this comment.
I think the comment is good for clarification, but I don't really see this as something super-actionable, so I think I'd prefer NOTE: here.
There was a problem hiding this comment.
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
fd5180a to
dedb0ac
Compare
networkException
force-pushed
the
nextcloud-loadcredential
branch
3 times, most recently
from
3d64615 to
db42190
Compare
|
Rebased onto |
|
So the code mostly looks reasonable to me and the tests are passing. |
Ma27
left a comment
Ma27
left a comment
There was a problem hiding this comment.
Works as expected, thanks 👍
No approval yet though because
- I think a brief entry in the release notes makes sense (I'm probably not the only one with a heavily configured Nextcloud and I had to adjust some parts of my config. Doesn't need much detail in this case, but I'd like to see it mentioned nonetheless)
- The notify_push thing I don't know much abotu is still unresolved (https://github.com/NixOS/nixpkgs/pull/367433/files#r1895052681): cc @SuperSandro2000 @dasJ @helsinki-Jo
Also cc @britter @dotlambda you may also be interested in taking a look.
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
db42190 to
e08ac4c
Compare
…-and-secrets This patch adds a subtest and corresponding configuration to with-declarative-redis-and-secrets to test for nextcloud notify_push to be working, just as in with-postgresql-and-redis. As notify_push needs to connect to the database, including it in this test checks that it can read the dbpassFile properly.
networkException
force-pushed
the
nextcloud-loadcredential
branch
from
e08ac4c to
549d8a6
Compare
|
Rebased onto |
What did you need to change? |
Also, I had another service for generating previews for new images as a systemd timer and inherited |
…ible change in the 25.05 release notes This patch adds a release note entry to the 25.05 release about the use of systemd credentials to read in secrets. It's part of the backward incompatibilities section as changes to the behavior of `nextcloud-occ` might break existing scripts.
github-actions
bot
added
8.has: documentation
`nextcloud-notify_push.service` requires `nextcloud-notify_push-setup.service`. If the latter fails (e.g. because of Nextcloud not being there yet), the push service would also fail with result 'dependency'. RestartMode=direct doesn't put a unit into failed state IF it's about to be restarted again. That way, `nextcloud-notify_push` will await several restart attempts. Only if the unit fails due to a rate-limit (i.e. too many restarts), the push service will also fail. If the startup is still too slow, it may make sense for administrators to configure higher intervals between the start attempts with RestartSec.
Ma27
left a comment
Ma27
left a comment
There was a problem hiding this comment.
I deploy my Nextcloud with these patches on top for ~2 months now. I'm getting 'eepy now, so I'll merge tomorrow afternoon.
If people still have reservations about this, please speak up.
|
I just cherry-picked that into my local nixos-unstable fork and shellcheck fails. I will do a follow up PR that fixes that. |
Where is the shellcheck even running? Tests were all building fwiw. |
|
I have systemd.enableStrictShellChecks |
|
Oof... that sounds like an invitation for random build failures whenever one updates their systems. I mean, I'll merge since it doesn't hurt and will fix people's setup with this option, but... I'm not sure if this option is a good idea in the first place. |
|
I have it on since a while and after the initial fixes I barely get any regressions. Maybe every 2 to 4 weeks. It is surprising. 😂 |
4 participants
This pull request adds support for using systemd's
LoadCredential=feature to read various secret files used by nextcloud service units.Previously credentials had to be readable by the nextcloud user, this is now no longer required.
The
nextcloud-occwrapper script has been adjusted to usesystemd-runfor loading credentials when being called from outside a service.See the individual commits for more details.
Depends on #365442
Continuation of #152141
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.