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

Resolve meraki webhook fail when none defined #436

Merged
merged 5 commits into from
Feb 1, 2023
Merged

Resolve meraki webhook fail when none defined #436

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3dc5d7
Added test coverage to check behaviour when there are no webhooks def…
mystery-rabbit Jan 27, 2023
af21031
removed hard coded list subscript to make sanitize_no_log_values cope…
mystery-rabbit Jan 27, 2023
92a2efc
resolved linting and sanity errors
mystery-rabbit Jan 27, 2023
12fa186
Added changelog fragment
mystery-rabbit Jan 30, 2023
16c45a7
refactored patch to correctly deal with the variety of types
mystery-rabbit Jan 30, 2023
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
2 changes: 2 additions & 0 deletions changelogs/fragments/Resolve_meraki_webhook_fail_when_none_defined.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
bugfix:
- Resolved an issue where an empty response from the API triggered an exception in module meraki_webhook
10 changes: 7 additions & 3 deletions plugins/modules/meraki_webhook.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,9 +210,13 @@ def sanitize_no_log_values(meraki):
except KeyError:
pass
try:
meraki.result["data"][0][
"shared_secret"
] = "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER"
i = 0
while i < len(meraki.result["data"]):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why a while loop instead of a for loop here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah. Your comment has made me re-look at this. There is still an issue here so it's not ready for merge.

So that is where i started - I've just tried this again to check - and I think i've been miss-interpreted an error i saw..

with ...

        for i in meraki.result['data']:
            i[
                "shared_secret"
            ] = "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER"

... I get the "Query one webhook" test failing with TypeError: 'str' object does not support item assignment. I took that to be an issue with the indirect assignment afforded by the for loop variable and so used a while loop instead with direct assignment.

I've miss-understood the meraki.result behaviour - assuming data is always a list - be it empty or with multiple entries. With a query naming a web hook, I'm guessing now data is a dict, not a single entry list pointing to a dict.

I'll look at this again.

meraki.result["data"][i][
"shared_secret"
] = "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER"
i += 1

except KeyError:
pass
try:
Expand Down
16 changes: 16 additions & 0 deletions tests/integration/targets/meraki_webhook/tasks/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,22 @@
net_name: '{{test_net_name}}'
type: appliance

- name: Query for any webhooks expecting None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall this looks good but should there be a new test looking for the shared_secret value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can certainly use the opportunity to call out the expected behaviour via the test cases yes. that's probably just the de-commenting of the idempotency tests and tightening the assert statements.

(.venv) vagrant@terra01:~/ansible_collections/cisco/meraki$ grep "\- name" tests/integration/targets/meraki_webhook/tasks/tests.yml 
  - name: Test an API key is provided
  - name: Create test network
  - name: Query for any webhooks expecting None
  - name: Create webhook with check mode
  - name: Create webhook
  - name: Query all webhooks expecting 1
  - name: Query one webhook
  - name: Query one webhook with id
  - name: Update webhook with check mode
  # - name: Update webhook
  # - name: Update webhook with idempotency
  # - name: Update webhook with id
  - name: Create webhook payload template for a webhook
  - name: Debug payload_template
  - name: Create webhook with a payload template
  - name: Delete webhook with payload template
  - name: Create test webhook
  - name: Get webhook status
  - name: Query all webhooks
  - name: Delete webhook invalid webhook
  - name: Delete webhook in check mode
  - name: Delete webhook
  - name: Delete webhook payload template for a webhook
  - name: Delete test network

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbreit

  • I added a test to my local branch testing that a seemingly idempotent change is always going to return "changed" and no shared_secret. passed.

  • I then added a test that did a change without shared_secret set - expecting changed='false' - and that's picked up another issue.

API behaviour - if the shared secret is not set in the request, then the API will still return 200 OK - it does not touch the secret - only updating any changed params. (verified through GUI/Postman)

module behaviour - if the shared secret param is not set, the module will set shared secret to null, and send that - API will return 400 Error. To my mind, it should not be setting the param at all.

I can see how to resolve that - any issues with me adding that to the PR?

meraki_webhook:
auth_key: '{{auth_key}}'
state: query
org_name: '{{test_org_name}}'
net_name: '{{test_net_name}}'
register: query_none

- debug:
var: query_none

- assert:
that:
- query_none is not changed
- query_none.data[0] is not defined

- name: Create webhook with check mode
meraki_webhook:
auth_key: '{{auth_key}}'
Expand Down