Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AzurePowerShell@2,3,4,5] [AzureFileCopy@2,3,4] [SqlAzureDacpacDeployment@1] Update Initialize-AzSubscription to use endpoint Environment in Connect-AzAccount to work with MSIs in AzureUSGov, ChinaCloud, etc. #14533

Merged
merged 30 commits into from
Apr 7, 2021

Conversation

cutecycle
Copy link
Contributor

@cutecycle cutecycle commented Mar 4, 2021

Task name: AzurePowerShell@5

Description: pass the environment to Connect-AzAccount.

Documentation changes required: (Y/N) N

Added unit tests: (Y/N) N

Attached related issue: (Y/N) Y: #14176

Managed Identity Service Connection environment names are available in the VSTS serviceConnection API, but not passed to Connect-AzAccount in AzurePowerShell@V5.

Initialize-AzSubscription passes the endpoint environment parameter only in the Service Principal branch of the elseif.

In our case, This causes RMProfileClient to attempt to search for an AzureUSGovernment subscription GUID in AzureCloud by name and not find it:

https://github.com/Azure/azure-powershell/blob/master/src/Accounts/Accounts/Models/RMProfileClient.cs#L352

2021-03-05T21:39:04.6976485Z ##[command]Clear-AzContext -Scope CurrentUser -Force -ErrorAction SilentlyContinue
2021-03-05T21:39:05.7611370Z ##[command]Clear-AzContext -Scope Process
2021-03-05T21:39:06.0440331Z ##[command]Connect-AzAccount -Identity @processScope
2021-03-05T21:39:07.6645186Z ##[command] Set-AzContext -SubscriptionId *** -TenantId ***
2021-03-05T21:39:07.8301202Z VERBOSE: Leaving Initialize-AzModule.
2021-03-05T21:39:07.8325880Z ##[debug]Error record:
2021-03-05T21:39:07.9355981Z ##[debug]Set-AzContext : Please provide a valid tenant or a valid subscription.
2021-03-05T21:39:07.9369095Z ##[debug]At C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.184.1\ps_modules\VstsAzureHelpers_\InitializeAzModuleFunctions.ps1:193 char:13
2021-03-05T21:39:07.9381118Z ##[debug]+     $null = Set-AzContext -SubscriptionId $SubscriptionId @additional
2021-03-05T21:39:07.9393365Z ##[debug]+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
2021-03-05T21:39:07.9406011Z ##[debug]    + CategoryInfo          : CloseError: (:) [Set-AzContext], ArgumentException
2021-03-05T21:39:07.9417862Z ##[debug]    + FullyQualifiedErrorId : Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand
2021-03-05T21:39:07.9445664Z ##[debug] 

