feat(msi): enable managed service identity for Kubernetes #479

colemickens · 2017-04-09T20:31:36Z

This PR enables the use of "Managed Identity" with Kubernetes. This removes the need for user supplied credentials for Kubernetes to be able to leverage the cloudprovider integration with Azure ARM APIs.

Specifically, this PR:

adds support for kubernetesConfig -> useManagedIdentity
requests a managed implicit identity for the k8s CRP VMs
enables the MSI VM Extension (when useManagedIdentity==true)
make the custom script (final provisioner) depend on it

It works for all configurations, including hybrid clusters with Windows nodes.

Bonuses:

No user supplied creds!
Easier to assign more limited scopes to worker VMs. They only need Reader permissions.
No user supplied creds!
No left over Apps/SPs in the user's AAD tenant (there are probably hundreds or thousands in the MSIT tenant, I just deleted hundreds on my personal test subscription the other day)
No user supplied creds!

Here's the caveats:

MSI is not GA'd (though it is globally deployed)
The deployer must be "Owner", not just "Contributor".
This requires a custom build of Kubernetes. Though the PR is out for 1.8: azure: msi: add managed identity field, logic kubernetes/kubernetes#48854 (The custom build of Kubernetes comes from this branch: https://github.com/colemickens/kubernetes/tree/msi)

Testing is most easily done by looking at the example/test/jenkins related commits and invoking the job via Jenkins, or locally.

This change is

msftclas · 2017-04-09T20:31:40Z

@colemickens,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

colemickens · 2017-04-09T20:32:04Z

@acs-bot test this please

colemickens · 2017-04-09T20:34:21Z

One more caveat: I've not been able to test the Windows side of things yet because of the other blocking issues affecting Kubernetes 1.6 + Windows (+kubenet, it looks like...)

rjtsdl

a few comments.

rjtsdl · 2017-04-11T23:25:43Z

examples/managed-identity/kubernetes.json

+    "orchestratorProfile": {
+      "orchestratorType": "Kubernetes",
+      "kubernetesConfig": {
+        "useManagedIdentity": true


This really makes me feel better.

rjtsdl · 2017-04-11T23:26:17Z

hack/rbacgenerator/function.js

@@ -0,0 +1,18 @@
+{


not sure whether we want to check in the hack/

I think so. Better than it only existing in a random Azure Functions deployment. It doesn't have to be permanent. Once ARM team implements uniqueGUID all of this can go away.

Even if its a hack/temp thing, it better to not name the folder as hack. Adding a doc file to explain the choice might be a better option.

Hm, maybe, sure. I'm open to suggestions. It's not totally unprecedented, kubernetes has a hack/ dir for temp/dev type things.

rjtsdl · 2017-04-11T23:27:09Z

hack/rbacgenerator/raw.js

@@ -0,0 +1,11 @@
+{


No idea why it's called that. It's package.json in reality. Will rename. Thanks.

rjtsdl · 2017-04-11T23:32:26Z

parts/kubernetesmaster-kube-apiserver.yaml

  volumes:
    - name: "etc-kubernetes"
      hostPath:
        path: "/etc/kubernetes"
    - name: "var-lib-kubelet"
      hostPath:
        path: "/var/lib/kubelet"
+    - name: msi
+      hostPath:
+      path: "/var/lib/waagent/ManagedIdentity-Settings"


need indent here

Good catch. What's weird is:

I don't know what apiserver uses cloudprovider for...

I'm surprised everything worked... I would've expected a problem if it tried to actually get a token for something...

Will fix. Looking into why this didn't surface any sort of failure to me...

It's used for SSH key propagation (it's a GKE thing I think). apiserver also parses the config file and passes it to admission controllers so they can use it. (Not sure of any that do).

We don't use SSH key stuff and we don't use any admission controllers that need to talk to ARM so it didn't hit on this.

Still, will fix, of course.

amanohar · 2017-04-13T16:54:07Z

examples/managed-identity/kubernetes.json

+    "orchestratorProfile": {
+      "orchestratorType": "Kubernetes",
+      "kubernetesConfig": {
+        "useManagedIdentity": true


Do you anticipate this to remain a true/false type setting or would there be other kinds of identities that might be available to users in the future?

Very interesting question. In the future, there will be explicit identity support, meaning that users can pre-provision AAD SPs as real resources (ie /Microsoft.Identity/explicitIdentities/k8s-master0).

I think that can be an additive change, so it might look like this in the future:

... { "orchestratorType": "Kubernetes", "kubernetesConfig": { "useManagedIdentity": true, "masterIdentity": "/.../Microsoft.Identity/master-sp", "nodeIdentity": "/.../Microsoft.Identity/node-sp", } } ...

Makes sense. I was thinking something along the lines of the following to make it extensible. This is because true/false type fields become outdated fairly quickly.

"kubernetesConfig": { "identityType": "Managed" }

OR

"kubernetesConfig": { "identity": { "type" : "Managed", "masterIdentity": "/.../Microsoft.Identity/master-sp", "nodeIdentity": "/.../Microsoft.Identity/node-sp", } }

Let me think on it and chat with you more about it. I'd be fine with either of those approaches and don't have any strong opinions here.

@colemickens @amanohar how do we want to proceed on this PR ?

@shrutir25 I think it's going to stay here a while longer. There's no need to rush it and I have a better solution than the hack/ stuff with the Azure function coming in ~3 weeks via ARM templates.

weinong · 2017-06-13T22:54:11Z

Based on my experience using MSI, I think we need to also automate adding MSI to the KeyVault. With cross resource group template deployment, we should include KV access policy in data model.
Also, I'd like to have master and agents accessing different Keyvault. So it seems more sensible to me to put MSI options under Master and AgentPoolProfiles.
Thoughts?

colemickens · 2017-07-13T06:46:51Z

Sorry @weinong I don't quite follow. This is for the cloudprovider integration in Kubernetes, not for getting assets from MSI necessarily (although it could do both). This change doesn't require that the identity be shared or any such thing (I don't think we can handle those more advanced scenarios well until explicit identities come in).

It would be cool to start having ACS-Engine stash the PKI assets, etc into KeyVault and retrieving them, but that's not in scope for this PR. This just does the SP + RBACing so that K8s can call ARM APIs.

colemickens · 2017-07-13T11:11:53Z

This is ready to review and merge hopefully. PR text has also been re-written to be more up-to-date and accurate.

cc: @dmitsh for reviewing the test config changes
cc: @weinong for reviewing the MSI change in general
cc: @seanknox for release/project management side of things - hopefully this one can be allowed... it's a new feature in isolation and pre-dates our new precedent for having an open issue.

I've tested this manually. I have a Jenkins job that has been running it.

From a cluster deployed via Jenkins, we see /etc/kubernetes/azure.json:

{ 
   "cloud":"AzurePublicCloud", 
   "tenantId": "72f988bf-86f1-41af-91ab-2d7cd011db47", 
   "subscriptionId": "8ecadfc9-d1a3-4ea4-b844-0d9f87e4d7c8", 
   "aadClientId": "msi", 
   "aadClientSecret": "msi", 
   "resourceGroup": "x-managed-identity-kubernetes-msi-westus-27-2-0", 
   "location": "westus", 
   "subnetName": "k8s-subnet", 
   "securityGroupName": "k8s-master-25273340-nsg", 
   "vnetName": "k8s-vnet-25273340", 
   "routeTableName": "k8s-master-25273340-routetable", 
   "primaryAvailabilitySetName": "linuxpool1-availabilitySet-25273340", 
   "cloudProviderBackoff": false, 
   "cloudProviderBackoffRetries": 6, 
   "cloudProviderBackoffExponent": 1.5, 
   "cloudProviderBackoffDuration": 5, 
   "cloudProviderBackoffJitter": 1, 
   "cloudProviderRatelimit": false, 
   "cloudProviderRateLimitQPS": 1, 
   "cloudProviderRateLimitBucket": 5, 
   "useManagedIdentityExtension": true 
}

LOOK MA! No scary credentials!

And in sudo journalctl -u kubelet | grep azure | grep "access token":

azure.go:159] azure: using managed identity extension to retrieve access token

And in kubectl get nodes:

2017-07-13 10:42:47.364600 I | proto: duplicate proto type registered: google.protobuf.Any 
2017-07-13 10:42:47.364650 I | proto: duplicate proto type registered: google.protobuf.Duration 
2017-07-13 10:42:47.364679 I | proto: duplicate proto type registered: google.protobuf.Timestamp 
NAME                        STATUS                     AGE       VERSION 
25273acs9010                Ready                      2m        v1.6.4-beta.0.147+2b3fc68910b4dd 
25273acs9011                Ready                      2m        v1.6.4-beta.0.147+2b3fc68910b4dd 
k8s-linuxpool1-25273340-0   Ready                      2m        v1.8.0-alpha.2.85+531664519485fd-dirty 
k8s-linuxpool1-25273340-1   Ready                      1m        v1.8.0-alpha.2.85+531664519485fd-dirty 
k8s-master-25273340-0       Ready,SchedulingDisabled   3m        v1.8.0-alpha.2.85+531664519485fd-dirty

colemickens · 2017-07-13T11:15:27Z

There is one small quirk that make this less than 100% ideal and would keep us from using MSI immediately:

An MSI-powered cluster is mutually exclusive with automatic ACR integration. As far as I can tell, ACR doesn't offer a way to authenticate via access tokens, only static credentials, so it's sort of fundamentally incompatible with MSI for the time being. As a result, it's also hard to test Windows given that it doesn't use the cloudprovider integration and we can't test with ACR. But I've made the necessary changes such that it should work.

That having been said, I'd strongly prefer if we can review and merge this. I've spent a pretty serious amount of time rebasing this over the past 6.5 months. It'd be great to get it in, get a soak test going, etc.

colemickens · 2017-07-13T12:00:49Z

Note: The PR is only failing because it runs in a different sub with a different SP than I was testing with. And I don't have access to the subscription, so I can't do anything about this. Hopefully @dmitsh or someone else with access to that subscription can either add me or the relevant SP Application ID to unblock this. In the meantime, here's a build with the same git hash that is live now (<jenkins_url>/job/colemick-jobs/job/colemick-acs-engine-msi2/29/)

rjtsdl · 2017-07-13T17:23:42Z

examples/managed-identity/kubernetes-msi.json.env

+CLUSTER_SERVICE_PRINCIPAL_CLIENT_ID=msi
+CLUSTER_SERVICE_PRINCIPAL_CLIENT_SECRET=msi
+CUSTOM_HYPERKUBE_SPEC=docker.io/colemickens/hyperkube-amd64:531664519485fdbb543e1e83c90bac6fef829d73
+EXPECTED_NODE_COUNT=5


nit: add a newline.

sure, I'll add a couple

rjtsdl · 2017-07-13T17:27:23Z

examples/managed-identity/kubernetes-msi.json

+    },
+    "servicePrincipalProfile": {
+      "servicePrincipalClientID": "",
+      "servicePrincipalClientSecret": ""


nit: this could be removed IMO.

And also, what this file is used for? The hyperkube here is up to date?

This is an example apimodel that activates MSI. I don't want to remove the SPP yet. It will require refactoring the validation code to be aware of what mode it's running in, plus this section will likely still be used for either ACR scenario or for AAD-kubectl scenarios.

colemickens · 2017-07-13T18:02:21Z

@acs-bot test this please

weinong · 2017-07-13T21:47:26Z

For scaling down, do we need to remove principalIDs from role assignment?

Reviewed 1 of 32 files at r2, 16 of 51 files at r3, 4 of 4 files at r4, 4 of 4 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

examples/managed-identity/kubernetes-msi.json, line 46 at r6 (raw file):

    "servicePrincipalProfile": {
      "servicePrincipalClientID": "",
      "servicePrincipalClientSecret": ""

do we still need spn secret with msi enabled?

parts/kubernetesagentresourcesvmas.t, line 133 at r6 (raw file):

      "location": "[variables('location')]",
      "name": "[concat(variables('{{.Name}}VMNamePrefix'), copyIndex(variables('{{.Name}}Offset')))]",
      {{if UseManagedIdentity}}

should we enable msi on agent pool separately?

Comments from Reviewable

seanknox

LGTM. Wil let another dev approve.

seanknox · 2017-07-13T22:10:49Z

parts/kubernetesagentresourcesvmas.t

+         "name": "vmLoopNode"
+       },
+      "name": "[guid(concat('Microsoft.Compute/virtualMachines/', variables('{{.Name}}VMNamePrefix'), copyIndex(), 'vmidentity'))]",
+      "type": "Microsoft.Authorization/roleAssignments",


Probably stupid question, excuse me in advance. Is it at all possible to extract this into another file? Looking for opportunities to make all the parts (literally and figuratively) a bit more visible.

Dunno why this showed in Reviewable for me to comment on but didn't post back here. Anyway, what I wrote:

I'm not sure actually. I don't know if go tmpl has a generic include function or if we use it anywhere. I'd selfishly say I'd prefer to punt on it given my lack of immediate knowledge.

weinong · 2017-07-13T23:40:18Z

thanks for clarifying offlien. LGTM

colemickens · 2017-07-13T23:52:56Z

Review status: 23 of 24 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

examples/managed-identity/kubernetes-msi.json, line 46 at r6 (raw file):

Previously, weinong (Weinong Wang) wrote…

do we still need spn secret with msi enabled?

It becomes optional. For now, it happens to still be usable in that the ACR integration will keep working, even in some scenario where user activates MSI and still supplies creds.

I've chosen not to omit it because of the refactoring changes required that I mentioned in reply to Jingtao.

examples/managed-identity/kubernetes-msi.json, line 46 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

This is an example apimodel that activates MSI. I don't want to remove the SPP yet. It will require refactoring the validation code to be aware of what mode it's running in, plus this section will likely still be used for either ACR scenario or for AAD-kubectl scenarios.

Done.

examples/managed-identity/kubernetes-msi.json.env, line 4 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

sure, I'll add a couple

Done.

parts/kubernetesagentresourcesvmas.t, line 133 at r6 (raw file):

Previously, weinong (Weinong Wang) wrote…

should we enable msi on agent pool separately?

IMO, no. The ServicePrincipalProfile applies uniformly today, this seems consistent.

parts/kubernetesagentresourcesvmas.t, line 205 at r6 (raw file):

Previously, seanknox (Sean Knox) wrote…

Probably stupid question, excuse me in advance. Is it at all possible to extract this into another file? Looking for opportunities to make all the parts (literally and figuratively) a bit more visible.

I'm not sure actually. I don't know if go tmpl has a generic include function or if we use it anywhere. I'd selfishly say I'd prefer to punt on it given my lack of immediate knowledge.

parts/kubernetesmaster-kube-apiserver.yaml, line 51 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

It's used for SSH key propagation (it's a GKE thing I think). apiserver also parses the config file and passes it to admission controllers so they can use it. (Not sure of any that do).

We don't use SSH key stuff and we don't use any admission controllers that need to talk to ARM so it didn't hit on this.

Still, will fix, of course.

Done.

examples/managed-identity/kubernetes.json, line 7 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

@shrutir25 I think it's going to stay here a while longer. There's no need to rush it and I have a better solution than the hack/ stuff with the Azure function coming in ~3 weeks via ARM templates.

Done.

hack/rbacgenerator/function.js, line 1 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Hm, maybe, sure. I'm open to suggestions. It's not totally unprecedented, kubernetes has a hack/ dir for temp/dev type things.

This no longer applies since I've removed hack/ and everything in lieu of guid().

hack/rbacgenerator/raw.js, line 1 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

No idea why it's called that. It's package.json in reality. Will rename. Thanks.

No longer applies. It's all been removed.

Comments from Reviewable

colemickens · 2017-07-14T00:20:39Z

Review status: 14 of 24 files reviewed at latest revision, 9 unresolved discussions.

examples/managed-identity/kubernetes-msi.json, line 46 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

It becomes optional. For now, it happens to still be usable in that the ACR integration will keep working, even in some scenario where user activates MSI and still supplies creds.

I've chosen not to omit it because of the refactoring changes required that I mentioned in reply to Jingtao.

Done.

examples/managed-identity/kubernetes-msi.json, line 46 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Done.

Done.

examples/managed-identity/kubernetes-msi.json.env, line 4 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Done.

Done.

parts/kubernetesagentresourcesvmas.t, line 133 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

IMO, no. The ServicePrincipalProfile applies uniformly today, this seems consistent.

Done.

parts/kubernetesagentresourcesvmas.t, line 205 at r6 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Dunno why this showed in Reviewable for me to comment on but didn't post back here. Anyway, what I wrote:

I'm not sure actually. I don't know if go tmpl has a generic include function or if we use it anywhere. I'd selfishly say I'd prefer to punt on it given my lack of immediate knowledge.

Done.

parts/kubernetesmaster-kube-apiserver.yaml, line 51 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Done.

Done.

examples/managed-identity/kubernetes.json, line 7 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

Done.

Done.

hack/rbacgenerator/function.js, line 1 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

This no longer applies since I've removed hack/ and everything in lieu of guid().

Done.

hack/rbacgenerator/raw.js, line 1 at r2 (raw file):

Previously, colemickens (Cole Mickens) wrote…

No longer applies. It's all been removed.

Done.

Comments from Reviewable

seanknox · 2017-07-14T01:00:48Z

omerlh · 2017-12-20T06:19:10Z

Any documentation for that? Also, is there support for containers authorization? e.g. granting access only to specific container to a given KeyVault?

msftclas added the cla-not-required label Apr 9, 2017

rjtsdl reviewed Apr 12, 2017

View reviewed changes

amanohar reviewed Apr 13, 2017

View reviewed changes

anhowe mentioned this pull request May 1, 2017

Feature request: Better API authentication mechanism #159

Closed

colemickens closed this May 10, 2017

colemickens deleted the msi-rebase2 branch May 10, 2017 21:50

colemickens restored the msi-rebase2 branch May 10, 2017 21:51

colemickens reopened this May 10, 2017

msftclas added the cla-not-required label May 10, 2017

colemickens mentioned this pull request May 18, 2017

Clusters without public IPs #221

Closed

colemickens force-pushed the msi-rebase2 branch 4 times, most recently from 4a8a2d7 to c52e49e Compare July 13, 2017 06:54

colemickens changed the title ~~k8s: managed service identity~~ feat(msi): enable managed service identity for Kubernetes Jul 13, 2017

colemickens force-pushed the msi-rebase2 branch 6 times, most recently from 73b2d6f to ffd3f9f Compare July 13, 2017 09:51

colemickens force-pushed the msi-rebase2 branch from ffd3f9f to d9cc8ab Compare July 13, 2017 11:16

rjtsdl requested a review from weinong July 13, 2017 17:20

rjtsdl reviewed Jul 13, 2017

View reviewed changes

seanknox reviewed Jul 13, 2017

View reviewed changes

weinong approved these changes Jul 13, 2017

View reviewed changes

colemickens force-pushed the msi-rebase2 branch from d9cc8ab to 91a9ba5 Compare July 13, 2017 23:41

colemickens added 11 commits July 13, 2017 17:03

feat(msi): add useManagedIdentity to apimodel

60c79eb

feat(msi): add global tmpl fn for checking msi status

d86269b

chore(engine): test case should be consistent, helpful

cc9cc76

feat(msi): mount MSI settings into req'd containers

81ba421

feat(msi): template changes: init + masters

03cbdbf

feat(msi): template changes: windows agents

da46e4d

feat(msi): template changes: linux agents

e5a453f

feat(msi): enable test to source env file earlier

24b6a17

feat(msi): add example apimodel + jenkins config

7488013

feat(msi): test MSI regression as part of PR job

13ecec7

chore(test infra): fix CUSTOM_HYPERKUBE_SPEC handling

7862291

colemickens force-pushed the msi-rebase2 branch from 91a9ba5 to 7862291 Compare July 14, 2017 00:10

seanknox approved these changes Jul 14, 2017

View reviewed changes

colemickens merged commit 2d54601 into Azure:master Jul 14, 2017

cehoffman mentioned this pull request May 26, 2018

Azure cloud provider configuration and upstream direction of external cloud configuration coreos/tectonic-installer#3246

Open

feat(msi): enable managed service identity for Kubernetes #479

feat(msi): enable managed service identity for Kubernetes #479

Conversation

colemickens commented Apr 9, 2017 • edited Loading

msftclas commented Apr 9, 2017

colemickens commented Apr 9, 2017

colemickens commented Apr 9, 2017

rjtsdl left a 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

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

amanohar Apr 13, 2017 • 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

weinong commented Jun 13, 2017

colemickens commented Jul 13, 2017

colemickens commented Jul 13, 2017

colemickens commented Jul 13, 2017

colemickens commented Jul 13, 2017

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

colemickens commented Jul 13, 2017

weinong commented Jul 13, 2017

seanknox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weinong commented Jul 13, 2017

colemickens commented Jul 13, 2017

colemickens commented Jul 14, 2017

seanknox commented Jul 14, 2017

omerlh commented Dec 20, 2017

colemickens commented Apr 9, 2017 •

edited

Loading

amanohar Apr 13, 2017 •

edited

Loading