Support for AAD pod-managed identities

[scoof]

Is your feature request related to a problem?/Why is this needed
I thought I had support for pod-managed identities by providing a way to label the pods, but as I dug deeper, I realized it wasn't enough.

Describe the solution you'd like in detail
There are currently two things that needs to be changed to fully support pod-managed identities:

• hostNetwork: false
• Injecting azure.json with the correct identity or without identity at all.

I have this working by mounting a manually configured ConfigMap in the pod, and using the azure-cred-file configmap to point to this CM.

Disabling hostNetwork is easy enough, but injecting azure.json with at least tenant, subscription and resource-group is a bit harder, since we need to pick up those from somewhere. I don't know if the only option is to provide them as values to the helm chart and render the azure.json file manually.

This should also remove the requirement to mount any hostPath volumes in the pod.

Describe alternatives you've considered
The alternative is to keep using the node managed identity, and that would be a less secure option, since that would delegate too many rights to the CSI driver.

[andyzhangx]

for the hostNetwork: false setting, I could customize this setting in helm install, by default, hostNetwork is true since optimize disk performance feature depends on this setting.

[andyzhangx]

PR(#939) would add customized hostNetwork setting in helm install.

[scoof]

for the hostNetwork: false setting, I could customize this setting in helm install, by default, hostNetwork is true since optimize disk performance feature depends on this setting.

As far as I can see (device_perf_linux.go) this feature depends on querying the Azure API for data about disks, and tweaking stuff in /sys? The first should use AAD Pod ID, so would work fine with hostNetwork: false, while the latter just uses a hostPath? Am I missing anything?

[scoof]

The /csi/csi.sock unix socket mount in the node pod does seem to have issues with hostNetwork: false though.

I am mostly focusing on the controller pod at the moment, since that has the largest attack surface.

[scoof]

The /csi/csi.sock unix socket mount in the node pod does seem to have issues with hostNetwork: false though.

I am mostly focusing on the controller pod at the moment, since that has the largest attack surface.

I don't think it's the socket mount that is creating problems, but rather that azuredisk is not reaching the listening phase. It stops at: skus.go:121] NewNodeInfo: Starting to populate node and disk sku information. - so the problem is probably that it tries to get a token from IMDS, but is not authorized for that user. I'll try mounting an azure.json with an identity it can assume.

[andyzhangx]

The /csi/csi.sock unix socket mount in the node pod does seem to have issues with hostNetwork: false though.
I am mostly focusing on the controller pod at the moment, since that has the largest attack surface.

I don't think it's the socket mount that is creating problems, but rather that azuredisk is not reaching the listening phase. It stops at: skus.go:121] NewNodeInfo: Starting to populate node and disk sku information. - so the problem is probably that it tries to get a token from IMDS, but is not authorized for that user. I'll try mounting an azure.json with an identity it can assume.

@scoof device_perf is not an essential feature(disabled by default now), I think that's ok, you could go with hostNetwork: false

[scoof]

I have the node pods working with hostNetwork: false now - they just required a proper azure.json to be able to inspect the azure config

[scoof]

device_perf fails because the devices in /sys/block are symlinks to /sys/devices, and that is mounted ro. It works if I add a hostPath volume for /sys/devices, but I don't know if that is too broad (at least it is less broad than the full sysfs that hostNetwork: true provides)

[andyzhangx]

device_perf fails because the devices in /sys/block are symlinks to /sys/devices, and that is mounted ro. It works if I add a hostPath volume for /sys/devices, but I don't know if that is too broad (at least it is less broad than the full sysfs that hostNetwork: true provides)

@scoof device_perf depends on IMDS, so it won't work if hostNetwork is disabled, that' expected:

azuredisk-csi-driver/pkg/optimization/skus.go

