pkg/asset: use service accounts for checkpointer and kube-proxy creds #767

ericchiang · 2017-11-09T20:43:19Z

Today, both the checkpointer and kube-proxy re-use the kubelet's credentials to talk to the API server. This means we can't target them with specific RBAC profiles or identify them in audit logs. In addition, when we move to TLS bootstrapping (#663), the kubelet's kubeconfig will no longer hold real credentials.

Switch the checkpointer and kube-proxy to use service account credentials instead of the kubelet's kubeconfig.

Extracted from #663 to make that PR smaller.

cc @dghubble @rphillips

aaronlevy · 2017-11-09T21:20:09Z

Can you add a description of why this is needed?

ericchiang · 2017-11-09T22:45:57Z

@aaronlevy added a description.

dghubble · 2017-11-09T22:47:11Z

We don't have any hard rules around this, but I'd aim to put this in a v0.9.0 followup to v0.8.2 since there is a new render asset.

dghubble · 2017-11-09T23:02:38Z

Can we have kube-proxy and checkpointer each use their own named service account instead of a service-account-kubeconfig.yaml?

ericchiang · 2017-11-10T00:22:13Z

@dghubble updated with RBAC roles.

ericchiang · 2017-11-10T00:31:37Z

pkg/asset/internal/templates.go

+`)
+
+var CheckpointerRole = []byte(`apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole


Note: if the pod-checkpointer only watches kube-system (e.g. its own namespace) we can drop this to a Role. It doesn't currently.

I think this is ok to keep for now but add to the checkpointer README and the release note.

dghubble

Mainly just questions

dghubble · 2017-11-14T18:24:05Z

pkg/asset/internal/templates.go

+
+// ServiceAccountKubeConfig is used for pods that don't have access to the
+// KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT environment variables.
+// This is used for kube-proxy, since it configures these endpoints itself,


Its also being used for the pod-checkpointer here. Whats the reason there?

dghubble · 2017-11-14T18:24:33Z

pkg/asset/internal/templates.go

+  namespace: kube-system
+`)
+
+// ServiceAccountKubeConfig is used for pods that don't have access to the


Can you clarify "used for"? This still feels odd and kinda opaque. So before we relied on the host's kubeconfig which contained both endpoint info and credentials. With the service account approach you use an alternate kubeconfig which specifies the endpoint info and the local service account in the pod.

What's the tie in to these environment variables?

KUBERNETES_SERVICE_PORT=443 KUBERNETES_PORT=tcp://10.3.0.1:443

Clients compiled with client-go require the KUBERNETES_SERVICE_PORT and KUBERNETES_PORT to use the in-cluster config.

https://github.com/kubernetes/client-go/blob/kubernetes-1.8.2/rest/config.go#L318-L321

These aren't exposed to kube-proxy (since kube proxy sets up the endpoint) or the checkpointer, since it runs as a static pod.

This work:

// ServiceAccountKubeConfig instructs clients to use their service account token, but unlike an in-cluster client // doesn't rely on the `KUBERNETES_SERVICE_PORT` and `KUBERNETES_PORT` to determine the API // servers address. This kubeconfig is used bootstrapping pods that might not have access to these env vars, // such as kube-proxy, which sets up the endpoint (chicken and egg), and the checkpointer, which needs to run // as a static pod even if the API server isn't available.

dghubble · 2017-11-14T18:26:05Z

pkg/asset/asset.go

@@ -37,6 +37,9 @@ const (
 	AssetPathManifests                   = "manifests"
 	AssetPathKubelet                     = "manifests/kubelet.yaml"
 	AssetPathProxy                       = "manifests/kube-proxy.yaml"
+	AssetPathProxySA                     = "manifests/kube-proxy-sa.yaml"
+	AssetPathProxyRoleBinding            = "manifests/kube-proxy-role-binding.yaml"
+	AssetPathServiceAccountKubeConfig    = "manifests/service-account-kubeconfig.yaml"


nit: everywhere else, we say the name followed by the type. KubeconfigServiceAccount and kubeconfig-sa.yaml?

See below, got confused. Its not a service account.

dghubble · 2017-11-14T18:26:37Z

pkg/asset/internal/templates.go

+var ServiceAccountKubeConfigTemplate = []byte(`apiVersion: v1
+kind: Secret
+metadata:
+  name: service-account-kubeconfig


nit: kubeconfig-service-account or just kubeconfig

dghubble · 2017-11-14T18:28:12Z

pkg/asset/internal/templates.go

+metadata:
+  name: service-account-kubeconfig
+  namespace: kube-system
+stringData:


Cool, I missed this getting into upstream.

dghubble · 2017-11-14T18:34:53Z

pkg/asset/internal/templates.go

-      - name: etc-kubernetes
-        hostPath:
-          path: /etc/kubernetes
+        name: ssl-certs-host


accidental duplicate from a few lines above?

yes, thanks for catching.

ericchiang · 2017-11-14T19:15:08Z

@dghubble thanks for the review. Comments addressed.

dghubble · 2017-11-14T19:45:45Z

pkg/asset/asset.go

@@ -37,6 +37,9 @@ const (
 	AssetPathManifests                   = "manifests"
 	AssetPathKubelet                     = "manifests/kubelet.yaml"
 	AssetPathProxy                       = "manifests/kube-proxy.yaml"
+	AssetPathProxySA                     = "manifests/kube-proxy-sa.yaml"
+	AssetPathProxyRoleBinding            = "manifests/kube-proxy-role-binding.yaml"
+	AssetPathServiceAccountKubeConfig    = "manifests/kubeconfig-sa.yaml"


Sorry, my nit doesn't help much. Its still confusing to read since this is actually a secret. eh, better idea?

in-cluster-kubeconfig? which happens to be a secret or configmap?

Does manifest/kubeconfig-in-cluster.yaml work?

ericchiang · 2017-11-14T20:43:51Z

e2e passed but it looks like there's something wrong with the Jenkins GitHub plugin https://gist.github.com/ericchiang/1b80ef7c23ed4adf27527e728e04e2d3

dghubble · 2017-11-14T21:57:29Z

Also, FAIL: TestCheckpointerUnscheduleCheckpointer. Can you re-run? Possibly with the renamed in-cluster-kubeconfig

ericchiang · 2017-11-15T00:18:51Z

/assign @diegs

For final merge.

diegs

LGTM

Nice work! Just a few small comments but I'm good to merge.

diegs · 2017-11-15T00:21:01Z

pkg/asset/internal/templates.go

+`)
+
+var CheckpointerRole = []byte(`apiVersion: rbac.authorization.k8s.io/v1
+kind: ClusterRole


I think this is ok to keep for now but add to the checkpointer README and the release note.

diegs · 2017-11-15T00:22:15Z

pkg/asset/k8s.go

@@ -40,6 +41,9 @@ func newStaticAssets(imageVersions ImageVersions) Assets {
 		MustCreateAssetFromTemplate(AssetPathControllerManagerDisruption, internal.ControllerManagerDisruptionTemplate, conf),
 		MustCreateAssetFromTemplate(AssetPathKubeDNSDeployment, internal.DNSDeploymentTemplate, conf),
 		MustCreateAssetFromTemplate(AssetPathCheckpointer, internal.CheckpointerTemplate, conf),
+		Asset{AssetPathCheckpointerSA, internal.CheckpointerServiceAccount},


I think it might be more uniform (and less surprising in the future) to pass these through MustCreateAssetFromTemplate even if they currently do not do any templating.

ericchiang · 2017-11-15T01:34:32Z

coreosbot run e2e

ericchiang · 2017-11-15T17:20:31Z

Last couple nits addressed.

diegs

Awesome!

aaronlevy · 2017-11-15T19:27:59Z

pkg/asset/internal/templates.go

+`)
+
+// TODO: Drop checkpointer RBAC resources to a Role and RoleBinding if
+// the checkpoint switches to only watching kube-system.


Should we just make this change? It only needs to watch kube-system (also can be follow-up PR)

I can open a separate PR. We'd have to wait the checkpointer change to merged before we drop this to a role, so we can't do it in one PR anyway.

Why would the checkpointer have to be changed? It just wouldn't have access to non-kube-system right?

Checkpointer attempts to watch all namespaces. If we restrict it now that request will fail. We have to submit a change to the checkpointer.

https://github.com/kubernetes-incubator/bootkube/blob/v0.8.2/pkg/checkpoint/apiserver.go#L18

Opened the checkpointer PR #774

* Create separate service accounts for kube-proxy and pod-checkpointer * Switch kube-proxy and pod-checkpointer to use a kubeconfig that references the local service account, rather than the host kubeconfig * kubernetes-retired/bootkube#767

redbaron · 2017-12-05T17:34:50Z

pkg/asset/internal/templates.go

+// (chicken and egg), and the checkpointer, which needs to run as a static pod
+// even if the API server isn't available.
+var KubeConfigInClusterTemplate = []byte(`apiVersion: v1
+kind: Secret


Does it need to be a secret? There is no sensitive data in this resource, would you consider merging PR which changes it to ConfigMap?

redbaron · 2017-12-05T17:38:31Z

pkg/asset/internal/templates.go

+    - name: local
+      cluster:
+        server: {{ .Server }}
+        certificate-authority-data: {{ .CACert }}


Another options is to use certificate-authority: /run/secrets/kubernetes.io/serviceaccount/ca.crt which makes it somewhat easier to rotate keys in the future (assuming that resources installed by bootkube are forming a basis for core cluster resources, which are then managed separately)

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 9, 2017

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 10, 2017

ericchiang commented Nov 10, 2017

View reviewed changes

dghubble reviewed Nov 14, 2017

View reviewed changes

dghubble approved these changes Nov 15, 2017

View reviewed changes

k8s-ci-robot assigned diegs Nov 15, 2017

diegs approved these changes Nov 15, 2017

View reviewed changes

pkg/asset: use service accounts for checkpointer and kube-proxy creds

1fc0d7c

diegs approved these changes Nov 15, 2017

View reviewed changes

aaronlevy reviewed Nov 15, 2017

View reviewed changes

This was referenced Nov 15, 2017

Add TLS auto-approval for kubelet. #663

Merged

checkpoint: only watch pods in kube-system #774

Merged

dghubble merged commit e851d3e into kubernetes-retired:master Nov 15, 2017

ericchiang deleted the authn-checkpointer-kube-proxy-sa branch November 15, 2017 23:41

dghubble mentioned this pull request Nov 20, 2017

Remove --deprecated-self-hosted-kubelet option #760

Merged

ericchiang mentioned this pull request Nov 22, 2017

Remove need for kube-proxy kubeconfig host-mount #301

Closed

redbaron reviewed Dec 5, 2017

View reviewed changes

abhinavdahiya mentioned this pull request Jan 23, 2018

kube-core: add global cluster config options coreos/tectonic-config#2

Merged

mrbobbytables unassigned diegs Nov 7, 2019

pkg/asset: use service accounts for checkpointer and kube-proxy creds #767

pkg/asset: use service accounts for checkpointer and kube-proxy creds #767

Conversation

ericchiang commented Nov 9, 2017 • edited Loading

aaronlevy commented Nov 9, 2017

ericchiang commented Nov 9, 2017

dghubble commented Nov 9, 2017

dghubble commented Nov 9, 2017 • edited Loading

ericchiang commented Nov 10, 2017

ericchiang Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble 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

ericchiang commented Nov 14, 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

ericchiang commented Nov 14, 2017

dghubble commented Nov 14, 2017

ericchiang commented Nov 15, 2017

diegs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Nov 15, 2017

ericchiang commented Nov 15, 2017

diegs 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

ericchiang Nov 15, 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

ericchiang commented Nov 9, 2017 •

edited

Loading

dghubble commented Nov 9, 2017 •

edited

Loading

ericchiang Nov 10, 2017 •

edited

Loading

ericchiang Nov 15, 2017 •

edited

Loading