feat: enable system-assigned identity by default #3856

jackfrancis · 2020-09-22T19:44:42Z

Reason for Change:

This PR changes the defaults configuration assignment implementation to set useManagedIdentity to true by default, effectively enabling system-assigned identity for all node VMs.

Official Azure docs here:

https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/overview#managed-identity-types

In order to do so reliably, the API version for system-assigned identity has been updated to 2018-09-01-preview so that the roleAssignment resource doesn't reference a VM-derived service principal before it has replicated (that API version provides a solution for that race condition).

Note: this default change was not applied for Azure Stack, which doesn't support managed identity at this time.

Also modified the default --auth-method for all CLI gestures to cli, from client_secret, to further deprioritize the suggestion to rely upon manually curated service principals as part of the AKS Engine solution set. Using that auth method by default w/ system-assigned identity by default yields a much simpler working, default CLI expression:

$ aks-engine deploy --resource-group $RG --location $LOCATION --api-model $API_MODEL_JSON_PATH

Also updated doc examples to reflect this simpler default flow.

Issue Fixed:

Fixes #3885

Credit Where Due:

Does this change contain code from or inspired by another project?

No
Yes

If "Yes," did you notify that project's maintainers and provide attribution?

No
Yes

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

codecov · 2020-09-22T19:58:24Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5e6a1f1). Click here to learn what that means.
The diff coverage is 97.29%.

@@            Coverage Diff            @@
##             master    #3856   +/-   ##
=========================================
  Coverage          ?   72.99%           
=========================================
  Files             ?      149           
  Lines             ?    23201           
  Branches          ?        0           
=========================================
  Hits              ?    16935           
  Misses            ?     5156           
  Partials          ?     1110

Impacted Files	Coverage Δ
pkg/api/vlabs/types.go	`71.81% <ø> (ø)`
pkg/engine/armtype.go	`85.00% <ø> (ø)`
pkg/engine/params_k8s.go	`85.23% <50.00%> (ø)`
cmd/deploy.go	`60.59% <100.00%> (ø)`
cmd/generate.go	`49.18% <100.00%> (ø)`
cmd/root.go	`64.60% <100.00%> (ø)`
pkg/api/defaults.go	`92.91% <100.00%> (ø)`
pkg/api/types.go	`93.83% <100.00%> (ø)`
pkg/api/vlabs/validate.go	`79.64% <100.00%> (ø)`
pkg/engine/armresources.go	`83.47% <100.00%> (ø)`
... and 157 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6a1f1...2ca66f4. Read the comment docs.

CecileRobertMichon · 2020-09-22T20:37:00Z

pkg/api/defaults.go

+			!cs.Properties.IsCustomCloudProfile() &&
+			!cs.Properties.MasterProfile.IsVirtualMachineScaleSets() &&
+			o.KubernetesConfig.UseManagedIdentity == nil {
+			o.KubernetesConfig.UseManagedIdentity = to.BoolPtr(true)


Does this also enable it during upgrade? since we weren't setting a default before, most of my generated apimodels have no set value for useManagedIdentity

Good catch, addressed!

CecileRobertMichon · 2020-09-22T20:37:16Z

pkg/api/defaults.go

@@ -177,6 +177,13 @@ func (cs *ContainerService) setOrchestratorDefaults(isUpgrade, isScale bool) {
 			}
 		}

+		if !cs.Properties.IsHostedMasterProfile() &&
+			!cs.Properties.IsCustomCloudProfile() &&
+			!cs.Properties.MasterProfile.IsVirtualMachineScaleSets() &&


Why disable for VMSS masters?

According to vlabs/validate.go VMSS master VMs only support user-assigned identity (not system-assigned)

I actually ran some real world cluster buildouts to see what happens w/ VMSS control plane + system-assigned identity, and sure enough on the first test I am unable to create a Load Balancer:

Events: Type Reason Age From Message ---- ------ ---- ---- ------- Normal EnsuringLoadBalancer 2m25s (x8 over 12m) service-controller Ensuring load balancer Warning ListLoadBalancers 2m25s (x8 over 12m) azure-cloud-provider Retriable: false, RetryAfter: 0s, HTTPStatusCode: 400, RawError: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 400, RawError: azure.BearerAuthorizer#WithAuthorization: Failed to refresh the Token for request to https://management.azure.com/subscriptions/aa3d3369-e814-4495-899d-d31e8d7d09ce/resourceGroups/kubernetes-westus2-95074/providers/Microsoft.Network/loadBalancers?api-version=2019-06-01: StatusCode=400 -- Original Error: adal: Refresh request failed. Status Code = '400'. Response body: {"error":"invalid_request","error_description":"Identity not found"} Warning SyncLoadBalancerFailed 2m25s (x8 over 12m) service-controller Error syncing load balancer: failed to ensure load balancer: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 400, RawError: Retriable: false, RetryAfter: 0s, HTTPStatusCode: 400, RawError: azure.BearerAuthorizer#WithAuthorization: Failed to refresh the Token for request to https://management.azure.com/subscriptions/aa3d3369-e814-4495-899d-d31e8d7d09ce/resourceGroups/kubernetes-westus2-95074/providers/Microsoft.Network/loadBalancers?api-version=2019-06-01: StatusCode=400 -- Original Error: adal: Refresh request failed. Status Code = '400'. Response body: {"error":"invalid_request","error_description":"Identity not found"}

