Support bootstrapping CNI configs from a template file - #2126
Conversation
…ig test Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
This couples the content to its type, and eliminates quite a few extra fields from the test tables. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
The check if the config needs to be parsed as conflist or as conf can be done once during struct initialization. The struct can then simply check if the confList field is non-nil. That's an implementation detail to support (un-)marshalling and shouldn't be relevant to struct users. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
The tests were asserting the error messages only, so no need to wrap them into errors. Also, remove the messages completely from those table tests in which they were nil for every contained test case. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
The different test cases assert on an expected set of subnets, but all used their own variants of pulling those subnets out of the CNINetworkConfig struct. Consolidate this by replacing those assertions with a one that checks for the correctness of the keys returned by getPodCIDRsMapFromCNISpec. That is much more compact, and has the same outcome. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
Those would've caused a panic later on. Instead of looking up the bridge plugin config from the conflist on the fly, do it when the struct is initialized, and store the pointer in the conf field. This allows for the removal of getBridgePlugin. Also harden the config parsing against some null JSON fields. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
A CNI configuration file that's copied into the CNI configuration directory by an init container is already visible to the container runtime before kube-router has completed it, so on a freshly booted node, pod sandbox creation may transiently fail with errors like "no IP ranges specified" until kube-router has filled in the missing parts. To avoid this, introduce CNI configuration bootstrapping via a template file. If the KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE environment variable is not empty, kube-router reads the config from the template instead, and writes the completed config to the usual CNI config file path, which doesn't need to exist beforehand. The template file's content is interpreted according to the CNI config file name. The template's own file name has no significance. As a result, the container runtime won't pick up an incomplete configuration and try to set up pod networking too early. Since the config is re-derived from the pristine template on every start, the written config is wholly owned by kube-router. Signed-off-by: Tom Wieczorek <twieczorek@mirantis.com>
|
| Filename | Overview |
|---|---|
| pkg/utils/cni.go | Adds separate template input and destination paths, validates conflist bridge selection, and preserves the selected configuration when writing. |
| pkg/controllers/routing/network_routes_controller.go | Wires the template environment variable into controller initialization and permits a previously absent destination file when a template is configured. |
| pkg/utils/cni_test.go | Refactors CNI fixtures and extends coverage for templates, null conflists, null plugins, missing bridge plugins, and destination writes. |
| daemonset/generic-kuberouter-cni-conf-template.yaml | Adds an example deployment that mounts an incomplete CNI template outside the runtime-watched configuration directory. |
| docs/user-guide.md | Documents CNI configuration management, template semantics, and the relationship between destination filenames and configuration-list parsing. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[KUBE_ROUTER_CNI_CONF_TEMPLATE_FILE configured?] -->|Yes| B[Read pristine template]
A -->|No| C[Read existing CNI configuration]
B --> D[Interpret using destination filename]
C --> D
D --> E[Select bridge plugin]
E --> F[Insert node pod CIDRs and MTU]
F --> G[Set up kube-bridge]
G --> H[Write completed CNI configuration]
H --> I[Container runtime discovers usable config]
Reviews (1): Last reviewed commit: "feat: Support bootstrapping CNI configs ..." | Re-trigger Greptile
catherinetcai
left a comment
catherinetcai
left a comment
There was a problem hiding this comment.
Great work @twz123 - just some minor, surface level comments.
What do you think about having this templating behavior be the user-facing default moving forward? I'm not sure if I see the benefit of continuing to push the legacy/racy behavior, so I think it's worth just updating all of the examples under the daemonsets folder to use the template instead and calling it out in the release notes.
| kube-router's own contribution is the routing of pod traffic between nodes and the management of the plugins' | ||
| configuration file, filling in the parts that are node-specific and only known at runtime. The latter is what | ||
| `--enable-cni` controls, which is enabled by default. Disable it to use kube-router's features alongside another CNI | ||
| provider. On startup, kube-router |
There was a problem hiding this comment.
I think "On startup..." is a good spot to place a line break for readability.
I think adding a colon at the end helps visually for formatting with the bullet points right below.
|
|
||
| - inserts the node's pod CIDRs into the `host-local` IPAM ranges of the `bridge` plugin, as allocated to the node by | ||
| the kube-controller-manager, or as set via the `kube-router.io/pod-cidrs` annotation | ||
| - and sets the `bridge` plugin's MTU, if `--auto-mtu` is enabled (see [MTU](#mtu)). |
There was a problem hiding this comment.
Nit: I would drop the starting "and" for this second point and drop the ending period since this is a bulleted list and not meant to be read like complete sentences.
| content: getConfListWithIPv6DuplicateRanges(), | ||
| err: nil, | ||
| ranges: []string{"10.242.0.0/24", "10.242.1.0/24", "10.242.2.0/24", "2001:db8:42:2::/64"}, | ||
| name: "Ensure conflist subnets get de-deduplicated with ranges when repeats exist", |
There was a problem hiding this comment.
I think this test name is missing calling out IPv6
| isConfList: false, | ||
| content: getConfWithNoType(), | ||
| err: errors.New("error load CNI config, file appears to have no type: "), | ||
| name: "Ensure conf subnet get consolidated into ranges when only subnet exists", |
There was a problem hiding this comment.
I think the args for this test are the same as the "Attempt reading from conf" test:
{
name: "Attempt reading from conf",
content: getConf(),
ranges: []string{"10.242.0.0/24"},
},| isConfList: true, | ||
| content: getConfListWithNoPlugins(), | ||
| err: errors.New("CNI config list "), | ||
| name: "Ensure conflist subnet get consolidated into ranges when only subnet exists", |
There was a problem hiding this comment.
Seems to be the same args as a previous test:
{
name: "Attempt reading from conflist",
content: getConfList(),
ranges: []string{"10.242.0.0/24"},
},| } | ||
|
|
||
| func NewCNINetworkConfig(cniConfFilePath string) (*CNINetworkConfig, error) { | ||
| return NewCNINetworkConfigFromTemplate(cniConfFilePath, "") |
There was a problem hiding this comment.
Is this calling down to NewCNINtworkConfigFromTemplate just to try and not break the API? I think it's okay to just update the API for NewCNINetworkConfig to accept cniConfTemplateFilePath.
I think this entire utils package really belongs in internal (and we should do that refactor at some point...)
2 participants
What type of PR is this?
feature
What this PR does / why we need it:
A CNI configuration file that's copied into the CNI configuration directory by an init container is already visible to the container runtime before kube-router has completed it, so on a freshly booted node, pod sandbox creation may transiently fail with errors like "no IP ranges specified" until kube-router has filled in the missing parts.
To avoid this, introduce CNI configuration bootstrapping via a template file. If the
KUBE_ROUTER_CNI_CONF_TEMPLATE_FILEenvironment variable is not empty, kube-router reads the config from the template instead, and writes the completed config to the usual CNI config file path, which doesn't need to exist beforehand. The template file's content is interpreted according to the CNI config file name. The template's own file name has no significance.As a result, the container runtime won't pick up an incomplete configuration and try to set up pod networking too early. Since the config is re-derived from the pristine template on every start, the written config is wholly owned by kube-router.
Write this up in the user guide and add another example DaemonSet.
The PR can be reviewed commit by commit:
Which issue(s) this PR is related to:
The problem itself should dissolve as soon as the correct CNI config landed on disk. However, this is very reliably triggering
containernetworking/plugins#1265, especially in air-gapped scenarios when there's no images to be pulled and the container runtime can start pods right away.
Was AI used during the creation of this PR?
Yes, for design discussions, patch series review, and the initial draft of the changes to the user guide.
What, if any, amount of integration testing was done with this change in a Kubernetes environment?
Unit tests so far. I plan to give this a spin in the k0s CI, of course.
Does this PR introduce a breaking change?
No.
Anything else the reviewer should know that wasn't already covered?
I deliberately didn't try to do some write-temprary-file-then-rename dance to make the config file appear atomically. The time it takes to write the file is negligible, probably microseconds in practice. Getting this atomic thing right, along with all its edge cases, warrants its own library. (And, no, I don't think renameio gets this right, either.)