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.

Sign up for GitHub

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

Jump to bottom

prometheus_client plugin: Add transport encryption via TLS and authentication via http basic_auth #3719

Merged
merged 6 commits into from
Feb 1, 2018
Merged

prometheus_client plugin: Add transport encryption via TLS and authentication via http basic_auth #3719

merged 6 commits into from
Feb 1, 2018

Conversation

phlipse
Copy link
Contributor

@phlipse phlipse commented Jan 24, 2018 •
edited
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Hello community,
this commit adds support for TLS transport encryption and http basic_auth for authentication in prometheus_client output plugin.

Configurations:
Prometheus:

- job_name: 'ssl_test'
    scheme: 'https'
    tls_config:
      #insecure_skip_verify: true
    basic_auth:
      username: 'Foo'
      password: 'Bar'
    static_configs:
      - targets: ['localhost:9200']`

Telegraf configuration:

[[outputs.prometheus_client]]
  listen = ":9200"
  tls_cert = "test.crt"
  tls_key = "test.key"
  basic_username = "Foo"
  basic_password = "Bar"
  path = "/metrics"
  expiration_interval = "60s"

Greetings,
Philipp

@phlipse phlipse changed the title Add transport encryption via TLS and authentication via http basic_auth prometheus_client plugin: Add transport encryption via TLS and authentication via http basic_auth Jan 24, 2018
@phlipse
Copy link
Contributor Author

phlipse commented Jan 24, 2018 •
edited
Loading

Sorry, I have implemented this last november and took it to the current master. I didn't recognize the parameter change in promhttp.HandlerFor() call. Now all tests should pass.

danielnelson
danielnelson reviewed Jan 29, 2018
View reviewed changes
Copy link
Contributor

@danielnelson danielnelson left a comment

Choose a reason for hiding this comment

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

Thanks, can you make the following changes just for consistency between plugins?

plugins/outputs/prometheus_client/README.md Outdated
@@ -10,6 +10,16 @@ This plugin starts a [Prometheus](https://prometheus.io/) Client, it exposes all
# Address to listen on
listen = ":9273"

# Use TLS
tls = true
tls_crt = "/etc/ssl/telegraf.crt"
Copy link
Contributor

Choose a reason for hiding this comment

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

Call this tls_cert

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in de9d7fa

plugins/outputs/prometheus_client/README.md Outdated
@@ -10,6 +10,16 @@ This plugin starts a [Prometheus](https://prometheus.io/) Client, it exposes all
# Address to listen on
listen = ":9273"

# Use TLS
tls = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this option, if other tls options are set then enable tls, see http_listener input for an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in ffc0690

plugins/outputs/prometheus_client/README.md Outdated
# Use http basic authentication
basic_auth = true
username = "Foo"
password = "Bar"
Copy link
Contributor

Choose a reason for hiding this comment

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

For basic auth lets do it like in #3496, enable if either username or password is set and no enable flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in bfe91ac

@danielnelson danielnelson added feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin area/prometheus labels Jan 29, 2018
danielnelson
danielnelson reviewed Jan 31, 2018
View reviewed changes
plugins/outputs/prometheus_client/prometheus_client.go Outdated
PreferServerCipherSuites: true,
CipherSuites: []uint16{
tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we be using p.TLSCert and p.TLSKey somewhere? Why are we so specific on the ciphers and tls version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We specify the certificate and key in ListenAndServeTLS() in line 171.

The specified cipher is the mandatory cipher for HTTP/2 which is used by prometheus when using TLS. If you want we can leave it open but it is the mandatory cipher for HTTP/2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, yeah lets leave CipherSuites and PreferServerCipherSuites unset so that it will use the defaults.

@phlipse
Copy link
Contributor Author

phlipse commented Feb 1, 2018

I have removed the specific TLS settings and use the defaults.

@danielnelson danielnelson added this to the 1.6.0 milestone Feb 1, 2018
@danielnelson danielnelson merged commit a263557 into influxdata:master Feb 1, 2018
maxunt pushed a commit that referenced this pull request Jun 26, 2018
@phlipse @danielnelson
Add TLS and http basic_auth to prometheus_client output (#3719)
b39dd3a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielnelson danielnelson danielnelson left review comments

Assignees
No one assigned
Labels
area/prometheus feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin
Projects
None yet
Milestone
1.6.0
Development

Successfully merging this pull request may close these issues.
2 participants
@phlipse @danielnelson