@ritazh @sozercan were you involved in implementing VMSS masters? Do you recall if this is one of the expected symptoms that dictates that system-assigned identity is not a working solution?

jackfrancis · 2020-09-25T22:13:03Z

Hold while we address race conditions in the template spec

Addressed!

jadarsie · 2020-09-29T16:21:39Z

pkg/api/defaults.go

@@ -177,6 +177,14 @@ func (cs *ContainerService) setOrchestratorDefaults(isUpgrade, isScale bool) {
 			}
 		}

+		if !isUpgrade && !isScale &&
+			!cs.Properties.IsHostedMasterProfile() &&
+			!cs.Properties.IsCustomCloudProfile() &&


Thx for taking custom clouds into account.

jackfrancis · 2020-10-02T19:34:23Z

cmd/root.go

@@ -146,6 +146,11 @@ func (authArgs *authArgs) validateAuthArgs() error {
 		return errors.New("--auth-method is a required parameter")
 	}

+	// Back-compat to accommodate existing client usage patterns that assume that "client-secret" is the default


@jadarsie Do you have any concerns here for Azure Stack? Does the az CLI auth model work for Azure Stack? If not, are you comfortable with this back-compat solution that "re-sets" the default auth method to client_secret if the service principal ID and password are included in the command line arguments?

