-
Notifications
You must be signed in to change notification settings - Fork 65
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
Support for system-assigned and user-assigned managed identities #330
base: master
Are you sure you want to change the base?
Conversation
- Managed identities are formerly known as MSIs Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
- If we are using an authentication scheme other than client secret returning a map of credentials secret data is no longer possible. Thus, we need a change in the function signature. - azure.GetAuthInfo now returns subscription ID, which is used by the downstream ARM resource SDK clients. Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
pkg/clients/compute/aks.go
Outdated
// EnsureManagedCluster ensures the supplied AKS cluster exists, including | ||
// ensuring any required service principals and role assignments exist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to update this comment since we're no longer ensuring SP creation and its role assignments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ezgidemirel. Removed mentions of the service principals and role assignments as we are no longer dealing with them.
pkg/clients/compute/aks.go
Outdated
// DeleteManagedCluster deletes the supplied AKS cluster, including its service | ||
// principals and any role assignments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, we're not deleting SP or its role assignments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -280,38 +129,40 @@ func newManagedCluster(c *v1alpha3.AKSCluster, appID, secret string) containerse | |||
{ | |||
Name: to.StringPtr(AgentPoolProfileName), | |||
Count: &nodeCount, | |||
VMSize: containerservice.VMSizeTypes(c.Spec.NodeVMSize), | |||
VMSize: to.StringPtr(c.Spec.NodeVMSize), | |||
Mode: containerservice.AgentPoolModeSystem, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though system mode is set in here, I got the following error when I tried to create an AKS cluster with a user-managed identity
Warning CannotCreateExternalResource 96s (x25 over 3m21s) managed/akscluster.compute.azure.crossplane.io cannot create AKSCluster: containerservice.ManagedClustersClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="MustDefineAtLeastOneSystemPool" Message="Must define at least one system pool."
I used following manifest:
apiVersion: compute.azure.crossplane.io/v1alpha3
kind: AKSCluster
metadata:
name: ezgi-akscluster
labels:
example: "true"
spec:
resourceGroupName: ezgi
vnetSubnetIDRef:
name: ezgi-sub
location: West US 2
version: "1.21.7"
nodeCount: 1
nodeVMSize: Standard_B2s
dnsNamePrefix: ezgi-aks
disableRBAC: false
identity:
identityNames:
- ezgiidentity
type: UserAssigned
providerConfigRef:
name: example
writeConnectionSecretToRef:
namespace: crossplane-system
name: ezgi-akscluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ezgidemirel,
Thank you for giving this a try. It looks like to be an issue specific to provisioning AKS clusters as you have mentioned below that provisioning some other resources were successful. Using SP credentials, I tried provisioning an AKS cluster with success using the example manifest from this PR. You are using the provider package ulucinar/provider-azure:build-3eacbce2
, right?
I will also give it a try using a user-assigned managed identity in ProviderConfig and report back to you, although it should not be related.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello again,
Using the provider package ulucinar/provider-azure:build-3eacbce2
, on an AKS cluster with a user-assigned managed kubelet identity, I tried to provision an AKS cluster using the following manifests successfully:
apiVersion: azure.crossplane.io/v1beta1
kind: ProviderConfig
metadata:
name: example
spec:
clientID: <client-id of the user-assigned managed kubelet identity>
credentials:
source: InjectedIdentity
subscriptionID: <Azure subscription ID>
---
apiVersion: compute.azure.crossplane.io/v1alpha3
kind: AKSCluster
metadata:
name: example-akscluster-alper2
labels:
example: "true"
spec:
resourceGroupName: alper
location: West US 2
version: "1.21.7"
nodeCount: 1
nodeVMSize: Standard_B2s
dnsNamePrefix: crossplane-aks
disableRBAC: false
identity:
type: SystemAssigned
providerConfigRef:
name: example
writeConnectionSecretToRef:
namespace: crossplane-system
name: example-akscluster
I have also verified accessing the provisioned AKS cluster using the credentials (kubeconfig) stored in the connection secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The AKS cluster I tried to provision has UserAssigned
managed identity. I believe that's the reason why my attempt is failing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ezgidemirel,
The following also succeeded in my cluster:
apiVersion: compute.azure.crossplane.io/v1alpha3
kind: AKSCluster
metadata:
name: example-akscluster-alper2
labels:
example: "true"
spec:
resourceGroupName: alper
location: West US 2
version: "1.21.7"
nodeCount: 1
nodeVMSize: Standard_B2s
dnsNamePrefix: crossplane-aks
disableRBAC: false
identity:
type: UserAssigned
identityNames:
- control-plane-identity-alper
providerConfigRef:
name: example
writeConnectionSecretToRef:
namespace: crossplane-system
name: example-akscluster
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updating resource manifests to use the same location didn't work. However, removing vnetSubnetIDRef
from the spec did the trick. I could successfully provision an AKS cluster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that we are not setting the default node pool's mode to System iff a subnet is specified :)
Fixed the bug and successfully provisioned an AKS cluster by specifying a subnet for its node pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @ezgidemirel for finding this issue and helping me in fixing it. Very much appreciated!
pkg/clients/compute/aks.go
Outdated
for _, n := range c.Spec.Identity.IdentityNames { | ||
resourceID := fmt.Sprintf(fmtUserAssignedManagedIdentityID, subscriptionID, c.Spec.ResourceGroupName, n) | ||
p.Identity.UserAssignedIdentities[resourceID] = &containerservice.ManagedClusterIdentityUserAssignedIdentitiesValue{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still wonder what is the use case of having multiple user identities here :) I'll dive in later.
I tested the PR with user-assigned managed identity with following steps:
While preparing a cluster with user-managed identity, I noticed that kubelet's identity should also be set. Following curl command helped me to troubleshoot. I created a pod with curl binary in the cluster and executed it inside it:
|
Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
…d identities instead of the plain identity names Signed-off-by: Alper Rifat Ulucinar <ulucinar@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a ton @ulucinar for adding managed identity support, fixing an existing bug and, cleaning up some SP generation code for AKS clusters. I learned a lot while reviewing/testing it. Amazing work from an amazing mentor :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
@turkenh I have a client who would like to use Manage Identities. I would be happy to try and help fix whatever CodeCov is saying is wrong but I do not have access to see what the problem is. Can you let us know what it is or help to get it addressed so that we can all start using these amazingly helpful changes from @ulucinar? |
any estimate on this feature delivery? it would be really useful |
I would also love to see this PR merged because it would simplify our current setup using Crossplane with Service Principals a lot! |
Any chance we can get this merged? |
Any ETA here? Hoping this helps me automate giving the role "DNS Zone Contributor" to my AKS clusters so external-dns will work. |
I'm looking at implementing this provider. The feature you've introduced aligns perfectly with our current needs. Thank you for your valuable work @ulucinar ! I see this was approved on @turkenh / @ezgidemirel if possible, could you revisit this matter for a second evaluation? The report is no longer available, and a previous user mentioned that external users don't have access to it. Your assistance in this matter would be greatly appreciated. |
Any updates? Would love to see this feature implemented |
Description of your changes
Fixes #164, #292, #319
This PR adds support for system-assigned and user-assigned managed identities, formerly known as Managed Service Identities (MSIs) via the Azure Identity client module.
An example
ProviderConfig
that uses a system-assigned managed identity is as follows:Another example using a user-assigned managed identity:
I have:
make reviewable test
to ensure this PR is ready for review.How has this code been tested
I have successfully provisioned a
ResourceGroup
and anAKSCluster
using a system-assigned managed identity and also a user-assigned managed identity with theProviderConfig
s given above.