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 TLS support for Prometheus Reader #3055

Merged
merged 3 commits into from
Jun 4, 2021

Conversation

albertteoh
Copy link
Contributor

@albertteoh albertteoh commented Jun 4, 2021

Signed-off-by: albertteoh albert.teoh@logz.io

Which problem is this PR solving?

Short description of the changes

  • Adds TLS encryption to Prometheus server.

Testing

  • As documented, Prometheus does not natively support TLS encryption for connections to its HTTP API, however, it does suggest an alternative means of enforcing TLS encryption at the proxy layer with something like nginx.
  • This PR was tested using nginx as a reverse proxy to Prometheus, as suggested above.
# Queries succeed: Directly connect to Prometheus, bypassing nginx
$ METRICS_STORAGE_TYPE=prometheus ./cmd/query/query-darwin-amd64 \
  --prometheus.server-url http://localhost:9090

# Queries succeed: Enable TLS and provide public key
$ METRICS_STORAGE_TYPE=prometheus ./cmd/query/query-darwin-amd64 \
  --prometheus.server-url https://example.com/prometheus \
  --prometheus.tls.ca=example.com.crt \
  --prometheus.tls.enabled

# Queries fail: TLS not enabled
$ METRICS_STORAGE_TYPE=prometheus ./cmd/query/query-darwin-amd64 \
  --prometheus.server-url https://example.com/prometheus  \
  --prometheus.tls.ca=example.com.crt 
...failed executing metrics query: Post "https://example.com/prometheus/api/v1/query_range": x509: certificate signed by unknown authority...

# Queries fail: Missing public key
$ METRICS_STORAGE_TYPE=prometheus ./cmd/query/query-darwin-amd64 \
  --prometheus.server-url https://example.com/prometheus  \
  --prometheus.tls.enabled
...failed executing metrics query: Post "https://example.com/prometheus/api/v1/query_range": x509: certificate signed by unknown authority...

Signed-off-by: albertteoh <albert.teoh@logz.io>
Comment on lines -28 to +29
suffixHostPort = ".host-port"
suffixServerURL = ".server-url"
Copy link
Contributor Author

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 either http or https.
  • Consistency with ES options server-urls, and made more sense than host-port if requiring the protocol scheme.

@codecov
Copy link

codecov bot commented Jun 4, 2021

Codecov Report

Merging #3055 (e9d6712) into master (0b59004) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
plugin/metrics/prometheus/factory.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/metricsstore/reader.go 100.00% <100.00%> (ø)
plugin/metrics/prometheus/options.go 100.00% <100.00%> (ø)
...lugin/sampling/strategystore/adaptive/processor.go 99.07% <0.00%> (-0.93%) ⬇️
plugin/storage/integration/integration.go 77.34% <0.00%> (-0.56%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b59004...e9d6712. Read the comment docs.

@albertteoh albertteoh marked this pull request as ready for review June 4, 2021 08:58
@albertteoh albertteoh requested a review from a team as a code owner June 4, 2021 08:58
@albertteoh albertteoh changed the title Add TLS support Add TLS support for Prometheus Reader Jun 4, 2021

// 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"`
Copy link
Member

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.

Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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?

Copy link
Contributor Author

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.

albertteoh added 2 commits June 5, 2021 06:34
Signed-off-by: albertteoh <albert.teoh@logz.io>
@yurishkuro yurishkuro enabled auto-merge (squash) June 4, 2021 21:09
@albertteoh
Copy link
Contributor Author

Thanks for the review @yurishkuro!

@yurishkuro yurishkuro merged commit 013ecf2 into jaegertracing:master Jun 4, 2021
@albertteoh albertteoh deleted the 2954-add-tls branch June 5, 2021 07:33
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.

2 participants