From 0002114a0fb1ed2d8ba1119fcbb60cf21cd60264 Mon Sep 17 00:00:00 2001 From: Tyler Yahn Date: Tue, 7 Dec 2021 14:52:43 -0800 Subject: [PATCH] Fix per-signal endpoint parsing in otlptrace --- .../internal/otlpconfig/envconfig.go | 74 +++++++++++++------ .../internal/otlpconfig/options_test.go | 42 ++++------- .../otlp/otlptrace/otlptracehttp/client.go | 5 +- 3 files changed, 71 insertions(+), 50 deletions(-) diff --git a/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go b/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go index 352a88206da..027592fbd3b 100644 --- a/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go +++ b/exporters/otlp/otlptrace/internal/otlpconfig/envconfig.go @@ -20,7 +20,7 @@ import ( "io/ioutil" "net/url" "os" - "regexp" + "path" "strconv" "strings" "time" @@ -28,8 +28,6 @@ import ( "go.opentelemetry.io/otel" ) -var httpSchemeRegexp = regexp.MustCompile(`(?i)^(http://|https://)`) - func ApplyGRPCEnvConfigs(cfg *Config) { e := EnvOptionsReader{ GetEnv: os.Getenv, @@ -72,22 +70,55 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption { // Endpoint if v, ok := e.getEnvValue("ENDPOINT"); ok { - if isInsecureEndpoint(v) { - opts = append(opts, WithInsecure()) - } else { - opts = append(opts, WithSecure()) + u, err := url.Parse(v) + // Ignore invalid values. + if err == nil { + // This is used to set the scheme for OTLP/HTTP. + if insecureSchema(u.Scheme) { + opts = append(opts, WithInsecure()) + } else { + opts = append(opts, WithSecure()) + } + opts = append(opts, newSplitOption(func(cfg *Config) { + cfg.Traces.Endpoint = u.Host + // For OTLP/HTTP endpoint URLs without a per-signal + // configuration, the passed endpoint is used as a base URL + // and the signals are sent to these paths relative to that. + cfg.Traces.URLPath = path.Join(u.Path, DefaultTracesPath) + }, func(cfg *Config) { + // For OTLP/gRPC endpoints, this is the target to which the + // exporter is going to send telemetry. + cfg.Traces.Endpoint = path.Join(u.Host, u.Path) + })) } - - opts = append(opts, WithEndpoint(trimSchema(v))) } if v, ok := e.getEnvValue("TRACES_ENDPOINT"); ok { - if isInsecureEndpoint(v) { - opts = append(opts, WithInsecure()) - } else { - opts = append(opts, WithSecure()) + u, err := url.Parse(v) + // Ignore invalid values. + if err == nil { + // This is used to set the scheme for OTLP/HTTP. + if insecureSchema(u.Scheme) { + opts = append(opts, WithInsecure()) + } else { + opts = append(opts, WithSecure()) + } + opts = append(opts, newSplitOption(func(cfg *Config) { + cfg.Traces.Endpoint = u.Host + // For endpoint URLs for OTLP/HTTP per-signal variables, the + // URL MUST be used as-is without any modification. The only + // exception is that if an URL contains no path part, the root + // path / MUST be used. + path := u.Path + if path == "" { + path = "/" + } + cfg.Traces.URLPath = path + }, func(cfg *Config) { + // For OTLP/gRPC endpoints, this is the target to which the + // exporter is going to send telemetry. + cfg.Traces.Endpoint = path.Join(u.Host, u.Path) + })) } - - opts = append(opts, WithEndpoint(trimSchema(v))) } // Certificate File @@ -136,12 +167,13 @@ func (e *EnvOptionsReader) GetOptionsFromEnv() []GenericOption { return opts } -func isInsecureEndpoint(endpoint string) bool { - return strings.HasPrefix(strings.ToLower(endpoint), "http://") || strings.HasPrefix(strings.ToLower(endpoint), "unix://") -} - -func trimSchema(endpoint string) string { - return httpSchemeRegexp.ReplaceAllString(endpoint, "") +func insecureSchema(schema string) bool { + switch strings.ToLower(schema) { + case "http", "unix": + return true + default: + return false + } } // getEnvValue gets an OTLP environment variable value of the specified key using the GetEnv function. diff --git a/exporters/otlp/otlptrace/internal/otlpconfig/options_test.go b/exporters/otlp/otlptrace/internal/otlpconfig/options_test.go index ad25efdac04..63b5cdbb8c5 100644 --- a/exporters/otlp/otlptrace/internal/otlpconfig/options_test.go +++ b/exporters/otlp/otlptrace/internal/otlpconfig/options_test.go @@ -96,20 +96,30 @@ func TestConfigs(t *testing.T) { { name: "Test Environment Endpoint", env: map[string]string{ - "OTEL_EXPORTER_OTLP_ENDPOINT": "env_endpoint", + "OTEL_EXPORTER_OTLP_ENDPOINT": "https://env.endpoint/prefix", }, asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) { - assert.Equal(t, "env_endpoint", c.Traces.Endpoint) + assert.False(t, c.Traces.Insecure) + if grpcOption { + assert.Equal(t, "env.endpoint/prefix", c.Traces.Endpoint) + } else { + assert.Equal(t, "env.endpoint", c.Traces.Endpoint) + assert.Equal(t, "/prefix/v1/traces", c.Traces.URLPath) + } }, }, { name: "Test Environment Signal Specific Endpoint", env: map[string]string{ - "OTEL_EXPORTER_OTLP_ENDPOINT": "overrode_by_signal_specific", - "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "env_traces_endpoint", + "OTEL_EXPORTER_OTLP_ENDPOINT": "https://overrode.by.signal.specific/env/var", + "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env.traces.endpoint", }, asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) { - assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint) + assert.True(t, c.Traces.Insecure) + assert.Equal(t, "env.traces.endpoint", c.Traces.Endpoint) + if !grpcOption { + assert.Equal(t, "/", c.Traces.URLPath) + } }, }, { @@ -154,28 +164,6 @@ func TestConfigs(t *testing.T) { assert.Equal(t, false, c.Traces.Insecure) }, }, - { - name: "Test Environment Signal Specific Endpoint", - env: map[string]string{ - "OTEL_EXPORTER_OTLP_ENDPOINT": "http://overrode_by_signal_specific", - "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env_traces_endpoint", - }, - asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) { - assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint) - assert.Equal(t, true, c.Traces.Insecure) - }, - }, - { - name: "Test Environment Signal Specific Endpoint #2", - env: map[string]string{ - "OTEL_EXPORTER_OTLP_ENDPOINT": "http://overrode_by_signal_specific", - "OTEL_EXPORTER_OTLP_TRACES_ENDPOINT": "http://env_traces_endpoint", - }, - asserts: func(t *testing.T, c *otlpconfig.Config, grpcOption bool) { - assert.Equal(t, "env_traces_endpoint", c.Traces.Endpoint) - assert.Equal(t, true, c.Traces.Insecure) - }, - }, { name: "Test Environment Signal Specific Endpoint with uppercase scheme", env: map[string]string{ diff --git a/exporters/otlp/otlptrace/otlptracehttp/client.go b/exporters/otlp/otlptrace/otlptracehttp/client.go index 93a7e4a7e86..dbc6c5ba785 100644 --- a/exporters/otlp/otlptrace/otlptracehttp/client.go +++ b/exporters/otlp/otlptrace/otlptracehttp/client.go @@ -23,6 +23,7 @@ import ( "io/ioutil" "net" "net/http" + "net/url" "path" "strconv" "strings" @@ -201,8 +202,8 @@ func (d *client) UploadTraces(ctx context.Context, protoSpans []*tracepb.Resourc } func (d *client) newRequest(body []byte) (request, error) { - address := fmt.Sprintf("%s://%s%s", d.getScheme(), d.cfg.Endpoint, d.cfg.URLPath) - r, err := http.NewRequest(http.MethodPost, address, nil) + u := url.URL{Scheme: d.getScheme(), Host: d.cfg.Endpoint, Path: d.cfg.URLPath} + r, err := http.NewRequest(http.MethodPost, u.String(), nil) if err != nil { return request{Request: r}, err }