Bug-Fix Issue #4732 Add client_cert_path and server_cert_path into model registry rest store utils

[ericgosno91]

What changes are proposed in this pull request?

Currently the model registry API is not correctly attaching the client and/or server certificate (when defined) as tracking API. The issue is due to both client_cert_path and server_cert_path are not passed into MlflowHostCreds for model registry rest store utils (mlflow/tracking/_model_registry/utils.py). This is causing issue when we tried to use mlflow model serve cli that hitting on certificate protected DNS

The proposed solution is just simply pass the client_cert_path and server_cert_path into MLFlowHost similar with tracking rest store (mlflow/tracking/_tracking_service/utils.py)

How is this patch tested?

Running mlflow model serve cli into remote mlflow server whose DNS is certificate-protected

Release Notes

Is this a user-facing change?

• No. You can skip the rest of this section.
• Yes. Give a description of this change to be included in the release notes for MLflow users.

(Details in 1-2 sentences. You can just refer to another PR with a description if this PR is part of a larger change.)
There should be no changes on the user-facing, the change will only impact API for remote model registry that is protected by certificate

What component(s), interfaces, languages, and integrations does this PR affect?

Components

• area/artifacts: Artifact stores and artifact logging
• area/build: Build and test infrastructure for MLflow
• area/docs: MLflow documentation pages
• area/examples: Example code
• area/model-registry: Model Registry service, APIs, and the fluent client calls for Model Registry
• area/models: MLmodel format, model serialization/deserialization, flavors
• area/projects: MLproject format, project running backends
• area/scoring: MLflow Model server, model deployment tools, Spark UDFs
• area/server-infra: MLflow Tracking server backend
• area/tracking: Tracking Service, tracking client APIs, autologging

Interface

• area/uiux: Front-end, user experience, plotting, JavaScript, JavaScript dev server
• area/docker: Docker use across MLflow's components, such as MLflow Projects and MLflow Models
• area/sqlalchemy: Use of SQLAlchemy in the Tracking Service or Model Registry
• area/windows: Windows support

Language

• language/r: R APIs and clients
• language/java: Java APIs and clients
• language/new: Proposals for new client languages

Integrations

• integrations/azure: Azure and Azure ML integrations
• integrations/sagemaker: SageMaker integrations
• integrations/databricks: Databricks integrations

How should the PR be classified in the release notes? Choose one:

• rn/breaking-change - The PR will be mentioned in the "Breaking Changes" section
• rn/none - No description will be included. The PR will be mentioned only by the PR number in the "Small Bugfixes and Documentation Updates" section
• rn/feature - A new user-facing feature worth mentioning in the release notes
• rn/bug-fix - A user-facing bug fix worth mentioning in the release notes
• rn/documentation - A user-facing documentation change worth mentioning in the release notes

[github-actions]

@ericgosno91 Thanks for the contribution! The DCO check failed. Please sign off your commits by following the instructions here: https://github.com/mlflow/mlflow/runs/3409836731. See https://github.com/mlflow/mlflow/blob/master/CONTRIBUTING.rst#sign-your-work for more details.

[ericgosno91]

Dear Ankit @ankit-db,
Is there any update regarding this PR? because this issue forced us to use our own forked branch instead of actual mlflow version release.

any review/suggestion will be appreciated, thank you

[ankit-db]

Thanks for pinging @ericgosno91 ! I am slightly concerned that we are using tracking environment variables here as an architectural pattern going forward, but we do that for the other arguments too so I won't block this PR on it. Thanks for the fix and appreciate your contribution to the community here!

[ankit-db]

Going to loop in @harupy here since he modified some of this code recently just to confirm before merging.

[harupy]

LGTM!

[ericgosno91]

Thank you for quick review and followup @ankit-db and @harupy

I do agree that the solution is not ideal architectural wise, I think the better solution is either to join utils for model registry (mlflow/tracking/_model_registry/utils.py) and tracking service (mlflow/tracking/_tracking_service/utils.py) or to separate the env for both services entirely.

Since both solutions brought pattern changes and especially the later solutions may cause a breaking backward compatibility, I think this decision better be decided by more experienced contributor

[ericgosno91]

hi @ankit-db, please kindly help to approve the workflow again, as I just rebase the commit into latest master (the previous commit seems failed on version matrix test due to being behind in dependency versioning)

[ericgosno91]

hi @ankit-db, the PR seems to have passed all checks, can you help to merge the PR? Thanks for all the assistance until now 🙏

[ankit-db]

Done! Thanks for the contribution!