Added deployment files #4

piosz · 2017-05-31T05:26:31Z

deads2k · 2017-05-31T13:12:31Z

deploy/metrics-server-deployment.yaml

+kind: Deployment
+metadata:
+  name: metrics-server
+  namespace: kube-system


This shouldn't go into kube-system. We need to limit the number of pods created in this namespace since pods mount SA tokens and the SA tokens in kube-system are very powerful. You should create your own namespace, probably kube-metrics to contain this data.

This is a replacement for Heapster which runs in kube-system and this will run of the the box in every Kubernetes cluster so I think kube-system is an appropriate namespace here.

@piosz I think the point is -- "heapster runs in kube-system which is suboptimal. Let's change that when we do this disruptive change anyway"

@piosz I think the point is -- "heapster runs in kube-system which is suboptimal. Let's change that when we do this disruptive change anyway"

Correct. Separating components into different namespaces helps us reason about which areas have access to particular escalation powers. kube-system is a super-powered namespace that heapster doesn't need access to. The final goal is zero pods in kube-system.

so... as an operator, I kind of like having all the k8s services in one namespace. These comments seem to conflate the default service account in kube-system with the namespace itself? Can't we just make a service account for metrics-server and use that in the kube-system namespace instead?

Changing to the other namespace is problematic:

addon manager only consider pods running in kube-system

rescheduler the same

I strongly believe that there are some other parts of the code (maybe in Kubernetes, definitely outside) that relies on the fact that system components run in kube-system

I'm not against but I don't want to handle all that stuff as a part of metrics-server effort.

deads2k · 2017-05-31T13:15:13Z

deploy/metrics-server-deployment.yaml

+        command:
+        - /metrics-server
+        - --source=kubernetes.summary_api:''
+        - --requestheader-client-ca-file=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt


You shouldn't use this as your request header CA file. The request header CA is used to identify your front proxy, but the CA from an SA token is used to identify the kube-apiserver. You don't want to accidentally allow certs signed by this CA to be used as valid proxies.

The command ought to auto-discover this from the configmap in kube-system. Otherwise, package and pass the correct CA.

@deads2k do you have a diagram anywhere of how all the different servers fit together along with the CA and certificate files?

@deads2k do you have a diagram anywhere of how all the different servers fit together along with the CA and certificate files?

I don't think we ever made one. All these flags are "normal" flags that operate the same way they're used for the kube-apiserver

I do think it would still be useful, as I don't have a good understanding of what goes where yet.

I have a document I submitted that should answer this question. It's in the apiserver-builder repo: https://github.com/kubernetes-incubator/apiserver-builder/blob/master/docs/concepts/auth.md

Again my fault. Until kubernetes/kubernetes#43716 is fixed this is a work-around/hack to allow development to proceed. Once its fixed we will no longer need the work-around.

kubeadm that already sets the proxy flags don't need this, right?

@deads2k I thought it's not required but without that apiserver complained about this.
@cheftako since kubernetes/kubernetes#43716 is fixed, am I able to remove this flag now?

deads2k · 2017-05-31T13:15:51Z

deploy/metrics-server-deployment.yaml

+        - --requestheader-client-ca-file=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt
+        volumeMounts:
+        - name: ssl-certs
+          mountPath: /etc/ssl/certs


I don't recognize this path. What is using the values from here, what is contained, and what are they used for? I'd suggest using the "normal" parameters whenever possible to ease people reading them.

I think it's for ca certificates. That could probably be copied into the container image instead...

TBH I don't fully understand why we need this here and in a number of other places, but this is a kind of standard in a number of deployments. Should this run without it?

