Fix bug where IPs were assigned from affine blocks of disabled pools.

[ozdanborne]

Description

Fixes #689 .

Of course, this change will need to be pulled into projectcalico/cni-plugin before we see any results.

Todos

• Tests
• Documentation
• Release note

Release Note

Fixes a bug where IPs could be assigned from disabled IP pools.

[caseydavenport]

We create a context logger just a little bit below - I think you should move that above and then make sure all the logs are using logCtx

[caseydavenport]

Probably worth encapsulating the pool determination logic into its own function for easier testing and to clean up the autoAssign code a bit.

[caseydavenport]

This code then becomes:

pools, err := c.determinePools(requestedPools)
if err != nil { ... }

[caseydavenport]

This log is wrong now - we're not finding an unclaimed block yet (we might use an already claimed one)

[caseydavenport]

Just a few comments.

[ozdanborne]

@caseydavenport I'm wondering if erroring here is a bit harsh. We could just drop any pools that do not exist, just like we drop any pools that aren't enabled.

[ozdanborne]

Actually, I'm thinking this code is duplicate of the above comparison. Let me look into it further...

[caseydavenport]

Just a personal preference, but I'd drop the "else" branch here and do something like this:

if len(requestedPools) > 0 {
  // Build a map
  ...

  // Make sure each pool exists
  ...

  // Return the requested pools.
  return requestedPools
}

return enabledPools

I think it makes the code a bit easier to follow.

[ozdanborne]

Neat. Way cleaner. Fixed

[caseydavenport]

One more nit here - it's bad to mix named and unnamed return values.

I'd suggest removing the named return value from above, and simply returning requestedPools if we don't hit an error in the loop above.

That's nice because ultimately this function will always return either the requested pools, or all enabled pools and it becomes more obvious with that change.

[caseydavenport]

e.g. something like this:

for _, rp := range requestedPools{
  if !ok {
    ERROR!
  }
}

// Requested pools are valid
return requestedPools, nil

[gunjan5]

nit: don't need the else here
(not reviewing but I was flying by and saw this)