Configure Envoy tracing from OTEL_EXPORTER_OTLP_ENDPOINT (#637)

`--otlp-collector-address` defaults to
`os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT")`, unify all stacks to use
environment variables.

#563 

- [x] Tests pass
- [x] Appropriate changes to documentation are included in the PR

cmd/atenet/internal/router/cmd.go

@@ -16,6 +16,7 @@ package router
 
 import (
 	"fmt"
+	"os"
 	"time"
 
 	"github.com/spf13/cobra"
@@ -54,7 +55,11 @@ func NewRouterCmd() *cobra.Command {
 	cmd.Flags().DurationVar(&cfg.HealthInterval, "health-interval", 1*time.Second, "Interval for checking health of dependent services")
 	cmd.Flags().IntVar(&cfg.HttpsPort, "port-https", 8443, "TCP port for HTTPS workload traffic entering through the Envoy Router")
 	cmd.Flags().StringVar(&cfg.EnvoyCertPath, "envoy-cert-path", "", "Path to the Envoy certificate file.")
-	cmd.Flags().StringVar(&cfg.OtlpCollectorAddress, "otlp-collector-address", "", "host:port of the OTLP gRPC collector that Envoy reports tracing spans to (empty disables Envoy tracing)")
+	// Envoy learns the collector over xDS rather than from its own environment,
+	// so the router has to carry the address for it. Defaulting to
+	// OTEL_EXPORTER_OTLP_ENDPOINT — the same variable the router's own exporter
+	// reads — keeps one setting per pod, as in ate-apiserver and atelet.
+	cmd.Flags().StringVar(&cfg.OtlpCollectorAddress, "otlp-collector-address", os.Getenv("OTEL_EXPORTER_OTLP_ENDPOINT"), "OTLP gRPC collector that Envoy reports tracing spans to, as host:port or an http:// URL. Defaults to $OTEL_EXPORTER_OTLP_ENDPOINT. An address Envoy cannot use — an https endpoint, for one, since the tracer cluster is plaintext — disables Envoy-side tracing with a warning rather than failing startup. Pass empty to disable Envoy tracing while leaving the router's own spans enabled")
 	cmd.Flags().StringVar(&cfg.Auth.AteapiCAFile, "ateapi-ca-file", "", "PEM file with CAs trusted to verify the ateapi server cert. Required.")
 	cmd.Flags().StringVar(&cfg.Auth.AteapiClientCertPath, "ateapi-client-cert", "", "Credential bundle presented as the client certificate when dialing ateapi. Required unless --ateapi-use-token-auth is set, ignored otherwise.")
 	cmd.Flags().StringVar(&cfg.Auth.AteapiServerName, "ateapi-server-name", "", "SNI / hostname expected on the ateapi server cert. Optional.")

cmd/atenet/internal/router/config.go

@@ -51,8 +51,14 @@ type routerConfig struct {
 	EnvoyCertPath  string
 	LogLevel       string
 	MetricsAddr    string
-	// OtlpCollectorAddress is the host:port of the OTLP gRPC collector that
-	// Envoy reports tracing spans to. Empty disables Envoy-side tracing.
+	// OtlpCollectorAddress is the OTLP gRPC collector that Envoy reports
+	// tracing spans to, as host:port or an http:// URL. It defaults to
+	// OTEL_EXPORTER_OTLP_ENDPOINT — Envoy gets its whole configuration over
+	// xDS and never reads the router's environment, so the router has to relay
+	// the address on its behalf. Empty disables Envoy-side tracing; the
+	// router's own exporter still reads the env var directly. An address Envoy
+	// cannot use disables Envoy-side tracing rather than failing startup — see
+	// setOtlpCollector.
 	OtlpCollectorAddress string
 
 	Auth authConfig

cmd/atenet/internal/router/config_test.go

@@ -84,3 +84,61 @@ func TestRouterConfigExtProcMaxRequests(t *testing.T) {
 		})
 	}
 }
+
+func TestSetOtlpCollector(t *testing.T) {
+	// No collector address may keep the router from starting. The address
+	// defaults to OTEL_EXPORTER_OTLP_ENDPOINT, which also feeds the router's
+	// own exporter and where https is perfectly valid; the router is the xDS
+	// control plane for every Envoy in the mesh, so dropping Envoy's spans is
+	// always the cheaper failure. setOtlpCollector returns nothing precisely so
+	// this cannot regress into a startup error.
+	tests := []struct {
+		name     string
+		addr     string
+		wantHost string
+		wantPort uint32
+	}{
+		{
+			name:     "usable address is applied",
+			addr:     "http://collector.otel-system.svc:4317",
+			wantHost: "collector.otel-system.svc",
+			wantPort: 4317,
+		},
+		{name: "https disables Envoy tracing", addr: "https://collector.otel-system.svc:4317"},
+		{name: "unknown scheme disables Envoy tracing", addr: "grpc://collector.otel-system.svc:4317"},
+		{name: "hostless URL disables Envoy tracing", addr: "http://:4317"},
+		{name: "non-numeric port disables Envoy tracing", addr: "collector.otel-system.svc:grpc"},
+		{name: "empty disables Envoy tracing", addr: ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			x := NewXdsServer(0)
+			setOtlpCollector(t.Context(), x, tc.addr)
+
+			if x.otlpHost != tc.wantHost || x.otlpPort != tc.wantPort {
+				t.Errorf("collector = %q:%d, want %q:%d", x.otlpHost, x.otlpPort, tc.wantHost, tc.wantPort)
+			}
+			// The router comes up either way, so what actually differs is
+			// whether Envoy is told to trace at all.
+			if gotTracing := x.buildTracing() != nil; gotTracing != (tc.wantHost != "") {
+				t.Errorf("buildTracing() non-nil = %v, want %v", gotTracing, tc.wantHost != "")
+			}
+		})
+	}
+}
+
+func TestSetOtlpCollectorClearsPreviousCollector(t *testing.T) {
+	// A rejected address must not leave a stale collector configured: Envoy
+	// would keep shipping spans to an endpoint the operator has since
+	// repointed.
+	x := NewXdsServer(0)
+	setOtlpCollector(t.Context(), x, "http://collector.otel-system.svc:4317")
+	setOtlpCollector(t.Context(), x, "https://collector.otel-system.svc:4317")
+
+	if x.otlpHost != "" || x.otlpPort != 0 {
+		t.Errorf("collector after rejected address = %q:%d, want disabled", x.otlpHost, x.otlpPort)
+	}
+	if tr := x.buildTracing(); tr != nil {
+		t.Errorf("buildTracing() = %v, want nil after a rejected address", tr)
+	}
+}

cmd/atenet/internal/router/router.go

@@ -196,9 +196,7 @@ func (s *RouterServer) Run(ctx context.Context) error {
 
 	xdsSrv := NewXdsServer(s.cfg.XdsPort)
 	xdsSrv.SetConfig(s.cfg.HttpPort, s.cfg.ExtprocPort, s.cfg.ExtprocAddr)
-	if err := xdsSrv.SetOtlpCollector(s.cfg.OtlpCollectorAddress); err != nil {
-		return fmt.Errorf("configure OTLP collector: %w", err)
-	}
+	setOtlpCollector(ctx, xdsSrv, s.cfg.OtlpCollectorAddress)
 
 	xdsSrv.SetExtProcMaxRequests(s.cfg.extProcMaxRequests())
 	if parkCfg.enabled() {
@@ -287,3 +285,21 @@ func (s *RouterServer) Run(ctx context.Context) error {
 
 	return g.Wait()
 }
+
+// setOtlpCollector points Envoy's tracer at the configured collector, and
+// gives up on Envoy-side tracing if the address is one Envoy cannot use.
+//
+// It never fails the router. The address defaults to
+// OTEL_EXPORTER_OTLP_ENDPOINT, which the router's own exporter reads too and
+// which legitimately carries forms Envoy's plaintext tracer cluster cannot
+// reach — an https collector, most of all. Refusing to start would take the
+// xDS control plane for every Envoy in the mesh down over a tracing endpoint
+// that works fine for its other reader. Losing Envoy's spans is the smaller
+// failure, so take it and say so loudly.
+func setOtlpCollector(ctx context.Context, xdsSrv *XdsServer, addr string) {
+	if err := xdsSrv.SetOtlpCollector(addr); err != nil {
+		slog.WarnContext(ctx, "Envoy-side tracing disabled: the OTLP collector address is not one Envoy can use. The router's own spans are unaffected; set --otlp-collector-address to point Envoy at a plaintext collector",
+			slog.String("address", addr), slog.Any("err", err))
+		xdsSrv.DisableOtlpCollector()
+	}
+}

cmd/atenet/internal/router/xds.go

@@ -19,8 +19,10 @@ import (
 	"fmt"
 	"log/slog"
 	"net"
+	"net/url"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"sync"
 	"time"
 
@@ -174,29 +176,91 @@ func (x *XdsServer) SetTlsConfig(httpsPort int, certPath string) {
 	x.certPath = certPath
 }
 
+// otlpDefaultPort is the OTLP/gRPC default port, used when the collector
+// endpoint names no port.
+const otlpDefaultPort = "4317"
+
 // SetOtlpCollector enables Envoy-side tracing pointed at the OTLP gRPC
-// collector at host:port. addr empty disables tracing. port defaults to
-// 4317 if omitted.
+// collector. addr empty disables tracing. See normalizeOtlpCollector for the
+// accepted forms.
 func (x *XdsServer) SetOtlpCollector(addr string) error {
-	x.mu.Lock()
-	defer x.mu.Unlock()
 	if addr == "" {
-		x.otlpHost = ""
-		x.otlpPort = 0
+		x.DisableOtlpCollector()
 		return nil
 	}
-	host, portStr, err := net.SplitHostPort(addr)
+	// normalizeOtlpCollector reads nothing off x, so it runs unlocked.
+	host, port, err := normalizeOtlpCollector(addr)
 	if err != nil {
-		host = addr
-		portStr = "4317"
+		return err
+	}
+	x.mu.Lock()
+	defer x.mu.Unlock()
+	x.otlpHost = host
+	x.otlpPort = port
+	return nil
+}
+
+// DisableOtlpCollector turns Envoy-side tracing off. The router's own exporter
+// is independent of this and keeps reporting spans.
+func (x *XdsServer) DisableOtlpCollector() {
+	x.mu.Lock()
+	defer x.mu.Unlock()
+	x.otlpHost = ""
+	x.otlpPort = 0
+}
+
+// normalizeOtlpCollector resolves a collector endpoint to the bare host and
+// numeric port an xDS SocketAddress requires (buildOtlpCollectorCluster).
+//
+// It accepts both a bare "host:port" and the URL form carried by
+// OTEL_EXPORTER_OTLP_ENDPOINT, which is where --otlp-collector-address gets
+// its default: Envoy's tracer reaches the collector through a named cluster,
+// and a cluster endpoint has no room for a scheme or a path. Port defaults to
+// otlpDefaultPort when omitted.
+//
+// https is rejected rather than downgraded — the tracer cluster carries no
+// UpstreamTlsContext, so honoring it would mean shipping spans in plaintext to
+// an endpoint that asked for TLS. Rejection here only means "Envoy cannot use
+// this", not that the router should stop: the same endpoint is usable by the
+// router's own exporter, so the caller warns and runs without Envoy-side
+// tracing (see setOtlpCollector).
+func normalizeOtlpCollector(addr string) (string, uint32, error) {
+	hostport := addr
+	if strings.Contains(addr, "://") {
+		u, err := url.Parse(addr)
+		if err != nil {
+			return "", 0, fmt.Errorf("parse OTLP collector endpoint %q: %w", addr, err)
+		}
+		switch u.Scheme {
+		case "http":
+		case "https":
+			return "", 0, fmt.Errorf("OTLP collector endpoint %q uses https, which Envoy-side tracing does not support: the tracer cluster is plaintext h2c. Point --otlp-collector-address at an http:// endpoint, or pass it empty to disable Envoy-side tracing", addr)
+		default:
+			return "", 0, fmt.Errorf("OTLP collector endpoint %q has unsupported scheme %q, want http", addr, u.Scheme)
+		}
+		if p := strings.Trim(u.Path, "/"); p != "" {
+			// Envoy's OpenTelemetry tracer derives the gRPC method itself, so a
+			// path here cannot be honored. Warn instead of failing: the OTLP
+			// spec lets the signal-agnostic env var carry one.
+			slog.Warn("Ignoring path in OTLP collector endpoint; Envoy-side tracing addresses the collector by host and port only",
+				slog.String("endpoint", addr), slog.String("path", u.Path))
+		}
+		hostport = u.Host
+	}
+
+	host, portStr, err := net.SplitHostPort(hostport)
+	if err != nil {
+		host = strings.Trim(hostport, "[]")
+		portStr = otlpDefaultPort
+	}
+	if host == "" {
+		return "", 0, fmt.Errorf("OTLP collector endpoint %q names no host", addr)
 	}
 	port, err := strconv.ParseUint(portStr, 10, 32)
 	if err != nil {
-		return fmt.Errorf("parse OTLP collector port from %q: %w", addr, err)
+		return "", 0, fmt.Errorf("parse OTLP collector port from %q: %w", addr, err)
 	}
-	x.otlpHost = host
-	x.otlpPort = uint32(port)
-	return nil
+	return host, uint32(port), nil
 }
 
 func (x *XdsServer) UpdateSnapshot() error {

cmd/atenet/internal/router/xds_test.go

@@ -609,3 +609,89 @@ func TestXdsServer_ExtProcCircuitBreaker(t *testing.T) {
 		}
 	})
 }
+
+func TestXdsServer_SetOtlpCollector(t *testing.T) {
+	// --otlp-collector-address defaults to OTEL_EXPORTER_OTLP_ENDPOINT, so the
+	// URL forms that variable carries have to reduce to the bare host and port
+	// an xDS SocketAddress accepts.
+	tests := []struct {
+		name     string
+		addr     string
+		wantHost string
+		wantPort uint32
+	}{
+		{"HostPort", "collector.otel-system.svc:4317", "collector.otel-system.svc", 4317},
+		{"HostOnlyDefaultsPort", "collector.otel-system.svc", "collector.otel-system.svc", 4317},
+		{"HttpURL", "http://collector.otel-system.svc:4317", "collector.otel-system.svc", 4317},
+		{"HttpURLNoPort", "http://collector.otel-system.svc", "collector.otel-system.svc", 4317},
+		{"HttpURLTrailingSlash", "http://collector.otel-system.svc:4317/", "collector.otel-system.svc", 4317},
+		{"HttpURLWithPath", "http://collector.otel-system.svc:4317/v1/traces", "collector.otel-system.svc", 4317},
+		{"NonDefaultPort", "http://collector.otel-system.svc:14317", "collector.otel-system.svc", 14317},
+		{"IPv6", "[::1]:4317", "::1", 4317},
+		{"IPv6URL", "http://[::1]:4317", "::1", 4317},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			x := NewXdsServer(0)
+			if err := x.SetOtlpCollector(tc.addr); err != nil {
+				t.Fatalf("SetOtlpCollector(%q) failed: %v", tc.addr, err)
+			}
+			if x.otlpHost != tc.wantHost || x.otlpPort != tc.wantPort {
+				t.Errorf("SetOtlpCollector(%q) = %q:%d, want %q:%d", tc.addr, x.otlpHost, x.otlpPort, tc.wantHost, tc.wantPort)
+			}
+
+			// The address only matters insofar as it reaches Envoy: it must
+			// land in the tracer cluster's socket address, unaltered.
+			sock := x.buildOtlpCollectorCluster().GetLoadAssignment().GetEndpoints()[0].GetLbEndpoints()[0].GetEndpoint().GetAddress().GetSocketAddress()
+			if sock.GetAddress() != tc.wantHost || sock.GetPortValue() != tc.wantPort {
+				t.Errorf("tracer cluster endpoint = %q:%d, want %q:%d", sock.GetAddress(), sock.GetPortValue(), tc.wantHost, tc.wantPort)
+			}
+		})
+	}
+}
+
+func TestXdsServer_SetOtlpCollector_Rejects(t *testing.T) {
+	// An endpoint Envoy cannot use has to be reported rather than silently
+	// accepted: https downgraded to the plaintext tracer cluster would leak
+	// spans, and a garbage port would yield a cluster that never connects.
+	// Reporting it is as far as this layer goes — setOtlpCollector turns the
+	// error into a warning and runs without Envoy tracing, never a startup
+	// failure. See TestSetOtlpCollector.
+	for _, tc := range []struct {
+		name string
+		addr string
+	}{
+		{"Https", "https://collector.otel-system.svc:4317"},
+		{"UnknownScheme", "grpc://collector.otel-system.svc:4317"},
+		{"NoHost", "http://:4317"},
+		{"NonNumericPort", "collector.otel-system.svc:grpc"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := NewXdsServer(0).SetOtlpCollector(tc.addr); err == nil {
+				t.Errorf("SetOtlpCollector(%q) succeeded, want error", tc.addr)
+			}
+		})
+	}
+}
+
+func TestXdsServer_SetOtlpCollector_EmptyDisablesTracing(t *testing.T) {
+	// Empty has to stay a working off switch: the router's own spans keep
+	// flowing via OTEL_EXPORTER_OTLP_ENDPOINT, but Envoy emits none.
+	x := NewXdsServer(0)
+	if err := x.SetOtlpCollector(""); err != nil {
+		t.Fatalf("SetOtlpCollector(\"\") failed: %v", err)
+	}
+	if tr := x.buildTracing(); tr != nil {
+		t.Errorf("buildTracing() = %v, want nil when no collector is configured", tr)
+	}
+	if err := x.UpdateSnapshot(); err != nil {
+		t.Fatalf("UpdateSnapshot failed: %v", err)
+	}
+	res, err := x.snapshot.GetSnapshot(NodeID)
+	if err != nil {
+		t.Fatalf("GetSnapshot failed: %v", err)
+	}
+	if _, ok := res.GetResources(resourcev3.ClusterType)[OtlpClusterName]; ok {
+		t.Errorf("snapshot contains cluster %q, want it omitted when tracing is disabled", OtlpClusterName)
+	}
+}

manifests/ate-install/atenet-router.yaml

@@ -134,7 +134,6 @@ spec:
         - "--status-port=4040"
         - "--port-https=8443"
         - "--envoy-cert-path=/run/servicedns.podcert.ate.dev/credential-bundle.pem"
-        - "--otlp-collector-address=opentelemetry-collector.gke-managed-otel.svc.cluster.local:4317"
         # Client auth to ateapi (mtls): verify the serving cert against the
         # servicedns trust bundle and present the podidentity client cert.
         - "--ateapi-address=dns:///api.ate-system.svc:443"

manifests/ate-install/kind-token-client/kustomization.yaml

@@ -53,10 +53,10 @@ patches:
       namespace: ate-system
     patch: |-
       - op: test
-        path: /spec/template/spec/containers/0/args/13
+        path: /spec/template/spec/containers/0/args/12
         value: --ateapi-client-cert=/run/podidentity.podcert.ate.dev/credential-bundle.pem
       - op: remove
-        path: /spec/template/spec/containers/0/args/13
+        path: /spec/template/spec/containers/0/args/12
       - op: add
         path: /spec/template/spec/containers/0/args/-
         value: --ateapi-use-token-auth=true

manifests/ate-install/kind/kustomization.yaml

@@ -95,3 +95,17 @@ patches:
                 value: "10000"
               - name: OTEL_METRIC_EXPORT_TIMEOUT
                 value: "10000"
+  - patch: |-
+      apiVersion: apps/v1
+      kind: Deployment
+      metadata:
+        name: atenet-router
+        namespace: ate-system
+      spec:
+        template:
+          spec:
+            containers:
+            - name: atenet-router
+              env:
+              - name: OTEL_EXPORTER_OTLP_ENDPOINT
+                value: http://opentelemetry-collector.otel-system.svc:4317

manifests/ate-install/token-client/kustomization.yaml

@@ -59,10 +59,10 @@ patches:
       namespace: ate-system
     patch: |-
       - op: test
-        path: /spec/template/spec/containers/0/args/13
+        path: /spec/template/spec/containers/0/args/12
         value: --ateapi-client-cert=/run/podidentity.podcert.ate.dev/credential-bundle.pem
       - op: remove
-        path: /spec/template/spec/containers/0/args/13
+        path: /spec/template/spec/containers/0/args/12
       - op: add
         path: /spec/template/spec/containers/0/args/-
         value: --ateapi-use-token-auth=true