[exporter/otlp] Improve endpoint validation

[TylerHelmuth]

Is your feature request related to a problem? Please describe.
Currently the otlp exporter's endpoint validation only checks that the endpoint has :PORT and that PORT is an integer. This does not handle situations like:

• port in the wrong place: https://my-backend/v1/traces:4317
• invalid dns endpoint: dns://my-backend:4317

Describe the solution you'd like
Add more intelligent endpoint validation that can detect these errors.

Additional context
These are both situations I've seen customer using the collector experience.

[wasim-nihal]

Hello @TylerHelmuth! I would like to work on improving this endpoint validation. Is this open for contribution?

[TylerHelmuth]

@wasim-nihal yes

[JPeer264]

I am not sure if this is part of this ticket, but when I have following config:

exporters:
  otlp/metrics:
    endpoint: https://prometheus-prod-xxx.grafana.net:443/api/prom/push
    auth:
      authenticator: basicauth/metrics

Then the collector is actually validating as following:

exporters::otlp/metrics: invalid port "443/api/prom/push"

Which indicates that the port is always expected to be last. However, if I leave the port away I get the expected error message:

exporters::otlp/metrics: address prometheus-prod-xxx.grafana.net/api/prom/push: missing port in address

I could see in, the unfortunately closed PR, #11398 exactly this use case is missing in the test cases. Unfortunately my Go knowledge is not existent to actively contribute in this specific use case effectively.

[TylerHelmuth]

@JPeer264 can you open a new issue, that feels a lot like a bug

Also try prometheus-prod-xxx.grafana.net:443/api/prom/push (no https) just in case

[JPeer264]

Alright thanks, I created a new ticket (#12536) including your suggestion.

[jade-guiton-dd]

Unless the OTLP exporter has specific requirements, shouldn't this be handled in configgrpc?

[jade-guiton-dd]

#12536 was closed, but I think some of it is relevant to this issue. Here is the situation as I understand it:

gRPC does not support paths.

gRPC uses HTTP under the hood, but the path is always determined by the service and method name. The gRPC "target", as specified in this doc, does not support the HTTP URI scheme; when no scheme is provided, according to the code, the default is "passthrough", which does not support paths after the host and port.

• When https://my-backend/v1/traces:4317 is supplied, it will be parsed as port 4317 on the my-backend/v1/traces hostname. I'm not sure how exactly this is handled by the DNS resolver, but in my tests this is resolved differently from my-backend. This is confusing behavior, and in fact may be a security risk, as on my machine the localhost/test hostname resolves successfully to an external IP...

• When https://my-backend:4317/v1/traces is supplied, as described in [exporter/otlp] port is not recognized correctly #12536, this will fail to validate on our side, as /v1/traces is treated as part of the port, resulting in an "invalid port error". The same thing happens deeper in the stack if we allow it to be passed to gRPC. This is a confusing error.

Because users confusing otlp and otlphttp (which I expect there to be many) are likely to try supplying a path, and because the behavior in both cases is confusing, I think that as part of improving endpoint validation, we should reject all endpoints containing a path with an error explaining to users that paths are not supported by OTLP over gRPC, and that they may be looking for the otlphttp exporter instead.

I would argue we should also stop silently stripping out the http:// or https:// prefix in endpoints, as it only adds to the confusion and leaves people to believe the endpoint is parsed as an HTTP URI, but that may be too big of a breaking change.