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

fix(grpc-exporter): use non-normalized URL to determine channel security #3019

Merged

Conversation

pichlermarc
Copy link
Member

@pichlermarc pichlermarc commented Jun 3, 2022

Which problem is this PR solving?

In #2675, @mplachter brought up that the gRPC exporter is currently not working as intended. Investigating this bug revealed that the collector.url passed to configureSecurity() was already stripped of its protocol (http:// or https://) which is used to in configureSecurity() to determine the security mode (implemented in #2827).

This caused all gRPC exporters configured with URLs containing http:// to be configured with a secure gRPC channel, and therefore did not work with the (by default insecure) collector gRPC receiver.

This PR fixes this issue by passing a URL that still contains the protocol to configureSecurity()

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Unit tests
  • Manual testing using the otlp-exporter-node example

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@codecov
Copy link

codecov bot commented Jun 3, 2022

Codecov Report

Merging #3019 (991888f) into main (1f742e4) will decrease coverage by 1.54%.
The diff coverage is n/a.

❗ Current head 991888f differs from pull request most recent head 743aa7c. Consider uploading reports for the commit 743aa7c to get more accurate results

@@            Coverage Diff             @@
##             main    #3019      +/-   ##
==========================================
- Coverage   92.78%   91.24%   -1.55%     
==========================================
  Files         185       68     -117     
  Lines        6142     1850    -4292     
  Branches     1300      392     -908     
==========================================
- Hits         5699     1688    -4011     
+ Misses        443      162     -281     
Impacted Files Coverage Δ
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...ckages/opentelemetry-sdk-trace-web/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 79.41% <0.00%> (-17.65%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
... and 117 more

Copy link
Contributor

@seemk seemk left a comment

Choose a reason for hiding this comment

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

Thanks! Just stumbled upon this

@mplachter
Copy link

@pichlermarc thanks for working on this quickly. When can this be merged and released been waiting on a useable release with an updated proto for a little while now :)

@pichlermarc
Copy link
Member Author

@mplachter sorry, it did not make it into 0.29.2. I'll sync with the maintainers to see if we can cut another release soon.

@pichlermarc
Copy link
Member Author

Failing tests are due to unrelated reasons, waiting on #3024 to merge so that the tests go green again.

@dyladan
Copy link
Member

dyladan commented Jun 8, 2022

Would like to see a review from @svetlanabrennan to ensure we're not missing something since she wrote the original security pr

@svetlanabrennan
Copy link
Contributor

Sorry for the delayed review. Looks good to me.

@mplachter
Copy link

@dyladan if we could get this merged and released I would really appreciate it.

Thank you team for actioning on this :)

@dyladan dyladan merged commit 0ed21bc into open-telemetry:main Jun 8, 2022
@dyladan dyladan deleted the fix-grpc-security branch June 8, 2022 14:38
@matschaffer
Copy link
Contributor

Yay! I did this to workaround the usage of the normalized URL, but it'd be great to eliminate that code.

https://github.com/elastic/kibana/pull/133171/files#diff-cc07ce777d17b9fafe6933322c29cfa1cdefb7f0049d74467113ce620e84e019R112-R124

@pfdgithub
Copy link

Any release plan?

@pichlermarc
Copy link
Member Author

We'll need to do a feature release. I expect this to come around this week. Sorry for the delay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants