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

NH-23785 Bundle public AO certificate for liboboe Reporter #70

Merged
merged 5 commits into from
Nov 7, 2022

Conversation

tammy-baylis-swi
Copy link
Contributor

@tammy-baylis-swi tammy-baylis-swi commented Nov 4, 2022

This bumps Python APM library's liboboe version to 11.0.0 and inits the Reporter with a string for certificates. It no longer inits with trusted_path, is_grpc_clean_hack_enabled, nor w3c_trace_format.

As suggested at a standup a little while back, the public cert has been hard-coded as a constant in solarwinds_apm and is now part of the installed package. I'll be adjusting the integration tests to use this too if approved.

I've tested this manually with the different behaviours recorded here: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3309503060/2022-11-04+Reporter+init+with+certificate+string. In two sentences: pointing at NH collector is the same as before, while Pointing to AO prod/staging uses either the packaged cert by default OR uses what's at SW_APM_TRUSTEDPATH if there's a file there. Pointing to any other collector (e.g. NH/SWO) does the same except the default is empty string.

Please let me know as always if any suggestions! 🙂

@tammy-baylis-swi
Copy link
Contributor Author

Tests failing on GH, boo! Fix coming.

@tammy-baylis-swi tammy-baylis-swi marked this pull request as draft November 4, 2022 22:07
@tammy-baylis-swi tammy-baylis-swi marked this pull request as ready for review November 4, 2022 22:23
@tammy-baylis-swi tammy-baylis-swi marked this pull request as draft November 4, 2022 22:24
@tammy-baylis-swi tammy-baylis-swi marked this pull request as ready for review November 4, 2022 22:52
@tammy-baylis-swi
Copy link
Contributor Author

Fixed the GH test runs in 7c138aa. Ready for review!

@tammy-baylis-swi
Copy link
Contributor Author

tammy-baylis-swi commented Nov 4, 2022

I just realized that AO staging might need a different cert than AO prod -- @cheempz is that correct? Also where might I get the staging cert?

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LOL there must be something very confusing with our trustedpath/certificate handling logic, every implementation needed a tweak ;)

We actually want to keep supporting SW_APM_TRUSTEDPATH handling even for SWO. So the logic should be:

If SW_APM_TRUSTEDPATH is set, we should attempt to load the content of the given file and pass it to liboboe as the certificate. This should be regardless what the collector endpoint is.

If SW_APM_TRUSTEDPATH is not set AND the collector endpoint is AO, we should default to the bundled AO cert.

@cheempz
Copy link
Contributor

cheempz commented Nov 4, 2022

I just realized that AO staging might need a different cert than AO prod -- @cheempz is that correct? Also where might I get the staging cert?

Ah, the AO staging cert is thankfully signed by the same private CA that signs for AO prod, so using the same bundled AO CA cert is all good there -- and in fact I've been using your awesome fastapi books app to send test data to AO staging.

@tammy-baylis-swi
Copy link
Contributor Author

LOL there must be something very confusing with our trustedpath/certificate handling logic, every implementation needed a tweak ;)

We actually want to keep supporting SW_APM_TRUSTEDPATH handling even for SWO. So the logic should be:

If SW_APM_TRUSTEDPATH is set, we should attempt to load the content of the given file and pass it to liboboe as the certificate. This should be regardless what the collector endpoint is.

If SW_APM_TRUSTEDPATH is not set AND the collector endpoint is AO, we should default to the bundled AO cert.

Thank you @cheempz ! Addressed in 779821e and 3a2e979, with updated outcomes in confluence: https://swicloud.atlassian.net/wiki/spaces/NIT/pages/3309503060/2022-11-04+Reporter+init+with+certificate+string

Please let me know if anything else!

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the revisit and tests @tammy-baylis-swi!

@tammy-baylis-swi tammy-baylis-swi merged commit 6355790 into main Nov 7, 2022
@tammy-baylis-swi tammy-baylis-swi deleted the NH-23785-bundle-certificate-for-reporter branch November 7, 2022 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants