Skip to content

Commit aa2525a

Browse files
lexfreilexfrei
committed
fix(metrics-server): move the listener off the kubelet's port
metrics-server scrapes every kubelet on 10250 and, by upstream default, serves its own aggregated API there too. Sharing that number makes hostNetwork unusable. The chart offers it for clusters whose apiserver cannot reach Pod IPs, but at the default port the process joins the node namespace where the kubelet already holds 10250 and exits with "bind: address already in use". Upstream has had this reported and kept the default anyway, on the view that hostNetwork is non-standard here and that those users can set a port themselves. It also decides how a misrouted connection would present. Were a connection to the Service to arrive at a node IP rather than a Pod IP it would reach the kubelet, and because the APIService sets insecureSkipTLSVerify the aggregation layer would never check whose certificate it got, leaving nothing in the error to name the wrong server. Nothing here establishes that such a misroute occurs; moving the port only makes one fail as a connection refusal rather than as a rejection from an unexpected daemon. The override gives up the firewall coverage 10250 buys on substrates permitting only 443 and 10250 to nodes. Cozystack's own do not filter it, but this package ships into every tenant cluster and no e2e exercises the metrics API. 4443 is the value upstream's manifests carried through v0.6.4, before v0.7.0 shipped 10250. It sits clear of the 10248-10259 block the node components occupy and nothing else in this repo binds it. The Service's targetPort and both probes reach the container by name, so the number moves without a rollout gap. The tests pin that alongside the value, since the vendored chart ships no schema that would reject an upstream rename of the key. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
1 parent 7e206e2 commit aa2525a

3 files changed

Lines changed: 146 additions & 0 deletions

File tree

‎packages/system/metrics-server/Makefile‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@ export NAMESPACE=cozy-monitoring
33

44
include ../../../hack/package.mk
55

6+
test:
7+
helm unittest .
8+
69
update:
710
rm -rf charts
811
helm repo add metrics-server https://kubernetes-sigs.github.io/metrics-server/

‎packages/system/metrics-server/tests/container_port_test.yaml‎

Lines changed: 84 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,84 @@
1+
suite: metrics-server container port
2+
3+
# Keeps metrics-server's own listener off 10250, the port it scrapes kubelets
4+
# on. The values.yaml override this guards carries the reasoning; it is not
5+
# repeated here.
6+
#
7+
# The first test is the regression guard. The vendored chart ships no
8+
# values.schema.json, so an upstream rename of `containerPort` is accepted and
9+
# silently ignored rather than failing helm — this test is the only thing
10+
# between a re-vendored chart and a silent return to 10250.
11+
#
12+
# The rest pin preconditions the fix depends on and that already hold on their
13+
# own: that the port lives in the Pod's network namespace rather than the
14+
# node's, and that the Service and both probes reach it by name rather than by
15+
# number, which is what lets the number move without anything else having to
16+
# be taught to follow it. `make update` re-vendors this chart wholesale, so
17+
# these are the regression surface for that too.
18+
19+
templates:
20+
- charts/metrics-server/templates/deployment.yaml
21+
- charts/metrics-server/templates/service.yaml
22+
23+
release:
24+
name: metrics-server
25+
namespace: cozy-monitoring
26+
27+
tests:
28+
- it: serves on 4443, not 10250, the port it scrapes kubelets on
29+
template: charts/metrics-server/templates/deployment.yaml
30+
asserts:
31+
- isKind:
32+
of: Deployment
33+
- contains:
34+
path: spec.template.spec.containers[0].args
35+
content: --secure-port=4443
36+
- equal:
37+
path: spec.template.spec.containers[0].ports[0]
38+
value:
39+
name: https
40+
protocol: TCP
41+
containerPort: 4443
42+
43+
# Also a tripwire: enabling hostNetwork here trips this test, which is the
44+
# intended prompt to go and add 4443 to the clusterwide policy in
45+
# packages/system/cilium-networkpolicy before the listener joins the node's
46+
# namespace. See the reasoning in values.yaml.
47+
- it: keeps metrics-server off the host network, so the port lives in the Pod netns
48+
template: charts/metrics-server/templates/deployment.yaml
49+
asserts:
50+
# The template emits the key only when hostNetwork.enabled is truthy, so
51+
# absence is the only shape a disabled hostNetwork can take here.
52+
# isNullOrEmpty would not do: it fails on a path that does not exist.
53+
- notExists:
54+
path: spec.template.spec.hostNetwork
55+
56+
- it: reaches the container by port name, so the port can move without a traffic gap
57+
template: charts/metrics-server/templates/service.yaml
58+
asserts:
59+
- isKind:
60+
of: Service
61+
- equal:
62+
path: spec.ports[0].name
63+
value: https
64+
- equal:
65+
path: spec.ports[0].port
66+
value: 443
67+
# A name, not a number. A numeric targetPort is copied into every
68+
# endpoint regardless of what each Pod actually listens on, which would
69+
# aim the Service at a port the outgoing Pod does not serve until its
70+
# replacement passes readiness. Upstream already names it here; if that
71+
# ever regresses to an integer, moving the port reintroduces that gap.
72+
- equal:
73+
path: spec.ports[0].targetPort
74+
value: https
75+
76+
- it: probes the container by port name, so a port change cannot strand readiness
77+
template: charts/metrics-server/templates/deployment.yaml
78+
asserts:
79+
- equal:
80+
path: spec.template.spec.containers[0].livenessProbe.httpGet.port
81+
value: https
82+
- equal:
83+
path: spec.template.spec.containers[0].readinessProbe.httpGet.port
84+
value: https

