[test resources] Consolidate naming logic and generate short hash nam…

…es for local use (#5242) Follow up fix related to discussion on Azure/azure-sdk-for-net#33262 - Make name generation simpler and more consistent across test resource scripts (found a bug where remove-testresources.ps1 would not have worked for nested service directories like data/aztables) - Use a short hash for resource names deployed locally, to avoid edge cases where a long service directory or username causes resource names to be too long for validation (e.g. keyvault). - Fix issue with `EnvironmentVariables` ref getting updated from the parameters when splatting in CI mode and polluting the env variable outputs on subsequent runs in the same shell. - Add more warning logging if we're overwriting environment variables, for #4931 Fixes #4931
Azure · Jan 27, 2023 · a03f747 · a03f747
1 parent 184ce1b
commit a03f747
Show file tree

Hide file tree

Showing 4 changed files with 109 additions and 61 deletions.
diff --git 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) {
@@ -316,13 +323,20 @@ function SetDeploymentOutputs([string]$serviceName, [object]$azContext, [object]
             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.
         $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 +361,7 @@ function SetDeploymentOutputs([string]$serviceName, [object]$azContext, [object]
         }
     }
 
-    return $deploymentOutputs
+    return $deploymentEnvironmentVariables, $deploymentOutputs
 }
 
 # Support actions to invoke on exit.
@@ -400,17 +414,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 +556,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
     }
 
@@ -581,7 +580,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'"
@@ -592,16 +590,17 @@ 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]@{
                 ResourceGroupName = $resourceGroupName
                 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."
         }
     }
 
@@ -623,7 +622,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
@@ -650,7 +652,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
         }
 
@@ -667,7 +670,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')
     }
 
@@ -677,12 +684,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."
         }
     }
 
@@ -773,7 +786,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) {
@@ -793,7 +811,7 @@ try {
 
 # Suppress output locally
 if ($CI) {
-    return $EnvironmentVariables
+    return $deploymentEnvironmentVariables
 }
 
 <#

diff --git 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
@@ -16,10 +16,45 @@ 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}-$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"
+
+        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()
+    # 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)
+    $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'"
+
+    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
@@ -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 $false
 
 # This script is intended for interactive users. Make sure they are logged in or fail.
 $context = Get-AzContext