Skip to content

Conversation

ti-mo
Copy link
Contributor

@ti-mo ti-mo commented Jan 15, 2025

This turned out to be a large patch set, but I've narrowed things down as much as I could and sent a few prerequisites in another PR. Please see individual commits for more detailed explanation and reasoning.

At its core, this PR removes ELFVariableSubstitutions and aims to establish a single declaration for each datapath config value, making them all runtime-provided in the process.

Currently, this only addresses endpoint-specific configuration. A follow-up PR will introduce the equivalent for gradually replacing node_config.h with this mechanism by embedding a struct Node into each configuration object.

Fixes: #30893

Configure the datapath using type-safe Go structs generated at compile time

@maintainer-s-little-helper 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 Jan 15, 2025
@ti-mo ti-mo added the release-note/misc This PR makes changes that have no direct user impact. label Jan 15, 2025
@maintainer-s-little-helper 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 Jan 15, 2025
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 15, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from 8153ffd to f742683 Compare January 16, 2025 09:24
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 16, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from f742683 to 49bfbb7 Compare January 16, 2025 15:27
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 16, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch 3 times, most recently from dd77405 to abb5c15 Compare January 21, 2025 15:47
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 21, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from abb5c15 to 095e4bb Compare January 21, 2025 18:41
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 21, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from 095e4bb to 7ebf7a4 Compare January 21, 2025 19:31
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 21, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from 7ebf7a4 to 9f09023 Compare January 21, 2025 20:07
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 21, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from 9f09023 to a5676e6 Compare January 22, 2025 08:16
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 22, 2025

/test

1 similar comment
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 22, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from 2ce46df to c6de9dd Compare January 22, 2025 20:06
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 22, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from c6de9dd to af705fb Compare January 23, 2025 10:33
@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 23, 2025

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented Jan 28, 2025

/test

@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from db4dcfe to 5fce5b5 Compare January 28, 2025 21:22
A follow-up commit will turn this constant into a runtime-provided constant.
Remove it from complexity tests in a separate commit to make the diff a little
easier to grok.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Moved the following variables:

- SECCTX_FROM_IPCACHE: in bpf_host.c instead of conditionally in node_config.h,
  no longer defaults to 1 in some cases
- ETH_HLEN: in bpf_host.c instead of writeStaticData()
- LXC_IP: ep_config.h -> config/endpoint.h, no longer defaults to beef:..
- LXC_IPV4: ep_config.h -> config/endpoint.h, no longer defaults to 1.2.3.4
- LXC_ID: ep_config.h -> config/endpoint.h, no longer defaults to 0x2a
- SECLABEL: ep_config.h -> config/endpoint.h, no longer defaults to 0xfffff
- POLICY_VERDICT_LOG_FILTER: single definition in lib/policy_log.h, no longer defaults to 0xffff
- ENDPOINT_NETNS_COOKIE: ep_config.h -> config/lxc.h, no longer defaults to maxuint32
- THIS_MTU: lib/common.h -> lib/nodeport.h

Signed-off-by: Timo Beckers <timo@isovalent.com>
Moved the following variables:

- THIS_INTERFACE_MAC: node_config.h -> config/global.h
- THIS_INTERFACE_IFINDEX: node_config.h -> config/global.h
- IPV4_MASQUERADE: lib/stubs.h -> lib/nat.h
- IPV6_MASQUERADE: lib/stubs.h -> lib/nat.h
- ROUTER_IP: temporary hack to prevent it from showing up in Go codegen, still rendered into node_config.h by the agent
- HOST_IP: only used in tests, moved out of node_config.h and renamed to NODE_IPV6 to avoid confusion

Signed-off-by: Timo Beckers <timo@isovalent.com>
…figs

This commit makes CollectionOptions.Constants take an annotated struct generated
by dpgen instead of the previous string map. Uses config.StructToMap to convert
the struct to a map to apply to the CollectionSpec before loading.

Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit invokes dpgen during agent build to generate configuration
scaffoldings for all of Cilium's bpf objects.

It also sets up DECLARE_CONFIG to emit config values with the kind:object
decl tag so dpgen can pick them out when generating structs.

Signed-off-by: Timo Beckers <timo@isovalent.com>
This commit removes ELFVariableSubstitutions and finally chops up the
pipeline into separate code paths for tailoring objects to specific
devices and endpoints.

Instead of the typical map[string]any, bpf.LoadAndAssign now takes an
annotated struct of config values to apply to the object, making the
provenance of a value easier to find by playing nice with editor
tooling, as well as being completely type-safe from here on out.

In the process, we figured out exactly which values need to be injected
into which objects to avoid dead code and general surprising behaviour.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Signed-off-by: Timo Beckers <timo@isovalent.com>
A previous commit introduced the ciliumHostRewrites and endpointRewrites functions
to obtain a configuration object to feed into the bpf loader at load time.

With most of the ELF templating logic gone and compile-time values being migrated
to load-time values, the endpoint regeneration logic was no longer responding to
policy changes, specifically due to the security label now being a load-time config.

This commit includes contents of host and workload endpoint rewrites in the endpoint
hash. Since EndpointHash() now blocks when the endpoint write lock is held, its call
had to be moved out of the locked context in runPreCompilationSteps, after the secID
has been updated.

Signed-off-by: Timo Beckers <timo@isovalent.com>
Prior commits made these helpers unused. Remove them.

Signed-off-by: Timo Beckers <timo@isovalent.com>
@ti-mo ti-mo force-pushed the tb/datapath-config-structs branch from b54cceb to b4abbf2 Compare February 19, 2025 12:08
@ti-mo
Copy link
Contributor Author

ti-mo commented Feb 19, 2025

/test

@ti-mo
Copy link
Contributor Author

ti-mo commented Feb 19, 2025

@smagnani96 Checkpatch complains about line continuations in a macro, that's hard to avoid, same for the long line warning, since we have some long strings in there.

The sudo failure was due to #37736.

@ti-mo ti-mo enabled auto-merge February 19, 2025 12:11
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 19, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 19, 2025
@joestringer joestringer requested review from a team and nathanjsweet and removed request for tommyp1ckles and a team February 19, 2025 17:21
@ti-mo ti-mo added this pull request to the merge queue Feb 19, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 19, 2025
Merged via the queue into cilium:main with commit d92791c Feb 20, 2025
69 of 71 checks passed
@ti-mo ti-mo deleted the tb/datapath-config-structs branch February 20, 2025 00:10
return "", fmt.Errorf("hashing host rewrites: %w", err)
}
} else {
cfg, _ := endpointRewrites(epCfg)
Copy link
Member

@joestringer joestringer Mar 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this generate a different hash for each Endpoint on the node, meaning that every hash is unique so we now have to compile the datapath once for every endpoint rather than once for all endpoints?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short answer with recent versions is that we still compile the datapath once for all workload endpoints (+ once for host, as expected). I guess the abstraction has shifted recently. I wonder whether we can just completely remove the EndpointHash(..) logic then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unify datapath configuration infrastructure
10 participants