Skip to content
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.

Sign up for GitHub

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

Jump to bottom

mimir: Add TLS config to S3 client #7959

Merged
merged 14 commits into from
Apr 25, 2024
Merged

mimir: Add TLS config to S3 client #7959

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
70b9048
Add tls config to s3
LeviHarrison Aug 4, 2022
f638e6c
Add changelog entry
LeviHarrison Aug 4, 2022
90aa4cf
Merge branch 'main' of https://github.com/grafana/mimir into feat/s3-…
Itaykal Apr 19, 2024
2a07f42
Update CHANGELOG.md
Itaykal Apr 19, 2024
d5432c3
Merge branch 'main' into feat/s3-tls-config
Itaykal Apr 24, 2024
c1507f5
fix: using = instead of :
Itaykal Apr 24, 2024
952aa26
fix: moved ENHANCEMENT
Itaykal Apr 24, 2024
34b2b25
fix: converted naming to be compatible with tls.ClientConfig from dskit
Itaykal Apr 24, 2024
d5e0720
fix: flattened TLSConfig
Itaykal Apr 24, 2024
bff24ac
fix: lined up config usage to tls.ClientConfig
Itaykal Apr 24, 2024
d3e33ab
make docs
Itaykal Apr 24, 2024
01f5414
make reference-help
Itaykal Apr 24, 2024
066e973
Update CHANGELOG.md
Itaykal Apr 25, 2024
63431bc
Update CHANGELOG.md
Itaykal Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this with the other [ENHANCEMENT] entries?

* [BUGFIX] Ingester: do not reuse exemplars slice in the write request if there are more than 10 exemplars per series. This should help to reduce the in-use memory in case of few requests with a very large number of exemplars. #7936
* [BUGFIX] Distributor: fix down scaling of native histograms in the distributor when timeseries unmarshal cache is in use. #7947


Itaykal marked this conversation as resolved.
Show resolved Hide resolved
### Mixin

* [CHANGE] Alerts: Removed obsolete `MimirQueriesIncorrect` alert that used test-exporter metrics. Test-exporter support was however removed in Mimir 2.0 release. #7774
Expand Down
216 changes: 216 additions & 0 deletions cmd/mimir/config-descriptor.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6054,6 +6054,60 @@
"fieldFlag": "blocks-storage.s3.max-connections-per-host",
"fieldType": "int",
"fieldCategory": "advanced"
},
{
"kind": "block",
"name": "tls_config",
"required": false,
"desc": "",
"blockEntries": [
{
"kind": "field",
"name": "ca_file",
"required": false,
"desc": "The CA certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "blocks-storage.s3.http.tls.ca-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "cert_file",
"required": false,
"desc": "The client certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "blocks-storage.s3.http.tls.cert-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "key_file",
"required": false,
"desc": "The client key file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "blocks-storage.s3.http.tls.key-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "server_name",
"required": false,
"desc": "The name of the server for verification.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "blocks-storage.s3.http.tls.server-name",
"fieldType": "string",
"fieldCategory": "advanced"
}
],
"fieldValue": null,
"fieldDefaultValue": null
}
],
"fieldValue": null,
Expand Down Expand Up @@ -11939,6 +11993,60 @@
"fieldFlag": "ruler-storage.s3.max-connections-per-host",
"fieldType": "int",
"fieldCategory": "advanced"
},
{
"kind": "block",
"name": "tls_config",
"required": false,
"desc": "",
"blockEntries": [
{
"kind": "field",
"name": "ca_file",
"required": false,
"desc": "The CA certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "ruler-storage.s3.http.tls.ca-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "cert_file",
"required": false,
"desc": "The client certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "ruler-storage.s3.http.tls.cert-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "key_file",
"required": false,
"desc": "The client key file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "ruler-storage.s3.http.tls.key-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "server_name",
"required": false,
"desc": "The name of the server for verification.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "ruler-storage.s3.http.tls.server-name",
"fieldType": "string",
"fieldCategory": "advanced"
}
],
"fieldValue": null,
"fieldDefaultValue": null
}
],
"fieldValue": null,
Expand Down Expand Up @@ -14024,6 +14132,60 @@
"fieldFlag": "alertmanager-storage.s3.max-connections-per-host",
"fieldType": "int",
"fieldCategory": "advanced"
},
{
"kind": "block",
"name": "tls_config",
"required": false,
"desc": "",
"blockEntries": [
{
"kind": "field",
"name": "ca_file",
"required": false,
"desc": "The CA certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "alertmanager-storage.s3.http.tls.ca-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "cert_file",
"required": false,
"desc": "The client certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "alertmanager-storage.s3.http.tls.cert-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "key_file",
"required": false,
"desc": "The client key file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "alertmanager-storage.s3.http.tls.key-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "server_name",
"required": false,
"desc": "The name of the server for verification.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "alertmanager-storage.s3.http.tls.server-name",
"fieldType": "string",
"fieldCategory": "advanced"
}
],
"fieldValue": null,
"fieldDefaultValue": null
}
],
"fieldValue": null,
Expand Down Expand Up @@ -16342,6 +16504,60 @@
"fieldFlag": "common.storage.s3.max-connections-per-host",
"fieldType": "int",
"fieldCategory": "advanced"
},
{
"kind": "block",
"name": "tls_config",
"required": false,
"desc": "",
"blockEntries": [
{
"kind": "field",
"name": "ca_file",
"required": false,
"desc": "The CA certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "common.storage.s3.http.tls.ca-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "cert_file",
"required": false,
"desc": "The client certificate file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "common.storage.s3.http.tls.cert-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "key_file",
"required": false,
"desc": "The client key file path.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "common.storage.s3.http.tls.key-file",
"fieldType": "string",
"fieldCategory": "advanced"
},
{
"kind": "field",
"name": "server_name",
"required": false,
"desc": "The name of the server for verification.",
"fieldValue": null,
"fieldDefaultValue": "",
"fieldFlag": "common.storage.s3.http.tls.server-name",
"fieldType": "string",
"fieldCategory": "advanced"
}
],
"fieldValue": null,
"fieldDefaultValue": null
}
],
"fieldValue": null,
Expand Down
32 changes: 32 additions & 0 deletions cmd/mimir/help-all.txt.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,14 @@ Usage of ./cmd/mimir/mimir:
If the client connects to S3 via HTTPS and this option is enabled, the client will accept any certificate and hostname.
-alertmanager-storage.s3.http.response-header-timeout duration
The amount of time the client will wait for a servers response headers. (default 2m0s)
-alertmanager-storage.s3.http.tls.ca-file string
The CA certificate file path.
-alertmanager-storage.s3.http.tls.cert-file string
The client certificate file path.
-alertmanager-storage.s3.http.tls.key-file string
The client key file path.
-alertmanager-storage.s3.http.tls.server-name string
The name of the server for verification.
-alertmanager-storage.s3.insecure
If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.
-alertmanager-storage.s3.list-objects-version string
Expand Down Expand Up @@ -685,6 +693,14 @@ Usage of ./cmd/mimir/mimir:
If the client connects to S3 via HTTPS and this option is enabled, the client will accept any certificate and hostname.
-blocks-storage.s3.http.response-header-timeout duration
The amount of time the client will wait for a servers response headers. (default 2m0s)
-blocks-storage.s3.http.tls.ca-file string
The CA certificate file path.
-blocks-storage.s3.http.tls.cert-file string
The client certificate file path.
-blocks-storage.s3.http.tls.key-file string
The client key file path.
-blocks-storage.s3.http.tls.server-name string
The name of the server for verification.
-blocks-storage.s3.insecure
If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.
-blocks-storage.s3.list-objects-version string
Expand Down Expand Up @@ -855,6 +871,14 @@ Usage of ./cmd/mimir/mimir:
If the client connects to S3 via HTTPS and this option is enabled, the client will accept any certificate and hostname.
-common.storage.s3.http.response-header-timeout duration
The amount of time the client will wait for a servers response headers. (default 2m0s)
-common.storage.s3.http.tls.ca-file string
The CA certificate file path.
-common.storage.s3.http.tls.cert-file string
The client certificate file path.
-common.storage.s3.http.tls.key-file string
The client key file path.
-common.storage.s3.http.tls.server-name string
The name of the server for verification.
-common.storage.s3.insecure
If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.
-common.storage.s3.list-objects-version string
Expand Down Expand Up @@ -2277,6 +2301,14 @@ Usage of ./cmd/mimir/mimir:
If the client connects to S3 via HTTPS and this option is enabled, the client will accept any certificate and hostname.
-ruler-storage.s3.http.response-header-timeout duration
The amount of time the client will wait for a servers response headers. (default 2m0s)
-ruler-storage.s3.http.tls.ca-file string
The CA certificate file path.
-ruler-storage.s3.http.tls.cert-file string
The client certificate file path.
-ruler-storage.s3.http.tls.key-file string
The client key file path.
-ruler-storage.s3.http.tls.server-name string
The name of the server for verification.
-ruler-storage.s3.insecure
If enabled, use http:// for the S3 endpoint instead of https://. This could be useful in local dev/test environments while using an S3-compatible backend storage, like Minio.
-ruler-storage.s3.list-objects-version string
Expand Down
17 changes: 17 additions & 0 deletions docs/sources/mimir/configure/configuration-parameters/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4723,6 +4723,23 @@ http:
# (advanced) Maximum number of connections per host. 0 means no limit.
# CLI flag: -<prefix>.s3.max-connections-per-host
[max_connections_per_host: <int> | default = 0]