@piosz Test removing it. If it starts failing (which I don't think it will), add it back. Otherwise, remove it, as it adds an unnecessary dependency on the host.

IIUC, the metrics server won't ever hit public domains like google.com, will it?
That's why CA certificates would be required, to verify the authenticity of normal sites...

deads2k · 2017-05-31T13:16:24Z

deploy/user-role.yaml

+kind: ClusterRole
+metadata:
+  labels:
+    kubernetes.io/bootstrapping: rbac-defaults


You shouldn't apply this annotation yourself. It's for bootstrap roles and this is an add-on

removed file as it's useless

deads2k · 2017-05-31T13:18:12Z

deploy/auth-user.yaml

+  name: metrics-server
+subjects:
+- kind: User
+  name: system:anonymous


This looks off. You're granting anonymous the ability to read metrics which implies the ability to learn node and pod names? Why?

This is partly my fault, Piotr borrowed some of my test setup. I had done this locally to make it easy for me to test. I think we should either determine the correct users allowed to read metrics or describe to the customer how to set up read permissions for their users.

Should this be bound to the SA of the metrics-server instead?

I'd like to have access to metrics through apiserver. Without this I wasn't able to do it.
@deads2k @cheftako what should I do instead here?

@piosz @deads2k What about system:authenticated instead?

I'd like to have access to metrics through apiserver. Without this I wasn't able to do it.
@deads2k @cheftako what should I do instead here?

I suspect that means that your proxy configuration is messed up somehow. Try turning on the audit log in your metrics server and see if all requests appear as unauthenticated. Allowing unrestricted access to metrics isn't suitable.

removed file as it's useless

deads2k · 2017-05-31T13:18:17Z

deploy/auth-user.yaml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1beta1


filename looks weird.

removed file as it's useless

cheftako · 2017-05-31T20:40:29Z

deploy/auth-delegator.yaml

@@ -0,0 +1,13 @@
+apiVersion: rbac.authorization.k8s.io/v1alpha1


If you want this to work on GKE I believe you need to use at least the v1beta1 version of core resources such as rbac.

changed to beta

cheftako · 2017-05-31T20:41:42Z

deploy/auth-reader.yaml

@@ -0,0 +1,14 @@
+apiVersion: rbac.authorization.k8s.io/v1alpha1


changed to beta as well

cheftako · 2017-05-31T20:45:16Z

deploy/auth-user.yaml

+  name: metrics-server
+subjects:
+- kind: User
+  name: system:anonymous


This is partly my fault, Piotr borrowed some of my test setup. I had done this locally to make it easy for me to test. I think we should either determine the correct users allowed to read metrics or describe to the customer how to set up read permissions for their users.

luxas

I'd love to try this out soon...

luxas · 2017-06-08T18:15:49Z

deploy/metrics-server-deployment.yaml

+        - --requestheader-client-ca-file=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt
+        volumeMounts:
+        - name: ssl-certs
+          mountPath: /etc/ssl/certs


I think it's for ca certificates. That could probably be copied into the container image instead...

luxas · 2017-06-08T18:16:14Z

deploy/metrics-server-service.yaml

+  name: metrics-server
+  namespace: kube-system
+  labels:
+    kubernetes.io/name: "Metrics-server"


Metrics-Server or metrics-server?

The other services start with capital letter:
https://github.com/kubernetes/kubernetes/tree/master/cluster/addons/cluster-monitoring/influxdb

luxas · 2017-06-08T18:16:56Z

deploy/metrics-apiservice.yaml

+  name: v1alpha1.metrics
+spec:
+  insecureSkipTLSVerify: true
+  group: metrics


why not metrics.k8s.io or something like that?

Legacy reasons, mostly. It's already just "metrics" in the code. We'd probably eventually want to move to "metrics.k8s.io" when we move to beta.

Yeah, I followed what is in metrics repos. I'm not convinced that we want to change this though. Do we plan to change autoscaling to autoscaling.k8s.io? I think metrics api group is a first class api group in Kubernetes (whatever it means) similarly to apps or autoscaling. Only due to performance reasons it's implemented through another apiserver.

"first class" vs non-"first class" is not precisely the issue here (also, IIRC, some of the built-in versions are getting fully-qualified as well). We should keep metrics as the group name for backwards compat, but the eventual trend is to move to qualified group names, AFAIK.

We should eventually move to resource-metrics.metrics.k8s.io or something of the sort (as opposed to custom-metrics.metrics.k8s.io, which is the CM group), but probably not until we move to beta, or something like that.

We should keep metrics as the group name for backwards compat, but the eventual trend is to move to qualified group names, AFAIK

Why not do that now, while you're in alpha and can change things? I don't understand

Why not do that now, while you're in alpha and can change things? I don't understand

Seconded.

Let's change this as a part of moving api to beta (outside of the scope of this PR)

luxas · 2017-06-08T18:18:07Z

deploy/metrics-server-deployment.yaml

+        command:
+        - /metrics-server
+        - --source=kubernetes.summary_api:''
+        - --requestheader-client-ca-file=/var/run/secrets/kubernetes.io/serviceaccount/ca.crt


kubeadm that already sets the proxy flags don't need this, right?

luxas · 2017-06-08T18:20:37Z

deploy/auth-user.yaml

+  name: metrics-server
+subjects:
+- kind: User
+  name: system:anonymous


Should this be bound to the SA of the metrics-server instead?

piosz · 2017-08-23T12:58:32Z

@cheftako @deads2k @DirectXMan12 @kfox1111 @luxas @ncdc
Thanks for your feedback! I applied the comments so PTAL.

Apologies for the delay - it's been evicted by higher priority work.

I'm not able to release v0.1.0 because I want to bump deps and some repos are not synced. I'll do it once the problem is fixed. cc @sttts
Meanwhile you can try with dev version (it's from my local HEAD).

luxas · 2017-08-23T19:02:08Z

/assign @luxas
(not sure if that will work)

luxas · 2017-08-23T19:02:26Z

I'll take a look later

deads2k · 2017-08-24T12:39:13Z

deploy/auth-delegator.yaml

@@ -0,0 +1,12 @@
+apiVersion: rbac.authorization.k8s.io/v1beta1


This has moved to v1: kubernetes/kubernetes#49642

luxas

Didn't see anything particularly suspicious.
Depending on what version you're targeting, use these API groups:
v1.7 & v1.8:

rbac.authorization.k8s.io/v1beta1
extensions/v1beta1

v1.8+:

rbac.authorization.k8s.io/v1
apps/v1beta2

piosz · 2017-08-29T09:11:26Z

@deads2k @luxas let's stay with v1beta1 here, because people may want to test it with 1.7 as well. Once 1.8 is released I'll update to v1 (I assume in 1.8 both are supported).

piosz · 2017-08-30T18:27:51Z

ping

sttts · 2017-08-30T18:49:34Z

let's stay with v1beta1 here, because people may want to test it with 1.7 as well. Once 1.8 is released I'll update to v1 (I assume in 1.8 both are supported).

Sounds reasonable.

luxas

/lgtm

The nits I left there are non-blocking

luxas · 2017-08-30T22:09:47Z

deploy/metrics-apiservice.yaml

+  service:
+    name: metrics-server
+    namespace: kube-system
+  group: metrics


Again, why not move to metrics.k8s.io now when we can?

Will do once kubernetes/kubernetes#51653 is merged.

luxas · 2017-08-30T22:12:34Z

deploy/metrics-apiservice.yaml

+  group: metrics
+  version: v1alpha1
+  insecureSkipTLSVerify: true
+  groupPriorityMinimum: 100


Are these two values actually the recommended ones? IIRC one of them should be higher

piosz · 2017-09-01T08:52:58Z

Merging - in case of more comments I'll address them in a follow up PR.

…_tools Bump release tools

Vulnerability kubernetes-sigs#1: GO-2024-2611 Infinite loop in JSON unmarshaling in google.golang.org/protobuf More info: https://pkg.go.dev/vuln/GO-2024-2611 Module: google.golang.org/protobuf Found in: google.golang.org/protobuf@v1.32.0 Fixed in: google.golang.org/protobuf@v1.33.0 Example traces found: kubernetes-sigs#1: json.Decoder.Peek kubernetes-sigs#2: json.Decoder.Read kubernetes-sigs#3: protojson.Unmarshal kubernetes-sigs#4: protojson.UnmarshalOptions.Unmarshal Signed-off-by: Damien Grisonnet <dgrisonn@redhat.com>

piosz assigned DirectXMan12 May 31, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 31, 2017

deads2k reviewed May 31, 2017

View reviewed changes

cheftako reviewed May 31, 2017

View reviewed changes

luxas reviewed Jun 8, 2017

View reviewed changes

piosz mentioned this pull request Jun 19, 2017

Metrics Server (for resource metrics API) kubernetes/enhancements#271

Closed

Added deployment files

c511f44

piosz force-pushed the deploy branch from 158d274 to c511f44 Compare August 23, 2017 12:58

k8s-ci-robot assigned luxas Aug 23, 2017

piosz closed this Aug 24, 2017

piosz reopened this Aug 24, 2017

deads2k reviewed Aug 24, 2017

View reviewed changes

luxas reviewed Aug 24, 2017

View reviewed changes

KarolKraskiewicz mentioned this pull request Aug 30, 2017

metrics client for VPA, using metrics-server kubernetes/autoscaler#282

Closed

luxas approved these changes Aug 30, 2017

View reviewed changes

piosz merged commit 685b735 into kubernetes-sigs:master Sep 1, 2017

piosz deleted the deploy branch September 1, 2017 08:53

piosz mentioned this pull request Sep 1, 2017

Introducing metrics-server kubernetes/kubernetes#51792

Merged

kennethaasan mentioned this pull request May 18, 2018

Adds metrics-server chart helm/charts#5538

Merged

smarterclayton pushed a commit to smarterclayton/metrics-server that referenced this pull request Nov 11, 2018

Merge pull request kubernetes-sigs#4 from smarterclayton/bump_release…

3ded845

…_tools Bump release tools

		@@ -0,0 +1,12 @@
		apiVersion: rbac.authorization.k8s.io/v1beta1

		@@ -0,0 +1,13 @@
		apiVersion: rbac.authorization.k8s.io/v1alpha1

		@@ -0,0 +1,14 @@
		apiVersion: rbac.authorization.k8s.io/v1alpha1

Added deployment files #4

Added deployment files #4

Conversation

piosz commented May 31, 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

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

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

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

luxas 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

DirectXMan12 Jun 20, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piosz commented Aug 23, 2017

luxas commented Aug 23, 2017

luxas commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment • edited Loading

Choose a reason for hiding this comment

piosz commented Aug 29, 2017

piosz commented Aug 30, 2017

sttts commented Aug 30, 2017

luxas 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

piosz commented Sep 1, 2017

DirectXMan12 Jun 20, 2017 •

edited

Loading

luxas left a comment •

edited

Loading