From a03f7473f349302384fb36eff82e34a01f59aba1 Mon Sep 17 00:00:00 2001 From: Ben Broderick Phillips Date: Fri, 27 Jan 2023 15:21:31 -0500 Subject: [PATCH] [test resources] Consolidate naming logic and generate short hash names for local use (#5242) Follow up fix related to discussion on https://github.com/Azure/azure-sdk-for-net/pull/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 --- .../TestResources/New-TestResources.ps1 | 98 +++++++++++-------- .../TestResources/Remove-TestResources.ps1 | 15 ++- .../TestResources/SubConfig-Helpers.ps1 | 39 +++++++- .../TestResources/Update-TestResources.ps1 | 18 ++-- 4 files changed, 109 insertions(+), 61 deletions(-) diff --git a/eng/common/TestResources/New-TestResources.ps1 b/eng/common/TestResources/New-TestResources.ps1 index 3e5a60f5585..8e4e5e9d4f7 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) { @@ -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,8 +590,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]@{ @@ -601,7 +598,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." } } @@ -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 index 069f2bf8a63..4e2b52c63a6 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 cc93def6aa8..f87b088c3dd 100644 --- 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 index 7715ec4fcfb..a9983f547a9 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 $false # This script is intended for interactive users. Make sure they are logged in or fail. $context = Get-AzContext