Skip to content

Commit 3fafb10

Browse files
johannaojelingFiery-Fenix
authored andcommitted
[extension/opamp] Load TLS config only for wss and https endpoints (open-telemetry#39516)
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description Loads TLS config for the OpAMP Extension's OpAMP agent only if the `server::ws::endpoint`/`server::http::endpoint` config is set to a URL with scheme `wss`/`https`. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Fixes open-telemetry#39515 <!--Describe what testing was performed and which tests were added.--> #### Testing - Added unit tests - Tested manually with the steps described in **Steps to Reproduce** in open-telemetry#39515 <!--Please delete paragraphs that you did not use before submitting.-->
1 parent 3868d42 commit 3fafb10

File tree

4 files changed

+108
-3
lines changed

4 files changed

+108
-3
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
# Use this changelog template to create an entry for release notes.
2+
3+
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
4+
change_type: bug_fix
5+
6+
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
7+
component: opampextension
8+
9+
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
10+
note: Skips loading TLS config for insecure endpoints
11+
12+
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
13+
issues: [39515]
14+
15+
# (Optional) One or more lines of additional information to render under the primary note.
16+
# These lines will be padded with 2 spaces and then inserted directly into the document.
17+
# Use pipe (|) for multiline entries.
18+
subtext:
19+
20+
# If your change doesn't affect end users or the exported elements of any package,
21+
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
22+
# Optional: The change log or logs in which this entry should be included.
23+
# e.g. '[user]' or '[user, api]'
24+
# Include 'user' if the change is relevant to end users.
25+
# Include 'api' if there is a change to a library API.
26+
# Default: '[user]'
27+
change_logs: []

extension/opampextension/config.go

+18-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44
package opampextension // import "github.com/open-telemetry/opentelemetry-collector-contrib/extension/opampextension"
55

66
import (
7+
"context"
8+
"crypto/tls"
79
"errors"
10+
"fmt"
811
"net/url"
912
"time"
1013

@@ -146,7 +149,21 @@ func (s OpAMPServer) GetHeaders() map[string]configopaque.String {
146149
return map[string]configopaque.String{}
147150
}
148151

149-
func (s OpAMPServer) GetTLSSetting() configtls.ClientConfig {
152+
// GetTLSConfig returns a TLS config if the endpoint is secure (wss or https)
153+
func (s OpAMPServer) GetTLSConfig(ctx context.Context) (*tls.Config, error) {
154+
parsedURL, err := url.Parse(s.GetEndpoint())
155+
if err != nil {
156+
return nil, fmt.Errorf("parse server endpoint: %w", err)
157+
}
158+
159+
if parsedURL.Scheme != "wss" && parsedURL.Scheme != "https" {
160+
return nil, nil
161+
}
162+
163+
return s.getTLSSetting().LoadTLSConfig(ctx)
164+
}
165+
166+
func (s OpAMPServer) getTLSSetting() configtls.ClientConfig {
150167
if s.WS != nil {
151168
return s.WS.TLSSetting
152169
} else if s.HTTP != nil {

extension/opampextension/config_test.go

+62-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package opampextension
55

66
import (
7+
"context"
78
"path/filepath"
89
"testing"
910
"time"
@@ -144,12 +145,72 @@ func TestConfig_Getters(t *testing.T) {
144145
for _, tt := range tests {
145146
t.Run(tt.name, func(t *testing.T) {
146147
tt.expected.headers(t, tt.fields.Server.GetHeaders())
147-
tt.expected.tls(t, tt.fields.Server.GetTLSSetting())
148+
tt.expected.tls(t, tt.fields.Server.getTLSSetting())
148149
tt.expected.endpoint(t, tt.fields.Server.GetEndpoint())
149150
})
150151
}
151152
}
152153

154+
func TestOpAMPServer_GetTLSConfig(t *testing.T) {
155+
tests := []struct {
156+
name string
157+
server OpAMPServer
158+
expectedTLSConfig assert.ValueAssertionFunc
159+
}{
160+
{
161+
name: "wss endpoint",
162+
server: OpAMPServer{
163+
WS: &commonFields{
164+
Endpoint: "wss://example.com",
165+
TLSSetting: configtls.NewDefaultClientConfig(),
166+
},
167+
},
168+
expectedTLSConfig: assert.NotNil,
169+
},
170+
{
171+
name: "https endpoint",
172+
server: OpAMPServer{
173+
HTTP: &httpFields{
174+
commonFields: commonFields{
175+
Endpoint: "https://example.com",
176+
TLSSetting: configtls.NewDefaultClientConfig(),
177+
},
178+
},
179+
},
180+
expectedTLSConfig: assert.NotNil,
181+
},
182+
{
183+
name: "ws endpoint",
184+
server: OpAMPServer{
185+
WS: &commonFields{
186+
Endpoint: "ws://example.com",
187+
},
188+
},
189+
expectedTLSConfig: assert.Nil,
190+
},
191+
{
192+
name: "http endpoint",
193+
server: OpAMPServer{
194+
HTTP: &httpFields{
195+
commonFields: commonFields{
196+
Endpoint: "http://example.com",
197+
},
198+
},
199+
},
200+
expectedTLSConfig: assert.Nil,
201+
},
202+
}
203+
204+
for _, tt := range tests {
205+
t.Run(tt.name, func(t *testing.T) {
206+
ctx := context.Background()
207+
tlsConfig, err := tt.server.GetTLSConfig(ctx)
208+
assert.NoError(t, err)
209+
tt.expectedTLSConfig(t, tlsConfig)
210+
})
211+
}
212+
}
213+
153214
func TestConfig_Validate(t *testing.T) {
154215
type fields struct {
155216
Server *OpAMPServer

extension/opampextension/opamp_agent.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ func (o *opampAgent) Start(ctx context.Context, host component.Host) error {
120120
header.Set(k, string(v))
121121
}
122122

123-
tls, err := o.cfg.Server.GetTLSSetting().LoadTLSConfig(ctx)
123+
tls, err := o.cfg.Server.GetTLSConfig(ctx)
124124
if err != nil {
125125
return err
126126
}

0 commit comments

Comments
 (0)