Support TLS client authentication #264
Conversation
Codecov Report
@@ Coverage Diff @@
## master #264 +/- ##
=========================================
Coverage ? 75.82%
=========================================
Files ? 2
Lines ? 761
Branches ? 0
=========================================
Hits ? 577
Misses ? 156
Partials ? 28
Continue to review full report at Codecov.
|
|
@joesis - are you still working on this PR? The cert support looks good and, once the README is done, I'd be happy to merge this. |
|
Okay changed README @oliver006. |
|
The last build failed with a rather odd error and it seems like there's stuff missing from your drone file (the redis 2.8 test instance is not up). |
|
Hmm I could have sworn that I saw CI passed but that can't be true... Rebased master and all green now! |
README.md
Outdated
| @@ -128,6 +128,8 @@ redis-only-metrics | REDIS_EXPORTER_REDIS_ONLY_METRICS | Whether to also | |||
| include-system-metrics | REDIS_EXPORTER_INCL_SYSTEM_METRICS | Whether to include system metrics like `total_system_memory_bytes`, defaults to false. | |||
| is-tile38 | REDIS_EXPORTER_IS_TILE38 | Whether to scrape Tile38 specific metrics, defaults to false. | |||
| skip-tls-verification | REDIS_EXPORTER_SKIP_TLS_VERIFICATION | Whether to to skip TLS verification | |||
| tls-client-key-file | REDIS_EXPORTER_TLS_CLIENT_KEY_FILE | Full path to the client key file if the server requires TLS client authentication | |||
There was a problem hiding this comment.
Question: your docs say full path to the client key file. Does that mean the expected parameter is a path (and the redis lib figures out what the file is) or is it the full name of the client key file?
There was a problem hiding this comment.
fyi, the text here is somewhat inconsistent with the --help output which says Optional client certificate file...
There was a problem hiding this comment.
It's the path including the file name which is passed to tls.LoadX509KeyPair. I should make the doc consistent with the flag description. Which is less ambiguous to you?
There was a problem hiding this comment.
Yeah, in that case maybe dbe a bit more explicit that it's a file name that's expected:
e.g. Name of the client key file (including full path) if the server requires TLS client authentication
There was a problem hiding this comment.
Updated both the README and flag descriptions!
|
Looks good, just one last question regarding the README. |
|
Released as https://github.com/oliver006/redis_exporter/releases/tag/v1.0.3 - thanks for the PR! |
It adds two features to support scraping
redissinstances in our setup.rediss.I'm not sure if it's any useful to other users but if the basic ideas are accepted, I can change tests and README accordingly.