Patch ESLZ ARM #727

krowlandson · 2021-08-04T11:46:50Z

Overview/Summary

Minor updates to the ESLZ ARM templates to fix a few issues observed during development of the Terraform Module.

This PR fixes/adds/changes/removes

Fix naming for lzsManagementGroup variable
Fix value for sqlEncryptionPolicyAssignment variable
Update name for the following policies to improve clarity on purpose:
1. Deny-Priv-Esc-AKS --> Deny-Priv-Escalation-AKS
2. Deny-Privileged-AKS --> Deny-Priv-Containers-AKS
Update description for Deny-Subnet-Without-Nsg Policy Assignment
Update metadata.displayName for CognitiveServicesCMKEffect parameter in Enforce-Encryption-CMK Policy Set Definition to remove unusual whitespace character (discovered during testing, as per the below image)

Breaking Changes

n/a

Testing Evidence

These changes replicate values set in a working deployment using the Terraform version of ES.

For the update on Enforce-Encryption-CMK, please see the below evidence of how this change effects the resource in question:

As part of this Pull Request I have

Checked for duplicate Pull Requests
Associated it with relevant issues, for tracking and closure.
Ensured my code/branch is up-to-date with the latest changes in the main branch
Performed testing and provided evidence.
Updated relevant and associated documentation.
Updated the "What's New?" wiki page (located: /docs/wiki/whats-new.md)

Fix value for sqlEncryptionPolicyAssignment variable

krowlandson · 2021-08-04T13:42:55Z

I don't believe there are any specific updates needed on the "What's new" page as these are all covered under the existing descriptions, but please let me know if you think it needs an addition.

jtracey93

All looks good to me @krowlandson!

Just a couple of questions/notes:

Did you also change the Azure Backup Policy Assignment variable as the azBackupIdentityPolicyAssignment variable was never used?
You dont need to update the RIs in the docs/reference/xxxx/ folders now for Adventure Works, Contoso, WingTip & Trey separately anymore; only the files in eslzArm/ 👍

krnese · 2021-08-09T09:33:58Z

docs/reference/adventureworks/armTemplates/auxiliary/policies.json

@@ -20173,7 +20173,7 @@
                  "Disabled"
                ],
                "metadata": {
-                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
+                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
                  "description": "Customer-managed keys (CMK) are commonly required to meet regulatory compliance standards. CMKs enable the data stored in Cognitive Services to be encrypted with an Azure Key Vault key created and owned by you. You have full control and responsibility for the key lifecycle, including rotation and management. Learn more about CMK encryption at https://aka.ms/cosmosdb-cmk."
                }


We are no longer using this template.

Any harm in this fix?

krnese · 2021-08-09T09:34:05Z

docs/reference/contoso/armTemplates/auxiliary/policies.json

@@ -20173,7 +20173,7 @@
                  "Disabled"
                ],
                "metadata": {
-                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
+                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
                  "description": "Customer-managed keys (CMK) are commonly required to meet regulatory compliance standards. CMKs enable the data stored in Cognitive Services to be encrypted with an Azure Key Vault key created and owned by you. You have full control and responsibility for the key lifecycle, including rotation and management. Learn more about CMK encryption at https://aka.ms/cosmosdb-cmk."
                }


We are no longer using this template.

Any harm in this fix?

krnese · 2021-08-09T09:34:11Z

docs/reference/treyresearch/armTemplates/auxiliary/policies.json

@@ -20173,7 +20173,7 @@
                  "Disabled"
                ],
                "metadata": {
-                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
+                  "displayName": "Cognitive Services accounts should enable data encryption with a customer-managed key (CMK)",
                  "description": "Customer-managed keys (CMK) are commonly required to meet regulatory compliance standards. CMKs enable the data stored in Cognitive Services to be encrypted with an Azure Key Vault key created and owned by you. You have full control and responsibility for the key lifecycle, including rotation and management. Learn more about CMK encryption at https://aka.ms/cosmosdb-cmk."
                }
              },


We are no longer using this template.

Any harm in this fix?