tls_config:
# (advanced) The CA certificate file path.
# CLI flag: -<prefix>.s3.http.tls.ca-file
[ca_file: <string> | default = ""]

# (advanced) The client certificate file path.
# CLI flag: -<prefix>.s3.http.tls.cert-file
[cert_file: <string> | default = ""]

# (advanced) The client key file path.
# CLI flag: -<prefix>.s3.http.tls.key-file
[key_file: <string> | default = ""]

# (advanced) The name of the server for verification.
# CLI flag: -<prefix>.s3.http.tls.server-name
[server_name: <string> | default = ""]
```

### gcs_storage_backend
Expand Down
7 changes: 7 additions & 0 deletions pkg/storage/bucket/s3/bucket_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/go-kit/log"
"github.com/prometheus/common/model"
"github.com/thanos-io/objstore"
"github.com/thanos-io/objstore/exthttp"
"github.com/thanos-io/objstore/providers/s3"
)

Expand Down Expand Up @@ -73,6 +74,12 @@ func newS3Config(cfg Config) (s3.Config, error) {
MaxIdleConnsPerHost: cfg.HTTP.MaxIdleConnsPerHost,
MaxConnsPerHost: cfg.HTTP.MaxConnsPerHost,
Transport: cfg.HTTP.Transport,
TLSConfig: exthttp.TLSConfig{
CAFile: cfg.HTTP.TLSConfig.CAFile,
CertFile: cfg.HTTP.TLSConfig.CertFile,
KeyFile: cfg.HTTP.TLSConfig.KeyFile,
ServerName: cfg.HTTP.TLSConfig.ServerName,
},
},
// Enforce signature version 2 if CLI flag is set
SignatureV2: cfg.SignatureVersion == SignatureVersionV2,
Expand Down
19 changes: 19 additions & 0 deletions pkg/storage/bucket/s3/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,16 @@ type HTTPConfig struct {

// Allow upstream callers to inject a round tripper
Transport http.RoundTripper `yaml:"-"`

TLSConfig TLSConfig `yaml:"tls_config"`
}

// TLSConfig configures the options for TLS connections.
Copy link
Contributor

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:

`yaml:",inline"`

Copy link
Contributor Author

@Itaykal Itaykal Apr 24, 2024 •
edited
Loading

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 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.

Copy link
Contributor

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 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

Copy link
Contributor Author

@Itaykal Itaykal Apr 24, 2024 •
edited
Loading

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 as tls_insecure_skip_verify which would cause some more inconsistencies later on.

Copy link
Contributor Author

@Itaykal Itaykal Apr 24, 2024 •
edited
Loading

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 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?

Copy link
Contributor

@56quarters 56quarters Apr 24, 2024 •
edited
Loading

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 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.

type TLSConfig struct {
CAFile string `yaml:"ca_file" category:"advanced"`
CertFile string `yaml:"cert_file" category:"advanced"`
KeyFile string `yaml:"key_file" category:"advanced"`
ServerName string `yaml:"server_name" category:"advanced"`
}

// RegisterFlagsWithPrefix registers the flags for s3 storage with the provided prefix
Expand All @@ -90,6 +100,15 @@ func (cfg *HTTPConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.IntVar(&cfg.MaxIdleConns, prefix+"s3.max-idle-connections", 100, "Maximum number of idle (keep-alive) connections across all hosts. 0 means no limit.")
f.IntVar(&cfg.MaxIdleConnsPerHost, prefix+"s3.max-idle-connections-per-host", 100, "Maximum number of idle (keep-alive) connections to keep per-host. If 0, a built-in default value is used.")
f.IntVar(&cfg.MaxConnsPerHost, prefix+"s3.max-connections-per-host", 0, "Maximum number of connections per host. 0 means no limit.")
cfg.TLSConfig.RegisterFlagsWithPrefix(prefix, f)
}

// RegisterFlagsWithPrefix registers the flags for s3 storage with the provided prefix.
func (cfg *TLSConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.StringVar(&cfg.CAFile, prefix+"s3.http.tls.ca-file", "", "The CA certificate file path.")
f.StringVar(&cfg.CertFile, prefix+"s3.http.tls.cert-file", "", "The client certificate file path.")
f.StringVar(&cfg.KeyFile, prefix+"s3.http.tls.key-file", "", "The client key file path.")
f.StringVar(&cfg.ServerName, prefix+"s3.http.tls.server-name", "", "The name of the server for verification.")
}

// Config holds the config options for an S3 backend
Expand Down
Loading