From 37d51248df0d844a876ca4f109a70e68cf16909c Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Thu, 26 Jan 2023 12:56:35 -0500 Subject: [PATCH 1/5] Consolidate naming logic and generate short hash names for local use --- .../TestResources/New-TestResources.ps1 | 31 +++++------------ .../TestResources/Remove-TestResources.ps1 | 15 ++++---- .../TestResources/SubConfig-Helpers.ps1 | 34 +++++++++++++++++-- .../TestResources/Update-TestResources.ps1 | 18 ++++------ 4 files changed, 54 insertions(+), 44 deletions(-) diff --git a/eng/common/TestResources/New-TestResources.ps1 b/eng/common/TestResources/New-TestResources.ps1 index 3e5a60f5585cc..08705c7248e74 100644 --- a/eng/common/TestResources/New-TestResources.ps1 +++ b/eng/common/TestResources/New-TestResources.ps1 @@ -400,17 +400,13 @@ try { exit } - $UserName = GetUserName - - if (!$BaseName) { - if ($CI) { - $BaseName = 't' + (New-Guid).ToString('n').Substring(0, 16) - Log "Generated base name '$BaseName' for CI build" - } else { - $BaseName = GetBaseName $UserName (GetServiceLeafDirectoryName $ServiceDirectory) - Log "BaseName was not set. Using default base name '$BaseName'" - } - } + $serviceName = GetServiceLeafDirectoryName $ServiceDirectory + $BaseName, $ResourceGroupName = GetBaseAndResourceGroupNames ` + -baseNameDefault $BaseName ` + -resourceGroupNameDefault $ResourceGroupName ` + -user (GetUserName) ` + -serviceDirectoryName $serviceName ` + -CI $CI # Make sure pre- and post-scripts are passed formerly required arguments. $PSBoundParameters['BaseName'] = $BaseName @@ -546,19 +542,8 @@ try { $ProvisionerApplicationOid = $sp.Id } - $serviceName = GetServiceLeafDirectoryName $ServiceDirectory - - $ResourceGroupName = if ($ResourceGroupName) { - $ResourceGroupName - } elseif ($CI) { - # Format the resource group name based on resource group naming recommendations and limitations. - "rg-{0}-$BaseName" -f ($serviceName -replace '[\.\\\/:]', '-').ToLowerInvariant().Substring(0, [Math]::Min($serviceName.Length, 90 - $BaseName.Length - 4)).Trim('-') - } else { - "rg-$BaseName" - } - $tags = @{ - Owners = $UserName + Owners = (GetUserName) ServiceDirectory = $ServiceDirectory } diff --git a/eng/common/TestResources/Remove-TestResources.ps1 b/eng/common/TestResources/Remove-TestResources.ps1 index 069f2bf8a634a..4e2b52c63a6d7 100644 --- a/eng/common/TestResources/Remove-TestResources.ps1 +++ b/eng/common/TestResources/Remove-TestResources.ps1 @@ -147,14 +147,13 @@ if (!$ResourceGroupName) { exit 0 } } else { - if (!$BaseName) { - $UserName = GetUserName - $BaseName = GetBaseName $UserName $ServiceDirectory - Log "BaseName was not set. Using default base name '$BaseName'" - } - - # Format the resource group name like in New-TestResources.ps1. - $ResourceGroupName = "rg-$BaseName" + $serviceName = GetServiceLeafDirectoryName $ServiceDirectory + $BaseName, $ResourceGroupName = GetBaseAndResourceGroupNames ` + -baseNameDefault $BaseName ` + -resourceGroupNameDefault $ResourceGroupName ` + -user (GetUserName) ` + -serviceDirectoryName $serviceName ` + -CI $CI } } diff --git a/eng/common/TestResources/SubConfig-Helpers.ps1 b/eng/common/TestResources/SubConfig-Helpers.ps1 index cc93def6aa89d..6a9d8ea792006 100644 --- a/eng/common/TestResources/SubConfig-Helpers.ps1 +++ b/eng/common/TestResources/SubConfig-Helpers.ps1 @@ -16,10 +16,40 @@ function GetUserName() { return $UserName } -function GetBaseName([string]$user, [string]$serviceDirectoryName) { +function GetBaseAndResourceGroupNames( + [string]$baseNameDefault, + [string]$resourceGroupNameDefault, + [string]$user, + [string]$serviceDirectoryName, + [bool]$CI +) { + if ($CI) { + $base = 't' + (New-Guid).ToString('n').Substring(0, 16) + # Format the resource group name based on resource group naming recommendations and limitations. + $generatedGroup = "rg-{0}-$BaseName" -f ($serviceName -replace '[\.\\\/:]', '-').ToLowerInvariant().Substring(0, [Math]::Min($serviceName.Length, 90 - $BaseName.Length - 4)).Trim('-') + $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : $generatedGroup + + Log "Generated resource base name '$base' and resource group name '$group' for CI build" + + return $base, $group + } + + if ($baseNameDefault) { + $base = $baseNameDefault.ToLowerInvariant() + $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : ("rg-$baseNameDefault".ToLowerInvariant()) + return $base, $group + } + # Handle service directories in nested directories, e.g. `data/aztables` $serviceDirectorySafeName = $serviceDirectoryName -replace '[\./\\]', '' - return "$user$serviceDirectorySafeName".ToLowerInvariant() + # Hash to keep resource names short enough to not break naming restrictions (e.g. keyvault name length) + $baseNameStream = [IO.MemoryStream]::new([Text.Encoding]::UTF8.GetBytes("${user}${serviceDirectorySafeName}")) + $base = 't' + (Get-FileHash -InputStream $baseNameStream -Algorithm SHA1).Hash.Substring(0, 16).ToLowerInvariant() + $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : "rg-${user}${serviceDirectorySafeName}".ToLowerInvariant(); + + Log "BaseName was not set. Generating resource group name '$group' and resource base name '$base'" + + return $base, $group } function ShouldMarkValueAsSecret([string]$serviceName, [string]$key, [string]$value, [array]$allowedValues = @()) diff --git a/eng/common/TestResources/Update-TestResources.ps1 b/eng/common/TestResources/Update-TestResources.ps1 index 7715ec4fcfbc5..b57ee485fcf7e 100644 --- a/eng/common/TestResources/Update-TestResources.ps1 +++ b/eng/common/TestResources/Update-TestResources.ps1 @@ -69,17 +69,13 @@ $exitActions = @({ } }) -# Make sure $ResourceGroupName is set. -if (!$ResourceGroupName) { - # Make sure $BaseName is set. - if (!$BaseName) { - $UserName = GetUserName - $BaseName = GetBaseName $UserName $ServiceDirectory - Log "BaseName was not set. Using default base name '$BaseName'" - } - - $ResourceGroupName = "rg-$BaseName" -} +$serviceName = GetServiceLeafDirectoryName $ServiceDirectory +$BaseName, $ResourceGroupName = GetBaseAndResourceGroupNames ` + -baseNameDefault $BaseName ` + -resourceGroupNameDefault $ResourceGroupName ` + -user (GetUserName) ` + -serviceDirectoryName $serviceName ` + -CI $CI # This script is intended for interactive users. Make sure they are logged in or fail. $context = Get-AzContext From 7ba61307c3771ea4a4be8798c374d28d891786bc Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Fri, 27 Jan 2023 12:19:17 -0500 Subject: [PATCH 2/5] Shorten long lines --- .../TestResources/New-TestResources.ps1 | 28 +++++++++++++++---- .../TestResources/SubConfig-Helpers.ps1 | 5 +++- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/eng/common/TestResources/New-TestResources.ps1 b/eng/common/TestResources/New-TestResources.ps1 index 08705c7248e74..d818cc59b3ee0 100644 --- a/eng/common/TestResources/New-TestResources.ps1 +++ b/eng/common/TestResources/New-TestResources.ps1 @@ -586,7 +586,9 @@ try { Location = $Location } } else { - Write-Error "Resource group '$ResourceGroupName' already exists." -Category ResourceExists -RecommendedAction "Delete resource group '$ResourceGroupName', or overwrite it when redeploying." + Write-Error "Resource group '$ResourceGroupName' already exists." ` + -Category ResourceExists ` + -RecommendedAction "Delete resource group '$ResourceGroupName', or overwrite it when redeploying." } } @@ -608,7 +610,10 @@ try { $displayName = "$($baseName)$suffix.$ResourceType-resources.azure.sdk" } - $servicePrincipalWrapper = NewServicePrincipalWrapper -subscription $SubscriptionId -resourceGroup $ResourceGroupName -displayName $DisplayName + $servicePrincipalWrapper = NewServicePrincipalWrapper ` + -subscription $SubscriptionId ` + -resourceGroup $ResourceGroupName ` + -displayName $DisplayName $global:AzureTestPrincipal = $servicePrincipalWrapper $global:AzureTestSubscription = $SubscriptionId @@ -635,7 +640,8 @@ try { } } catch { - Write-Warning "The Object ID of the test application was unable to be queried. You may want to consider passing it explicitly with the 'TestApplicationOid` parameter." + Write-Warning "The Object ID of the test application was unable to be queried. " + ` + "You may want to consider passing it explicitly with the 'TestApplicationOid` parameter." throw $_.Exception } @@ -652,7 +658,11 @@ try { # If the role hasn't been explicitly assigned to the resource group and a cached service principal is in use, # query to see if the grant is needed. if (!$resourceGroupRoleAssigned -and $AzureTestPrincipal) { - $roleAssignment = Get-AzRoleAssignment -ObjectId $AzureTestPrincipal.Id -RoleDefinitionName 'Owner' -ResourceGroupName "$ResourceGroupName" -ErrorAction SilentlyContinue + $roleAssignment = Get-AzRoleAssignment ` + -ObjectId $AzureTestPrincipal.Id ` + -RoleDefinitionName 'Owner' ` + -ResourceGroupName "$ResourceGroupName" ` + -ErrorAction SilentlyContinue $resourceGroupRoleAssigned = ($roleAssignment.RoleDefinitionName -eq 'Owner') } @@ -662,12 +672,18 @@ try { # the explicit grant. if (!$resourceGroupRoleAssigned) { Log "Attempting to assigning the 'Owner' role for '$ResourceGroupName' to the Test Application '$TestApplicationId'" - $principalOwnerAssignment = New-AzRoleAssignment -RoleDefinitionName "Owner" -ApplicationId "$TestApplicationId" -ResourceGroupName "$ResourceGroupName" -ErrorAction SilentlyContinue + $principalOwnerAssignment = New-AzRoleAssignment ` + -RoleDefinitionName "Owner" ` + -ApplicationId "$TestApplicationId" ` + -ResourceGroupName "$ResourceGroupName" ` + -ErrorAction SilentlyContinue if ($principalOwnerAssignment.RoleDefinitionName -eq 'Owner') { Write-Verbose "Successfully assigned ownership of '$ResourceGroupName' to the Test Application '$TestApplicationId'" } else { - Write-Warning "The 'Owner' role for '$ResourceGroupName' could not be assigned. You may need to manually grant 'Owner' for the resource group to the Test Application '$TestApplicationId' if it does not have subscription-level permissions." + Write-Warning "The 'Owner' role for '$ResourceGroupName' could not be assigned. " + ` + "You may need to manually grant 'Owner' for the resource group to the " + ` + "Test Application '$TestApplicationId' if it does not have subscription-level permissions." } } diff --git a/eng/common/TestResources/SubConfig-Helpers.ps1 b/eng/common/TestResources/SubConfig-Helpers.ps1 index 6a9d8ea792006..18199a06a2f4e 100644 --- a/eng/common/TestResources/SubConfig-Helpers.ps1 +++ b/eng/common/TestResources/SubConfig-Helpers.ps1 @@ -26,7 +26,10 @@ function GetBaseAndResourceGroupNames( if ($CI) { $base = 't' + (New-Guid).ToString('n').Substring(0, 16) # Format the resource group name based on resource group naming recommendations and limitations. - $generatedGroup = "rg-{0}-$BaseName" -f ($serviceName -replace '[\.\\\/:]', '-').ToLowerInvariant().Substring(0, [Math]::Min($serviceName.Length, 90 - $BaseName.Length - 4)).Trim('-') + $generatedGroup = "rg-{0}-$base" -f ($serviceName -replace '[\.\\\/:]', '-'). + Substring(0, [Math]::Min($serviceDirectoryName.Length, 90 - $base.Length - 4)). + Trim('-'). + ToLowerInvariant() $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : $generatedGroup Log "Generated resource base name '$base' and resource group name '$group' for CI build" From d82be9366790e6f68a5bb59bb28bbc900d0af728 Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Fri, 27 Jan 2023 13:00:30 -0500 Subject: [PATCH 3/5] Handle issues with EnvironmentVariable parameter ref being updated --- .../TestResources/New-TestResources.ps1 | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/eng/common/TestResources/New-TestResources.ps1 b/eng/common/TestResources/New-TestResources.ps1 index d818cc59b3ee0..a9b2a046eb5c5 100644 --- a/eng/common/TestResources/New-TestResources.ps1 +++ b/eng/common/TestResources/New-TestResources.ps1 @@ -260,7 +260,7 @@ function BuildBicepFile([System.IO.FileSystemInfo] $file) return $templateFilePath } -function BuildDeploymentOutputs([string]$serviceName, [object]$azContext, [object]$deployment) { +function BuildDeploymentOutputs([string]$serviceName, [object]$azContext, [object]$deployment, [hashtable]$environmentVariables) { $serviceDirectoryPrefix = BuildServiceDirectoryPrefix $serviceName # Add default values $deploymentOutputs = [Ordered]@{ @@ -277,7 +277,7 @@ function BuildDeploymentOutputs([string]$serviceName, [object]$azContext, [objec "AZURE_SERVICE_DIRECTORY" = $serviceName.ToUpperInvariant(); } - MergeHashes $EnvironmentVariables $(Get-Variable deploymentOutputs) + MergeHashes $environmentVariables $(Get-Variable deploymentOutputs) foreach ($key in $deployment.Outputs.Keys) { $variable = $deployment.Outputs[$key] @@ -293,8 +293,15 @@ function BuildDeploymentOutputs([string]$serviceName, [object]$azContext, [objec return $deploymentOutputs } -function SetDeploymentOutputs([string]$serviceName, [object]$azContext, [object]$deployment, [object]$templateFile) { - $deploymentOutputs = BuildDeploymentOutputs $serviceName $azContext $deployment +function SetDeploymentOutputs( + [string]$serviceName, + [object]$azContext, + [object]$deployment, + [object]$templateFile, + [hashtable]$environmentVariables = @{} +) { + $deploymentEnvironmentVariables = $environmentVariables.Clone() + $deploymentOutputs = BuildDeploymentOutputs $serviceName $azContext $deployment $deploymentEnvironmentVariables if ($OutFile) { if (!$IsWindows) { @@ -322,7 +329,7 @@ function SetDeploymentOutputs([string]$serviceName, [object]$azContext, [object] $notSecretValues = @() foreach ($key in $deploymentOutputs.Keys) { $value = $deploymentOutputs[$key] - $EnvironmentVariables[$key] = $value + $deploymentEnvironmentVariables[$key] = $value if ($CI) { if (ShouldMarkValueAsSecret $serviceName $key $value $notSecretValues) { @@ -347,7 +354,7 @@ function SetDeploymentOutputs([string]$serviceName, [object]$azContext, [object] } } - return $deploymentOutputs + return $deploymentEnvironmentVariables, $deploymentOutputs } # Support actions to invoke on exit. @@ -566,7 +573,6 @@ try { # to determine whether resources should be removed. Write-Host "Setting variable 'CI_HAS_DEPLOYED_RESOURCES': 'true'" LogVsoCommand "##vso[task.setvariable variable=CI_HAS_DEPLOYED_RESOURCES;]true" - $EnvironmentVariables['CI_HAS_DEPLOYED_RESOURCES'] = $true } Log "Creating resource group '$ResourceGroupName' in location '$Location'" @@ -577,8 +583,7 @@ try { if ($resourceGroup.ProvisioningState -eq 'Succeeded') { # New-AzResourceGroup would've written an error and stopped the pipeline by default anyway. Write-Verbose "Successfully created resource group '$($resourceGroup.ResourceGroupName)'" - } - elseif (!$resourceGroup) { + } elseif (!$resourceGroup) { if (!$PSCmdlet.ShouldProcess($resourceGroupName)) { # If the -WhatIf flag was passed, there will be no resource group created. Fake it. $resourceGroup = [PSCustomObject]@{ @@ -774,7 +779,12 @@ try { Write-Host "Deployment '$($deployment.DeploymentName)' has CorrelationId '$($deployment.CorrelationId)'" Write-Host "Successfully deployed template '$($templateFile.jsonFilePath)' to resource group '$($resourceGroup.ResourceGroupName)'" - $deploymentOutputs = SetDeploymentOutputs $serviceName $context $deployment $templateFile + $deploymentEnvironmentVariables, $deploymentOutputs = SetDeploymentOutputs ` + -serviceName $serviceName ` + -azContext $context ` + -deployment $deployment ` + -templateFile $templateFile ` + -environmentVariables $EnvironmentVariables $postDeploymentScript = $templateFile.originalFilePath | Split-Path | Join-Path -ChildPath "$ResourceType-resources-post.ps1" if (Test-Path $postDeploymentScript) { @@ -794,7 +804,7 @@ try { # Suppress output locally if ($CI) { - return $EnvironmentVariables + return $deploymentEnvironmentVariables } <# From 55feec2f4a25a8dac4efc6dbd6030700e70bbd68 Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Fri, 27 Jan 2023 14:07:37 -0500 Subject: [PATCH 4/5] Warn on env variable overwrite. Base name generation off resource group --- eng/common/TestResources/New-TestResources.ps1 | 7 +++++++ eng/common/TestResources/SubConfig-Helpers.ps1 | 4 +++- eng/common/TestResources/Update-TestResources.ps1 | 2 +- 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/eng/common/TestResources/New-TestResources.ps1 b/eng/common/TestResources/New-TestResources.ps1 index a9b2a046eb5c5..8e4e5e9d4f786 100644 --- a/eng/common/TestResources/New-TestResources.ps1 +++ b/eng/common/TestResources/New-TestResources.ps1 @@ -323,6 +323,13 @@ function SetDeploymentOutputs( Log "Persist the following environment variables based on your detected shell ($shell):`n" } + # Write overwrite warnings first, since local execution prints a runnable command to export variables + foreach ($key in $deploymentOutputs.Keys) { + if ([Environment]::GetEnvironmentVariable($key)) { + Write-Warning "Deployment outputs will overwrite pre-existing environment variable '$key'" + } + } + # Marking values as secret by allowed keys below is not sufficient, as there may be outputs set in the ARM/bicep # file that re-mark those values as secret (since all user-provided deployment outputs are treated as secret by default). # This variable supports a second check on not marking previously allowed keys/values as secret. diff --git a/eng/common/TestResources/SubConfig-Helpers.ps1 b/eng/common/TestResources/SubConfig-Helpers.ps1 index 18199a06a2f4e..0eb2361c169cc 100644 --- a/eng/common/TestResources/SubConfig-Helpers.ps1 +++ b/eng/common/TestResources/SubConfig-Helpers.ps1 @@ -45,8 +45,10 @@ function GetBaseAndResourceGroupNames( # Handle service directories in nested directories, e.g. `data/aztables` $serviceDirectorySafeName = $serviceDirectoryName -replace '[\./\\]', '' + # Seed off resource group name if set to avoid name conflicts with deployments where it is not set + $seed = $resourceGroupNameDefault ? $resourceGroupNameDefault : "${user}${serviceDirectorySafeName}" + $baseNameStream = [IO.MemoryStream]::new([Text.Encoding]::UTF8.GetBytes("$seed")) # Hash to keep resource names short enough to not break naming restrictions (e.g. keyvault name length) - $baseNameStream = [IO.MemoryStream]::new([Text.Encoding]::UTF8.GetBytes("${user}${serviceDirectorySafeName}")) $base = 't' + (Get-FileHash -InputStream $baseNameStream -Algorithm SHA1).Hash.Substring(0, 16).ToLowerInvariant() $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : "rg-${user}${serviceDirectorySafeName}".ToLowerInvariant(); diff --git a/eng/common/TestResources/Update-TestResources.ps1 b/eng/common/TestResources/Update-TestResources.ps1 index b57ee485fcf7e..a9983f547a956 100644 --- a/eng/common/TestResources/Update-TestResources.ps1 +++ b/eng/common/TestResources/Update-TestResources.ps1 @@ -75,7 +75,7 @@ $BaseName, $ResourceGroupName = GetBaseAndResourceGroupNames ` -resourceGroupNameDefault $ResourceGroupName ` -user (GetUserName) ` -serviceDirectoryName $serviceName ` - -CI $CI + -CI $false # This script is intended for interactive users. Make sure they are logged in or fail. $context = Get-AzContext From 118a4465586ee96c4329d1b2219788df249887e2 Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Fri, 27 Jan 2023 14:15:15 -0500 Subject: [PATCH 5/5] Use SHA256 algorithm for short name hash --- eng/common/TestResources/SubConfig-Helpers.ps1 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eng/common/TestResources/SubConfig-Helpers.ps1 b/eng/common/TestResources/SubConfig-Helpers.ps1 index 0eb2361c169cc..f87b088c3dd3e 100644 --- a/eng/common/TestResources/SubConfig-Helpers.ps1 +++ b/eng/common/TestResources/SubConfig-Helpers.ps1 @@ -49,7 +49,7 @@ function GetBaseAndResourceGroupNames( $seed = $resourceGroupNameDefault ? $resourceGroupNameDefault : "${user}${serviceDirectorySafeName}" $baseNameStream = [IO.MemoryStream]::new([Text.Encoding]::UTF8.GetBytes("$seed")) # Hash to keep resource names short enough to not break naming restrictions (e.g. keyvault name length) - $base = 't' + (Get-FileHash -InputStream $baseNameStream -Algorithm SHA1).Hash.Substring(0, 16).ToLowerInvariant() + $base = 't' + (Get-FileHash -InputStream $baseNameStream -Algorithm SHA256).Hash.Substring(0, 16).ToLowerInvariant() $group = $resourceGroupNameDefault ? $resourceGroupNameDefault : "rg-${user}${serviceDirectorySafeName}".ToLowerInvariant(); Log "BaseName was not set. Generating resource group name '$group' and resource base name '$base'"