Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Telemetry API (extensionProviders) for traces results in Host header with special characters #36521

Closed
nathan-vp opened this issue Dec 15, 2021 · 4 comments

Comments

@nathan-vp
Copy link

nathan-vp commented Dec 15, 2021

Bug Description

I am trying to setup tracing integration using the Telemetry API.

The traces are sent with a Host header value that is equal to the cluster_name (e.g. Host: outbound|9411||tempo-tempo-distributed-distributor.tempo.svc.cluster.local).

This value includes special characters such as |, which can rejected by tracers. On Tempo, this manifests as a HTTP/1.1 400 Bad Request: malformed Host header response:

POST /api/v2/spans HTTP/1.1
host: outbound|9411||tempo-tempo-distributed-distributor.tempo.svc.cluster.local
content-type: application/json
x-envoy-internal: true
x-forwarded-for: 10.72.109.145
x-envoy-expected-rq-timeout-ms: 5000
transfer-encoding: chunked

<span cut for brevity>

HTTP/1.1 400 Bad Request: malformed Host header
Content-Type: text/plain; charset=utf-8
Connection: close

400 Bad Request: malformed Host header

Our meshConfig:

meshConfig:
  extensionProviders:
  - name: "traces-tempo-zipkin"
    zipkin:
      service: "tempo-tempo-distributed-distributor.tempo.svc.cluster.local"
      port: 9411
  defaultProviders:
    tracing:
    - traces-tempo-zipkin

Results in the following tracing configuration on the listener:

"provider": {
  "name": "traces-tempo-zipkin",
  "typed_config": {
    "@type": "type.googleapis.com/envoy.config.trace.v3.ZipkinConfig",
    "collector_cluster": "outbound|9411||tempo-tempo-distributed-distributor.tempo.svc.cluster.local",
    "collector_endpoint": "/api/v2/spans",
    "trace_id_128bit": true,
    "shared_span_context": false,
    "collector_endpoint_version": "HTTP_JSON"
  }
}

This is a regression from the previous method of configuring tracing. Configuring meshConfig.tracing.zipkin.address, created a cluster with a fixed name of zipkin. As zipkin does not contain any special characters, this Host header was accepted by our trace collector.

Possible solutions that I can see:

  1. Can we make the Host header configurable, by exposing the collector_hostname parameter in Envoy in some way? This would also solve External tracing.zipkin.address results in requests with host=zipkin #36166.
  2. Alternatively, can we revert to the previous behaviour, hardcoding collector_hostname to zipkin ?

Version

$ istioctl version
client version: 1.12.1
control plane version: 1.12.1
data plane version: 1.12.1 (12 proxies)
$ kubectl version --short
Client Version: v1.23.0
Server Version: v1.21.2-eks-0389ca3

Additional Information

No response

@howardjohn
Copy link
Member

Alternatively, can we revert to the previous behaviour, hardcoding collector_hostname to zipkin ?

This doesn't feel right to me.

I think we should set collector_hostname to meshConfig.extensionProviders.zipkin.service. WDYT?

@nathan-vp
Copy link
Author

I think we should set collector_hostname to meshConfig.extensionProviders.zipkin.service. WDYT?

Hi @howardjohn, thanks for the quick feedback.

I agree with you, that would be a good compromise.

This is the field description for meshConfig.extensionProviders.zipkin.service:

REQUIRED. Specifies the service that the Zipkin API. The format is [<Namespace>/]<Hostname>. The specification of <Namespace> is required only when it is insufficient to unambiguously resolve a service in the service registry. The <Hostname> is a fully qualified host name of a service defined by the Kubernetes service or ServiceEntry.

Example: “zipkin.default.svc.cluster.local” or “bar/zipkin.example.com”.

For the second case of “bar/zipkin.example.com”, I assume we should then split on the / and only use zipkin.example.com for the Host header?

I'll have a look at the code and see if I can do a PR.

@howardjohn
Copy link
Member

Oh yeah, forgot the namespace part. Yeah we should split it

@nathan-vp
Copy link
Author

This should be fixed by #36277. Unfortunately it seems this PR did not make it for 1.12.1.

Closing as duplicate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants