Add `WithHostHeader` config option to override Host header in opentelemetry requests

[enuret]

Adding configuration option to override Host header. This header is not overriding by providing it as part of overrided Headers and a separate field "Host" in Request is used. https://pkg.go.dev/net/http#Request

Host header is used by some load balancers, for example, envoy uses it for routing requests. The similar change is planned to be made in otelcollector

Use case: My go client sends otel data via envoy go client -> envoy -> otel collector

Since envoy performs routing is based on "Host" header , it has to be added into request.

[dmathieu]

Why can't you use the "WithHeaders" config?

[enuret]

Why can't you use the "WithHeaders" config?

Opentelemetry client uses net.http.Request to make http requests and this library doesn't override Host header when it is passed as Request.Headers field and that's why WithHeaders doesn't work for Host header. They have a special field Request.Host if Host header has to be overrided.

This is a reason why I added a separate configuration for Host to be consistent with the net.http go library. I worked with Java instrumentation and there is no such behavior, Headers overrides host as well.

I can alternatively check if Host is in the headers when forming the client request and then set up this Request.Host field instead of adding a separate configuration if it looks like a more preferred way.

[dmathieu]

Thank you for the details. I think exposing this option, when we already allow setting a host when the endpoint is initialized is exposing a net/http implementation detail.

You could use a different Host than the one set when we create the http request with a custom transport which would modify the request accordingly and call the proper parent transport to make the request accordingly.

[enuret]

You could use a different Host than the one set when we create the http request with a custom transport which would modify the request accordingly and call the proper parent transport to make the request accordingly.

Hi, @dmathieu I didn't understand where you meant calling parent transport protocol, can you please clarify what you meant by that? In the meantime I changed preparation request place to check header name and set it to request where we copy headers. Please check if it is ok

[dmathieu]

My apologies. I thought the otlp exporter allowed specifying a custom RoundTripper, which would have been more extensible, as it would have allowed you to modify anything within the request. We don't provide that option, and previous discussions about it have other issues (see https://github.com/open-telemetry/opentelemetry-go/issues/1858).

Between both of your solutions, however, I think an option is better than reusing the Headers one, which is a bit too hacky.

[enuret]

Between both of your solutions, however, I think an option is better than reusing the Headers one, which is a bit too hacky.

I agree, let me revert my last commit

[enuret]

@dmathieu I reverted to the WithHostHeader version

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 91.17647% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 83.4%. Comparing base (76921e9) to head (563d2f3).

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #4780   +/-   ##
=====================================
  Coverage   83.4%   83.4%           
=====================================
  Files        238     238           
  Lines      15745   15779   +34     
=====================================
+ Hits       13143   13174   +31     
- Misses      2314    2317    +3     
  Partials     288     288           

Files	Coverage Δ
...tlpmetric/otlpmetricgrpc/internal/oconf/options.go	89.5% <100.0%> (+0.3%)	:arrow_up:
exporters/otlp/otlpmetric/otlpmetrichttp/client.go	82.3% <100.0%> (+0.2%)	:arrow_up:
exporters/otlp/otlpmetric/otlpmetrichttp/config.go	87.0% <100.0%> (+0.8%)	:arrow_up:
...tlpmetric/otlpmetrichttp/internal/oconf/options.go	92.8% <100.0%> (+0.2%)	:arrow_up:
...pmetric/otlpmetrichttp/internal/otest/collector.go	61.1% <100.0%> (+0.5%)	:arrow_up:
...trace/otlptracegrpc/internal/otlpconfig/options.go	93.6% <100.0%> (+0.2%)	:arrow_up:
exporters/otlp/otlptrace/otlptracehttp/client.go	79.2% <100.0%> (+0.2%)	:arrow_up:
...trace/otlptracehttp/internal/otlpconfig/options.go	92.1% <100.0%> (+0.2%)	:arrow_up:
exporters/otlp/otlptrace/otlptracehttp/options.go	100.0% <100.0%> (ø)
...pmetric/otlpmetricgrpc/internal/otest/collector.go	22.3% <0.0%> (-0.4%)	:arrow_down:

[enuret]

@dmathieu fixed Lint + rebase

[dmathieu]

I still don't get why we need this. In net/http, NewRequest will set the host to the hostname of the passed URL.

See https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/net/http/request.go;l=896

So, if you do: otlptracehttp.New(WithEndpoint("myenvoyhostname")), then the Host header should already be set properly, with the hostname provided in the endpoint.

[enuret]

I still don't get why we need this. In net/http, NewRequest will set the host to the hostname of the passed URL.

See https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/net/http/request.go;l=896

So, if you do: otlptracehttp.New(WithEndpoint("myenvoyhostname")), then the Host header should already be set properly, with the hostname provided in the endpoint.

You are right, but it is not what I need. So in general for letting envoy route it should be called like

curl -H 'Host: target.com' http://myenvoyhostname

WithEndpoint("myenvoyhostname") will produce Host = "myenvoyhostname" but it is not what I want, I want Host = "target.com"

that's why here I allow to declare "Host" different from the host that is set in WithEndpoint

[dmathieu]

You can run lints on your own machine. You do not need to rely on GH CI to check them for you.

[enuret]

You can run lints on your own machine. You do not need to rely on GH CI to check them for you.

which command should I use to run locally? I tried but it gives ablotulely different result from what I see in CI, I run golangci-lint run

[dmathieu]

make lint, same as the CI itself runs.

[enuret]

make lint, same as the CI itself runs.

I actually cannot make it work

Updating intra-repository dependencies in all go modules
go mod tidy in .
go mod tidy in ./bridge/opencensus
go: downloading github.com/kr/text v0.1.0
go mod tidy in ./bridge/opencensus/test
go mod tidy in ./bridge/opentracing
go mod tidy in ./bridge/opentracing/test
go mod tidy in ./example/dice
go mod tidy in ./example/namedtracer
go mod tidy in ./example/opencensus
go mod tidy in ./example/otel-collector
go mod tidy in ./example/passthrough
go mod tidy in ./example/prometheus
go mod tidy in ./example/zipkin
go mod tidy in ./exporters/otlp/otlpmetric/otlpmetricgrpc
go mod tidy in ./exporters/otlp/otlpmetric/otlpmetrichttp
go mod tidy in ./exporters/otlp/otlptrace
go mod tidy in ./exporters/otlp/otlptrace/otlptracegrpc
go mod tidy in ./exporters/otlp/otlptrace/otlptracehttp
go mod tidy in ./exporters/prometheus
go mod tidy in ./exporters/stdout/stdoutmetric
go mod tidy in ./exporters/stdout/stdouttrace
go mod tidy in ./exporters/zipkin
go mod tidy in ./internal/tools
go: downloading github.com/cloudflare/circl v1.3.7
go mod tidy in ./metric
go mod tidy in ./schema
go mod tidy in ./sdk
go mod tidy in ./sdk/metric
go mod tidy in ./trace
golangci-lint .
golangci-lint ./bridge/opencensus
golangci-lint ./bridge/opencensus/test
golangci-lint ./bridge/opentracing
golangci-lint ./bridge/opentracing/test
ERRO [linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies 
make: *** [golangci-lint/./bridge/opentracing/test] Error 7```

[enuret]

I have a bunch of things which probably work not in the way they are supposed to

1. make check-clean-work-tree doesn't give me any errors, while it gives some errors in the CI

2. when I do make precommit it changes one line in many files

-package oconf // import "go.opentelemetry.io/otel/exporters/otlp/otlpmetric/otlpmetricgrpc/internal/oconf"
+package oconf

3. make has some errors

make
go generate ./...
go generate ./bridge/opencensus/...
go generate ./bridge/opencensus/test/...
go generate ./bridge/opentracing/...
go generate ./bridge/opentracing/test/...
go generate ./example/dice/...
go generate ./example/namedtracer/...
go generate ./example/opencensus/...
go generate ./example/otel-collector/...
go generate ./example/passthrough/...
go generate ./example/prometheus/...
go generate ./example/zipkin/...
go generate ./exporters/otlp/otlpmetric/otlpmetricgrpc/...
go generate ./exporters/otlp/otlpmetric/otlpmetrichttp/...
go generate ./exporters/otlp/otlptrace/...
go generate ./exporters/otlp/otlptrace/otlptracegrpc/...
go generate ./exporters/otlp/otlptrace/otlptracehttp/...
go generate ./exporters/prometheus/...
go generate ./exporters/stdout/stdoutmetric/...
go generate ./exporters/stdout/stdouttrace/...
go generate ./exporters/zipkin/...
go generate ./metric/...
go generate ./schema/...
go generate ./sdk/...
go generate ./sdk/metric/...
stringer: no values defined for type InstrumentKind
instrument.go:15: running "stringer": exit status 1
stringer: no values defined for type Temporality
metricdata/temporality.go:15: running "stringer": exit status 1
make: *** [go-generate/./sdk/metric] Error 1

I don't where the problem is, maybe versions of something do not match.. And that's why I rely on CI results, but now I am stuck cause don't know why make check-clean-work-tree fails in CI but doesn't fail locally

[enuret]

Ok, found that I used go 1.21, and on 1.20 it is fine

[enuret]

I do not think we should add another option when we already have WithHeaders. This will only add confusion (how are conflicts resolved; which is the recommended option to use?).

Why not parse the headers passed to WithHeaders and set the Host field if it is passed there?

I actually agree with you now, it will be consistent with how i did https://github.com/open-telemetry/opentelemetry-collector/pull/9411 and more predictable on user side.

[dmathieu]

I suggested not using WithHeaders to better match the http.Request API, which has its own attribute and doesn't automagically retrieve the host from there.

[MadVikingGod]

I do not think we should add another option when we already have WithHeaders. This will only add confusion (how are conflicts resolved; which is the recommended option to use?).

Why not parse the headers passed to WithHeaders and set the Host field if it is passed there?

I think we should add ~WithHeaders~ WithHostHeader .

I don't think there is any confusion if you already use the http package. http will strip out any Host header you put in your request.Headers, and will only send Host if you use request.Host. We will follow the same logic, just with options instead of fields on the request. There also doesn't need to be any conflict resolution; if you include Host in your headers, the underlying http.Client will ignore it, and we will take no action to fill it in.

I made a gist to demonstrate the behavior: https://gist.github.com/MadVikingGod/16a92daebf61c67a7ff7d846b11d93c4 It's not a playground link because httptest hangs on the playground.

[pellared]

@enuret Can you please address @MrAlias comments and fix the build?

[pellared]

@enuret, bump.