login action for azurestack by rakku-ms · Pull Request #41 · Azure/login

Rakesh Kumar (rakku-ms) · 2020-07-21T05:36:22Z

No description provided.

Usha N (N-Usha)

These changes look good to me. I would let Akshaya or some engineering folks to review as well. Few comments/queries from my side -

The new attributes expected in the SPN are optional rt? I mean for non-Azure stack scenarios, user would not need to supply these in the JSON object. Is that rt?
What is the attribute of profileVersion? Are there any acceptable values for it?
What are the acceptable values for "customEnvironmentName" ? Does the user need to specify in any specific format? I assume these additional attributes will be returned when user runs the Az CLI command "az ad sp create-for-rbac --sdk-auth". In case user manually tries to construct the JSOn object, we may need to provide the required guidance for this. Also should we have any additional checks in the code to validate that the values we get for this string follow the required notation?

Rakesh Kumar (rakku-ms) · 2020-07-28T20:35:03Z

These changes look good to me. I would let Akshaya or some engineering folks to review as well. Few comments/queries from my side -

The new attributes expected in the SPN are optional rt? I mean for non-Azure stack scenarios, user would not need to supply these in the JSON object. Is that rt?

Yes, they are optional. not needed in the JSON object for non-Azure stack scenarios

What is the attribute of profileVersion? Are there any acceptable values for it?

Can be one of these {2017-03-09-profile, 2018-03-01-hybrid, 2019-03-01-hybrid, latest}. Azure Stack currently supports '2019-03-01-hybrid'

What are the acceptable values for "customEnvironmentName" ? Does the user need to specify in any specific format?

Can be any unique name (such as AzureStack) other than known clouds ('AzureCloud', 'AzureUSGovernment', etc)

I assume these additional attributes will be returned when user runs the Az CLI command "az ad sp create-for-rbac --sdk-auth". In case user manually tries to construct the JSOn object, we may need to provide the required guidance for this. Also should we have any additional checks in the code to validate that the values we get for this string follow the required notation?

No, these 2 additional attributes (profileVersion and customEnvironmentName) will not be generated by the above Azure CLI cmd. User will need to manually add these to the existing JSON object. We will provide the required guidance in the doc. We can add the validation for profileVersion parameter but the list of acceptable values will change in future. Hence, we will need to update the code whenever new profileVersion is released. I prefer documenting this instead of adding validation. But based on the review suggestions, I can add validation.

Usha N (N-Usha) · 2020-07-29T06:51:58Z

Thanks Rakesh Rakesh Kumar (@rakku-ms) . Few additional comments on below

"No, these 2 additional attributes (profileVersion and customEnvironmentName) will not be generated by the above Azure CLI cmd. User will need to manually add these to the existing JSON object. We will provide the required guidance in the doc. We can add the validation for profileVersion parameter but the list of acceptable values will change in future. Hence, we will need to update the code whenever new profileVersion is released. I prefer documenting this instead of adding validation. But based on the review suggestions, I can add validation."

If the additional attributes are to be provided manually by the user, we believe that these should be inputs in action.yml than fudging the details into the JSON object. Let the JSON be what the output of Az CLI command is. This will also give us an advantage of providing guidance for each input as field "description" in the action.yml - which also shows up when the action is selected in the marketplace search in the right pane experience while editing an action workflow.
For "customEnvironmentName", we believe there should be a standard list of allowed values which should include one for "AzureStack" which should be validatable in the code and hence its not recommended to accommodate "Can be any unique name other than known clouds "
IMO seeking "profileVersion" from the customers is not an optimal experience as they may have a dependency on referring to documentation each time for a supported notation and latest version detail. Could we instead avoid this input, hardcode the value and keep bumping it up whenever there is a new API version update? Could you please confirm the release cycle for these versions? How often are they updated? And would there be backward compatibility issues if the version is not updated?

bganapa · 2020-07-30T15:48:45Z

Agree. instead of placing the details in the json object, it makes sense for them to be part of action.yml, Can you pleas confirm that the values in the action.yml could be overrriden in the individual workflow yml? if the customer want to run a work flow against azure and azurestack, allowing the override would be helpful.
customEnvironmentName can be anything, it is not restricted to a known set. For example the customer may have multiple azurestack environments and name them in any way they want. We do not restrict the name to a set in CLI or powershell (az could register -n / Add-AzEnvironment -Name )
profiles point to a set of RP api versions and allow customers to stick to them without consuming any breaking changes inadvertently. AzureStack is a boxed product and we support last three versions. These three versions may be on a different profile. There is no ARM call to get to know the supported API profile from the stack environment at the moment. So it is essential to provide the option of specifying the profile as a parameter. We can have the default value as the latest supported API profile .

