ldap auth method - add missing configure params by vault api names

[ceesios]

Deprecation notice from the maintainers

Several parameters on this method have been normalized to match their API names, for example user_dn is now userdn, group_filter is now groupfilter, etc.

For backwards compatibility, the old names still work, however they will be removed in v3.0.0 and if you use them from v1.2.0 you will receive a deprecation warning. Please update your calls to use the new names. Setting these parameters positionally should be unaffected in this case, but we recommend updating to use named parameters.

---

Would fix #974

Please let me know if this is the way to go or if i'm missing something.

[briantist]

Hi @ceesios ! Thanks for opening the issue and putting up a change!

As written, there are some duplicated dictionary keys.

But the bigger issue is that this would change the order of parameters, which will break anyone passing in unnamed arguments. We could perhaps put them at the end, but I want to be cautious about what we're doing here.

I'd also want to see tests added to ensure coverage for the new parameters.

[codecov]

Codecov Report

Merging #975 (3b9ecdf) into main (b52ed19) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
+ Coverage   84.78%   85.00%   +0.21%     
==========================================
  Files          65       65              
  Lines        3083     3127      +44     
==========================================
+ Hits         2614     2658      +44     
  Misses        469      469              

Files Changed	Coverage Δ
hvac/api/auth_methods/ldap.py	100.00% <100.00%> (ø)
hvac/utils.py	84.25% <100.00%> (+5.99%)	⬆️

[ceesios]

@briantist thank you for the review. I've removed the duplicates and moved the parameters to the end. I will look into the test coverage tomorrow. I have no experience with it but look forward to learning about it.

[ceesios]

@briantist i've added some unit and integration tests. Is this as expected?

[ceesios]

I still can't get the test to fail locally, however the error seems to be related to the userfilter trying to parse non-existent variables. Should be fixed by using a raw string.

[briantist]

I still can't get the test to fail locally, however the error seems to be related to the userfilter trying to parse non-existent variables. Should be fixed by using a raw string.

Ok, I still haven't had much of a chance to review the actual content on this PR, I've been so busy with getting the CI and release process fixed, and with external things. It might still be some time before I can properly review it, thanks for your patience.

[ceesios]

No problem,
I've removed the userfilter from the test. I can't seem to find a way to get it to fail locally, and in the CI the syntax keeps failing.

[briantist]

My guess is that the test failed for a specific (older) Vault version. You may not have been able to reproduce locally because your Vault version is new enough. Removing the test to make it pass means we would have a blind spot in that case.

It should probably be restored and we should figure out why, that way we can figure out what we need to add to the code to account for that in older Vault versions.

Using a container for your local Vault could help you reproduce it easily.

[briantist]

Ok, so here's what I've come up with.

• I've added a general utility in the library that works as a decorator around a method, to declare one or more aliases for parameters.
  • It can optionally mark the aliases as deprecated
  • It can raise exceptions when a value is supplied via more than one name
  • It can distinguish between None passed explicitly vs. being assigned as a default on an optional parameter (important for duplicate detection and deprecation warnings)
  • It can handle positional arguments for the canonical name (the name we want to keep), but not for alias names. This one is pretty important since almost all of our existing arguments in the library can be specified positionally.
  • The aliases don't need to be added to the function declaration, but they should be added to the docstring (which was already done here).
  • It has full coverage in its own set of tests, covering all the options.
• With that in place, we could solve declaration of the aliases, deprecation notices only on use, and value resolution by adding the decorators to the method.
  • Since we only ever need to worry about the canonical name of the parameter within the function, we could also replace the old (now aliased) names with the new (canonical) names, by position, which combined with the decorator's positional support, means all positional uses of the parameters will continue to work smoothly.
• Updated the docstrings on the aliases to mention that they are aliases and will be removed in v3.0.0.
• [Minor] I added * before the brand new parameters, so that they will always need to be supplied by keyword. This is something I need to write up more generally as a recommendation but I think we should go through the library and eventually make this the case for everything.
• [Minor, Internals] Integration tests were set to suppress all deprecation warnings, which took me way longer to figure out than I'd like to admit.
  • I've disabled that completely: if we're intentionally calling something deprecated in tests, then we should also be asserting the warning, which will capture it.
  • Disabling these as revealed some actual internal deprecated stuff that we'll need to address at some point, so, win-win.

@ceesios Since this is something of a substantial change, I will look to get review from someone else on the project for my code, and I'd also appreciate if you can pull down and test the changes and ensure they still work for you as expected.

[ceesios]

I've been working on another project at work, sorry for the silence the past weeks.

This looks like a significant improvement. I've tested all options against vault 1.11.2 manually and all work as expected. Today and tomorrow i will be in a lot of meetings. I hope to review in more detail this Thursday.

[ceesios]

certificates is defined twice in remove_nones.
I suggest ordering the params in remove_nones alphabetically to make it more readable and prevent this.

There are some more params missing;
Client TLS was always present. I overlooked them becouse they are not returend by a GET just like bindpass.

• client_tls_cert
• client_tls_key

added in v1.11.x

• connection_timeout
• max_page_size

added in v1.14.x

• dereference_aliases

I will add them tomorrow and will also look at the tests and deprecation warnings.

[ceesios]

looks good to me. I've also tested on 1.14.0 with the ansible module i am using and after adding all the new params there they all work.

[briantist]

Thanks for these new commits @ceesios ! Could you also add the new parameters to the unit tests?

[adammike]

I really like the use of decorators here for the aliased parameters and deprecation warning. Testing for both the aliased and un-aliased paremters is great to see also.

Well done.