[AzureCLI] Loading service principal profile on request in azure config #8819
Conversation
Tasks/AzureCLIV1/azureclitask.ts
Outdated
| //login using svn | ||
| this.throwIfError(tl.execSync("az", "login --service-principal -u \"" + servicePrincipalId + "\" -p \"" + cliPassword + "\" --tenant \"" + tenantId + "\""), tl.loc("LoginFailed")); | ||
| this.isLoggedIn = true; | ||
| //set the subscription imported to the current subscription | ||
| this.throwIfError(tl.execSync("az", "account set --subscription \"" + subscriptionID + "\""), tl.loc("ErrorInSettingUpSubscription")); | ||
| } | ||
|
|
||
| private static loadServicePrincipalsIfRequired(servicePrincipalId, client_key, subscriptionID): void { | ||
| var loadFileName = tl.getVariable("LOAD_SERVICE_PRINCIPAL"); |
There was a problem hiding this comment.
We should not expect the user to specify the file name as the file name is kind of fixed in the az cli code and expect user to specify true/false only.
Secondly LOAD word does not look right. We should change it to CREATE_SERVICE_PRINCIPAL_FILE
Thirdly if there is a logout action in the task, we should delete the file as well.
There was a problem hiding this comment.
What is the scenario for this requirement?
There was a problem hiding this comment.
This is required for supporting a scenario where the user wants to use the SPN details of the serviceEndpoint while creating a new AKS cluster, instead of creating a new service principal every time.
There was a problem hiding this comment.
Got it. It also addresses #8726 . Should the secrets rather be set as secret variables instead of this file for easier consumption? Also I believe this file will get deleted after the task exists (due to logout) - is that correct?
There was a problem hiding this comment.
This file is a convention followed by az cli which we figured out by following the source code.
I noticed az logout doesn't explicitly delete that file, so I'm adding logic for unlinking upon exit.
There was a problem hiding this comment.
If this is the already saved by az cli - then why do we need to store it again?
There was a problem hiding this comment.
I meant, the file is a convention for the az aks module i.e. it searches for this file in the config path, otherwise tries to create a new service-principal for AKS cluster creation.
This file is not available by default.
There was a problem hiding this comment.
Got it. Given that we have requests for use cases other than aks (as pointed above), and also this file syntax is an internal implementation of aks (subject to change) - should we consider the environment variables so that this feature is more usable for all scenarios.
There was a problem hiding this comment.
Azure endpoint can take client key or a certificate. Az CLI today does no support certificate yet (Azure PS does). @Ajay-MS would likely enhance Az CLI as well. Want to know if the solution proposed will scale for both client key and certificate?
Tasks/AzureCLIV1/azureclitask.ts
Outdated
|
|
||
| // create config.json | ||
| this.loadServicePrincipalsIfRequired(servicePrincipalId, cliPassword, subscriptionID); | ||
|
|
||
| //set the subscription imported to the current subscription | ||
| this.throwIfError(tl.execSync("az", "account set --subscription \"" + subscriptionID + "\""), tl.loc("ErrorInSettingUpSubscription")); | ||
| } | ||
|
|
||
| private static loadServicePrincipalsIfRequired(servicePrincipalId, client_key, subscriptionID): void { |
There was a problem hiding this comment.
rename the function as well to createServicePrincipalsIfRequired
Tasks/AzureCLIV1/azureclitask.ts
Outdated
| JSON.stringify({ [subscriptionID]: { client_secret: client_key, service_principal: servicePrincipalId } }) | ||
| ); | ||
|
|
||
| fs.link(fileName, path.join(this.azCliConfigPath, fileName)); |
There was a problem hiding this comment.
Why are you not creating the file in the eventual location path.join(this.azCliConfigPath, fileName) itself?
| fs.link(fileName, path.join(this.azCliConfigPath, fileName)); | |
| fs.writeFileSync(path.join(this.azCliConfigPath, fileName), JSON.stringify..); |
There was a problem hiding this comment.
writeFileSync fails to create a new file if it is outside the application directory. And azCliConfigPath is outside it, so I had to create it here, and move it there.
There was a problem hiding this comment.
Just for my knowledge, does link method move the file, or creates a link? if it just links, we would need to delete the original file as well.
Tasks/AzureCLIV1/azureclitask.ts
Outdated
| //login using svn | ||
| this.throwIfError(tl.execSync("az", "login --service-principal -u \"" + servicePrincipalId + "\" -p \"" + cliPassword + "\" --tenant \"" + tenantId + "\""), tl.loc("LoginFailed")); | ||
| this.isLoggedIn = true; | ||
| //set the subscription imported to the current subscription | ||
| this.throwIfError(tl.execSync("az", "account set --subscription \"" + subscriptionID + "\""), tl.loc("ErrorInSettingUpSubscription")); | ||
| } | ||
|
|
||
| private static loadServicePrincipalsIfRequired(servicePrincipalId, client_key, subscriptionID): void { | ||
| var loadFileName = tl.getVariable("LOAD_SERVICE_PRINCIPAL"); |
There was a problem hiding this comment.
If this is the already saved by az cli - then why do we need to store it again?
… users/desattir/azureCLIstoreCreds
No description provided.