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>
knylander-grafana
left a comment
knylander-grafana
left a comment
There was a problem hiding this comment.
Thank you for adding doc.
Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
|
|
||
| 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>
|
|
||
| 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.
* Adjust s3 config for dskit/crypto/tls common configuration Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update changelog Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Add nil config check Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Restore default tls min version from default minio client Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update config manifest Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Include docs to the other supported tls configurations Signed-off-by: Zach Leslie <zach.leslie@grafana.com> * Update tls_min_version flag name to be consistent Signed-off-by: Zach Leslie <zach.leslie@grafana.com> --------- Signed-off-by: Zach Leslie <zach.leslie@grafana.com>
4 participants
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]