Pass OTEL_EXPORTER_OTLP_HEADERS env var to mcpg container; remove headers from gateway JSON config#32280
Conversation
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…ve headers from gateway JSON config Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR moves OTLP authentication headers out of the rendered MCP gateway stdin JSON config and into the mcpg container environment (OTEL_EXPORTER_OTLP_HEADERS) to avoid credential exposure and fix auth failures against collectors requiring headers.
Changes:
- Forward
OTEL_EXPORTER_OTLP_HEADERSto the gateway container when OTLP is configured, and dedupe it from other env-flag sources. - Stop rendering an
opentelemetry.headersfield in the gateway JSON config; removeheadersfrom the gateway config JSON schemas. - Update docs and add/adjust tests to assert headers never appear in rendered gateway JSON.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/schemas/mcp-gateway-config.schema.json | Removes opentelemetry.headers from the internal gateway config schema. |
| docs/public/schemas/mcp-gateway-config.schema.json | Mirrors the schema change for published docs artifacts. |
| pkg/workflow/mcp_setup_generator.go | Adds -e OTEL_EXPORTER_OTLP_HEADERS when OTLP is enabled; registers it for env dedupe. |
| pkg/workflow/mcp_renderer.go | Removes emission of opentelemetry.headers in rendered gateway JSON config. |
| pkg/workflow/mcp_renderer_test.go | Updates assertions to ensure "headers" never appears in rendered gateway JSON. |
| pkg/workflow/mcp_setup_generator_test.go | Adds coverage to ensure the env var is passed to the container and not rendered into JSON. |
| docs/src/content/docs/reference/mcp-gateway.md | Removes JSON headers field docs and documents env-var-based auth headers. |
| .changeset/minor-otlp-headers-env-var.md | Release note for the breaking change and migration guidance. |
| .github/workflows/linter-miner.lock.yml | Regenerated workflow lock file (but currently still appears inconsistent with the new OTLP headers behavior—see comments). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 4
| esac | ||
| DOCKER_SOCK_GID=$(stat -c '%g' "$DOCKER_SOCK_PATH" 2>/dev/null || echo '0') | ||
| export MCP_GATEWAY_DOCKER_COMMAND='docker run -i --rm --network host --add-host host.docker.internal:127.0.0.1 --user '"${MCP_GATEWAY_UID}"':'"${MCP_GATEWAY_GID}"' --group-add '"${DOCKER_SOCK_GID}"' -v '"${DOCKER_SOCK_PATH}"':/var/run/docker.sock -e MCP_GATEWAY_PORT -e MCP_GATEWAY_DOMAIN -e MCP_GATEWAY_API_KEY -e MCP_GATEWAY_PAYLOAD_DIR -e MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD -e DOCKER_HOST=unix:///var/run/docker.sock -e DEBUG -e MCP_GATEWAY_LOG_DIR -e GH_AW_MCP_LOG_DIR -e GH_AW_SAFE_OUTPUTS -e GH_AW_SAFE_OUTPUTS_CONFIG_PATH -e GH_AW_SAFE_OUTPUTS_TOOLS_PATH -e GH_AW_ASSETS_BRANCH -e GH_AW_ASSETS_MAX_SIZE_KB -e GH_AW_ASSETS_ALLOWED_EXTS -e DEFAULT_BRANCH -e GITHUB_MCP_SERVER_TOKEN -e GITHUB_MCP_GUARD_MIN_INTEGRITY -e GITHUB_MCP_GUARD_REPOS -e GITHUB_REPOSITORY -e GITHUB_SERVER_URL -e GITHUB_SHA -e GITHUB_WORKSPACE -e GITHUB_TOKEN -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RUN_ATTEMPT -e GITHUB_JOB -e GITHUB_ACTION -e GITHUB_EVENT_NAME -e GITHUB_EVENT_PATH -e GITHUB_ACTOR -e GITHUB_ACTOR_ID -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_WORKFLOW_REF -e GITHUB_WORKFLOW_SHA -e GITHUB_REF -e GITHUB_REF_NAME -e GITHUB_REF_TYPE -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GH_AW_SAFE_OUTPUTS_PORT -e GH_AW_SAFE_OUTPUTS_API_KEY -e GITHUB_AW_OTEL_TRACE_ID -e GITHUB_AW_OTEL_PARENT_SPAN_ID -v /tmp/gh-aw/mcp-payloads:/tmp/gh-aw/mcp-payloads:rw -v /opt:/opt:ro -v /tmp:/tmp:rw -v '"${GITHUB_WORKSPACE}"':'"${GITHUB_WORKSPACE}"':rw ghcr.io/github/gh-aw-mcpg:v0.3.9' | ||
|
|
| | `serviceName` | string | No | Logical service name reported in the `service.name` resource attribute of all emitted spans. Identifies the gateway in the tracing backend. Defaults to `"mcp-gateway"` when not specified. | | ||
|
|
||
| > [!NOTE] | ||
| > Authentication headers (e.g., `Authorization: Bearer <token>`) for the OTLP collector MUST be provided via the `OTEL_EXPORTER_OTLP_HEADERS` environment variable, not through the JSON config. This follows the standard [OTel SDK environment variable convention](https://opentelemetry.io/docs/specs/otel/protocol/exporter/#configuration-options) and keeps credentials out of the stdin config pipe. The format is `key1=value1,key2=value2` per the W3C Baggage header spec. When gh-aw compiles a workflow with `observability.otlp.headers`, the value is automatically forwarded to the gateway container via `-e OTEL_EXPORTER_OTLP_HEADERS`. |
| OTLP auth headers (e.g., `Authorization: Bearer <token>`) are security-sensitive. The correct mechanism is the `OTEL_EXPORTER_OTLP_HEADERS` environment variable, which is: | ||
| - Isolated inside the container (not visible to processes outside) | ||
| - The standard OTel convention used by all OTel SDKs and collectors | ||
| - Not embedded in logs or config pipes where it could be inadvertently exposed |
| @@ -213,13 +213,11 @@ func RenderJSONMCPConfig( | |||
| // the run block (RGS-008 compliance). All four fields use ${VARIABLE_NAME} expressions | |||
| // expanded by bash from workflow-level env vars. | |||
| // Per MCP Gateway Specification §4.1.3.6 and the opentelemetryConfig schema. | |||
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent test quality
Test Classification DetailsView All Test Classifications
Analysis
i️ Test Inflation Note
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). All three tests enforce observable behavioral contracts, particularly the security invariant that OTLP auth credentials travel via container env var, not the JSON config pipe.
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.3M
| @@ -515,28 +515,24 @@ func TestRenderJSONMCPConfig_OTLPGateway(t *testing.T) { | |||
| name string | |||
| otlpEndpoint string | |||
| otlpHeaders string | |||
There was a problem hiding this comment.
[/tdd] The otlpHeaders field in the test struct is now dead data. Since the renderer no longer uses OTLPHeaders at all, the "OTLP endpoint only" and "OTLP endpoint and headers" test cases are functionally identical — both exercise the same code path and produce the same output. The field should be removed from the struct to make the test's intent clear:
tests := []struct {
name string
otlpEndpoint string
wantEndpoint bool
}{
{
name: "OTLP endpoint configured",
otlpEndpoint: "(otel.example.com/redacted)",
wantEndpoint: true,
},
...
}Leaving it in suggests there's still some renderer behavior that varies with headers, which is misleading now that headers are exclusively an env var concern.
| // Verify OTEL_EXPORTER_OTLP_HEADERS is NOT in the docker command | ||
| assert.NotContains(t, yamlStr, "-e OTEL_EXPORTER_OTLP_HEADERS", | ||
| "OTEL_EXPORTER_OTLP_HEADERS should NOT be in docker command without OTLP config") | ||
| } |
There was a problem hiding this comment.
[/tdd] Missing test: what happens when OTLP is configured but headers is empty/unset? Currently OTEL_EXPORTER_OTLP_HEADERS is unconditionally forwarded whenever OTLPEndpoint != "", regardless of whether any headers value is set. This is the right behavior (an empty env var is harmless), but a test case explicitly asserting it would pin the contract:
func TestOTLPHeadersEnvVarPassedToGatewayContainerWithoutHeaders(t *testing.T) {
// observability.otlp configured with endpoint only, no headers
// assert: -e OTEL_EXPORTER_OTLP_HEADERS still appears in docker command
// (host env var will simply be empty at runtime — that's fine)
}Without this test, a future reader could misread the intent of the buildAddedGatewayEnvVarSet registration and think the flag should only be added when headers are non-empty.
|
@copilot address review feedback |
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (102 new lines in business directories, threshold 100) but does not have a linked Architecture Decision Record (ADR). AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. This PR is a credential-handling change with a real security and functional motivation — exactly the kind of decision that benefits from being recorded explicitly. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in 🔎 Why this PR triggered the gate
A related but distinct ADR exists (ADR-28524: Object Form for
References: §25899764158
|
… Baggage reference Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Addressed all four review items in commit
|
|
@copilot merge main |
…orter-otlp-headers # Conflicts: # .github/workflows/linter-miner.lock.yml Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Merged |
|
@copilot remove the copilot otel configuration to write to a file and the code to upload the otel data |
….jsonl upload Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Removed in the latest commit:
|
|
@copilot update the compiler to remove the step that uploaded the copilot-otel data |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in the latest commit. Removed the |
|
|
OTLP auth headers were being written into the stdin JSON config pipe as
"headers": "${OTEL_EXPORTER_OTLP_HEADERS}", embedding credentials in a pipe where they could be logged or leaked. mcpg never received the env var directly, causing 401s against any collector requiring auth (Sentry, Datadog, Grafana Cloud).Changes
mcp_setup_generator.go-e OTEL_EXPORTER_OTLP_HEADERSto the mcpg docker container wheneverOTLPEndpoint != "", following the same pattern asGITHUB_AW_OTEL_TRACE_IDOTEL_EXPORTER_OTLP_HEADERSinbuildAddedGatewayEnvVarSetto prevent duplicate-eflags from HTTP MCP env deduplicationmcp_renderer.go"headers": "${OTEL_EXPORTER_OTLP_HEADERS}"line from the rendered opentelemetry JSON config block — headers are now exclusively container envJSON schemas (
pkg/workflow/schemas/anddocs/public/schemas/)headersproperty fromopentelemetryConfigdefinition in both copies ofmcp-gateway-config.schema.jsondocs/src/content/docs/reference/mcp-gateway.mdheadersfrom §4.1.3.7 field table; add note directing toOTEL_EXPORTER_OTLP_HEADERSenv varTests
TestRenderJSONMCPConfig_OTLPGateway: assert"headers"never appears in rendered JSON (regardless ofOTLPHeadersvalue)TestOTLPHeadersEnvVarPassedToGatewayContainer: compiles a workflow withobservability.otlp.headers, asserts-e OTEL_EXPORTER_OTLP_HEADERSappears in the docker command and no"headers"key in JSON configTestOTLPHeadersEnvVarNotPassedWithoutOTLP: asserts env var is absent when no OTLP is configuredBefore (credentials in JSON pipe):
After (credentials via container env):