Review feedback and pair with Ben

Author: danieljurek

eng/common/TestResources/New-TestResources.ps1

@@ -92,14 +92,10 @@ param (
     [Parameter()]
     [switch] $SuppressVsoCommands = ($null -eq $env:SYSTEM_TEAMPROJECTID),
 
-    # Default behavior is to use -UserAuth
+    # Default behavior is to use logged in credentials
     [Parameter()]
     [switch] $ServicePrincipalAuth,
 
-    [Parameter()]
-    [switch] $FederatedAuth,
-
-
     # Captures any arguments not declared here (no parameter errors)
     # This enables backwards compatibility with old script versions in
     # hotfix branches if and when the dynamic subscription configuration
@@ -110,19 +106,9 @@ param (
 
 . $PSScriptRoot/SubConfig-Helpers.ps1
 
-if ($FederatedAuth -and $ServicePrincipalAuth) {
-    Write-Error "Only one of 'FederatedAuth' and 'ServicePrincipalAuth' can be set."
-    exit 1
-}
-
-$UserAuth = $true
-if ($ServicePrincipalAuth) {
-    $UserAuth = $false
-}
-
-if ($FederatedAuth) {
-    # Clear secrets if FederatedAuth is set. This prevents secrets from being
-    # passed to pre- and post-scripts.
+if (!$ServicePrincipalAuth) {
+    # Clear secrets if not using Service Principal auth. This prevents secrets
+    # from being passed to pre- and post-scripts.
     $PSBoundParameters['TestApplicationSecret'] = $TestApplicationSecret = ''
     $PSBoundParameters['ProvisionerApplicationSecret'] = $ProvisionerApplicationSecret = ''
 }
@@ -299,7 +285,7 @@ function BuildDeploymentOutputs([string]$serviceName, [object]$azContext, [objec
         "AZURE_SERVICE_DIRECTORY" = $serviceName.ToUpperInvariant();
     }
 
-    if (!$FederatedAuth) {
+    if ($ServicePrincipalAuth) {
         $deploymentOutputs["${serviceDirectoryPrefix}CLIENT_ID"] = $TestApplicationId;
         $deploymentOutputs["${serviceDirectoryPrefix}CLIENT_SECRET"] = $TestApplicationSecret;
         $deploymentOutputs["${serviceDirectoryPrefix}TENANT_ID"] = $azContext.Tenant.Id;
@@ -543,9 +529,8 @@ try {
         }
     }
 
-    # If a provisioner service principal was provided (and not using Federated
-    # Auth), log into it to perform the pre- and post-scripts and deployments.
-    if ($ProvisionerApplicationId -and !$FederatedAuth) {
+    # If a provisioner service principal was provided log into it to perform the pre- and post-scripts and deployments.
+    if ($ProvisionerApplicationId -and $ServicePrincipalAuth) {
         $null = Disable-AzContextAutosave -Scope Process
 
         Log "Logging into service principal '$ProvisionerApplicationId'."
@@ -640,9 +625,9 @@ try {
         }
     }
 
-    if (!$CI -and !$FederatedAuth -and $UserAuth) {
+    if (!$CI -and !$ServicePrincipalAuth) {
         if ($TestApplicationId) {
-            Write-Warning "The specified TestApplicationId '$TestApplicationId' will be ignored when UserAuth is set."
+            Write-Warning "The specified TestApplicationId '$TestApplicationId' will be ignored when -ServicePrincipalAutth is not set."
         }
 
         $userAccount = (Get-AzADUser -UserPrincipalName (Get-AzContext).Account)
@@ -712,7 +697,7 @@ try {
     # Make sure pre- and post-scripts are passed formerly required arguments.
     $PSBoundParameters['TestApplicationId'] = $TestApplicationId
     $PSBoundParameters['TestApplicationOid'] = $TestApplicationOid
-    if (!$FederatedAuth) {
+    if ($ServicePrincipalAuth) {
         $PSBoundParameters['TestApplicationSecret'] = $TestApplicationSecret
     }
 
@@ -732,7 +717,7 @@ try {
    # considered a critical failure, as the test application may have subscription-level permissions and not require
    # the explicit grant.
    if (!$resourceGroupRoleAssigned) {
-        $idSlug = if ($UserAuth) { "User '$userAccountName' ('$TestApplicationId')" } else { "Test Application '$TestApplicationId'"};
+        $idSlug = if (!$ServicePrincipalAuth) { "User '$userAccountName' ('$TestApplicationId')" } else { "Test Application '$TestApplicationId'"};
         Log "Attempting to assign the 'Owner' role for '$ResourceGroupName' to the $idSlug"
         $ownerAssignment = New-AzRoleAssignment `
                             -RoleDefinitionName "Owner" `
@@ -762,7 +747,7 @@ try {
     if ($TenantId) {
         $templateParameters.Add('tenantId', $TenantId)
     }
-    if ($TestApplicationSecret -and !$FederatedAuth) {
+    if ($TestApplicationSecret -and $ServicePrincipalAuth) {
         $templateParameters.Add('testApplicationSecret', $TestApplicationSecret)
     }
 
@@ -1044,30 +1029,15 @@ The environment file will be named for the test resources template that it was
 generated for. For ARM templates, it will be test-resources.json.env. For
 Bicep templates, test-resources.bicep.env.
 
-.PARAMETER UserAuth
-Create the resource group and deploy the template using the signed in user's credentials.
-No service principal will be created or used.
-
-The environment file will be named for the test resources template that it was
-generated for. For ARM templates, it will be test-resources.json.env. For
-Bicep templates, test-resources.bicep.env.
-
 .PARAMETER SuppressVsoCommands
 By default, the -CI parameter will print out secrets to logs with Azure Pipelines log
 commands that cause them to be redacted. For CI environments that don't support this (like
 stress test clusters), this flag can be set to $false to avoid printing out these secrets to the logs.
 
 .PARAMETER ServicePrincipalAuth
-Use the signed in user's credentials to create a service principal for
-provisioning. This is useful for some local development scenarios.
-
-.PARAMETER FederatedAuth
-Use signed in user's credentials for provisioninig. No service principal will be
-created. This is used in CI where the execution context already has a signed in
-user.
-
-In cases where provisioner or test applications are specified, secrets for those
-apps will not be exported or made available to pre- or post- scripts.
+Use the provisioner SP credentials to deploy, and pass the test SP credentials
+to tests. If provisioner and test SP are not set, provision an SP with user
+credentials and pass the new SP to tests.
 
 .EXAMPLE
 Connect-AzAccount -Subscription 'REPLACE_WITH_SUBSCRIPTION_ID'

eng/common/TestResources/Remove-TestResources.ps1

@@ -56,8 +56,10 @@ param (
     [ValidateSet('test', 'perf')]
     [string] $ResourceType = 'test',
 
+    [Parameter(ParameterSetName = 'Default+Provisioner')]
+    [Parameter(ParameterSetName = 'ResourceGroup+Provisioner')]
     [Parameter()]
-    [switch] $FederatedAuth,
+    [switch] $ServicePrincipalAuth,
 
     [Parameter()]
     [switch] $Force,
@@ -113,7 +115,7 @@ function Retry([scriptblock] $Action, [int] $Attempts = 5) {
     }
 }
 
-if ($ProvisionerApplicationId -and !$FederatedAuth) {
+if ($ProvisionerApplicationId -and $ServicePrincipalAuth) {
     $null = Disable-AzContextAutosave -Scope Process
 
     Log "Logging into service principal '$ProvisionerApplicationId'"
@@ -308,9 +310,8 @@ Run script in CI mode. Infers various environment variable names based on CI con
 .PARAMETER Force
 Force removal of resource group without asking for user confirmation
 
-.PARAMETER FederatedAuth
-Use signed in user's credentials for provisioninig. This is used in CI where
-the execution context already has a signed in user.
+.PARAMETER ServicePrincipalAuth
+Log in with provided Provisioner application credentials.
 
 .EXAMPLE
 Remove-TestResources.ps1 keyvault -Force

eng/common/TestResources/deploy-test-resources.yml

@@ -69,7 +69,6 @@ steps:
             @subscriptionConfiguration `
             -AdditionalParameters ${{ parameters.ArmTemplateParameters }} `
             -CI `
-            -UseFederatedAuth `
             -Force `
             -Verbose | Out-Null
 
@@ -92,6 +91,7 @@ steps:
           @subscriptionConfiguration `
           -AdditionalParameters ${{ parameters.ArmTemplateParameters }} `
           -CI `
+          -ServicePrincipalAuth `
           -Force `
           -Verbose | Out-Null
       displayName: Deploy test resources

eng/common/TestResources/remove-test-resources.yml

@@ -46,7 +46,6 @@ steps:
             -ServiceDirectory "${{ parameters.ServiceDirectory }}" `
             -CI `
             -Force `
-            -UseFederatedAuth `
             -Verbose
 
   - ${{ else }}:
@@ -61,6 +60,7 @@ steps:
           @subscriptionConfiguration `
           -ResourceType '${{ parameters.ResourceType }}' `
           -ServiceDirectory "${{ parameters.ServiceDirectory }}" `
+          -ServicePrincipalAuth `
           -CI `
           -Force `
           -Verbose