- Notifications
You must be signed in to change notification settings - Fork126
[PECOBLR-727] Add kerberos support for proxy auth#675
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Pull Request Overview
This PR adds Kerberos/SPNEGO authentication support for proxy authentication by consolidating proxy handling logic and introducing a new authentication method system.
- Consolidates proxy detection and authentication logic into a shared utility module
- Replaces specific proxy credential parameters with a generic
proxy_auth_methodparameter - Implements Kerberos/SPNEGO authentication support using the
requests-kerberoslibrary
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/databricks/sql/common/http_utils.py | New utility module containing shared proxy detection and authentication logic with support for basic and Kerberos authentication methods |
src/databricks/sql/common/unified_http_client.py | Updated to use shared proxy utilities and support per-request proxy decisions with dual pool managers |
src/databricks/sql/auth/thrift_http_client.py | Refactored to use shared proxy utilities and removed duplicate authentication logic |
src/databricks/sql/backend/sea/utils/http_client.py | Updated to use shared proxy utilities and cleaned up variable names |
src/databricks/sql/auth/common.py | Replaced specific proxy credential parameters with genericproxy_auth_method parameter |
src/databricks/sql/utils.py | Updated to pass newproxy_auth_method parameter instead of individual credentials |
src/databricks/sql/backend/thrift_backend.py | Added support for passing proxy authentication method to transport layer |
tests/unit/test_thrift_backend.py | Updated test to use new shared utility function |
tests/unit/test_telemetry.py | Fixed method name references to match renamed methods |
pyproject.toml | Added optionalrequests-kerberos dependency for Kerberos authentication support |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
jprakash-db left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. Thanks for making the changes
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
8e97878 intomainUh oh!
There was an error while loading.Please reload this page.
| headers:Optional[Dict[str,str]]=None, | ||
| **kwargs, | ||
| )->Generator[urllib3.HTTPResponse,None,None]: | ||
| )->Generator[urllib3.BaseHTTPResponse,None,None]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here urllib3 has no BaseHTTPResponse this class is in urllib3.response
Uh oh!
There was an error while loading.Please reload this page.
What type of PR is this?
Description
This PR adds Kerberos/SPNEGO authentication support for proxy authentication by consolidating proxy handling logic and introducing a new authentication method system.
Consolidates proxy detection and authentication logic into a shared utility module
Replaces specific proxy credential parameters with a generic proxy_auth_method parameter
Implements Kerberos/SPNEGO authentication support using the requests-kerberos library
How is this tested?
Related Tickets & Documents