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.

Sign up for GitHub

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

Split out OTLP gRPC vs. HTTP endpoint configuration #1729

Merged
merged 13 commits into from
Jul 20, 2021
Merged

Split out OTLP gRPC vs. HTTP endpoint configuration #1729

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
1f4b672
Split out OTLP gRPC vs. HTTP endpoint configuration
alanwest May 27, 2021
927f066
Update CHANGELOG.md
alanwest Jun 1, 2021
cb5d942
Merge remote-tracking branch 'upstream/main' into alanwest/otlp-endpo…
alanwest Jun 7, 2021
e3b5752
Require implementation of insecure setting only if endpoint may have …
alanwest Jun 7, 2021
08b9cea
Merge branch 'alanwest/otlp-endpoint-configuration' of github.com:ala…
alanwest Jun 7, 2021
6d9287d
Update specification/protocol/exporter.md
alanwest Jun 29, 2021
ecabd55
Clarify handling of schemes in gRPC endpoint configuration
alanwest Jun 29, 2021
3ca03ae
Clarify the insecure OTLP/gRPC configuration option
alanwest Jun 30, 2021
0abb3f2
Merge branch 'main' into alanwest/otlp-endpoint-configuration
alanwest Jun 30, 2021
adc812c
Update changelog to reflect changes from PR feedback
alanwest Jun 30, 2021
3e0c5ec
Merge branch 'main' into alanwest/otlp-endpoint-configuration
alanwest Jul 8, 2021
6ffff19
Merge branch 'main' into alanwest/otlp-endpoint-configuration
carlosalberto Jul 14, 2021
3fed1f1
Merge branch 'main' into alanwest/otlp-endpoint-configuration
carlosalberto Jul 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion specification/protocol/exporter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ The following configuration options MUST be available to configure the OTLP expo

| Configuration Option | Description | Default | Env variable |
| -------------------- | ------------------------------------------------------------ | ----------------- | ------------------------------------------------------------ |
| Endpoint | Target to which the exporter is going to send spans or metrics. The endpoint MUST be a valid URL with scheme (http or https) and host, and MAY contain a port and path. A scheme of https indicates a secure connection. When using `OTEL_EXPORTER_OTLP_ENDPOINT` with OTLP/HTTP, exporters SHOULD follow the collector convention of appending the version and signal to the path (e.g. `v1/traces` or `v1/metrics`). The per-signal endpoint configuration options take precedence and can be used to override this behavior. See the [OTLP Specification][otlphttp-req] for more details. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` |
| Endpoint (OTLP/HTTP) | Target to which the exporter is going to send spans or metrics. The endpoint MUST be a valid URL with scheme (http or https) and host, and MAY contain a port and path. A scheme of https indicates a secure connection. When using `OTEL_EXPORTER_OTLP_ENDPOINT`, exporters SHOULD follow the collector convention of appending the version and signal to the path (e.g. `v1/traces` or `v1/metrics`). The per-signal endpoint configuration options take precedence and can be used to override this behavior. See the [OTLP Specification][otlphttp-req] for more details. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` |
alanwest marked this conversation as resolved.
Show resolved Hide resolved
| Endpoint (OTLP/gRPC) | Target to which the exporter is going to send spans or metrics. The endpoint MUST accept a URL with or without a scheme. If it includes a scheme (http or https), a scheme of https indicates a secure connection. | `https://localhost:4317` | `OTEL_EXPORTER_OTLP_ENDPOINT` `OTEL_EXPORTER_OTLP_TRACES_ENDPOINT` `OTEL_EXPORTER_OTLP_METRICS_ENDPOINT` |
| Insecure | Whether to enable client transport security for the exporter's `grpc` connection. Only applies to OTLP/gRPC when a scheme is not included in the `endpoint`. OTLP/HTTP always uses the scheme provided for the endpoint. | `false` | `OTEL_EXPORTER_OTLP_INSECURE` `OTEL_EXPORTER_OTLP_SPAN_INSECURE` `OTEL_EXPORTER_OTLP_METRIC_INSECURE` |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean SDKs all have to support this variable? I'd prefer not to support it - we already have a method for picking secure / insecure in the endpoint scheme, having another way of doing the same thing is extra documentation / support overhead (not to mention bringing back this problematic word insecure which security scanners don't like much). If the idea is to allow either (no-scheme url + insecure flag) or (scheme url) than that's ok but don't see that mentioned in this wording.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another point is that it's always been quite mysterious when host:port defaults to TLS - I don't think that matches most users' expectations, it's just a security push by us I think. The schemed URLs solved this problem nicely, if allowing non-schemed URLs I really recommend the default being insecure, I think it matches most users' expectations (even curl google.com will hit HTTP, not HTTPS, this is what people think IMO).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mean SDKs all have to support this variable?

Yes, my thought was that SDKs and collector would support the insecure setting.

I'd prefer not to support it - we already have a method for picking secure / insecure in the endpoint scheme, having another way of doing the same thing is extra documentation / support overhead (not to mention bringing back this problematic word insecure which security scanners don't like much).

I agree supporting it is not a thrilling idea. Coming from .NET, the client we're using for the OTLP exporter requires a scheme anyways. Though my understanding is that quite a few gRPC client libraries for other languages support declaring an endpoint without a scheme and that they default to using the dns scheme and a secure channel is the default.

The insecure wording seems to be inspired by gRPC libraries themselves - here are some examples for setting up an insecure channel in various languages.

If the idea is to allow either (no-scheme url + insecure flag) or (scheme url) than that's ok but don't see that mentioned in this wording.

This is the idea. I attempted to capture that with my wording:

  • "Only applies to OTLP/gRPC when a scheme is not included in the endpoint."

Another point is that it's always been quite mysterious when host:port defaults to TLS - I don't think that matches most users' expectations, it's just a security push by us I think.

I'm not an expert, though I'm not entirely certain that it was just a security push on the part of OpenTelemetry. My read of the examples I linked to above seems to suggest that secure channels are considered the default by most gRPC libraries.

All this said, I had also hoped that scheme alone could solve this issue, but it seems that declaring a URI without a scheme is common with many libraries. In speaking with the collector SIG they would like to maintain the ability to configure without a scheme. Go SDK also still supports the insecure config setting.

So I guess it's a question of how important it is we drive consistency across the language SDKs. The inconsistency has been a point of confusion to some.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like to maintain the ability to configure without a scheme. Go SDK also still supports the insecure config setting

Do they want to configure without scheme but still use TLS?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it's required for collector and SDK to be consistent here as I think these variables are mostly for SDK.

SDKs will always have extra knobs compared to what's in the spec. Usually they're language specific but it doesn't seem terrible if one SDK supports insecure flag if they want to. For Java though we've had no user issues related to this flag and it seems to work well, so I'm thinking don't fix what isn't broken. I can't see any chance of improvement by requiring support of the new flag but a chance of more user confusion when having multiple flags.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do they want to configure without scheme but still use TLS?

Correct, TLS is the default in Go.

Go looks something like this

        driver := otlpgrpc.NewDriver(
		otlpgrpc.WithInsecure(), // This option is required if you do not want TLS otherwise TLS is the default
		otlpgrpc.WithEndpoint("localhost:4317"), // To my knowledge, including a scheme here does not work
	)
	exp, err := otlp.NewExporter(ctx, driver)

On the other hand, Go does support a scheme when configuration is done via the OTEL_EXPORTER_OTLP_ENDPOINT environment variable. The scheme http implies the WithInsecure option.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SDKs will always have extra knobs compared to what's in the spec. Usually they're language specific but it doesn't seem terrible if one SDK supports insecure flag if they want to.

This stance seems reasonable.

Perhaps at a minimum do you think it makes sense to say for OTLP/gRPC:

  1. SDKs MUST support endpoint with configured with a scheme and the scheme https indicates use TLS.
  2. SDKs MAY support an endpoint without a scheme but if they do then an additional insecure configuration is required.

First point effectively maintains status quo. Second point enables the flexibility for SDKs and requires a level of consistency when adopting this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup I have no problem with text related to an insecure flag if it's optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've clarified things so that implementing the insecure flag is optional if the SDK does not allow for an endpoint to be configured without a scheme.

| Certificate File | Path to certificate file for TLS credentials of gRPC client. Should only be used for a secure connection. | n/a | `OTEL_EXPORTER_OTLP_CERTIFICATE` `OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE` `OTEL_EXPORTER_OTLP_METRICS_CERTIFICATE` |
| Headers | Key-value pairs to be used as headers associated with gRPC or HTTP requests. See [Specifying headers](./exporter.md#specifying-headers-via-environment-variables) for more details. | n/a | `OTEL_EXPORTER_OTLP_HEADERS` `OTEL_EXPORTER_OTLP_TRACES_HEADERS` `OTEL_EXPORTER_OTLP_METRICS_HEADERS` |
| Compression | Compression key for supported compression types. Supported compression: `gzip`| No value | `OTEL_EXPORTER_OTLP_COMPRESSION` `OTEL_EXPORTER_OTLP_TRACES_COMPRESSION` `OTEL_EXPORTER_OTLP_METRICS_COMPRESSION` |
Expand Down