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

Default TLS settings for otlpexporter #2601

Conversation

alanwest
Copy link
Member

@alanwest alanwest commented Mar 3, 2021

Description:

This PR seeks to address some issues with the configuration of the otlpexporter. The motivation for this PR is two-fold.

1. Default TLS configuration for https

As it stands, in order to use TLS over gRPC, one must specify TLS Settings requiring a cert_file and key_file. This PR proposes that the TLS settings default to using system certificates when the scheme of the endpoint configuration is https. This has also been proposed by @vmihailenco in the opentelemetry-go project: open-telemetry/opentelemetry-go#1584.

Update: Turns out this is not true. You do not need to declare a cert_file/key_file. This was my misunderstanding. Communication over https just work. However, I'm still interested in hearing people's thoughts regarding my proposal below for aligning the configuration with the spec and requiring a scheme in the endpoint configuration.

2. Align otlpexporter endpoint configuration with the specification

From the OTLP exporter specification regarding endpoint configuration:

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.

Currently, as I noted in #2539, including a scheme in the endpoint configuration for the otlpexporter causes the error too many colons in address upon export. This PR aligns the otlpexporter with the specification by requiring that endpoint be a valid URL. My understanding is the spec requires this irrespective of what protocol is used - i.e., http or grpc. It appears the the otlphttpexporter already requires a valid URL.

Question If a scheme is not included in the endpoint configuration an error occurs but only upon export. Does it make sense to:

  1. error upon start up?, or
  2. continue to support configuring the endpoint without a scheme (contrary to the specification)?

Link to tracking Issue:

#2539

Testing:

TODO - I will complete test coverage if the direction of this PR is acceptable.

Documentation:

TODO - The TLS Configuration Settings documentation will need to be updated.

@alanwest alanwest changed the title Alanwest/otlp exporter default tls settings Default TLS settings for otlpexporter Mar 3, 2021
@tigrannajaryan
Copy link
Member

From the OTLP exporter specification regarding endpoint configuration:

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.

This paragraph in the spec is incorrectly worded. URL is non intended to be overridable in OTLP/gRPC. I don't think any implementation does and the protocol specification does not say that it is allowed or possible. The URL is only overridable in OTLP/HTTP. This is the source of truth for the protocol specification: https://github.com/open-telemetry/opentelemetry-specification/blob/1afab39e5658f807315abf2f3256809293bfd421/specification/protocol/otlp.md and it only

Also, the page that you are quoting does not apply to Collector, it applies to language libraries only. We should probably called that out explicitly.

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 13, 2021
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Mar 20, 2021
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants