Allow using SSL between transformer and predictor/explainer #2911

greenmoon55 · 2023-05-16T20:04:45Z

What this PR does / why we need it: Currently the transformer is hard-coded to use http to communicate with the predictor. We add a use_ssl property to Model class to allow using SSL between transformer and predictor/explainer for both rest and grpc.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2863

Type of changes
Please delete options that are not relevant.

New feature (non-breaking change which adds functionality)

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Run a local transformer with a remote predictor with REST v2 with ssl enabled. Make sure we get the inference result.
Run a local transformer with a remote predictor with GRPC with ssl enabled. Make sure we get the inference result.

Note that if you need a custom root CA, you can set an environment variable like this export GRPC_DEFAULT_SSL_ROOTS_FILE_PATH=bundle.pem

Logs

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
No.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

Release note:

Added `use_ssl` property to `Model` to allow using SSL between transformer and predictor/explainer.

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

yuzisun · 2023-05-16T22:59:45Z

Thanks @greenmoon55 ! Can you help update the local transformer testing instruction here for using https predictor url?
https://github.com/kserve/kserve/tree/master/docs/samples/v1beta1/transformer/torchserve_image_transformer#testing-with-a-local-transformer--predictor-in-kubernetes-kserve-developer-guide

greenmoon55 · 2023-05-17T14:58:19Z

https://github.com/kserve/kserve/tree/master/docs/samples/v1beta1/transformer/torchserve_image_transformer#testing-with-a-local-transformer--predictor-in-kubernetes-kserve-developer-guide

@yuzisun Is it better to update these two files?
https://github.com/kserve/website/tree/main/docs/modelserving/v1beta1/transformer/torchserve_image_transformer https://github.com/kserve/kserve/blob/master/python/custom_transformer/model.py

houshengbo · 2023-05-17T15:18:52Z

python/kserve/kserve/model.py

@@ -232,9 +237,10 @@ def postprocess(self, response: Union[Dict, InferResponse, ModelInferResponse],
        return response

    async def _http_predict(self, payload: Union[Dict, InferRequest], headers: Dict[str, str] = None) -> Dict:
-        predict_url = PREDICTOR_URL_FORMAT.format(self.predictor_host, self.name)
+        protocol = "https" if self.use_ssl else "http"


Do we have test cases for this function to verify if http or https is used?

Unfortunately not. I can't find a good way to add tests here. We have e2e tests for http but not for https.

houshengbo · 2023-05-17T15:19:19Z

python/kserve/kserve/model.py

@@ -311,9 +317,11 @@ async def explain(self, payload: Dict, headers: Dict[str, str] = None) -> Dict:
        """
        if self.explainer_host is None:
            raise NotImplementedError("Could not find explainer_host.")
-        explain_url = EXPLAINER_URL_FORMAT.format(self.explainer_host, self.name)
+
+        protocol = "https" if self.use_ssl else "http"


I am wondering if we have test cases for this function.

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

docs/samples/v1beta1/transformer/torchserve_image_transformer/README.md

yuzisun · 2023-05-17T22:44:36Z

python/kserve/kserve/model.py

-                self.predictor_host = self.predictor_host + ":80"
-            _channel = grpc.aio.insecure_channel(self.predictor_host)
+                port = 443 if self.use_ssl else 80
+                self.predictor_host = self.predictor_host + f":{port}"


Nit: both predictor_host and port can be in the fstring

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

greenmoon55 · 2023-05-18T18:19:31Z

python/custom_transformer/model.py

@@ -103,9 +104,12 @@ def postprocess(self, infer_response: Union[Dict, ModelInferResponse], headers:
 parser.add_argument(
    "--model_name", help="The name that the model is served under."
 )
+parser.add_argument(
+    "--use_ssl", help="Use ssl for connecting to the predictor", action='store_true'


It will be true with --use_ssl, otherwise False.

yuzisun · 2023-05-18T23:19:23Z

Great work @greenmoon55 !

/lgtm
/approve

kserve-oss-bot · 2023-05-18T23:19:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: greenmoon55, yuzisun

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yuzisun]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

) * Allow using SSL between transformer and predictor/explainer Signed-off-by: Jin Dong <jdong183@bloomberg.net> * Update custom transformer example Signed-off-by: Jin Dong <jdong183@bloomberg.net> * Update document Signed-off-by: Jin Dong <jdong183@bloomberg.net> * Update doc and use f string format Signed-off-by: Jin Dong <jdong183@bloomberg.net> --------- Signed-off-by: Jin Dong <jdong183@bloomberg.net> Signed-off-by: iamlovingit <freecode666@gmail.com>

Allow using SSL between transformer and predictor/explainer

3ec47db

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

kserve-oss-bot requested review from adriangonz and theofpa May 16, 2023 20:04

houshengbo reviewed May 17, 2023

View reviewed changes

greenmoon55 added 2 commits May 17, 2023 12:30

Update custom transformer example

fdaf3bd

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

Update document

c31af6f

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

yuzisun reviewed May 17, 2023

View reviewed changes

docs/samples/v1beta1/transformer/torchserve_image_transformer/README.md Show resolved Hide resolved

yuzisun reviewed May 17, 2023

View reviewed changes

Update doc and use f string format

f348aaf

Signed-off-by: Jin Dong <jdong183@bloomberg.net>

greenmoon55 commented May 18, 2023

View reviewed changes

kserve-oss-bot assigned yuzisun May 18, 2023

kserve-oss-bot added the lgtm label May 18, 2023

kserve-oss-bot added the approved label May 18, 2023

kserve-oss-bot merged commit 0d3e41d into kserve:master May 18, 2023
52 checks passed

greenmoon55 deleted the allow-ssl branch June 20, 2023 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow using SSL between transformer and predictor/explainer #2911

Allow using SSL between transformer and predictor/explainer #2911

greenmoon55 commented May 16, 2023 •

edited

yuzisun commented May 16, 2023

greenmoon55 commented May 17, 2023

houshengbo May 17, 2023

greenmoon55 May 18, 2023

houshengbo May 17, 2023

yuzisun May 17, 2023

greenmoon55 May 18, 2023

yuzisun commented May 18, 2023

kserve-oss-bot commented May 18, 2023

Allow using SSL between transformer and predictor/explainer #2911

Allow using SSL between transformer and predictor/explainer #2911

Conversation

greenmoon55 commented May 16, 2023 • edited

yuzisun commented May 16, 2023

greenmoon55 commented May 17, 2023

houshengbo May 17, 2023

Choose a reason for hiding this comment

greenmoon55 May 18, 2023

Choose a reason for hiding this comment

houshengbo May 17, 2023

Choose a reason for hiding this comment

yuzisun May 17, 2023

Choose a reason for hiding this comment

greenmoon55 May 18, 2023

Choose a reason for hiding this comment

yuzisun commented May 18, 2023

kserve-oss-bot commented May 18, 2023

greenmoon55 commented May 16, 2023 •

edited