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.

Sign up for GitHub

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

Jump to bottom

Add more detailed error message on default credentials not found error #419

Merged
merged 1 commit into from
Nov 3, 2023
Merged

Add more detailed error message on default credentials not found error #419

merged 1 commit into from
Nov 3, 2023

Conversation

edwardfeng-db
Copy link
Contributor

@edwardfeng-db edwardfeng-db commented Nov 1, 2023 •
edited
Loading

Changes

  • Added more detailed error message for default credentials not found error - found the original error message was a bit difficult to follow / understand

Tests

Not quite sure if I need to do anything special if CI checks passed.
Manually tried using default auth locally without .databrickscfg file, verified that the prompt is printed out

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

@edwardfeng-db edwardfeng-db force-pushed the default-credential-error-message branch 3 times, most recently from 75165de to c9472a6 Compare November 2, 2023 09:05
edwardfeng-db
edwardfeng-db commented Nov 2, 2023
View reviewed changes
databricks/sdk/core.py Outdated
elif self._credentials_provider.auth_type() == "default":
# Empty debug string when auth type is default means we are likely missing some files or environment variables. Add a debug string to make it easier to understand.
default_auth_prompt = "Please check https://github.com/databricks/databricks-sdk-py/tree/main#default-authentication-flow to configure credentials for your preferred authentication method."
message = f'{message.rstrip(".")}. {default_auth_prompt}'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this prompt for cases that debug string is empty while using default auth.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is great, but can we add it inside of the ValueError raised by DefaultCredentials when this issue arises instead? This function is called whenever logging any error, including non-auth-related errors, but the cue to look at the reference docs for unified auth should only be presented when the default credential provider fails.

mgyucht
mgyucht reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Great idea, but one suggestion on where to include this.

databricks/sdk/core.py Outdated
elif self._credentials_provider.auth_type() == "default":
# Empty debug string when auth type is default means we are likely missing some files or environment variables. Add a debug string to make it easier to understand.
default_auth_prompt = "Please check https://github.com/databricks/databricks-sdk-py/tree/main#default-authentication-flow to configure credentials for your preferred authentication method."
message = f'{message.rstrip(".")}. {default_auth_prompt}'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is great, but can we add it inside of the ValueError raised by DefaultCredentials when this issue arises instead? This function is called whenever logging any error, including non-auth-related errors, but the cue to look at the reference docs for unified auth should only be presented when the default credential provider fails.

@mgyucht mgyucht changed the title [DECO] Add more detailed error message on default credentials not found error Add more detailed error message on default credentials not found error Nov 2, 2023
@edwardfeng-db edwardfeng-db force-pushed the default-credential-error-message branch from c9472a6 to b1cba9b Compare November 2, 2023 09:27
mgyucht
mgyucht approved these changes Nov 2, 2023
View reviewed changes
Copy link
Contributor

@mgyucht mgyucht left a comment

Choose a reason for hiding this comment

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

