Changing default values for sys.initialize parameters secret_shares and secret_threshold

[briantist]

Fixes #1030

For more details see:

• Vault Initialization fails when using azurekeyvault for auto unsealing #1030

Because we set default values for secret_shares and secret_threshold in the initialize call, it's not possible to make the API request without values set.

Unfortunately, there are certain cases in the API call where these values must not be set, when using certain auto-unseal configurations, in which only recovery_shares and recovery_threshold should be set, and Vault will reject the call if the other two are set as well.

Breaking change

We're changing the default value of these parameters to None along with the rest.

In v2.0.0, if secret_shares is None, and recovery_shares is also None, then we will set secret_shares to 5 (which was the default previously), and issue a warning.
Likewise with secret_threshold and recovery_threshold, where secret_threshold will be set to 3 (the previous default) when both of those parameters are None.

In v3.0.0 we will remove the default fallback.

This is very unlikely to actually be a breaking change in v2.0.0 but it could be if all of the following are true:

• there are any versions of the API call where setting both sets of parameters is actually valid (unconfirmed if this is even possible)
• you are making such a call
• you are only setting recovery_shares and/or recovery_threshold while relying on the defaults for secret_shares and/or secret_threshold

Action needed

If you currently call the initialize method relying on the previous default values, you should update to using explicit values right away. This can be done right away, even in already released versions.

With hvac>=2,<3, relying on defaults will work (with above caveats) but will issue a deprecation warning.

In hvac>=3 default values will no longer be provided and your calls will probably break unless you specify your values explicitly.

[codecov]

Codecov Report

Merging #1063 (6ad5f9f) into main (eaa862e) will increase coverage by 1.48%.
Report is 4 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1063      +/-   ##
==========================================
+ Coverage   85.08%   86.57%   +1.48%     
==========================================
  Files          65       65              
  Lines        3145     3187      +42     
==========================================
+ Hits         2676     2759      +83     
+ Misses        469      428      -41     

Files	Coverage Δ
hvac/api/system_backend/init.py	100.00% <100.00%> (+43.75%)	⬆️

... and 5 files with indirect coverage changes

[kaypeter87]

I think this is a good compromise. Plus deprecation warnings have been implemented 👍 LGTM

[Tylerlhess]

Why use an explicit error_msg variable here when the current file has previously not done so? Its not a breaking instance of code just stylistic.

[briantist]

looking at left side in the side-by-side diff, it seems it's because the previous code did in fact use this variable, and I just changed the message minimally but didn't remove use of the var. I don't feel strongly about it, if you want to put in a suggested change in the comment that removes the variable I'll probably accept it.

[Tylerlhess]

Code looks good from my side just a single stylistic inconsistency.

[briantist]

been a little while since CI ran so I'm going to close/re-open to kick off CI again