-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Azure client upgrade to allow identity options #15287
Changes from 46 commits
a77a257
17e7074
5aa991f
dfe948b
02343d4
70c6cf6
d9b28eb
983ff52
9aaf451
6a99496
ddcfc03
305eb85
9e84ade
fae6fa1
c2a82fe
43e81c5
9812a67
28cbf0a
12ab481
0f80541
405310c
7da2f4d
13dd4d2
0787dbb
295db27
bf74260
260d72f
bb72518
129d3d9
d023f63
2c5d257
423d226
60a7b96
ce79b59
cfff1ee
9ca19e0
8133e1d
fe5758f
92f7535
a4fa3a2
4654815
be67328
292ff42
3dfabb3
53f7dd7
daa6179
9a069ea
9f04d39
6899704
907c4e9
8d2d3c0
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 |
---|---|---|
|
@@ -46,6 +46,12 @@ public class AzureAccountConfig | |
@JsonProperty | ||
private String sharedAccessStorageToken; | ||
|
||
@JsonProperty | ||
private String managedIdentityClientId; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add validation to error out if the user sets this without setting |
||
|
||
@JsonProperty | ||
private Boolean useAzureCredentialsChain = Boolean.FALSE; | ||
|
||
@SuppressWarnings("unused") // Used by Jackson deserialization? | ||
public void setProtocol(String protocol) | ||
{ | ||
|
@@ -94,9 +100,25 @@ public String getSharedAccessStorageToken() | |
return sharedAccessStorageToken; | ||
} | ||
|
||
public Boolean getUseAzureCredentialsChain() | ||
{ | ||
return useAzureCredentialsChain; | ||
} | ||
|
||
public String getManagedIdentityClientId() | ||
{ | ||
return managedIdentityClientId; | ||
} | ||
|
||
|
||
@SuppressWarnings("unused") // Used by Jackson deserialization? | ||
public void setSharedAccessStorageToken(String sharedAccessStorageToken) | ||
{ | ||
this.sharedAccessStorageToken = sharedAccessStorageToken; | ||
} | ||
|
||
public void setUseAzureCredentialsChain(Boolean useAzureCredentialsChain) | ||
{ | ||
this.useAzureCredentialsChain = useAzureCredentialsChain; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
|
||
package org.apache.druid.storage.azure; | ||
|
||
import com.azure.core.http.policy.ExponentialBackoffOptions; | ||
import com.azure.core.http.policy.RetryOptions; | ||
import com.azure.identity.DefaultAzureCredentialBuilder; | ||
import com.azure.storage.blob.BlobServiceClient; | ||
import com.azure.storage.blob.BlobServiceClientBuilder; | ||
import com.azure.storage.common.StorageSharedKeyCredential; | ||
|
||
import javax.annotation.Nonnull; | ||
import java.time.Duration; | ||
|
||
/** | ||
* Factory class for generating BlobServiceClient and BlobContainerClient objects. This is necessary instead of using | ||
* BlobServiceClient.createBlobContainerIfNotExists because sometimes we need different retryOptions on our container | ||
* clients and Azure doesn't let us override this setting on the default BlobServiceClient. | ||
*/ | ||
public class AzureClientFactory | ||
{ | ||
|
||
private final AzureAccountConfig config; | ||
|
||
public AzureClientFactory(AzureAccountConfig config) | ||
{ | ||
this.config = config; | ||
} | ||
|
||
public BlobServiceClient getBlobServiceClient() | ||
{ | ||
return getAuthenticatedBlobServiceClientBuilder().buildClient(); | ||
} | ||
|
||
public BlobServiceClient getRetriableBlobServiceClient(@Nonnull Integer retryCount) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason for this factory to return both a retriable and non-retriable client? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. explained this in a below comment |
||
{ | ||
BlobServiceClientBuilder clientBuilder = getAuthenticatedBlobServiceClientBuilder() | ||
.retryOptions(new RetryOptions( | ||
new ExponentialBackoffOptions() | ||
.setMaxRetries(retryCount) | ||
.setBaseDelay(Duration.ofMillis(1000)) | ||
.setMaxDelay(Duration.ofMillis(60000)) | ||
)); | ||
return clientBuilder.buildClient(); | ||
} | ||
|
||
private BlobServiceClientBuilder getAuthenticatedBlobServiceClientBuilder() | ||
{ | ||
BlobServiceClientBuilder clientBuilder = new BlobServiceClientBuilder() | ||
.endpoint("https://" + config.getAccount() + ".blob.core.windows.net"); | ||
|
||
if (config.getKey() != null) { | ||
clientBuilder.credential(new StorageSharedKeyCredential(config.getAccount(), config.getKey())); | ||
} else if (config.getSharedAccessStorageToken() != null) { | ||
clientBuilder.sasToken(config.getSharedAccessStorageToken()); | ||
} else if (config.getUseAzureCredentialsChain()) { | ||
// We might not use the managed identity client id in the credential chain but we can just set it here and it will no-op. | ||
DefaultAzureCredentialBuilder defaultAzureCredentialBuilder = new DefaultAzureCredentialBuilder() | ||
.managedIdentityClientId(config.getManagedIdentityClientId()); | ||
clientBuilder.credential(defaultAzureCredentialBuilder.build()); | ||
} | ||
return clientBuilder; | ||
} | ||
} |
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.
Should these dependencies be the same version?
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.
unfortunately azure does not sync these versions
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.
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?