Usha N (@N-Usha) , cc: Rakesh Kumar (@rakku-ms)

Rakesh Kumar (rakku-ms) · 2020-08-03T21:21:08Z

Moved profileVersion and customEnvironmentName parameters from secret json object to workflow yml
Added keyvault and storage endpoints suffixes during cloud registration

FYI: Usha N (@N-Usha) bganapa

Ashish Ranjan (asranja) · 2020-08-05T04:50:44Z

No, these 2 additional attributes (profileVersion and customEnvironmentName) will not be generated by the above Azure CLI cmd. User will need to manually add these to the existing JSON object. We will provide the required guidance in the doc. We can add the validation for profileVersion parameter but the list of acceptable values will change in future. Hence, we will need to update the code whenever new profileVersion is released. I prefer documenting this instead of adding validation. But based on the review suggestions, I can add validation.

Rakesh Kumar (@rakku-ms) Will the az ad sp create-for-rbac command populate appropriate value for resourceManagerEndpointUrl when run against Azure Stack environment or do users need to specify it manually ?

Ashish Ranjan (asranja) · 2020-08-05T05:02:10Z

            throw new Error("Not all values are present in the creds object. Ensure clientId, clientSecret, tenantId and subscriptionId are supplied.");
        }
        // Attempting Az cli login
