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

[exporter/signalfxexporter] allow user to add their own CA to the cert pool #16250

Merged
merged 3 commits into from
Nov 18, 2022

Conversation

dloucasfx
Copy link
Contributor

Signed-off-by: Dani Louca dlouca@splunk.com

Description:
Adding new feature to allow the user to add their own CA to the ingest and api client cert pool.
This is needed when the exporter is pointing to a TLS enabled signalfx receiver or/and TLS enabled http_forwarder
and the CA is not in the system cert pool (ex: agent <--> GW setup)

Testing:
Unit test and manual validation

Documentation:

ingest_ca_file and api_url can be used to set the absolute path to the CA file.
This is needed when the exporter is pointing to a TLS enabled signalfx receiver or/and TLS enabled http_forwarder
and the CA is not in the system cert pool

@dmitryax
Copy link
Member

@dloucasfx
Copy link
Contributor Author

Can we apply https://github.com/open-telemetry/opentelemetry-collector/blob/main/config/configtls/README.md instead?

yes, I can make that change.
One thing reading the configtls code, if I ended up using LoadTLSConfig the code will not append the CA cert to the system cert pool, I guess this is fine as the client is only used to talk to this one endpoint, do you see any other reasons that we might need the system certs?

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

few nits, otherwise LGTM

exporter/signalfxexporter/exporter_test.go Outdated Show resolved Hide resolved
exporter/signalfxexporter/exporter_test.go Outdated Show resolved Hide resolved
exporter/signalfxexporter/README.md Outdated Show resolved Hide resolved
@dmitryax
Copy link
Member

@dloucasfx please take a look at the failing tests

@dloucasfx
Copy link
Contributor Author

dloucasfx commented Nov 18, 2022

@dloucasfx please take a look at the failing tests

It's a difference in error message between golang 1.19 (the version I developed with) and 1.18 where the CI is failing.

1.18 error:

error making HTTP request to https://127.0.0.1:45393/v2/dimension/key/id/_/sfxagent: Patch \"https://127.0.0.1:45393/v2/dimension/key/id/_/sfxagent\": x509: certificate signed by unknown authority

1.19 error:

error making HTTP request to https://127.0.0.1:65128/v2/dimension/key/id/_/sfxagent: Patch "https://127.0.0.1:65128/v2/dimension/key/id/_/sfxagent": x509: “OpenTelemetry Collector” certificate is not trusted

I used regex instead of contains and this error making HTTP request.*x509 should work fine on both... test updated.

Signed-off-by: Dani Louca <dlouca@splunk.com>
exporter/signalfxexporter/README.md Outdated Show resolved Hide resolved
exporter/signalfxexporter/README.md Outdated Show resolved Hide resolved
dloucasfx and others added 2 commits November 18, 2022 00:03
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label Nov 18, 2022
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Looks like github-action re-requested a review from you @dmitryax, can you confirm this is still good? I'm not sure what triggered the action after your approval 🤔

@dmitryax
Copy link
Member

dmitryax commented Nov 18, 2022

Maybe because of additional commits after that. It's good to merge

@dmitryax dmitryax merged commit 40e7b46 into open-telemetry:main Nov 18, 2022
JaredTan95 pushed a commit to openinsight-proj/opentelemetry-collector-contrib that referenced this pull request Nov 21, 2022
…t pool (open-telemetry#16250)

Adding new feature to allow the user to add their own CA to the ingest and api client cert pool.
This is needed when the exporter is pointing to a TLS enabled signalfx receiver or/and TLS enabled http_forwarder
and the CA is not in the system cert pool (ex: agent <--> GW setup)

Signed-off-by: Dani Louca <dlouca@splunk.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
shalper2 pushed a commit to shalper2/opentelemetry-collector-contrib that referenced this pull request Dec 6, 2022
…t pool (open-telemetry#16250)

Adding new feature to allow the user to add their own CA to the ingest and api client cert pool.
This is needed when the exporter is pointing to a TLS enabled signalfx receiver or/and TLS enabled http_forwarder
and the CA is not in the system cert pool (ex: agent <--> GW setup)

Signed-off-by: Dani Louca <dlouca@splunk.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
@plantfansam plantfansam mentioned this pull request Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/signalfx ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants