-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 TLS for SAPM and SignalFx receiver #215
Add TLS for SAPM and SignalFx receiver #215
Conversation
cc @brianashby-sfx for doc changes. |
@ccaraman I think we should share the config from core, see: Do you think that is possible? |
We were also trying to do this in #198 |
Yes, that is totally possible. I didn't see that PR so I didn't know that was coming in the pipeline. I'll take a look @jrcamp - should we then wait on #198 and get the first mentioned pr to work for HTTP as well? |
I think the upstream issue linked above will supersede most of #198. (Though we're doing some other common config structs for HTTP that will be built on top of TLSConfig but shouldn't block your work here.) |
@bogdandrutu and @jrcamp - since this is using the common receiver settings (for http server) - i don't think this blocks this pr getting reviewed. There is testing logic around https connection so that can be reviewed before the refactor of the tls configuration. |
50b8b63
to
9e38d61
Compare
Codecov Report
@@ Coverage Diff @@
## master #215 +/- ##
==========================================
+ Coverage 78.51% 78.54% +0.03%
==========================================
Files 153 153
Lines 7567 7578 +11
==========================================
+ Hits 5941 5952 +11
Misses 1295 1295
Partials 331 331
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm just a couple suggestions
resp, err = sendSapm(tt.args.config.Endpoint, tt.args.sapm, tt.args.zipped) | ||
assert.NoError(t, err, fmt.Sprintf("should not have failed when sending sapm %v", resp)) | ||
resp, err = sendSapm(tt.args.config.Endpoint, tt.args.sapm, tt.args.zipped, tt.args.useTLS) | ||
require.NoError(t, err, fmt.Sprintf("should not have failed when sending sapm %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NoError(t, err, fmt.Sprintf("should not have failed when sending sapm %v", err)) | |
require.NoErrorf(t, err, "should not have failed when sending sapm %v", err) |
} | ||
|
||
resp, err := client.Do(req) | ||
require.NoError(t, err, fmt.Sprintf("should not have failed when sending to signalFx receiver %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NoError(t, err, fmt.Sprintf("should not have failed when sending to signalFx receiver %v", err)) | |
require.NoErrorf(t, err, "should not have failed when sending to signalFx receiver %v", err) |
url := fmt.Sprintf("https://%s/v2/datapoint", addr) | ||
|
||
req, err := http.NewRequest("POST", url, bytes.NewReader(body)) | ||
require.NoError(t, err, fmt.Sprintf("should have no errors with new request: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NoError(t, err, fmt.Sprintf("should have no errors with new request: %v", err)) | |
require.NoErrorf(t, err, "should have no errors with new request: %v", err) |
req.Header.Set("Content-Type", "application/x-protobuf") | ||
|
||
caCert, err := ioutil.ReadFile("./testdata/testcert.crt") | ||
require.NoError(t, err, fmt.Sprintf("failed to load certificate: %v", err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require.NoError(t, err, fmt.Sprintf("failed to load certificate: %v", err)) | |
require.NoErrorf(t, err, "failed to load certificate: %v", err) |
053a3f5
to
5cba3f8
Compare
Adds TLS for both SAPM and SignalFx Receiver. Testing: Added tests to create a TLS connection to the receiver and manually tested using Otel Collector and Smart Agent. Documentation: Expanded readme's to add information about TLS.
Description: Adds TLS for both SAPM and SignalFx Receiver.
Testing: Added tests to create a TLS connection to the receiver and manually tested using Otel Collector and Smart Agent.
Documentation: Expanded readme's to add information about TLS.