Skip to content

openldap: load client config from /etc, not the nix store#182080

Merged
mweinelt merged 1 commit intoNixOS:masterfrom
danc86:openldap-sysconfdir
Jul 21, 2022
Merged

openldap: load client config from /etc, not the nix store#182080
mweinelt merged 1 commit intoNixOS:masterfrom
danc86:openldap-sysconfdir

Conversation

@danc86
Copy link
Copy Markdown
Contributor

@danc86 danc86 commented Jul 19, 2022 •
edited
Loading

Description of changes

Fixes #181937 by passing sysconfdir= twice.

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.

@ofborg ofborg bot requested review from ajs124, dasJ and mweinelt July 19, 2022 10:14
@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. labels Jul 19, 2022
@mweinelt
Copy link
Copy Markdown
Member

mweinelt commented Jul 19, 2022

I assume you found that out emperically? Because I don't find these flags intuitive at all. 😄

@danc86
Copy link
Copy Markdown
Contributor Author

danc86 commented Jul 19, 2022

I've worked on (and been responsible for) enough automake messes throughout my life that I was able to guess at this "solution" 🙃

@mweinelt
Copy link
Copy Markdown
Member

mweinelt commented Jul 19, 2022

Ideally we would now create a test case to make sure we don't break this again. Do you think that's something you could contribute?

https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/openldap.nix

@danc86
Copy link
Copy Markdown
Contributor Author

danc86 commented Jul 19, 2022

Sure, I can give it a go.

@danc86
openldap: load client config from /etc, not the nix store
be2175d 
We want Openldap clients to load /etc/ldap.conf at runtime, not
${pkgs.openldap}/etc/ldap.conf which is always a sample config.

Pass sysconfdir=/etc at compile time, so that /etc/krb5.conf is embedded
in the library as the path of its config file.

Pass sysconfdir=${out}/etc at install time, so that the sample configs
and schema files are correctly included in the build output.

This hack works because the Makefiles are not smart enough to notice
that the sysconfdir variable has changed across invocations -- because
nobody ever writes their Makefiles to be that smart. :-)

Fixes NixOS#181937.
@danc86 danc86 force-pushed the openldap-sysconfdir branch from e7395f2 to be2175d Compare July 19, 2022 12:33
@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jul 19, 2022
@danc86
Copy link
Copy Markdown
Contributor Author

danc86 commented Jul 19, 2022

Amended to make the tests rely on client config, so that they will fail if openldap isn't reading /etc/ldap.conf from the right place.

@mweinelt mweinelt requested review from Mic92 and kwohlfahrt July 19, 2022 13:33
@danc86
Copy link
Copy Markdown
Contributor Author

danc86 commented Jul 20, 2022

Well, I did run nixpkgs-review. It spent ~24 hours trying to build 2058 packages, of which 1331 succeeded, 44 failed, and 683 skipped. I don't think there's much value in pasting the full results here.

I quickly skimmed the failures and they all look like unrelated pre-existing breakages. I was a little concerned about _389-ds-base which failed with:

/nix/store/ld11fgq92p6bci267ywy55j9825sbi66-binutils-2.38/bin/ld: cannot find -lldap_r: No such file or directory

but that problem reproduces on the master branch too.

@mweinelt mweinelt merged commit b435482 into NixOS:master Jul 21, 2022
@mweinelt mweinelt mentioned this pull request Jul 21, 2022
Merged
@mweinelt
Copy link
Copy Markdown
Member

mweinelt commented Jul 21, 2022

Sorry, merged then reverted, this should've gone into staging, not master, given that it causes alot of rebuilds.

kwohlfahrt
kwohlfahrt reviewed Jul 21, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kwohlfahrt kwohlfahrt left a comment

Choose a reason for hiding this comment

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

I'm late to the party, but agree the change looks good - I guess the PR needs to be reopened now to fix the correct base branch branch?

@github-actions github-actions bot mentioned this pull request Jul 21, 2022
Merged
1 task
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jul 21, 2022

Successfully created backport PR #182395 for staging-22.05.

@mweinelt
Copy link
Copy Markdown
Member

mweinelt commented Jul 21, 2022 •
edited
Loading

I'm currently doing the rebase locally and moving a few things around due to the test changes I just merged. I'm just building up to the openldap test on staging, so it takes a few minutes.

@mweinelt
Copy link
Copy Markdown
Member

mweinelt commented Jul 22, 2022

b30534e

@danc86
Copy link
Copy Markdown
Contributor Author

danc86 commented Jul 23, 2022

Sorry, merged then reverted, this should've gone into staging, not master, given that it causes alot of rebuilds.

Oops, sorry about that. I'm new to Nixos and didn't know about the staging branch.

@danc86 danc86 deleted the openldap-sysconfdir branch April 26, 2023 22:09
@danc86 danc86 mentioned this pull request May 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mweinelt mweinelt Awaiting requested review from mweinelt

@dasJ dasJ Awaiting requested review from dasJ

@ajs124 ajs124 Awaiting requested review from ajs124

@Mic92 Mic92 Awaiting requested review from Mic92

1 more reviewer

@kwohlfahrt kwohlfahrt kwohlfahrt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 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.

Openldap clients ignore system-wide LDAP client configuraiton in /etc/ldap.conf

3 participants

@danc86 @mweinelt @kwohlfahrt