fix(otel): normalize endpoint URL and infer insecure mode from scheme#362
Merged
gandalf-at-lerian merged 1 commit intodevelopfrom Mar 19, 2026
Merged
Conversation
The gRPC WithEndpoint() expects host:port, not a full URL. When consumers pass OTEL_EXPORTER_OTLP_ENDPOINT as 'http://host:4317' (standard format), the SDK incorrectly appends :443, producing 'http://host:4317:443'. This was introduced in 387a994 (March 7) when InsecureExporter became a conditional flag. Previously WithInsecure() was always hardcoded. Consumers that migrated to v4 without setting InsecureExporter=true got broken OTEL export silently. Fix: auto-detect scheme in CollectorExporterEndpoint: - http:// → strip scheme, set InsecureExporter=true - https:// → strip scheme, keep InsecureExporter as-is - no scheme → assume insecure (common in k8s internal comms) This is backwards-compatible: no consumer code changes needed.
WalkthroughThe pull request introduces endpoint normalization logic in the 🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@commons/opentelemetry/otel_test.go`:
- Around line 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.
In `@commons/opentelemetry/otel.go`:
- Around line 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.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 131a8b4d-befa-4677-b756-b16df0902e3e
📒 Files selected for processing (2)
commons/opentelemetry/otel.gocommons/opentelemetry/otel_test.go
Comment on lines
+102
to
+140
| 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, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
Comment on lines
+104
to
+114
| 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 | ||
| } |
There was a problem hiding this comment.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
otlptracegrpc.WithEndpoint()(and metric/log equivalents) expecthost:port, not a full URL. Consumers commonly passOTEL_EXPORTER_OTLP_ENDPOINTashttp://host:4317.When
InsecureExporterisfalse(the default since v4), the gRPC SDK assumes TLS and appends:443, resulting inhttp://host:4317:443— which fails with:This affects all services on lib-commons v4 that don't explicitly set
InsecureExporter: true(tenant-manager, matcher, etc).Root Cause
Introduced in
387a994(March 7, 2026) whenInsecureExporterbecame a conditional flag. PreviouslyWithInsecure()was always hardcoded in v2/v3.Fix
Auto-detect scheme in
CollectorExporterEndpointduringNewTelemetry():http://→ strip scheme, setInsecureExporter = truehttps://→ strip scheme, keepInsecureExporteras-isBackwards-compatible: no consumer code changes needed. Existing consumers that pass
http://host:4317will work correctly without addingInsecureExporter: true.Tests
5 table-driven test cases covering all normalization scenarios. All existing tests pass.