8000 feat: add grpc tls support on gateway by samsja · Pull Request #4522 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
10000

feat: add grpc tls support on gateway #4522

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
Merged
merged 4 commits into from
Mar 23, 2022
Merged

feat: add grpc tls support on gateway #4522

merged 4 commits into from
Mar 23, 2022

Conversation

samsja
Copy link
Contributor
@samsja samsja commented Mar 21, 2022 •
edited
Loading

This pr add tls suppport for the grpc gateway

  • allow tls on grpc server (gateway)
  • refactor to have a common API for http and gtpc tls support
  • (skip) OPTIONAL : intra pod communication using tls

PS: websocket and http gateway already handle tls

@samsja samsja mentioned this pull request Mar 21, 2022
Closed
@github-actions github-actions bot added size/M area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing labels Mar 21, 2022
@samsja samsja closed this Mar 22, 2022
@samsja samsja reopened this Mar 22, 2022
@codecov
Copy link
codecov bot commented Mar 22, 2022 •
edited
Loading

Codecov Report

Merging #4522 (03ec9ce) into master (b0f839b) will increase coverage by 0.43%.
The diff coverage is 95.83%.

@@            Coverage Diff             @@
##           master    #4522      +/-   ##
==========================================
+ Coverage   87.91%   88.34%   +0.43%     
==========================================
  Files         116      116              
  Lines        8455     8484      +29     
==========================================
+ Hits         7433     7495      +62     
+ Misses       1022      989      -33
Flag Coverage Δ
daemon 41.98% <37.50%> (-0.01%) ⬇️
jina 88.09% <95.83%> (+0.44%) ⬆️

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

Impacted Files Coverage Δ
jina/orchestrate/flow/base.py 91.45% <ø> (ø)
jina/serve/runtimes/gateway/grpc/__init__.py 95.83% <90.90%> (-1.61%) ⬇️
jina/parsers/orchestrate/runtimes/remote.py 100.00% <100.00%> (ø)
jina/serve/runtimes/gateway/http/__init__.py 97.77% <100.00%> (+0.21%) ⬆️
jina/serve/runtimes/gateway/websocket/__init__.py 97.77% <100.00%> (+0.21%) ⬆️
jina/clients/base/__init__.py 90.38% <0.00%> (-0.53%) ⬇️
jina/serve/executors/__init__.py 84.72% <0.00%> (+0.43%) ⬆️
jina/serve/networking.py 88.43% <0.00%> (+0.68%) ⬆️
jina/orchestrate/deployments/__init__.py 84.81% <0.00%> (+2.95%) ⬆️
...deployments/config/k8slib/kubernetes_deployment.py 51.21% <0.00%> (+4.87%) ⬆️
... and 2 more

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 b0f839b...03ec9ce. Read the comment docs.

@samsja samsja force-pushed the feat-tls-for-grpc branch 2 times, most recently from cb76f37 to 8320a7b Compare March 22, 2022 11:55
@github-actions
Copy link
github-actions bot commented Mar 22, 2022 •
edited
Loading

Latency summary

Current PR yields:

  • 😶 index QPS at 1228, delta to last 2 avg.: -4%
  • 🐢🐢 query QPS at 61, delta to last 2 avg.: -16%
  • 🐎🐎🐎🐎 avg flow time within 1.5466 seconds, delta to last 2 avg.: +22%
  • 😶 import jina within 0.3972 seconds, delta to last 2 avg.: +3%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1228 61 1.5466 0.3972
3.2.8 1278 72 1.2679 0.3831
3.2.7 1294 72 1.2633 0.3832

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

feat: add grpc ssl on server side
a1dbe5f 
style: fix overload and cli autocomplete

feat: add tls
@samsja samsja force-pushed the feat-tls-for-grpc branch from 8320a7b to a1dbe5f Compare March 22, 2022 12:57
@samsja samsja closed this Mar 22, 2022
@samsja samsja reopened this Mar 22, 2022
@hanxiao
Copy link
Member
hanxiao commented Mar 22, 2022 •
edited
Loading

it is not clear to me if websocket'a TLS is included: if not, better include it. If it is already included, then please add it to PR's body

@samsja
Copy link
Contributor Author
samsja commented Mar 23, 2022

it is not clear to me if websocket'a TLS is included: if not, better include it. If it is already included, then please add it to PR's body

This pr is only about grpc as websocket is already handle both on server and on client side

@samsja samsja force-pushed the feat-tls-for-grpc branch from 5fe46ba to f559aad Compare March 23, 2022 07:45
@samsja samsja marked this pull request as ready for review March 23, 2022 07:47
@samsja samsja closed this Mar 23, 2022
@samsja samsja reopened this Mar 23, 2022
@samsja samsja force-pushed the feat-tls-for-grpc branch from d9958e5 to 392dccf Compare March 23, 2022 08:05
@samsja samsja closed this Mar 23, 2022
@samsja samsja reopened this Mar 23, 2022
@JoanFM
Copy link
Contributor
JoanFM commented Mar 23, 2022

Please, add documentation

JoanFM
JoanFM reviewed Mar 23, 2022
View reviewed changes
Copy link
Contributor
@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

Documentation required

JoanFM
JoanFM suggested changes Mar 23, 2022
View reviewed changes
@samsja
Copy link
Contributor Author
samsja commented Mar 23, 2022

Documentation required

there is a pending PR that will add documentation for everything related to tls. #4500
I think that it is better to don't add documentation for this PR and do it only in the documentation PR

@samsja samsja closed this Mar 23, 2022
@samsja samsja reopened this Mar 23, 2022
@samsja samsja requested a review from JoanFM March 23, 2022 12:35
@JoanFM JoanFM merged commit dd5f08e into master Mar 23, 2022
@JoanFM JoanFM deleted the feat-tls-for-grpc branch March 23, 2022 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoanFM JoanFM JoanFM approved these changes

Assignees
No one assigned
Labels
area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@samsja @hanxiao @JoanFM @jina-bot
0