Don't send optional parameters unless explicitly specified #533

llamasoft · 2019-10-15T05:08:14Z

Apologies in advance, this is a big PR. This fixes issue #527 in addition to a few other small things.

This PR primarily does two things:

Optional parameters now have a default value of None.
- We no longer have to worry about our default values conflicting with Vault's default values. We now just let Vault decide what the correct default should be, even if that value changes between versions.
Optional parameters are no longer sent when their value is None.
- The "update" functions no longer clobbers existing values if the user didn't specify them.

This PR may contain breaking changes (depending on what your definition of "breaking" is). If anything, the "clobbering" side effect has been removed and some previously required parameters have been made optional.

I did my best to use the following pattern when creating the params value for API calls:

# Required parameters are always set
params = {
    'id': id,
    'username': username,
}

# Then optional parameters are added, assuming they're not None
params.update(
    utils.remove_nones({
        'ttl': ttl,
        'metadata': metadata,
    })
)

# Then we add optional parameters that require special handling
if policies is not None:
    params['policies'] = ','.join(policies)

# If we have a catch-all argument, it's added last
params.update(extras)

Please let me know if you have any questions!

This modifies functions to only send optional parameters when a value has explicitly been supplied. Doing so prevents "update" functions from clobbering preexisting values when they're not supplied. Additionally, all default parameter values have been changed to to None so that the Vault server can select the appropriate default value.

update-docs · 2019-10-15T05:08:17Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes.
See: https://github.com/hvac/hvac/blob/develop/CONTRIBUTING.md#documentation

llamasoft · 2019-10-15T05:11:06Z

I also took the following notes while making the above changes. It contains comments about notable changes along with a few observations that I found important.

Azure:

CHANGED: create_role parameter policies now accepts CSV string or list of strings

Database:

CHANGED: create_role documentation updated to something meaningful 🙃

GCP:

configure parameter google_certs_endpoint is deprecated
create_role parameter project_id is deprecated by bound_projects (list)

GitHub:

configure is missing a lot of parameters

LDAP:

CHANGED: configure parameters user_dn and group_dn made optional
- Retained argument position to prevent being a breaking change
CHANGED: hvac/constants/ldap.py file removed as it is no longer used

MFA:

This entire endpoint is deprecated so I didn't bother updating it

Okta:

CHANGED: configure parameter base_url default value now differs from API documentation
- This is likely just a documentation issue
register_user, read_user, and delete_user duplicate URL parameter username in JSON payload
- I left this one as-is as it doesn't appear to hurt anything
Ditto for delete_group, but register_group and list_group correctly omit it

PKI:

CHANGED: sign_data and verify_signed_data optional parameter marshaling_algorithm added

RADIUS:

configure is missing a lot of parameters
BUG: register_user attempted to convert username string into a CSV list (?!) for POST data
- Didn't hurt anything as username is extracted from URL path in Vault server
BUG: register_user parameter policies never actually passed as parameter

System Backend:

Auth
- enable_auth_method parameter plugin_name is deprecated
- CHANGED: enable_audit_device optional parameter local was added
Init
- initialize provides default for required API parameters secret_shares and secret_threshold
Key
- start_root_token_generation parameter otp is deprecated

Misc:

There seems to be some discrepancy on how "extra arguments" are accepted:
- Some methods use only **kwargs (e.g. hvac/api/system_backend/auth.py)
- Some use *args and **kwargs (e.g. hvac/api/secrets_engines/active_directory.py)
- hvac/api/secrets_engines/pki.py uses extra_params={}
Most argument names match API parameter names, but some don't
- Example: hvac/api/auth_methods/ldap.py configure uses user_dn instead of userdn
- Example: hvac/api/system_backend/auth.py configure uses method_type instead of type
Many methods duplicate URL parameters into JSON payload as well
- This isn't necessary and fortunately Vault ignores the extra parameters
ttl, max_ttl, policies, period, num_uses and a few other fields are deprecated as of Vault version 1.2.0
- https://github.com/hashicorp/vault/blob/master/CHANGELOG.md#120-july-30th-2019

codecov-io · 2019-10-15T16:58:50Z

Codecov Report

Merging #533 into develop will increase coverage by 0.89%.
The diff coverage is 81%.

@@             Coverage Diff             @@
##           develop     #533      +/-   ##
===========================================
+ Coverage    82.13%   83.03%   +0.89%     
===========================================
  Files           56       55       -1     
  Lines         3029     2976      -53     
===========================================
- Hits          2488     2471      -17     
+ Misses         541      505      -36

Impacted Files	Coverage Δ
hvac/api/auth_methods/ldap.py	`100% <100%> (ø)`	⬆️
hvac/api/system_backend/audit.py	`95.45% <100%> (+0.21%)`	⬆️
hvac/utils.py	`75.7% <100%> (-1.45%)`	⬇️
hvac/api/auth_methods/okta.py	`100% <100%> (ø)`	⬆️
hvac/api/secrets_engines/pki.py	`100% <100%> (ø)`	⬆️
hvac/api/secrets_engines/aws.py	`98.18% <100%> (+0.1%)`	⬆️
hvac/api/secrets_engines/gcp.py	`46.15% <100%> (+0.84%)`	⬆️
hvac/api/auth_methods/github.py	`100% <100%> (ø)`	⬆️
hvac/api/system_backend/auth.py	`87.5% <100%> (+0.32%)`	⬆️
hvac/constants/transit.py	`100% <100%> (ø)`	⬆️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae0fdc6...546f5d6. Read the comment docs.

jeffwecan · 2019-10-16T18:29:04Z

As a heads-up, I think this sort of change is valuable and will help prevent a lot of previously seen footshoots. I simply haven't had the time to go through the PR in the depth required by the scope of changes. I.e., just want let ya know I haven't forgotten about this PR or anything ;)

edit: But if it does seem like we have forgotten about it, feel free to comment here and/or hit us up on gitter.im/hvac/community!

jeffwecan · 2019-11-20T16:03:21Z

Generating a release today and planning on doing a minor release afterwards with this PR and #537 included. 👍

jeffwecan

Apologies for letting this PR linger for so long. Let's go ahead and shortly!

jeffwecan · 2019-12-30T17:20:30Z

(I'm working to get the merge conflicts patched up now...)

ebpmp · 2020-02-26T16:50:18Z

Is there a release expected for this one? It's currently blocking some work I have. I debugged for a while yesterday (sign/hmac & verify) only to find the same issue (locally made a fix, then found this PR).

jeffwecan · 2020-02-26T21:54:48Z

Apologies about that @ebpmp! Other responsibilities have kept me from getting this shipped as of yet. I will do my darndest to get that done this evening (i.e., sometime over the next 4-6 hours).

* Only send optional parameters when supplied This modifies functions to only send optional parameters when a value has explicitly been supplied. Doing so prevents "update" functions from clobbering preexisting values when they're not supplied. Additionally, all default parameter values have been changed to to None so that the Vault server can select the appropriate default value. * Fix and extend Azure test case Co-authored-by: Jeffrey Hogan <jeff@jeffhogan.me>

* adjust CRL endpoint for retrieving CRL in DER and PEM format * adjusting tests, comments * adjust method to download only PEM format and convert response text to string due to pyton2.7 * Fixes typo: much -> must * Fixes close quotes in example usage of read_secret_version for those of us who are reckless copy-pasters * Don't send optional parameters unless explicitly specified (#533) * Only send optional parameters when supplied This modifies functions to only send optional parameters when a value has explicitly been supplied. Doing so prevents "update" functions from clobbering preexisting values when they're not supplied. Additionally, all default parameter values have been changed to to None so that the Vault server can select the appropriate default value. * Fix and extend Azure test case Co-authored-by: Jeffrey Hogan <jeff@jeffhogan.me> * Bump version: 0.9.6 → 0.10.0 * Changelog updates for v0.10.0 Co-authored-by: Moisés Guimarães de Medeiros <moisesguimaraes@users.noreply.github.com> Co-authored-by: Phil Herbert <9078954+philherbert@users.noreply.github.com> Co-authored-by: Marcus T <marcus.truscello@gmail.com>

ebpmp · 2020-03-03T19:00:26Z

Thank you so much. My tests are passing now after a lot of hair pulling. I built a competitive library to hvac and am in the process of decommissioning it now that I see how the api directory works. I will dig in and see if I can add some value there and make some PRs.

llamasoft added 2 commits October 14, 2019 19:26

Fix and extend Azure test case

9a9f25a

llamasoft requested a review from a team as a code owner October 15, 2019 05:08

Merge remote-tracking branch 'upstream/develop' into noclobber

955f1b1

jeffwecan added the misc Used as a release-drafter "category" label Dec 30, 2019

jeffwecan added this to the 0.10.0 milestone Dec 30, 2019

jeffwecan approved these changes Dec 30, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/develop' into noclobber

546f5d6

jeffwecan mentioned this pull request Feb 3, 2020

Fix a bug when creating external groups #553

Closed

jeffwecan merged commit 09e0702 into hvac:develop Feb 13, 2020

llamasoft deleted the noclobber branch March 4, 2020 18:11

jeffwecan mentioned this pull request Mar 30, 2020

Fix "Exception: member entities can't be set manually for external groups" #558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't send optional parameters unless explicitly specified #533

Don't send optional parameters unless explicitly specified #533

llamasoft commented Oct 15, 2019

update-docs bot commented Oct 15, 2019

llamasoft commented Oct 15, 2019

codecov-io commented Oct 15, 2019 •

edited

Loading

jeffwecan commented Oct 16, 2019 •

edited

Loading

jeffwecan commented Nov 20, 2019

jeffwecan left a comment

jeffwecan commented Dec 30, 2019

ebpmp commented Feb 26, 2020

jeffwecan commented Feb 26, 2020

ebpmp commented Mar 3, 2020

Don't send optional parameters unless explicitly specified #533

Don't send optional parameters unless explicitly specified #533

Conversation

llamasoft commented Oct 15, 2019

update-docs bot commented Oct 15, 2019

llamasoft commented Oct 15, 2019

codecov-io commented Oct 15, 2019 • edited Loading

Codecov Report

jeffwecan commented Oct 16, 2019 • edited Loading

jeffwecan commented Nov 20, 2019

jeffwecan left a comment

Choose a reason for hiding this comment

jeffwecan commented Dec 30, 2019

ebpmp commented Feb 26, 2020

jeffwecan commented Feb 26, 2020

ebpmp commented Mar 3, 2020

codecov-io commented Oct 15, 2019 •

edited

Loading

jeffwecan commented Oct 16, 2019 •

edited

Loading