Enable CORS settings on OTLP HTTP endpoint #4586

pmuls99 · 2023-07-16T06:02:57Z

Which problem is this PR solving?

Resolves [Feature]: Support CORS headers on OTEL HTTP endpoint #4459

Short description of the changes

Added a new package corscfg to handle the CORS requests on OTLP HTTP Endpoint and Zipkin

pmuls99 · 2023-07-16T06:04:39Z

@yurishkuro , had to close the previous PR, had signed off commits that were not authored by me. I will make the updates on this PR. Sorry for the inconvinience.

yurishkuro · 2023-07-16T14:57:54Z

That's fine, but please refer to comments on the old PR

codecov · 2023-07-17T15:27:23Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (587fb74) 97.03% compared to head (1e1994a) 97.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4586      +/-   ##
==========================================
+ Coverage   97.03%   97.05%   +0.02%     
==========================================
  Files         301      302       +1     
  Lines       17839    17853      +14     
==========================================
+ Hits        17310    17328      +18     
+ Misses        424      421       -3     
+ Partials      105      104       -1

Flag	Coverage Δ
unittests	`97.05% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/collector/app/collector.go	`87.90% <100.00%> (-0.10%)`	⬇️
cmd/collector/app/flags/flags.go	`100.00% <100.00%> (ø)`
cmd/collector/app/handler/otlp_receiver.go	`100.00% <100.00%> (ø)`
cmd/collector/app/server/zipkin.go	`92.30% <100.00%> (-0.42%)`	⬇️
pkg/config/corscfg/flags.go	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

pmuls99 · 2023-07-17T15:28:50Z

@yurishkuro , have added tests and since you had pointed out about the backward compatibility with Zipkin endpoint, fixed that.

yurishkuro · 2023-07-17T16:17:32Z

please run make fmt

yurishkuro · 2023-07-17T16:19:01Z

cmd/collector/app/server/zipkin.go

-	origins := strings.Split(strings.ReplaceAll(params.AllowedOrigins, " ", ""), ",")
-	headers := strings.Split(strings.ReplaceAll(params.AllowedHeaders, " ", ""), ",")
+	allowedOrigins := strings.Split(strings.ReplaceAll(params.CORSConfig.AllowedOrigins, " ", ""), ",")
+	allowedHeaders := strings.Split(strings.ReplaceAll(params.CORSConfig.AllowedHeaders, " ", ""), ",")


I think this can be done once in corscfg/flags when parsing from Viper

yurishkuro · 2023-07-18T01:14:19Z

pkg/config/corscfg/flags.go

+}
+
+func (c Flags) AddFlags(flags *flag.FlagSet) {
+	flags.String(c.Prefix+corsAllowedHeaders, "content-type", "Allowed headers for the HTTP port , default content-type")


Suggested change

flags.String(c.Prefix+corsAllowedHeaders, "content-type", "Allowed headers for the HTTP port , default content-type")

flags.String(c.Prefix+corsAllowedHeaders, "content-type", "Comma-separated CORS allowed headers. Cf. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers")

pkg/config/corscfg/flags.go

yurishkuro · 2023-07-18T01:19:19Z

pkg/config/corscfg/flags_test.go

+		v := viper.New()
+		command := cobra.Command{}
+		flagSet := &flag.FlagSet{}
+		flagCfg := Flags{
+			Prefix: "prefix",
+		}
+		flagCfg.AddFlags(flagSet)
+		command.PersistentFlags().AddGoFlagSet(flagSet)
+		v.BindPFlags(command.PersistentFlags())


Use Viperize, e.g.

jaeger/cmd/query/app/flags_test.go

Line 35 in 587fb74

v, command := config.Viperize(AddFlags)

pkg/config/corscfg/options.go

yurishkuro · 2023-07-18T01:21:15Z

almost done. Please make sure all commits are signed, see CONTRIBUTING

pkg/config/corscfg/flags.go

pmuls99 · 2023-07-18T05:56:32Z

Even after signing off previous commits, the DCO check doesnt seem to pass. i.e if I signoff the previous commit, it creates a new commit hash and the previous commit stays on the branch. Is there an alternate strategy you have for this @yurishkuro ?

yurishkuro · 2023-07-18T06:16:21Z

See CONTRIBUTING_GUIDELINES.md for how to deal with unsigned commits

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

pmuls99 · 2023-07-18T06:32:03Z

Done, @yurishkuro should I change the default value of Allowed origins to nil ? Previously with Zipkin endpoint , * was the default value , so I had set that. But as you said, that doesnt seem right.

yurishkuro · 2023-07-19T00:52:19Z

Yes, I would change the default to "". However, I question if we should check for both fields being empty before adding the CORS handler in the first place - if user didn't configure it, maybe they don't need CORS.

pmuls99 · 2023-07-19T07:18:18Z

Yes, we could do that. I checked the implementation of CORS in the confighttp.ServerSettings, they also seem to use cors.New to configure cors. This sets the default value for Origins being All, and Headers being the sensible ones. Should we prompt the user in the usage string regarding the same ?

yurishkuro · 2023-07-19T07:46:06Z

can you elaborate or point to code?

pmuls99 · 2023-07-19T13:33:58Z

If we absoutely dont want any CORS, we can do that. But if we set the value to "" in the cors flags default string, then here are the default values for them https://github.com/rs/cors/blob/e90f167479505c4dbe1161306c3c977f162c1442/cors.go#L136

yurishkuro · 2023-07-19T15:40:46Z

what about OTEL receiver? If it's CORS struct is empty, does it still install a CORS handler, with said defaults?

I am ok with going with blank defaults (which for Oorigins seems equivalent to current * in Zipkin handler, based on the code pointer you provided).

pmuls99 · 2023-07-19T15:58:20Z

It doesnt. It does allow default value for Allowed headers if Allowed origins is specified. Else it does not install CORS handler at all.
We can have it similar like Zipkin endpoint as you said.

yurishkuro · 2023-07-19T16:10:02Z

Let's just go with blank defaults and not over-complicate the logic. Sounds like doing so will be backwards compatible with both Zipkin and OTEL endpoints.

pmuls99 · 2023-07-19T16:13:12Z

Alright!

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

pkg/config/corscfg/flags.go

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

yurishkuro · 2023-07-19T16:47:30Z

Thanks!

vikashpisces · 2023-12-08T11:11:54Z

@pmuls99 While scrolling issues tab for CORS solution, I came across this PR.

I have been strruggling in configuring/ disabling cors for OTEL's http receiver endpoint on port 4318. I'm getting below error on latest all-in-one build:

Access to XMLHttpRequest at 'http://localhost:4318/v1/traces' from origin 'https://localhost:5173' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource.

I did try configuring over yaml, env and cli option for the flag: --collector.otlp.http.cors.allowed-origins

While the same port 4318 is working fine for sending otel traces from nodejs backend service, however same fails from browser application on the same endpoint http://localhost:4318/v1/traces.

Any help or direction what is going wrong or anything that I'm missing in configuring it.

Thanks in Advance.

pmuls99 · 2023-12-09T12:24:03Z

@vikashpisces , is your nodejs backend hosted somewhere and not running on the local host?

vikashpisces · 2023-12-11T10:11:59Z

@pmuls99 Never mind, I was only passing allowed_origins and it did'nt work. I had to add allowed_headers as well and it worked, not sure why. Ideally Access-Control-Allow-Origin header should have been added without allowed_headers being not there.

yurishkuro · 2023-12-11T16:42:02Z

@vikashpisces please post the full command that worked for you so that it's easier for people to reuse when someone comes across this

blm768 · 2024-04-17T21:09:44Z

This worked for me:

jaeger-all-in-one \
  --collector.otlp.http.cors.allowed-origins https://my.origin \
  --collector.otlp.http.cors.allowed-headers '*' \
  --collector.otlp.http.tls.enabled \
  --collector.otlp.http.tls.cert jaeger.crt \
  --collector.otlp.http.tls.key jaeger.key

This didn't.

jaeger-all-in-one \
  --collector.otlp.http.cors.allowed-origins https://my.origin \
  --collector.otlp.http.tls.enabled \
  --collector.otlp.http.tls.cert jaeger.crt \
  --collector.otlp.http.tls.key jaeger.key

The TLS stuff is probably only necessary for setups like mine that have the root page served over HTTPS.

pmuls99 requested a review from a team as a code owner July 16, 2023 06:02

pmuls99 requested a review from vprithvi July 16, 2023 06:02

yurishkuro reviewed Jul 17, 2023

View reviewed changes

yurishkuro reviewed Jul 18, 2023

View reviewed changes

pkg/config/corscfg/flags.go Outdated Show resolved Hide resolved

yurishkuro reviewed Jul 18, 2023

View reviewed changes

pkg/config/corscfg/flags.go Outdated Show resolved Hide resolved

pmuls99 force-pushed the enableCORSSettings branch from 477c439 to cd33dce Compare July 18, 2023 05:53

signing off all the commits

cf3b55e

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

pmuls99 force-pushed the enableCORSSettings branch from cd33dce to cf3b55e Compare July 18, 2023 06:29

pmuls99 changed the title ~~[WIP] Enabling CORS Settings on OTLP HTTP Endpoint~~ Enabling CORS Settings on OTLP HTTP Endpoint Jul 19, 2023

changed the default value

fa0fd24

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

yurishkuro reviewed Jul 19, 2023

View reviewed changes

pkg/config/corscfg/flags.go Outdated Show resolved Hide resolved

changed usage string

1e1994a

Signed-off-by: bugslayer-332 <ayashwanth9503@gmail.com>

yurishkuro approved these changes Jul 19, 2023

View reviewed changes

yurishkuro changed the title ~~Enabling CORS Settings on OTLP HTTP Endpoint~~ Enable CORS settings on OTLP HTTP endpoint Jul 19, 2023

yurishkuro enabled auto-merge (squash) July 19, 2023 16:47

yurishkuro merged commit 013ec4d into jaegertracing:main Jul 19, 2023
31 checks passed

pmuls99 deleted the enableCORSSettings branch July 20, 2023 07:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CORS settings on OTLP HTTP endpoint #4586

Enable CORS settings on OTLP HTTP endpoint #4586

pmuls99 commented Jul 16, 2023

pmuls99 commented Jul 16, 2023 •

edited

yurishkuro commented Jul 16, 2023

codecov bot commented Jul 17, 2023 •

edited

pmuls99 commented Jul 17, 2023

yurishkuro commented Jul 17, 2023

yurishkuro Jul 17, 2023

pmuls99 Jul 18, 2023

yurishkuro Jul 18, 2023

yurishkuro Jul 18, 2023

yurishkuro commented Jul 18, 2023

pmuls99 commented Jul 18, 2023

yurishkuro commented Jul 18, 2023

pmuls99 commented Jul 18, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

vikashpisces commented Dec 8, 2023

pmuls99 commented Dec 9, 2023

vikashpisces commented Dec 11, 2023

yurishkuro commented Dec 11, 2023

blm768 commented Apr 17, 2024

	flags.String(c.Prefix+corsAllowedHeaders, "content-type", "Allowed headers for the HTTP port , default content-type")
	flags.String(c.Prefix+corsAllowedHeaders, "content-type", "Comma-separated CORS allowed headers. Cf. https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Headers")

Enable CORS settings on OTLP HTTP endpoint #4586

Enable CORS settings on OTLP HTTP endpoint #4586

Conversation

pmuls99 commented Jul 16, 2023

Which problem is this PR solving?

Short description of the changes

pmuls99 commented Jul 16, 2023 • edited

yurishkuro commented Jul 16, 2023

codecov bot commented Jul 17, 2023 • edited

Codecov Report

pmuls99 commented Jul 17, 2023

yurishkuro commented Jul 17, 2023

yurishkuro Jul 17, 2023

Choose a reason for hiding this comment

pmuls99 Jul 18, 2023

Choose a reason for hiding this comment

yurishkuro Jul 18, 2023

Choose a reason for hiding this comment

yurishkuro Jul 18, 2023

Choose a reason for hiding this comment

yurishkuro commented Jul 18, 2023

pmuls99 commented Jul 18, 2023

yurishkuro commented Jul 18, 2023

pmuls99 commented Jul 18, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

pmuls99 commented Jul 19, 2023

yurishkuro commented Jul 19, 2023

vikashpisces commented Dec 8, 2023

pmuls99 commented Dec 9, 2023

vikashpisces commented Dec 11, 2023

yurishkuro commented Dec 11, 2023

blm768 commented Apr 17, 2024

pmuls99 commented Jul 16, 2023 •

edited

codecov bot commented Jul 17, 2023 •

edited