Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

socket-LB: Terminate pod netns connections to deleted service backends #33459

Merged
merged 5 commits into from
Jul 16, 2024
Merged

socket-LB: Terminate pod netns connections to deleted service backends #33459

merged 5 commits into from
Jul 16, 2024
+103 −14

Conversation

aditighag
Copy link
Member

@aditighag aditighag commented Jun 28, 2024 •
edited
Loading

Extension of #25169 to terminate connections to deleted service backends in pod network namespaces.

Testing:

Manually tested with starting a pair of UDP server and pod applications, and deleted the service backend.

$ k get pods -o wide
NAME          READY   STATUS    RESTARTS   AGE    IP             NODE          NOMINATED NODE   READINESS GATES
client        1/1     Running   0          163m   10.244.1.228   kind-worker   <none>           <none>

$ kubectl exec -it server -- iperf3 -s -p  5010
-----------------------------------------------------------
Server listening on 5010 (test #1)
-----------------------------------------------------------
Accepted connection from 10.244.1.228, port 42620
[  5] local 10.244.1.204 port 5010 connected to 10.244.1.228 port 35150


time="2024-06-28T20:53:54Z" level=debug msg="handling udp connections to deleted backend 10.244.1.204:5010" subsys=service
time="2024-06-28T20:53:54Z" level=info msg="socket {35150 5010 10.244.1.228 10.244.1.204 0 [297562409 0]}" subsys=datapath-sockets
time="2024-06-28T20:53:54Z" level=debug msg="Destroyed socket: {35150 5010 10.244.1.228 10.244.1.204 0 [297562409 0]}" subsys=datapath-sockets
time="2024-06-28T20:53:54Z" level=info msg="Forcefully terminated sockets" errors="<nil>" failed=0 filter="{10.244.1.204 5010 2 17 0x4584220}" subsys=datapath-sockets success=1

Fixes: #27300

Forcefully terminate stale connections in pod network namespaces that are connected to deleted service backends when socket-lb is enabled, and allow pod applications to re-connect to active backends.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 28, 2024
@aditighag aditighag added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 28, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jun 28, 2024
@aditighag
Copy link
Member Author

/test

1 similar comment
@aditighag
Copy link
Member Author

/test

@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 959acde to a5cc4a5 Compare June 28, 2024 21:00
@aditighag
Copy link
Member Author

/test

@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from a5cc4a5 to 2901d27 Compare June 28, 2024 23:42
@aditighag
Copy link
Member Author

/test

@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 2901d27 to 5735d56 Compare July 8, 2024 22:46
@aditighag aditighag added area/loadbalancing Impacts load-balancing and Kubernetes service implementations needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch labels Jul 9, 2024
@aditighag
Copy link
Member Author

/test

@aditighag aditighag added the release-blocker/1.16 This issue will prevent the release of the next version of Cilium. label Jul 10, 2024
@joestringer joestringer added this to the 1.16 milestone Jul 10, 2024
@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 5735d56 to 4c22991 Compare July 11, 2024 00:39
@aditighag
Copy link
Member Author

/test

@aditighag aditighag requested a review from ysksuzuki July 11, 2024 00:46
@aditighag aditighag marked this pull request as ready for review July 11, 2024 00:48
@aditighag aditighag requested review from a team as code owners July 11, 2024 00:48
@julianwiedmann julianwiedmann added the feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. label Jul 11, 2024
@aditighag aditighag requested a review from ysksuzuki July 11, 2024 22:21
@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 5cda92b to 492958f Compare July 11, 2024 22:29
@aditighag
Copy link
Member Author

Fixed up rebase.

@aditighag
Copy link
Member Author

/test

ysksuzuki
ysksuzuki approved these changes Jul 12, 2024
View reviewed changes
Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ldelossa
ldelossa approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. So w/r/t #25169 we took an approach where we don't necessary join the host mount ns, but just mount in the namespace dir stead?

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jul 15, 2024
@joestringer joestringer added the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Jul 15, 2024
@joestringer
Copy link
Member

I see there's open discussion threads, feel free to drop the label when those are resolved.

@aditighag
Copy link
Member Author

aditighag commented Jul 15, 2024 •
edited
Loading

LGTM. So w/r/t #25169 we took an approach where we don't necessary join the host mount ns, but just mount in the namespace dir stead?

#25169 resolved the issue in the root netns. This PR resolves the issue for pod network namespaces by mounting the netns dir so that the agent can terminate stale sockets by entering pod network namespaces.

@joestringer joestringer removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Jul 16, 2024
@joestringer
Copy link
Member

There's still two threads that should be resolved in GitHub UI before we merge, but it seems tentatively like the content of those threads is resolved. I'm fine with following up with a commit to document the capabilities dependency in the YAML after this PR.

@aditighag
config: Add flag for socket LB pod connections termination
109e7bb 
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
install: Mount /var/run/netns
f86ab72 
This is required to be able to exec into pod
network namespaces by opening `/var/run/netns/<pod-netns>`.
The current use case is to be able to terminate stale
pod connections when socket-Lb is enabled.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
pkg/service: Terminate stale pod netns connections
172128d 
Extension of PR#25169 [1] to terminate pod netns connections
to deleted service backends in pod network namespaces.

[1] cilium#25169

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag
docs: Update socket-LB connection termination note
766a9d8 
Update the section as the agent now terminates stale
connections in pod network namespaces as well.

Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 492958f to 80295a9 Compare July 16, 2024 14:44
@aditighag
install: Document requirements for SYS_ADMIN
f677fba 
Switching into non-root network namespaces requires
SYS_ADMIN permissions. The health endpoint infrastructure
has had this dependency, socket-LB will have a similar
requirement.

Suggested-by: Daniel Borkmann <daniel@iogearbox.net>
Suggested-by: Sebastian Wicki <sebastian@isovalent.com>
Signed-off-by: Aditi Ghag <aditi@cilium.io>
@aditighag aditighag force-pushed the pr/aditighag/socket-termination-pod-netns branch from 80295a9 to f677fba Compare July 16, 2024 15:02
@aditighag
Copy link
Member Author

/test

@joestringer joestringer added this pull request to the merge queue Jul 16, 2024
Merged via the queue into cilium:main with commit cd8f76e Jul 16, 2024
66 checks passed
This was referenced Jul 16, 2024
Closed
Open
@aditighag aditighag deleted the pr/aditighag/socket-termination-pod-netns branch July 16, 2024 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro approved these changes

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer left review comments

@chancez chancez chancez left review comments

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@ldelossa ldelossa ldelossa approved these changes

@learnitall learnitall learnitall approved these changes

Assignees

@aditighag aditighag

Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. needs-backport/1.16 This PR / issue needs backporting to the v1.16 branch ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.16 This issue will prevent the release of the next version of Cilium. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
Release blockers
Status: Done
Milestone
1.16
Development

Successfully merging this pull request may close these issues.

NGINX always connects to same coredns endpoint, even if it no longer exists
10 participants
@aditighag @joestringer @gandro @nathanjsweet @borkmann @chancez @ysksuzuki @ldelossa @learnitall @julianwiedmann