Repository navigation
fix(metrics-server): move the listener off the kubelet's port - #3361
Conversation
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]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a port conflict between the metrics-server and the kubelet by moving the metrics-server listener to port 4443. This change ensures that hostNetwork mode can function correctly without triggering 'address already in use' errors, while also preventing potential routing ambiguities. The implementation leverages existing named port references in the chart to ensure a smooth transition without requiring additional patches to the service or probes. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
size/L
dosubot
Bot
added
the
area/monitoring
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe metrics-server chart now explicitly configures container port ChangesMetrics-server port wiring
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a test target to the metrics-server Makefile to run Helm unit tests, introduces a new test suite (container_port_test.yaml) to verify container port configurations, and overrides the default containerPort to 4443 in values.yaml to prevent port conflicts and hostNetwork issues. The review feedback correctly identifies that notExists is not a supported assertion in helm-unittest and suggests using isNull instead to verify the absence of the hostNetwork field.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - notExists: | ||
| path: spec.template.spec.hostNetwork |
There was a problem hiding this comment.
notExists is not a supported assertion in helm-unittest. To assert that a field is absent or not defined in the rendered manifest, you should use the isNull assertion instead.
- isNull:
path: spec.template.spec.hostNetwork
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — the change is right and the port choice is well reasoned. One request, and it is about the description rather than the code: the severity as written is not supported by what ships.
Business context: third occupant of port 10250 after #3359 and #3360, moved to 4443 — deliberately not 10260, so that a hostNetwork instance cannot occupy in the node netns the port those two webhook fixes now depend on.
Please restate the severity
The description says metrics-server has a first-class hostNetwork mode that "on the default port cannot start at all" (bind: address already in use), framing this as a hard defect.
Evidence that it is latent and opt-in only: charts/metrics-server/values.yaml:70-76 sets hostNetwork.enabled: false; the template gate is at templates/deployment.yaml:51-53; packages/system/metrics-server/values.yaml on main sets no hostNetwork key; and a repo-wide grep finds no enablement anywhere, management or tenant. So the claim is true about the upstream chart but describes a condition no shipped cozystack configuration reaches.
Notably the PR's own values.yaml comment is honest about this ("Nothing here establishes that this happens") — it is only the description that hardens it into a hard defect. Since the description is what reviewers and changelog readers see, it is worth aligning the two. The change is still clearly worth making on the port-hygiene argument alone.
Second, narrow the bundle claim: "ships default-enabled in the platform bundle" holds for isp-full and isp-full-generic, but not isp-hosted, which sets system.enabled: false (values-isp-hosted.yaml:4-5).
Verified in the PR's favour
- Tenant propagation is real — this was my leading candidate for a wrong claim and it checks out.
packages/core/platform/sources/kubernetes-application.yaml:63-64points the tenant component atpath: system/metrics-server, the same directory assources/metrics-server.yaml:19, and the HelmRelease atpackages/apps/kubernetes/templates/helmreleases/metrics-server.yamlhas novalues:and novaluesFrom:at all. One values.yaml, two consumers — the blast radius genuinely is every tenant cluster. - The 4443-not-10260 reasoning is sound, even though it turns out not to be an ordering constraint (see below). 4443 is metrics-server's own pre-v0.7.0 manifest value, and the literal appears nowhere else in the tree as a port.
- No e2e coverage of
metrics.k8s.io— confirmed by grep acrosshack/,packages/apps/kubernetes/and.github/: zero hits formetrics.k8s.ioorkubectl top. The PR's admitted risk is accurate and honestly stated. - Tenant reachability is de-risked further than the PR claims: tenant control planes reach tenant pods via konnectivity (
packages/apps/kubernetes/templates/cluster.yaml:389), which tunnels arbitrary TCP with no port allowlist, and the tenant package ships no NetworkPolicy restricting pod ports. So the open "does some substrate filter 4443" question does not bite on the tenant path. apiService.insecureSkipTLSVerify: trueconfirmed (charts/metrics-server/values.yaml:45); ServicetargetPort: httpsand both probesport: httpsconfirmed (templates/service.yaml:18-22,values.yaml:102-115).- VPA does not consume
metrics.k8s.io— it reads Prometheus storage (vertical-pod-autoscaler/values.yaml:30,36), so it is unaffected. - The new
test:target genuinely wires the suite into CI. The Makefile had none on main, andhack/helm-unit-tests.sh:22skips packages without one, so without this addition the suite would never have run.helm unittest .→ 4 tests, all pass.
Minor precision point: the ServiceMonitor names the Service port, not the container port, so it is insulated by one indirection rather than by the container port being named. The values.yaml comment says this correctly; the description lumps the two together.
Merge order: none required
The concern that this PR merging late could leave a window where metrics-server squats 10260 does not hold, for two independent reasons: it moves 10250 → 4443 and never transits 10260 in any intermediate state, and hostNetwork is off by default and enabled nowhere in the tree, so metrics-server is pod-netns in every shipped configuration. The 4443 choice remains good forward-looking design — just not an ordering constraint.
Dismissed
- VPA admission-controller colliding on 4443 under hostNetwork —
hostNetwork: false(charts/.../values.yaml:108) and its ports are 8000/8944 (deployment.yaml:165-166). - A Talos service on 4443 — the repo ships no Talos machine-config;
packages/core/talos/is imager profiles only.
…opes (#3362) ## What this PR does PRs titled with the `cert-manager`, `external-secrets` or `metrics-server` scopes landed on `area/uncategorized` because the labeler's scope map had no entry for them — all three surfaced at once in the current port-collision series (#3359, #3360, #3361). metrics-server feeds the monitoring pipeline, so it maps to `area/monitoring`; cert-manager and external-secrets are platform services, so they map to `area/platform`. ### Release note ```release-note NONE ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Improved pull request labeling automation. - Pull requests involving metrics, certificate management, and external secrets are now categorized more accurately with the appropriate area labels. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
What this PR does
metrics-server is the third component serving on port 10250 — the kubelet's port — after the webhooks addressed in #3359 and #3360. It ships default-enabled in the platform bundle and is also installed into every tenant Kubernetes cluster with no value overrides, so a package-root override propagates to both.
The hard defect here is different from the webhook cases. The chart has first-class
hostNetwork.enabledsupport, documented upstream for clusters where the API server cannot reach pod IPs — and on the default port that mode cannot start at all: the kubelet already holds 10250, so the process dies withbind: address already in use. Upstream is aware (kubernetes-sigs/metrics-server#1026 carries the exact panic) and declined to change the chart default, advising hostNetwork users to set their own port. This override is that port, set ahead of need.The misleading-certificate angle of the sibling fixes mostly does not apply: the chart defaults
apiService.insecureSkipTLSVerifyto true, so a connection misrouted to a node would fail silently rather than with a wrong-certificate error — and with endpoints resolving to pod IPs such a misroute is not established to occur here at all. The comment states that as a hypothesis, not a fact. What is factual is the consumer set:kubectl top, tenant HPAs, and the console's cluster-usage page all readmetrics.k8s.io; the shipped VPA does not — it is configured with Prometheus storage.The port is 4443 rather than the siblings' 10260, for a reason worth keeping: metrics-server is the one component of the three with supported hostNetwork, and parking it on 10260 would put a live host-network listener on exactly the port the webhook fixes rely on being silent — turning their honest connection refusals back into answered handshakes. 4443 is the value metrics-server's own manifests shipped through v0.6.4, before v0.7.0 moved them to 10250; it sits outside the 10248-10259 node-component block, and nothing in the tree binds it.
No chart patch is needed: the Service, both probes and the ServiceMonitor already reference the port by name, so the rollout-gap that required a vendored patch in #3360 does not exist here. The package also gains the standard
test:target it was missing.Not verified without a cluster: that the aggregated APIService stays Available through an upgrade, and that no supported substrate filters 4443 inbound on nodes — the tree configures no host firewall, but Talos and managed substrates are outside what a render can prove. There is no e2e coverage of
metrics.k8s.ioat all; a wrong guess here would surface as HPAs silently not scaling.Release note
Summary by CodeRabbit
Bug Fixes
Tests