From 93ef79e1acaff068beba7661fbc4fa84c81ce8ad Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Mon, 30 Jun 2025 13:38:15 -0400 Subject: [PATCH 1/2] apply TrimSpace from config map values --- configmap/parser/parse.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/configmap/parser/parse.go b/configmap/parser/parse.go index a468727aa9..df7662af45 100644 --- a/configmap/parser/parse.go +++ b/configmap/parser/parse.go @@ -19,6 +19,7 @@ package parser import ( "fmt" "strconv" + "strings" "time" ) @@ -71,7 +72,7 @@ func parse[T parseable](s string) (T, error) { switch any(zero).(type) { case string: - val = s + val = strings.TrimSpace(s) case int16: val, err = strconv.ParseInt(s, 10, 16) val = int16(val.(int64)) From a28da6ea2533b126dd5d050d93e43ba97ab14eb7 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Mon, 30 Jun 2025 13:54:51 -0400 Subject: [PATCH 2/2] Convert opaque URLs to hiearchical ones. When testing prometheus endpoints I entered some.domain.com:9090/some/path And this took `some.domain.com` as the scheme. To avoid this landmine if we end up with an opaque URL we prepend the 'https' scheme to the endpoint --- observability/metrics/provider.go | 37 ++++++++++++++++++++----- observability/metrics/provider_test.go | 35 ++++++++++++++++++++++-- observability/tracing/provider.go | 38 +++++++++++++++++++++----- observability/tracing/provider_test.go | 33 ++++++++++++++++++++-- 4 files changed, 125 insertions(+), 18 deletions(-) diff --git a/observability/metrics/provider.go b/observability/metrics/provider.go index 990f4c2ac6..5a67201dd2 100644 --- a/observability/metrics/provider.go +++ b/observability/metrics/provider.go @@ -17,9 +17,11 @@ limitations under the License. package metrics import ( + "cmp" "context" "errors" "fmt" + "net/url" "os" "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc" @@ -87,7 +89,10 @@ func readerFor(ctx context.Context, cfg Config) (sdkmetric.Reader, shutdownFunc, func buildGRPC(ctx context.Context, cfg Config) (sdkmetric.Reader, shutdownFunc, error) { var grpcOpts []otlpmetricgrpc.Option - if opt := endpointFor(cfg, otlpmetricgrpc.WithEndpointURL); opt != nil { + opt, err := endpointFor(cfg, otlpmetricgrpc.WithEndpointURL) + if err != nil { + return nil, noopFunc, fmt.Errorf("unable to process metrics endpoint: %w", err) + } else if opt != nil { grpcOpts = append(grpcOpts, opt) } @@ -108,7 +113,10 @@ func buildGRPC(ctx context.Context, cfg Config) (sdkmetric.Reader, shutdownFunc, func buildHTTP(ctx context.Context, cfg Config) (sdkmetric.Reader, shutdownFunc, error) { var httpOpts []otlpmetrichttp.Option - if opt := endpointFor(cfg, otlpmetrichttp.WithEndpointURL); opt != nil { + opt, err := endpointFor(cfg, otlpmetrichttp.WithEndpointURL) + if err != nil { + return nil, noopFunc, fmt.Errorf("unable to process metrics endpoint: %w", err) + } else if opt != nil { httpOpts = append(httpOpts, opt) } @@ -138,12 +146,27 @@ func intervalFor(cfg Config) sdkmetric.PeriodicReaderOption { } // If the OTEL_EXPORTER_OTLP_ENDPOINT or OTEL_EXPORTER_OTLP_METRICS_ENDPOINT -func endpointFor[T any](cfg Config, opt func(string) T) T { +func endpointFor[T any](cfg Config, opt func(string) T) (T, error) { var epOption T - if (os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" && - os.Getenv("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT") == "") && cfg.Endpoint != "" { - epOption = opt(cfg.Endpoint) + override := cmp.Or( + os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT"), + os.Getenv("OTEL_EXPORTER_OTLP_METRICS_ENDPOINT"), + ) + if override != "" { + return epOption, nil + } + + ep := cfg.Endpoint + + u, err := url.Parse(cfg.Endpoint) + if err != nil { + return epOption, err } - return epOption + if u.Opaque != "" { + ep = "https://" + ep + } + + epOption = opt(ep) + return epOption, nil } diff --git a/observability/metrics/provider_test.go b/observability/metrics/provider_test.go index 29f4f9e0da..c87e1ea4ad 100644 --- a/observability/metrics/provider_test.go +++ b/observability/metrics/provider_test.go @@ -20,6 +20,9 @@ import ( "context" "testing" "time" + + "github.com/google/go-cmp/cmp" + "knative.dev/pkg/ptr" ) func TestNewMeterProviderProtocols(t *testing.T) { @@ -36,6 +39,13 @@ func TestNewMeterProviderProtocols(t *testing.T) { }, { name: "http", c: Config{Protocol: ProtocolHTTPProtobuf}, + }, { + name: "http - bad URL", + c: Config{ + Protocol: ProtocolHTTPProtobuf, + Endpoint: "://hello", + }, + wantErr: true, }, { name: "prometheus", c: Config{Protocol: ProtocolPrometheus}, @@ -119,7 +129,10 @@ func TestEndpointFor(t *testing.T) { t.Run("override "+envKey, func(t *testing.T) { t.Setenv(envKey, "https://override.example.com") - opt := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + opt, err := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + if err != nil { + t.Fatal("unexpected err", err) + } if opt != nil { t.Error("expected the option to not be present when 'OTEL_EXPORTER_OTLP_ENDPOINT' is set") @@ -132,11 +145,29 @@ func TestEndpointFor(t *testing.T) { result := "result" return &result } - opt := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + opt, err := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + if err != nil { + t.Fatal("unexpected err", err) + } if *opt != "result" { t.Error("expected option to work when no env vars are present") } }) + + t.Run("missing scheme - defaults to https", func(t *testing.T) { + optFunc := func(r string) *string { + return &r + } + got, err := endpointFor(Config{Endpoint: "otel.example.com:8080"}, optFunc) + if err != nil { + t.Fatal("unexpected err", err) + } + + want := ptr.String("https://otel.example.com:8080") + if diff := cmp.Diff(want, got); diff != "" { + t.Error("expected option to work when no env vars are present: (-want +got): ", diff) + } + }) } func TestIntervalFor(t *testing.T) { diff --git a/observability/tracing/provider.go b/observability/tracing/provider.go index 57c4b3a8bf..35cee20f0a 100644 --- a/observability/tracing/provider.go +++ b/observability/tracing/provider.go @@ -17,8 +17,10 @@ limitations under the License. package tracing import ( + "cmp" "context" "fmt" + "net/url" "os" "strconv" @@ -94,7 +96,10 @@ func exporterFor(ctx context.Context, cfg Config) (sdktrace.SpanExporter, error) func buildGRPC(ctx context.Context, cfg Config) (sdktrace.SpanExporter, error) { var grpcOpts []otlptracegrpc.Option - if opt := endpointFor(cfg, otlptracegrpc.WithEndpointURL); opt != nil { + opt, err := endpointFor(cfg, otlptracegrpc.WithEndpointURL) + if err != nil { + return nil, fmt.Errorf("unable to process traces endpoint: %w", err) + } else if opt != nil { grpcOpts = append(grpcOpts, opt) } @@ -108,7 +113,10 @@ func buildGRPC(ctx context.Context, cfg Config) (sdktrace.SpanExporter, error) { func buildHTTP(ctx context.Context, cfg Config) (sdktrace.SpanExporter, error) { var httpOpts []otlptracehttp.Option - if opt := endpointFor(cfg, otlptracehttp.WithEndpointURL); opt != nil { + opt, err := endpointFor(cfg, otlptracehttp.WithEndpointURL) + if err != nil { + return nil, fmt.Errorf("unable to process traces endpoint: %w", err) + } else if opt != nil { httpOpts = append(httpOpts, opt) } @@ -122,14 +130,30 @@ func buildHTTP(ctx context.Context, cfg Config) (sdktrace.SpanExporter, error) { // If the OTEL_EXPORTER_OTLP_ENDPOINT or OTEL_EXPORTER_OTLP_TRACES_ENDPOINT is // set then we will prefer that over what's in the Config -func endpointFor[T any](cfg Config, opt func(string) T) T { +func endpointFor[T any](cfg Config, opt func(string) T) (T, error) { var epOption T - if (os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT") == "" && - os.Getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT") == "") && cfg.Endpoint != "" { - epOption = opt(cfg.Endpoint) + override := cmp.Or( + os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT"), + os.Getenv("OTEL_EXPORTER_OTLP_TRACES_ENDPOINT"), + ) + + if override != "" { + return epOption, nil + } + + ep := cfg.Endpoint + + u, err := url.Parse(cfg.Endpoint) + if err != nil { + return epOption, err } - return epOption + if u.Opaque != "" { + ep = "https://" + ep + } + + epOption = opt(ep) + return epOption, nil } func sampleFor(cfg Config) (sdktrace.Sampler, error) { diff --git a/observability/tracing/provider_test.go b/observability/tracing/provider_test.go index 7247fbf9bd..fd6a92f12a 100644 --- a/observability/tracing/provider_test.go +++ b/observability/tracing/provider_test.go @@ -22,6 +22,7 @@ import ( "testing" "github.com/google/go-cmp/cmp" + "knative.dev/pkg/ptr" ) func TestNewTrackerProviderProtocols(t *testing.T) { @@ -42,6 +43,13 @@ func TestNewTrackerProviderProtocols(t *testing.T) { name: "bad protocol", c: Config{Protocol: "bad"}, wantErr: true, + }, { + name: "http - bad URL", + c: Config{ + Protocol: ProtocolHTTPProtobuf, + Endpoint: "://hello", + }, + wantErr: true, }, { name: "http with path in endpoint", c: Config{ @@ -78,7 +86,10 @@ func TestEndpointFor(t *testing.T) { t.Run("override "+envKey, func(t *testing.T) { t.Setenv(envKey, "https://override.example.com") - opt := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + opt, err := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + if err != nil { + t.Fatal("unexpected error", err) + } if opt != nil { t.Error("expected the option to not be present when 'OTEL_EXPORTER_OTLP_ENDPOINT' is set") @@ -91,11 +102,29 @@ func TestEndpointFor(t *testing.T) { result := "result" return &result } - opt := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + opt, err := endpointFor(Config{Endpoint: "https://otel.example.com"}, optFunc) + if err != nil { + t.Fatal("unexpected error", err) + } if *opt != "result" { t.Error("expected option to work when no env vars are present") } }) + + t.Run("missing scheme - defaults to https", func(t *testing.T) { + optFunc := func(r string) *string { + return &r + } + got, err := endpointFor(Config{Endpoint: "otel.example.com:8080"}, optFunc) + if err != nil { + t.Fatal("unexpected err", err) + } + + want := ptr.String("https://otel.example.com:8080") + if diff := cmp.Diff(want, got); diff != "" { + t.Error("expected option to work when no env vars are present: (-want +got): ", diff) + } + }) } func TestTracerProviderShutdown(t *testing.T) {