No harm, but we are planning to delete these files so I don't see any reason to why we need to update them

krnese · 2021-08-09T09:35:19Z

eslzArm/eslzArm.json

@@ -607,8 +607,7 @@
            "ascConfigPolicyInitiative": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-ASCConfigPolicyAssignment.json')]",
            "azVmMonitorPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-VMMonitoringPolicyAssignment.json')]",
            "azVmssMonitorPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-VMSSMonitoringPolicyAssignment.json')]",
-            "azBackupLzPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-VMBackupPolicyAssignment.json')]",
-            "azBackupIdentityPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-VMBackupPolicyAssignment.json')]",
+            "azVmBackupPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-VMBackupPolicyAssignment.json')]",
            "azPolicyForAksPolicyAssignment": "[uri(deployment().properties.templateLink.uri, 'managementGroupTemplates/policyAssignments/DINE-AksPolicyPolicyAssignment.json')]",


If identity subscription is enabled, we also recommend to assign VM Backup policy to the identity management group, hence we need a deterministic guid for the subsequent role assignment for that policy, so we need both these variables.

As azBackupIdentityPolicyAssignment isn't referenced anywhere else in the repository and the values are the same, how does this make a difference when an identity subscription is enabled?

It is it's own (optional) deployment name, and we cannot have duplicated resource name in the same ARM template.

Just my 2 cents here 😃

I have tested a deployment from @krowlandson's branch for this PR and it works 👍

Including the Backup policies involved also:

From my initial review, and re-review today, the ARM deployment names are unchanged just the reference to the deployment template URI and a de-duplication of a variable that holds this (which from looking at the current templates, it looks like the azBackupIdentityPolicyAssignment variable is not referenced anywhere as the azBackupLzPolicyAssignment is used for all nested deployments which do the assignments. Also as the conditionals are not based on these and other parameters/variables from the portal UX experience then I think this is all good 👍

Kevin Rowlandson added 3 commits August 4, 2021 12:06

Fix naming for lzsManagementGroup variable

026c57c

Fix value for sqlEncryptionPolicyAssignment variable

Update name and description

fa7f898

Replace invalid whitespace character

d4553ab

krowlandson added the bug Something isn't working label Aug 4, 2021

krowlandson requested a review from krnese August 4, 2021 11:46

krowlandson self-assigned this Aug 4, 2021

jtracey93 previously approved these changes Aug 4, 2021

View reviewed changes

krnese reviewed Aug 9, 2021

View reviewed changes

Kevin Rowlandson added 2 commits August 9, 2021 10:52

Merge branch 'main' into patch-eslzArm

d418389

Revert Policy Assignment names

f6d021f

krowlandson dismissed jtracey93’s stale review via f6d021f August 11, 2021 08:07

krowlandson requested a review from a team as a code owner August 11, 2021 08:07

Merge branch 'main' into patch-eslzArm

8e03f33

krnese approved these changes Aug 11, 2021

View reviewed changes

krnese merged commit 1b79b70 into Azure:main Aug 11, 2021

krowlandson deleted the patch-eslzArm branch August 14, 2022 20:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch ESLZ ARM #727

Patch ESLZ ARM #727

krowlandson commented Aug 4, 2021 •

edited

Loading

krowlandson commented Aug 4, 2021

jtracey93 left a comment

krnese Aug 9, 2021

krowlandson Aug 9, 2021

krnese Aug 9, 2021

krowlandson Aug 9, 2021

krnese Aug 9, 2021

krowlandson Aug 9, 2021

krnese Aug 9, 2021

krnese Aug 9, 2021

krowlandson Aug 9, 2021

krnese Aug 9, 2021

jtracey93 Aug 9, 2021

Patch ESLZ ARM #727

Patch ESLZ ARM #727

Conversation

krowlandson commented Aug 4, 2021 • edited Loading

Overview/Summary

This PR fixes/adds/changes/removes

Breaking Changes

Testing Evidence

As part of this Pull Request I have

krowlandson commented Aug 4, 2021

jtracey93 left a 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

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

krowlandson commented Aug 4, 2021 •

edited

Loading