-
Notifications
You must be signed in to change notification settings - Fork 2.1k
[New API] Tables Sovereign Cloud Support #45246
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
base: main
Are you sure you want to change the base?
Conversation
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
API change check APIView has identified API level changes in this PR and created following API reviews. |
@@ -744,4 +750,17 @@ public TableClientBuilder enableTenantDiscovery() { | |||
|
|||
return this; | |||
} | |||
|
|||
/** | |||
* Sets the {@link TableAudience} to use when authenticating with the Table Service. |
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.
nit;
* Sets the {@link TableAudience} to use when authenticating with the Table Service. | |
* Sets the {@link TableAudience} to use when authenticating with the Azure Tables service. |
* Sets the {@link TableAudience audience} for the Azure Table service. | ||
* | ||
* @param audience The {@link TableAudience audience} for the Azure Table service. |
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.
nit;
* Sets the {@link TableAudience audience} for the Azure Table service. | |
* | |
* @param audience The {@link TableAudience audience} for the Azure Table service. | |
* Sets the {@link TableAudience audience} for the Azure Tables service. | |
* | |
* @param audience The {@link TableAudience audience} for the Azure Tables service. |
import java.net.URISyntaxException; | ||
|
||
/** | ||
* Defines the audience for the Azure Table service. |
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.
nit;
* Defines the audience for the Azure Table service. | |
* Defines the audience for the Azure Tables service. |
* The audience can be one of the following: | ||
* <ul> | ||
* <li>AZURE_STORAGE_PUBLIC_CLOUD</li> | ||
* <li>AZURE_STORAGE_CHINAD</li> |
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.
nit; Typo
* <li>AZURE_STORAGE_CHINAD</li> | |
* <li>AZURE_STORAGE_CHINA</li> |
*/ | ||
public static final TableAudience AZURE_COSMOS_CHINA = fromString("https://cosmos.azure.cn"); | ||
/** | ||
* The audience for the Azure Cosmos service in the US government. |
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.
* The audience for the Azure Cosmos service in the US government. | |
* The audience for the Azure Cosmos service in the US government cloud. |
public static final TableAudience AZURE_STORAGE_CHINA = fromString("https://storage.azure.cn"); | ||
|
||
/** | ||
* The audience for the Azure Storage service in the US government. |
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.
* The audience for the Azure Storage service in the US government. | |
* The audience for the Azure Storage service in the US government cloud. |
/** | ||
* @deprecated The audience is for the public. | ||
*/ | ||
@Deprecated | ||
public TableAudience() { | ||
// This constructor is deprecated and should not be used. | ||
} |
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 need a public constructor when using an ExpandableStringEnum
? Can we make it private or package-private instead? It just feels weird adding a new API and have it become deprecated at the same time.
perRetryAdditionalPolicies, configuration, logger, enableTenantDiscovery, null); | ||
} | ||
|
||
static HttpPipeline buildPipeline(AzureNamedKeyCredential azureNamedKeyCredential, |
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 need to add a new overload for the method above? Since it's package-private, we can add the new parameter to it.
audience = (audience != null) | ||
? audience | ||
: (TableUtils.isCosmosEndpoint(endpoint) | ||
? TableAudience.AZURE_COSMOS_PUBLIC_CLOUD | ||
: TableAudience.AZURE_STORAGE_PUBLIC_CLOUD); |
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.
For easier readability, let's not use ternary in ternary operators.
@@ -218,6 +220,10 @@ public TableClientBuilder() { | |||
public TableClient buildClient() { | |||
TableServiceVersion serviceVersion = version != null ? version : TableServiceVersion.getLatest(); | |||
|
|||
if (audience == null) { | |||
audience = TableAudience.AZURE_STORAGE_PUBLIC_CLOUD; |
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.
This is modifying the builder itself, which isn't what we want when creating the client.
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.
Also, is defaulting to Storage public cloud scope the correct choice here? If it is, please add comments explaining why this is the right choice. As the other client builder doesn't add in a default audience
.
} | ||
|
||
@Test | ||
public void testCustomAudience() { |
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.
Please make these parameterized tests where the arguments as the input string for fromString
and the expected value.
Testing like this can be painful if a change breaks a few cases as you'll need to run and fail multiple times to find all cases that broke. A parameterized test will tell you every case that breaks.
*/ | ||
public static final TableAudience AZURE_COSMOS_US_GOVERNMENT = fromString("https://cosmos.azure.us"); | ||
|
||
private final ClientLogger logger = new ClientLogger(TableAudience.class); |
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.
Make this a constant, otherwise a new ClientLogger will be instantiated every time a new instance is created.
This pull request introduces the
TableAudience
class to define and manage audiences for Azure Table services, allowing for more flexible authentication and endpoint configuration. It also updates theTableClientBuilder
andTableServiceClientBuilder
to support this new functionality. Below is a summary of the most important changes:New Feature:
TableAudience
ClassTableAudience
class to define audiences for Azure Table services, including predefined audiences for Azure Storage and Cosmos services in public cloud, China, and US government environments. It also provides a method to generate default scopes for authentication. (sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/models/TableAudience.java
)Builder Enhancements
TableClientBuilder
andTableServiceClientBuilder
to include anaudience
property, allowing users to specify the audience during client configuration. Added a newaudience(TableAudience audience)
method to set this property. (sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/TableClientBuilder.java
,sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/TableServiceClientBuilder.java
) [1] [2] [3] [4]Pipeline Updates
buildPipeline
method inBuilderHelper
to accept aTableAudience
parameter and determine the appropriate default scope based on the audience. (sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/BuilderHelper.java
) [1] [2]Constants Simplification
COSMOS_SCOPE
,STORAGE_SCOPE
) with a genericDEFAULT_SCOPE
inTablesConstants
. (sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/implementation/TablesConstants.java
,sdk/tables/azure-data-tables/src/main/java/com/azure/data/tables/implementation/StorageConstants.java
) [1] [2]Testing
TableAudienceTests
to validate the behavior of theTableAudience
class, including predefined audiences and custom audience handling. (sdk/tables/azure-data-tables/src/test/java/com/azure/data/tables/TableAudienceTests.java
)