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

OTLP Log gRPC ignores OTEL_EXPORTER_OTLP_INSECURE=true #5719

Closed
jpkrohling opened this issue Aug 18, 2024 · 3 comments · Fixed by #5739
Closed

OTLP Log gRPC ignores OTEL_EXPORTER_OTLP_INSECURE=true #5719

jpkrohling opened this issue Aug 18, 2024 · 3 comments · Fixed by #5739
Assignees
Labels
good first issue Good for newcomers
Milestone

Comments

@jpkrohling
Copy link
Member

Context:

When setting OTEL_EXPORTER_OTLP_INSECURE=true, this is ignored, as the envInsecure is using envEndpoint, when it should use something like:

	envInsecure = []string{
		"OTEL_EXPORTER_OTLP_LOGS_INSECURE",
		"OTEL_EXPORTER_OTLP_INSECURE",
	}
@jpkrohling jpkrohling added the good first issue Good for newcomers label Aug 18, 2024
@amanakin
Copy link
Contributor

Hi there!
The current behavior makes sense, as stated in the specification:

Insecure: Whether to enable client transport security for the exporter’s gRPC connection. This option only applies to OTLP/gRPC when an endpoint is provided without the http or https scheme - OTLP/HTTP always uses the scheme provided for the endpoint. Implementations MAY choose to not implement the insecure option if it is not required or supported by the underlying gRPC client implementation.

So when OTEL_EXPORTER_OTLP_ENDPOINT is passed, we use Insecure=false if the scheme is https, and Insecure=true otherwise.

func convInsecure(s string) (bool, error) {

I want to draw attention to the fact that the value of this environment variable must include a schema (or look like //url.without.scheme:8080 for urls without scheme). This is because we use url.Parse, which states:

Trying to parse a hostname and path without a scheme is invalid but may not necessarily return an error, due to parsing ambiguities.
(Probably that should be stated more explicitly)

Additionally, I'm not sure if it is correct behavior that if we create an exporter with these settings:
otlploggrpc.New(nil, otlploggrpc.WithEndpointURL("https://some.url:8080"), otlploggrpc.WithInsecure())
We will get an insecure connection with an https endpoint, because WithInsecure goes after WithEndpointURL.

@jpkrohling
Copy link
Member Author

jpkrohling commented Aug 19, 2024

OTEL_EXPORTER_OTLP_ENDPOINT is not provided in my case, I'm relying on the default value. Note that the behavior is correct for traces and metrics.

@amanakin
Copy link
Contributor

Yeah, you are right :)
I just wanted to point out that the fix could be a bit tricky and not just a simple change to envInsecure.
Anyway, I can work on it.

dmathieu added a commit that referenced this issue Sep 6, 2024
…URE environments in grpc exporter (#5739)

Closes #5719

In this commit I add OTEL_EXPORTER_OTLP_LOGS_INSECURE and
OTEL_EXPORTER_OTLP_INSECURE env options to `otlploggrpc.Exporter`.

Now insecure option is fetched from env endpoint value
(OTEL_EXPORTER_OTLP_LOGS_ENDPOINT/OTEL_EXPORTER_OTLP_ENDPOINT).
According to
[spec](https://opentelemetry.io/docs/specs/otel/protocol/exporter/):
> Insecure: Whether to enable client transport security for the
exporter’s gRPC connection. This option only applies to OTLP/gRPC when
an endpoint is provided without the http or https scheme - OTLP/HTTP
always uses the scheme provided for the endpoint.

So with current behavior we have several problems:
- If default endpoint is used, we can't use insecure connection (with
setting OTEL_EXPORTER_OTLP_INSECURE).
- If endpoint provided with option without scheme (e.g. `WithEndpoint`)
we can't use insecure connection with env settings.
- If endpoint provided with env variable without scheme (e.g.
`//env.endpoint:8080/`) we can't use insecure connection.

This commit fixes this.

The same problem with `otlploghttp.Exporter`, and probably it should be
fixed there too.
I'm open to suggestions on how to fix the current behavior in a more
elegant way.

---------

Co-authored-by: Sam Xie <sam@samxie.me>
Co-authored-by: Damien Mathieu <42@dmathieu.com>
@XSAM XSAM added this to the v1.30.0 milestone Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
3 participants