Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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
mimir: Add TLS config to S3 client #7959
mimir: Add TLS config to S3 client #7959
Changes from 12 commits
70b9048
f638e6c
90aa4cf
2a07f42
d5432c3
c1507f5
952aa26
34b2b25
d5e0720
bff24ac
d3e33ab
01f5414
066e973
63431bc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few notes about this request that I have difficulties understanding:
Firstly, I see that
insecure_skip_verify
which is present ontls.ClientConfig
from dskit is being overridden.As mentioned in previous MR (#2652)
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 withtls.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withHTTPConfig
, name the fields the same liketls_ca_path
, and name the CLI flags like-ruler-storage.s3.http.tls-ca-path
, etcThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do so, please note that other components reference
insecure_skip_verify
astls_insecure_skip_verify
which would cause some more inconsistencies later on.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 runningmake 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine to leave as-is. We can deprecate and replace the old option in another PR or just leave it.
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.