Skip to content

mgmt, sql , fix live test failures#49163

Open
v-huizhu2 wants to merge 14 commits into
Azure:mainfrom
v-huizhu2:java_mgmt_sql
Open

mgmt, sql , fix live test failures#49163
v-huizhu2 wants to merge 14 commits into
Azure:mainfrom
v-huizhu2:java_mgmt_sql

Conversation

@v-huizhu2
Copy link
Copy Markdown
Member

Introduces a method to enable Microsoft Entra-only authentication when creating SQL Servers, addressing scenarios where policies require disabling SQL authentication. Updates tests and utilities to use Entra admin exclusively, improving compliance with platform security requirements.

Description

#49070

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

Introduces a method to enable Microsoft Entra-only authentication when creating SQL Servers, addressing scenarios where policies require disabling SQL authentication. Updates tests and utilities to use Entra admin exclusively, improving compliance with platform security requirements.
@github-actions github-actions Bot added the Mgmt This issue is related to a management-plane library. label May 13, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For general region, we could make it a field property and reference it in each test, in case we need to update it again in future recording tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Region as field property — Done. Extracted DEFAULT_REGION, SECONDARY_REGION, TERTIARY_REGION as private static final constants at class level.

Comment on lines 895 to 897
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the accountKey if SAS is disabled?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the accountKey if SAS is disabled? — That's exactly why the threat detection policy test is now commented out. The AAD-only tenant requires disableSharedKeyAccess(), which makes getKeys() return keys the data plane won't accept. The securityAlertPolicies API has no Managed Identity option (unlike auditingSettings), so there's no working auth path. The import/export test was rewritten to use withManagedIdentity(uamiResourceId) instead.

Comment on lines 67 to 86
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seems not needed?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seems not needed?" (adminLogin/adminSid in SqlServerTest) — Correct, removed. Each test now calls azureCliSignedInUser() directly to get the AzureUser, making the adminLogin()/adminSid() helpers and their caching logic unnecessary.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this due to concurrent test running? If so, we could enhance the azureCliSignedInUser method, to cache the AzureUser instance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Is this due to concurrent test running? Cache azureCliSignedInUser?" — The azureCliSignedInUser() method is inherited from ResourceManagerTestProxyTestBase. Each test method calls it independently; tests don't run concurrently within a single class (JUnit 5 default). If caching is desired for performance, it can be done in the base class. Current usage is simple and correct without caching.

v-huizhu2 added 5 commits May 15, 2026 21:32
Enables user-assigned managed identity authentication for SQL database import/export operations, allowing secure access to storage accounts when shared-key authentication is disabled. Updates public APIs, implementation, and tests to support this authentication method. Also refines Entra-only authentication setup, streamlining interface and test usage.

Relates to customer scenarios with storage accounts enforcing identity-based access.
Cleans up formatting by deleting a redundant empty line,
improving code readability and maintaining style consistency.
Removes unnecessary line breaks and indentation in several interface
method and type declarations to improve code readability and
maintain consistency across model definitions.
Allows user-assigned managed identity authentication for SQL database import, export, and creation stages in the fluent API, clarifying that these enhancements impact only SDK-implemented interfaces and do not break user implementations.
Comment thread eng/lintingconfigs/revapi/track2/revapi.json Outdated
@XiaofeiCao XiaofeiCao marked this pull request as ready for review May 29, 2026 11:35
Copilot AI review requested due to automatic review settings May 29, 2026 11:35
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for creating Azure SQL Servers with Microsoft Entra-only authentication and for using user-assigned managed identities (UAMI) in database import/export, so live tests can comply with policies that disallow SQL admin login/password. Existing live tests are migrated to use the signed-in Entra user (or a UAMI) instead of withAdministratorLogin/withAdministratorPassword, and a few unrelated quota/region tweaks make the tests less flaky.

Changes:

  • New SqlServer definition surface: withAzureActiveDirectoryOnlyAuthentication, withExternalActiveDirectoryAdministrator, isAzureActiveDirectoryOnlyAuthenticationEnabled, and withPrimaryUserAssignedManagedServiceIdentity; Definition chain rewired so AAD-only auth is reachable from WithAdministratorLogin.
  • New AuthenticationType.MANAGED_IDENTITY plus withManagedIdentity on SqlDatabaseImportRequest, SqlDatabaseExportRequest, and the import flow on SqlDatabase/SqlDatabaseOperations, with storage MI also propagated in export and new-database import helpers; storage helpers no longer overwrite an MI-based storage key when the account has shared key access disabled.
  • Tests migrated to Entra-only auth (using azureCliSignedInUser() and a UAMI), with new MSI/Authorization test dependencies, region constants, smaller SKUs, and a more robust replication failover wait; the legacy threat-detection test is disabled.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/SqlServer.java Adds new AAD-only/external admin/UAMI stages and rewires the Definition chain.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/implementation/SqlServerImpl.java Implements AAD-only auth, external Entra admin, primary UAMI, and isAzureActiveDirectoryOnlyAuthenticationEnabled.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/AuthenticationType.java Adds MANAGED_IDENTITY enum value.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/SqlDatabaseImportRequest.java Adds default withManagedIdentity stage method.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/SqlDatabaseExportRequest.java Adds default withManagedIdentity stage method.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/SqlDatabaseOperations.java Adds withManagedIdentity on new-database import auth stages.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/models/SqlDatabase.java Adds withManagedIdentity on nested database-in-server import auth stages.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/implementation/SqlDatabaseImpl.java Implements MI-based import auth and avoids overwriting MI storage key when shared key access is disabled.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/implementation/SqlDatabaseForElasticPoolImpl.java Delegates withManagedIdentity to the underlying SqlDatabaseImpl.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/implementation/SqlDatabaseImportRequestImpl.java Implements MI-based SQL auth for existing-database import.
sdk/sql/azure-resourcemanager-sql/src/main/java/com/azure/resourcemanager/sql/implementation/SqlDatabaseExportRequestImpl.java Implements MI-based SQL+storage auth for export, with shared-key guard.
sdk/sql/azure-resourcemanager-sql/src/test/java/com/azure/resourcemanager/sql/SqlServerTest.java Wires MsiManager and AuthorizationManager for tests.
sdk/sql/azure-resourcemanager-sql/src/test/java/com/azure/resourcemanager/sql/SqlServerOperationsTests.java Migrates tests to Entra auth/UAMI, centralizes regions, tweaks SKUs, hardens replication wait, disables legacy ATP test.
sdk/sql/azure-resourcemanager-sql/pom.xml Adds test-scope MSI/Authorization deps and --add-reads for the test module.
sdk/sql/azure-resourcemanager-sql/CHANGELOG.md Documents the new AAD-only and managed-identity features.
sdk/sql/azure-resourcemanager-sql/assets.json Updates the recorded test assets tag.

Comment on lines +113 to +114
.withAzureActiveDirectoryOnlyAuthentication()
.withExternalActiveDirectoryAdministrator(user.userPrincipalName(), user.id(), PrincipalType.USER)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent reminds me that we still use the name/pass for database in tests
https://github.com/v-huizhu2/azure-sdk-for-java/blob/aa51b4834956dc215d30a410d49b9cdc643e4241/sdk/sql/azure-resourcemanager-sql/src/test/java/com/azure/resourcemanager/sql/SqlServerOperationsTests.java#L134-L135

We may need to add a comment to the test, that the real sql data sync require name/pass.

https://learn.microsoft.com/en-us/azure/azure-sql/database/sql-data-sync-data-sql-server-sql-database?view=azuresql

SQL Data Sync requires SQL authentication for connections to the hub and member databases. Microsoft Entra (Azure AD) authentication isn't supported by SQL Data Sync. Because SQL authentication relies on static passwords, it doesn't benefit from modern protections like multifactor authentication (MFA), Conditional Access, or managed identities. This can increase exposure for the entire SQL instance to credential theft, brute‑force attacks, and operational overhead for password rotation and policy enforcement. Where possible, prefer solutions that support Microsoft Entra authentication or managed identities. Since SQL Data Sync is scheduled for retirement, migrate to an alternative that aligns with your organization's security standards.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Seems SQL Data Sync will be retired in 2027, logged issue to deprecate and replace: #49343

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I wasn't aware that they are deprecating.

}

@Override
public SqlDatabaseImportRequestImpl withManagedIdentity(String managedIdentityResourceId) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agent seems correct:

  • storage side → storageKeyType(MANAGED_IDENTITY) and storageKey(managedIdentityResourceId)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, missed this one. Seems this method doesn't have any test cases for it (exports in ExportRequest, and import in SqlDatabase interface). Let me ask agent to add one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Also did a simple fix for a potential bug with sami and uami setting order. We didn't expose MI update, so I didn't include the whole logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Mgmt This issue is related to a management-plane library.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants