Skip to content
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

Support for custom host in ApiClient #576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Gnakhle
Copy link

@Gnakhle Gnakhle commented Oct 27, 2022

This is to allow enterprise users who are forced to interact with Databricks via a proxy to set a custom host for ApiClient.

Example
Before
client = ApiClient(host='https://company.cloud.databricks.com?o=123')
print(client.get_url("/"))

client = ApiClient(host='https://mydatabricksproxyservice.com/databricks/proxy')
print(client.get_url("/"))

-------------
https://company.cloud.databricks.com/api/2.0/
https://mydatabricksproxyservice.com/api/2.0/

After

client = ApiClient(host='https://company.cloud.databricks.com?o=123')
print(client.get_url("/"))

client = ApiClient(host='https://mydatabricksproxyservice.com/databricks/proxy')
print(client.get_url("/"))

-------------
https://company.cloud.databricks.com/api/2.0/
https://mydatabricksproxyservice.com/databricks/proxy/api/2.0/

@pietern
Copy link
Contributor

pietern commented Nov 4, 2022

Thanks for submitting the PR.

I'm hesitant to merge this because:

  1. It might break existing users who happened to have a path component in their host parameter
  2. It doesn't carry over to other tooling, e.g. Terraform

What kind of tools are you using to interact with Databricks where you need to use this proxy? E.g. do you use the ApiClient directly or also the CLI itself? Perhaps other custom tools?

If you're using ApiClient directly, you could subclass it and prepend your path prefix, for example:

class ProxyApiClient(ApiClient):
    def __init__(self, path_prefix=None, **kwargs):
        super().__init__(**kwargs)
        self.path_prefix = path_prefix

    def get_url(self, path, version=None):
        url = super().get_url(path, version)
        if self.path_prefix:
            o = urlparse(url)
            o = o._replace(path=self.path_prefix + o.path)
            url = o.geturl()
        return url

@Gnakhle
Copy link
Author

Gnakhle commented Nov 11, 2022

Thanks for review @pietern!

We interact with Databricks via the CLI directly so we can't modify code unless we fork it. Do you have an idea of a way of implementing this thats acceptable to merge? I could implement it if so

@uint0
Copy link

uint0 commented Nov 14, 2022

Would this change be more palatable if a DATABRICKS_BASE_URL or something similar was added. This would allow us to support this feature without breaking backcompat, but does seem like a bit of a hack 😅.

        if base_url := os.environ.get("DATABRICKS_BASE_URL"):  # this would probably be moved to config
            self.url = base_url
        else:
            ... original logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants