[observability] Default missing endpoint scheme to HTTPS

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

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
 }

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

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

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