Fix fromCIDR policy on kernels 4.10 or older and extend test coverage #11333
Conversation
Make use of Context to share more test preparation for the CIDR tests, and allow the toCIDR / fromCIDR tests to be split out to validate them separately. Signed-off-by: Joe Stringer <joe@cilium.io>
This docker network will be used by upcoming CIDR policy tests, so share it in the common helpers package. Signed-off-by: Joe Stringer <joe@cilium.io>
Previously, we only tested that CIDR policy does not unintentionally open up connectivity to containers that reside within the CIDR range. This test now actually validates that the "fromCIDR" policy applies to traffic from outside Cilium's control, assuming it resides within the IP range allowed by the policy. Signed-off-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
|
Please set the appropriate release note label. |
joestringer
added
the
release-note/bug
joestringer
added
the
sig/datapath
|
test-me-please |
There was a problem hiding this comment.
BPF changes LGTM. I suggest someone else reviews my CI changes in this PR :-)
@will would you mind renaming the PR to something like "Fix fromCIDR policy on kernels 4.10 or older and extend test coverage"? This will be useful for generating the release notes to clearly highlight which release gets this fix.
Sorry, it doesn't look like I have permissions to change the name of the PR. |
willdeuschle
changed the title
|
Updated, looks like the test failure was an etcd flake |
|
@will ha! My apologies, my not-so-nimble fingers intended that for @willdeuschle ;-) |
|
@willdeuschle I wouldn't worry too much about the GKE CI run right now, it's not marked required and there are known flakes on that job at the moment. I think we just need CI review. |
maintainer-s-little-helper
bot
added
the
ready-to-merge
pchaigno
removed
the
ready-to-merge
|
It looks like the bug was introduced in 1.2 so we would at least need to backport to 1.7. Do we also want to backport to previous releases? As far as I understand the impact of the bug is unexpected packet drops, right? |
|
I'm not sure whether the tests will backport cleanly as they might be relying on other refactoring that occurred during the v1.8 dev cycle. I don't mind taking on the backport of the functional commit only to v1.7. I'd probably avoid backporting further just given that we have no user reports of this issue. |
willdeuschle
force-pushed
the
wd/resubmit-from-cidr-runtime-tests
branch
from
685c4b4
to
69258bb
Compare
Signed-off-by: Will Deuschle <wdeuschle@palantir.com>
willdeuschle
force-pushed
the
wd/resubmit-from-cidr-runtime-tests
branch
from
69258bb
to
ddda0bb
Compare
|
test-me-please |
maintainer-s-little-helper
bot
added
the
ready-to-merge
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.7
in 1.7.4
maintainer-s-little-helper
bot
moved this from Backport pending to v1.7
to Backport done to v1.7
in 1.7.4
maintainer-s-little-helper
bot
moved this from Backport pending to v1.7
to Backport done to v1.7
in 1.7.4
Builds on the draft PR originally opened by @joestringer #10580
That PR body:
This series extends the runtime CI tests to validate that fromCIDR policy works correctly using a local docker network, similar to how existing FQDN tests do.
Review patch-by-patch, there is some large refactoring of the CIDR tests in policy but it's mostly kept to one non-functional patch.
On top of that, see the final commit I added. The tests were failing in CI (and for me locally) because for kernel versions without LPM maps, ipcache_lookup4 is unable to lookup CIDRs based on a prefix other than a /32. Changing from ipcache_lookup4 to lookup_ip4_remote_endpoint fixes this because the latter function uses unrolled hash map lookups to simulate a LPM map lookups for kernels that don't support LPM maps. See bpf/lib/eps.h for further details on that.
It may be desirable to replace usage of ipcache_lookup[46] everywhere with lookup_ip[46]_remote_endpoint but that is beyond the scope of this PR.