CNI config errors retured as error instead of logging warning

[kunalkushwaha]

While loading CNI config, if any config file have syntax error,
it is returned as error instead of logging as warning.

This will fix early detection of CNI errors and report user exact
error instead of generic error while requesting CNI network.

Signed-off-by: Kunal Kushwaha [email protected]

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kunalkushwaha
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

Hi @kunalkushwaha. Thanks for your PR.

I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[kunalkushwaha]

This fix is in reference with podman issue: containers/podman#2909.

What it does:
This will return error in InitCNI() / loadNetwork() function if any syntax error is found in any of the conflist in specified confDir.
Currently, in such situation, it logs Warning and continue loading rest of config files.

what will be affected

• This will affect the invocation of InitCNI() function, which normally rarely returns error now, in both CRI-o & podman. Now if case of any invalid config file exist in CNI confDir it will return error.

[kunalkushwaha]

@rajatchopra @sameo @dcbw PTAL

[dcbw]

Looking at the code loadNetworks() only gets called by syncNetworkConfig(). That is only called by monitorConfDir() which does log the error, and initCNI() which throws the error away. I don't quite see how this will change the current behavior of logging the error?

Second issue is that if any file in the conf dir has an error, no other files will be processed even if they might be valid and could be the requested default network. The original code was permissive and tried to allow that, I think by choice.

I think the code should definitely make sure it logs syntax errors, but I don't think that should prevent it from finding valid files, especially in the case that a default network has been explicitly specified. I guess I can see how you'd want to error earlier if no default network was specified (eg the behavior of picking first in the directory), but we don't want to break the case of a random file preventing the explicitly specified network from being found.

[kunalkushwaha]

Looking at the code loadNetworks() only gets called by syncNetworkConfig(). That is only called by monitorConfDir() which does log the error, and initCNI() which throws the error away. I don't quite see how this will change the current behavior of logging the error?

error returned by initCNI() are not in case of cni - config file error.

Second issue is that if any file in the conf dir has an error, no other files will be processed even if they might be valid and could be the requested default network. The original code was permissive and tried to allow that, I think by choice.

I had also similar view initially. This solution was suggested by @mheon
[complete discussion : https://github.com/containers/podman/issues/2909#issuecomment-482404424 ]

Also I observed other container runtime like containerd fails if any of config file have error in CNI conf directory.

[rhatdan]

Any movement on this?

[kunalkushwaha]

@dcbw PTAL at comments

[vrothberg]

Friendly ping. I'm going through libpod issues and containers/podman#2909 requires this PR.

[rhatdan]

@vrothberg Is this still needed?

[vrothberg]

@vrothberg Is this still needed?

Given the age, I don't think so.