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
Open

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

Show file tree
Hide file tree
Changes from all 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`` option had no effect if the ``retries`` option was set. Fix now also sets the parameter correctly in the retry request session (https://github.com/ansible-collections/community.hashi_vault/issues/461).
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
17 changes: 17 additions & 0 deletions tests/integration/targets/connection_options/tasks/controller.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@

- <<: *assert

- name: test HTTPS with no cert validation and retries
<<: *test
vars:
ansible_hashi_vault_validate_certs: false
ansible_hashi_vault_retries: 2

- <<: *assert

- name: test HTTPS with cert validation
<<: *test
vars:
Expand All @@ -72,6 +80,15 @@

- <<: *assert

- name: test HTTPS with cert validation and retries
<<: *test
vars:
vault_test_connection_want_args: true
vault_test_connection_want_exception: true
ansible_hashi_vault_retries: 2

- <<: *assert

- name: test HTTPS with proxy & cert validation
<<: *test
vars:
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/targets/connection_options/tasks/target.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,29 @@

- <<: *assert

- name: test HTTPS with no cert validation and retries
register: result
vault_test_connection:
validate_certs: false
retries: 2

- <<: *assert

- name: test HTTPS with cert validation
register: result
vault_test_connection:
want_args: true

- <<: *assert

- name: test HTTPS with cert validation and retries
register: result
vault_test_connection:
want_args: true
retries: 2

- <<: *assert

- name: test HTTPS with proxy & cert validation
register: result
vault_test_connection:
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/plugins/module_utils/test_hashi_vault_connection_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ def test_get_hvac_connection_options(
assert 'proxies' not in opts or opts['proxies'] == predefined_options['proxies']
assert 'namespace' not in opts or opts['namespace'] == predefined_options['namespace']
assert 'timeout' not in opts or opts['timeout'] == predefined_options['timeout']
assert 'session' not in opts or isinstance(opts['session'], Session)
assert 'session' not in opts or (isinstance(opts['session'], Session) and opts['session'].verify == connection_options._conopt_verify)

@mock.patch('ansible_collections.community.hashi_vault.plugins.module_utils._connection_options.HAS_RETRIES', new=False)
def test_get_hvac_connection_options_retry_not_available(self, connection_options, adapter):
Expand Down
Loading