Use comma sep. string to set install binaries

[abhinavdahiya]

addresses #339

Update the install-cni.sh to loop through /opt/cni/bin/* and copy only binaries that are requested in comma string as env variable INSTALL_CNI_BINARIES

Default value set to "calico,calico-ipam,flannel,loopback,host-local" to keep backward compatibility.

/cc: @yifan-gu

Release Note

The calico/cni container now supports setting `SKIP_CNI_BINARIES` to skip installation of certain binaries.

[CLAassistant]

All committers have signed the CLA.

[tmjd]

I'm thinking this new env variable should specify the binaries to not install. In the future when new binaries are added or current ones renamed the current proposed list will mean that any upgrades will result in a broken setup. If it was a list of binaries to exclude then new ones can be added without the need to update the env var.
@caseydavenport WDYT?

[abhinavdahiya]

@tmjd
I think INSTALL_CNI_BINARIES allows users to set it to calico,calico-ipam and not worry about other binaries. If we use something like SKIP_CNI_BINARIES users who want just calico,calico-ipam will have to specify list of all other binaries to skip which might keep on changing.

[tmjd]

I understand that, but I'm thinking if calico or calico-ipam were renamed or began to depend on a 3rd binary then a setup with INSTALL_CNI_BINARIES would break on upgrade.

If a new binary was added to the installer that was not needed in some setups, then not having it listed in SKIP_CNI_BINARIES would, I think, more than likely not break the setup.

I'm asking the question, which configuration is more likely to break on upgrade?

Or maybe this setting is a specific corner case, and that it breaking during an upgrade is too small to be concerned with.

[yifan-gu]

@tmjd Good point. The use case we have is not to override some binaries provided by 3rdparty. So SKIP_CNI_BINARIES SGTM.

[abhinavdahiya]

@caseydavenport @tmjd @yifan-gu If everybody agrees that SKIP_CNI_BINARIES is more useful. I'll update the PR.

[abhinavdahiya]

@yifan-gu @tmjd @caseydavenport
Since every one suggested that SKIP_CNI_BINARIES would be a better solution. I have pushed changes to change INSTALL_CNI_BINARIES flag to SKIP_CNI_BINARIES

[tmjd]

Looking good, just the one comment that applies twice.

[tmjd]

Is there a reason for the double negative? [ ! x != y ] If not please switch to equality check, otherwise please add a comment.

[tmjd]

double negative

[abhinavdahiya]

@tmjd changed the double negative to equality check. Thanks!

[tmjd]

@abhinavdahiya Before we can merge this could you describe the testing you have done for this change?

[tmjd]

I pulled down the changes and built a new calico/cni container and then did two runs with it. The first with no SKIP_CNI_BINARIES value set and the second run with portmap set. Both runs worked as expected and when I inspected /opt/cni/bin for the 2nd run the portmap binary was removed.

[abhinavdahiya]

I ran following test-cases locally:
-1:
SKIP_CNI_BINARIES=""
/opt/cni/bin = calico,calico-ipam,flannel,host-local,loopback,portmap

-2:
SKIP_CNI_BINARIES="flannel"
/opt/cni/bin = calico,calico-ipam,host-local,loopback,portmap

-3:
SKIP_CNI_BINARIES="flannel,portmap"
/opt/cni/bin = calico,calico-ipam,host-local,loopback

-4:
SKIP_CNI_BINARIES="flannel,non-existing-binary"
/opt/cni/bin = calico,calico-ipam,host-local,loopback,portmap

-5:
SKIP_CNI_BINARIES="flannel,,"
/opt/cni/bin = calico,calico-ipam,host-local,loopback,portmap

I think these cover most of the cases..?

[caseydavenport]

@abhinavdahiya @tmjd this LGTM.

We'll need to submit a docs PR for this change. Looking at our docs, I don't think we have a reference section for the install-cni container.

@tmjd could you run with that?