cilium / cilium

eBPF-based Networking, Security, and Observability
https://cilium.io
Apache License 2.0
20.14k stars 2.96k forks source link

Remove dynamic map names in Cilium agent #34706

Open ti-mo opened 1 month ago

ti-mo commented 1 month ago

This is a major prerequisite for https://github.com/cilium/cilium/issues/29216 and will allow us to pre-build an ELF containing all of the agent's map definitions.

This can then be used to pull map BTF out of at runtime, so agent-managed maps can have value pretty-printing and use new BPF features that require map BTF. (notably spinlocks and timers)

Also contributes towards https://github.com/cilium/cilium/issues/30894.


Many of Cilium's maps have a definition like this:

#ifdef CONFIG_MAP
struct {
    __uint(type, BPF_MAP_TYPE_ARRAY);
    __type(key, __u32);
    __type(value, __u64);
    __uint(pinning, LIBBPF_PIN_BY_NAME);
    __uint(max_entries, CONFIG_MAP_SIZE);
} CONFIG_MAP __section_maps_btf;
#endif

and are accessed like this:

time = map_lookup_elem(&CONFIG_MAP, &index);

and require matching agent code like this:

cDefinesMap["CONFIG_MAP"] = configmap.MapName
cDefinesMap["CONFIG_MAP_SIZE"] = fmt.Sprintf("%d", configmap.MaxEntries)

This issue is for making the names of all map definitions static, removing the cDefinesMap entries, removing the xyz.MapName variables and fixing up bpf C call sites with direct references. Bonus points for inlining maxentries if those cannot change (e.g. `configmap.MaxEntries is a const...).

joestringer commented 1 month ago

I assume that this is not considering policymaps, since those are currently per-endpoint?

ti-mo commented 1 month ago

@joestringer I glossed over that in the description, but yes, this needs to be done for per-endpoint maps as well. We're no longer bound by the ELF rewrite hack, so we don't need the cilium_calls_65535 templating mechanism anymore.

In C code, the symbol can simply be named cilium_calls, and the loader needs to alter CollectionSpec.Maps["cilium_calls"].Name = "cilium_calls_00123" for specific endpoints. This behaviour is already in place today, albeit cilium_calls_65535 -> cilium_calls_00123 instead of cilium_calls -> cilium_calls_00123 in the future.

joestringer commented 1 month ago

The proposal makes sense to me. I think the only danger is it could feel a bit magic in that all code references will refer to a single map name / reference but in practice the per-endpoint ones will be distinct instances, but I'm sure we'll figure out a reasonable representation of these maps.

ti-mo commented 1 month ago

You're describing the status quo. Everything refers to CALLS_MAP and one needs to jump through multiple layers of indirection to find the real map name.