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.

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(lookup/grafana_dashboard): add custom certs verification logic #356

Merged
merged 3 commits into from
Apr 24, 2024

Conversation

shantanoo-desai
Copy link
Contributor

SUMMARY

This fix adds the validate_certs, ca_path options to the lookup plugin. Both parameters comply with the get_url functionality of ansible-core and provides additional utility to perform lookup of dashboards from Grafana instances that are configured with Self-Signed Certificates.

validate_certs option value defaults to true - following the pattern of url lookup plugin from the Core.

ca_path option value is set explicitly when using the plugin else defaults to None.

Fixes #346

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

Lookup Plugin for Grafana Dashboards

ADDITIONAL INFORMATION

Tested with a Grafana Docker image with Self-Signed Certificates as well as a local Certificate Authority. The Local CA along with a self-signed certificate is used to form a chain.crt and validated as ca_path option for the plugin.

@shantanoo-desai shantanoo-desai force-pushed the main branch 2 times, most recently from 527e3e8 to fefb2e6 Compare April 14, 2024 12:14
@shantanoo-desai
Copy link
Contributor Author

/cc @rndmh3ro

Copy link

codecov bot commented Apr 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.18%. Comparing base (4d0af1a) to head (fefb2e6).

❗ Current head fefb2e6 differs from pull request most recent head 2fcf0c2. Consider uploading reports for the commit 2fcf0c2 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #356       +/-   ##
===========================================
+ Coverage   23.42%   65.18%   +41.76%     
===========================================
  Files          15        9        -6     
  Lines        1601      968      -633     
  Branches      336      133      -203     
===========================================
+ Hits          375      631      +256     
+ Misses       1219      309      -910     
- Partials        7       28       +21     
Flag Coverage Δ
sanity ?
units 65.18% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shantanoo-desai
Copy link
Contributor Author

shantanoo-desai commented Apr 14, 2024

@Nemental One of the CI Sanity check for development fails for a test file which doesn't belong to my PR. The other Lint problem will be solved by me now. I am not sure why the particular test file for Sanity Develop Job fails?

Edit: also I used the wrong number for the fragments file (used issue number instead of PR number)

Tasks

  • Refactor python file for linting based on black
  • Rename fragments to PR number and not Issue Number

@Nemental
Copy link
Collaborator

Hey @shantanoo-desai
Thanks for your PR!
I'll do a final review of the changes tomorrow, but so far it looks pretty good. The remaining sanity failings have already been fixed in another PR.

@shantanoo-desai
Copy link
Contributor Author

shantanoo-desai commented Apr 14, 2024

Update

the two Integration Tests seem to fail with HTTP 429 Too Many Requests on the Grafana Endpoint when trying to retrieve some dashboards. I am assuming the tests would just need to be re-run and nothing from my patch should actually break the tests.

This fix adds the `validate_certs`, `ca_path` options to the lookup
plugin. Both parameters comply with the `get_url` functionality
of ansible-core and provides additional utility to perform lookup
of dashboards from Grafana instances that are configured with
Self-Signed Certificates.

`validate_certs` option value defaults to `true` - following the
pattern of `url` lookup plugin from the Core.

`ca_path` option value is set explicitly when using the plugin else
defaults to `None`.

closes ansible-collections#346

Signed-off-by: Shantanoo 'Shan' Desai <[email protected]>
@shantanoo-desai
Copy link
Contributor Author

shantanoo-desai commented Apr 16, 2024

@Nemental and @rndmh3ro The PR is rebased to main branch and the changes have been made. The Sanity tests still seem to fail however. Because of the failure strategy in the CI system, I am not sure why some Sanity Checks fail on some branches and some don't.

@Nemental
Copy link
Collaborator

The remaining sanity workflow failures are not related with your changes and have already been fixed in #354 :)

Copy link
Collaborator

@Nemental Nemental left a comment

Choose a reason for hiding this comment

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

lgtm! 🥳

@Nemental Nemental merged commit fc4f19a into ansible-collections:main Apr 24, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

grafana_dashboard lookup plugin does not accept validate_certs as a parameter and does not perform accordingly
3 participants