New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

metrics-server assumes same TLS config for kube-apiserver and kubelets #25

Open
clhodapp opened this Issue Oct 24, 2017 · 18 comments

Comments

Projects
None yet
9 participants
@clhodapp

clhodapp commented Oct 24, 2017

Presently, metrics-server re-uses the TLS config that it constructs for communication with kube-apiserver in its configuration for talking with the kubelets. This is bad because kube-apiserver and kubelet are supposed to (or at least can) use separate CAs. As it stands, bringing metrics-server into the mix requires you to use the same CA for kube-apiserver and your kubelets.

Problem line:

TLSClientConfig: kubeConfig.TLSClientConfig,

@weiwunb

This comment has been minimized.

weiwunb commented Nov 23, 2017

I guess this configuration will cause the swaggerui can not work correctly.

My k8s cluster was built with kubeadmin, and when I try to visit the swaggerui, I get these errors.

itaas@kvm-013487:~/k8s$ kubectl logs metrics-server-859cb9bd4b-tnhtx -n=kube-system
I1122 06:41:18.721787       1 heapster.go:71] /metrics-server --source=kubernetes.summary_api:''
I1122 06:41:18.721867       1 heapster.go:72] Metrics Server version v0.2.0
I1122 06:41:18.722052       1 configs.go:61] Using Kubernetes client with master "https://10.96.0.1:443" and version 
I1122 06:41:18.722076       1 configs.go:62] Using kubelet port 10255
I1122 06:41:18.723357       1 heapster.go:128] Starting with Metric Sink
I1122 06:41:19.349881       1 serving.go:308] Generated self-signed cert (apiserver.local.config/certificates/apiserver.crt, apiserver.local.config/certificates/apiserver.key)
I1122 06:41:19.661749       1 heapster.go:101] Starting Heapster API server...
[restful] 2017/11/22 06:41:19 log.go:33: [restful/swagger] listing is available at https:///swaggerapi
[restful] 2017/11/22 06:41:19 log.go:33: [restful/swagger] https:///swaggerui/ is mapped to folder /swagger-ui/
I1122 06:41:19.663614       1 serve.go:85] Serving securely on 0.0.0.0:443
I1123 01:27:49.900802       1 logs.go:41] http: TLS handshake error from 192.168.222.192:47540: remote error: tls: bad certificate
I1123 01:27:50.116378       1 logs.go:41] http: TLS handshake error from 192.168.222.192:47542: remote error: tls: bad certificate
I1123 01:27:56.031159       1 logs.go:41] http: TLS handshake error from 192.168.222.192:47556: remote error: tls: bad certificate
@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Nov 27, 2017

I don't think this would cause issues with the swagger UI. Please file a separate bug for that.

@patrickf55places

This comment has been minimized.

patrickf55places commented Jan 4, 2018

The master API server receives the kubelet CA from the --kubelet-certificate-authority CLI option when it boots. AFAIK, the API server does not expose this via an API or ConfigMap.

@clhodapp

This comment has been minimized.

clhodapp commented Jan 5, 2018

I believe one possible option is to augment metrics-server to accept an additional set of arguments to specify the kubelet CA & client credentials.

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Jan 16, 2018

that seems fairly reasonable. PRs are welcome (and/or one of @piosz or I will get to it eventually)

@SpComb

This comment has been minimized.

SpComb commented Mar 22, 2018

The TLSClientConfig also includes the InsecureSkipTLSVerify => Insecure - for the default case of the kubelet using a self-signed cert, you need to disable TLS cert verification for the kubelet API. Currently you can't do that without also disabling verification of the kube API cert.

@aurcioli-handy

This comment has been minimized.

aurcioli-handy commented Apr 26, 2018

Does anyone have a solution to this? How can we get metrics-server to talk to kubelet when kubelet has tls client auth?

@clhodapp

This comment has been minimized.

clhodapp commented Apr 27, 2018

@aurcioli-handy You can do it but you have to use the same CA for both Kubelet and the main API, which feels slightly dirty but should not in and of itself be a security issue if done properly.

@aurcioli-handy

This comment has been minimized.

aurcioli-handy commented Apr 27, 2018

Okay, I was able to get this working with the following setup:

kubelet

ca.pem is same CA as kube-apiserver

  --client-ca-file=/etc/kubernetes/ssl/ca.pem \
  --read-only-port=0

metrics-server

- command:
        - /metrics-server
        - --source=kubernetes.summary_api:https://kubernetes.default.svc?inClusterConfig=false&kubeletHttps=true&kubeletPort=10250&auth=/etc/kubernetes/ssl/kubeconfig&insecure=true

with kubeconfig

apiVersion: v1
kind: Config
clusters:
- name: local
  cluster:
    server: https://kubernetes.default
    insecure-skip-tls-verify: true
users:
- name: kubelet
  user:
    client-certificate: /etc/kubernetes/ssl/kubelet.pem
    client-key: /etc/kubernetes/ssl/kubelet-key.pem
contexts:
- context:
    cluster: local
    user: kubelet
  name: kubelet-context
current-context: kubelet-context

And the rest was just massaging RBAC to get the permissions right.

@igelkotten

This comment has been minimized.

igelkotten commented Jul 31, 2018

@aurcioli-handy

Wouldn't this line in your deployment of metric-server:

Result in it accepting 'insecure' certificates? and that way it doesn't matter what certificates's you put there?

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Aug 1, 2018

We should probably have a new --kubelet-kubeconfig option. I'll try and tackle post-refactor.

@caitong93

This comment has been minimized.

caitong93 commented Dec 3, 2018

@DirectXMan12 @clhodapp

How about add kubelet-client-certificate, --kubelet-client-key, --kubelet-certificate-authority like kube-apiserver does ?
For those who use separate ca for kubelet, this may be a easier approach because they may not have a kubeconfig file containing client certs.

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Dec 4, 2018

that's an option too (although probably just the kubelet-certificate-authority is necessary. If we have the other 2, we should to a kubeconfig, since it's easier to manage and inspect than having to set three separate flags. At any rate, I probably won't have time to work on this any time soon, so it's a good issue for a community member to pick up.

/good-first-issue

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 4, 2018

@DirectXMan12:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

that's an option too (although probably just the kubelet-certificate-authority is necessary. If we have the other 2, we should to a kubeconfig, since it's easier to manage and inspect than having to set three separate flags. At any rate, I probably won't have time to work on this any time soon, so it's a good issue for a community member to pick up.

/good-first-issue

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@caitong93

This comment has been minimized.

caitong93 commented Dec 5, 2018

I would like to try fix this.

If we have the other 2, we should to a kubeconfig, since it's easier to manage and inspect than having to set three separate flags

Do you mean a file in format of kubeconfig for combine things together? If so, it is reasonable, by this way, client certificate and bearer tokens can be both supported.

@DirectXMan12

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Dec 5, 2018

yeah, that was my suggestion -- if you use a file in kubeconfig format, you can just re-use the logic for loading a kubeconfig, etc. On the other hand, I've learned (recently) that some people find this method confusing, so flags are probably fine too. Let's start with a kubelet-certificate-authority flag, since that's the most pressing.

@DirectXMan12

This comment has been minimized.

Contributor

DirectXMan12 commented Dec 5, 2018

/assign @caitong93

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 5, 2018

@DirectXMan12: GitHub didn't allow me to assign the following users: caitong93.

Note that only kubernetes-incubator members and repo collaborators can be assigned.
For more information please see the contributor guide

In response to this:

/assign @caitong93

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment