Add cors_allowed_headers option to confighttp #2454
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2454 +/- ##
=======================================
Coverage 91.76% 91.76%
=======================================
Files 265 265
Lines 15111 15111
=======================================
Hits 13867 13867
Misses 866 866
Partials 378 378
Continue to review full report at Codecov.
|
pmm-sumo
force-pushed
the
cors-headers
branch
from
318cee9
to
ea084e2
Compare
pmm-sumo
force-pushed
the
cors-headers
branch
from
ea084e2
to
a81cf71
Compare
|
Thank you @keitwb I adjusted the code accordingly, provided more details in doc and included few more test-cases |
|
@tigrannajaryan can you take a look as well? |
| CorsOrigins []string `mapstructure:"cors_allowed_origins"` | ||
|
|
||
| // CorsHeaders are the allowed CORS headers for HTTP/JSON requests to grpc-gateway adapter | ||
| // for the OTLP receiver. See github.com/rs/cors | ||
| // CORS needs to be enabled first by providing a non-empty list in CorsOrigins | ||
| // A wildcard (*) can be used to match any header. | ||
| CorsHeaders []string `mapstructure:"cors_allowed_headers"` |
There was a problem hiding this comment.
In an ideal world, would you have allowed headers per origin?
There was a problem hiding this comment.
Asking because we still have an opportunity to fix this before GA, so I would like to not just add things now and have to cleanup in 2 months :)
Maybe we can get this right now and deprecate the old way.
There was a problem hiding this comment.
Good question. I think we are largely relying on rs/cors model here, which does not seem to provide such capability. At the same time, something like this can be also achieved with separate receivers (though they would need different ports, so not ideal, but when used via reverse proxy maybe not so bad either). @keitwb @kkruk-sumo - what do you think?
There was a problem hiding this comment.
@pmm-sumo based on your experience what is the more common case:
- Having same headers per multiple origins?
- Having special headers per each origin?
Based on this we can optimize for one case or the other :)
There was a problem hiding this comment.
Unfortunately, I don't have much experience with CORS. My impression is that within a single deployment, origins can be quite similar to each other so they could share the set of headers (so option 1). But would be great to check with someone more experienced, hence the call to @keitwb and @kkruk-sumo :)
There was a problem hiding this comment.
I doubt many people will want special headers per origin for a single instance of a CORS enabled receiver. That seems overly complicated for the config and you can just make multiple receiver instances with separate ports if you really wanted such a thing.
The capability is out of scope of the PR
pmm-sumo
force-pushed
the
cors-headers
branch
from
caf9227
to
fc414ee
Compare
…metry#2454) Bumps [github.com/jaegertracing/jaeger](https://github.com/jaegertracing/jaeger) from 1.40.0 to 1.41.0. - [Release notes](https://github.com/jaegertracing/jaeger/releases) - [Changelog](https://github.com/jaegertracing/jaeger/blob/main/CHANGELOG.md) - [Commits](jaegertracing/jaeger@v1.40.0...v1.41.0) --- updated-dependencies: - dependency-name: github.com/jaegertracing/jaeger dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Description:
Fixes #1383 by exposing ability to set CORS Allowed Headers
Link to tracking Issue: #1383
Testing: Config unit tests updated, manual tests performed with requests sent from browser via OpenTelemetry-JS to another domain (where the collector was residing)
Documentation: Description of the newly added option added