bpf: add metrics for fragmented ipv4 packets

[jibi]

This commit introduces 2 new metrics in the datapath logic related to
fragmented IPv4 packets:

• REASON_FRAG_PACKET: number of received fragmented packets
• REASON_FRAG_PACKET_UPDATE: number of failures in updating the
  IPV4_FRAG_DATAGRAMS_MAP map to register the first logical fragment of
  a datagram

Regarding testing: I did a smoke test by running make build_all and I'm now waiting for e2e tests to finish

[maintainer-s-little-helper]

Commit c4f46f8f434938630d2fc4868c61c1e6da1a84d7 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[jibi]

Including "metrics.h" in lib/ipv4.h broke the tests:

➜  cilium git:(pr/jibi/data-path-add-metrics-for-frag-packets) make -C test/bpf
make: Entering directory '/home/gilberto/go/src/github.com/cilium/cilium/test/bpf'
clang -Wall -Wextra -Werror -Wshadow -Wno-unused-parameter -Wno-address-of-packed-member -Wno-unknown-warning-option -Wno-gnu-variable-sized-type-not-at-end -Wdeclaration-after-statement -I../../bpf/ -I../../bpf/include -I. -D__NR_CPUS__=8 -O2 -I../../bpf/ unit-test.c -o unit-test
In file included from unit-test.c:28:
../../bpf/tests/ipv6_test.h:60:30: error: unexpected type name '__be32': expected identifier
LPM_LOOKUP_FN(lpm4_lookup32, __be32, PREFIX32, dummy_map, match_dummy_prefix)
                             ^
../../bpf/tests/ipv6_test.h:60:38: error: expected identifier
LPM_LOOKUP_FN(lpm4_lookup32, __be32, PREFIX32, dummy_map, match_dummy_prefix)
                                     ^
../../bpf/tests/ipv6_test.h:55:18: note: expanded from macro 'PREFIX32'
#define PREFIX32 32,
                 ^
../../bpf/tests/ipv6_test.h:61:1: error: expected function body after function declarator
LPM_LOOKUP_FN(lpm4_lookup31, __be32, PREFIX31, dummy_map, match_dummy_prefix)
^

(looking into it)

[qmonnet]

I suspect that injecting "lib/metrics.h" (which in turns includes "lib/maps.h") in the inclusion chain somehow prevents LPM_LOOKUP_FN from being defined, because SKIP_UNDEF_LPM_LOOKUP_FN is undefined at that stage. Previously, the header was likely only included from bpf/tests/ipv6_test.h, where the macro is defined.

Includes: "tests/conntrack_test.h" -> "lib/conntrack.h" -> "ipv4.h" -> "metrics.h" ->"maps.h".

Some solutions could be to reorder the includes in test/bpf/unit-test.c to have "tests/ipv6_tests.h" first, or moving the #define SKIP_UNDEF_LPM_LOOKUP_FN to that file, above the includes.

[qmonnet]

Looks good to me, with a few nitpicks and pending the unit test build is fixed :)

[qmonnet]

Oh and you probably want to add a description in pkg/monitor/api/drop.go for the new metrics.

[jibi]

Oh and you probably want to add a description in pkg/monitor/api/drop.go for the new metrics.

There it is :) I looked for that mapping but couldn't find anything by grepping for the constants defined in common.h.

Do you think it makes sense to add a comment in common.h pointing to that file?

[qmonnet]

Do you think it makes sense to add a comment in common.h pointing to that file?

I can't see how that would hurt :) Please go ahead!

[pchaigno]

I think we also need to extend the existing e2e fragment tracking test (K8sServicesTest Checks service across nodes Supports IPv4 fragments) to ensure we don't miss events or count them several times.

[jibi]

I suspect that injecting "lib/metrics.h" (which in turns includes "lib/maps.h") in the inclusion chain somehow prevents LPM_LOOKUP_FN from being defined, because SKIP_UNDEF_LPM_LOOKUP_FN is undefined at that stage. Previously, the header was likely only included from bpf/tests/ipv6_test.h, where the macro is defined.

Includes: "tests/conntrack_test.h" -> "lib/conntrack.h" -> "ipv4.h" -> "metrics.h" ->"maps.h".

Some solutions could be to reorder the includes in test/bpf/unit-test.c to have "tests/ipv6_tests.h" first, or moving the #define SKIP_UNDEF_LPM_LOOKUP_FN to that file, above the includes.

I went for the latter and moved #define SKIP_UNDEF_LPM_LOOKUP_FN in test/bpf/unit-test.c 👍

[jibi]

I should have addressed all the comments, beside e2e testing (looking into now).

For the REASON_FRAG_PACKET metric it should just be a matter of parsing the output of cilium monitor when we send a fragmented packet in test/k8sT/Services.go, making sure metrics are updated accordingly.

The other 2 cases are a bit more tricky as they require to:

• forge an IP fragmented packet which is not the initial one (so we can't just use nc)
• make map_update_elem(&IPV4_FRAG_DATAGRAMS_MAP, ..) fail (not sure when that can happen)

[pchaigno]

For the REASON_FRAG_PACKET metric it should just be a matter of parsing the output of cilium monitor when we send a fragmented packet in test/k8sT/Services.go, making sure metrics are updated accordingly.

I think you want cilium metrics list. It's likely that some existing tests already test metrics, so worth checking.

[pchaigno]

test-me-please

[qmonnet]

• forge an IP fragmented packet which is not the initial one (so we can't just use nc)

Surely there are libraries for creating packets in go? Otherwise, stick to nc but add some egress rule on the source pod to drop the first fragment by targetting the UDP port?

• make map_update_elem(&IPV4_FRAG_DATAGRAMS_MAP, ..) fail (not sure when that can happen)

We can't destroy the map since the program uses it, we can't freeze it after it's been created. We use the BPF_ANY flag so we can't play on that without modifying the eBPF program, same thing for using a wrong size for the keys and values. We can't block on a full map, it's a LRU. We can configure its size but we set a lower limit, so we can't set it to zero. 🤔 I can't find a way to have this update fail, besides running out of kernel memory. Not sure if there's a good way to test this one.

[jibi]

test-me-please

[pchaigno]

On a positive note, this pull request seems to fix #13931 in the dev. VM 🎉

[jibi]

(@brb I re-requested your review as for some reason the approval for cilium/agent is still missing 🤔)

[pchaigno]

I backported the four commits related to relax_verifier() to v1.8 to fix a complexity issue (cf. #15440):

datapath/probes: add support for misc features
bpf: reintroduce relax_verifier()
bpf: optimize relax_verifier()
bpf: fix complexity issue on kernels <5.3 

I'm not adding backport labels considering the metric introduced in this PR wasn't backported.