+        if (customEnvironmentName != "") {


Can we fix the customEnvironmentName to be AzureStack and check for that condition here?

Rakesh Kumar (@rakku-ms) - Could you please confirm if there is a strong requirement for knowing the Environment "name"? Or could we just seek Environment "type" which can just be an allowed list of keywords like "AzureStack" ?

yes, A customer can have several instances of AzureStack and the name cannot be limited to a particular set. You can see that when we do 'az cloud register' , the name is a parameter for customer to name it whatever they want.

bganapa

In our Azure DevOps world, we use a list of pre-defined values for the different cloud types. Users targeting Azure Stack, only needs to select AzureStack as the environment and provide the Azure Stack server URL.

The az cloud register persists for only one session and cannot be reused in a new session. Also, a user cannot login into multiple environments in a single workflow job. I am not sure if we are gaining anything by letting a user name the environment whatever they want. If the user wants to differentiate between the instances, he can specify it in the job name, something like :

name: Run on Azure Stack jobs: run-on-instance-1: - uses: azure/login@v1 with: environment: AzureStack creds: ${{ secrets.AZURE_CREDENTIALS_FOR_INSTANCE_1 }} run-on-instance-2: - uses: azure/login@v1 with: environment: AzureStack creds: ${{ secrets.AZURE_CREDENTIALS_FOR_INSTANCE_2 }}

We also do not ask for profile version in Azure DevOps, just the Server Url. Can this value be fixed in the action instead of taking input from user.

Ashish Ranjan (@asranja) I am using self-hosted runner as Azure Stack env is not publicly accessible from internet. For me registered cloud is getting persisted on my desktop. So, I am unable to run workflow on 2 different Azure Stack environment with same environmentName AzureStack. The second run also happens against the first since the cloud with the name AzureStack already exists. The register cmd doesn't overwrite the previous environment and just try to login to the existing one. One workaround would be to first remove the already existing AzureStack environment and then try to register. If this is what your team prefers, I will make this change. FYI: bganapa

PS login> az cloud list --output table IsActive Name Profile ---------- -------------------- ----------------- False AzureCloud latest False AzureChinaCloud latest False AzureUSGovernment latest False AzureGermanCloud latest False AzureStackRedmond001 2019-03-01-hybrid False AzureStackTest01 2019-03-01-hybrid False AzureStackTest02 2019-03-01-hybrid False AzureStackLH01 2019-03-01-hybrid False AzureStackRedmond 2019-03-01-hybrid

Rakesh Kumar (@rakku-ms) Thanks for giving more details regarding the environment name. We will discuss this internally and get back.

Regarding the profile version: As per my understanding, most Azure resources will have different api-version depending on the profile version. Currently, we have hard-coded all the api-versions in our REST Apis, for example. I am not sure how are we planning on updating the rest api-versions depending on the profile version.

Rakesh Kumar (@rakku-ms) - Can you please help understand how many customers usually need to access two or more different Azure stack environments? Coz am still wondering if this is a real issue, why isn't that any of the Azure DevOps customers ever flag this as a blocker?

Ashish Ranjan (asranja) · 2020-08-05T05:03:12Z

        let tenantId = secrets.getSecret("$.tenantId", false);
        let subscriptionId = secrets.getSecret("$.subscriptionId", false);
+        let resourceManagerEndpointUrl = secrets.getSecret("$.resourceManagerEndpointUrl", false);
+        let profileVersion = core.getInput("profileVersion");


inputs have to be added in action.yml file as well

added environmentName and profileVersion in action.yml

Ashish Ranjan (asranja) · 2020-08-05T05:03:45Z

        let subscriptionId = secrets.getSecret("$.subscriptionId", false);
+        let resourceManagerEndpointUrl = secrets.getSecret("$.resourceManagerEndpointUrl", false);
+        let profileVersion = core.getInput("profileVersion");
+        let customEnvironmentName = core.getInput("customEnvironmentName");


call it just environment instead of customEnvironmentName?

environmentName sounds good to me

updated to environmentName

Rakesh Kumar (rakku-ms) · 2020-08-05T20:31:42Z

No, these 2 additional attributes (profileVersion and customEnvironmentName) will not be generated by the above Azure CLI cmd. User will need to manually add these to the existing JSON object. We will provide the required guidance in the doc. We can add the validation for profileVersion parameter but the list of acceptable values will change in future. Hence, we will need to update the code whenever new profileVersion is released. I prefer documenting this instead of adding validation. But based on the review suggestions, I can add validation.

Rakesh Kumar (@rakku-ms) Will the az ad sp create-for-rbac command populate appropriate value for resourceManagerEndpointUrl when run against Azure Stack environment or do users need to specify it manually ?

Yes, sp Azure CLI cmd will populate the correct value for resourceManagerEndpointUrl when run against AAD based Azure Stack. We don't support this cmd for ADFS based Azure Stack. We plan to write a json generator script to do this job for ADFS based Azure Stack.

Rakesh Kumar (rakku-ms) · 2020-08-19T17:38:21Z

Ashish Ranjan (@asranja) let us know if you have any other concerns regarding this PR. If not, can we please publish this?
FYI, bganapa

Ashish Ranjan (asranja) · 2020-08-21T06:14:27Z

    required: false
    default: false
+  environment:
+    description: 'Name of the environment (Set this to AzureStack for AzureStack environment)'


Usha N (@N-Usha) for the description.

We also need to mention a default value for this parameter with value: public. Description can be "Name of the environment. Supported values are AzureStack, public. Default being public". And is the value expected to be case sensitive?

Usha N (@N-Usha) I have updated the description. Also made modification to make this parameter case insensitive.

Usha N (N-Usha) · 2020-08-24T09:53:33Z

    required: false
    default: false
+  environment:
+    description: 'Name of the environment (Set this to AzureStack for AzureStack environment)'


We also need to mention a default value for this parameter with value: public. Description can be "Name of the environment. Supported values are AzureStack, public. Default being public". And is the value expected to be case sensitive?

Usha N (N-Usha) · 2020-08-25T10:31:16Z

IMO we should NOT be merging the changes into master but rather into a new branch exclusively for AzureStackSupport-Beta... Naveen Desu (@nadesu) to reconfirm. I or one of our team members can create the new branch if you want us to.. Rakesh Kumar (@rakku-ms) - Please confirm.

Naveen Desu (nadesu) · 2020-08-25T10:36:25Z

IMO we should NOT be merging the changes into master but rather into a new branch exclusively for AzureStackSupport-Beta... Naveen Desu (@nadesu) to reconfirm. I or one of our team members can create the new branch if you want us to.. Rakesh Kumar (@rakku-ms) - Please confirm.

I agree with the above proposal

Rakesh Kumar (rakku-ms) · 2020-08-25T18:43:47Z

IMO we should NOT be merging the changes into master but rather into a new branch exclusively for AzureStackSupport-Beta... Naveen Desu (@nadesu) to reconfirm. I or one of our team members can create the new branch if you want us to.. Rakesh Kumar (@rakku-ms) - Please confirm.

I agree with the above proposal

Usha N (@N-Usha) Naveen Desu (@nadesu) yes, please create a new branch for AzureStackSupport. FYI, bganapa sarathys

Usha N (N-Usha) · 2020-08-26T07:52:38Z

IMO we should NOT be merging the changes into master but rather into a new branch exclusively for AzureStackSupport-Beta... Naveen Desu (@nadesu) to reconfirm. I or one of our team members can create the new branch if you want us to.. Rakesh Kumar (@rakku-ms) - Please confirm.

I agree with the above proposal

Usha N (@N-Usha) Naveen Desu (@nadesu) yes, please create a new branch for AzureStackSupport. FYI, bganapa sarathys

https://github.com/Azure/login/tree/AzureStackSupport-Beta - Please use this branch.

Rakesh Kumar (rakku-ms) · 2020-08-27T18:56:32Z

https://github.com/Azure/login/tree/AzureStackSupport-Beta - Please use this branch.

thanks Usha N (@N-Usha)!
FYI, garvitarai

Rakesh Kumar (rakku-ms) force-pushed the azslogin branch from e98e7f4 to 99a7777 Compare July 21, 2020 06:27

Rakesh Kumar (rakku-ms) added 2 commits July 21, 2020 11:21

login action for azurestack

3f560d5

test update for azurestack

c8d5cdd

Rakesh Kumar (rakku-ms) force-pushed the azslogin branch from 99a7777 to c8d5cdd Compare July 21, 2020 18:21

sarathys approved these changes Jul 24, 2020

View reviewed changes

Usha N (N-Usha) reviewed Jul 28, 2020

View reviewed changes

bganapa reviewed Jul 30, 2020

View reviewed changes

Comment thread src/main.ts Outdated

review changes

4e60fd4

Ashish Ranjan (asranja) reviewed Aug 5, 2020

View reviewed changes

Comment thread src/main.ts Outdated

review changes v2

8e1d6e9

Rakesh Kumar (rakku-ms) force-pushed the azslogin branch 2 times, most recently from 2afa2e1 to 359d40b Compare August 17, 2020 21:15

Ashish Ranjan (asranja) reviewed Aug 18, 2020

View reviewed changes

Comment thread src/PowerShell/Utilities/ScriptBuilder.ts

Ashish Ranjan (asranja) reviewed Aug 18, 2020

View reviewed changes

Comment thread src/main.ts

Rakesh Kumar (rakku-ms) force-pushed the azslogin branch 2 times, most recently from 6a1dfa1 to 5c8c57a Compare August 19, 2020 06:08

bganapa approved these changes Aug 19, 2020

View reviewed changes

Ashish Ranjan (asranja) reviewed Aug 21, 2020

View reviewed changes

Usha N (N-Usha) suggested changes Aug 24, 2020

View reviewed changes

review changes v3

9e18a8d

Rakesh Kumar (rakku-ms) force-pushed the azslogin branch from 5c8c57a to 9e18a8d Compare August 25, 2020 00:03

Rakesh Kumar (rakku-ms) requested a review from Usha N (N-Usha) August 25, 2020 00:06

Usha N (N-Usha) approved these changes Aug 25, 2020

View reviewed changes

Ashish Ranjan (asranja) changed the base branch from master to AzureStackSupport-Beta August 27, 2020 14:31

Ashish Ranjan (asranja) approved these changes Aug 27, 2020

View reviewed changes

Ashish Ranjan (asranja) merged commit 513cdfa into Azure:AzureStackSupport-Beta Aug 28, 2020

Uh oh!

Conversation

Rakesh Kumar (rakku-ms) commented Jul 21, 2020

Uh oh!

Usha N (N-Usha) left a comment

Choose a reason for hiding this comment

Uh oh!

Rakesh Kumar (rakku-ms) commented Jul 28, 2020

Uh oh!

Usha N (N-Usha) commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bganapa commented Jul 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Rakesh Kumar (rakku-ms) commented Aug 3, 2020

Uh oh!

Ashish Ranjan (asranja) commented Aug 5, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Usha N (N-Usha) Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rakesh Kumar (rakku-ms) Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rakesh Kumar (rakku-ms) Aug 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rakesh Kumar (rakku-ms) commented Aug 5, 2020

Uh oh!

Uh oh!

Uh oh!

Rakesh Kumar (rakku-ms) commented Aug 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Usha N (N-Usha) commented Aug 25, 2020

Uh oh!

Naveen Desu (nadesu) commented Aug 25, 2020

Uh oh!

Rakesh Kumar (rakku-ms) commented Aug 25, 2020

Uh oh!

Usha N (N-Usha) commented Aug 26, 2020

Uh oh!

Rakesh Kumar (rakku-ms) commented Aug 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Usha N (N-Usha) commented Jul 29, 2020 •

edited

Loading

bganapa commented Jul 30, 2020 •

edited

Loading

Usha N (N-Usha) Aug 10, 2020 •

edited

Loading

Rakesh Kumar (rakku-ms) Aug 5, 2020 •

edited

Loading

Rakesh Kumar (rakku-ms) Aug 5, 2020 •

edited

Loading