The goal for us is to default to CLI as the auth model because for most users it is easier (don't have to generate/maintain service principals, easier command statements). If there's a reason that a local az context isn't possible, then this back-compat solution should work for pre-existing scripts that pass in the id and pass but haven't ever bothered to set the --auth-method to client_secret explicitly, because it's always been the default.

ping @jadarsie @haofan-ms

jackfrancis · 2020-10-02T19:37:47Z

cmd/deploy.go

@@ -270,13 +270,13 @@ func autofillApimodel(dc *deployCmd) error {
 		if dc.dnsPrefix == "" {
 			return errors.New("apimodel: missing masterProfile.dnsPrefix and --dns-prefix was not specified")
 		}
-		log.Warnf("apimodel: missing masterProfile.dnsPrefix will use %q", dc.dnsPrefix)


This shouldn't be a warning, it's literally a CLI feature

jackfrancis · 2020-10-02T19:49:54Z

docs/tutorials/quickstart.md

@@ -74,108 +74,48 @@ Note, we have launched a browser for you to login. For old experience with devic
 You have logged in. Now let us find all the subscriptions to which you have access...
 ```

-Next, we'll create a resource group. A resource group is a container that holds related resources for an Azure solution. In Azure, you logically group related resources such as storage accounts, virtual networks, and virtual machines (VMs) to deploy, manage, and maintain them as a single entity. In this case, we want to deploy, manage and maintain the whole Kubernetes cluster as a single entity.


These steps are actually unnecessary, so are not in the spirit of "quick start" for first time users.

👍 I have wondered before why we weren't letting aks-engine deploy create the RG for us...

jackfrancis · 2020-10-02T19:52:19Z

examples/addons/aad-pod-identity/README.md

@@ -41,10 +41,6 @@ This is the AAD Pod Identity add-on.  Add this add-on to your json file as shown
            }
          ]
        }
-      },


Cleaning up these placeholder objects as we're no longer emphasizing the service principal solution

jackfrancis · 2020-10-02T19:57:39Z

pkg/api/vlabs/validate.go

@@ -998,44 +994,6 @@ func (a *Properties) validateServicePrincipalProfile() error {
 	return nil
 }

-func (a *Properties) validateManagedIdentity() error {


This entire validation logic is to ensure you're using k8s >= 1.12.0, which is not necessary.

jackfrancis · 2020-10-09T16:52:26Z

/azp run pr-e2e

azure-pipelines · 2020-10-09T16:52:36Z

Azure Pipelines successfully started running 1 pipeline(s).

mboersma · 2020-10-13T16:24:42Z

docs/tutorials/quickstart.md

@@ -74,108 +74,48 @@ Note, we have launched a browser for you to login. For old experience with devic
 You have logged in. Now let us find all the subscriptions to which you have access...
 ```

-Next, we'll create a resource group. A resource group is a container that holds related resources for an Azure solution. In Azure, you logically group related resources such as storage accounts, virtual networks, and virtual machines (VMs) to deploy, manage, and maintain them as a single entity. In this case, we want to deploy, manage and maintain the whole Kubernetes cluster as a single entity.


👍 I have wondered before why we weren't letting aks-engine deploy create the RG for us...

docs/tutorials/quickstart.md

test/e2e/azure/cli.go

spelling correction Co-authored-by: Matt Boersma <Matt.Boersma@microsoft.com>

…/jackfrancis/aks-engine into system-assigned-identity-default

mboersma

/lgtm

acs-bot · 2020-10-13T19:17:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jackfrancis,mboersma]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

acs-bot added size/L approved labels Sep 22, 2020

CecileRobertMichon reviewed Sep 22, 2020

View reviewed changes

jackfrancis added the do-not-merge/hold label Sep 25, 2020

jadarsie reviewed Sep 29, 2020

View reviewed changes

jackfrancis force-pushed the system-assigned-identity-default branch from c0b127a to f9f0ce6 Compare October 2, 2020 00:32

acs-bot added size/XXL and removed size/L labels Oct 2, 2020

jackfrancis commented Oct 2, 2020

View reviewed changes

jackfrancis removed the do-not-merge/hold label Oct 2, 2020

jackfrancis commented Oct 2, 2020

View reviewed changes

jackfrancis force-pushed the system-assigned-identity-default branch 2 times, most recently from a930693 to 8170a5f Compare October 9, 2020 16:41

jackfrancis added 9 commits October 12, 2020 15:26

feat: enable system-assigned identity by default

acc8aa9

fix: don't change defaults for master vmss or customCloudProfile

beb78e7

don't change val during upgrade/scale

c3d176b

docs and example updates

6b22340

typo

ed25e4c

don't retry E2E if PrincipalNotFound

4424538

default to cli auth method, update docs

f4facf4

fix UT

36e3244

remove unnecessary client_secret auth examples

a92b91d

jackfrancis force-pushed the system-assigned-identity-default branch from 08c37d6 to a92b91d Compare October 12, 2020 22:26

mboersma reviewed Oct 13, 2020

View reviewed changes

jackfrancis and others added 3 commits October 13, 2020 09:59

Update docs/tutorials/quickstart.md

5c1332b

spelling correction Co-authored-by: Matt Boersma <Matt.Boersma@microsoft.com>

just remove this retry

f75f4bc

Merge branch 'system-assigned-identity-default' of https://github.com…

2ca66f4

…/jackfrancis/aks-engine into system-assigned-identity-default

mboersma approved these changes Oct 13, 2020

View reviewed changes

acs-bot assigned mboersma Oct 13, 2020

acs-bot added the lgtm label Oct 13, 2020

jackfrancis merged commit 0b6b43b into Azure:master Oct 13, 2020

jackfrancis deleted the system-assigned-identity-default branch October 13, 2020 19:18

penggu pushed a commit to penggu/aks-engine that referenced this pull request Oct 28, 2020

feat: enable system-assigned identity by default (Azure#3856)

852eb36

fmotrifork mentioned this pull request Dec 8, 2020

aks-engine 0.58.0 fishworks/fish-food#1098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable system-assigned identity by default #3856

feat: enable system-assigned identity by default #3856

jackfrancis commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

CecileRobertMichon Sep 22, 2020

jackfrancis Sep 22, 2020

CecileRobertMichon Sep 22, 2020

jackfrancis Sep 22, 2020

jackfrancis Oct 2, 2020

jackfrancis commented Sep 25, 2020 •

edited

Loading

jadarsie Sep 29, 2020

jackfrancis Oct 2, 2020

jackfrancis Oct 12, 2020

jackfrancis Oct 2, 2020

jackfrancis Oct 2, 2020

mboersma Oct 13, 2020

jackfrancis Oct 2, 2020

jackfrancis Oct 2, 2020

jackfrancis commented Oct 9, 2020

azure-pipelines bot commented Oct 9, 2020

mboersma Oct 13, 2020

mboersma left a comment

acs-bot commented Oct 13, 2020

feat: enable system-assigned identity by default #3856

feat: enable system-assigned identity by default #3856

Conversation

jackfrancis commented Sep 22, 2020 • edited Loading

codecov bot commented Sep 22, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Oct 9, 2020

azure-pipelines bot commented Oct 9, 2020

Choose a reason for hiding this comment

mboersma left a comment

Choose a reason for hiding this comment

acs-bot commented Oct 13, 2020

jackfrancis commented Sep 22, 2020 •

edited

Loading

codecov bot commented Sep 22, 2020 •

edited

Loading

jackfrancis commented Sep 25, 2020 •

edited

Loading