I've added the -Environment parameter to Initialize-AzSubscription.

    } elseif ($Endpoint.Auth.Scheme -eq 'ManagedServiceIdentity') {
        try {
            Write-Host "##[command]Connect-AzAccount -Environment $environmentName -Identity @processScope"
            $null = Connect-AzAccount -Environment $environmentName -Identity @processScope
...

Shouldn't Connect-AzAccount fail?

I thought so, so I tested the assumption and found that the MSI successfully connects to AzureCloud even if the self-hosted agent virtual machine is in an AzureUSGovernment subscription:

##[debug] Arguments: '-NoLogo -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -Command ". 'C:\agent\_work\_temp\10e49590-89b6-42d4-84fe-884dd65006a5.ps1'"'
##[debug] FileName: 'C:\windows\System32\WindowsPowerShell\v1.0\powershell.exe'
##[debug] WorkingDirectory: 'C:\agent\_work\1\s'
"C:\windows\System32\WindowsPowerShell\v1.0\powershell.exe" -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -Command ". 'C:\agent\_work\_temp\10e49590-89b6-42d4-84fe-884dd65006a5.ps1'"

Name                                     Account             SubscriptionName    Environment         TenantId          
----                                     -------             ----------------    -----------         --------          
*** ... MSI@50342                               AzureCloud          ***..
 Set-AzContext -SubscriptionId *** -TenantId ***
Set-AzContext : Please provide a valid tenant or a valid subscription.
At C:\agent\_work\_temp\10e49590-89b6-42d4-84fe-884dd65006a5.ps1:778 char:13
+     $null = Set-AzContext -SubscriptionId $SubscriptionId @additional
+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : CloseError: (:) [Set-AzContext], ArgumentException
    + FullyQualifiedErrorId : Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand

Specifying -Environment fixes this:


Name                                     Account             SubscriptionName    Environment         TenantId          
----                                     -------             ----------------    -----------         --------          
*** ***... MSI@50342           ***        AzureUSGovernment   ***...
 Set-AzContext -SubscriptionId *** -TenantId ***
*** (***... MSI@50342           ***       AzureUSGovernment  ***...

Checklist:

  • Task version was bumped - please check instruction how to do it
  • Checked that applied changes work as expected

@cutecycle cutecycle marked this pull request as draft March 4, 2021 16:09
@cutecycle cutecycle changed the title Update AzurePowerShell connection to function with SPNs in AzureUSGov, ChinaCloud, etc. Update AzurePowerShell connection to use -Envirnoment in connect function for SPNs in AzureUSGov, ChinaCloud, etc. Mar 4, 2021
@cutecycle cutecycle changed the title Update AzurePowerShell connection to use -Envirnoment in connect function for SPNs in AzureUSGov, ChinaCloud, etc. Update AzurePowerShell connection to use -Environment in connect function for SPNs in AzureUSGov, ChinaCloud, etc. Mar 4, 2021
@cutecycle cutecycle marked this pull request as ready for review March 4, 2021 16:21
@cutecycle cutecycle marked this pull request as draft March 4, 2021 16:31
@cutecycle
Copy link
Contributor Author

noting: it wasn't the Connect-AzAccount, it was the Set-AzContext; updated

@cutecycle
Copy link
Contributor Author

noting: Set-AzContext lacks an environment parameter

@cutecycle cutecycle changed the title Update AzurePowerShell connection to use -Environment in connect function for SPNs in AzureUSGov, ChinaCloud, etc. Update AzurePowerShell connection to use context environment in Set-AzContext for SPNs in AzureUSGov, ChinaCloud, etc. Mar 4, 2021
@cutecycle cutecycle marked this pull request as ready for review March 4, 2021 18:00
@cutecycle cutecycle changed the title Update AzurePowerShell connection to use context environment in Set-AzContext for SPNs in AzureUSGov, ChinaCloud, etc. Update AzurePowerShell connection to use Connect-AzAccount-derived tenantId in Set-AzContext to work with SPNs in AzureUSGov, ChinaCloud, etc. Mar 4, 2021
@cutecycle cutecycle marked this pull request as draft March 4, 2021 20:02
@cutecycle
Copy link
Contributor Author

More complicated than expected: the module responsible is not AzurePowerShell@5/initializeaz.ps1, it's InitializeAzModuleFunctions -> Set-CurrentAzureSubscription:


 Set-AzContext -SubscriptionId *** -TenantId ***
VERBOSE: Leaving Initialize-AzModule.
##[debug]Error record:
##[debug]Set-AzContext : Please provide a valid tenant or a valid subscription.
##[debug]At C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.182.0\ps_modules\VstsAzureHelpers_\InitializeAzModuleFunctions.ps1:190 char:13
##[debug]+     $null = Set-AzContext -SubscriptionId $SubscriptionId @additional
##[debug]+             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
##[debug]    + CategoryInfo          : CloseError: (:) [Set-AzContext], ArgumentException
##[debug]    + FullyQualifiedErrorId : Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand
##[debug] 
##[debug]Script stack trace:
##[debug]at Set-CurrentAzSubscription, C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.182.0\ps_modules\VstsAzureHelpers_\InitializeAzModuleFunctions.ps1: line 190
##[debug]at Initialize-AzSubscription, C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.182.0\ps_modules\VstsAzureHelpers_\InitializeAzModuleFunctions.ps1: line 146
##[debug]at Initialize-AzModule, C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.182.0\ps_modules\VstsAzureHelpers_\InitializeAzModuleFunctions.ps1: line 17
##[debug]at <ScriptBlock>, C:\agent\_work\_tasks\AzurePowerShell_72a1931b-effb-4d2e-8fd8-f8472a07cb62\5.182.0\CoreAz.ps1: line 21
##[debug]at <ScriptBlock>, C:\agent\_work\_temp\e1ff9025-60f0-4984-adc5-0f012d6b93d3.ps1: line 3
##[debug]at <ScriptBlock>, <No file>: line 1
##[debug]Exception:
##[debug]System.ArgumentException: Please provide a valid tenant or a valid subscription.
##[debug]   at Microsoft.Azure.Commands.ResourceManager.Common.RMProfileClient.SetCurrentContext(String subscriptionNameOrId, String tenantId, String name)
##[debug]   at Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand.<>c__DisplayClass37_0.<ExecuteCmdlet>b__2(AzureRmProfile profile, RMProfileClient client, String name)
##[debug]   at Microsoft.Azure.Commands.Profile.Common.AzureContextModificationCmdlet.ModifyContext(Action`2 contextAction)
##[debug]   at Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand.SetContextWithOverwritePrompt(Action`3 setContextAction)
##[debug]   at Microsoft.Azure.Commands.Profile.SetAzureRMContextCommand.ExecuteCmdlet()
##[debug]   at Microsoft.WindowsAzure.Commands.Utilities.Common.AzurePSCmdlet.ProcessRecord()

@cutecycle cutecycle changed the title Update AzurePowerShell connection to use Connect-AzAccount-derived tenantId in Set-AzContext to work with SPNs in AzureUSGov, ChinaCloud, etc. Update AzurePowerShell connection to use Connect-AzAccount-derived tenantId in Set-AzContext to work with MSIs in AzureUSGov, ChinaCloud, etc. Mar 5, 2021
@cutecycle cutecycle changed the title Update AzurePowerShell connection to use Connect-AzAccount-derived tenantId in Set-AzContext to work with MSIs in AzureUSGov, ChinaCloud, etc. Update InitializeAz to use endpoint Environment in Connect-AzAccount to work with MSIs in AzureUSGov, ChinaCloud, etc. Mar 6, 2021
@Macromullet
Copy link

Is there a way we can gain additional traction on this issue? It is a major productivity enhancement for teams that need to support both public and gov clouds in the same enterprise solution.

@Macromullet
Copy link

It looks like all comments are resolved. What do we need to get the merge?

@nadesu
Copy link
Contributor

nadesu commented Mar 26, 2021

@cutecycle @AmrutaKawade - Are the corresponding automated tests updated for this change?

@cutecycle
Copy link
Contributor Author

cutecycle commented Mar 26, 2021

@nadesu:

@cutecycle @AmrutaKawade - Are the corresponding automated tests updated for this change?

I want to be sure about a few things;

  • given this is a modification to Initialize-AzModule, which is Register-Mocked in ChecksForPowerShell.ps1, is the existing azure-pipelines-tasks/Tasks/AzurePowerShellV5/Tests/ChecksForPowerShell.ps1 sufficient, or should there be some kind of separate azure-pipelines-tasks/Tasks/AzurePowerShellV5/Tests/TestEnvironmentFlag.ps1?
  • given the number of dependent modules this affects, should there be tests in just AzurePowerShellv5, or also AzurePowershell 2,3,4, AzureFileCopy, SqlAzureDacpacDeployment, etc?

I'm having some difficulty running tests myself; when I run

npm i; node make.js test --task AzurePowerShellV5 --suite L0 

on Win10/powershell/node 14.15.1

I'm getting errors from a node dependency that's trying to use netcat to install an older node?

Downloading file: https://nodejs.org/dist/v6.10.3/win-x64/node.exe
Could not use "nc", falling back to slower node.js method for sync requests.
C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:77
    throw new Error(res.stderr.toString());
    ^

Error
    at doRequestWith (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:77:11)
    at doRequest (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:20:10)
    at downloadFile (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make-util.js:411:22)
    at Object.installNode (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make-util.js:376:35)
    at Function.target.test (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make.js:394:14)    at Object.global.target.<computed> [as test] (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:28:26)
    at C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:38:27
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:36:10)

should I try in a linux container, or is there some additional setup outside of installing and running test like this?

@nadesu
Copy link
Contributor

nadesu commented Mar 29, 2021

This is a short week for us due to local festivals. @AmrutaKawade will revert on this next week.

@AmrutaKawade
Copy link
Contributor

@nadesu:

@cutecycle @AmrutaKawade - Are the corresponding automated tests updated for this change?

I want to be sure about a few things;

  • given this is a modification to Initialize-AzModule, which is Register-Mocked in ChecksForPowerShell.ps1, is the existing azure-pipelines-tasks/Tasks/AzurePowerShellV5/Tests/ChecksForPowerShell.ps1 sufficient, or should there be some kind of separate azure-pipelines-tasks/Tasks/AzurePowerShellV5/Tests/TestEnvironmentFlag.ps1?
  • given the number of dependent modules this affects, should there be tests in just AzurePowerShellv5, or also AzurePowershell 2,3,4, AzureFileCopy, SqlAzureDacpacDeployment, etc?

I'm having some difficulty running tests myself; when I run

npm i; node make.js test --task AzurePowerShellV5 --suite L0 

on Win10/powershell/node 14.15.1

I'm getting errors from a node dependency that's trying to use netcat to install an older node?

Downloading file: https://nodejs.org/dist/v6.10.3/win-x64/node.exe
Could not use "nc", falling back to slower node.js method for sync requests.
C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:77
    throw new Error(res.stderr.toString());
    ^

Error
    at doRequestWith (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:77:11)
    at doRequest (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\sync-request\index.js:20:10)
    at downloadFile (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make-util.js:411:22)
    at Object.installNode (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make-util.js:376:35)
    at Function.target.test (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\make.js:394:14)    at Object.global.target.<computed> [as test] (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:28:26)
    at C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:38:27
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (C:\Users\ninareynolds\Documents\repos\ext\azure-pipelines-tasks\node_modules\shelljs\make.js:36:10)

should I try in a linux container, or is there some additional setup outside of installing and running test like this?

Go to root of azure-pipeline-tasks folder and run below commands
npm install
node_modules\.bin\mocha _build\Tasks\AzurePowerShellV5\Tests\L0.js

@AmrutaKawade
Copy link
Contributor

@cutecycle @AmrutaKawade - Are the corresponding automated tests updated for this change?

I have updated integration tests for this change.

@cutecycle
Copy link
Contributor Author

it looks like the ci jobs got condition-skipped... is this okay? I'd expect some failures given some tasks are marked as 184 while we're on sprint 186

@AmrutaKawade
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 4 pipeline(s).

@AmrutaKawade AmrutaKawade merged commit 8c190dc into microsoft:master Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants