Skip to content

Comments

[staging] openldap: remove deprecated options, improve encapsulation#176449

Closed
kwohlfahrt wants to merge 9 commits intoNixOS:stagingfrom
kwohlfahrt:openldap-path
Closed

[staging] openldap: remove deprecated options, improve encapsulation#176449
kwohlfahrt wants to merge 9 commits intoNixOS:stagingfrom
kwohlfahrt:openldap-path

Conversation

@kwohlfahrt
Copy link
Contributor

@kwohlfahrt kwohlfahrt commented Jun 5, 2022 •
edited
Loading

Description of changes

This removes options deprecated in #94610. It also picks up a number of changes from #141240 (cc @poscat0x04), which adds a mutableConfig option and starts slapd under a systemd-controlled user/group, rather than relying on the daemon to drop privileges.

Tests are refactored to make them faster using the specialization mechanism, and improved to cover various use-cases (starting with an empty DB, mutable config, and immutable config). With these changes, starting with an empty DB works in the test, so this might obsolete #92544 (cc @mystfox).

A new change in this PR is to change the default Unix socket for OpenLDAP to /run/openldap/ldapi (from /run/ldapi). The former can be created by openldap running as non-root, the latter cannot (so the default path ldapi:/// did not previously work on NixOS).

cc also @Mic92 , who was following #141240.

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.

@github-actions github-actions bot added 6.topic: kernel The Linux kernel 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 5, 2022
@kwohlfahrt kwohlfahrt changed the base branch from master to staging June 5, 2022 19:13
@github-actions github-actions bot removed 6.topic: kernel The Linux kernel 6.topic: lua Lua is a powerful, efficient, lightweight, embeddable scripting language. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: stdenv Standard environment labels Jun 5, 2022
@ofborg ofborg bot requested review from ajs124, dasJ and mweinelt June 5, 2022 19:41
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. and removed 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. labels Jun 5, 2022
@mweinelt mweinelt requested a review from Mic92 June 6, 2022 16:42
@mweinelt
Copy link
Member

mweinelt commented Jun 6, 2022

Probably conflicts due to merging #176290

kwohlfahrt added 9 commits June 6, 2022 19:49
@kwohlfahrt
openldap: remove deprecated options
b1a09d1
@kwohlfahrt
openldap: change default ldapi directory
c48f3a6 
By default, this is /run/ldapi, which is not compatible with systemd's
runtime directories. Change it to /run/slapd/ldapi (in library and
server). This makes `ldapi:///` work as a default socket again.
@kwohlfahrt
openldap: Allow notify outside of main thread
510650a 
This fixes a bug I observed in deployment on a RPi, but not able to
reproduce in tests.
@kwohlfahrt
openldap: run in foreground
d7790c5 
Now that we use notify daemon type, this works safely and simplifies
configuration.
@kwohlfahrt
openldap: run under systemd-defined user/group
8ce97b3 
This improves security, by starting the service as an unprivileged user,
rather than starting as root and relying on the service to drop
privileges. This requires a significant cleanup of pre-init scripts, to
make use of StateDirectory and RuntimeDirectory for permissions.
@kwohlfahrt
openldap: use specialisations for tests
9b8843a 
This speeds up tests a bit. Also, ensure that mutable config works for
manual config dir.
@kwohlfahrt
openldap: test and fix mutable config
9c2ff8a
@kwohlfahrt
openldap: test starting with empty DB
449a8eb 
This addresses the original concern behind NixOS#92544
@kwohlfahrt
openldap: change runtime directory
6581ab9 
Use `openldap` for consistency between `/var/lib` and `/run`.
@ofborg ofborg bot added 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches. and removed 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild on Darwin and must target a staging branch. 10.rebuild-linux-stdenv This PR causes stdenv to rebuild on Linux and must target a staging branch. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. labels Jun 6, 2022
@ajs124
Copy link
Member

ajs124 commented Jun 7, 2022

The changes to the package look alright, I haven't really ever looked at the module though. Would be nice if somebody else could review those, that knows how this module works.

@mweinelt
Copy link
Member

mweinelt commented Jun 7, 2022 •
edited
Loading

Approving the following commits, which should probably also be backported, as they're fixing problems I've seen on 22.05:

@kwohlfahrt
Copy link
Contributor Author

kwohlfahrt commented Jun 8, 2022

@mweinelt what's the best way to proceed then - should I cherry-pick those changes out to a new PR, or wait for further review here?

@mweinelt mweinelt mentioned this pull request Jun 13, 2022
Merged
1 task
@Ekleog
Copy link
Member

Ekleog commented Jun 13, 2022

I just landed #177084, which does the equivalent of 510650a ; this should be one fewer change to cherry-pick out :) (sorry I don't have much time for reviewing PRs right now, I learned of yours after making #177084 and decided to land as it was ready while yours seems much bigger and to still need some work at least from the review point of view)

Overall my suggestion for getting that PR landed would be, if your commits are more or less independent you should probably try to split it up into multiple small PRs, that has a better chance of getting it under the "I have 5 minutes to perform a review while $stuff is compiling at work" bar that gets PRs reviewed faster :)

@Mic92 Mic92 mentioned this pull request Jun 29, 2022
Merged
13 tasks
@mweinelt mweinelt closed this Jul 22, 2022
@mweinelt
Copy link
Member

mweinelt commented Jul 22, 2022

Merged #179597, the rebased version, instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@FRidh FRidh Awaiting requested review from FRidh

@jonringer jonringer Awaiting requested review from jonringer

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@ajs124 ajs124 Awaiting requested review from ajs124

@mweinelt mweinelt Awaiting requested review from mweinelt

@dasJ dasJ Awaiting requested review from dasJ

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: clean-up This PR removes packages or removes other cruft 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 501-1000 This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 2501-5000 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kwohlfahrt @mweinelt @ajs124 @Ekleog