Basically LGTM. My only question is whether you think it may be better to refer to our public docs (https://docs.databricks.com/en/dev-tools/auth.html#databricks-client-unified-authentication) or our repo's docs. To be honest, to eliminate having two different doc sources for the SDK, it might be better to link to public docs and later remove some of the documentation from the SDK README.md. What do you think?

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
databricks/sdk/core.py 81.34% <100.00%> (+0.01%) ⬆️

📢 Thoughts on this report? Let us know!.

tanmay-db
tanmay-db reviewed Nov 2, 2023
View reviewed changes
tests/test_auth.py Outdated
@@ -4,19 +4,20 @@

from .conftest import __tests__, raises

default_auth_base_error_message = "default auth: cannot configure default credentials, please check https://github.com/databricks/databricks-sdk-py/tree/main#default-authentication-flow to configure credentials for your preferred authentication method"
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. I think @hectorcast-db was working on better handling, not sure if there are intersections but mentioning for viz.

Also, if going ahead with this, we would have to do this change in all SDKs for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to fix the error messages coming from the API to us (which we display to the users). There are probably cases were both messages are displayed, but in general is a different error message.

@edwardfeng-db edwardfeng-db force-pushed the default-credential-error-message branch 2 times, most recently from 758f29f to d4fb086 Compare November 3, 2023 09:29
@edwardfeng-db edwardfeng-db force-pushed the default-credential-error-message branch from d4fb086 to 29fdf28 Compare November 3, 2023 09:38
@edwardfeng-db edwardfeng-db added this pull request to the merge queue Nov 3, 2023
Merged via the queue into main with commit 5bd7f6b Nov 3, 2023
9 checks passed
@edwardfeng-db edwardfeng-db deleted the default-credential-error-message branch November 3, 2023 09:43
@edwardfeng-db edwardfeng-db mentioned this pull request Nov 3, 2023
Merged
3 tasks
github-merge-queue bot pushed a commit to databricks/databricks-sdk-go that referenced this pull request Nov 6, 2023
@edwardfeng-db
Add more detailed error message on default credentials not found error (
6e2bb1d 
#679)

## Changes
<!-- Summary of your changes that are easy to understand -->
- Added more detailed error message for default credentials not found
error - found the original error message was a bit difficult to follow /
understand
- To keep consistency with the python sdk as change introduced here
databricks/databricks-sdk-py#419

## Tests
<!-- 
How is this tested? Please see the checklist below and also describe any
other relevant tests
-->

- [ ] `make test` passing
- [ ] `make fmt` applied
- [ ] relevant integration tests applied
@edwardfeng-db edwardfeng-db mentioned this pull request Nov 7, 2023
Merged
mgyucht added a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0
9747f59 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`, `BadRequest`, `PermissionDenied`, `InternalError`, and others ([#376](#376)). This makes it easier to handle errors thrown by the Databricks API. Instead of catching `DatabricksError` and checking the error_code field, you can catch one of these subtypes of `DatabricksError`, which is more ergonomic and removes the need to rethrow exceptions that you don't want to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK ([#440](#440)). This change ensures that SCIM list() APIs use a default limit of 100 resources, leveraging SCIM's offset + limit pagination to batch requests to the Databricks API.
* Added taskValues support in remoteDbUtils ([#406](#406)).
* Added more detailed error message on default credentials not found error ([#419](#419)).
* Request management token via Azure CLI only for Service Principals and not human users ([#408](#408)).

API Changes:

 * Fixed `create()` method for [w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html) workspace-level service and corresponding `databricks.sdk.service.catalog.CreateFunction` and `databricks.sdk.service.catalog.FunctionInfo` dataclasses.
 * Changed `create()` method for [w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html) workspace-level service with new required argument order.
 * Changed `storage_root` field for `databricks.sdk.service.catalog.CreateMetastore` to be optional.
 * Added `skip_validation` field for `databricks.sdk.service.catalog.UpdateExternalLocation`.
 * Added `libraries` field for `databricks.sdk.service.compute.CreatePolicy`, `databricks.sdk.service.compute.EditPolicy` and `databricks.sdk.service.compute.Policy`.
 * Added `init_scripts` field for `databricks.sdk.service.compute.EventDetails`.
 * Added `file` field for `databricks.sdk.service.compute.InitScriptInfo`.
 * Added `zone_id` field for `databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
 * Replaced `ui_state` field with `edit_mode` for `databricks.sdk.service.jobs.CreateJob` and `databricks.sdk.service.jobs.JobSettings`.
 * Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with `databricks.sdk.service.jobs.CreateJobEditMode`.
 * Added `include_resolved_values` field for `databricks.sdk.service.jobs.GetRunRequest`.
 * Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass with `databricks.sdk.service.jobs.JobSettingsEditMode`.
 * Removed [a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html) account-level service. This was only used to aid in OAuth enablement during the public preview of OAuth. OAuth is now enabled for all AWS E2 accounts, so usage of this API is no longer needed.
 * Added `network_connectivity_config_id` field for `databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
 * Added [a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html) account-level service.
 * Added `string_shared_as` field for `databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template ([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup ([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
@mgyucht mgyucht mentioned this pull request Nov 14, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 14, 2023
@mgyucht
Release v0.13.0 (#442)
86ca043 
* Introduce more specific exceptions, like `NotFound`, `AlreadyExists`,
`BadRequest`, `PermissionDenied`, `InternalError`, and others
([#376](#376)). This
makes it easier to handle errors thrown by the Databricks API. Instead
of catching `DatabricksError` and checking the error_code field, you can
catch one of these subtypes of `DatabricksError`, which is more
ergonomic and removes the need to rethrow exceptions that you don't want
to catch. For example:
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except DatabricksError as e:
  if e.error_code in [
    "RESOURCE_DOES_NOT_EXIST",
    "RESOURCE_NOT_FOUND",
    "PERMISSION_DENIED",
    "FEATURE_DISABLED",
    "BAD_REQUEST"]:
    logger.warning(...)
    return None
  raise RetryableError(...) from e
```
can be replaced with
```python
try:
  return (self._ws
    .permissions
    .get(object_type, object_id))
except PermissionDenied, FeatureDisabled:
  logger.warning(...)
  return None
except NotFound:
  raise RetryableError(...)
```
* Paginate all SCIM list requests in the SDK
([#440](#440)). This
change ensures that SCIM list() APIs use a default limit of 100
resources, leveraging SCIM's offset + limit pagination to batch requests
to the Databricks API.
* Added taskValues support in remoteDbUtils
([#406](#406)).
* Added more detailed error message on default credentials not found
error
([#419](#419)).
* Request management token via Azure CLI only for Service Principals and
not human users
([#408](#408)).

API Changes:

* Fixed `create()` method for
[w.functions](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/functions.html)
workspace-level service and corresponding
`databricks.sdk.service.catalog.CreateFunction` and
`databricks.sdk.service.catalog.FunctionInfo` dataclasses.
* Changed `create()` method for
[w.metastores](https://databricks-sdk-py.readthedocs.io/en/latest/workspace/metastores.html)
workspace-level service with new required argument order.
* Changed `storage_root` field for
`databricks.sdk.service.catalog.CreateMetastore` to be optional.
* Added `skip_validation` field for
`databricks.sdk.service.catalog.UpdateExternalLocation`.
* Added `libraries` field for
`databricks.sdk.service.compute.CreatePolicy`,
`databricks.sdk.service.compute.EditPolicy` and
`databricks.sdk.service.compute.Policy`.
* Added `init_scripts` field for
`databricks.sdk.service.compute.EventDetails`.
* Added `file` field for
`databricks.sdk.service.compute.InitScriptInfo`.
* Added `zone_id` field for
`databricks.sdk.service.compute.InstancePoolGcpAttributes`.
 * Added several dataclasses related to init scripts.
 * Added `databricks.sdk.service.compute.LocalFileInfo` dataclass.
* Replaced `ui_state` field with `edit_mode` for
`databricks.sdk.service.jobs.CreateJob` and
`databricks.sdk.service.jobs.JobSettings`.
* Replaced `databricks.sdk.service.jobs.CreateJobUiState` dataclass with
`databricks.sdk.service.jobs.CreateJobEditMode`.
* Added `include_resolved_values` field for
`databricks.sdk.service.jobs.GetRunRequest`.
* Replaced `databricks.sdk.service.jobs.JobSettingsUiState` dataclass
with `databricks.sdk.service.jobs.JobSettingsEditMode`.
* Removed
[a.o_auth_enrollment](https://databricks-sdk-py.readthedocs.io/en/latest/account/o_auth_enrollment.html)
account-level service. This was only used to aid in OAuth enablement
during the public preview of OAuth. OAuth is now enabled for all AWS E2
accounts, so usage of this API is no longer needed.
* Added `network_connectivity_config_id` field for
`databricks.sdk.service.provisioning.UpdateWorkspaceRequest`.
* Added
[a.network_connectivity](https://databricks-sdk-py.readthedocs.io/en/latest/account/network_connectivity.html)
account-level service.
* Added `string_shared_as` field for
`databricks.sdk.service.sharing.SharedDataObject`.

Internal changes:

* Added regression question to issue template
([#414](#414)).
* Made test_auth no longer fail if you have a default profile setup
([#426](#426)).

OpenAPI SHA: d136ad0541f036372601bad9a4382db06c3c912d, Date: 2023-11-14
github-merge-queue bot pushed a commit to databricks/databricks-sdk-java that referenced this pull request Nov 15, 2023
@edwardfeng-db
Add more detailed error message on default credentials not found error (
3d34102 
#180)

## Changes
<!-- Summary of your changes that are easy to understand -->
- Added more detailed error message for default credentials not found
error - found the original error message was a bit difficult to follow /
understand
- To keep consistency with the python sdk as change introduced here
databricks/databricks-sdk-py#419

## Tests
<!-- How is this tested? -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hectorcast-db hectorcast-db hectorcast-db left review comments

@tanmay-db tanmay-db tanmay-db left review comments

@mgyucht mgyucht mgyucht approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@edwardfeng-db @codecov-commenter @mgyucht @tanmay-db @hectorcast-db