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.

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

Use TLS config from dskit #2407

Merged
merged 7 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@
* [ENHANCEMENT] Add synchronous read mode to vParquet and vParquet2 optionally enabled by env vars [#2165](https://github.com/grafana/tempo/pull/2165) (@mdisibio)
* [ENHANCEMENT] Add option to override metrics-generator ring port [#2399](https://github.com/grafana/tempo/pull/2399) (@mdisibio)
* [BUGFIX] tempodb integer divide by zero error [#2167](https://github.com/grafana/tempo/issues/2167) (@kroksys)
* [CHANGE] **Breaking Change** Rename s3.insecure_skip_verify [#???](https://github.com/grafana/tempo/pull/???) (@zalegrala)
```yaml
storage:
trace:
s3:
insecure_skip_verify: true // renamed to tls_insecure_skip_verify

```

## v2.1.1 / 2023-04-28
* [BUGFIX] Fix issue where Tempo sometimes flips booleans from false->true at storage time. [#2400](https://github.com/grafana/tempo/issues/2400) (@joe-elliott)
Expand Down
28 changes: 26 additions & 2 deletions docs/sources/tempo/configuration/_index.md
Original file line number Diff line number Diff line change
Expand Up @@ -683,8 +683,32 @@ storage:
[insecure: <bool>]

# optional.
# Set to true to disable verification of an TLS endpoint. The default value is false.
[insecure_skip_verify: <bool>]
# Path to the client certificate file.
[tls_cert_path: <string>]

# optional.
# Path to the private client key file.
[tls_key_path: <string>]

# optional.
# Path to the CA certificate file.
[tls_ca_path: <string>]

# optional.
# Path to the CA certificate file.
[tls_server_name: <string>]

# optional.
# Set to true to disable verification of a TLS endpoint. The default value is false.
[tls_insecure_skip_verify: <bool>]

# optional.
# Override the default cipher suite list, separated by commas.
[tls_cipher_suites: <string>]

# optional.
# Override the default minimum TLS version. The default value is VersionTLS12. Allowed values: VersionTLS10, VersionTLS11, VersionTLS12, VersionTLS13
[tls_min_version: <string>]

# optional.
# enable to use path-style requests.
Expand Down
2 changes: 1 addition & 1 deletion docs/sources/tempo/configuration/manifest.md
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ storage:
secret_key: ""
session_token: ""
insecure: false
insecure_skip_verify: false
tls_insecure_skip_verify: false
part_size: 0
hedge_requests_at: 0s
hedge_requests_up_to: 2
Expand Down
2 changes: 1 addition & 1 deletion modules/storage/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"time"

"github.com/grafana/tempo/pkg/cache"

"github.com/grafana/tempo/pkg/util"
"github.com/grafana/tempo/tempodb"
"github.com/grafana/tempo/tempodb/backend"
Expand Down Expand Up @@ -78,6 +77,7 @@ func (cfg *Config) RegisterFlagsAndApplyDefaults(prefix string, f *flag.FlagSet)
f.StringVar(&cfg.Trace.S3.Prefix, util.PrefixConfig(prefix, "trace.s3.prefix"), "", "s3 root directory to store blocks in.")
f.StringVar(&cfg.Trace.S3.Endpoint, util.PrefixConfig(prefix, "trace.s3.endpoint"), "", "s3 endpoint to push blocks to.")
f.StringVar(&cfg.Trace.S3.AccessKey, util.PrefixConfig(prefix, "trace.s3.access_key"), "", "s3 access key.")
f.StringVar(&cfg.Trace.S3.MinVersion, util.PrefixConfig(prefix, "trace.s3.tls_min_version"), "VersionTLS12", "minimum version of TLS to use when connecting to s3.")
f.Var(&cfg.Trace.S3.SecretKey, util.PrefixConfig(prefix, "trace.s3.secret_key"), "s3 secret key.")
f.Var(&cfg.Trace.S3.SessionToken, util.PrefixConfig(prefix, "trace.s3.session_token"), "s3 session token.")
cfg.Trace.S3.HedgeRequestsUpTo = 2
Expand Down
26 changes: 14 additions & 12 deletions tempodb/backend/s3/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,24 @@ package s3
import (
"time"

"github.com/grafana/dskit/crypto/tls"
"github.com/grafana/dskit/flagext"
)

type Config struct {
Bucket string `yaml:"bucket"`
Prefix string `yaml:"prefix"`
Endpoint string `yaml:"endpoint"`
Region string `yaml:"region"`
AccessKey string `yaml:"access_key"`
SecretKey flagext.Secret `yaml:"secret_key"`
SessionToken flagext.Secret `yaml:"session_token"`
Insecure bool `yaml:"insecure"`
InsecureSkipVerify bool `yaml:"insecure_skip_verify"`
PartSize uint64 `yaml:"part_size"`
HedgeRequestsAt time.Duration `yaml:"hedge_requests_at"`
HedgeRequestsUpTo int `yaml:"hedge_requests_up_to"`
tls.ClientConfig `yaml:",inline"`

Bucket string `yaml:"bucket"`
Prefix string `yaml:"prefix"`
Endpoint string `yaml:"endpoint"`
Region string `yaml:"region"`
AccessKey string `yaml:"access_key"`
SecretKey flagext.Secret `yaml:"secret_key"`
SessionToken flagext.Secret `yaml:"session_token"`
Insecure bool `yaml:"insecure"`
PartSize uint64 `yaml:"part_size"`
HedgeRequestsAt time.Duration `yaml:"hedge_requests_at"`
HedgeRequestsUpTo int `yaml:"hedge_requests_up_to"`
// SignatureV2 configures the object storage to use V2 signing instead of V4
SignatureV2 bool `yaml:"signature_v2"`
ForcePathStyle bool `yaml:"forcepathstyle"`
Expand Down
13 changes: 11 additions & 2 deletions tempodb/backend/s3/s3.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ func New(cfg *Config) (backend.RawReader, backend.RawWriter, backend.Compactor,
}

func internalNew(cfg *Config, confirm bool) (backend.RawReader, backend.RawWriter, backend.Compactor, error) {
if cfg == nil {
return nil, nil, nil, fmt.Errorf("config is nil")
}

l := log.Logger

core, err := createCore(cfg, false)
Expand Down Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

if err != nil {
return nil, errors.Wrap(err, "failed to create TLS config")
}

if tlsConfig != nil {
customTransport.TLSClientConfig = tlsConfig
}

// add instrumentation
Expand Down
8 changes: 8 additions & 0 deletions tempodb/backend/s3/s3_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,14 @@ func TestHedge(t *testing.T) {
}
}

func TestNilConfig(t *testing.T) {
_, _, _, err := New(nil)
require.Error(t, err)

_, _, _, err = NewNoConfirm(nil)
require.Error(t, err)
}

func fakeServer(t *testing.T, returnIn time.Duration, counter *int32) *httptest.Server {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
time.Sleep(returnIn)
Expand Down