Change Zipkin CORS origins and headers to comma separated list

[JonasVerhofste]

Signed-off-by: Jonas Verhofsté 25819942+JonasVerhofste@users.noreply.github.com

Which problem is this PR solving?

When I implemented the CORS handling in f9702c4 (#1463), I forgot that there can be more than one origin and one header. This adds the functionality of specifying multiple origins and headers as a comma separated list.

[codecov]

Codecov Report

Merging #1556 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1556      +/-   ##
==========================================
- Coverage   98.36%   98.32%   -0.05%     
==========================================
  Files         193      193              
  Lines        9364     9364              
==========================================
- Hits         9211     9207       -4     
- Misses        119      122       +3     
- Partials       34       35       +1

Impacted Files	Coverage Δ
cmd/collector/app/builder/builder_flags.go	100% <100%> (ø)	⬆️
cmd/query/app/static_handler.go	83.33% <0%> (-3.51%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75b670f...47c720b. Read the comment docs.

[yurishkuro]

Could you add tests?

[JonasVerhofste]

@yurishkuro Well, it's just an edit of what I added in #1463, where you told me there are no tests? If I do want me to write some, could you point me in the direction of where I'm supposed to add them?

[JonasVerhofste]

I also based myself on how arrays are handled in the storage plugins, but I'm not sure if this is the right way. Can Viper handle arrays like this? For example, in yaml:

collector:
  zipkin:
    allowed-origins:
      - origin1
      - origin2

As far as I understand the format above is not going to work, but this format would:

collector:
  zipkin:
    allowed-origins: "origin1, origin2"

I could be wrong of course, haven't gotten the time yet to test it, it's just all just how I interpret the code. Although if I am right, I would suggest taking a look if we could somehow use both (to provide backwards compatibility). The first example is much more human readable than the second one, in my opinion. If I'm wrong, do correct me! Can always have missed something.

[yurishkuro]

I don't know about your Viper question, you'd have to try.

My earlier comment about no-testing was in relation to CORS functionality. But we do have tests for parsing command line parameters:

$ go test -cover ./cmd/collector/app/builder/
ok  	github.com/jaegertracing/jaeger/cmd/collector/app/builder	0.019s	coverage: 100.0% of statements

[JonasVerhofste]

I don't know about your Viper question, you'd have to try.

It seems I was right, only the second one works, arrays don't work. I'll see if I can fix that.

But we do have tests for parsing command line parameters:

@yurishkuro the only test I can see there is span_handler_builder_test.go, as far as I can tell, there's no test for the main.go or the builder_flags.go, which are the only files I've edited (so far).

[yurishkuro]

we shouldn't do this in main, it is better to encapsulate this into the builder, similar to this:

jaeger/cmd/agent/app/reporter/grpc/flags.go

Lines 47 to 50 in 4ed618d

	hostPorts := v.GetString(collectorHostPort)
	if hostPorts != "" {
	b.CollectorHostPorts = strings.Split(hostPorts, ",")
	}

[yurishkuro]

@JonasVerhofste do you plan to finish this one?

[JonasVerhofste]

@yurishkuro Sure, but I'm still wondering where I should add those tests. Like I noted before, cmd/collector/app/builder/builder_flags.go doesn't have a test file. Whatever I change to the file, the coverage stays the same. Which is probably because only cmd/collector/app/builder/span_handler_builder.go has a test-file?