kubernetes-sigs / cluster-api-provider-azure

Cluster API implementation for Microsoft Azure
https://capz.sigs.k8s.io/
Apache License 2.0
297 stars 428 forks source link

Clarify `identityRef` and `identity` fields in `AzureManagedControlPlaneClassSpec ` #5202

Open djryanj opened 1 month ago

djryanj commented 1 month ago

When deploying a managed control plane thus:

apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: AzureManagedControlPlane
metadata:
  name: poc
spec:
  location: canadacentral
  resourceGroupName: rg
  # sshPublicKey: ${AZURE_SSH_PUBLIC_KEY_B64:=""}
  subscriptionID: 00000000-0000-0000-0000-000000000000 # fake uuid
  version: v1.30.0
  networkPolicy: azure
  networkPlugin: azure
  nodeResourceGroupName: rg2
  sku:
    tier: Free # or Standard
  identity:
    type: UserAssigned
    userAssignedIdentityResourceID: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/rg/providers/Microsoft.ManagedIdentity/userAssignedIdentities/uai
  identityRef:
    kind: AzureClusterIdentity
    name: cluster-identity
    namespace: to-be-patched
---
apiVersion: infrastructure.cluster.x-k8s.io/v1beta1
kind: AzureClusterIdentity
metadata:
  name: cluster-identity
spec:
  type: WorkloadIdentity
  allowedNamespaces:
    list:
    - <cluster-namespace>
  tenantID: <your-tenant-id>
  clientID: <your-client-id>

If you don't provide identity for a user assigned identity, it won't deploy with the user UAI, even though identityRef is provided (and the intention is to use that reference). In my case, both identity and identityRef point to the same identity. I realize that may not be the intention, but I can't find any descriptive documentation on what each really does to be able to understand each one.

From https://capz.sigs.k8s.io/reference/v1beta1-api#infrastructure.cluster.x-k8s.io/v1beta1.AzureManagedControlPlaneClassSpec:

identityRef
Kubernetes core/v1.ObjectReference | IdentityRef is a reference to a AzureClusterIdentity to be used when reconciling this cluster -- | --

And:

identity
Identity | (Optional) Identity configuration used by the AKS control plane. -- | --

Given the names, I would assume that they do the same thing, where one is a reference and the other is a directly encoded value. However, based on the CRD documentation, it seems like one is used by CAPI to reconcile, and the other is used by the cluster itself.

Any clarity would be appreciated.

nojnhuh commented 1 month ago

Your interpretation of the CRD docs is correct. identityRef represents the credentials used when invoking Azure to create the AKS cluster. identity maps to the same-named field in the AKS API which assigns that identity to the control plane of the AKS cluster.

This is an unfortunate intersection between our tendency to try to align CAPZ field names with AKS API field names and the prior decision to call that field identityRef in all the other CAPZ types that use it. The good news is that the new ASO-based API doesn't use AzureClusterIdentity and therefore doesn't have an identityRef field to confuse with the identity AKS cluster property.

Overall, I think your correct read of the docs is a sign that we're doing about as well as we can here without changing the API. Is there some other specific change you'd like to see?

djryanj commented 1 month ago

Hi @nojnhuh , thanks for the reply.

Given that the interpretation is correct, I can think of a few possible ideas.

  1. Clearer documentation on the AzureManagedControlPlane CRD itself and in other places in the book. As a suggestion:
    • identityRef: A reference to an AzureClusterIdentity that is used by Cluster API to create and manage the cluster via the Azure API.
    • identity: (Optional) Identity to be used by the control plane of the created AKS cluster, i.e., for use with Azure Workload Identity
  2. Along with the previous point, on the AzureClusterIdentity CRD, perhaps some clarity on what it's used for:
    • AzureClusterIdentity is used by Cluster API to create and manage clusters via the Azure API.
  3. Clearer documentation on the generated templates and e.g., https://capz.sigs.k8s.io/topics/workload-identity (or wherever else this intersection and happen).

I was about to suggest modifications to the documentation around moving away from v1 API towards ASO as potential v2, but I see that you have that in there already and since it's not set in stone, nothing to do there.

As a general statement, I think the documentation could be much improved. While I completely understand "only" providing an API reference, the lack of rendered YAML templates as documentation is frustrating to work with (but this is due in part to my general aversion to needing to install clusterctl alongside the plethora of other CLI tools when it's entirely unneeded--but that's beside the point). In a big improvement over the base cluster API book, CAPZ includes plenty of legitimate full YAMLs in the docs, but I think a full section on them would be a huge usability improvement. That said, if the project is open to it, I may open a PR or two with that goal in mind based on my recent experiences with CAPZ.