‎packages/system/metrics-server/values.yaml‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,63 @@
11
metrics-server:
2+
# Upstream defaults this to 10250 — the kubelet's port, and the port
3+
# metrics-server itself scrapes every kubelet on.
4+
#
5+
# That default makes hostNetwork unusable. The chart offers
6+
# hostNetwork.enabled for clusters whose apiserver cannot reach Pod IPs, but
7+
# enabling it at the default puts metrics-server in the node's namespace
8+
# where the kubelet already holds 10250, and the process exits with "bind:
9+
# address already in use". Upstream has had this reported
10+
# (kubernetes-sigs/metrics-server#1026) and kept the default anyway, on the
11+
# view that hostNetwork is non-standard here and that those users can set a
12+
# port themselves. This is that port, picked before the need rather than
13+
# after it.
14+
#
15+
# It also removes a collision that would make a misrouted connection hard to
16+
# read. Were a connection to the metrics-server Service to arrive at a node
17+
# IP instead of a Pod IP, it would reach the kubelet, listening on the same
18+
# number. Nothing here establishes that this happens — with pod-networked
19+
# endpoints there is no ordinary path to it — so this is not the fix for a
20+
# known routing bug. It only decides how such a misroute would present: as a
21+
# connection refusal, rather than as a rejection from an unexpected daemon.
22+
# That distinction is sharper here than for a webhook, because this chart's
23+
# APIService sets insecureSkipTLSVerify, so the aggregation layer never
24+
# checks whose certificate it received and nothing in the resulting error
25+
# would name the mismatch.
26+
#
27+
# What the override gives up is worth stating. Upstream moved to 10250 so
28+
# metrics-server would be covered by the firewall rules most clusters
29+
# already have between control plane and nodes, and work without new ones. A
30+
# substrate permitting only 443 and 10250 to nodes therefore needs this
31+
# value reconsidered. Cozystack's own — Talos, k3s, kubeadm — do not filter
32+
# it, but this package also ships into every tenant cluster and no e2e
33+
# exercises the metrics API, so a wrong guess here would first surface as a
34+
# user's HPA quietly not scaling, or as the dashboard's cluster-usage view
35+
# going blank.
36+
#
37+
# 4443 is the value upstream's manifests carried through v0.6.4, before
38+
# v0.7.0 shipped 10250, so it is a number already associated with this
39+
# component. It sits clear of the 10248-10259 block the node components
40+
# occupy, and nothing in this repo binds it.
41+
#
42+
# Under hostNetwork this listener joins the node's namespace, where the
43+
# clusterwide policy in packages/system/cilium-networkpolicy denies 10250
44+
# from the world but has no entry for 4443. That matters more than it would
45+
# elsewhere, because metrics.enabled below makes the chart add
46+
# --authorization-always-allow-paths=/metrics, leaving that one path
47+
# unauthenticated. What it exposes is metrics-server's own scrape
48+
# instrumentation, not the resource data, which stays behind the
49+
# authenticated aggregated API — but enabling hostNetwork still means adding
50+
# 4443 to that policy first rather than opening it on every node IP.
51+
#
52+
# Every reference to the port inside the chart is by name — the Service's
53+
# targetPort and both probes — so the number moves without aiming any
54+
# endpoint at a port its Pod does not serve. (The ServiceMonitor names the
55+
# Service port rather than this one, so it is unaffected either way.)
56+
# tests/container_port_test.yaml pins that and the value itself: the
57+
# vendored chart ships no values.schema.json, so an upstream rename of this
58+
# key would be silently ignored rather than failing helm.
59+
containerPort: 4443
60+
261
defaultArgs:
362
- --cert-dir=/tmp
463
- --kubelet-preferred-address-types=InternalIP,ExternalIP,Hostname

0 commit comments

Comments
 (0)