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

[cmd/opampsupervisor] Conditionally use TLS config #35363

Merged

Conversation

dpaasman00
Copy link
Contributor

@dpaasman00 dpaasman00 commented Sep 23, 2024

Description:

Fixes an issue where TLS would be used despite the opamp server using ws or http protocols.

Before a TLS config would always get created, causing the connection to always use TLS settings. This change first checks which protocol we're using before creating a TLS config.

Link to tracking Issue: Fixes #35283

Testing:
Removed tls.insecure_skip_verify: true from e2e test configs which were using ws protocol since they are no longer needed.

Documentation:

@dpaasman00 dpaasman00 force-pushed the fix-opampsupervisor-tls-settings branch from d5741d0 to 9635840 Compare September 23, 2024 15:47
@dpaasman00 dpaasman00 marked this pull request as ready for review September 23, 2024 15:48
@dpaasman00 dpaasman00 requested a review from a team as a code owner September 23, 2024 15:48
Copy link
Contributor

@BinaryFissionGames BinaryFissionGames left a comment

Choose a reason for hiding this comment

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

This is working well for me (tested both ws and wss against BPOP), seems good after Florian's comment is addressed.

@dpaasman00 dpaasman00 requested a review from bacherfl September 26, 2024 19:07
@BinaryFissionGames
Copy link
Contributor

@tigrannajaryan PTAL when you get a chance

Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thanks @dpaasman00!

@evan-bradley evan-bradley merged commit 2e0d4d6 into open-telemetry:main Oct 2, 2024
156 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 2, 2024
@dpaasman00 dpaasman00 deleted the fix-opampsupervisor-tls-settings branch October 2, 2024 19:00
jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this pull request Oct 4, 2024
)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
Fixes an issue where TLS would be used despite the opamp server using
`ws` or `http` protocols.

Before a TLS config would always get created, causing the connection to
always use TLS settings. This change first checks which protocol we're
using before creating a TLS config.

**Link to tracking Issue:** <Issue number if applicable> Fixes open-telemetry#35283 

**Testing:** <Describe what testing was performed and which tests were
added.>
Removed `tls.insecure_skip_verify: true` from e2e test configs which
were using `ws` protocol since they are no longer needed.

**Documentation:** <Describe the documentation added.>
@priyeshsingh550
Copy link

@dpaasman00 Is this addressed i am still getting the same tls handshake error while using ws protocol

2024/10/24 07:23:38 Failed to connect to the server: tls: first record does not look like a TLS handshake
2024/10/24 07:23:38 Connection failed (tls: first record does not look like a TLS handshake), will retry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cmd/opampsupervisor] TLS should only be used for 'wss' and 'https'
7 participants