confighttp: add ExposedHeaders to CORSConfig#15119
confighttp: add ExposedHeaders to CORSConfig#15119Rajneesh180 wants to merge 1 commit intoopen-telemetry:mainfrom
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 <rajneeshrehsaan48@gmail.com>
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. |
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()