Users/ajya/nationalcloudclientsidechanges #3551

Ajay-MS · 2017-02-07T11:25:29Z

No description provided.

desattir · 2017-02-07T12:56:14Z

LGTM 👍

sachinma

I have looked at the changes for AzureRG only

sachinma · 2017-02-08T04:39:25Z

Tasks/AzureResourceGroupDeployment/operations/azure-rest/azure-arm-common.ts

    private token_deferred: Q.Promise<string>;

-    constructor(clientId: string, domain: string, secret: string) {
+    constructor(clientId: string, domain: string, secret: string, armUrl: string, authUrl: string) {


authUrl is confusing since auth is mostly used for authentication. I would prefer full name authorityUrl

asranja · 2017-02-08T05:43:10Z

Tasks/AzureNLBManagement/nlbazureutility.ts


 	var deferred = Q.defer<string>();
-	var authorityUrl = authUrl + SPN.tenantID + "/oauth2/token/";
+	var envAuthUrl = endpoint.envAuthUrl;


Check if it is necessary to hard-code envAuthUrl if it is null.

vincent1173

Looks Good.
✅ Azure App Service Manage Task
✅ Azure App Service Deploy Task

chshrikh · 2017-02-08T11:13:07Z

Tasks/Common/VstsAzureHelpers_/InitializeFunctions.ps1

@@ -41,6 +41,11 @@ function Initialize-AzureSubscription {

    #Set UserAgent for Azure Calls
    Set-UserAgent
+
+    $environmentName = "AzureCloud"
+    if( $Endpoint.Data.Environment ) {


this does not validate if it contains an empty string with space. Please check if you want to trim it before it gets used

chshrikh · 2017-02-08T11:13:54Z

Tasks/Common/VstsAzureHelpers_/InitializeFunctions.ps1

@@ -127,7 +127,7 @@ function Initialize-AzureSubscription {
            # Else, this is AzureRM.
            try {
                Write-Host "##[command]Add-AzureRMAccount -ServicePrincipal -Tenant $($Endpoint.Auth.Parameters.TenantId) -Credential $psCredential"
-                $null = Add-AzureRMAccount -ServicePrincipal -Tenant $Endpoint.Auth.Parameters.TenantId -Credential $psCredential
+                $null = Add-AzureRMAccount -ServicePrincipal -Tenant $Endpoint.Auth.Parameters.TenantId -Credential $psCredential -EnvironmentName $environmentName


better to log $environmentName if it is within security rules

kmkumaran · 2017-02-08T11:28:17Z

Tasks/AzureResourceGroupDeployment/task.json

@@ -321,6 +321,8 @@
        "ClientIdCannotBeEmpty": "clientId must be a non empty string.",
        "DomainCannotBeEmpty": "domain must be a non empty string.",
        "SecretCannotBeEmpty": "secret must be a non empty string.",
+        "armUrlCannotBeEmpty": "arm Url must be a non empty string.",


Is the plan only to add support in latest task version for ARG? I am fine with that call, but want to confirm with PMs.

kmkumaran · 2017-02-08T12:01:57Z

Tasks/Common/VstsAzureRestHelpers_/VstsAzureRestHelpers_.psm1

@@ -1,8 +1,8 @@
 # Private module-scope variables.
 $script:jsonContentType = "application/json;charset=utf-8"
 $script:formContentType = "application/x-www-form-urlencoded;charset=utf-8"
-$script:azureRmUri = "https://management.azure.com"
-$script:authUri = "https://login.microsoftonline.com/"
+$script:defaultAuthUri = "https://login.microsoftonline.com/"


I can't find this URL in the get-Environment output. What url does this map to?

login.windows.net is legacy auth URL, login.microsoftonline.com is latest auth url. In environment property its showing only latest one. Since other URL is being used at many places in application so added it as another property.

kmkumaran · 2017-02-08T12:03:01Z

Tasks/Common/VstsAzureRestHelpers_/VstsAzureRestHelpers_.psm1

+    $authUrl = $script:defaultAuthUri
+    if($endpoint.Data.activeDirectoryAuthority)
+    {
+        $authUrl = $endpoint.Data.activeDirectoryAuthority


Is activeDirecotryAuthority and environment Auth Url used interchangeably?

In the GetAccessToken in nlbUtility you were using the environment Auth url. Why?

kmkumaran · 2017-02-08T12:04:54Z

Tasks/Common/VstsAzureHelpers_/InitializeFunctions.ps1

@@ -41,6 +41,11 @@ function Initialize-AzureSubscription {

    #Set UserAgent for Azure Calls
    Set-UserAgent
+


Can you ensure Azure PowerShell task is covered as part of your testing?

Yes I have tested Azure Powershell Task

kmkumaran · 2017-02-15T12:35:16Z

Tasks/AzureNLBManagement/nlbazureutility.ts

@@ -5,19 +5,20 @@ import * as httpClient from 'vso-node-api/HttpClient';
 import * as restClient from 'vso-node-api/RestClient';

 var httpObj = new httpClient.HttpCallbackClient(tl.getVariable("AZURE_HTTP_USER_AGENT"));
-var authUrl = 'https://login.windows.net/';
+var defaultAuthUrl = 'https://login.windows.net/';
 var azureApiVersion = '2016-09-01';


Did this API version work fine in other Cloud environment types. Do you see any issue with Azure deployments causing issues?

kmkumaran · 2017-02-15T12:36:36Z

Tasks/AzureNLBManagement/nlbazureutility.ts

 var azureApiVersion = '2016-09-01';

-function getAccessToken(SPN, endpointUrl: string): Q.Promise<string> {
+function getAccessToken(endpoint, endpointUrl: string): Q.Promise<string> {


having the second parameter as enpointurl is confusing. There is already endpoint taken as input which has endpoint.url. I presume this is the resource for which you are requesting token. So should we just call it resource?

kmkumaran · 2017-02-15T12:57:58Z

Tasks/Common/VstsAzureRestHelpers_/VstsAzureRestHelpers_.psm1

+    $authUrl = $script:defaultAuthUri
+    if($endpoint.Data.activeDirectoryAuthority)
+    {
+        $authUrl = $endpoint.Data.activeDirectoryAuthority


In the GetAccessToken in nlbUtility you were using the environment Auth url. Why?

* Changes for national cloud * consume arm url from endpoint * authUrl and envAuthUrl fix * Test correction for client side changes * bump up task version and fixing review comments * null check for auth url in NLB task * added URL in endpoint object for NLB task * added environment to logs * reverted variable name back to SPN * Bump up task version corrsponding to master * corrections * Increased patch version for AFC

* Users/ajya/nationalcloudclientsidechanges (#3551) * Changes for national cloud * consume arm url from endpoint * authUrl and envAuthUrl fix * Test correction for client side changes * bump up task version and fixing review comments * null check for auth url in NLB task * added URL in endpoint object for NLB task * added environment to logs * reverted variable name back to SPN * Bump up task version corrsponding to master * corrections * Increased patch version for AFC * back compat for xml sub * resolve merge conflict * loc.json updated * Increase patch version as release branch version has been bumbed up * Increased patch version due to vincent xml sub fix * patch version bump up due to vincent fix * default URL check for Deploy Azure RG Task * Increased patch version of AFC to build package

prValidator added 4 commits February 6, 2017 18:59

Changes for national cloud

75ff28b

consume arm url from endpoint

c0ea485

authUrl and envAuthUrl fix

c948cb9

Test correction for client side changes

23cbd25

msftclas added the cla-not-required label Feb 7, 2017

Ajay-MS requested review from kmkumaran, mvvsubbu, pavanadepu2, sachinma and desattir February 7, 2017 13:45

sachinma approved these changes Feb 8, 2017

View reviewed changes

bump up task version and fixing review comments

4162125

desattir approved these changes Feb 8, 2017

View reviewed changes

Ajay-MS assigned asranja Feb 8, 2017

Ajay-MS requested a review from asranja February 8, 2017 05:36

Ajay-MS unassigned asranja Feb 8, 2017

Ajay-MS requested a review from vincent1173 February 8, 2017 05:37

asranja approved these changes Feb 8, 2017

View reviewed changes

prValidator added 2 commits February 8, 2017 11:14

null check for auth url in NLB task

2ae2c5e

added URL in endpoint object for NLB task

a484fcf

Ajay-MS requested a review from chshrikh February 8, 2017 06:27

vincent1173 approved these changes Feb 8, 2017

View reviewed changes

chshrikh reviewed Feb 8, 2017

View reviewed changes

chshrikh approved these changes Feb 8, 2017

View reviewed changes

added environment to logs

047f93c

kmkumaran reviewed Feb 8, 2017

View reviewed changes

kmkumaran reviewed Feb 15, 2017

View reviewed changes

reverted variable name back to SPN

ff1f403

Ajay Yadav added 4 commits February 15, 2017 19:39

Bump up task version corrsponding to master

107a0ee

Merge branch 'master' into users/ajya/nationalcloudclientsidechanges

a5cea63

corrections

28c263d

Increased patch version for AFC

41b7a4e

Ajay-MS merged commit d539c24 into master Feb 16, 2017

bryanmacfarlane deleted the users/ajya/nationalcloudclientsidechanges branch January 31, 2018 00:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Users/ajya/nationalcloudclientsidechanges #3551

Users/ajya/nationalcloudclientsidechanges #3551

Ajay-MS commented Feb 7, 2017

desattir commented Feb 7, 2017

sachinma left a comment

sachinma Feb 8, 2017

asranja Feb 8, 2017

vincent1173 left a comment

chshrikh Feb 8, 2017

chshrikh Feb 8, 2017

kmkumaran Feb 8, 2017

kmkumaran Feb 8, 2017

Ajay-MS Feb 8, 2017

kmkumaran Feb 8, 2017

kmkumaran Feb 15, 2017

kmkumaran Feb 8, 2017

Ajay-MS Feb 9, 2017

kmkumaran Feb 15, 2017

kmkumaran Feb 15, 2017

kmkumaran Feb 15, 2017

		@@ -41,6 +41,11 @@ function Initialize-AzureSubscription {

		#Set UserAgent for Azure Calls
		Set-UserAgent

Users/ajya/nationalcloudclientsidechanges #3551

Users/ajya/nationalcloudclientsidechanges #3551

Conversation

Ajay-MS commented Feb 7, 2017

desattir commented Feb 7, 2017

sachinma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincent1173 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment