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: sandbox tls #4608

Merged
merged 2 commits into from
Apr 13, 2022
Merged

fix: sandbox tls #4608

merged 2 commits into from
Apr 13, 2022

Conversation

jacobowitz
Copy link
Contributor

Fix gRPC reflection using TLS

Description

gRPC reflection is not respecting the TLS setttings. This makes all encrypted gRPC connections fail. This happens e.g. for sandbox.
This PR changes the logic so that we just reuse the existing channel for the reflection. This way I don't have to worry about the settings

@github-actions github-actions bot added size/S area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing labels Apr 13, 2022
@github-actions
Copy link

github-actions bot commented Apr 13, 2022

Latency summary

Current PR yields:

  • 🐎🐎🐎🐎 index QPS at 1329, delta to last 2 avg.: +24%
  • 🐎🐎🐎🐎 query QPS at 77, delta to last 2 avg.: +41%
  • 🐢🐢 avg flow time within 1.1651 seconds, delta to last 2 avg.: -13%
  • 🐢🐢 import jina within 0.3988 seconds, delta to last 2 avg.: -19%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1329 77 1.1651 0.3988
3.3.0 1088 52 1.0886 0.4908
3.2.10 1040 56 1.599 0.5029

Backed by latency-tracking. Further commits will update this comment.

@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #4608 (dc95d42) into master (929054e) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4608      +/-   ##
==========================================
- Coverage   87.72%   87.70%   -0.03%     
==========================================
  Files         117      117              
  Lines        8484     8485       +1     
==========================================
- Hits         7443     7442       -1     
- Misses       1041     1043       +2     
Flag Coverage Δ
jina 87.70% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
jina/serve/networking.py 86.62% <100.00%> (+0.03%) ⬆️
...a/orchestrate/deployments/config/docker_compose.py 98.97% <0.00%> (-1.03%) ⬇️

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 07db554...dc95d42. Read the comment docs.

@jacobowitz jacobowitz marked this pull request as ready for review April 13, 2022 08:12
available_services = GrpcConnectionPool.get_available_services(self.address)
async def _init_stubs(self):
available_services = await GrpcConnectionPool.get_available_services(
self.channel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is the important change as we use the existing self.channel and not create a new one from address

Copy link
Member

@mapleeit mapleeit left a comment

Choose a reason for hiding this comment

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

I can understand the important change and the test passed. So looks good to me.

But others can have a precise review since I'm not a Python expert.

@jacobowitz jacobowitz merged commit b6d9681 into master Apr 13, 2022
@jacobowitz jacobowitz deleted the fix-tls-grpc-reflection branch April 13, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants