Repository navigation
fix(external-secrets): move the webhook off the kubelet's port - #3360
Conversation
github-actions
Bot
added
the
size/L
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 critical port collision issue where the external-secrets webhook shared the kubelet's port (10250). By moving the webhook to port 10260 and updating the Service to use a named target port, the changes eliminate potential misrouting scenarios that previously resulted in confusing TLS handshake failures. These updates ensure more robust admission control and CRD conversion stability during operator deployments. 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. Ignored Files
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
area/uncategorized
dosubot
Bot
added
the
area/platform
📝 WalkthroughWalkthroughThe external-secrets operator chart configures its webhook on port 10260, routes Service traffic through the named ChangesExternal Secrets webhook networking
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 configures the external-secrets-operator package to run its webhook on port 10260 instead of the default 10250 to prevent certificate validation errors caused by traffic misrouting to the kubelet. It adds a new Helm unit test suite to verify these port configurations and admission policies, and updates the Makefile to apply a downstream patch and run these tests. The review feedback suggests adding a trailing newline to the Makefile to comply with standard POSIX formatting.
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.
| # the named targetPort makes patch(1) silently reverse the hunk and exit | ||
| # 0, quietly restoring the numeric form. With it, an already-applied | ||
| # patch fails loudly instead. | ||
| patch --no-backup-if-mismatch --forward -p4 < patches/webhook-service-named-target-port.patch No newline at end of file |
There was a problem hiding this comment.
The Makefile is missing a newline at the end of the file. It is standard practice to ensure all source files end with a newline character to adhere to POSIX standards and avoid warnings from various development tools.
patch --no-backup-if-mismatch --forward -p4 < patches/webhook-service-named-target-port.patch
The chart defaults the validating webhook to port 10250, which the kubelet also serves on. When a connection to the webhook Service resolves to a node IP rather than a Pod IP it reaches the kubelet, which completes the TLS handshake with its node serving certificate, and the apiserver rejects the call with an x509 error naming the node instead of the webhook Service. The certificate is valid, just the wrong server's, so the failure reads as a PKI fault and hides the misroute behind it. Moving the listener to a port nothing else answers on turns the same misroute into a connection refusal, which names the real problem. This does not prevent the misroute. 9443, the controller-runtime default this chart would otherwise take, is disqualified: the etcd kube-rbac-proxy binds it on host-network, so it would swap one wrong certificate for another. Nothing in the tree binds 10260 in a node network namespace. It remains CloudControllerManagerWebhookPort in k8s.io/cloud-provider, so a host-network cloud-controller-manager added later has to keep clear of it. Point the Service at the "webhook" container-port name so the move is safe. Upstream renders targetPort from webhook.port as a number, which every endpoint shares regardless of what its Pod listens on, so changing the port alone would aim the Service at a port the outgoing Pod does not serve until the replacement passes readiness. That window rejects admission for externalsecrets, secretstores and clustersecretstores, and breaks CRD conversion, which routes through the same Service. No values-only setting closes it; a named targetPort resolves per-Pod and does. Add unit tests pinning the listener flag, the containerPort and the named targetPort. This chart's values.schema.json allows unknown keys, so a renamed upstream value would be silently ignored and the port would revert to 10250 with nothing else noticing. Assisted-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
70a4548 to
a90fa38
Compare
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
LGTM — this was the highest-severity item in the series and it holds up under the most rigorous check available short of a live cluster. The failurePolicy: Fail gap is real and the patch genuinely closes it.
Business context: same trap as cert-manager (#3359) — the external-secrets webhook listens on 10250, the kubelet's port, so a misroute to a node IP yields a valid-but-wrong certificate instead of an honest connection refused. The difference is that here the Service targets the port by number, so a bare port change would have opened a real admission and conversion outage on rollout.
Verified end to end
The gap without the patch is real. Upstream renders targetPort: {{ .Values.webhook.port }} as an integer. With replicaCount: 1 (charts/external-secrets/values.yaml:248), changing only the port would leave the Service's single endpoint pointing at 10260 on a pod still serving 10250 — every admission call for externalsecrets / secretstores / clustersecretstores and CRD conversion failing until the replacement passes readiness. failurePolicy: Fail (values.yaml:258) makes that a hard cluster-wide outage, not a degradation. The PR is right that surge-first does not help when both pods sit behind one port number.
The patch closes it. The containerPort is already named webhook upstream (charts/external-secrets/templates/webhook-deployment.yaml:78-80), so the named targetPort resolves per-pod and the old and new pods are each reached correctly during the overlap. The metrics port in the same Service was already named, which confirms the PR's "consistent with its neighbour" framing rather than introducing a new convention.
I exercised the patch mechanics rather than reading them. Restored the pristine upstream file from origin/main and ran the exact Makefile command: patch --no-backup-if-mismatch --forward -p4 applied cleanly (exit 0) and produced exactly the committed state — targetPort: webhook at line 26, targetPort: metrics at line 32. -p4 is the correct strip depth given make's working directory is the package dir.
The --forward guard works as documented. Re-running against the already-patched tree produces "Reversed (or previously applied) patch detected! Skipping patch." and exit 1, so make update aborts loudly. That is the intended tripwire for the eventual chart bump past 0.19.1, and it is not theoretical.
The test is load-bearing here in a way it is not for #3359. external-secrets' values.schema.json contains zero additionalProperties entries, so unlike cert-manager, an upstream rename of webhook.port would be silently accepted and ignored — reverting to 10250 with no error anywhere. The test suite really is the only guard, and the PR states this correctly. helm unittest . → 6 tests, all pass.
No maintenance note needed. Helm's kind sorter applies Service before Deployment in the same release, and with the named targetPort there is no window with zero ready endpoints.
Follow-ups (non-blocking)
1. The exclusivity claim goes stale at merge. packages/system/external-secrets-operator/values.yaml:40 states "Nothing in this repo binds 10260." Once #3359 lands, cert-manager binds it (packages/system/cert-manager/values.yaml:41), so the sentence becomes literally false regardless of merge order.
Note #3359 got this right by scoping to the network namespace — "nothing answers on it in the node's network namespace" — which stays true because the external-secrets webhook is pod-netns. Suggest rescoping this sentence the same way.
2. Note the sibling under hostNetwork. After both PRs land, cert-manager's webhook and this one both target 10260. Harmless as shipped (both pod-netns), but both charts document hostNetwork as the supported remedy for managed clusters where the apiserver cannot reach pod IPs. Enabling it on both gives the second pod bind: address already in use — exactly what #3361 avoids for metrics-server. The hostNetwork caveat at values.yaml:48-50 currently only warns about the cilium network policy; one sentence naming cert-manager would close it.
Note on merge order
None required for this series — metrics-server (#3361) moves 10250 → 4443 and never transits 10260, and its hostNetwork is off by default and enabled nowhere in the tree.
## 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.enabled` support, 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 with `bind: 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.insecureSkipTLSVerify` to 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 read `metrics.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.io` at all; a wrong guess here would surface as HPAs silently not scaling. ### Release note ```release-note fix(metrics-server): move the listener off port 10250, which the kubelet also holds. The default made hostNetwork mode unable to start at all, and the new port keeps node-port collisions out of the metrics path. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Configured the metrics server to listen on secure port 4443. * Updated service routing and health checks to use the named HTTPS port, improving reliability when port settings change. * Ensured pod networking does not rely on host networking. * **Tests** * Added automated chart tests covering container ports, service routing, and liveness/readiness probes. * Added a test command for running the chart test suite. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…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
The external-secrets webhook listens on port 10250 — the kubelet's port — with
failurePolicy: Fail. This is the same trap being removed from cert-manager in #3359: a connection to the webhook Service that resolves to a node IP instead of the pod IP reaches the kubelet, which completes the handshake with its own node serving certificate, and the API server rejects the call with an x509 error that blames the webhook's PKI rather than the routing layer. The port collision does not cause the misroute; it turns a rare transient one into a confidently misattributed failure.The blast radius here is wider than admission alone:
crds.conversion.enableddefaults to true and points every ExternalSecret CRD's conversion webhook at the same Service, so a misrouted connection also breaks CRD conversion — reads at non-storage versions, not just writes.The webhook moves to 10260, matching #3359. The controller-runtime default 9443 was considered and rejected: the etcd kube-rbac-proxy binds it on host network, which would reproduce exactly the same wrong-certificate failure with a different certificate. Nothing in the tree binds 10260.
Unlike cert-manager, the upstream chart's Service targets the port numerically —
targetPort: {{ .Values.webhook.port }}— and a numeric targetPort is copied into every endpoint verbatim rather than resolved per pod. With one replica, a port change would direct traffic at a port the still-running old pod does not serve while the replacement is not yet Ready, failing every admission and conversion call in the gap; surge-first rollout does not help because both pods sit behind the same number. A one-line vendored-chart patch points the Service at thewebhookcontainer-port name upstream already declares (the metrics port in the same Service is already named), which closes the gap: during the overlap each pod is routed on the port it actually serves. The patch is applied bymake updatewith--forward, so an already-applied hunk fails loudly instead of being silently reversed. Upstream has already made the same change — merged as external-secrets/external-secrets#5108 and shipped since chart 0.19.1 — but the vendored chart here is 0.10.4, nine minor releases earlier, so the patch carries the fix until the chart is bumped past that point. The--forwardguard is exactly what will flag that moment: on a bumped chart the patch fails loudly, signalling it should be dropped.external-secrets-operator ships disabled by default and is enabled per-cluster via
bundles.enabledPackages, so this is defensive hardening for opt-in installs rather than a response to an observed incident.metrics-serveris a third instance of the same port collision, left out here: it serves an APIService rather than an admission webhook, a lesser failure class deserving its own change.Release note
Summary by CodeRabbit
targetPort(webhook) for pod traffic.