Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized #677

noredistribution · 2023-10-11T18:43:05Z

Change Summary

Added error raises for cv_facts_v3 to raise a proper error when a user is not authorized

Related Issue(s)

Fixes #675

Component(s) name

arista.cvp.cv_facts_v3

Proposed changes

How to test

Generate a service account token that has No Access to inventory or other features:

- name: Test CV Facts
  hosts: CVP #cv_server
  connection: local
  tasks:
  - name: gather cvp facts
    arista.cvp.cv_facts_v3:
      facts:
      - devices
      #- tasks
      #- images
      #- containers
      - configlets
      #verbose: short
    register: facts
  - name: print facts
    debug:
      msg: "{{facts}}"

should result in

ansible-playbook get_cv_facts.yaml -i ../inventory.yaml

PLAY [Test CV Facts] **********************************************************************************************************

TASK [gather cvp facts] *******************************************************************************************************
fatal: [cv_server]: FAILED! => changed=false
  msg: |-
    GET: https://10.83.12.79:443/web/inventory/devices?provisioned=True : Request Error: Forbidden - 403 Forbidden

PLAY RECAP ********************************************************************************************************************
cv_server                  : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

another test is to create a service account token that expires in a few seconds, and then run the playbook with it, the result will be something like:

TASK [gather cvp facts] *******************************************************************************************************************************************************************************************************************************************************
fatal: [cv_server]: FAILED! => changed=false
  msg: |2-

    10.83.12.79: Authenticate: https://10.83.12.79:443/api/v1/rest: Request Error: Unauthorized

Checklist

User Checklist

N/A

Repository Checklist

[x ] My code has been rebased from devel before I start
[ x] I have read the CONTRIBUTING document.
My change requires a change to the documentation and documentation have been updated accordingly. (check the box if not applicable)
I have updated molecule CI testing accordingly. (check the box if not applicable)

chetryan

I think we need to be raising the Exceptions higher up that stack as Authentication/Authorization issues are a subset of possible reasons why CvpApiError could be raised.

ansible_collections/arista/cvp/plugins/module_utils/facts_tools.py

noredistribution · 2023-10-12T15:47:49Z

moved it to draft and holding off on this for now as looks like we could just use /api/v1/rest/ and fix this on cvprac side only

chetryan · 2023-10-13T14:21:24Z

ansible_collections/arista/cvp/plugins/modules/cv_facts_v3.py

+    try:
+      facts = facts_collector.facts(scope=ansible_module.params['facts'], regex_filter=ansible_module.params['regexp_filter'],
+                                    verbose=ansible_module.params['verbose'])
+    except Exception as e:


maybe we should only capture CvpApiError here, since that's the one we are raising, and want to present just the msg. For other Excpetions we might still want the full trace and those would be unexpected. Happy to hear what others think.

as discussed offline with CvpApiError ansible will throw the entire python traceback:

