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

Add support for TLS client cert/key in tls_config #442

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

simondeziel
Copy link
Member

@simondeziel simondeziel commented Jan 29, 2023

Context

The prometheus2 machine charm supports injecting a TLS client cert/key to use when scraping a given target.

This PR adds support for TLS client cert/key (cert_file/key_file) to the tls_config key.

Note: the LXD charm uses that feature when related with prometheus2 and we'd like to keep using it when related with prometheus-k8s.

Release Notes

Support TLS client authentication when scraping metrics.

Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

I think think this makes sense.
Comments inline.

tests/unit/test_charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
@simondeziel simondeziel force-pushed the tls-cert-key branch 3 times, most recently from b807548 to 94fc5ed Compare January 31, 2023 22:58
@sed-i sed-i requested a review from rbarry82 February 1, 2023 00:35
Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @simondeziel!

@simondeziel
Copy link
Member Author

@rbarry82 please let me know if you'd like me to make any changes, thanks!

Copy link
Contributor

@sed-i sed-i left a comment

Choose a reason for hiding this comment

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

Very sorry for the delay!
In the second pass I noticed two issues that are easy to fix.

src/charm.py Outdated Show resolved Hide resolved
src/charm.py Outdated Show resolved Hide resolved
tests/unit/test_charm.py Show resolved Hide resolved
@simondeziel simondeziel force-pushed the tls-cert-key branch 2 times, most recently from 4aff893 to 4711109 Compare April 11, 2023 23:39
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
…uested

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel simondeziel force-pushed the tls-cert-key branch 2 times, most recently from 0a6d244 to 3622e98 Compare April 12, 2023 01:14
@simondeziel
Copy link
Member Author

@sed-i I've integrated your feedback, please let me know if I missed something! Thanks!

@sed-i
Copy link
Contributor

sed-i commented Apr 12, 2023

Looks great, thank you!
There's a CI bug at the moment that prevents me from merging. Stay tuned :)

@sed-i
Copy link
Contributor

sed-i commented Apr 12, 2023

@simondeziel the CI was fixed. Do you mind pushing an empty commit to retrigger CI?

git commit --allow-empty -m "Retrigger CI (empty commit)" 

Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
@simondeziel
Copy link
Member Author

@sed-i it seems that "PR / Quality Checks / Integration Tests" is still waiting on something.

@rbarry82
Copy link
Contributor

@sed-i it seems that "PR / Quality Checks / Integration Tests" is still waiting on something.

These actually passed, from the looks of it. GH just isn't reporting it. This works in manual testing. Anything still pending @simondeziel ?

@simondeziel
Copy link
Member Author

@rbarry82 nope, all done on my side, thanks!

@rbarry82 rbarry82 merged commit b128cf2 into canonical:main Apr 12, 2023
@simondeziel simondeziel deleted the tls-cert-key branch April 12, 2023 17:53
@sed-i sed-i mentioned this pull request Jul 17, 2023
7 tasks
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.

None yet

3 participants