SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups #4990

ranisha2 · 2017-11-15T09:06:44Z

Description

This checklist is used to make sure that common guidelines for a pull request are followed. You can find a more complete discussion of PowerShell cmdlet best practices here.

I have read the contribution guidelines.
If changes were made to any cmdlet, the XML help was regenerated using the platyPSHelp module.
If any large changes are made to a service, they are reflected in the respective change log.

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.
The pull request does not introduce breaking changes (unless a major version change occurs in the assembly and module).

Testing Guidelines

Pull request includes test coverage for the included changes.
PowerShell scripts used in tests should do any necessary setup as part of the test or suite setup, and should not use hard-coded values for locations or existing resources.

Cmdlet Signature Guidelines

New cmdlets that make changes or have side effects should implement ShouldProcess and have SupportShouldProcess=true specified in the cmdlet attribute. You can find more information on ShouldProcess here.
Cmdlet specifies OutputType attribute if any output is produced - if the cmdlet produces no output, it should implement a PassThru parameter.

Cmdlet Parameter Guidelines

Parameter types should not expose types from the management library - complex parameter types should be defined in the module.
Complex parameter types are discouraged - a parameter type should be simple types as often as possible. If complex types are used, they should be shallow and easily creatable from a constructor or another cmdlet.
Cmdlet parameter sets should be mutually exclusive - each parameter set must have at least one mandatory parameter not in other parameter sets.

Sync

Remove AUDIT_CHANGE_GROUP from available AuditActionGroups and update markdown files.

markcowl · 2017-11-16T18:38:20Z

src/ResourceManager/Sql/Commands.Sql/Auditing/Model/BaseBlobAuditingPolicyModel.cs

@@ -18,8 +18,7 @@ public enum AuditActionGroups
    {
            BATCH_STARTED_GROUP,
            BATCH_COMPLETED_GROUP,
-            APPLICATION_ROLE_CHANGE_PASSWORD_GROUP, 
-            AUDIT_CHANGE_GROUP,
+            APPLICATION_ROLE_CHANGE_PASSWORD_GROUP,


So, this is a breaking change, right? You will need to deprecate these values, and, if provided by the user, map them into other values. Otherwise, users with scripts that already use these values will be broken.

@markcowl AUDIT_CHANGE_GROUP was never supported. This action group was mistakenly added, and when provided the backend throws an exception anyway.

Also, it cannot be mapped to anything, it should just be removed, as it shouldn't have been there in the first place.

@ranisha2 just to be 100% sure (☺️), if a user provided the AUDIT_CHANGE_GROUP value for any parameter that accepts an AuditActionGroup enum, then it would have never worked and always thrown an exception?

@cormacpayne When enabling a policy with AUDIT_CHANGE_GROUP value, our backend will always throw an exception. The only way to provide this value with no exception to be thrown is when disabling the policy, which does not make any sense as this value does not take effect when the policy is disabled.

@ranisha2 But could there be a customer script that used this value when disabling a policy? As long as there can be no working script that uses this value, then this change is OK, otherwise, it is a breaking change, and the proper thing is to deprecate the value rather than removing it. If possible, for enable scenarios, the value would be mapped to anohter (default) value, although throwing would be OK, if that is what the service does now.

cormacpayne · 2017-11-27T16:48:26Z

@ranisha2 would you mind updating the Sql change log with a snippet about this change? Something along the lines of

* Fixed issue https://github.com/Azure/azure-powershell/issues/4974
    - Removed invalid AUDIT_CHANGED_GROUP value from auditing cmdlets

Sync

Update SQL change log

cormacpayne · 2017-11-28T17:10:05Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/201/

markcowl · 2017-11-28T21:34:31Z

src/ResourceManager/Sql/Commands.Sql/Auditing/Model/BaseBlobAuditingPolicyModel.cs

@@ -18,8 +18,7 @@ public enum AuditActionGroups
    {
            BATCH_STARTED_GROUP,
            BATCH_COMPLETED_GROUP,
-            APPLICATION_ROLE_CHANGE_PASSWORD_GROUP, 
-            AUDIT_CHANGE_GROUP,
+            APPLICATION_ROLE_CHANGE_PASSWORD_GROUP,


@ranisha2 But could there be a customer script that used this value when disabling a policy? As long as there can be no working script that uses this value, then this change is OK, otherwise, it is a breaking change, and the proper thing is to deprecate the value rather than removing it. If possible, for enable scenarios, the value would be mapped to anohter (default) value, although throwing would be OK, if that is what the service does now.

ranisha2 · 2017-11-30T12:56:29Z

@markcowl @cormacpayne
I added a warning message when using this action group, and removed it from the list when provided.
If it's provided alone, the default value will take effect (As non providing action groups). If not, it will be simply removed (There's no mapping for default value as the user can provide a list of action groups)

ranisha2 · 2017-11-30T13:00:29Z

src/ResourceManager/Sql/ChangeLog.md

@@ -19,6 +19,8 @@
 -->
 ## Current Release
 * Added ability to rename database using Set-AzureRmSqlDatabase
+* Fixed issue https://github.com/Azure/azure-powershell/issues/4974
+	- Removed invalid AUDIT_CHANGED_GROUP value from auditing cmdlets


@cormacpayne, @markcowl Do I still need to include this?

@ranisha2 I would change the second line to say "Providing invalid AUDIT_CHANGED_GROUP value for auditing cmdlets no longer throws an error and will be removed in an upcoming release."

@cormacpayne Done

…into preview

cormacpayne · 2017-12-04T05:14:21Z

...ceManager/Sql/Commands.Sql/Auditing/Cmdlet/AuditingSettings/SqlDatabaseAuditingCmdletBase.cs

@@ -54,6 +56,15 @@ protected override SqlAuditAdapter InitModelAdapter(IAzureSubscription subscript
        /// <param name="model">The model object with the data to be sent to the REST endpoints</param>
        protected override DatabaseBlobAuditingSettingsModel PersistChanges(DatabaseBlobAuditingSettingsModel model)
        {
+            if (Array.IndexOf(model.AuditActionGroup, AuditActionGroups.AUDIT_CHANGE_GROUP) > -1)


@ranisha2 is model.AuditActionGroup always going to be an initialized array?

@cormacpayne We perform a GetPolicy request in order to override only the provided parameters.
When not provided, we'll use the saved AuditActionGroup value, which is always initialized.

cormacpayne · 2017-12-04T05:14:32Z

...urceManager/Sql/Commands.Sql/Auditing/Cmdlet/AuditingSettings/SqlServerAuditingCmdletBase.cs

@@ -61,6 +64,15 @@ protected override SqlAuditAdapter InitModelAdapter(IAzureSubscription subscript
        /// <param name="baseModel">The model object with the data to be sent to the REST endpoints</param>
        protected override ServerBlobAuditingSettingsModel PersistChanges(ServerBlobAuditingSettingsModel baseModel)
        {
+            if (Array.IndexOf(baseModel.AuditActionGroup, AuditActionGroups.AUDIT_CHANGE_GROUP) > -1)


@ranisha2 same comment here

Approved offline

cormacpayne · 2017-12-05T15:59:51Z

On-demand: https://azuresdkci.westus2.cloudapp.azure.com/job/powershell-demand/236/

markcowl · 2017-12-05T17:32:33Z

second on demand run: https://azuresdkci.westus2.cloudapp.azure.com/view/PowerShell/job/powershell-demand/238/

ranisha2 added 2 commits November 15, 2017 10:00

Merge pull request #4 from Azure/preview

977061d

Sync

SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups

f06dddc

Remove AUDIT_CHANGE_GROUP from available AuditActionGroups and update markdown files.

ranisha2 mentioned this pull request Nov 15, 2017

InvalidBlobAuditActionsAndGroups Error using Set-AzureRmSqlServerAuditing #4974

Closed

azuresdkci added the needs-review label Nov 15, 2017

azuresdkci assigned markcowl Nov 15, 2017

jaredmoo previously approved these changes Nov 15, 2017

View reviewed changes

markcowl requested changes Nov 16, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Nov 16, 2017

markcowl assigned ranisha2 and unassigned markcowl Nov 16, 2017

ranisha2 added 2 commits November 28, 2017 11:15

Merge pull request #5 from Azure/preview

8714473

Sync

Update SQL change log

2d1cc1e

Update SQL change log

ranisha2 dismissed jaredmoo’s stale review via 2d1cc1e November 28, 2017 09:18

cormacpayne previously approved these changes Nov 28, 2017

View reviewed changes

cormacpayne removed the needs-revision label Nov 28, 2017

cormacpayne assigned markcowl and unassigned ranisha2 Nov 28, 2017

cormacpayne added the needs-review label Nov 28, 2017

markcowl previously requested changes Nov 28, 2017

View reviewed changes

markcowl added needs-revision and removed needs-review labels Nov 28, 2017

markcowl assigned ranisha2 and unassigned markcowl Nov 28, 2017

Re add 'AUDIT_CHANGE_GROUP' and write warning instead when used

2114f53

ranisha2 dismissed cormacpayne’s stale review via 2114f53 November 30, 2017 12:23

ranisha2 assigned markcowl and unassigned ranisha2 Nov 30, 2017

ranisha2 commented Nov 30, 2017

View reviewed changes

Merge branch 'preview' into preview

e68aea8

markcowl assigned ranisha2 and unassigned markcowl Dec 1, 2017

ranisha2 added 2 commits December 1, 2017 18:01

Update change log note for removing AUDIT_CHANGED_GROUP

31817de

Merge branch 'preview' of https://github.com/ranisha2/azure-powershell …

84c4182

…into preview

maddieclayton assigned markcowl and unassigned ranisha2 Dec 1, 2017

maddieclayton added needs-review and removed needs-revision labels Dec 1, 2017

cormacpayne approved these changes Dec 1, 2017

View reviewed changes

cormacpayne suggested changes Dec 4, 2017

View reviewed changes

cormacpayne added needs-revision and removed needs-review labels Dec 4, 2017

cormacpayne assigned ranisha2 and unassigned markcowl Dec 4, 2017

markcowl added 2 commits December 4, 2017 10:47

Merge branch 'preview' into preview

f928f8c

Merge branch 'preview' into preview

8f19f74

cormacpayne approved these changes Dec 5, 2017

View reviewed changes

cormacpayne added Waiting for On Demand and removed needs-revision labels Dec 5, 2017

cormacpayne unassigned ranisha2 Dec 5, 2017

cormacpayne merged commit 78d3aae into Azure:preview Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups #4990

SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups #4990

ranisha2 commented Nov 15, 2017 •

edited by azuresdkci

Loading

markcowl Nov 16, 2017

ranisha2 Nov 21, 2017

ranisha2 Nov 21, 2017

cormacpayne Nov 21, 2017

ranisha2 Nov 26, 2017

markcowl Nov 28, 2017

cormacpayne commented Nov 27, 2017

cormacpayne commented Nov 28, 2017

markcowl Nov 28, 2017

ranisha2 commented Nov 30, 2017

ranisha2 Nov 30, 2017

cormacpayne Nov 30, 2017

ranisha2 Dec 1, 2017

cormacpayne Dec 4, 2017

ranisha2 Dec 5, 2017

cormacpayne Dec 4, 2017

ranisha2 Dec 5, 2017

cormacpayne commented Dec 5, 2017

markcowl commented Dec 5, 2017

SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups #4990

SQL Auditing: Remove AUDIT_CHANGE_GROUP from available AuditActionGroups #4990

Conversation

ranisha2 commented Nov 15, 2017 • edited by azuresdkci Loading

Description

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

cormacpayne commented Nov 27, 2017

cormacpayne commented Nov 28, 2017

Choose a reason for hiding this comment

ranisha2 commented Nov 30, 2017

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

Choose a reason for hiding this comment

cormacpayne commented Dec 5, 2017

markcowl commented Dec 5, 2017

ranisha2 commented Nov 15, 2017 •

edited by azuresdkci

Loading