Traceback (most recent call last): File "/Users/tamas/.ansible/tmp/ansible-local-931202vqrfpkn/ansible-tmp-1697208253.829348-93185-191262445100178/AnsiballZ_cv_facts_v3.py", line 107, in <module> _ansiballz_main() File "/Users/tamas/.ansible/tmp/ansible-local-931202vqrfpkn/ansible-tmp-1697208253.829348-93185-191262445100178/AnsiballZ_cv_facts_v3.py", line 99, in _ansiballz_main invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS) File "/Users/tamas/.ansible/tmp/ansible-local-931202vqrfpkn/ansible-tmp-1697208253.829348-93185-191262445100178/AnsiballZ_cv_facts_v3.py", line 47, in invoke_module runpy.run_module(mod_name='ansible_collections.arista.cvp.plugins.modules.cv_facts_v3', init_globals=dict(_module_fqn='ansible_collections.arista.cvp.plugins.modules.cv_facts_v3', _modlib_path=modlib_path), File "<frozen runpy>", line 226, in run_module File "<frozen runpy>", line 98, in _run_module_code File "<frozen runpy>", line 88, in _run_code File "/var/folders/xj/58jh5s5x05n4d_hcd_ybhgmr0000gq/T/ansible_arista.cvp.cv_facts_v3_payload_qsyiw987/ansible_arista.cvp.cv_facts_v3_payload.zip/ansible_collections/arista/cvp/plugins/modules/cv_facts_v3.py", line 200, in <module> File "/var/folders/xj/58jh5s5x05n4d_hcd_ybhgmr0000gq/T/ansible_arista.cvp.cv_facts_v3_payload_qsyiw987/ansible_arista.cvp.cv_facts_v3_payload.zip/ansible_collections/arista/cvp/plugins/modules/cv_facts_v3.py", line 188, in main File "/var/folders/xj/58jh5s5x05n4d_hcd_ybhgmr0000gq/T/ansible_arista.cvp.cv_facts_v3_payload_qsyiw987/ansible_arista.cvp.cv_facts_v3_payload.zip/ansible_collections/arista/cvp/plugins/module_utils/facts_tools.py", line 378, in facts File "/var/folders/xj/58jh5s5x05n4d_hcd_ybhgmr0000gq/T/ansible_arista.cvp.cv_facts_v3_payload_qsyiw987/ansible_arista.cvp.cv_facts_v3_payload.zip/ansible_collections/arista/cvp/plugins/module_utils/facts_tools.py", line 592, in __fact_devices File "/Users/tamas/go/src/github.com/chetryan/cvprac/cvprac/cvp_api.py", line 598, in get_inventory data = self.clnt.get('/inventory/devices?provisioned=%s' % provisioned, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tamas/go/src/github.com/chetryan/cvprac/cvprac/cvp_client.py", line 905, in get return self._make_request('GET', url, timeout) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tamas/go/src/github.com/chetryan/cvprac/cvprac/cvp_client.py", line 656, in _make_request response = self._send_request(req_type, full_url, timeout, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/Users/tamas/go/src/github.com/chetryan/cvprac/cvprac/cvp_client.py", line 832, in _send_request self._is_good_response(response, '%s: %s ' % File "/Users/tamas/go/src/github.com/chetryan/cvprac/cvprac/cvp_client.py", line 443, in _is_good_response raise CvpRequestError(msg) cvprac.cvp_client_errors.CvpRequestError: GET: https://10.83.12.79:443/web/inventory/devices?provisioned=True : Request Error: Forbidden - 403 Forbidden module_stdout: '' msg: |- MODULE FAILURE See stdout/stderr for the exact error

vs

TASK [gather cvp facts] ******************************************************************************************************* fatal: [cv_server]: FAILED! => changed=false msg: |- GET: https://10.83.12.79:443/web/inventory/devices?provisioned=True : Request Error: Forbidden - 403 Forbidden

Good point. What about using CvpClientError ? I agree that all cvprac error should be handled and reported using fail_json.

yea that worked!

chetryan

LGTM

sugetha24 · 2023-11-09T21:54:18Z

@noredistribution and @chetryan, can we make these changes in all of the other modules as well?

chetryan · 2023-11-10T08:45:56Z

@noredistribution and @chetryan, can we make these changes in all of the other modules as well?

I think the logic could be extended to other modules. Should that be in this PR or in another one?

github-actions bot assigned noredistribution Oct 11, 2023

github-actions bot requested a review from guillaumeVilar October 11, 2023 18:43

github-actions bot added module: cv_facts_v3 module_utils: cv_client cv_client implementation issue labels Oct 11, 2023

github-actions bot requested a review from sugetha24 October 11, 2023 18:43

chetryan requested changes Oct 12, 2023

View reviewed changes

ansible_collections/arista/cvp/plugins/module_utils/facts_tools.py Outdated Show resolved Hide resolved

noredistribution marked this pull request as draft October 12, 2023 15:36

chetryan reviewed Oct 13, 2023

View reviewed changes

noredistribution changed the title ~~Fix(cv_facts_v3): return authorization error when svc token expires~~ Fix(cv_facts_v3): raise errors when svcaccount/user is not authorized Oct 13, 2023

noredistribution marked this pull request as ready for review October 13, 2023 15:01

chetryan approved these changes Oct 27, 2023

View reviewed changes

sugetha24 modified the milestones: v3.9.0, v3.8.0 Nov 1, 2023

sugetha24 force-pushed the issue675 branch 2 times, most recently from 6df740f to bd38309 Compare November 9, 2023 21:53

github-actions bot added module: cv_device_v3 Issue related to cv_device module in v3 module: cv_container_v3 Issue related to cv_container module in v3 module: cv_task_v3 Issue related to cv_tasks module in v3 labels Nov 11, 2023

noredistribution added 6 commits November 20, 2023 14:32

Fix(cv_facts_v3): return authorization error when svc token expires

21d5b6f

reverting previous changes and raising errors instead

17fe837

changing exception to CvpClientError

524fa26

fixing ansible lint issues

b14d714

fixing ansible lint issues

68b8726

adding CvpRequestError 403 handling for device tools

03cd0a0

noredistribution and others added 5 commits November 20, 2023 14:32

adding CvpRequestError 403 handling for tag tools

36ab311

pylint fix

c8c7248

adding CvpRequestError 403 handling for apply image

f89cbb6

adding CvpRequestError 403 handling for CC, task, container

5ffba45

update error messages for device tools

665d9b7

sugetha24 force-pushed the issue675 branch from b62f3cc to 665d9b7 Compare November 20, 2023 22:32

sugetha24 added 2 commits November 20, 2023 14:48

pep8 fix

306ab08

pylint fix-again

7eaeb26

sugetha24 merged commit 290363f into aristanetworks:devel Nov 20, 2023
18 checks passed

sugetha24 changed the title ~~Fix(cv_facts_v3): raise errors when svcaccount/user is not authorized~~ Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized #677

Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized #677

noredistribution commented Oct 11, 2023 •

edited

Loading

chetryan left a comment

noredistribution commented Oct 12, 2023

chetryan Oct 13, 2023

noredistribution Oct 13, 2023

chetryan Oct 13, 2023

noredistribution Oct 16, 2023

chetryan left a comment

sugetha24 commented Nov 9, 2023

chetryan commented Nov 10, 2023

Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized #677

Feat(cv_facts_v3): raise errors when svcaccount/user is not authorized #677

Conversation

noredistribution commented Oct 11, 2023 • edited Loading

Change Summary

Related Issue(s)

Component(s) name

Proposed changes

How to test

Checklist

User Checklist

Repository Checklist

chetryan left a comment

Choose a reason for hiding this comment

noredistribution commented Oct 12, 2023

chetryan Oct 13, 2023

Choose a reason for hiding this comment

noredistribution Oct 13, 2023

Choose a reason for hiding this comment

chetryan Oct 13, 2023

Choose a reason for hiding this comment

noredistribution Oct 16, 2023

Choose a reason for hiding this comment

chetryan left a comment

Choose a reason for hiding this comment

sugetha24 commented Nov 9, 2023

chetryan commented Nov 10, 2023

noredistribution commented Oct 11, 2023 •

edited

Loading