confighttp: add ExposedHeaders to CORSConfig#15119
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends config/confighttp server CORS configuration to support setting Access-Control-Expose-Headers via receiver configuration, aligning CORSConfig with rs/cors capabilities.
Changes:
- Add
ExposedHeaders []stringtoCORSConfig. - Map
CORSConfig.ExposedHeadersintocors.OptionsinServerConfig.ToServer(). - Add a server-side test and update README documentation; add a changelog entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| config/confighttp/server.go | Adds ExposedHeaders to CORSConfig and wires it into cors.Options. |
| config/confighttp/server_test.go | Adds a test asserting Access-Control-Expose-Headers is emitted on actual requests. |
| config/confighttp/README.md | Documents the new exposed_headers config key and provides an example. |
| .chloggen/add-cors-exposed-headers.yaml | Adds release-note entry for the new configuration option. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ExposedHeaders sets the value of the Access-Control-Expose-Headers response | ||
| // header, indicating which headers are safe to expose to the API of a CORS response. | ||
| ExposedHeaders []string `mapstructure:"exposed_headers,omitempty"` |
There was a problem hiding this comment.
CORSConfig gained ExposedHeaders, but the package’s config.schema.yaml still defines cors_config without an exposed_headers property. This will make the new setting invisible/invalid for schema-driven tooling. Update config/confighttp/config.schema.yaml to include exposed_headers (type array of strings) with a matching description.
| co := cors.Options{ | ||
| AllowedOrigins: corsConfig.AllowedOrigins, | ||
| AllowCredentials: true, | ||
| AllowedHeaders: corsConfig.AllowedHeaders, | ||
| ExposedHeaders: corsConfig.ExposedHeaders, |
There was a problem hiding this comment.
CORS config is ignored when AllowedOrigins is empty, but the warning currently only triggers when AllowedHeaders is set. With the new ExposedHeaders field (and existing MaxAge), misconfigurations can now be silently ignored. Consider broadening the warning condition/message to include other non-empty CORS fields (e.g., ExposedHeaders, MaxAge).
| func TestHTTPCorsExposedHeaders(t *testing.T) { | ||
| sc := &ServerConfig{ | ||
| NetAddr: confignet.AddrConfig{ | ||
| Endpoint: "localhost:0", | ||
| Transport: confignet.TransportTypeTCP, | ||
| }, | ||
| CORS: configoptional.Some(CORSConfig{ | ||
| AllowedOrigins: []string{"http://allowed.com"}, | ||
| ExposedHeaders: []string{"X-Custom-Header", "X-Another-Header"}, | ||
| }), |
There was a problem hiding this comment.
This test validates wiring into cors.Options, but it doesn’t verify that exposed_headers can be set via YAML/unmarshal (the core motivation in the PR description). Consider extending the existing YAML unmarshal “comprehensive config” testdata + assertions to include exposed_headers, so the mapstructure tag is exercised.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #15119 +/- ##
==========================================
+ Coverage 91.24% 91.25% +0.01%
==========================================
Files 699 699
Lines 44913 44914 +1
==========================================
+ Hits 40979 40986 +7
+ Misses 2786 2782 -4
+ Partials 1148 1146 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Rajneesh Chaudhary <[email protected]>
5d10680 to
2a4db6b
Compare
Merging this PR will degrade performance by 30.87%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | BenchmarkHTTPProtoLogsSequential |
3.7 ms | 5 ms | -25.49% |
| ❌ | BenchmarkGRPCLogsSequential |
4.3 ms | 6.2 ms | -30.87% |
Comparing Rajneesh180:feat/cors-exposed-headers (2a4db6b) with main (307e3ab)
Footnotes
-
76 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
@mx-psi The CodSpeed regression looks like a false positive — their own report flags "Unknown Walltime execution environment detected" and "Different runtime environments detected," which explains the variance. The change itself only adds a The YAML unmarshal test for |
|
Yes, codespeed is rather unreliable. You can ignore it. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
07f953b
|
Thank you for your contribution @Rajneesh180! 🎉 We would like to hear from you about your experience contributing to OpenTelemetry by taking a few minutes to fill out this survey. |
…rtracing#7827) confighttp v0.154.0 added ExposedHeaders to CORSConfig (open-telemetry/opentelemetry-collector#15119), so the MCP server no longer needs a custom middleware to send Access-Control-Expose-Headers. Browser-based MCP clients can read the Mcp-Session-Id response header by listing it under cors::exposed_headers. - bump confighttp to v0.154.0 along with its interlocked collector modules - drop the custom expose-headers middleware - document the CORS setup for browser clients in README and Config - cover CORS preflight, origin matching, allowed-headers auto-mirror, and exposed_headers in server tests Signed-off-by: Rajneesh Chaudhary <[email protected]>
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [go.opentelemetry.io/collector/component](https://github.com/open-telemetry/opentelemetry-collector) | `v1.59.0` → `v1.60.0` |  |  | | [go.opentelemetry.io/collector/component/componenttest](https://github.com/open-telemetry/opentelemetry-collector) | `v0.153.0` → `v0.154.0` |  |  | | [go.opentelemetry.io/collector/confmap](https://github.com/open-telemetry/opentelemetry-collector) | `v1.59.0` → `v1.60.0` |  |  | | [go.opentelemetry.io/collector/consumer](https://github.com/open-telemetry/opentelemetry-collector) | `v1.59.0` → `v1.60.0` |  |  | | [go.opentelemetry.io/collector/consumer/consumertest](https://github.com/open-telemetry/opentelemetry-collector) | `v0.153.0` → `v0.154.0` |  |  | | [go.opentelemetry.io/collector/pdata](https://github.com/open-telemetry/opentelemetry-collector) | `v1.59.0` → `v1.60.0` |  |  | | [go.opentelemetry.io/collector/processor](https://github.com/open-telemetry/opentelemetry-collector) | `v1.59.0` → `v1.60.0` |  |  | | [go.opentelemetry.io/collector/processor/processortest](https://github.com/open-telemetry/opentelemetry-collector) | `v0.153.0` → `v0.154.0` |  |  | --- ### Release Notes <details> <summary>open-telemetry/opentelemetry-collector (go.opentelemetry.io/collector/component)</summary> ### [`v1.60.0`](https://github.com/open-telemetry/opentelemetry-collector/blob/HEAD/CHANGELOG.md#v1600v01540) ##### 🛑 Breaking changes 🛑 - `cmd/builder`: The `--skip-get-modules` flag will no longer regenerate your `go.mod` file. ([#​15390](open-telemetry/opentelemetry-collector#15390)) This is mostly a bug fix, as it led to adverse behaviour that was unintended in the described flow in the README. Now when you run `--skip-get-modules`, your `go.mod` file will truly be untouched by `ocb` as the info log claims. ##### 💡 Enhancements 💡 - `pkg/config/configtls`: Add `include_insecure_cipher_suites` to configtls to enable insecure cipher suites. Insecure cipher suites are disabled by default for security. ([#​13829](open-telemetry/opentelemetry-collector#13829)) - `pkg/confighttp`: Add `ExposedHeaders` field to `CORSConfig` to allow setting the `Access-Control-Expose-Headers` response header. ([#​15119](open-telemetry/opentelemetry-collector#15119)) ##### 🧰 Bug fixes 🧰 - `cmd/mdatagen`: Removes the extra line in the README.md between status and description ([#​15306](open-telemetry/opentelemetry-collector#15306)) - `pkg/exporterhelper`: Fix nil-pointer panic in `sending_queue::batch` Unmarshal when `sending_queue::sizer` is set and `sending_queue::batch::enabled` is false. ([#​14687](open-telemetry/opentelemetry-collector#14687)) When `sending_queue::sizer` was set and `sending_queue::batch::enabled: false` cleared the batch Optional to None, the sizer-inheritance branch in `queuebatch.Config.Unmarshal` dereferenced a nil Optional and crashed the collector at startup. The branch now also requires `Batch.HasValue()`. <!-- previous-version --> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My41LjQiLCJ1cGRhdGVkSW5WZXIiOiI0My41LjQiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbXX0=--> Reviewed-on: https://gitea.t000-n.de/t.behrendt/tracebasedlogsampler/pulls/75 Reviewed-by: t.behrendt <[email protected]> Co-authored-by: Renovate Bot <[email protected]> Co-committed-by: Renovate Bot <[email protected]>
CORSConfigmapsAllowedOrigins,AllowedHeaders, andMaxAgeintocors.Options, but doesn't mapExposedHeaderseven thoughrs/corssupports it. This means there's no way to setAccess-Control-Expose-Headersthrough receiver config — anyone who needs it has to write a custom middleware.This came up in jaegertracing/jaeger#8128 where the Jaeger UI needs specific headers exposed over CORS but has to work around this gap with a hand-rolled middleware.
Changes
ExposedHeaders []stringtoCORSConfigcors.OptionsinToServer()