-
Notifications
You must be signed in to change notification settings - Fork 33
Infer azure tenant ID. #482
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
Changes from 3 commits
9c7ae67
56859bf
312c32a
f06c006
cf281fd
9f9f7a1
22adeed
21ba14f
0bb96a2
ea7ebc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,10 +14,15 @@ | |
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.lang.reflect.Field; | ||
| import java.net.URL; | ||
| import java.util.*; | ||
| import org.apache.http.HttpMessage; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| public class DatabricksConfig { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(DatabricksConfig.class); | ||
| private CredentialsProvider credentialsProvider = new DefaultCredentialsProvider(); | ||
|
|
||
| @ConfigAttribute(env = "DATABRICKS_HOST") | ||
|
|
@@ -726,7 +731,7 @@ private DatabricksConfig clone(Set<String> fieldsToSkip) { | |
| } | ||
|
|
||
| public DatabricksConfig clone() { | ||
| return clone(new HashSet<>()); | ||
| return clone(new HashSet<>(Collections.singletonList("logger"))); | ||
|
||
| } | ||
|
|
||
| public DatabricksConfig newWithWorkspaceHost(String host) { | ||
|
|
@@ -736,6 +741,7 @@ public DatabricksConfig newWithWorkspaceHost(String host) { | |
| // The config for WorkspaceClient has a different host and Azure Workspace resource | ||
| // ID, and also omits | ||
| // the account ID. | ||
| "logger", | ||
| "host", | ||
| "accountId", | ||
| "azureWorkspaceResourceId", | ||
|
|
@@ -755,4 +761,81 @@ public DatabricksConfig newWithWorkspaceHost(String host) { | |
| public String getEffectiveOAuthRedirectUrl() { | ||
| return redirectUrl != null ? redirectUrl : "http://localhost:8080/callback"; | ||
| } | ||
|
|
||
| /** | ||
| * [Internal] Load the Azure tenant ID from the Azure Databricks login page. If the tenant ID is | ||
| * already set, this method does nothing. | ||
| */ | ||
| public void loadAzureTenantId() { | ||
sreekanth-db marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (!isAzure() || azureTenantId != null || host == null) { | ||
| return; | ||
| } | ||
|
|
||
| final String azureAuthEndpoint = "/aad/auth"; | ||
sreekanth-db marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| String loginUrl = host + azureAuthEndpoint; | ||
| logger.debug("Loading tenant ID from {}", loginUrl); | ||
|
|
||
| try { | ||
| String redirectLocation = getRedirectLocation(loginUrl); | ||
| if (redirectLocation == null) { | ||
| return; | ||
| } | ||
|
|
||
| String extractedTenantId = extractTenantIdFromUrl(redirectLocation); | ||
| if (extractedTenantId == null) { | ||
| return; | ||
| } | ||
|
|
||
| this.azureTenantId = extractedTenantId; | ||
| logger.debug("Loaded tenant ID: {}", this.azureTenantId); | ||
|
|
||
| } catch (Exception e) { | ||
| logger.warn("Failed to load tenant ID: {}", e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private String getRedirectLocation(String loginUrl) throws IOException { | ||
|
|
||
| Request request = new Request("GET", loginUrl); | ||
| request.setRedirectionBehavior(false); | ||
| Response response = getHttpClient().execute(request); | ||
| int statusCode = response.getStatusCode(); | ||
|
|
||
| if (statusCode / 100 != 3) { | ||
sreekanth-db marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| logger.warn( | ||
| "Failed to get tenant ID from {}: expected status code 3xx, got {}", | ||
| loginUrl, | ||
| statusCode); | ||
| return null; | ||
| } | ||
|
|
||
| String location = response.getFirstHeader("Location"); | ||
| if (location == null) { | ||
| logger.warn("No Location header in response from {}", loginUrl); | ||
| } | ||
|
|
||
| return location; | ||
| } | ||
|
|
||
| private String extractTenantIdFromUrl(String redirectUrl) { | ||
| try { | ||
| // The Location header has the following form: | ||
| // https://login.microsoftonline.com/<tenant-id>/oauth2/authorize?... | ||
| // The domain may change depending on the Azure cloud (e.g. login.microsoftonline.us for US | ||
| // Government cloud). | ||
| URL entraIdUrl = new URL(redirectUrl); | ||
| String[] pathSegments = entraIdUrl.getPath().split("/"); | ||
|
|
||
| if (pathSegments.length < 2) { | ||
| logger.warn("Invalid path in Location header: {}", entraIdUrl.getPath()); | ||
| return null; | ||
| } | ||
|
|
||
| return pathSegments[1]; | ||
sreekanth-db marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } catch (Exception e) { | ||
| logger.warn("Failed to extract tenant ID from URL {}: {}", redirectUrl, e.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,12 +22,12 @@ public String authType() { | |
| public OAuthHeaderFactory configure(DatabricksConfig config) { | ||
| if (!config.isAzure() | ||
| || config.getAzureClientId() == null | ||
| || config.getAzureClientSecret() == null | ||
| || config.getAzureTenantId() == null) { | ||
| || config.getAzureClientSecret() == null) { | ||
| return null; | ||
| } | ||
| AzureUtils.ensureHostPresent( | ||
| config, mapper, AzureServicePrincipalCredentialsProvider::tokenSourceFor); | ||
| config.loadAzureTenantId(); | ||
|
||
| CachedTokenSource inner = tokenSourceFor(config, config.getEffectiveAzureLoginAppId()); | ||
| CachedTokenSource cloud = | ||
| tokenSourceFor(config, config.getAzureEnvironment().getServiceManagementEndpoint()); | ||
|
|
||
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.
Do we have to clone this method for all our config classes?
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.
Could you elaborate on this comment? Are you asking whether we need to implement similar
clone()methods in other configuration classes, or are you suggesting a different architectural approach for handling object cloning?