-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 TLS support for Prometheus Reader #3055
Conversation
Signed-off-by: albertteoh <albert.teoh@logz.io>
suffixHostPort = ".host-port" | ||
suffixServerURL = ".server-url" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation for renaming:
- To use TLS with Prometheus requires the protocol scheme
https
, as with how ES's server URLs are defined. Hence, I felt it made more sense that users are required to provide the appropriate protocol scheme of eitherhttp
orhttps
. - Consistency with ES options
server-urls
, and made more sense thanhost-port
if requiring the protocol scheme.
Codecov Report
@@ Coverage Diff @@
## master #3055 +/- ##
==========================================
- Coverage 96.03% 96.01% -0.03%
==========================================
Files 229 229
Lines 9941 9956 +15
==========================================
+ Hits 9547 9559 +12
- Misses 325 328 +3
Partials 69 69
Continue to review full report at Codecov.
|
pkg/prometheus/config/config.go
Outdated
|
||
// Configuration describes the options to customize the storage behavior. | ||
type Configuration struct { | ||
HostPort string `validate:"nonzero" mapstructure:"server"` | ||
ConnectTimeout time.Duration `validate:"nonzero" mapstructure:"timeout"` | ||
ServerURL string `validate:"nonzero" mapstructure:"server"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate:"nonzero" mapstructure:"server"
Where are these being used? I thought they were introduced when @pavolloffay tried to use OTEL's config mechanism to populate Jaeger configs, but we stopped doing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not used, I would remove the annotations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The annotations are based on existing example config structs, and I didn't know what they were used for as well tbh; removed.
|
||
func getHTTPRoundTripper(c *config.Configuration, logger *zap.Logger) (http.RoundTripper, error) { | ||
if !c.TLS.Enabled { | ||
return nil, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that means you never want to support non-TLS connections?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It means to use the DefaultRoundTripper
supplied by Prometheus client (the client will do this on our behalf), but you make a good point that we lose our ConnectTimeout
configuration with this approach. I've written a test to fail this code and fixed the implementation.
Signed-off-by: albertteoh <albert.teoh@logz.io>
Thanks for the review @yurishkuro! |
Signed-off-by: albertteoh albert.teoh@logz.io
Which problem is this PR solving?
Short description of the changes
Testing