Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions commons/opentelemetry/otel.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
}

// NewTelemetry builds telemetry providers and exporters from configuration.
func NewTelemetry(cfg TelemetryConfig) (*Telemetry, error) {

Check failure on line 88 in commons/opentelemetry/otel.go

View workflow job for this annotation

GitHub Actions / Run GoLangCI-Lint to SDK

cyclomatic complexity 19 of func `NewTelemetry` is high (> 16) (gocyclo)
if cfg.Logger == nil {
return nil, ErrNilTelemetryLogger
}
Expand All @@ -98,6 +98,22 @@
cfg.Redactor = NewDefaultRedactor()
}

// Normalize endpoint: strip URL scheme and infer security mode.
// gRPC WithEndpoint() expects host:port, not a full URL.
// Consumers commonly pass OTEL_EXPORTER_OTLP_ENDPOINT as "http://host:4317".
if ep := strings.TrimSpace(cfg.CollectorExporterEndpoint); ep != "" {
switch {
case strings.HasPrefix(ep, "http://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "http://")
cfg.InsecureExporter = true
case strings.HasPrefix(ep, "https://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "https://")
default:
// No scheme — assume insecure (common in k8s internal comms).
cfg.InsecureExporter = true
}
Comment on lines +104 to +114
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Prevent silent TLS downgrade and persist normalized endpoint.

Line [112]-[113] forces InsecureExporter = true for all schemeless endpoints, which can unintentionally downgrade secure setups. Also, Line [104] trims into ep, but the no-scheme branch never writes the trimmed value back to cfg.CollectorExporterEndpoint.

Proposed fix
 if ep := strings.TrimSpace(cfg.CollectorExporterEndpoint); ep != "" {
+    cfg.CollectorExporterEndpoint = ep
 	switch {
 	case strings.HasPrefix(ep, "http://"):
 		cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "http://")
 		cfg.InsecureExporter = true
 	case strings.HasPrefix(ep, "https://"):
 		cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "https://")
 	default:
-		// No scheme — assume insecure (common in k8s internal comms).
-		cfg.InsecureExporter = true
+		// No scheme: keep caller-provided InsecureExporter to avoid silent transport downgrade.
 	}
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if ep := strings.TrimSpace(cfg.CollectorExporterEndpoint); ep != "" {
switch {
case strings.HasPrefix(ep, "http://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "http://")
cfg.InsecureExporter = true
case strings.HasPrefix(ep, "https://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "https://")
default:
// No scheme — assume insecure (common in k8s internal comms).
cfg.InsecureExporter = true
}
if ep := strings.TrimSpace(cfg.CollectorExporterEndpoint); ep != "" {
cfg.CollectorExporterEndpoint = ep
switch {
case strings.HasPrefix(ep, "http://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "http://")
cfg.InsecureExporter = true
case strings.HasPrefix(ep, "https://"):
cfg.CollectorExporterEndpoint = strings.TrimPrefix(ep, "https://")
default:
// No scheme: keep caller-provided InsecureExporter to avoid silent transport downgrade.
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@commons/opentelemetry/otel.go` around lines 104 - 114, The endpoint
normalization currently trims into a local ep but fails to persist the trimmed
value for the schemeless branch and unconditionally sets
cfg.InsecureExporter=true for schemeless endpoints, risking a silent TLS
downgrade; update the switch in the endpoint handling (the block that inspects
ep and sets cfg.CollectorExporterEndpoint and cfg.InsecureExporter) so that you
always write the trimmed ep back into cfg.CollectorExporterEndpoint, set
cfg.InsecureExporter=true only when the scheme is explicitly "http://" and
ensure "https://" sets or leaves InsecureExporter=false; for the default (no
scheme) branch do not change cfg.InsecureExporter (just persist the trimmed
endpoint) so TLS behavior isn’t downgraded implicitly.

}

if cfg.EnableTelemetry && strings.TrimSpace(cfg.CollectorExporterEndpoint) == "" {
return nil, ErrEmptyEndpoint
}
Expand Down
69 changes: 69 additions & 0 deletions commons/opentelemetry/otel_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,75 @@ func TestNewTelemetry_DefaultPropagatorAndRedactor(t *testing.T) {
assert.NotNil(t, tl.Redactor, "default redactor should be set")
}

// ===========================================================================
// 1b. Endpoint normalization
// ===========================================================================

func TestNewTelemetry_EndpointNormalization(t *testing.T) {
t.Parallel()

tests := []struct {
name string
endpoint string
wantEndpoint string
wantInsecure bool
insecureOverride bool // initial InsecureExporter value
}{
{
name: "http scheme stripped and insecure inferred",
endpoint: "http://otel-collector:4317",
wantEndpoint: "otel-collector:4317",
wantInsecure: true,
},
{
name: "https scheme stripped and insecure stays false",
endpoint: "https://otel-collector:4317",
wantEndpoint: "otel-collector:4317",
wantInsecure: false,
},
{
name: "no scheme defaults to insecure",
endpoint: "otel-collector:4317",
wantEndpoint: "otel-collector:4317",
wantInsecure: true,
},
{
name: "https with explicit insecure override preserved",
endpoint: "https://otel-collector:4317",
insecureOverride: true,
wantEndpoint: "otel-collector:4317",
wantInsecure: true,
},
{
name: "http with trailing slash",
endpoint: "http://otel-collector:4317/",
wantEndpoint: "otel-collector:4317/",
wantInsecure: true,
},
}
Comment on lines +102 to +140
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add regression cases for schemeless secure intent and whitespace normalization.

The table misses two high-impact scenarios in this change path: preserving secure intent for schemeless endpoints and trimming schemeless endpoints with surrounding whitespace.

Proposed test additions
 	{
 		name:         "no scheme defaults to insecure",
 		endpoint:     "otel-collector:4317",
 		wantEndpoint: "otel-collector:4317",
 		wantInsecure: true,
 	},
+	{
+		name:             "no scheme with explicit secure intent is preserved",
+		endpoint:         "otel-collector:4317",
+		insecureOverride: false,
+		wantEndpoint:     "otel-collector:4317",
+		wantInsecure:     false,
+	},
+	{
+		name:             "no scheme trims surrounding whitespace",
+		endpoint:         "   otel-collector:4317   ",
+		insecureOverride: true,
+		wantEndpoint:     "otel-collector:4317",
+		wantInsecure:     true,
+	},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@commons/opentelemetry/otel_test.go` around lines 102 - 140, Add two test
cases to the existing tests table in otel_test.go: (1) a "schemeless with
explicit secure intent preserved" case using endpoint "otel-collector:4317" with
insecureOverride set to true/false as appropriate to represent an explicitly
secure configuration (set insecureOverride: false) and expect wantEndpoint
"otel-collector:4317" and wantInsecure false so schemeless secure intent is
preserved; (2) a "schemeless with surrounding whitespace trimmed" case using
endpoint "  otel-collector:4317  " (with surrounding spaces) and expect
wantEndpoint "otel-collector:4317" and wantInsecure true to verify whitespace is
trimmed; add both rows to the tests slice that contains fields name, endpoint,
wantEndpoint, wantInsecure, insecureOverride so the table-driven test runs these
regressions.


for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

// Use telemetry disabled so we don't need a real collector.
tl, err := NewTelemetry(TelemetryConfig{
LibraryName: "test-lib",
EnableTelemetry: false,
CollectorExporterEndpoint: tt.endpoint,
InsecureExporter: tt.insecureOverride,
Logger: log.NewNop(),
})
require.NoError(t, err)
require.NotNil(t, tl)
assert.Equal(t, tt.wantEndpoint, tl.CollectorExporterEndpoint,
"endpoint should be normalized")
assert.Equal(t, tt.wantInsecure, tl.InsecureExporter,
"InsecureExporter should be inferred from scheme")
})
}
}

// ===========================================================================
// 2. Telemetry methods on nil receiver
// ===========================================================================
Expand Down
Loading