Skip to content

SQL server AD identity and data encryption support #3334

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.

Sign up for GitHub

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

Jump to bottom
Merged
shahabhijeet merged 9 commits into from
Jun 12, 2017
Merged

SQL server AD identity and data encryption support #3334

shahabhijeet merged 9 commits into from
Jun 12, 2017

Conversation

@jaredmoo
Copy link
Contributor

@jaredmoo jaredmoo commented Jun 7, 2017 •
edited
Loading

Description

  • Updated Servers api version from 2014-04-01 to 2015-05-01-preview, which is SDK compatible and includes support for server managed identity
  • Added support for server keys and encryption protectors
  • Added support for check server name availability
  • Added support for virtual network firewall rules
  • Updated server azure ad admin from swagger
  • Minor nonfunctional updates to database blob auditing
  • Breaking changes DatabaseMetrics and ServerMetrics renamed to DatabaeUsage and ServerUsage. These were misleadingly named because metrics is a different API.
  • Added database metrics and elastic pool metrics

Note: the server azure ad admin change looks like a breaking change, but the previous version was not yet shipped so it's not breaking.

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • 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.

SDK Generation Guidelines

  • 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.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@jaredmoo
Copy link
Contributor Author

jaredmoo commented Jun 7, 2017

@mihymel FYI

@mihymel
Copy link
Contributor

mihymel commented Jun 7, 2017

@jaredmoo Thanks for the FYI. Looks good to me.

@shahabhijeet
Copy link
Contributor

shahabhijeet commented Jun 7, 2017

@jaredmoo can you add swagger spec PR link to this PR

shahabhijeet
shahabhijeet suggested changes Jun 7, 2017
View reviewed changes
src/SDKs/SqlManagement/Sql.Tests/Sql.Tests.csproj
@@ -9,6 +9,13 @@
<TargetFrameworks>netcoreapp1.1</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Azure.KeyVault" Version="2.0.6" />
Copy link
Contributor

@shahabhijeet shahabhijeet Jun 7, 2017

Choose a reason for hiding this comment

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

@jaredmoo I was under the impression Nathan Fan updated all SQL Tests to use latest Resource Manager nuget.
I see you are using a really old resource manager to record your tests, please update to the latest (>=1.6.0)

Copy link
Contributor Author

@jaredmoo jaredmoo Jun 7, 2017

Choose a reason for hiding this comment

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

Yes, Nathan did that. Where are you seeing the old resource manager?

Copy link
Contributor Author

@jaredmoo jaredmoo Jun 7, 2017

Choose a reason for hiding this comment

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

It look like all the json recordings have Microsoft.Azure.Management.ResourceManager.ResourceManagementClient/1.6.0.0

Copy link
Contributor

@shahabhijeet shahabhijeet Jun 12, 2017

Choose a reason for hiding this comment

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

@jaredmoo So by default every test project get's 1.1.0 reference of Resource manager.
So unless you explicitly add 1.6.0 to the test project you are still using 1.1.0 resource manager nuget
Here is the link for the change
https://github.com/Azure/azure-sdk-for-net/pull/3272/files#diff-c8ac009921fcb73848451401f0d039f2R16

@jaredmoo
Copy link
Contributor Author

jaredmoo commented Jun 7, 2017

There are a few swagger PR combined into this SDK PR:

shahabhijeet
shahabhijeet suggested changes Jun 7, 2017
View reviewed changes
src/SDKs/SqlManagement/Sql.Tests/AzureRmSqlServerActiveDirectoryAdministratorTest.cs
// Add new Active Directory Admin
ServerAzureADAdministrator newAdmin = new ServerAzureADAdministrator("DSEngAll", new Guid("5e90ef3b-9b42-4777-819b-25c36961ea4d"), new Guid("72f988bf-86f1-41af-91ab-2d7cd011db47"), "sqlcrudtest-3421", "2c647056-bab2-4175-b172-493ff049eb29");
ServerAzureADAdministrator createResult = sqlClient.ServerAzureADAdministrators.CreateOrUpdateAzureADAdministrator(resourceGroup.Name, server.Name, newAdmin);
ServerAzureADAdministrator createResult = sqlClient.ServerAzureADAdministrators.CreateOrUpdate(resourceGroup.Name, server.Name, newAdmin);
Copy link
Contributor

@shahabhijeet shahabhijeet Jun 7, 2017

Choose a reason for hiding this comment

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

@jaredmoo why not use TestFramework to get all your custom key-value pairs injected in your tests.
https://github.com/Azure/azure-powershell/blob/preview/documentation/Using-Azure-TestFramework.md#environment--custom

Copy link
Contributor Author

@jaredmoo jaredmoo Jun 7, 2017

Choose a reason for hiding this comment

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

Good point, since we didn't get this feedback on the original PR (#3196) can we fix this as a separate issue?

@jaredmoo
Copy link
Contributor Author

jaredmoo commented Jun 8, 2017

Hi @shahabhijeet , thanks for the review so far. I don't think your comments require immediate action. I have updated again to latest swagger - the metrics changes just got merged earlier this afternoon ( Azure/azure-rest-api-specs#1284 ). I also reverted adding the new VNetFirewallRule SDK support for now; we have written tests but need to re-record them and are having temporary difficulties.

shahabhijeet
shahabhijeet suggested changes Jun 12, 2017
View reviewed changes
src/SDKs/SqlManagement/Sql.Tests/Sql.Tests.csproj
@@ -9,6 +9,13 @@
<TargetFrameworks>netcoreapp1.1</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Azure.KeyVault" Version="2.0.6" />
Copy link
Contributor

@shahabhijeet shahabhijeet Jun 12, 2017

Choose a reason for hiding this comment

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

@jaredmoo So by default every test project get's 1.1.0 reference of Resource manager.
So unless you explicitly add 1.6.0 to the test project you are still using 1.1.0 resource manager nuget
Here is the link for the change
https://github.com/Azure/azure-sdk-for-net/pull/3272/files#diff-c8ac009921fcb73848451401f0d039f2R16

@shahabhijeet shahabhijeet merged commit 87bd249 into Azure:psSdkJson6 Jun 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@shahabhijeet shahabhijeet shahabhijeet approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jaredmoo @mihymel @shahabhijeet @msftclas @nathannfan