policy: return ICMP "Destination unreachable" on ipv4 ingress policy denials by 41ks · Pull Request #44184 · cilium/cilium

41ks · 2026-02-04T12:43:15Z

Description

This is mostly a copy of PR #42068 following the changes introduced in PR #43226. See #41859 for the full list of TODOs.

This PR adds the ICMP response functionality to ingress policy denials.
The functionality works for local and remote endpoints with/without tunneling and with/without eBPF Host Routing.

Testing

Created a new unit test
Performed integration tests: backported the PR to Cilium 1.18.6, then created a pod with an ingress policy denying all traffic. Then tried to reach this pod from multiple different places and confirmed ping was returning "Packet filtered":

# ping 10.130.112.4
 PING 10.130.112.4 (10.130.112.4) 56(84) bytes of data.
 From 10.130.112.4 icmp_seq=1 Packet filtered
 From 10.130.112.4 icmp_seq=2 Packet filtered
 [...]

Local pod from same node [legacy routing]
Pod from a different node in the same cluster [legacy routing]
Pod in a different cluster [legacy routing]
Local pod from same node [eBPF host routing]
Pod from a different node in the same cluster [eBPF host routing]
Pod in a different cluster [eBPF host routing]

policy: add option to return ICMP "Destination unreachable" on ipv4 ingress policy denials

julianwiedmann · 2026-02-05T08:04:17Z

👋 please rebase on-top of #44186

ldelossa · 2026-02-05T16:02:47Z

@41ks it appears the initial container build on CI failed. This doesn't seem to be an issue with this PR. however, the simplest thing to re-kick all the failed tests if so perform an amend and a force push.

qmonnet

I'm only reviewing for the doc update - the change looks good, thanks!

bpf/bpf_lxc.c

This expands the functionality enabled by --policy-deny-response=icmp from egress-only to egress and ingress. Now ICMP responses can be sent for ingress policy denials as well. Signed-off-by: Alex Melhem <alex.melhem@datadoghq.com>

julianwiedmann

Thank you for tackling this! I see that the change currently only has your sign-off - please check with @antonipp how you can accurately attribute his contributions.

I left some initial comments inline. Once we have more test coverage, it's easier to evaluate the proposed change.

bpf/lib/common.h

bpf/bpf_lxc.c

julianwiedmann · 2026-02-09T12:29:00Z

bpf/bpf_lxc.c

+			if (!IS_ERR(ret)) {
+				update_metrics(ctx_full_len(ctx), metric_dir,
+					       __DROP_REASON(verdict));
+				return ret;


Instead of an if-else construct, you could just fall through here.

Let me know if the new implementation looks better

bpf/tests/tc_policy_reject_response_test.c

This applies PR suggestions and refactors icmp deny response test suit to add more test cases for the ingress path. Signed-off-by: Alex Melhem <alex.melhem@datadoghq.com>

41ks · 2026-02-10T13:39:45Z

Hey @julianwiedmann I applied your suggestions in the second commit. I also revamped the test suite to make it more complete. Let me know what you think and if I have correctly applied your suggestions.

pchaigno · 2026-02-11T14:50:49Z

/test

julianwiedmann · 2026-02-13T14:05:45Z

bpf/bpf_lxc.c

+
+		if (ret == CTX_ACT_OK) {
+#if !defined(ENABLE_HOST_ROUTING) && !defined(ENABLE_ROUTING)
+			/* If the shared helper returned CTX_ACT_OK (pass to stack) but


As you can imagine, this is the really tricky part. I don't think it's robust to assume that we have one specific scenario (!defined(ENABLE_HOST_ROUTING) && !defined(ENABLE_ROUTING)) that we should special-case - we always need to be ready to handle a CTX_ACT_OK.

And it's gets more interesting - the pod ingress path can be entered in two ways:

as to-container on the pod's TC hook (I believe this is what your tests currently exercise). Any returned CTX_ACT_OK would then result in the ICMP packet accidentally slipping into the original destination pod.

in cil_lxc_policy(), when another BPF program calls tail_call_policy() to push a packet straight to the destination. Not handling the CTX_ACT_OK would mean that the ICMP packet then continues in the initial program's context. We need to be careful here, I think we might need to bring some of the epilogue logic in cil_lxc_policy() over into our new tailcall program.

In the end CTX_ACT_OK should always mean "send to host stack". Maybe we just need to be explicit about that, and redirect the packet to the cilium_host veth pair ... ?

We should also think about tests for (2.). There's prior art for how to correctly enter this path, if you look for local_delivery_fill_meta() in bpf/tests.

joestringer · 2026-03-04T16:41:12Z

I've marked this PR as draft. When you have addressed the comments and workflow test results, bring the PR back to the attention of reviewers by scrolling to the bottom of the page and clicking the "Ready for Review" button. Thanks!

github-actions · 2026-04-04T02:13:54Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

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 Feb 4, 2026

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Feb 4, 2026

41ks force-pushed the alex.melhem/ingress-deny-icmp branch from eeeff75 to 212d3fd Compare February 4, 2026 13:47

41ks marked this pull request as ready for review February 4, 2026 14:18

41ks requested review from a team as code owners February 4, 2026 14:18

41ks requested review from fristonio, jrife and qmonnet February 4, 2026 14:18

julianwiedmann self-requested a review February 5, 2026 08:04

41ks force-pushed the alex.melhem/ingress-deny-icmp branch from 212d3fd to 39a2a7a Compare February 5, 2026 09:38

41ks force-pushed the alex.melhem/ingress-deny-icmp branch from 39a2a7a to 412ff7c Compare February 5, 2026 16:47

qmonnet approved these changes Feb 6, 2026

View reviewed changes

qmonnet added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Feb 6, 2026

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 Feb 6, 2026

qmonnet added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Feb 6, 2026

Andreagit97 reviewed Feb 8, 2026

View reviewed changes

bpf/bpf_lxc.c Show resolved Hide resolved

41ks force-pushed the alex.melhem/ingress-deny-icmp branch from 412ff7c to dcea67d Compare February 9, 2026 09:23

julianwiedmann reviewed Feb 9, 2026

View reviewed changes

Applying suggestions and improving test suite

5d0fb20

This applies PR suggestions and refactors icmp deny response test suit to add more test cases for the ingress path. Signed-off-by: Alex Melhem <alex.melhem@datadoghq.com>

41ks force-pushed the alex.melhem/ingress-deny-icmp branch from f91cf23 to 5d0fb20 Compare February 10, 2026 13:31

antonipp mentioned this pull request Feb 11, 2026

policy: return ICMP "Destination unreachable" on ipv4 ingress policy denials #42068

Closed

6 tasks

julianwiedmann self-requested a review February 11, 2026 08:07

julianwiedmann reviewed Feb 13, 2026

View reviewed changes

jrife removed their request for review February 16, 2026 21:45

joestringer marked this pull request as draft March 4, 2026 16:41

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 4, 2026

Conversation

41ks commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

julianwiedmann commented Feb 5, 2026

Uh oh!

ldelossa commented Feb 5, 2026

Uh oh!

qmonnet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julianwiedmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julianwiedmann Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

41ks Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

41ks commented Feb 10, 2026

Uh oh!

pchaigno commented Feb 11, 2026

Uh oh!

julianwiedmann Feb 13, 2026

Choose a reason for hiding this comment

Uh oh!

joestringer commented Mar 4, 2026

Uh oh!

github-actions bot commented Apr 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

41ks commented Feb 4, 2026 •

edited

Loading