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

use MSI for az login #116

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

chaudhryfaisal
Copy link

@chaudhryfaisal chaudhryfaisal commented Jan 11, 2021

rancher/rke#2413

use MSI for az login if useManagedIdentityExtension=true and aaClientSecret is empty

@superseb
Copy link
Contributor

Thanks for the PR, can you please create an issue in rancher/rke explaining what you ran into and why this is needed (if we ever need to lookup why something was added).

@chaudhryfaisal
Copy link
Author

@superseb issue was created 2 days ago

@chaudhryfaisal
Copy link
Author

@superseb would you be able to review this PR?

@@ -32,8 +33,12 @@ set_azure_config() {
az cloud set --name ${azure_cloud}

# login to Azure
az login --service-principal -u ${azure_client_id} -p ${azure_client_secret} --tenant ${azure_tenant_id} 2>&1 > /dev/null

if [ "$az_managed_identity_extension" = "true" ] && [ "${azure_client_secret}" = "" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the curly braces on az_managed_identity_extension

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couldnt we also make this easier with no param and just say if azure_client_secret is empty to fall back on an az login --idtentity ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would also work for usecase where az_managed_identity_extension is true however want to use different credentials for azure_client_secret

@luthermonson
Copy link
Contributor

you are going going to need to add the same functionality to the powershell windows file: https://github.com/rancher/rke-tools/blob/master/windows/cloud-provider.psm1

Copy link
Author

@chaudhryfaisal chaudhryfaisal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have powershell / windows to test, feel free to criticize / update / suggest changes

6a8ac93

@@ -32,8 +33,12 @@ set_azure_config() {
az cloud set --name ${azure_cloud}

# login to Azure
az login --service-principal -u ${azure_client_id} -p ${azure_client_secret} --tenant ${azure_tenant_id} 2>&1 > /dev/null

if [ "$az_managed_identity_extension" = "true" ] && [ "${azure_client_secret}" = "" ]; then
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would also work for usecase where az_managed_identity_extension is true however want to use different credentials for azure_client_secret

@superseb
Copy link
Contributor

Please rebase on master so we don't run CI on removed Windows versions

@chaudhryfaisal
Copy link
Author

Please rebase on master so we don't run CI on removed Windows versions

Done

@superseb superseb requested review from a team and removed request for luthermonson and rosskirkpat August 30, 2022 09:59
@a-blender
Copy link

Does this PR still need reviews? It's been over 6 months since it was updated.

@Sartigan
Copy link

I'm confused on why this PR has never been implemented. You either use managed identity or a Service Principals not both...?

We're having similar issues with Linux + Windows Nodes. Please assist.

@superseb
Copy link
Contributor

Windows support for RKE1 was stopped September 1 2022. I don't believe this will be addressed anymore.

@aiyengar2 @HarrisonWAffel I believe we can close this?

@superseb superseb removed the request for review from a team September 11, 2023 12:50
@Sartigan
Copy link

Although I agree for windows node, it should at least be done for Linux images

@superseb
Copy link
Contributor

@Sartigan My bad, I agree. I will put it in the queue

@superseb superseb requested a review from a team September 11, 2023 15:24
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.

6 participants