check version incompatibility

[bn222]

When nested CNI version is 0.3.1 or less while the top level CNI version
is 0.4.0, error out. Version 0.3.1 or less doesn't support the CHECK
command. This closes issue #737.

[coveralls]

Pull Request Test Coverage Report for Build 1481899134

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 70.885%

---

Totals
Change from base Build 1480949645:	0.0%
Covered Lines:	1322
Relevant Lines:	1865

---

💛 - Coveralls

[coveralls]

Changes unknown
when pulling 5b51db5 on bn222:master
into ** on k8snetworkplumbingwg:master**.

[dougbtv]

Hey @bn222 ! Thanks for the contribution.

We'll also need to take care of this in the next generation configuration generation, which can be found approximately here:

https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/cmd/controller/main.go
https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/cmd/config-generation/kubeconfig.go

[bn222]

Hi @dougbtv, I'm not sure what I should change in the two files you mention. I guess you might actually mean the following file:

https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/pkg/config/generator.go#L37

I could enforce checking that the delegate configurations are using a compatible CNI version, although that would require me to change the type of Delegates in MultusConf if we want to avoid converting to json and checking the cni version in json.

Let me know what you think!

[dougbtv]

Hey Balazs, what I mean is that we have two methods for entrypoint as we're in a transition (which will be superceded by the go code, making the bash script obsolete)

So, in two different modes, either the bash runs, or the go runs (the files I linked)

Meaning that you'll have to implement the equivalent logic in the the go code as well as the bash 👍

[bn222]

@dougbtv afaict, this covers the code path that goes through the thick plugin. Let me know if more paths need patching.

[lgtm-com]

This pull request introduces 1 alert when merging 69d8dd7 into 4d9731b - view on LGTM.com

new alerts:

• 1 for Useless assignment to local variable

[maiqueb]

Other than the comments I'm providing in the code, I can't help to notice that the new functionality is not being unit tested; you'll have to provide these tests as well.

I also disagree with your choice of where to implement this: I think it should be done in the configuration generator constructor, not in its generate method.

[maiqueb]

It feels a bit weird to not check the error on newMultusConfigWithDelegates now that it can return one.

Unfortunately, I don't have a good alternative to doing that: either check it on the test's body, or introduce another helper for it.

Pretty much everything else is opinionated.

[maiqueb]

Thanks for the patience, and for the patch.

@dougbtv I've only reviewed the golang related code.

[s1061123]

It looks good to me. Apologize that the PR takes a very long time. Let me merge into master. Thank you so much!