Safe subdirectory based CNI chain configuration loading #1419
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
dougbtv
force-pushed
the
rebase5-cni-subdirectory-chain
branch
from
fa49747 to
d7a450c
Compare
| @@ -258,16 +258,25 @@ func confDel(rt *libcni.RuntimeConf, rawNetconf []byte, multusNetconf *types.Net | |||
| return err | |||
| } | |||
|
|
|||
| func conflistAdd(rt *libcni.RuntimeConf, rawnetconflist []byte, multusNetconf *types.NetConf, exec invoke.Exec) (cnitypes.Result, error) { | |||
| func conflistAdd(rt *libcni.RuntimeConf, rawnetconflist []byte, cniConfList *libcni.NetworkConfigList, multusNetconf *types.NetConf, exec invoke.Exec) (cnitypes.Result, error) { | |||
There was a problem hiding this comment.
TODO: Check that this works without conflists
There was a problem hiding this comment.
So, I think it works as intended without conflists... If you don't have a conflist to begin with -- you can't chain on it.
But in that case, you can use AuxiliaryCNIChainName and have that as a place to chain instead.
pkg/types/conf.go
Outdated
| return delegateConf, nil | ||
| } | ||
|
|
||
| // !bang |
dougbtv
force-pushed
the
rebase5-cni-subdirectory-chain
branch
2 times, most recently
from
31b08b3 to
591f6d1
Compare
dougbtv
force-pushed
the
rebase5-cni-subdirectory-chain
branch
9 times, most recently
from
085c8a7 to
f41b35a
Compare
docs/thick-plugin.md
Outdated
| @@ -72,6 +72,7 @@ is provided. | |||
| - `"logLevel"`: the logging level for the multus daemon logs. | |||
| - `"logToStderr"`: enable this to have the daemon multus logs echoed to stderr | |||
| as well. By default, it is disabled. | |||
| - `"auxiliaryCNIChainName"`: set a value to executed chained cni configurations from disk in an auxiliary CNI chain (see details in [configuration.md](configuration.md)) | |||
There was a problem hiding this comment.
nit: replace executed with execute
docs/thick-plugin.md
Outdated
| @@ -111,3 +112,4 @@ It allows the following keys: | |||
| In thick plugin case, delegate CNI plugin is executed by multus-daemon from Pod, hence if the delegate CNI requires resources in container host, for example unix socket or even file, then CNI plugin is failed to execute because multus-daemon runs in Pod. Multus-daemon supports "chrootDir" option which executes delegate CNI under chroot (to container host). | |||
|
|
|||
| This configuration is enabled in deployments/multus-daemonset-thick.yml as default. | |||
|
|
|||
There was a problem hiding this comment.
got it, should just be newline now
| @@ -538,31 +540,117 @@ func getNetDelegate(client *ClientInfo, pod *v1.Pod, netname, confdir, namespace | |||
| } else { | |||
| // option4) if file path (absolute), then load it directly | |||
| if strings.HasSuffix(netname, ".conflist") { | |||
| confList, err := libcni.ConfListFromFile(netname) | |||
| confList, err := LoadChainedPluginsFromFile(netname) | |||
| if err != nil { | |||
| return nil, resourceMap, logging.Errorf("error loading CNI conflist file %s: %v", netname, err) | |||
There was a problem hiding this comment.
i think we should change the log to reflect the change from loading in conf files to loading in plugins
There was a problem hiding this comment.
good call, I put a few debug output lines in LoadChainedPluginsFromFile so that one can see that it's happening if it matters at that time.
| } | ||
| delegate, err := types.LoadDelegateNetConf(configBytes, nil, "", "") | ||
|
|
There was a problem hiding this comment.
how would we get to this point? i remember you had a brief explanation when we talkied, might be good to add a comment here
There was a problem hiding this comment.
yeah! good question, so, you can definitely get there, say, if you provide a file path that is a .conf -- which is the case for say, ovn-kubernetes. In that case, you can't chain onto a .conf, only a .conflist, in which case -- you must use the auxiliaryCNIChain to add chaining. So if you use ovn-k as your primary CNI, you'll have to configure multus to use the aux CNI chain, which will then load the subdirectory chain.
…ation loading. Removes the it `fails to execute confListDel given no 'plugins' key"` test. This test no longer fails after libcni version 1.2.3. It probably shouldn't failduring a DEL action as it is, we want the least error prone path. The GC test now uses both cni.dev attachment formats. Uses both attachment formats as per containernetworking/cni#1101 for GC's cni.dev/valid-attachments & cni.dev/attachments
Adds a test for plain subdirectory chaining and also using passthru CNI with auxiliaryCNIChainName
dougbtv
force-pushed
the
rebase5-cni-subdirectory-chain
branch
from
f41b35a to
4104fea
Compare
Implements a method for loading CNI plugins in a chain using a cnilib v1.3.0 feature for safe subdirectory based configuration loading, as introduced in containernetworking/cni#1052
See also, this asciinema demo of the functionality: https://asciinema.org/a/LKwyxyxdafgps5BXneME7ZMJE