Cri-o: --cni-plugin-dir flag acts strangely

[haircommander]

Description
in researching a fix for https://github.com/cri-o/cri-o/issues/3823, I found bug where we always overwrite the first plugin_dir to the values in a config file when using --cni-plugin-dir, instead of taking the cli as precedence

This is all with plugin_dirs defined in both /etc/crio/crio.conf and /etc/crio/crio.conf.d/00-default.conf:

grep -r plugin_dirs -A 3 /etc/crio/crio.conf*
/etc/crio/crio.conf:plugin_dirs = [
/etc/crio/crio.conf-    "/opt/cni/bin/",
/etc/crio/crio.conf-]
/etc/crio/crio.conf-
--
/etc/crio/crio.conf.d/00-default.conf:plugin_dirs = [
/etc/crio/crio.conf.d/00-default.conf-  "/opt/cni/bin/",
/etc/crio/crio.conf.d/00-default.conf-]

Describe the results you received:

$ ./bin/crio --cni-plugin-dir /tmp/a --cni-plugin-dir /tmp/b config | grep plugin_dirs -A 3
plugin_dirs = [
    "/opt/cni/bin/",
    "/tmp/b",
]
$ ./bin/crio --cni-plugin-dir /tmp/a  config | grep plugin_dirs -A 3
plugin_dirs = [
    "/opt/cni/bin/",
]

Describe the results you expected:

$ ./bin/crio --cni-plugin-dir /tmp/a --cni-plugin-dir /tmp/b config | grep plugin_dirs -A 3
plugin_dirs = [
    "/tmp/a",
    "/tmp/b",
]
$ ./bin/crio --cni-plugin-dir /tmp/a  config | grep plugin_dirs -A 3
plugin_dirs = [
    "/tmp/a",
]

Note: we get the correct output if we do:

./bin/crio -c "" -d "" --cni-plugin-dir /tmp/a config | grep plugin_dirs -A 3
INFO Using default capabilities: CAP_CHOWN, CAP_DAC_OVERRIDE, CAP_FSETID, CAP_FOWNER, CAP_SETGID, CAP_SETUID, CAP_SETPCAP, CAP_NET_BIND_SERVICE, CAP_KILL 
plugin_dirs = [
    "/tmp/a",
]

Output of crio --version:

./bin/crio -v
crio version 
Version:       1.19.0-dev
GitCommit:     588911bc60db9825a0aa85cd19f0acf56ac0d84c
GitTreeState:  dirty
BuildDate:     2020-06-09T20:37:27Z
GoVersion:     go1.14.2
Compiler:      gc
Platform:      linux/amd64
Linkmode:      dynamic

though I think I observed it as far back as 1.16

[haircommander]

I also suspect other slice config values behave similarly, probably should test them all in integration/unit tests...

[saschagrunert]

@rhafer maybe wanna take this one? :)

[rhafer]

@saschagrunert Yeah. I'll take a look.

[rhafer]

It looks as if mergeConfig() is not really idempotent. Upon a subsequent calls it might overwrite some args in the cli context. And in the case of crio config it is actually called twice via GetConfigFromContext(). (here: https://github.com/cri-o/cri-o/blob/master/cmd/crio/main.go#L147 and here: https://github.com/cri-o/cri-o/blob/master/cmd/crio/config.go#L52). The same is true for crio wipe I think.

I don't think it is really needed to call it twice. The subcommands could just get the parsed config via app.Metadata I guess, that would also avoid parsing all config files twice.

Additionally I guess we should return a copy of the slice in StringSliceTrySplit() (https://github.com/cri-o/cri-o/blob/master/internal/criocli/criocli.go#L774) to avoid it being overwritten.

... working on a fix.

[saschagrunert]

Sounds all good to me, thank you @rhafer :pray:

[rhafer]

3880 backports this to 1.18. Does this issue qualify for backports to even older releases?