config: reload cert files from disk automatically by simonpasquier · Pull Request #173 · prometheus/common

simonpasquier · 2019-02-28T15:09:39Z

brian-brazil · 2019-02-28T15:10:44Z

This would only apply on reload, it shouldn't require that to be in line with how the other auth files work.

brancz · 2019-02-28T16:35:52Z

@brian-brazil Are you saying the other file references need to be handled as well, or in the same way? As far as I can tell the only other one in the http config is the bearerTokenFile, which is already handled by re-reading on every scrape. Not objecting, just trying to understand what you meant.

brian-brazil · 2019-02-28T17:23:09Z

The other ones are already handled this way.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier · 2019-03-04T16:27:20Z

@brian-brazil @brancz can you have a look again? FWIW I've tested it locally.
This PR is using tls.Config.GetClientCertificate which is only available since Go 1.8. I've assumed that the change doesn't have to be made for earlier versions of Go so I've added // +build go1.8 directives. If this is wrong, I could amend the PR to remove this "limitation".

brian-brazil · 2019-03-05T10:08:56Z

I've assumed that the change doesn't have to be made for earlier versions of Go so I've added // +build go1.8 directives. If this is wrong, I could amend the PR to remove this "limitation".

This won't be used by client_golang, so that seems a safe assumption. FYI @beorn7

brian-brazil · 2019-03-05T10:16:39Z


-	if cfg.BasicAuth != nil {
-		rt = NewBasicAuthRoundTripper(cfg.BasicAuth.Username, cfg.BasicAuth.Password, cfg.BasicAuth.PasswordFile, rt)
+	if len(cfg.TLSConfig.CAFile) == 0 {


I'm not getting what you're trying to do here.

If no CA file is provided, we don't need a round tripper that reloads the CA. So we return a "normal" round-tripper.

What if the root changes?

Which root?

I meant the CA (as that's usually the root), but that would miss the client ssl auth changing.

Client cert and key files are handled differently: we leverage tls.Config.GetClientCertificate so they are always read from disk.

https://github.com/prometheus/common/pull/173/files#diff-a2356a3b837239d300d6a0326a452aafR319

https://github.com/prometheus/common/pull/173/files#diff-a2356a3b837239d300d6a0326a452aafR345

beorn7 · 2019-03-05T13:43:52Z

This won't be used by client_golang, so that seems a safe assumption.

And even if we did at some point in the future, I see little problems raising the min requirement for client_golang to 1.8. Those are ancient versions, and while I think we should support them as long as it doesn't cause any overhead, we should not commit to more than that.

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

brian-brazil · 2019-03-07T13:29:58Z

 }

+func (rt *bearerAuthRoundTripper) CloseIdleConnections() {
+	if ci, ok := rt.rt.(closeIdler); ok {


This is cleaner in Go 1.12 as it exposes this directly, without having to reach into the transport

You refer to client.CloseIdleConnections()? I don't know how we can avoid going through the nested round-trippers until we call http.Transport.CloseIdleConnections().

https://golang.org/src/net/http/client.go?s=27593:27632#L841

Have we even switched over to go 1.12 everywhere? I feel like it might be a little early to use that. I think I'd prefer this approach at this point.

Can we add a TODO at least to clean this up?

I'm still not clear about what the TODO should be about... AFAICT consumers of the library using Go 1.12 will be able to do this and it will just work:

client, err := config.NewClient(...) ... client.CloseIdleConnections()

But again I don't see how we could avoid having CloseIdleConnections() for all the custom round-trippers if we want client.CloseIdleConnections() to be effective.

brancz · 2019-03-07T14:24:37Z

lgtm 👍

roidelapluie · 2019-03-07T21:06:41Z

@simonpasquier this is so awesome. I will be able to drop my https://github.com/roidelapluie/sslproxy :)

brian-brazil · 2019-03-11T13:49:31Z

+}
+
+func (t *tlsRoundTripper) CloseIdleConnections() {
+	t.mtx.Lock()


Wouldn't this be a read lock?

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

This change bumps github.com/prometheus/common to include prometheus/common#173 Signed-off-by: Simon Pasquier <spasquie@redhat.com>

* Reload certificates from disk automatically This change bumps github.com/prometheus/common to include prometheus/common#173 Signed-off-by: Simon Pasquier <spasquie@redhat.com> * scrape: close idle connections on reload/stop Signed-off-by: Simon Pasquier <spasquie@redhat.com> * use v0.3.0 tag Signed-off-by: Simon Pasquier <spasquie@redhat.com>

Trace Websockets requests

simonpasquier force-pushed the fix-4155 branch from 9ccc579 to 8c3ca15 Compare March 4, 2019 10:48

simonpasquier changed the title ~~config: add CheckFiles() method to TLSConfig~~ config: reload cert files from disk automatically Mar 4, 2019

config: reload cert files from disk automatically

ccc5b66

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the fix-4155 branch 3 times, most recently from 844be5f to d8d76bd Compare March 4, 2019 13:58

Support Go < 1.8

d8829bc

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier force-pushed the fix-4155 branch from d8d76bd to d8829bc Compare March 4, 2019 14:02

brian-brazil reviewed Mar 5, 2019

View reviewed changes

Close idle connections for the previous round-tripper

67171c8

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

brian-brazil reviewed Mar 7, 2019

View reviewed changes

brian-brazil reviewed Mar 11, 2019

View reviewed changes

Change to read-lock

e1e644e

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

brian-brazil merged commit e6686eb into prometheus:master Mar 12, 2019

simonpasquier deleted the fix-4155 branch March 12, 2019 11:44

simonpasquier mentioned this pull request Mar 12, 2019

Reload certificates from disk automatically + bump to v2.7.2 openshift/prometheus#26

Merged

simonpasquier mentioned this pull request Mar 12, 2019

Bump prometheus/common to v0.3.0 prometheus/prometheus#5344

Merged

simonpasquier added a commit to simonpasquier/prometheus that referenced this pull request Apr 10, 2019

Reload certificates from disk automatically

2316329

This change bumps github.com/prometheus/common to include prometheus/common#173 Signed-off-by: Simon Pasquier <spasquie@redhat.com>

fujin mentioned this pull request Jun 30, 2021

Reload TLS certificates / keys from disk automatically VictoriaMetrics/VictoriaMetrics#1420

Closed

alanprot pushed a commit to alanprot/common that referenced this pull request Mar 15, 2023

Merge pull request prometheus#173 from weaveworks/trace-websockets

f95aa0e

Trace Websockets requests

Uh oh!

Conversation

simonpasquier commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-brazil commented Feb 28, 2019

Uh oh!

brancz commented Feb 28, 2019

Uh oh!

brian-brazil commented Feb 28, 2019

Uh oh!

simonpasquier commented Mar 4, 2019

Uh oh!

brian-brazil commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

beorn7 commented Mar 5, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

simonpasquier Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brancz commented Mar 7, 2019

Uh oh!

roidelapluie commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

simonpasquier commented Feb 28, 2019 •

edited

Loading

simonpasquier Mar 7, 2019 •

edited

Loading