feat: Add SSL support for http api servers via bentoml serve #2886

sptowey · 2022-08-09T21:22:59Z

What does this PR address?

Before submitting:

Does the Pull Request follow Conventional Commits specification naming? Here are GitHub's
guide on how to create a pull request.
Does the code follow BentoML's code style, both make format and make lint script have passed (instructions)?
Did you read through contribution guidelines and follow development guidelines?
Did your changes require updates to the documentation? Have you updated
those accordingly? Here are documentation guidelines and tips on writting docs.
Did you write tests to cover your changes? N/A - cleared with Sean in Slack

Who can help review?

@ssheng
@sauyon

Feel free to tag members/contributors who can help review your PR.

Supress pylint warning

pep8speaks · 2022-08-09T21:23:04Z

Hello @sptowey, Thanks for updating this PR.

There are currently no PEP 8 issues detected in this PR. Cheers! 🍻

Comment last updated at 2022-08-11 19:51:29 UTC

aarnphm

Thanks for the contribution. Make sure to run make format and make lint locally 🎉

bentoml/_internal/configuration/containers.py

aarnphm · 2022-08-09T21:50:37Z

bentoml/serve.py

+    ssl_keyfile: t.Optional[str] = None,
+    ssl_certfile: t.Optional[str] = None,
+    ssl_keyfile_password: t.Optional[str] = None,
+    ssl_version: t.Optional[int] = None,
+    ssl_cert_reqs: t.Optional[int] = None,
+    ssl_ca_certs: t.Optional[str] = None,
+    ssl_ciphers: t.Optional[str] = None,


Should we use dependency injection here?

+1. Let's inject config here.

ssl_keyfile: t.Optional[str] = Provide[BentoMLContainer.api_server_config.ssl.keyfile]

Aha, I understand now. I'd considered doing it that was to start, but wasn't entirely clear on if Provide would default to None as expected.

parano · 2022-08-10T02:45:31Z

bentoml_cli/serve.py

+        "--ssl-keyfile",
+        type=click.STRING,
+        help="SSL key file",
+        default=None,


Should we set the default value from config here? e.g.

default=BentoMLContainer.api_server_config.ssl.keyfile.get(),

I think we should use dependency injection like Aaron said above, we probably should try to start avoiding depending on BentoML before the click initialization so that click completion isn't horribly slow.

After this PR got merge, I think we can do a quick cleanup on CLI and set default value to None, and then we can access the value via DI lazily.

Let me know if you want me to help out with this. Sounds like you all may have it under control though?

Since there is no default value for the SSL related configurations, doing dependency injection here is not very meaningful. I think we can leave it out.

It is more about user providing this option via config right?

Correct. What I meant was that we should do config injection at the places you suggested in serve_development and serve_production instead of CLI.

codecov · 2022-08-10T08:42:52Z

Codecov Report

Merging #2886 (9fbcba4) into main (ce6342d) will increase coverage by 0.14%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2886      +/-   ##
==========================================
+ Coverage   70.70%   70.85%   +0.14%     
==========================================
  Files         103      103              
  Lines        9302     9328      +26     
==========================================
+ Hits         6577     6609      +32     
+ Misses       2725     2719       -6

Impacted Files	Coverage Δ
bentoml/_internal/configuration/containers.py	`80.70% <ø> (ø)`
bentoml/_internal/utils/docker.py	`68.96% <0.00%> (-4.95%)`	⬇️
bentoml/_internal/yatai_client/__init__.py	`25.00% <0.00%> (-0.51%)`	⬇️
bentoml/_internal/runner/runnable.py	`93.75% <0.00%> (+1.56%)`	⬆️
bentoml/_internal/utils/analytics/usage_stats.py	`96.42% <0.00%> (+18.81%)`	⬆️

sptowey · 2022-08-10T13:27:19Z

Thanks for the contribution. Make sure to run make format and make lint locally 🎉

Whoops, I ran the linter but missed the formatter!

ssheng · 2022-08-11T19:33:39Z

bentoml_cli/serve.py

+        "--ssl-keyfile",
+        type=click.STRING,
+        help="SSL key file",
+        default=None,


Correct. What I meant was that we should do config injection at the places you suggested in serve_development and serve_production instead of CLI.

ssheng · 2022-08-11T19:33:59Z

bentoml/serve.py

+    ssl_keyfile: t.Optional[str] = None,
+    ssl_certfile: t.Optional[str] = None,
+    ssl_keyfile_password: t.Optional[str] = None,
+    ssl_version: t.Optional[int] = None,
+    ssl_cert_reqs: t.Optional[int] = None,
+    ssl_ca_certs: t.Optional[str] = None,
+    ssl_ciphers: t.Optional[str] = None,


+1. Let's inject config here.

ssl_keyfile: t.Optional[str] = Provide[BentoMLContainer.api_server_config.ssl.keyfile]

ssheng · 2022-08-11T19:34:57Z

bentoml/serve.py

+    ssl_keyfile: t.Optional[str] = None,
+    ssl_certfile: t.Optional[str] = None,
+    ssl_keyfile_password: t.Optional[str] = None,
+    ssl_version: t.Optional[int] = None,
+    ssl_cert_reqs: t.Optional[int] = None,
+    ssl_ca_certs: t.Optional[str] = None,
+    ssl_ciphers: t.Optional[str] = None,


And inject config here.

ssl_keyfile: t.Optional[str] = Provide[BentoMLContainer.api_server_config.ssl.keyfile],

aarnphm · 2022-08-12T02:14:36Z

bentoml/serve.py

+    api_server_watcher_args.extend(
+        ["--ssl-keyfile", ssl_keyfile]
+    ) if ssl_keyfile is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-certfile", ssl_certfile]
+    ) if ssl_certfile is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-keyfile-password", ssl_keyfile_password]
+    ) if ssl_keyfile_password is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-version", str(ssl_version)]
+    ) if ssl_version is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-cert-reqs", str(ssl_cert_reqs)]
+    ) if ssl_cert_reqs is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-ca-certs", ssl_ca_certs]
+    ) if ssl_ca_certs is not None else None  # pylint: disable=W0106
+    api_server_watcher_args.extend(
+        ["--ssl-ciphers", ssl_ciphers]
+    ) if ssl_ciphers is not None else None  # pylint: disable=W0106


https://github.com/encode/uvicorn/blob/b21ecabc5bf911f571e0629438315a1e5472065c/uvicorn/config.py

Uvicorn does handle None option for some option.

IMO, a nicer way to write None check is

if ssl_certfile: args.extend(["--ssl-certfile", ssl_certfile]) if ssl_cert_reqs: args.extend(...) if ssl_version: ...

Then we can pass all of the other options straight through

Yes, this is a weird bit of code. It doesn't (shouldn't?) actually pass None into uvicorn - here is where uvicorn actually gets the args if they exist.
I wrote it this way to try compacting the code a bit, but running it through the formatter made that a moot point. I agree that it should be reformatted to the way you proposed - much easier to understand.

aarnphm

Thanks again for the contribution. I can update the style with a follow-up PR.

* Passthrough SSL args from "bentoml server" to uvicorn api servers Supress pylint warning * Delete TODO comments * Fix code formatting * Inject ssl defaults from config Signed-off-by: Aaron Pham <[email protected]>

sptowey added 2 commits August 9, 2022 14:10

Passthrough SSL args from "bentoml server" to uvicorn api servers

d23713a

Supress pylint warning

Delete TODO comments

76c2beb

sptowey requested review from ssheng, parano and a team as code owners August 9, 2022 21:23

sptowey requested review from sauyon and removed request for a team August 9, 2022 21:23

aarnphm reviewed Aug 9, 2022

View reviewed changes

parano reviewed Aug 10, 2022

View reviewed changes

Fix code formatting

a1513a7

ssheng reviewed Aug 11, 2022

View reviewed changes

Inject ssl defaults from config

9fbcba4

aarnphm reviewed Aug 12, 2022

View reviewed changes

aarnphm approved these changes Aug 12, 2022

View reviewed changes

ssheng merged commit a48d83e into bentoml:main Aug 12, 2022

aarnphm mentioned this pull request Aug 12, 2022

patch: ssl styling and default value check #2899

Merged

5 tasks

gusghrlrl101 mentioned this pull request Apr 16, 2024

feature: add http api server's uvicorn option #4671

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add SSL support for http api servers via bentoml serve #2886

feat: Add SSL support for http api servers via bentoml serve #2886

sptowey commented Aug 9, 2022

pep8speaks commented Aug 9, 2022 •

edited

Loading

aarnphm left a comment

aarnphm Aug 9, 2022

ssheng Aug 11, 2022

sptowey Aug 11, 2022

parano Aug 10, 2022

sauyon Aug 10, 2022

aarnphm Aug 10, 2022

sptowey Aug 10, 2022

ssheng Aug 11, 2022

aarnphm Aug 11, 2022

ssheng Aug 11, 2022

codecov bot commented Aug 10, 2022 •

edited

Loading

sptowey commented Aug 10, 2022

ssheng Aug 11, 2022

ssheng Aug 11, 2022

ssheng Aug 11, 2022

aarnphm Aug 12, 2022

sptowey Aug 12, 2022

aarnphm left a comment •

edited

Loading

feat: Add SSL support for http api servers via bentoml serve #2886

feat: Add SSL support for http api servers via bentoml serve #2886

Conversation

sptowey commented Aug 9, 2022

What does this PR address?

Before submitting:

Who can help review?

pep8speaks commented Aug 9, 2022 • edited Loading

Comment last updated at 2022-08-11 19:51:29 UTC

aarnphm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2022 • edited Loading

Codecov Report

sptowey commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarnphm left a comment • edited Loading

Choose a reason for hiding this comment

pep8speaks commented Aug 9, 2022 •

edited

Loading

codecov bot commented Aug 10, 2022 •

edited

Loading

aarnphm left a comment •

edited

Loading