fix: adding more guidance on private network access (#843)

## Description


[![avm.res.resources.deployment-script](https://github.com/sebassem/bicep-registry-modules/actions/workflows/avm.res.resources.deployment-script.yml/badge.svg?branch=avm-deployment-script)](https://github.com/sebassem/bicep-registry-modules/actions/workflows/avm.res.resources.deployment-script.yml)

<!--Why this PR? What is changed? What is the effect? etc.-->

- Closes #812 
- Added guidance on not specifying storageAccountKeys in private network
scenario
- Conditionally provide the storageAccountKey property
- Added a disclaimer on the needed permissions in the private network
scenario
- Modified the default test to not deploy a storage account as part of
the dependencies

## Updating an existing module

<!--Run through the checklist if your PR updates an existing module.-->

- [X] This is a bug fix:
- [X] Someone has opened a bug report issue, and I have included "Closes
#{bug_report_issue_number}" in the PR description.
- [ ] The bug was found by the module author, and no one has opened an
issue to report it yet.
- [] I have run `brm validate` locally to verify the module files.
- [X] I have run deployment tests locally to ensure the module is
deployable.
- [X] I have read the [Updating an existing
module](https://github.com/Azure/bicep-registry-modules/blob/main/CONTRIBUTING.md#updating-an-existing-module)
section in the contributing guide and updated the `version.json` file
properly:
- [X] The PR contains backwards compatible bug fixes, and I have NOT
bumped the MAJOR or MINOR version in `version.json`.
- [] The PR contains backwards compatible feature updates, and I have
bumped the MINOR version in `version.json`.
- [] The PR contains breaking changes, and I have bumped the MAJOR
version in `version.json`.
- [ ] I have updated the examples in README with the latest module
version number.

---------

Co-authored-by: Alexander Sehr <[email protected]>

avm/res/resources/deployment-script/README.md

@@ -135,7 +135,7 @@ module deploymentScript 'br/public:avm/res/resources/deployment-script:<version>
 ### Example 2: _Using only defaults_
 
 This instance deploys the module with the minimum set of required parameters.
-> **Note:** The test currently implements additional non-required parameters to cater for a test-specific limitation.
+> **Note:** In this scenario, In this scenario, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys.
 
 
 
@@ -160,7 +160,6 @@ module deploymentScript 'br/public:avm/res/resources/deployment-script:<version>
     }
     retentionInterval: 'P1D'
     scriptContent: 'Write-Host \'AVM Deployment Script test!\''
-    storageAccountResourceId: '<storageAccountResourceId>'
   }
 }
 ```
@@ -203,9 +202,6 @@ module deploymentScript 'br/public:avm/res/resources/deployment-script:<version>
     },
     "scriptContent": {
       "value": "Write-Host \"AVM Deployment Script test!\""
-    },
-    "storageAccountResourceId": {
-      "value": "<storageAccountResourceId>"
     }
   }
 }
@@ -722,7 +718,7 @@ module deploymentScript 'br/public:avm/res/resources/deployment-script:<version>
 | [`runOnce`](#parameter-runonce) | bool | When set to false, script will run every time the template is deployed. When set to true, the script will only run once. |
 | [`scriptContent`](#parameter-scriptcontent) | string | Script body. Max length: 32000 characters. To run an external script, use primaryScriptURI instead. |
 | [`storageAccountResourceId`](#parameter-storageaccountresourceid) | string | The resource ID of the storage account to use for this deployment script. If none is provided, the deployment script uses a temporary, managed storage account. |
-| [`subnetResourceIds`](#parameter-subnetresourceids) | array | List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network. |
+| [`subnetResourceIds`](#parameter-subnetresourceids) | array | List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network. When using a private network, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys. Also, Shared-Keys must not be disabled on the used storage account [ref](https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/deployment-script-vnet). |
 | [`supportingScriptUris`](#parameter-supportingscripturis) | array | List of supporting files for the external script (defined in primaryScriptUri). Does not work with internal scripts (code defined in scriptContent). |
 | [`tags`](#parameter-tags) | object | Resource tags. |
 | [`timeout`](#parameter-timeout) | string | Maximum allowed script execution time specified in ISO 8601 format. Default value is PT1H - 1 hour; 'PT30M' - 30 minutes; 'P5D' - 5 days; 'P1Y' 1 year. |
@@ -1006,7 +1002,7 @@ The resource ID of the storage account to use for this deployment script. If non
 
 ### Parameter: `subnetResourceIds`
 
-List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network.
+List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network. When using a private network, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys. Also, Shared-Keys must not be disabled on the used storage account [ref](https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/deployment-script-vnet).
 
 - Required: No
 - Type: array

avm/res/resources/deployment-script/main.bicep

@@ -55,7 +55,7 @@ param environmentVariables object = {}
 @description('Optional. List of supporting files for the external script (defined in primaryScriptUri). Does not work with internal scripts (code defined in scriptContent).')
 param supportingScriptUris array?
 
-@description('Optional. List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network.')
+@description('Optional. List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network. When using a private network, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys. Also, Shared-Keys must not be disabled on the used storage account [ref](https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/deployment-script-vnet).')
 param subnetResourceIds string[]?
 
 @description('Optional. Command-line arguments to pass to the script. Arguments are separated by spaces.')
@@ -130,7 +130,7 @@ resource storageAccount 'Microsoft.Storage/storageAccounts@2021-04-01' existing
 }
 
 var storageAccountSettings = !empty(storageAccountResourceId) ? {
-  storageAccountKey: listKeys(storageAccount.id, '2023-01-01').keys[0].value
+  storageAccountKey: empty(subnetResourceIds) ? listKeys(storageAccount.id, '2023-01-01').keys[0].value : null
   storageAccountName: last(split(storageAccountResourceId, '/'))
 } : null
 

avm/res/resources/deployment-script/main.json

@@ -5,8 +5,8 @@
   "metadata": {
     "_generator": {
       "name": "bicep",
-      "version": "0.24.24.22086",
-      "templateHash": "3543265320789085006"
+      "version": "0.23.1.45101",
+      "templateHash": "8734511554398837213"
     },
     "name": "Deployment Scripts",
     "description": "This module deploys Deployment Scripts.",
@@ -208,7 +208,7 @@
       },
       "nullable": true,
       "metadata": {
-        "description": "Optional. List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network."
+        "description": "Optional. List of subnet IDs to use for the container group. This is required if you want to run the deployment script in a private network. When using a private network, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys. Also, Shared-Keys must not be disabled on the used storage account [ref](https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/deployment-script-vnet)."
       }
     },
     "arguments": {
@@ -394,7 +394,7 @@
         "azPowerShellVersion": "[if(equals(parameters('kind'), 'AzurePowerShell'), parameters('azPowerShellVersion'), null())]",
         "azCliVersion": "[if(equals(parameters('kind'), 'AzureCLI'), parameters('azCliVersion'), null())]",
         "containerSettings": "[if(not(empty(variables('containerSettings'))), variables('containerSettings'), null())]",
-        "storageAccountSettings": "[if(not(empty(parameters('storageAccountResourceId'))), if(not(empty(parameters('storageAccountResourceId'))), createObject('storageAccountKey', listKeys(extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), '//'), '/')[2], split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), '////'), '/')[4]), 'Microsoft.Storage/storageAccounts', last(split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), 'dummyAccount'), '/'))), '2023-01-01').keys[0].value, 'storageAccountName', last(split(parameters('storageAccountResourceId'), '/'))), null()), null())]",
+        "storageAccountSettings": "[if(not(empty(parameters('storageAccountResourceId'))), if(not(empty(parameters('storageAccountResourceId'))), createObject('storageAccountKey', if(empty(parameters('subnetResourceIds')), listKeys(extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), '//'), '/')[2], split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), '////'), '/')[4]), 'Microsoft.Storage/storageAccounts', last(split(if(not(empty(parameters('storageAccountResourceId'))), parameters('storageAccountResourceId'), 'dummyAccount'), '/'))), '2023-01-01').keys[0].value, null()), 'storageAccountName', last(split(parameters('storageAccountResourceId'), '/'))), null()), null())]",
         "arguments": "[parameters('arguments')]",
         "environmentVariables": "[if(not(empty(parameters('environmentVariables'))), parameters('environmentVariables').secureList, createArray())]",
         "scriptContent": "[if(not(empty(parameters('scriptContent'))), parameters('scriptContent'), null())]",

avm/res/resources/deployment-script/tests/e2e/defaults/dependencies.bicep

@@ -4,28 +4,10 @@ param location string = resourceGroup().location
 @description('Required. The name of the managed identity to create.')
 param managedIdentityName string
 
-@description('Required. The name of the Storage Account to create.')
-param storageAccountName string
-
 resource managedIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2018-11-30' = {
   name: managedIdentityName
   location: location
 }
 
-resource storageAccount 'Microsoft.Storage/storageAccounts@2021-09-01' = {
-  name: storageAccountName
-  location: location
-  sku: {
-    name: 'Standard_LRS'
-  }
-  kind: 'StorageV2'
-  properties: {
-    supportsHttpsTrafficOnly: true
-  }
-}
-
 @description('The resource ID of the created managed identity.')
 output managedIdentityResourceId string = managedIdentity.id
-
-@description('The resource ID of the created storage account.')
-output storageAccountResourceId string = storageAccount.id

avm/res/resources/deployment-script/tests/e2e/defaults/main.test.bicep

@@ -3,7 +3,7 @@ targetScope = 'subscription'
 metadata name = 'Using only defaults'
 metadata description = '''
 This instance deploys the module with the minimum set of required parameters.
-> **Note:** The test currently implements additional non-required parameters to cater for a test-specific limitation.
+> **Note:** In this scenario, In this scenario, the `Storage File Data Privileged Contributor` role needs to be assigned to the user-assigned managed identity and the deployment principal needs to have permissions to list the storage account keys.
 '''
 
 // ========== //
@@ -39,7 +39,6 @@ module nestedDependencies 'dependencies.bicep' = {
   name: '${uniqueString(deployment().name, location)}-nestedDependencies'
   params: {
     managedIdentityName: 'dep-${namePrefix}-msi-${serviceShort}'
-    storageAccountName: 'dep${namePrefix}st${serviceShort}'
     location: location
   }
 }
@@ -58,7 +57,6 @@ module testDeployment '../../../main.bicep' = {
     kind: 'AzurePowerShell'
     retentionInterval: 'P1D'
     scriptContent: 'Write-Host \'AVM Deployment Script test!\''
-    storageAccountResourceId: nestedDependencies.outputs.storageAccountResourceId
     managedIdentities: {
       userAssignedResourcesIds: [
         nestedDependencies.outputs.managedIdentityResourceId

avm/res/resources/deployment-script/tests/e2e/private-network/dependencies.bicep

@@ -23,7 +23,7 @@ resource managedIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2018-
   location: location
 }
 
-resource storagePermissions 'Microsoft.Authorization/roleAssignments@2022-04-01' = {
+resource storageFileSharePermissions 'Microsoft.Authorization/roleAssignments@2022-04-01' = {
   name: guid('storageFileDataPrivilegedContributorRole', managedIdentity.id, storageAccount.id)
   scope: storageAccount
   properties: {
@@ -42,6 +42,7 @@ resource storageAccount 'Microsoft.Storage/storageAccounts@2021-09-01' = {
   kind: 'StorageV2'
   properties: {
     supportsHttpsTrafficOnly: true
+    allowSharedKeyAccess: true // Cannot be set to false when using a private network for the deployment script
     networkAcls: {
       bypass: 'AzureServices'
       defaultAction: 'Deny'