Skip to content

Add possibility to use client cert. with tracking API. #2843

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

Merged
merged 13 commits into from
May 29, 2020

Conversation

PhilipMay
Copy link
Contributor

@PhilipMay PhilipMay commented May 17, 2020

see #2574

TODO

@PhilipMay
Copy link
Contributor Author

PhilipMay commented May 17, 2020

@dbczumar
Before I continue with this PR I would like to have some feedback.
In this PR I only plan to introduce client cert usage with environment variable.
I do not plan to open up the python tracking API.

Disregarding the missing documentation and tests: Is this implementation ok or do you see more needed changes on other places?

@dbczumar dbczumar self-requested a review May 18, 2020 07:37
@dbczumar
Copy link
Collaborator

@PhilipMay Sorry for the delay here. This implementation looks great! Let me know if you can add some tests and documentation - more than happy to get this merged once those have been added! Thank you for your contribution!

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2020

Codecov Report

Merging #2843 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2843   +/-   ##
=======================================
  Coverage   85.04%   85.04%           
=======================================
  Files          20       20           
  Lines        1050     1050           
=======================================
  Hits          893      893           
  Misses        157      157           

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 34e8174...9590c1e. Read the comment docs.

@PhilipMay
Copy link
Contributor Author

PhilipMay commented May 23, 2020

I am finished with this PR.

  • Tests written - positive and negative
  • Docstring for python
  • Documentation

CI Tests are all green...

@dbczumar
Can you please review this and give feedback?

Thanks,
Philip

@PhilipMay
Copy link
Contributor Author

Hi. Any comments on this PR?

if not host:
raise MlflowException("host is a required parameter for MlflowHostCreds")
if ignore_tls_verification and (server_cert_path is not None):
raise MlflowException("if 'MLFLOW_TRACKING_INSECURE_TLS' is set to true "
Copy link
Collaborator

@dbczumar dbczumar May 28, 2020

Choose a reason for hiding this comment

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

@PhilipMay This is a really minor point of feedback, but can we start this error message by indicating that ignore_tls_verification=True and server_cert_path should not both be specified and include the environment variable part afterwards as a hint? Users or some other piece of backend code may try to construct this object directly, without the use of environment variables.

Also, can we specify an error code of INVALID_PARAMETER_VALUE here? See https://github.com/mlflow/mlflow/blob/master/mlflow/utils/uri.py#L55 for an example.

Copy link
Collaborator

@dbczumar dbczumar left a comment

Choose a reason for hiding this comment

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

@PhilipMay Sorry for the delay again - this looks awesome! I left one small comment about the error message behavior. I'm going to give this a manual test tomorrow and should be ready to approve / merge it by EOD tomorrow!

@PhilipMay
Copy link
Contributor Author

PhilipMay commented May 28, 2020

The exception message has been improved. The error code is still missing.

2 lines above my exception is an other MlflowException without error code. Since I am not familiar with the proto stuff like in the example I am not sure how exactly to implement it...
Could you maybe add this after the PR?

@dbczumar What do you think?

@PhilipMay
Copy link
Contributor Author

PhilipMay commented May 29, 2020

I'm going to give this a manual test tomorrow and should be ready to approve / merge it by EOD tomorrow!

If you have problems with the manual test I can provide an howto. I start the server with this command:

mlflow server --gunicorn-opts "--keyfile=/home/mlflow/mlflow-cert/server.key --certfile=/home/mlflow/mlflow-cert/server.crt --cert-reqs=2 --ca-certs=/home/mlflow/mlflow-cert/client.crt" --backend-store-uri postgresql://mlflow:<password>@127.0.0.1/mlflow?sslmode=require --default-artifact-root /home/mlflow/default-artifact-root -h 0.0.0.0

Here is the demo client code:

import mlflow
import os

os.environ["MLFLOW_TRACKING_CLIENT_CERT_PATH"] = "/<some_dir>/client.pem"
os.environ["MLFLOW_TRACKING_SERVER_CERT_PATH"] = "/<some_dir>/server.crt"

mlflow.set_tracking_uri("https://<URL>:5000")
mlflow.set_experiment("cert-test-01")

for i in range(4):
    with mlflow.start_run():
        mlflow.log_metric('x', 2*i)
        mlflow.log_metric('y', 100/((i+1)*2))
        mlflow.set_tag('t1', 'tag:{}'.format(i))
        mlflow.log_param('p1', 'param:{}'.format(i))

@dbczumar
Copy link
Collaborator

@PhilipMay I've pushed the tweaks you mentioned for the INVALID_PARAMETER_VALUE error code; I've tested this manually and found that it works as expected! I'll go ahead and merge this once the tests pass!

Copy link
Collaborator

@dbczumar dbczumar left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks so much @PhilipMay !

@dbczumar dbczumar merged commit 924b1bd into mlflow:master May 29, 2020
@smurching smurching added the rn/feature Mention under Features in Changelogs. label Jun 19, 2020
avflor pushed a commit to avflor/mlflow that referenced this pull request Aug 22, 2020
* client and server cert from env var

* doc

* python docstring of MlflowHostCreds

* test_ignore_tls_verification_not_server_cert_path

* tests for client & server cert path

* client and server path env. var doc

* improved client and server path env. var doc

* improved python docstring of MlflowHostCreds

* improved client and server path env. var doc

* exception text improvement

* linter line too long fix

* Docs fixes

* Fix typo

Co-authored-by: Corey Zumar <corey.zumar@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn/feature Mention under Features in Changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants