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.

Sign up for GitHub

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

Jump to bottom

Split OTLP receiver by protocols, allow mTLS support #1223

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Split OTLP receiver by protocols, allow mTLS support #1223

merged 1 commit into from
Jun 29, 2020

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Jun 28, 2020 •
edited
Loading

Description: <Describe what has changed.
The github.com/soheilhy/cmux does not support mTLS, so we have to split the protocols and avoid using any mux to allow support for mTLS.

Temporary remove the support for unix sockets, will be added back soon with a change to grpc server settings to support transport.

@jpkrohling jpkrohling mentioned this pull request Jun 29, 2020
Closed
@codecov
Copy link

codecov bot commented Jun 29, 2020 •
edited
Loading

Codecov Report

Merging #1223 into master will increase coverage by 0.06%.
The diff coverage is 90.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
+ Coverage   88.07%   88.13%   +0.06%     
==========================================
  Files         203      201       -2     
  Lines       14669    14621      -48     
==========================================
- Hits        12919    12886      -33     
+ Misses       1311     1303       -8     
+ Partials      439      432       -7
Impacted Files Coverage Δ
receiver/otlpreceiver/factory.go 84.84% <84.61%> (-10.16%) ⬇️
receiver/otlpreceiver/otlp.go 93.65% <92.59%> (+6.92%) ⬆️
receiver/otlpreceiver/metrics/otlp.go 100.00% <100.00%> (+27.27%) ⬆️
receiver/otlpreceiver/trace/otlp.go 100.00% <100.00%> (+7.40%) ⬆️
translator/internaldata/resource_to_oc.go 83.72% <0.00%> (+2.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4bfe18...22eb2ab. Read the comment docs.

tigrannajaryan
tigrannajaryan reviewed Jun 29, 2020
View reviewed changes
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Please also update otlpreceiver/README.md

receiver/otlpreceiver/testdata/config.yaml Show resolved Hide resolved
receiver/otlpreceiver/testdata/config.yaml
Comment on lines 54 to +72
# The following entry demonstrates how to specify a Unix Domain Socket for the server.
otlp/uds:
transport: unix
endpoint: /tmp/otlp.sock

protocols:
grpc:
# transport: unix
endpoint: /tmp/grpc_otlp.sock
http:
# transport: unix
endpoint: /tmp/http_otlp.sock
Copy link
Member

Choose a reason for hiding this comment

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

PR description says Unix socket is not supported. In that case it is better to remove this from the config.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am planning to add it back in the next couple of PRs, and want to just uncomment that when it happens.

receiver/otlpreceiver/testdata/config.yaml
min_time: 10s
permit_without_stream: true
protocols:
grpc:
Copy link
Member

Choose a reason for hiding this comment

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

Is grpc-gateway enabled by "grpc" protocol or by "http" protocol?

Copy link
Member Author

Choose a reason for hiding this comment

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

By http.

@bogdandrutu bogdandrutu force-pushed the otlpmtls branch 2 times, most recently from 16edce2 to c6367e7 Compare June 29, 2020 17:46
@bogdandrutu
Copy link
Member Author

Please also update otlpreceiver/README.md
Done.

@tigrannajaryan tigrannajaryan self-assigned this Jun 29, 2020
@bogdandrutu bogdandrutu merged commit 144127f into open-telemetry:master Jun 29, 2020
@bogdandrutu bogdandrutu deleted the otlpmtls branch June 29, 2020 20:39
This was referenced Jun 29, 2020
Closed
Closed
@jmacd
Copy link
Contributor

jmacd commented Jul 2, 2020

Can we reconsider this change? This is a huge regression for usability.

The fact that github.com/soheilhy/cmux does not support mTLS on a shared port does not sound like a convincing reason to require multiple configurations for gRPC vs HTTP receivers.

@jmacd jmacd mentioned this pull request Jul 2, 2020
Closed
wyTrivail pushed a commit to mxiamxia/opentelemetry-collector that referenced this pull request Jul 13, 2020
@bogdandrutu @wyTrivail
Split OTLP receiver by protocols, allow mTLS support (open-telemetry#…
b732ef4 
…1223)

Signed-off-by: Bogdan Drutu <[email protected]>
@mat-rumian mat-rumian mentioned this pull request Jul 28, 2020
Closed
3 tasks
@tigrannajaryan tigrannajaryan mentioned this pull request Nov 3, 2020
Closed
hughesjj pushed a commit to hughesjj/opentelemetry-collector that referenced this pull request Apr 27, 2023
@dependabot @jeffreyc-splunk
Bump lycheeverse/lychee-action from 1.1.1 to 1.2.1 (open-telemetry#1223)
396f898 
* Bump lycheeverse/lychee-action from 1.1.1 to 1.2.1

Bumps [lycheeverse/lychee-action](https://github.com/lycheeverse/lychee-action) from 1.1.1 to 1.2.1.
- [Release notes](https://github.com/lycheeverse/lychee-action/releases)
- [Commits](lycheeverse/lychee-action@v1.1.1...v1.2.1)

---
updated-dependencies:
- dependency-name: lycheeverse/lychee-action
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>

* Update globs

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: jcheng-splunk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tigrannajaryan tigrannajaryan tigrannajaryan approved these changes

@dmitryax dmitryax Awaiting requested review from dmitryax

@flands flands Awaiting requested review from flands

@james-bebbington james-bebbington Awaiting requested review from james-bebbington

@owais owais Awaiting requested review from owais

@pjanotti pjanotti Awaiting requested review from pjanotti

@rghetia rghetia Awaiting requested review from rghetia

Assignees

@tigrannajaryan tigrannajaryan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bogdandrutu @jmacd @tigrannajaryan