Followup on OpenTelemetry migration (prometheus#10203)

* Followup on OpenTelemetry migration - tracing_config: Change with_insecure to insecure, default to false. - tracing_config: Call SetDirectory to make TLS certificates relative to the Prometheus configuration - documentation: Change bool to boolean in the configuration - documentation: document type float - tracing: Always restart the tracing manager when TLS config is set to reload certificates - tracing: Always set TLS config, which could be used e.g. in case of potential redirects. Signed-off-by: Julien Pivotto <[email protected]>\\
SimonDreher · Jan 29, 2022 · 8cb733d · 8cb733d
1 parent 6d76f09
commit 8cb733d
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 54 deletions.
diff --git a/config/config.go b/config/config.go
@@ -234,6 +234,7 @@ type Config struct {
 func (c *Config) SetDirectory(dir string) {
 	c.GlobalConfig.SetDirectory(dir)
 	c.AlertingConfig.SetDirectory(dir)
+	c.TracingConfig.SetDirectory(dir)
 	for i, file := range c.RuleFiles {
 		c.RuleFiles[i] = config.JoinDir(dir, file)
 	}
@@ -532,13 +533,21 @@ type TracingConfig struct {
 	ClientType       TracingClientType `yaml:"client_type,omitempty"`
 	Endpoint         string            `yaml:"endpoint,omitempty"`
 	SamplingFraction float64           `yaml:"sampling_fraction,omitempty"`
-	WithSecure       bool              `yaml:"with_secure,omitempty"`
+	Insecure         bool              `yaml:"insecure,omitempty"`
 	TLSConfig        config.TLSConfig  `yaml:"tls_config,omitempty"`
 }
 
+// SetDirectory joins any relative file paths with dir.
+func (t *TracingConfig) SetDirectory(dir string) {
+	t.TLSConfig.SetDirectory(dir)
+}
+
 // UnmarshalYAML implements the yaml.Unmarshaler interface.
 func (t *TracingConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
-	*t = TracingConfig{}
+	*t = TracingConfig{
+		ClientType: TracingClientGRPC,
+		Insecure:   true,
+	}
 	type plain TracingConfig
 	if err := unmarshal((*plain)(t)); err != nil {
 		return err
@@ -548,11 +557,6 @@ func (t *TracingConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 		return errors.New("tracing endpoint must be set")
 	}
 
-	// Fill in gRPC client as default if none is set.
-	if t.ClientType == "" {
-		t.ClientType = TracingClientGRPC
-	}
-
 	return nil
 }
 

diff --git a/config/config_test.go b/config/config_test.go
@@ -988,6 +988,7 @@ var expectedConf = &Config{
 	TracingConfig: TracingConfig{
 		Endpoint:   "localhost:4317",
 		ClientType: TracingClientGRPC,
+		Insecure:   true,
 	},
 }
 

diff --git a/docs/configuration/configuration.md b/docs/configuration/configuration.md
@@ -34,6 +34,7 @@ Generic placeholders are defined as follows:
 * `<boolean>`: a boolean that can take the values `true` or `false`
 * `<duration>`: a duration matching the regular expression `((([0-9]+)y)?(([0-9]+)w)?(([0-9]+)d)?(([0-9]+)h)?(([0-9]+)m)?(([0-9]+)s)?(([0-9]+)ms)?|0)`, e.g. `1d`, `1h30m`, `5m`, `10s`
 * `<filename>`: a valid path in the current working directory
+* `<float>`: a floating-point number
 * `<host>`: a valid string consisting of a hostname or IP followed by an optional port number
 * `<int>`: an integer value
 * `<labelname>`: a string matching the regular expression `[a-zA-Z_][a-zA-Z0-9_]*`
@@ -195,7 +196,7 @@ oauth2:
   [ <oauth2> ]
 
 # Configure whether scrape requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # Configures the scrape request's TLS settings.
 tls_config:
@@ -472,7 +473,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -568,7 +569,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -643,7 +644,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -740,7 +741,7 @@ oauth2:
   [ <oauth2> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 ```
 
@@ -903,7 +904,7 @@ oauth2:
   [ <oauth2> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 ```
 
@@ -1206,7 +1207,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 ```
 
 See [this example Prometheus configuration file](/documentation/examples/prometheus-puppetdb.yml)
@@ -1410,7 +1411,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -1491,7 +1492,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -1690,15 +1691,15 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
   [ <tls_config> ]
 
 # Optional namespace discovery. If omitted, all namespaces are used.
 namespaces:
-  own_namespace: <bool>
+  own_namespace: <boolean>
   names:
     [ - <string> ]
 
@@ -1788,7 +1789,7 @@ oauth2:
   [ <oauth2> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 ```
 
 The [relabeling phase](#relabel_config) is the preferred and more powerful way
@@ -1904,7 +1905,7 @@ oauth2:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -1987,7 +1988,7 @@ oauth2:
   [ <oauth2> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration for connecting to marathon servers
 tls_config:
@@ -2197,7 +2198,7 @@ tls_config:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # Refresh interval to re-read the app instance list.
 [ refresh_interval: <duration> | default = 30s ]
@@ -2301,7 +2302,7 @@ tags_filter:
 [ refresh_interval: <duration> | default = 60s ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # Optional proxy URL.
 [ proxy_url: <string> ]
@@ -2374,7 +2375,7 @@ oauth2:
   [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-  [ follow_redirects: <bool> | default = true ]
+  [ follow_redirects: <boolean> | default = true ]
 
 # TLS configuration.
 tls_config:
@@ -2556,7 +2557,7 @@ tls_config:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # List of Azure service discovery configurations.
 azure_sd_configs:
@@ -2658,18 +2659,24 @@ static_configs:
 relabel_configs:
   [ - <relabel_config> ... ]
 ```
+
 ### `<tracing_config>`
+
 `tracing_config` configures exporting traces from Prometheus to a tracing backend via the OTLP protocol. Tracing is currently an **experimental** feature and could change in the future.
 
 ```yaml
 # Client used to export the traces. Options are 'http' or 'grpc'.
 [ client_type: <string> | default = grpc ]
+
 # Endpoint to send the traces to.
 [ endpoint: <string> ]
+
 # Sets the probability a given trace will be sampled. Must be a float from 0 through 1.
 [ sampling_fraction: <float> | default = 0 ]
-# If enabled, the client will use a secure connection.
-[ with_secure: <bool> | default = false ]
+
+# If disabled, the client will use a secure connection.
+[ insecure: <boolean> | default = true ]
+
 # TLS configuration.
 tls_config:
   [ <tls_config> ]
@@ -2759,7 +2766,7 @@ tls_config:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 
 # Configures the queue used to write to remote storage.
 queue_config:
@@ -2860,7 +2867,7 @@ tls_config:
 [ proxy_url: <string> ]
 
 # Configure whether HTTP requests follow HTTP 3xx redirects.
-[ follow_redirects: <bool> | default = true ]
+[ follow_redirects: <boolean> | default = true ]
 ```
 
 There is a list of

diff --git a/tracing/testdata/ca.cer b/tracing/testdata/ca.cer
@@ -0,0 +1,22 @@
+-----BEGIN CERTIFICATE-----
+MIIDkTCCAnmgAwIBAgIJAJNsnimNN3tmMA0GCSqGSIb3DQEBCwUAMF8xCzAJBgNV
+BAYTAlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQg
+Q29tcGFueSBMdGQxGzAZBgNVBAMMElByb21ldGhldXMgVGVzdCBDQTAeFw0xNTA4
+MDQxNDA5MjFaFw0yNTA4MDExNDA5MjFaMF8xCzAJBgNVBAYTAlhYMRUwEwYDVQQH
+DAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29tcGFueSBMdGQxGzAZ
+BgNVBAMMElByb21ldGhldXMgVGVzdCBDQTCCASIwDQYJKoZIhvcNAQEBBQADggEP
+ADCCAQoCggEBAOlSBU3yWpUELbhzizznR0hnAL7dbEHzfEtEc6N3PoSvMNcqrUVq
+t4kjBRWzqkZ5uJVkzBPERKEBoOI9pWcrqtMTBkMzHJY2Ep7GHTab10e9KC2IFQT6
+FKP/jCYixaIVx3azEfajRJooD8r79FGoagWUfHdHyCFWJb/iLt8z8+S91kelSRMS
+yB9M1ypWomzBz1UFXZp1oiNO5o7/dgXW4MgLUfC2obJ9j5xqpc6GkhWMW4ZFwEr/
+VLjuzxG9B8tLfQuhnXKGn1W8+WzZVWCWMD/sLfZfmjKaWlwcXzL51g8E+IEIBJqV
+w51aMI6lDkcvAM7gLq1auLZMVXyKWSKw7XMCAwEAAaNQME4wHQYDVR0OBBYEFMz1
+BZnlqxJp2HiJSjHK8IsLrWYbMB8GA1UdIwQYMBaAFMz1BZnlqxJp2HiJSjHK8IsL
+rWYbMAwGA1UdEwQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBAI2iA3w3TK5J15Pu
+e4fPFB4jxQqsbUwuyXbCCv/jKLeFNCD4BjM181WZEYjPMumeTBVzU3aF45LWQIG1
+0DJcrCL4mjMz9qgAoGqA7aDDXiJGbukMgYYsn7vrnVmrZH8T3E8ySlltr7+W578k
+pJ5FxnbCroQwn0zLyVB3sFbS8E3vpBr3L8oy8PwPHhIScexcNVc3V6/m4vTZsXTH
+U+vUm1XhDgpDcFMTg2QQiJbfpOYUkwIgnRDAT7t282t2KQWtnlqc3zwPQ1F/6Cpx
+j19JeNsaF1DArkD7YlyKj/GhZLtHwFHG5cxznH0mLDJTW7bQvqqh2iQTeXmBk1lU
+mM5lH/s=
+-----END CERTIFICATE-----
diff --git a/tracing/tracing.go b/tracing/tracing.go
@@ -68,8 +68,11 @@ func (m *Manager) Run() {
 // ApplyConfig takes care of refreshing the tracing configuration by shutting down
 // the current tracer provider (if any is registered) and installing a new one.
 func (m *Manager) ApplyConfig(cfg *config.Config) error {
-	// Update only if a config change is detected.
-	if m.config == cfg.TracingConfig {
+	// Update only if a config change is detected. If TLS configuration is
+	// set, we have to restart the manager to make sure that new TLS
+	// certificates are picked up.
+	var blankTLSConfig config_util.TLSConfig
+	if m.config == cfg.TracingConfig && m.config.TLSConfig == blankTLSConfig {
 		return nil
 	}
 
@@ -177,27 +180,29 @@ func getClient(tracingCfg config.TracingConfig) (otlptrace.Client, error) {
 	switch tracingCfg.ClientType {
 	case config.TracingClientGRPC:
 		opts := []otlptracegrpc.Option{otlptracegrpc.WithEndpoint(tracingCfg.Endpoint)}
-		if !tracingCfg.WithSecure {
+		if tracingCfg.Insecure {
 			opts = append(opts, otlptracegrpc.WithInsecure())
-		} else {
-			tlsConf, err := config_util.NewTLSConfig(&tracingCfg.TLSConfig)
-			if err != nil {
-				return nil, err
-			}
-			opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConf)))
 		}
+
+		tlsConf, err := config_util.NewTLSConfig(&tracingCfg.TLSConfig)
+		if err != nil {
+			return nil, err
+		}
+		opts = append(opts, otlptracegrpc.WithTLSCredentials(credentials.NewTLS(tlsConf)))
+
 		client = otlptracegrpc.NewClient(opts...)
 	case config.TracingClientHTTP:
 		opts := []otlptracehttp.Option{otlptracehttp.WithEndpoint(tracingCfg.Endpoint)}
-		if !tracingCfg.WithSecure {
+		if tracingCfg.Insecure {
 			opts = append(opts, otlptracehttp.WithInsecure())
-		} else {
-			tlsConf, err := config_util.NewTLSConfig(&tracingCfg.TLSConfig)
-			if err != nil {
-				return nil, err
-			}
-			opts = append(opts, otlptracehttp.WithTLSClientConfig(tlsConf))
 		}
+
+		tlsConf, err := config_util.NewTLSConfig(&tracingCfg.TLSConfig)
+		if err != nil {
+			return nil, err
+		}
+		opts = append(opts, otlptracehttp.WithTLSClientConfig(tlsConf))
+
 		client = otlptracehttp.NewClient(opts...)
 	}
 

diff --git a/tracing/tracing_test.go b/tracing/tracing_test.go
@@ -46,11 +46,6 @@ func TestReinstallingTracerProvider(t *testing.T) {
 		TracingConfig: config.TracingConfig{
 			Endpoint:   "localhost:1234",
 			ClientType: config.TracingClientGRPC,
-			TLSConfig: config_util.TLSConfig{
-				CAFile:     "ca-file.pem",
-				CertFile:   "cert.pem",
-				ServerName: "test-server",
-			},
 		},
 	}
 
@@ -65,15 +60,30 @@ func TestReinstallingTracerProvider(t *testing.T) {
 		TracingConfig: config.TracingConfig{
 			Endpoint:   "localhost:1234",
 			ClientType: config.TracingClientHTTP,
+		},
+	}
+
+	require.NoError(t, m.ApplyConfig(&cfg2))
+	require.NotEqual(t, tpFirstConfig, otel.GetTracerProvider())
+}
+
+func TestReinstallingTracerProviderWithTLS(t *testing.T) {
+	m := NewManager(log.NewNopLogger())
+	cfg := config.Config{
+		TracingConfig: config.TracingConfig{
+			Endpoint:   "localhost:1234",
+			ClientType: config.TracingClientGRPC,
 			TLSConfig: config_util.TLSConfig{
-				CAFile:     "ca-file.pem",
-				CertFile:   "cert.pem",
-				ServerName: "test-server",
+				CAFile: "testdata/ca.cer",
 			},
 		},
 	}
 
-	require.NoError(t, m.ApplyConfig(&cfg2))
+	require.NoError(t, m.ApplyConfig(&cfg))
+	tpFirstConfig := otel.GetTracerProvider()
+
+	// Trying to apply the same config with TLS should reinstall provider.
+	require.NoError(t, m.ApplyConfig(&cfg))
 	require.NotEqual(t, tpFirstConfig, otel.GetTracerProvider())
 }