feat(csharp/src/Drivers/Databricks): Implement ClientCredentialsProvider#2743
Conversation
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...
96bdd44 to
efe06c2
Compare
test bug fix
This reverts commit 6a899a9.
8c66a9c to
1d3edf8
Compare
1d3edf8 to
ae6bb29
Compare
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"```
Implements OAuth 2.0 Client Credentials flow for machine-to-machine (M2M) authentication in the Databricks ADBC driver.
This is the first PR in a series to add complete OAuth client credentials support. This PR focuses on the core token
provider infrastructure.
Changes
OAuthClientCredentialsProviderclass: Handles OAuth token lifecycleOAuthClientCredentialsProviderTests:Testing
Unit Tests
Run the OAuth client credentials tests: