Azure client upgrade to allow identity options #15287

georgew5656 · 2023-10-31T15:49:53Z

Description

Druid does not support various forms of auth that Azure Storage Accounts has added support for, like Service Client auth, Workload Identity Auth (e.g. for AKS) or managed identity Auth. Adding the AzureDefaultCredential chain to the azure clients druid uses allows support for configuring these things. See: https://learn.microsoft.com/en-us/java/api/overview/azure/identity-readme?view=azure-java-stable

In order to support this new identity library, I had to upgrade the druid azure client libraries for blob storage (they were very old and deprecated anyways).

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Many of the changes from the client upgrade were pretty straightforward (I used this .NET docs a lot: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/storage/Azure.Storage.Blobs/AzureStorageNetMigrationV12.md#class-conversion-reference)
Most of these changes are in AzureStorage
Less straightforward changes
The client supports retrying requests by default, so I removed instances of RetryUtils in favor of using the client directly. There are several places where druid uses a different retry configuration for different azure operations, so I had to add AzureClientFactory to support creating additional clients on the fly (because the library doesn't let you override the retry policy on existing clients). I also updated AzureStorage to reuse these per-container overrided clients, the new client is stateless so it is technically fine to just recreate a client each time but i figured it would be better to reuse them as a performance optimization. A couple of the tests were removed because they were testing this retry functionality
I had to change the AzureCloudBlobIterator because the new client supports pagination by default (it only fetches additional pages if you request additional items)

Release note

Upgrade druid azure client libraries
Support AzureDefaultCredential for druid auth to azure services
I removed some of the mocks from the tests and added a test implementation (TestPagedResponse). This let me remove the ListBlobItemHolder class that was mostly there for testing.

Key changed/added classes in this PR

AzureClientFactory
AzureAccountConfig
AzureCloudBlobIterator
AzureStorage
AzureStorageDruidModule

This PR has:

I have tested the following scenarios for auth
SAS token auth
Key auth
Workload identity auth (using a k8s serviceAccount and managed identity to mount credentials for druid running on a pod in AKS)
Environment variable auth (using a App registration client id/secret)
Managed identity auth (I technically have not tested this directly on a VM, but I am pretty sure this will work on a VM with a managed identity by setting the managedIdentityClientId property https://github.com/Azure/azure-sdk-for-java/wiki/Azure-Identity-Examples#authenticating-in-azure-with-managed-identity)
Technically the AzureDefaultCredential supports more types of auth but I did not test any of them.

I also tested the following migration scenarios
Cluster running old azure extension using a sas token -> new azure extension using a sas token
Sas token (new client) -> workload credential auth (new client)

For actual functionality I tested all the basic druid functionality (ingesting data from azure, creating and loading segments, managing task logs, killing segments), but I didn't test any of the retry logic that the new clients promise.

…tUpgrade

Co-authored-by: 317brian <[email protected]>

…o azureClientUpgrade

georgew5656 · 2023-11-29T22:07:04Z

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java


  public AzureStorage(
-      Supplier<CloudBlobClient> cloudBlobClient
+      Supplier<BlobServiceClient> blobServiceClient,


i think i can definitely just remove this supplier and lazily call factory.create instead, but i would prefer to make that as a followup change.

georgew5656 · 2023-12-08T14:09:26Z

...-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureTaskLogsConfig.java

@@ -37,19 +36,14 @@ public class AzureTaskLogsConfig
  @NotNull
  private String prefix = null;

-  @JsonProperty
-  @Min(1)
-  private int maxTries = 3;


i don't really see this config documented anywhere and its not supported with S3TaskLogs or GoogleTaskLogs so i just removed it

YongGang · 2023-12-11T05:55:05Z

extensions-core/azure-extensions/pom.xml

+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-blob</artifactId>
+            <version>12.24.0</version>
+        </dependency>
+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-blob-batch</artifactId>
+            <version>12.20.0</version>
+        </dependency>
+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-common</artifactId>
+            <version>12.23.0</version>
+        </dependency>


Should these dependencies be the same version?

unfortunately azure does not sync these versions

https://learn.microsoft.com/en-us/azure/developer/java/sdk/get-started-maven#use-the-azure-sdk-for-java-build-tool - have you looked into adding this to see if the azure libraries are added correctly?

suneet-s · 2024-01-02T16:22:50Z

extensions-core/azure-extensions/pom.xml

+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-blob</artifactId>
+            <version>12.24.0</version>
+        </dependency>
+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-blob-batch</artifactId>
+            <version>12.20.0</version>
+        </dependency>
+        <dependency>
+            <groupId>com.azure</groupId>
+            <artifactId>azure-storage-common</artifactId>
+            <version>12.23.0</version>
+        </dependency>


https://learn.microsoft.com/en-us/azure/developer/java/sdk/get-started-maven#use-the-azure-sdk-for-java-build-tool - have you looked into adding this to see if the azure libraries are added correctly?

suneet-s · 2024-01-02T16:27:27Z

...s-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureAccountConfig.java

@@ -46,6 +46,12 @@ public class AzureAccountConfig
  @JsonProperty
  private String sharedAccessStorageToken;

+  @JsonProperty
+  private String managedIdentityClientId;


Can we add validation to error out if the user sets this without setting useAzureCredentialsChain = true as called out in the docs

suneet-s · 2024-01-02T16:33:58Z

...s-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureClientFactory.java

+   * @param retryCount number of retries
+   * @return BlobServiceClient with a custom retryCount
+   */
+  public BlobServiceClient getRetriableBlobServiceClient(@Nonnull Integer retryCount)


Is there a reason for this factory to return both a retriable and non-retriable client?

explained this in a below comment

suneet-s · 2024-01-02T16:41:30Z

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java

-  @Nullable
-  private BlobRequestOptions getRequestOptionsWithRetry(Integer maxAttempts)
+  @VisibleForTesting
+  BlobServiceClient getRetriableBlobServiceClient(@Nonnull Integer maxAttempts)


Suggested change

BlobServiceClient getRetriableBlobServiceClient(@Nonnull Integer maxAttempts)

BlobServiceClient getRetriableBlobServiceClient(int maxAttempts)

suneet-s · 2024-01-02T16:44:14Z

extensions-core/azure-extensions/src/main/java/org/apache/druid/storage/azure/AzureStorage.java

  {
-    if (maxAttempts == null) {
-      return null;
+    // To avoid keeping a ton of clients in memory, if maxAttempts is specified use a client with at least that many retries configured.


This seems a little confusing - why would we want max attempts to mean "at least retry attempts".

Also, if this is the desired behavior, we can push this logic into the factory instead of having it in the storage class

this change is mainly to balance backwards compatibility with performance. the retry logic in the extension is such that some requests don't specify any retries (the default behavior in the previous client was to not retry at all), and some requests specify retries (these can come from different configs). the new azure client doesn't let you override the retry configuration for a client, so in the interest of not either 1. creating a client every time we make a request, 2. keeping a bunch of different clients cached in memory, i added the "at least retry attempts" logic, this way we can keep 2 clients around only

suneet-s

🚢

* Include new dependencies * Mostly implemented * More azure fixes * Tests passing * Unit tests running * Test running after removing storage exception * Happy with coverage now * Add more tests * fix client factory * cleanup from testing * Remove old client * update docs * Exclude from spellcheck * Add licenses * Fix identity version * Save work * Add azure clients * add licenses * typos * Add dependencies * Exception is not thrown * Fix intellij check * Don't need to override * specify length * urldecode * encode path * Fix checks * Revert urlencode changes * Urlencode with azure library * Update docs/development/extensions-core/azure.md Co-authored-by: Abhishek Agarwal <[email protected]> * PR changes * Update docs/development/extensions-core/azure.md Co-authored-by: 317brian <[email protected]> * Deprecate AzureTaskLogsConfig.maxRetries * Clean up azure retry block * logic update to reuse clients * fix comments * Create container conditionally * Fix key auth * Remove container client logic * Add some more testing * Update comments * Add a comment explaining client reuse * Move logic to factory class * use bom for dependency management * fix license versions --------- Co-authored-by: Abhishek Agarwal <[email protected]> Co-authored-by: 317brian <[email protected]>

georgew5656 added 12 commits October 18, 2023 17:16

Include new dependencies

a77a257

Mostly implemented

17e7074

More azure fixes

5aa991f

Tests passing

dfe948b

Unit tests running

02343d4

Test running after removing storage exception

70c6cf6

Happy with coverage now

d9b28eb

Add more tests

983ff52

fix client factory

9aaf451

cleanup from testing

6a99496

Remove old client

ddcfc03

update docs

305eb85

github-actions bot added Area - Documentation Area - Dependencies labels Oct 31, 2023

georgew5656 added 5 commits October 31, 2023 12:58

Exclude from spellcheck

9e84ade

Add licenses

fae6fa1

Fix identity version

c2a82fe

Save work

43e81c5

Add azure clients

9812a67

georgew5656 requested review from abhishekagarwal87, suneet-s, LakshSingla and cryptoe and removed request for abhishekagarwal87 November 2, 2023 16:24

georgew5656 added 6 commits November 2, 2023 13:38

Merge branch 'master' of github.com:georgew5656/druid into azureClien…

28cbf0a

…tUpgrade

fix merge conflict

12ab481

add licenses

0f80541

typos

405310c

Add dependencies

7da2f4d

Merge branch 'master' of github.com:georgew5656/druid into azureClien…

13dd4d2

…tUpgrade

georgew5656 and others added 8 commits November 15, 2023 16:45

Update docs/development/extensions-core/azure.md

8133e1d

Co-authored-by: 317brian <[email protected]>

Deprecate AzureTaskLogsConfig.maxRetries

fe5758f

Merge branch 'azureClientUpgrade' of github.com:georgew5656/druid int…

92f7535

…o azureClientUpgrade

Clean up azure retry block

a4fa3a2

logic update to reuse clients

4654815

fix comments

be67328

Create container conditionally

292ff42

Fix key auth

3dfabb3

georgew5656 commented Nov 29, 2023

View reviewed changes

georgew5656 added 2 commits December 7, 2023 10:57

Remove container client logic

53f7dd7

Add some more testing

daa6179

georgew5656 commented Dec 8, 2023

View reviewed changes

georgew5656 added 2 commits December 8, 2023 09:20

Update comments

9a069ea

Add a comment explaining client reuse

9f04d39

YongGang reviewed Dec 11, 2023

View reviewed changes

suneet-s reviewed Jan 2, 2024

View reviewed changes

georgew5656 added 2 commits January 3, 2024 12:16

Move logic to factory class

6899704

use bom for dependency management

907c4e9

georgew5656 requested review from YongGang and suneet-s January 3, 2024 20:13

fix license versions

8d2d3c0

suneet-s approved these changes Jan 3, 2024

View reviewed changes

georgew5656 merged commit 8e95cea into apache:master Jan 3, 2024
83 checks passed

LakshSingla added this to the 29.0.0 milestone Jan 29, 2024

LakshSingla mentioned this pull request Feb 13, 2024

[DRAFT] 29.0.0 release notes #15896

Closed

cryptoe mentioned this pull request May 31, 2024

Fix race in AzureClient factory fetch #16525

Merged

pagrawal10 mentioned this pull request Jun 1, 2024

[BACKPORT] Fix CVEs for zookeeper, nimbus-jose-jwt and netty4 confluentinc/druid#204

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure client upgrade to allow identity options #15287

Azure client upgrade to allow identity options #15287

georgew5656 commented Oct 31, 2023 •

edited

Loading

georgew5656 Nov 29, 2023

georgew5656 Dec 8, 2023

YongGang Dec 11, 2023

georgew5656 Dec 18, 2023 •

edited

Loading

suneet-s Jan 2, 2024

suneet-s Jan 2, 2024

suneet-s Jan 2, 2024

suneet-s Jan 2, 2024

georgew5656 Jan 2, 2024

suneet-s Jan 2, 2024 •

edited

Loading

suneet-s Jan 2, 2024

georgew5656 Jan 2, 2024 •

edited

Loading

suneet-s left a comment

	BlobServiceClient getRetriableBlobServiceClient(@Nonnull Integer maxAttempts)
	BlobServiceClient getRetriableBlobServiceClient(int maxAttempts)

Azure client upgrade to allow identity options #15287

Azure client upgrade to allow identity options #15287

Conversation

georgew5656 commented Oct 31, 2023 • edited Loading

Description

Fixed the bug ...

Renamed the class ...

Added a forbidden-apis entry ...

Release note

Key changed/added classes in this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgew5656 Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suneet-s Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgew5656 Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

suneet-s left a comment

Choose a reason for hiding this comment

georgew5656 commented Oct 31, 2023 •

edited

Loading

georgew5656 Dec 18, 2023 •

edited

Loading

suneet-s Jan 2, 2024 •

edited

Loading

georgew5656 Jan 2, 2024 •

edited

Loading