fix(infra): set explicit host on envoy PreStop httpGet to fix hostNetwork drain - #9861
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e84c3ae56
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // pod IP for the PreStop hook in hostNetwork mode, causing the | ||
| // hook to fail with "failed to find networking container" | ||
| // (see https://github.com/kubernetes/kubernetes/issues/134285). | ||
| Host: "127.0.0.1", |
There was a problem hiding this comment.
When the generated pod does not use hostNetwork—including the default Deployment and DaemonSet configurations—the kubelet sends this HTTP lifecycle request from the node network namespace, so 127.0.0.1:19002 refers to the node rather than the pod where the shutdown-manager listens. The PreStop hook will therefore fail for ordinary pods, disabling graceful draining for the majority of installations; set this host only for host-networked pods or use a hook executed inside the pod's network namespace.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9861 +/- ##
==========================================
+ Coverage 76.29% 76.32% +0.02%
==========================================
Files 261 261
Lines 44607 44623 +16
==========================================
+ Hits 34034 34059 +25
+ Misses 8318 8311 -7
+ Partials 2255 2253 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // This must not be set for non-hostNetwork pods: the kubelet runs | ||
| // HTTPGet lifecycle hooks from the node's network namespace, so | ||
| // "127.0.0.1" would target the node itself rather than the pod. | ||
| preStopHTTPGet.Host = "127.0.0.1" |
There was a problem hiding this comment.
what about IPv6?
TBH, I don't think we should do this in code, mentioned it in somewhere should be enough.
|
Good point on IPv6 — hardcoding On whether this belongs in code at all: that's a fair call to make either way, and not one I want to push back on. If you'd rather we just document the kubelet bug and the workaround (e.g. a note in the hostNetwork docs) instead of carrying IP-family branching logic here for it, I'm fine closing this out in favor of that — happy to send that doc note if that's the preferred direction, just let me know. |
|
-1 to adding code that adds complexity for the patch case to solve an upstream bug, this could be worked around with another patch for |
|
Fair point on IPv6 — On doing it in code at all: I don't have a strong argument against just documenting it as a known kubelet limitation (with a link to kubernetes/kubernetes#134285) instead. Would you rather I drop this code change and turn it into a docs note, or is it worth extending to use |
+1 to adding gotchas + workarounds in docs |
…code workaround Per maintainer feedback on PR envoyproxy#9861 (zirain: IPv6 isn't handled and this shouldn't live in code; arkodg: prefer documenting the workaround), revert the Host override on the envoy PreStop httpGet hook and instead document the kubelet hostNetwork bug and the patch-based workaround. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Made this a docs-only note per the discussion above (dropped the code path, added the kubelet bug + workaround to the hostNetwork docs) — let me know if the wording needs adjusting or anything else would help move this along. |
…work drain **What this PR does / why we need it**: In `hostNetwork: true` deployments, the Envoy container's PreStop lifecycle hook (an httpGet call to the shutdown-manager sidecar's `/shutdown/ready` endpoint on port 19002) fails with a kubelet error: "failed to find networking container". Because the PreStop hook never completes, the shutdown-manager never gets a chance to drain connections and fail active health checks before Envoy exits, so in-flight connections can be dropped on pod termination. This is caused by a confirmed upstream kubelet bug (kubernetes/kubernetes#134285): for hostNetwork pods, the PodSandboxStatus reported by the CRI has an empty pod IP, so kubelet cannot resolve an implicit target address for the PreStop httpGet action. **Approach**: Set `Host: "127.0.0.1"` explicitly on the envoy container's PreStop httpGet action in `internal/infrastructure/kubernetes/proxy/resource.go`, instead of leaving it unset (which triggers kubelet's buggy pod-IP resolution path). Golden testdata files for the affected deployments and daemonsets are regenerated to include `host: 127.0.0.1` under the PreStop httpGet spec. **Which issue(s) this PR fixes**: Fixes # Assisted-by: claude-sonnet-5 (via Claude Code) --- **PR Checklist** - [x] **Authorship & ownership**: Coding agents / AI assistants are welcome, but I have reviewed every change, understand how and why it works, can explain and maintain it, and take full responsibility for this PR. I have not submitted generated output I do not understand. - [x] **DCO**: All commits are signed off (`git commit -s`). - [x] **API agreed first**: N/A this PR does not contain API changes. - [x] **Required checks pass**: `go build ./internal/infrastructure/kubernetes/proxy/...`, `go test ./internal/infrastructure/kubernetes/proxy/...`, and `golangci-lint run ./internal/infrastructure/kubernetes/proxy/...` (0 issues) all pass locally. `make generate gen-check` N/A (no API/CRD/helm changes). - [x] **Tests added/updated**: Existing golden-file tests (TestDeployment, TestDaemonSet, TestGatewayNamespaceModeMultipleResources) cover this container spec; regenerated via the repo's own `-override-testdata` flag after confirming they failed against the old golden data (Host: "" -> "127.0.0.1") and pass against the new golden data. - [x] **Docs**: N/A, no user-facing docs changes; this only changes an internal container spec. - [x] **Release notes**: Added `release-notes/current/bug_fixes/9853-envoy-prestop-hostnetwork.md`. - [x] **Generated files committed**: N/A, no API/helm chart changes. - [x] **Scope & compatibility**: Change is limited to one Go file plus mechanically regenerated golden testdata; backward compatible (no user-facing config or API changes). - [ ] **Codex review**: Requested a Codex review and addressed all of its comments. - [ ] **Copilot review**: Requested a Copilot review and addressed all of its comments. **Validation**: I do not have access to a live hostNetwork Kubernetes cluster in this environment, so I could not reproduce the exact end-to-end kubelet failure. Instead: (1) `go test ./internal/infrastructure/kubernetes/proxy/...` failed against the pre-fix golden data with a diff showing `Host: (string) "" -> Host: (string) "127.0.0.1"`, confirming the code change takes effect; (2) after regenerating golden data with `-override-testdata`, the full package test suite passes; (3) `golangci-lint run ./internal/infrastructure/kubernetes/proxy/...` reports 0 issues. The root cause is a documented, confirmed upstream kubelet bug referenced directly in the linked issue's comments. Report: envoyproxy#9853 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Setting Host: 127.0.0.1 unconditionally on the envoy PreStop hook broke graceful shutdown for the default (non-hostNetwork) Deployment/DaemonSet case, since kubelet runs HTTPGet lifecycle hooks from the node's network namespace: hardcoding the host made the request target the node instead of the pod. This was caught by all 6 e2e-test jobs failing on TestEGUpgrade/EnvoyShutdown, and matches a concern raised by an automated review bot on the PR. hostNetwork is only ever enabled here via a user-supplied Deployment/ DaemonSet patch (there's no first-class API field for it), so detect it by inspecting that patch and only apply the Host override when it's set, restoring the original working behavior otherwise. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…k pods only" This reverts commit b3dfb8f. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
… hostNetwork drain" This reverts commit 3e84c3a. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
…code workaround Per maintainer feedback on PR envoyproxy#9861 (zirain: IPv6 isn't handled and this shouldn't live in code; arkodg: prefer documenting the workaround), revert the Host override on the envoy PreStop httpGet hook and instead document the kubelet hostNetwork bug and the patch-based workaround. Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
Fixed the failing DCO check — the two revert commits were missing |
f49740a to
f773b38
Compare
3 participants
What this PR does / why we need it:
In
hostNetwork: truedeployments, the Envoy container's PreStop lifecyclehook (an httpGet call to the shutdown-manager sidecar's
/shutdown/readyendpoint on port 19002) fails with a kubelet error: "failed to find
networking container". Because the PreStop hook never completes, the
shutdown-manager never gets a chance to drain connections and fail active
health checks before Envoy exits, so in-flight connections can be dropped
on pod termination.
This is caused by a confirmed upstream kubelet bug
(kubernetes/kubernetes#134285): for hostNetwork
pods, the PodSandboxStatus reported by the CRI has an empty pod IP, so
kubelet cannot resolve an implicit target address for the PreStop httpGet
action.
Approach:
Set
Host: "127.0.0.1"explicitly on the envoy container's PreStophttpGet action in
internal/infrastructure/kubernetes/proxy/resource.go,instead of leaving it unset (which triggers kubelet's buggy pod-IP
resolution path). Golden testdata files for the affected deployments and
daemonsets are regenerated to include
host: 127.0.0.1under the PreStophttpGet spec.
Which issue(s) this PR fixes:
Fixes #
Assisted-by: claude-sonnet-5 (via Claude Code)
PR Checklist
git commit -s).go build ./internal/infrastructure/kubernetes/proxy/...,go test ./internal/infrastructure/kubernetes/proxy/..., andgolangci-lint run ./internal/infrastructure/kubernetes/proxy/...(0 issues) all pass locally.make generate gen-checkN/A (no API/CRD/helm changes).-override-testdataflag after confirming they failed against the old golden data (Host: "" -> "127.0.0.1") and pass against the new golden data.release-notes/current/bug_fixes/9853-envoy-prestop-hostnetwork.md.Validation: I do not have access to a live hostNetwork Kubernetes cluster
in this environment, so I could not reproduce the exact end-to-end kubelet
failure. Instead: (1)
go test ./internal/infrastructure/kubernetes/proxy/...failed against the pre-fix golden data with a diff showing
Host: (string) "" -> Host: (string) "127.0.0.1", confirming the codechange takes effect; (2) after regenerating golden data with
-override-testdata, the full package test suite passes; (3)golangci-lint run ./internal/infrastructure/kubernetes/proxy/...reports0 issues. The root cause is a documented, confirmed upstream kubelet bug
referenced directly in the linked issue's comments.
Report: #9853
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com
AI assistance: this change was drafted with Claude Code.