Lines 120 to 131 in c8f7537

	func NewNodeInfo(cloud cloudprovider.Interface, nodeID string) (*NodeInfo, error) {
	klog.V(2).Infof("NewNodeInfo: Starting to populate node and disk sku information.")
	instances, ok := cloud.Instances()
	if !ok {
	return nil, status.Error(codes.Internal, "NewNodeInfo: Failed to get instances from Azure cloud provider")
	}
	instanceType, err := instances.InstanceType(context.TODO(), types.NodeName(nodeID))
	if err != nil {
	return nil, fmt.Errorf("NewNodeInfo: Failed to get instance type from Azure cloud provider, nodeName: %v, error: %v", nodeID, err)
	}

About how to set cloud config, you could follow: https://github.com/kubernetes-sigs/azuredisk-csi-driver#prerequisite

[scoof]

device_perf fails because the devices in /sys/block are symlinks to /sys/devices, and that is mounted ro. It works if I add a hostPath volume for /sys/devices, but I don't know if that is too broad (at least it is less broad than the full sysfs that hostNetwork: true provides)

@scoof device_perf depends on IMDS, so it won't work if hostNetwork is disabled, that' expected:

But the whole point of this exercise is to use the Pod Identity IMDS instead of the node IMDS, since that will allow us to constrain the identities the pod is able to assume.

[andyzhangx]

device_perf fails because the devices in /sys/block are symlinks to /sys/devices, and that is mounted ro. It works if I add a hostPath volume for /sys/devices, but I don't know if that is too broad (at least it is less broad than the full sysfs that hostNetwork: true provides)

@scoof device_perf depends on IMDS, so it won't work if hostNetwork is disabled, that' expected:

But the whole point of this exercise is to use the Pod Identity IMDS instead of the node IMDS, since that will allow us to constrain the identities the pod is able to assume.

@scoof just disable hostNetwork since device_perf optimization feature is not a required feature for now. Maybe we could leverage Pod Identity IMDS to get node info in the future.
cc @bcho

[scoof]

@scoof just disable hostNetwork since device_perf optimization feature is not a required feature for now. Maybe we could leverage Pod Identity IMDS to get node info in the future.

I think we're talking past each other. Device_perf works fine with Pod Identity, when configured in the same manner as the controller, and when adding a hostpath volumemount to /sys/devices.
By labelling the pod with aadpodidentity, we can allow the pod to assume a particular identity (which is more constrained than the kubelet identity), and then it's able to get node info just fine, since Pod Identity exposes a limited IMDS endpoint.

[scoof]

About how to set cloud config, you could follow: https://github.com/kubernetes-sigs/azuredisk-csi-driver#prerequisite

My problem is that there's no way to inject an azure.json that is specific to this particular deployment - the Secret method relies on a hard coded name, which will clash with any other pods using the same method with the same name.
I think viable solutions are either

• to allow the user to specify another secret name (this would require code changes and not just chart changes I think)
• to allow the user to specify a custom azure.json content, which would then be deployed as a configmap or secret, and mounted into the pods

I think the latter is the better option, and I can provide a PR for that if you wish

[andyzhangx]

@scoof secret name is hardcoded here:

azuredisk-csi-driver/pkg/azuredisk/azure.go

Lines 56 to 57 in c8f7537

	SecretName: "azure-cloud-provider",
	SecretNamespace: "kube-system",

do you think we should specify controller.cloudConfigSecretName, controller.cloudConfigSecretNamespace, node.cloudConfigSecretName, node.cloudConfigSecretNamespace in helm chart?

I could make driver code change also, it's not a big change.

[scoof]

@andyzhangx since we already have a way of pointing to an alternative azure.json, I think it would make more sense to just have the helm chart add a secret, mount that secret and point to it using AZURE_CREDENTIAL_FILE, that would allow the user to just specify the contents

[andyzhangx]

@andyzhangx since we already have a way of pointing to an alternative azure.json, I think it would make more sense to just have the helm chart add a secret, mount that secret and point to it using AZURE_CREDENTIAL_FILE, that would allow the user to just specify the contents

@scoof that's two options: 1) let driver read from different secrets 2) project different secrets to driver cloud config path.
anyway we need to specify controller.cloudConfigSecretName, controller.cloudConfigSecretNamespace, node.cloudConfigSecretName, node.cloudConfigSecretNamespace in helm chart since node and controller may need different cloud configs.

[scoof]

@scoof that's two options: 1) let driver read from different secrets 2) project different secrets to driver cloud config path.
anyway we need to specify controller.cloudConfigSecretName, controller.cloudConfigSecretNamespace, node.cloudConfigSecretName, node.cloudConfigSecretNamespace in helm chart since node and controller may need different cloud configs.

I have no preference of one over the other.