bpf: Fix monitor aggregation for 'from-network'

[joestringer]

Previously, we did not take into account 'from-network' sources in the
monitor aggregation logic check in send_trace_notify(), which was fine
because we rarely ever sent such events (limited to ipsec for instance).
However, since commit c470e28 we also use this in bpf_host which
suddenly means that any and all traffic from the network will trigger
monitor events, flooding the monitor output.

Fixes: 7a4b0be ("bpf: Add MonitorAggregation option")
Fixes: c470e28 ("Adds TRACE_TO_NETWORK obs label and trace pkts in to-netdev prog.")
Fixes: #12555

---

This is only problematic in v1.8 due to c470e28 , but the bug goes back to Cilium v1.2. May as well backport to all supported branches.

[joestringer]

Ultimately here if you set monitorAggregationLevel to None, you would absolutely get a flood of monitor messages just because any packets reaching the host would be logged. This fixes it by ensuring that monitorAggregationLevel of low or above does not generate events for such packets. We likely still need to look closer at to-network events and the other aspects of the linked issue #12555 (comment) but I believe this will address the core problem where we generated 2.2GB of verbose monitor output in a single test run which triggered a test failure (+CI timeout).

[Weil0ng]

oops, thanks for the fix!

[Weil0ng]

We likely still need to look closer at to-network events

hmm, it seems like there's no datapath aggregation on egress at all?

[joestringer]

hmm, it seems like there's no datapath aggregation on egress at all?

Right, typically we have the most information on egress because we went through the entire datapath already and we've made a decision about how to forward the traffic. For most of the hubble use cases we've looked at so far, only gathering egress monitor events is sufficient for the visibility we need.

Furthermore, on egress we will have already gone through the CT table which allows us to filter monitor events based on the number of flows rather than the number of packets; and additionally we store a timestamp of the last monitor event on egress for that CT entry, and will avoid sending extra events for the flow if we already sent an event recently.

So I guess to-network should actually be fine from the above logic.

EDIT: Well, I assumed that to-network trace calls make use of this same infrastructure but at a glance it seems like maybe this is not the case. So to-network may still be noisy. Filed #12561 to follow up.

[joestringer]

test-me-please

[joestringer]

test-me-please

[christarazi]

Nice find!

[joestringer]

BPF checkpatch is complaining about my use of commit xxxxx in the commit message but at the bottom of the message there's a line that satisfies this so for the future commit log reader, all of the information is there. We can ignore it.

[coveralls]

Coverage increased (+0.002%) to 37.035% when pulling abae0f6 on joestringer:submit/from-network-aggregation into f55ec90 on cilium:master.

[brb]

retest-gke

[pchaigno]

I just finished running this fix in a loop, a hundred times, to validate it fixes the flake we were seeing 🎉

Should we nevertheless reopen #12555 to (1) track and address the memory issue (huge spike in memory consumption seen after failure on both Jenkins and locally) and (2) discuss whether we can and should reduce the verbosity level for that test (currently monitor -vv)?

[joestringer]

@pchaigno I considered this, but I think that the intent will be to move all of this CI monitor framework over to Hubble via #12416 & followups, at which point all of that code should be replaced.