kube/controller: Prevent leaking stale endpoints #55171

dwj300 · 2025-02-19T00:24:10Z

Please provide a description of this PR:
When selecting pods via a ServiceEntry, there are times when the pod may not have an IP address:

When it is first created
When it is Evicted/Failed.

The code today handles (1), however there exists a case when the IP is removed from the Pod in the same event that marks it with a terminal (Failed) status. Due to the cross-controller interaction (between the kube registry and serviceentry registry), the WorkloadInstance needs to have a valid IP, as the podCache and endpointShards are both keyed by address. This PR adds logic to fetch the oldIP from the ipByPods cache, and uses that when building the WorkloadInstance.

Fixes #54997

istio-testing · 2025-02-19T00:24:13Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2025-02-19T00:24:15Z

😊 Welcome @dwj300! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

dwj300 · 2025-02-19T00:28:13Z

/test all

pilot/pkg/serviceregistry/kube/controller/pod.go

hzxuzhonghu · 2025-02-19T03:41:25Z

pilot/pkg/serviceregistry/kube/controller/pod.go

@@ -148,8 +148,18 @@ func (pc *PodCache) onEvent(old, pod *v1.Pod, ev model.Event) error {
 	ip := pod.Status.PodIP
 	// PodIP will be empty when pod is just created, but before the IP is assigned
 	// via UpdateStatus.
+	// In the case of a pod being created, we should not add it to the cache until it is ready.
+	// However, if the pod *used to* have an IP, we do need to actually delete it.
 	if len(ip) == 0 {


IIUC, the ip is missing when being evicted. Then you can allow ip unset if this is a deletion event

But the problem is that all the caches (podCache, and endpointShardz) are indexed by podIP, not pod name.

Isn't there a map ipByPods map[types.NamespacedName]string

Whatever this effect is same, but checking delete may reduce map quering

Sorry, missed this! But yes, this PR checks in ipByPods, but even for "non deletions" (i.e. Failed), we need to check in the map. So we only do so if len(ip) == 0 already.

Change-Id: Ibe1264bfc48b4dee7e52964ad19cee9659631f1c

pilot/pkg/serviceregistry/kube/controller/pod.go

howardjohn

This looks great, I think a test covering this would go a long way. We have a lot of similar tests+infra for this exact type of test in pilot/pkg/serviceregistry/serviceregistry_test.go already that would be a good fit

howardjohn · 2025-02-19T20:47:55Z

pilot/pkg/serviceregistry/kube/controller/pod.go

@@ -149,7 +149,14 @@ func (pc *PodCache) onEvent(old, pod *v1.Pod, ev model.Event) error {
 	// PodIP will be empty when pod is just created, but before the IP is assigned
 	// via UpdateStatus.
 	if len(ip) == 0 {
-		return nil
+		// However, in the case of an Eviction, the event that marks the pod as Failed may *also* have removed the IP.
+		// If the pod *used to* have an IP, then we need to actually delete it.


Talking to myself...

So the event may be "update" but then we hit deleteIP anyways due to shouldPodBeInEndpoints and translate to a delete.

deleteIP already handles the case of "IP was removed" since its a map of ip->[]pods.

We then fallthrough to notifyWorkloadHandlers, build an EP with no IP but WorkloadInstance is identified by name not IP. This looks good ✔️

howardjohn

👍 👍

Fixes istio#54997 Change-Id: I59d90f775d86821060588f446e4d50d77ab97921

Backported from istio#55171 * pod.go: unexport ipByPods * kube/controller: Prevent leaking stale endpoints Fixes istio#54997 Change-Id: If9d45d7563e9e94ec0bbd477dfb0f57e7608e9a9

istio-testing · 2025-02-20T21:44:07Z

In response to a cherrypick label: #55171 failed to apply on top of branch "release-1.23":

Applying: pod.go: unexport ipByPods
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
Applying: kube/controller: Prevent leaking stale endpoints
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
M	pilot/pkg/serviceregistry/serviceregistry_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceregistry_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/pod.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 kube/controller: Prevent leaking stale endpoints

istio-testing · 2025-02-20T21:44:08Z

In response to a cherrypick label: new issue created for failed cherrypick: #55195

istio-testing · 2025-02-20T21:44:46Z

In response to a cherrypick label: #55171 failed to apply on top of branch "release-1.23":

Applying: pod.go: unexport ipByPods
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
Applying: kube/controller: Prevent leaking stale endpoints
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
M	pilot/pkg/serviceregistry/serviceregistry_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceregistry_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/pod.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 kube/controller: Prevent leaking stale endpoints

istio-testing · 2025-02-20T21:44:47Z

In response to a cherrypick label: new issue created for failed cherrypick: #55196

istio-testing · 2025-02-20T21:45:23Z

In response to a cherrypick label: #55171 failed to apply on top of branch "release-1.24":

Applying: pod.go: unexport ipByPods
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
Applying: kube/controller: Prevent leaking stale endpoints
Using index info to reconstruct a base tree...
M	pilot/pkg/serviceregistry/kube/controller/pod.go
M	pilot/pkg/serviceregistry/serviceregistry_test.go
Falling back to patching base and 3-way merge...
Auto-merging pilot/pkg/serviceregistry/serviceregistry_test.go
Auto-merging pilot/pkg/serviceregistry/kube/controller/pod.go
CONFLICT (content): Merge conflict in pilot/pkg/serviceregistry/kube/controller/pod.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 kube/controller: Prevent leaking stale endpoints

istio-testing · 2025-02-20T21:45:25Z

In response to a cherrypick label: new issue created for failed cherrypick: #55197

* pod.go: unexport ipByPods Change-Id: Ibe1264bfc48b4dee7e52964ad19cee9659631f1c * kube/controller: Prevent leaking stale endpoints Fixes istio#54997 Change-Id: I59d90f775d86821060588f446e4d50d77ab97921

Backported from istio#55171 * pod.go: unexport ipByPods * kube/controller: Prevent leaking stale endpoints Fixes istio#54997 Change-Id: If9d45d7563e9e94ec0bbd477dfb0f57e7608e9a9

* pod.go: unexport ipByPods Change-Id: Ibe1264bfc48b4dee7e52964ad19cee9659631f1c * kube/controller: Prevent leaking stale endpoints Fixes istio#54997 Change-Id: I59d90f775d86821060588f446e4d50d77ab97921

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 19, 2025

istio-policy-bot added area/networking area/perf and scalability feature/Multi-cluster issues related with multi-cluster support labels Feb 19, 2025

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 19, 2025

dwj300 force-pushed the dwj--stale-endpoints branch from f508451 to e3b0ba5 Compare February 19, 2025 00:31

hzxuzhonghu reviewed Feb 19, 2025

View reviewed changes

pilot/pkg/serviceregistry/kube/controller/pod.go Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Feb 19, 2025

View reviewed changes

pod.go: unexport ipByPods

5cae8d5

Change-Id: Ibe1264bfc48b4dee7e52964ad19cee9659631f1c

dwj300 force-pushed the dwj--stale-endpoints branch 3 times, most recently from b92c37b to adf530a Compare February 19, 2025 17:38

panchr reviewed Feb 19, 2025

View reviewed changes

pilot/pkg/serviceregistry/kube/controller/pod.go Outdated Show resolved Hide resolved

pilot/pkg/serviceregistry/kube/controller/pod.go Outdated Show resolved Hide resolved

dwj300 force-pushed the dwj--stale-endpoints branch from adf530a to 71de47f Compare February 19, 2025 17:54

dwj300 marked this pull request as ready for review February 19, 2025 20:04

dwj300 requested a review from a team as a code owner February 19, 2025 20:04

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Feb 19, 2025

dwj300 force-pushed the dwj--stale-endpoints branch from 71de47f to 14e3aa6 Compare February 19, 2025 20:08

howardjohn reviewed Feb 19, 2025

View reviewed changes

dwj300 force-pushed the dwj--stale-endpoints branch from 14e3aa6 to 250ea78 Compare February 19, 2025 21:32

howardjohn approved these changes Feb 19, 2025

View reviewed changes

kube/controller: Prevent leaking stale endpoints

c3bfbc0

Fixes istio#54997 Change-Id: I59d90f775d86821060588f446e4d50d77ab97921

dwj300 force-pushed the dwj--stale-endpoints branch from 250ea78 to c3bfbc0 Compare February 20, 2025 00:36

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 20, 2025

istio-testing merged commit 064cba1 into istio:master Feb 20, 2025
28 checks passed

dwj300 mentioned this pull request Feb 20, 2025

[1.23.4] kube/controller: Prevent leaking stale endpoints (#55171) airbnb/istio#8

Merged

dwj300 added cherrypick/release-1.23 Set this label on a PR to auto-merge it to the release-1.23 branch cherrypick/release-1.24 Set this label on a PR to auto-merge it to the release-1.24 branch labels Feb 20, 2025

istio-testing mentioned this pull request Feb 20, 2025

[release-1.23] kube/controller: Prevent leaking stale endpoints #55195

Open

istio-testing mentioned this pull request Feb 20, 2025

[release-1.23] kube/controller: Prevent leaking stale endpoints #55196

Closed

istio-testing mentioned this pull request Feb 20, 2025

[release-1.24] kube/controller: Prevent leaking stale endpoints #55197

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kube/controller: Prevent leaking stale endpoints #55171

kube/controller: Prevent leaking stale endpoints #55171

dwj300 commented Feb 19, 2025 •

edited

Loading

istio-testing commented Feb 19, 2025

istio-policy-bot commented Feb 19, 2025

dwj300 commented Feb 19, 2025

hzxuzhonghu Feb 19, 2025

dwj300 Feb 19, 2025

hzxuzhonghu Feb 20, 2025

hzxuzhonghu Feb 20, 2025 •

edited

Loading

dwj300 Feb 20, 2025

howardjohn left a comment

howardjohn Feb 19, 2025

howardjohn left a comment

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

kube/controller: Prevent leaking stale endpoints #55171

kube/controller: Prevent leaking stale endpoints #55171

Conversation

dwj300 commented Feb 19, 2025 • edited Loading

istio-testing commented Feb 19, 2025

istio-policy-bot commented Feb 19, 2025

dwj300 commented Feb 19, 2025

hzxuzhonghu Feb 19, 2025

Choose a reason for hiding this comment

dwj300 Feb 19, 2025

Choose a reason for hiding this comment

hzxuzhonghu Feb 20, 2025

Choose a reason for hiding this comment

hzxuzhonghu Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

dwj300 Feb 20, 2025

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

howardjohn Feb 19, 2025

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

istio-testing commented Feb 20, 2025

dwj300 commented Feb 19, 2025 •

edited

Loading

hzxuzhonghu Feb 20, 2025 •

edited

Loading