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] TLS should only be used for 'wss' and 'https' #35283

Closed
dpaasman00 opened this issue Sep 18, 2024 · 1 comment · Fixed by #35363
Closed

[cmd/opampsupervisor] TLS should only be used for 'wss' and 'https' #35283

dpaasman00 opened this issue Sep 18, 2024 · 1 comment · Fixed by #35363
Labels
bug Something isn't working cmd/opampsupervisor needs triage New item requiring triage

Comments

@dpaasman00
Copy link
Contributor

Component(s)

cmd/opampsupervisor

What happened?

Description

When connecting to an OpAMP management server using ws or http protocols, server.tls.insecure needs to be configured as true. This should not be necessary since it should be implied with those protocols that it is insecure.

Steps to Reproduce

Run the supervisor configured to connect to an OpAMP management server using ws or http. My server config is below:

server:
  endpoint: ws://localhost:3001/v1/opamp
  headers:
    Authorization: "Secret-Key <secret-key>"

Expected Result

I expected the supervisor to connect successfully without error.

Actual Result

The supervisor fails with this message and enters a retry loop that continually fails with the same error.

2024-09-18T13:50:48.573-0400	ERROR	supervisor/supervisor.go:411	Failed to connect to the server	{"error": "tls: first record does not look like a TLS handshake"}

Only once the supervisor config is updated with server.tls.insecure: true does the supervisor manage to successfully connect to the server.

Collector version

main (present in current commit 292f291)

Environment information

Environment

OS: macOS Sequoia 15.0
Compiler: go 1.23

OpenTelemetry Collector configuration

server:
  endpoint: ws://localhost:3001/v1/opamp
  headers:
    Authorization: "Secret-Key <secret key>"

capabilities:
  accepts_remote_config: true
  reports_remote_config: true

agent:
  executable: ./dist/collector_darwin_arm64

storage:
  directory: "./local/supervisor_storage"

Log output

2024-09-18T13:50:48.573-0400	ERROR	supervisor/supervisor.go:411	Failed to connect to the server	{"error": "tls: first record does not look like a TLS handshake"}
2024-09-18T13:50:48.573-0400	ERROR	supervisor/logger.go:26	Connection failed (tls: first record does not look like a TLS handshake), will retry.
2024-09-18T13:50:48.978-0400	ERROR	supervisor/supervisor.go:411	Failed to connect to the server	{"error": "tls: first record does not look like a TLS handshake"}
2024-09-18T13:50:48.979-0400	ERROR	supervisor/logger.go:26	Connection failed (tls: first record does not look like a TLS handshake), will retry.


This continues until I stop the process.

Additional context

I believe the relevant code is here. LoadTLSConfig() is always called when creating the OpAMP client and it does not consider the protocol being used. I have a fix in progress that will skip calling LoadTLSConfig() if we're using ws or http.

@dpaasman00 dpaasman00 added bug Something isn't working needs triage New item requiring triage labels Sep 18, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

jriguera pushed a commit to springernature/opentelemetry-collector-contrib that referenced this issue 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.>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cmd/opampsupervisor needs triage New item requiring triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant