Add replacement ldapquery::search function #47

alexjfisher · 2023-05-24T16:41:21Z

Sourcing ldap server configuration options from puppet.conf was conflating their original purpose, and a future release of Puppet may even remove these options.

It's still desirable to be able to set defaults for the function from a file, but a dedicated yaml file is far more flexible than an ini file.

In this commit, a new ldapquery::search function is added with a new implementation. The old version is kept, but marked as DEPRECATED.

traylenator · 2023-05-25T06:26:53Z

lib/puppet/functions/ldapquery/query.rb

@@ -16,6 +16,7 @@
        Optional[server] => String[1],


This is a single server .

The module supports multiple servers as

ldap.hosts = [[ldap1.ex.org,389], [ldap2.ex.org, 389]]

if reworking would be good.

That type is only used in the legacy dispatch. The new one accepts a Hash (that I might tighten down to a Struct), where you can set hosts.

I'm going to work on the docs and examples this morning. Hopefully it'll become clearer.

cruelsmith · 2023-05-25T15:34:15Z

lib/puppet/functions/ldapquery/query.rb

+  dispatch :query do
+    param 'Optional[String[1]]', :base
+    param 'Optional[String[1]]', :filter
+    param 'Array[String[1],1]', :attributes
+    optional_param 'LDAPArgs', :ldap_args
+    optional_param 'Enum["sub","base","single"]', :scope
+    optional_param 'Integer[1]', :search_timeout
+    return_type 'Array'
  end


From my personal view i do not get why that loading of the new config file and extended value support must be hidden beneath a new dispatch. Also that optional base and filter are not optional_param is strange for me.

ldapquery::query(undef, undef, ['uid']) looks strange.

I wanted to retain the current interface alongside the new one. Some people might have calls to the function spread out across multiple manifests and forcing them to refactor them all at once isn't as friendly as publishing a version that works with both. The dispatches have to be unambiguous for this to work.
I'd imagine most people will only put their connection details in the config file and almost always also use a filter. I made undef allowable for completeness.

Yes sure but why not simply using the "old" one and add there the new feature set?
I not really see any breaking change in doing it since in the end both use the same search function. Only the way from where defaults are coming differ.
So the kind of thing i wanted to do with #40.

I think there needs to be a way of distinguishing what config file the code should use. I really didn't want a function that tried to use both. In the original options hash, only server was relevant to connecting to ldap, base and scope are really search arguments. And do I merge server with host and hosts? Having ldap_args being fed directly (after converting somethings to symbols as required), to the ruby ldap search method allows for any/all options to be specified without us trying to magically set anything based on a combination of other settings.

Maybe it would be cleaner to just put the new code in openldap::search instead? (although I still think we just drop the original implementation at some point and not hack more stuff into it). And maybe it's not necessary to allow undef for base and filter. Imposing that all calls have the first 3 arguments (base, filter, attributes) wouldn't be a hardship on users and might make your code look less magic and more readable.

traylenator · 2023-09-28T21:45:34Z

Hi,
Was looking at starting to use this module - thanks.

Any chance of finishing this off , have written against this branch and its working well.

The idea of creating a new function ldapquery::search and deprecating the current ldapquery::query one makes
sense to me?

Am using it like:

 $_filter = "(&(objectClass=group)(|${_egroups.map | $_eg | { "(CN=${_eg})" }.join()}))"

 $_results = ldapquery::query(
   'OU=e-groups,OU=Workgroups,DC=example,DC=ch',
   $_filter,
   ['member'],
   {
     'hosts' => [
       ['ldap.example.ch', 389],
       ['ldap-critical.example.ch', 389],
     ],
     'scope'   => 'sub'
   },
 )

which is fine I'd say.

If the connection parameters came from a file f we certainly have more than one ldap server so that location path needs to be a configuration. I would just leave loading from a yaml or hiera as exercise for the reader.

alexjfisher · 2023-11-06T17:15:49Z

Any chance of finishing this off , have written against this branch and its working well.

@traylenator I've just got around to picking this up again. Decided the best approach is probably just a new function instead of messing around with multiple dispatches etc. Just doing a bit more testing locally, then I'll take this off draft.

.github/workflows/ci.yml

Sourcing ldap server configuration options from puppet.conf was conflating their original purpose, and a future release of Puppet may even remove these options. It's still desirable to be able to set defaults for the function from a file, but a dedicated yaml file is far more flexible than an ini file. In this commit, a new `ldapquery::search` function is added with a new implementation. The old version is kept, but marked as DEPRECATED.

traylenator · 2023-11-15T20:27:04Z

Thanks - had it mind to look at.

traylenator reviewed May 25, 2023

View reviewed changes

alexjfisher force-pushed the refactor_options branch 3 times, most recently from 8faf8be to 673d581 Compare May 25, 2023 15:32

cruelsmith reviewed May 25, 2023

View reviewed changes

alexjfisher force-pushed the refactor_options branch 13 times, most recently from 47d523c to 94c7756 Compare May 26, 2023 11:11

traylenator mentioned this pull request Oct 2, 2023

Rework for full parameter support #40

Closed

alexjfisher force-pushed the refactor_options branch 5 times, most recently from 845f588 to a076ef0 Compare November 6, 2023 17:06

alexjfisher changed the title ~~WIP: Rework ldap options~~ Add replacement ldapquery::search function Nov 8, 2023

alexjfisher marked this pull request as ready for review November 13, 2023 10:50

alexjfisher requested a review from traylenator November 13, 2023 11:41

bastelfreak reviewed Nov 15, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

alexjfisher force-pushed the refactor_options branch from a076ef0 to 91922b3 Compare November 15, 2023 11:33

bastelfreak approved these changes Nov 15, 2023

View reviewed changes

alexjfisher merged commit a3cbcd9 into voxpupuli:master Nov 15, 2023
28 checks passed

alexjfisher deleted the refactor_options branch November 15, 2023 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add replacement ldapquery::search function #47

Add replacement ldapquery::search function #47

alexjfisher commented May 24, 2023 •

edited

traylenator May 25, 2023

alexjfisher May 25, 2023

alexjfisher May 25, 2023

cruelsmith May 25, 2023

alexjfisher May 25, 2023

cruelsmith May 25, 2023 •

edited

alexjfisher May 25, 2023

alexjfisher May 25, 2023

traylenator commented Sep 28, 2023

alexjfisher commented Nov 6, 2023

traylenator commented Nov 15, 2023

Add replacement ldapquery::search function #47

Add replacement ldapquery::search function #47

Conversation

alexjfisher commented May 24, 2023 • edited

traylenator May 25, 2023

Choose a reason for hiding this comment

alexjfisher May 25, 2023

Choose a reason for hiding this comment

alexjfisher May 25, 2023

Choose a reason for hiding this comment

cruelsmith May 25, 2023

Choose a reason for hiding this comment

alexjfisher May 25, 2023

Choose a reason for hiding this comment

cruelsmith May 25, 2023 • edited

Choose a reason for hiding this comment

alexjfisher May 25, 2023

Choose a reason for hiding this comment

alexjfisher May 25, 2023

Choose a reason for hiding this comment

traylenator commented Sep 28, 2023

alexjfisher commented Nov 6, 2023

traylenator commented Nov 15, 2023

alexjfisher commented May 24, 2023 •

edited

cruelsmith May 25, 2023 •

edited