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

Assign TenantID to managed cluster identity #144

Merged
merged 1 commit into from
Aug 4, 2023

Conversation

MAXxATTAXx
Copy link

This is helpful when the Service Principal and the Resource group do not belong in the same Tenant.

Before this change trying to do so would give error showing that the ClientID was not found under TenantID of the Resource group.

@kkaempf kkaempf requested review from a-blender and mjura March 6, 2023 08:22
@mjura
Copy link
Contributor

mjura commented Mar 7, 2023

@MAXxATTAXx could you provide us instruction, how can we reproduce your scenario? We would like to test it as well.

Thank you so much for opening this PR

Copy link
Contributor

@mjura mjura 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 merge, we have to reproduce this problem first

@MAXxATTAXx
Copy link
Author

In our scenario, we have new environments spawned in exernal Azure tenats to our system. They allow us access on a particular resource_group to deploy and manage an aks cluster. The Service Principal or ClientID belongs to us so the tenantID of the ClientID is different than the tenantID of the resource_group. In azure world it is called Managed Resource Group or Managed Applications.

pkg/aks/create.go Show resolved Hide resolved
@MAXxATTAXx MAXxATTAXx requested a review from a team as a code owner April 20, 2023 18:40
pkg/aks/create.go Show resolved Hide resolved
@furkatgofurov7
Copy link
Contributor

@MAXxATTAXx can you take a look at #144 (comment) and address the comment?

@kkaempf kkaempf added the kind/bug Something isn't working label May 9, 2023
pkg/aks/create.go Outdated Show resolved Hide resolved
@furkatgofurov7
Copy link
Contributor

This looks good to me, @MAXxATTAXx can we add some simple tests as well?

@MAXxATTAXx
Copy link
Author

@furkatgofurov7
Test were added for TenantID to be empty string and when TenantID is provided.

@furkatgofurov7
Copy link
Contributor

@furkatgofurov7 Test were added for TenantID to be empty string and when TenantID is provided.

@MAXxATTAXx thanks, lgtm, can you please squash your commits?

@mjura PTAL once again

@MAXxATTAXx
Copy link
Author

@furkatgofurov7 Commits squashed and rebased to master

Copy link
Contributor

@mjura mjura left a comment

Choose a reason for hiding this comment

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

It looks OK

Could you please add explanation to commit message why this change is needed? Thank you

This allows for cloudCredentials to be provisioned in a different tenant
than the cluster. This is useful for scenarios where the cluster is
provisioned in a tenant that is not the same as the tenant that the
credentials are provisioned in.
@MAXxATTAXx MAXxATTAXx marked this pull request as ready for review July 17, 2023 16:46
@MAXxATTAXx
Copy link
Author

@mjura I updated the commit message

Copy link
Contributor

@mjura mjura left a comment

Choose a reason for hiding this comment

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

LGTM

@mjura mjura merged commit f712444 into rancher:master Aug 4, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Development

Successfully merging this pull request may close these issues.

8 participants