mimir: Add TLS config to S3 client #7959
Conversation
Signed-off-by: Levi Harrison <git@leviharrison.dev>
Signed-off-by: Levi Harrison <git@leviharrison.dev>
| TLSConfig TLSConfig `yaml:"tls_config"` | ||
| } | ||
|
|
||
| // TLSConfig configures the options for TLS connections. |
There was a problem hiding this comment.
Elsewhere in Mimir, we use tls.ClientConfig from dskit. Do you mind changing to that struct for consistency with existing TLS config? There's an example here. When you do, it probably makes sense to have the YAML and CLI flags be inline like the other places it's used:
`yaml:",inline"`
There was a problem hiding this comment.
A few notes about this request that I have difficulties understanding:
Firstly, I see that insecure_skip_verify which is present on tls.ClientConfig from dskit is being overridden.
As mentioned in previous MR (#2652)
Note: Although I mostly copied the TLS config from Thanos, I omitted insecure_skip_verify because its value is overridden by the field of the same name in the parent HTTP config and is seemingly useless:
I believe that enabling this as a configuration option in both places is not the correct way to go.
Secondly, the original suggestion was to allow only relevant values to be passed to the exthttp client as exthttp.TLSConfig doesn't have some of the options allowed with tls.ClientConfig from dskit (kind of the same issue as the first point) which I don't really know how to handle.
I apologize if my wording sounds a bit rude, I'm quite new to Mimir's codebase so I might not understand this correctly.
There was a problem hiding this comment.
Ah, so it does. In that case, could you adjust the struct a bit so that the settings being added match names if we were using tls.ClientConfig?
E.g. make TLSConfig inline with HTTPConfig , name the fields the same like tls_ca_path, and name the CLI flags like -ruler-storage.s3.http.tls-ca-path, etc
There was a problem hiding this comment.
I'll do so, please note that other components reference insecure_skip_verify as tls_insecure_skip_verify which would cause some more inconsistencies later on.
There was a problem hiding this comment.
Another issue that shows in the tests, this PR will change <prefix>.s3.http.tls.<option> to <prefix>.s3.http.tls-<options>. Currently I resolved it by running make reference-help but it sounds like a big change to me, are there any guidelines on changing configuration parameters naming to this degree?
There was a problem hiding this comment.
I'll do so, please note that other components reference insecure_skip_verify as tls_insecure_skip_verify which would cause some more inconsistencies later on.
That's fine to leave as-is. We can deprecate and replace the old option in another PR or just leave it.
Another issue that shows in the tests, this PR will change .s3.http.tls. to .s3.http.tls-. Currently I resolved it by running make reference-help but it sounds like a big change to me, are there any guidelines on changing configuration parameters naming to this degree?
It's expected to need to run make reference-help after changing CLI flags. These are all new options so there shouldn't be any issue changing them during the course of this PR.
CHANGELOG.md
Outdated
| @@ -36,9 +36,11 @@ | |||
| * [BUGFIX] Do not wrap error message with `sampled 1/<frequency>` if it's not actually sampled. #7784 | |||
| * [BUGFIX] Store-gateway: do not track cortex_querier_blocks_consistency_checks_failed_total metric if query has been canceled or interrued due to any error not related to blocks consistency check failed. #7752 | |||
| * [BUGFIX] Ingester: ignore instances with no tokens when calculating local limits to prevent discards during ingester scale-up #7881 | |||
| * [ENHANCEMENT] Expose TLS configiration for the S3 backend client. #2652 | |||
There was a problem hiding this comment.
Can you move this with the other [ENHANCEMENT] entries?
| TLSConfig TLSConfig `yaml:"tls_config"` | ||
| } | ||
|
|
||
| // TLSConfig configures the options for TLS connections. |
There was a problem hiding this comment.
I'll do so, please note that other components reference insecure_skip_verify as tls_insecure_skip_verify which would cause some more inconsistencies later on.
That's fine to leave as-is. We can deprecate and replace the old option in another PR or just leave it.
Another issue that shows in the tests, this PR will change .s3.http.tls. to .s3.http.tls-. Currently I resolved it by running make reference-help but it sounds like a big change to me, are there any guidelines on changing configuration parameters naming to this degree?
It's expected to need to run make reference-help after changing CLI flags. These are all new options so there shouldn't be any issue changing them during the course of this PR.
Co-authored-by: Jack Baldry <jack.baldry@grafana.com>
Co-authored-by: Jack Baldry <jack.baldry@grafana.com>
What this PR does
Adds the ability to configure the TLS config for the Thanos S3 client.
Continuing #2652
Which issue(s) this PR fixes or relates to
Fixes #1981, Fixes #1796
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.