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.

Sign up for GitHub

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

Jump to bottom

Modular OpenLDAP #21764

Closed
wants to merge 1 commit into from
Closed

Modular OpenLDAP #21764

wants to merge 1 commit into from

Conversation

ip1981
Copy link
Contributor

@ip1981 ip1981 commented Jan 9, 2017 •
edited

Turn on all overlays and all modules except perl, ndb, sql.
Those modules aren't built by default anyway.

This change does not add new dependencies, but will require
additional configuration to load modules, including back_mdb.

This is how OpenLDAP is built in Debian.

Motivation for this change
Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@ip1981, thanks for your PR! By analyzing the history of the files in this pull request, we identified @chaoflow, @edolstra and @DerTim1 to be potential reviewers.

@ip1981
Modular OpenLDAP
43805ec 
Turn on all overlays and all modules except perl, ndb, sql.
Those modules aren't built by default anyway.

This change does not add new dependencies, but will require
additional configuration to load modules, including `back_mdb`.

This is how OpenLDAP is built in Debian.
@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017

How does this affect existing configuration? Is it required to load modules manually?

@ip1981
Copy link
Contributor Author

ip1981 commented Jan 9, 2017 

dn: cn=module{0},cn=config
cn: module{0}
objectClass: olcModuleList
olcModuleLoad: {0}back_mdb

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017

Do you agree on this change @DerTim1 ? I am not an expert on ldap myself.

@DerTim1
Copy link

DerTim1 commented Jan 9, 2017

I'm doing a review at the moment...

@ip1981
Copy link
Contributor Author

ip1981 commented Jan 9, 2017

I'm an expert in openldap zalora/nixsap@7511e9b :D

DerTim1
DerTim1 reviewed Jan 9, 2017
View reviewed changes
pkgs/development/libraries/openldap/default.nix
"--disable-ndb"
"--disable-perl"
"--disable-sql"
"--enable-backends=mod"
Copy link

Choose a reason for hiding this comment

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

Put backends to modular load (default: yes) is problematic. If you load a module (e.g. back_hdb), you have to specify the path of modules-dir. This path is in a nix-store.
If we need this configure option, we have to add something like this
ln -s /nix/store/hv58j5qilznnc3arjgdzymkq58cxn9wc-openldap-2.4.44/libexec/openldap /usr/lib/ldap
to the openldap-service of NixOS. Then we can use a static path to load modules in config-files (slapd.conf) or with config-dir (slap.d).
But I think /usr/lib/ldap (default) isn't a good idea. Maybe /var/db/openldap/modules is a better choise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to specify the path of modules-dir

No you haven't. OpenLDAP modules are immediately available, because it knows its libdir ($out/lib/...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Third-party modules, if any, can be loaded by absolute path (/foo or /nix/store/).

Copy link

Choose a reason for hiding this comment

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

Ok, I will retry with a different slap.d-configuration.
Be right back...

@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017

@ip1981 Thanks, but I like the four eyes principle, because we are all humans.

DerTim1
DerTim1 approved these changes Jan 9, 2017
View reviewed changes
Copy link

@DerTim1 DerTim1 left a comment

Choose a reason for hiding this comment

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

Ok, approved with my slap.d-conf.
But I haven't tested the NixOS-module with the example slapd.conf

@DerTim1
Copy link

DerTim1 commented Jan 9, 2017 •
edited

@Mic92 Now also approved with example-slapd.conf...

@Mic92 Mic92 mentioned this pull request Jan 9, 2017
Merged
7 tasks
@Mic92
Copy link
Member

Mic92 commented Jan 9, 2017

@DerTim1 we have a different openldap pull request to be merged. Because this change will trigger a number of rebuilds this should be probably merged with #21648

@DerTim1
Copy link

DerTim1 commented Jan 10, 2017

@Mic92 Entirely right

@vcunat vcunat changed the base branch from master to staging January 10, 2017 16:40
@ip1981 ip1981 closed this Jan 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DerTim1 DerTim1 DerTim1 approved these changes

Assignees
No one assigned
Labels
1.severity: mass-rebuild 2.status: merge conflict
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ip1981 @mention-bot @Mic92 @DerTim1 @vcunat