Use TLS config from dskit
#2407
Conversation
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
zalegrala
requested review from
knylander-grafana,
joe-elliott,
annanay25,
mdisibio,
mapno and
kvrhdn
as code owners
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
| @@ -372,8 +376,13 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) { | |||
| return nil, errors.Wrap(err, "create minio.DefaultTransport") | |||
| } | |||
|
|
|||
| if cfg.InsecureSkipVerify { | |||
| customTransport.TLSClientConfig.InsecureSkipVerify = true | |||
| tlsConfig, err := cfg.GetTLSConfig() | |||
There was a problem hiding this comment.
how does this work? i'm reading the code here:
https://github.com/grafana/dskit/blob/main/crypto/tls/tls.go#L87
and it looks like you have to set a reader somewhere?
Also, it looks like GetTLSConfig() always returns a value and so it will always be set on line 385 below. Is this ok? Should there be a way to not set this and just use defaults?
There was a problem hiding this comment.
Good find. That commit isn't in the branch currently, but it looks like was introduced here: grafana/dskit#274
I assume the reader interface was just to provide a little more flexibility on where to read the config data.
The nil check is just a habit when I see a pointer that could be nil, just in case the implementation changes we're still being defensive. I can remove.
I'll double check, but it looked to me like if we don't set the config items, then we get a mostly default tls config, which looks okay to me. Also note, that what we got back from the minio tls code is non-default, and so we set the min version in our config RegisterFlagsAndApplyDefaults to account for this.
There was a problem hiding this comment.
ok, rereading this makes way more sense. I missed that GetTLSConfig() is being called on the composed struct above. What is returned here will be the product of the settings. Wuf.
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
| @@ -372,8 +376,13 @@ func createCore(cfg *Config, hedge bool) (*minio.Core, error) { | |||
| return nil, errors.Wrap(err, "create minio.DefaultTransport") | |||
| } | |||
|
|
|||
| if cfg.InsecureSkipVerify { | |||
| customTransport.TLSClientConfig.InsecureSkipVerify = true | |||
| tlsConfig, err := cfg.GetTLSConfig() | |||
There was a problem hiding this comment.
ok, rereading this makes way more sense. I missed that GetTLSConfig() is being called on the composed struct above. What is returned here will be the product of the settings. Wuf.
Here we implement the initial TLS configuration options from dskit on the s3 configuration. The change here is pretty minimal, but will set us up to make use of the other TLS common components, such as CA cert file loading and server name. Currently only the s3 config is modified, but I can see this impacting the others as well later.
What this PR does:
storage.trace.s3.insecure_skip_verifyconfig optionWhich issue(s) this PR fixes:
Fixes #2262
Fixes #1914
Fixes #2373
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]