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.

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

Fix behavior where packets leave node if there are no backends (v2) #22388

Merged
merged 3 commits into from
Nov 30, 2022

Conversation

julianwiedmann
Copy link
Member

@julianwiedmann julianwiedmann commented Nov 28, 2022

This is a re-spin of #21539. Now with the complexity improvement from #22342 and passing the problematic test on 4.19.

Quoting from the original PR:

This PR resolves an issue where an outgoing packet destined for a service will not be dropped if it does not have any backends. Currently we will not return the service if there are no backends for it, causing

cilium/bpf/bpf_lxc.c

Lines 1254 to 1266 in 58e4081

if (svc) {
#if defined(ENABLE_L7_LB)
if (lb4_svc_is_l7loadbalancer(svc)) {
proxy_port = (__u16)svc->l7_lb_proxy_port;
goto skip_service_lookup;
}
#endif /* ENABLE_L7_LB */
ret = lb4_local(get_ct_map4(&tuple), ctx, ETH_HLEN, l4_off,
&csum_off, &key, &tuple, svc, &ct_state_new,
ip4->saddr, has_l4_header, false);
if (IS_ERR(ret))
return ret;
}

To never be run, meaning we will never drop a packet in this case and instead simply route it through the kernels default routes.

Fixes: #21453

Traffic addressed to a service IP is dropped, if no backend is available.

michaelasp and others added 3 commits November 28, 2022 12:14
Resolve an issue where an outgoing packet destined for a service will not
be dropped if it does not have any backends.

Currently we will not return the service if there are no backends for it,
meaning we will never drop a packet in this case and instead simply route
it through the kernels default routes.

Fixes: cilium#21453
Signed-off-by: Michael Aspinwall <maspinwall@google.com>
[jwi: wordsmith the patch description]
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Packets that are adressed to a VIP without any backend should be dropped.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Packets that are adressed to a VIP without any backend should be dropped.

As the VIP doesn't get translated, this currently works "by accident" if no
matching allow-policy for the VIP is installed. But we actually want to
happen this indepedently of policy, with a proper drop reason.

Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
@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 Nov 28, 2022
@julianwiedmann
Copy link
Member Author

/test

@julianwiedmann julianwiedmann changed the title Svc no backend Fix behavior where packets leave node if there are no backends (v2) Nov 28, 2022
@julianwiedmann julianwiedmann added backport/author The backport will be carried out by the author of the PR. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Nov 28, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 28, 2022
@julianwiedmann julianwiedmann marked this pull request as ready for review November 28, 2022 12:57
@julianwiedmann julianwiedmann requested a review from a team as a code owner November 28, 2022 12:57
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.12.5 Nov 28, 2022
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

@julianwiedmann Looks great! Great job in appeasing the verifier. 🚀 The unit tests look good too.
@michaelasp Did you test the changes manually with policy? The behavior should be same as policy enforcement happens after svc xlate, but it'll be good double check.

@michaelasp
Copy link
Contributor

@julianwiedmann Looks great! Great job in appeasing the verifier. 🚀 The unit tests look good too. @michaelasp Did you test the changes manually with policy? The behavior should be same as policy enforcement happens after svc xlate, but it'll be good double check.

Hey Aditi! I think I only manually verified this without network policy with tcpdump, so it might be a good idea to check, even though I don't have reason to believe that it would impact anything there.

@julianwiedmann
Copy link
Member Author

/test-1.24-5.4

@julianwiedmann
Copy link
Member Author

/test-1.25-4.19

@julianwiedmann
Copy link
Member Author

/test-1.26-net-next

@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 Nov 30, 2022
@jrajahalme jrajahalme merged commit 920d898 into cilium:master Nov 30, 2022
@bimmlerd bimmlerd mentioned this pull request Dec 1, 2022
14 tasks
@borkmann
Copy link
Member

borkmann commented Dec 1, 2022

@michaelasp thanks for the PR. Just follow-up question, did you A/B test behavior against k8s / kube-proxy whether it's exactly in line what kube-proxy does if there's no backend on a node for a given svc?

@julianwiedmann julianwiedmann deleted the svc-no-backend branch December 6, 2022 09:12
@joestringer joestringer added this to Needs backport from master in 1.12.6 Dec 15, 2022
@joestringer joestringer removed this from Needs backport from master in 1.12.5 Dec 15, 2022
@michi-covalent michi-covalent added this to Needs backport from master in 1.12.7 Jan 26, 2023
@michi-covalent michi-covalent removed this from Needs backport from master in 1.12.6 Jan 26, 2023
@qmonnet
Copy link
Member

qmonnet commented Jan 31, 2023

Looks like the backport was handled in #23034.

@qmonnet qmonnet added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed needs-backport/1.12 labels Jan 31, 2023
@joestringer joestringer moved this from Needs backport from master to Backport done to v1.12 in 1.12.7 Feb 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.12.7
Backport done to v1.12
Development

Successfully merging this pull request may close these issues.

Packet will leave node if destination is a service without backends
6 participants