feat(csharp/src/Drivers/Databricks): Implement ClientCredentialsProvider #2743
Conversation
toddmeng-db
changed the title
toddmeng-db
force-pushed
the
toddmeng-db/client-creds-1
branch
from
0e1afe3 to
217c58a
Compare
| /// <summary> | ||
| /// Service for obtaining OAuth access tokens using the client credentials grant type. | ||
| /// </summary> | ||
| internal class OAuthClientCredentialsService : IDisposable |
There was a problem hiding this comment.
Can we rename this, it is not a service maybe call it a provider
| /// <param name="cancellationToken">A cancellation token to cancel the operation.</param> | ||
| /// <returns>The access token.</returns> | ||
| /// <exception cref="DatabricksException">Thrown when the token request fails or the response is invalid.</exception> | ||
| public async Task<string> GetAccessTokenAsync(CancellationToken cancellationToken = default) |
There was a problem hiding this comment.
Why do we need it to be async? This is get credentials, which should be blocking?
There was a problem hiding this comment.
Seems like it's a bit awkward to implement a non-async method when there's an http call involved in c#?
public Credentials GetCredentials()
{
var response = Task.Run(() => _httpClient.GetAsync(...))
.Unwrap()
.GetAwaiter()
.GetResult();
// process response...
return credentials;
}
There was a problem hiding this comment.
Yeah, I guess we need to take a look at the callsite, maybe check SparkHttpConnection to see how we pass those token as the header, I believe those functions are all sync functions, which means it should call sync functions as well.
Maybe we should just expose a sync version of GetAccessToken() which return GetAccessTokenAsync().GetAwaiter().GetResult();
There was a problem hiding this comment.
Just curious, what's the concern with using async here?
There was a problem hiding this comment.
There is no concern of using async, but c# appears need to follow a pattern than only async function can call async function. If you integrate with the rest of the driver you can see all the callsite is not async functions. So you must somehow return some results immediately, or else you will only get the Task.
There was a problem hiding this comment.
I see, I noticed that too...
| { | ||
| // For workspace URLs, the token endpoint is always /oidc/v1/token | ||
| // TODO: Might be different for Azure AAD SPs | ||
| return $"{_baseUri.Scheme}://{_baseUri.Host}/oidc/v1/token"; |
There was a problem hiding this comment.
The passed in url is just a host without scheme, you probably will need to hard code https://
toddmeng-db
force-pushed
the
toddmeng-db/client-creds-1
branch
from
96bdd44 to
efe06c2
Compare
CurtHagenlocher
changed the title
toddmeng-db
changed the title
csharp/src/Drivers/Databricks/Auth/OAuthClientCredentialsProvider.cs
Outdated
Show resolved
Hide resolved
| if (!string.IsNullOrEmpty(_tenantId)) | ||
| { | ||
| // Use the tenant-specific Azure OIDC token endpoint | ||
| return $"https://login.microsoftonline.com/{_tenantId}/oauth2/v2.0/token"; |
There was a problem hiding this comment.
This feature should only be used for Databricks Service Principal.
For Azure Service Principal, PBI is building support for it in another way we do not need here.
test bug fix
This reverts commit 6a899a9.
toddmeng-db
force-pushed
the
toddmeng-db/client-creds-1
branch
4 times, most recently
from
8c66a9c to
1d3edf8
Compare
toddmeng-db
force-pushed
the
toddmeng-db/client-creds-1
branch
from
1d3edf8 to
ae6bb29
Compare
CurtHagenlocher
left a comment
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Thanks! I take it there will be a followup where this is hooked up to the driver?
…der (apache#2743) First PR for Class to get token via oauth service for M2M authentication. Includes simple expiration and refresh logic. SDK refresh logic [here](https://github.com/databricks/databricks-sdk-java/blob/main/databricks-sdk-java/src/main/java/com/databricks/sdk/core/oauth/RefreshableTokenSource.java) Follow up is to integrate with the rest of the driver To test out: 1. Create a `databricks_test_config.json` ``` { "oauth_client_id": "...", "oauth_client_secret": "...", "host": "databricks....com" // workspace hostname } ``` 2. On macOS/Linux export DATABRICKS_TEST_CONFIG_FILE=/path/to/your/databricks_test_config.json On Windows PowerShell $env:DATABRICKS_TEST_CONFIG_FILE = "C:\path\to\your\databricks_test_config.json" 3. ```/csharp% dotnet test --filter "FullyQualifiedName~OAuthClientCredentialsServiceTests"```
3 participants
First PR for Class to get token via oauth service for M2M authentication. Includes simple expiration and refresh logic.
Follow up is to integrate with the rest of the driver
To test out:
databricks_test_config.jsonOn macOS/Linux
export DATABRICKS_TEST_CONFIG_FILE=/path/to/your/databricks_test_config.json
On Windows PowerShell
$env:DATABRICKS_TEST_CONFIG_FILE = "C:\path\to\your\databricks_test_config.json"
/csharp% dotnet test --filter "FullyQualifiedName~OAuthClientCredentialsServiceTests"