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

fix validate_certs parameter beeing ignored when used with the retries parameter #462

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+40 −3
Open

fix validate_certs parameter beeing ignored when used with the retries parameter #462

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
64e16e4
The validate_certs parameter had no effect if the retries parameter w…
argetlam-coder Dec 14, 2024
c0957e0
File name of the changelog fragment changed to the correct pull reque…
argetlam-coder Dec 14, 2024
09469a1
Updated changelogs/fragments/462-validate-certs-fix.yml with suggeste…
argetlam-coder Dec 14, 2024
6b7f165
added some integration tests to test the certificate validation with …
argetlam-coder Dec 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelogs/fragments/462-validate-certs-fix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
---
bugfixes:
- connection_options - the validate_certs parameter had no effect if the retries parameter was set. Fix now also sets the parameter correctly in the retry request session (https://github.com/ansible-collections/community.hashi_vault/issues/461).
argetlam-coder marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 3 additions & 2 deletions plugins/module_utils/_connection_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def _filter(k, v):

retry_action = hvopts.pop('retry_action')
if 'retries' in hvopts:
hvopts['session'] = self._get_custom_requests_session(new_callback=self._retry_callback_generator(retry_action), **hvopts.pop('retries'))
hvopts['session'] = self._get_custom_requests_session(new_callback=self._retry_callback_generator(retry_action), conopt_verify=self._conopt_verify, **hvopts.pop('retries'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm debating whether we should bother passing this as parameter. Instead, we can make the original call here, and then set hvopts['session'].verify = self._conopt_verify. It feels like that may be the correct place to do this. Let me know if you object.

Copy link
Author

Choose a reason for hiding this comment

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

We can do it the way you suggested. I actually had it that way in my first draft, but then I thought it wouldn't match the rest of the style of the code and changed it.
Should I change it?


return hvopts

Expand All @@ -120,7 +120,7 @@ def process_connection_options(self):
self._process_option_proxies()
self._process_option_retries()

def _get_custom_requests_session(self, **retry_kwargs):
def _get_custom_requests_session(self, conopt_verify, **retry_kwargs):
'''returns a requests.Session to pass to hvac (or None)'''

if not HAS_RETRIES:
Expand Down Expand Up @@ -159,6 +159,7 @@ def new(self, **kwargs):
sess = Session()
sess.mount("https://", adapter)
sess.mount("http://", adapter)
sess.verify = conopt_verify

return sess

Expand Down
Loading