adding CNS fix for requesting IPs with 0 NCs #2063
Conversation
cns/restserver/ipam.go
Outdated
| podIPInfo := make([]cns.PodIpInfo, numDesiredIPAddresses) | ||
| // if there are no NCs on the NNC there will be no IPs in the pool so return error | ||
| if numDesiredIPAddresses == 0 { | ||
| //nolint:goerr113 // return error |
There was a problem hiding this comment.
This linter bypass is inappropriate here. The error returned below should be defined like:
var errNoPoolIPs = errors.New("no NCs found on the NNC so no IPs are in the pool")and consequently used like:
return nil, errNoPoolIPsNote also the nil, instead of podIPInfo. In cases where an error is returned, it should be a nil or zero value being returned first.
This also applies to all of the other instances of //nolint:goerr113 // return error for this func.
There was a problem hiding this comment.
Added a ErrNoNCs error variable and check for it in the UTs as well.
cns/restserver/ipam.go
Outdated
| // Sets the number of desired IPs equal to the number of NCs so that we can get one IP per NC | ||
| numDesiredIPAddresses := len(desiredIPAddresses) | ||
| // Creates a slice of PodIpInfo with the size as number of NCs to hold the result for assigned IP configs | ||
| podIPInfo := make([]cns.PodIpInfo, numDesiredIPAddresses) | ||
| // if there are no NCs on the NNC there will be no IPs in the pool so return error | ||
| if numDesiredIPAddresses == 0 { |
There was a problem hiding this comment.
Where you basically want to map over desiredIPAddresses, it's usually more idiomatic to let the caller pass in a []cns.PodIpInfo that is partially complete (with just the IP addresses). This lets the caller control the allocation, rather than having these intermediate allocations in here.
That also forces the caller of this method to handle the request rather than spreading that responsibility among many other funcs.
If you don't pursue that, this conditional should be at the beginning of the method (so we don't allocate a slice).
There was a problem hiding this comment.
The majority of these lines were already present, so I don't think a major refactor is needed here, but agree with moving the input validation to be the first thing in the method.
There was a problem hiding this comment.
I should have emphasized the first part as non-blocking, but moving the conditional as blocking feedback. I still like to provide feedback on the code overall in case someone wants to refactor it... It's unlikely we will ever schedule refactoring this, so PRs provide the only opportunity.
There was a problem hiding this comment.
Moved the check higher but we won't refactor for this PR since we are trying to get it in fairly quickly. It is a good idea though.
cns/restserver/ipam.go
Outdated
| //nolint:goerr113 // return error | ||
| return podIPInfo, fmt.Errorf("No NCs found on the NNC so no IPs are in the pool") |
There was a problem hiding this comment.
This is exactly why the error should be reified in a var--so it can be reused here (and checked by callers).
rjdenney
force-pushed
the
IPConfigHandlerFix
branch
from
dfef9e1 to
3650c4f
Compare
Reason for Change:
A bug was found when using older versions of CNI with versions of CNS 1.5 and newer where it would panic when there aren;'t any NCs on the NNC. When CNI requested IPs in this state CNS would think it requires "0" IPs to return since it is currently based on the number of NCs. When trying to index a slice of 0 we would then see the panic.
This fix should lead to CNS returning an error in a few locations when we see that there are 0 NCs since this also implies that there aren't any IPs in the pool.
Issue Fixed:
Requirements:
Notes: