azure-cli-extensions icon indicating copy to clipboard operation
azure-cli-extensions copied to clipboard

[AKS] Service principal overrides enable-managed-identity

Open helayoty opened this issue 2 years ago • 5 comments

Describe the bug

CLI should prevent --service-principal and --enable-managed-identity from being used together in a single command.

Related command

az aks create

Errors

The current behavior will not return any error, but the service principal will be used, and the managed identity option will be ignored, which is confusing.

Issue script & Debug output

NA

Expected behavior

Expected behavior: "You can't set both service-principal and managed-identity"

Environment Summary

"azure-cli": "2.53.1", "azure-cli-core": "2.53.1", "azure-cli-telemetry": "1.1.0", "extensions": { "account": "0.2.5", "aks-preview": "0.5.168", "application-insights": "0.1.16", "azure-devops": "0.26.0", "front-door": "1.0.17", "ssh": "2.0.2" }

Additional context

No response

helayoty avatar Nov 20 '23 23:11 helayoty

Thank you for opening this issue, we will look into it.

yonzhan avatar Nov 20 '23 23:11 yonzhan

cc @FumingZhang - this might be an easy fix, not sure?

matthchr avatar Nov 20 '23 23:11 matthchr

Actually, the fix would be a breaking change. In az aks create, the default value of option --enable-managed-identity is True, which means that regardless of whether the user specifies this option, the cluster to be created will use msi, unless the user additionally specifies --service-principal and --client-secret, in this case, the code will backfill the value of --enable-managed-identity to False. From a behavioral point of view, when these three options are specified at the same time, the value of --enable-managed-identity is ignored.

Although setting the default value of an --enable-xxx option to True may seem bizarre, this behavior existed before I started refactoring the cli code about 3 years ago.

Fix in PR #27887. May have to wait until the next breaking change window (MS Build 24, around June) to merge this change.

FumingZhang avatar Nov 21 '23 03:11 FumingZhang

@FumingZhang Thanks for the fix. But the current description may lead to misleading: The default value is being changed to false but mentioned true here. image I understand that the default behavior won't change even default vault is being set to false, but can we have a better description for this parameter? Or users may think "the default AKS may no longer created with system-assigned managed identity since the default value is false".

JoeyC-Dev avatar May 17 '24 08:05 JoeyC-Dev

Thanks for pointing this out, fixing in #29036, #7661

FumingZhang avatar May 28 '24 02:05 FumingZhang