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

Add variables to support TLS version and cipher in the OTLP exporter #3088

Closed
wants to merge 10 commits into from

Conversation

marcalff
Copy link
Member

@marcalff marcalff commented Jan 9, 2023

Fixes #2932

Changes

Add environment variables to support:

  • min TLS version
  • max TLS version
  • TLS cipher list (for TLS <= 1.2)
  • TLS cipher suite (for TLS 1.3)

in the OTLP EXPORTER configuration.

@marcalff marcalff requested review from a team January 9, 2023 22:34
@marcalff marcalff changed the title OTLP EXPORTER TLS #2932 Add environment variables to support TLS in the OTLP exporter #2932 Jan 10, 2023
@arminru arminru added area:sdk Related to the SDK area:configuration Related to configuring the SDK labels Jan 12, 2023
@marcalff marcalff changed the title Add environment variables to support TLS in the OTLP exporter #2932 Add variables to support TLS version and cipher in the OTLP exporter Jan 12, 2023
@marcalff
Copy link
Member Author

To @open-telemetry/specs-approvers ,

Any comments on this PR ?

I am not even sure if it was noticed at all.

Regards.

@marcalff
Copy link
Member Author

Blocked by:

#2891 (comment)

@github-actions
Copy link

github-actions bot commented Feb 9, 2023

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 Feb 9, 2023
@github-actions
Copy link

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

@github-actions github-actions bot closed this Feb 17, 2023
@marcalff
Copy link
Member Author

So, this PR was closed automatically by a bot, without any human reviewer actually looking at it.

The contribution process is severely flawed in my opinion.

The author is not to blame for lack of activity here.

@yurishkuro
Copy link
Member

@marcalff I understand it can be upsetting when a PR is not being merged, but as a project we have priorities and limited resources, which are directed at things that people find important. It does not look like the feature requested in the ticket is in high demand, or supported by any SDKs / collector.

@marcalff
Copy link
Member Author

@marcalff I understand it can be upsetting when a PR is not being merged, but as a project we have priorities and limited resources, which are directed at things that people find important. It does not look like the feature requested in the ticket is in high demand, or supported by any SDKs / collector.

@yurishkuro Thanks for taking the time to comment.

I know full well people have limited resources, facing the same issues in opentelemetry-cpp.

The friction here is not so much about the PR being merged or not, but about the lack of any comments, even negative.

Had anyone asked, this feature is supported in the opentelemetry-cpp SDK, per PR open-telemetry/opentelemetry-cpp#1793, which will not be merged then, since the spec is rejected.

@marcalff
Copy link
Member Author

/reopen

@tigrannajaryan
Copy link
Member

@marcalff has anything changed since this was closed? I can reopen but it won't be of much use if no-one reviews/comments on it and it is closed again. To gain some traction I think it is best to find some other people who are interested in this and ask them to review and comment (ideally it would be spec approvers, but can also be any other Otel members). Also advertise in the specification SIG meeting and maintainers' meeting, in CNCF Slack channels and with any other relevant audiences.

@marcalff
Copy link
Member Author

@tigrannajaryan Thanks for taking the time to comment.

Nothing has changed indeed.

@svrnm
Copy link
Member

svrnm commented Jun 14, 2024

It looks like the underlying issue and by that this PR became important once again based on this discussion by @jpkrohling

- **TLS minimum version**: Minimum TLS version to use with SSL.
Should only be used for a secure connection.
See [TLS version](./exporter.md#tls-version) for more details.
- Default: n/a
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this should be set to 1.3 (if supported?)

@marcalff
Copy link
Member Author

It looks like the underlying issue and by that this PR became important once again based on this discussion by @jpkrohling

This is actually the discussion that made me look at this old PR again.

I think that at some point, and in particular to graduate opentelemetry in general, we will need to cover security much more extensively that we do today.

This assessment however, does not seem to be widely accepted yet.

@trask
Copy link
Member

trask commented Jun 14, 2024

hi @marcalff! are you able to join the Specification SIG meeting and drive some discussion around this PR? it's not uncommon for specification issues or PRs to go stale or be overlooked, and sometimes (🤞) all it takes is 10 minutes of synchronous discussion in the Specification SIG meeting to unblock or gather interest in it

@marcalff
Copy link
Member Author

Hi @trask

From what I could find, the spec SIG meeting is at: Tue at 8:00am Pacific Time

I will attend the meeting on Tuesday June 25, 2024 then (unable to attend next week).

@trask
Copy link
Member

trask commented Jun 14, 2024

Tuesday June 25 is OpenTelemetry Community Day, attendance will probably be very light (or canceled), so I'd recommend joining a future meeting instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK area:sdk Related to the SDK Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need new options to specify minimum TLS version and allowed ciphers list
7 participants