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

[DECO-2483] Handle Azure authentication when WorkspaceResourceID is provided #145

Merged
merged 3 commits into from
Sep 8, 2023
Merged

[DECO-2483] Handle Azure authentication when WorkspaceResourceID is provided #145

merged 3 commits into from
Sep 8, 2023

Conversation

hectorcast-db
Copy link
Contributor

@hectorcast-db hectorcast-db commented Aug 31, 2023 •
edited
Loading

Changes

Handle Azure authentication when WorkspaceResourceID is provided

Get token for the correct subscription

Tests

  • Created Unit tests
  • Manually listed workspace cluster in the following scenarios:
    • User with wrong default tenant. No WorkspaceResourceID provided: Fail (expected). WARN log emitted.
    • User with wrong default tenant. WorkspaceResourceID provided: Succeed
    • User with no subscription. No WorkspaceResourceID provided: Succeed. WARN log emitted.
    • User with no subscription. WorkspaceResourceID provided: Succeed (fallback mode, expected).
  • Run integration tests
    https://github.com/databricks/eng-dev-ecosystem/actions/runs/6038942050

@hectorcast-db hectorcast-db requested a review from mgyucht August 31, 2023 10:20
mgyucht
mgyucht reviewed Aug 31, 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.

I think I miscommunicated about the service principal question: you only need to make this change in the CLI credential provider pathway. Otherwise, this looks largely good, just some small nits on refactoring.

databricks-sdk-java/src/main/java/com/databricks/sdk/core/AzureCliCredentialsProvider.java Outdated
tokenSource.getToken(); // We need this for checking if Azure CLI is installed.
Optional<String> subscription = getSubscription(config);

if (subscription.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we refactor all of this into the tokenSourceFor method? I think that would prevent this configure() method from sprawling, and it seems to belong there in the first place.

...va/src/main/java/com/databricks/sdk/core/oauth/AzureServicePrincipalCredentialsProvider.java Outdated
RefreshableTokenSource inner = tokenSourceFor(config, config.getEffectiveAzureLoginAppId());
RefreshableTokenSource cloud =
tokenSourceFor(config, config.getAzureEnvironment().getServiceManagementEndpoint());
RefreshableTokenSource innerToken;
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 I was confused when we were talking about this earlier. We don't need to change this provider at all: the tenant ID must be explicitly specified, see line 27. What I meant was that: if a user is logged into the Azure CLI with a service principal, in AzureCliCredentialsProvider, we still will take the same pathway.

databricks-sdk-java/src/main/java/com/databricks/sdk/core/AzureCliCredentialsProvider.java Outdated
@@ -27,6 +28,24 @@ public CliTokenSource tokenSourceFor(DatabricksConfig config, String resource) {
return new CliTokenSource(cmd, "tokenType", "accessToken", "expiresOn", config::getAllEnv);
}

@Override
public CliTokenSource tokenSourceFor(
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to somehow unify this with the other tokenSourceFor method. Not only do they share their implementation, but they probably must share their implementation (i.e. changes made to one will likely need to be made to the other in the future).

hectorcast-db
hectorcast-db commented Aug 31, 2023
View reviewed changes
databricks-sdk-java/src/main/java/com/databricks/sdk/core/AzureCliCredentialsProvider.java
try {
mgmtTokenSource.getToken();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now get the token inside the "tokenSourceFor" function.

@hectorcast-db hectorcast-db requested a review from mgyucht August 31, 2023 12:19
mgyucht
mgyucht approved these changes Sep 5, 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.

LGTM!

@mgyucht mgyucht added this pull request to the merge queue Sep 5, 2023
@mgyucht mgyucht removed this pull request from the merge queue due to a manual request Sep 5, 2023
@mgyucht
Copy link
Contributor

mgyucht commented Sep 5, 2023

Let's merge this after the 0.8.1 release.

@hectorcast-db hectorcast-db added this pull request to the merge queue Sep 8, 2023
Merged via the queue into main with commit a96dc34 Sep 8, 2023
9 checks passed
@hectorcast-db hectorcast-db deleted the DECO-2483 branch September 8, 2023 08:41
tanmay-db added a commit that referenced this pull request Sep 11, 2023
@tanmay-db
Release v0.8.1
7645645 
* Fix Files API integration test ([#150](#150)).
* [DECO-2483] Handle Azure authentication when WorkspaceResourceID is provided ([#145](#145)).
@tanmay-db tanmay-db mentioned this pull request Sep 11, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Sep 11, 2023
@tanmay-db
Release v0.8.1 (#152)
0337b77 
* Added support for Azure authentication when WorkspaceResourceID is
provided
([#145](#145)).
* Fixed Files API integration test
([#150](#150)).

Integration tests passed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
2 participants
@hectorcast-db @mgyucht