Fix CNI crashing when there is no available IP addresses. #1499
Conversation
1. when IPAMD returns "no ip available error", the err variable get overwritten to nil when get VPCCIDRs 2. when DelNetwork returns err, the r.Success will cause nil-pointer exception and crash CNI
| } | ||
|
|
||
| if !r.Success { | ||
| } else if !r.Success { |
There was a problem hiding this comment.
nit: We will end up here only if delErr is nil right? So, don't see any value printing delErr in the below error message.
There was a problem hiding this comment.
Also, why not extend this to scenarios where add fails as well.
https://github.com/aws/amazon-vpc-cni-k8s/blob/master/cmd/routed-eni-cni-plugin/cni.go#L164
There was a problem hiding this comment.
@achevuru because when add fails, we always return within err != nil check.
There was a problem hiding this comment.
nit: We will end up here only if delErr is nil right? So, don't see any value printing delErr in the below error message.
make sense.
There was a problem hiding this comment.
I think later sometime we need to simplify this, like for instance make Add and Del functions return just one structure with the response and error, now there are two variables and anyone adding a piece of code should be aware of setting response when err is nil.
There was a problem hiding this comment.
yeah, this whole piece needs to restructured and better handled.
like how to handle err vs r.success. (seems there is a duplicate here)
* Fix two bug in CNI/IPamd code path 1. when IPAMD returns "no ip available error", the err variable get overwritten to nil when get VPCCIDRs 2. when DelNetwork returns err, the r.Success will cause nil-pointer exception and crash CNI * fix test cases * address commits
Currently when CNI is invoked to AddNetwork while there is no available IP address in IPAMD, CNI crashes.
It happens due to two bug below:
What type of PR is this?
Which issue does this PR fix:
What does this PR do / Why do we need it:
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Testing done on this change:
Automation added to e2e:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this change require updates to the CNI daemonset config files to work?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.