Reconcile ClusterCIDRs with Finalizers

[mneverov]

Currently, when a finalizer is present a ClusterCIDR reconciliation (create/update) is skipped. Since users can create ClusterCIDRs with finalizers already in the ClusterCIDR definition it leads to problem described in #17.
This patch makes the following changes:

• reconcile ClusterCIDRs even if finalizers are present
• make createClusterCIDR idempotent by checking if a clusterCIDR is already present in the cidrMap.

Performance concerns:

• for each ClusterCIDR modification the clusterCIDRList in the cidrmap will be traversed. Iterating through a slice even with a thousand elements (nodes) should not be a problem
• for each modification the allocator will issue the ClusterCIDR update. Again, it should not be a problem since ClusterCIDRs are immutable and such updates should not happen (often). See also Replace Update with Patch #38.

Moving finalizer logic from createClusterCIDR requires changes in syncClusterCIDR signature since if it does not return an error cidrQueue forgets the event. I'll do it in a separate PR to keep this small.

Fixes #17

[mneverov]

@sarveshr7 @ameukam could you ptal?

[aojea]

the problem here is that we were doing the assumption that "finalizer" was the indicator to create or update.

Independently, before Create or Update, we need to check the new object we are building against the old object (this is missing), and then decide if we want to create or update. Regarding PATCH vs PUT, since these objects are only managed by this controller entirely, we can use PUT to move the object to the state we just built

[mneverov]

My understanding is since ClusterCIDR has all fields marked as immutable the only update we do is we set the finalizer.
We already check if the finalizer exists. Previously, we skipped the reconciliation and adding a ClusterCIDR to the cidrMap in this case that caused the bug.
With the current fix the finalizer is always added (if needed). We create a ClusterCIDR only when it does not have the ResourceVersion which seems to me a better indicator if the resource has already been processed and stored in etcd.
I understand that it is not the best approach and would maybe add a tuple to the queue: namespaced name and operation, such that we would know exact operation and would process the CIDR accordingly.
Can provide a PoC with the latter approach.

[aojea]

apologize because I only skimmed through the code and my comment can be inaccurate.

I think this is ok, just wanted to say that reconcilation seems to depend on more things than the finalizer, hence, we need to process the entire object.

I understand that it is not the best approach and would maybe add a tuple to the queue: namespaced name and operation, such that we would know exact operation and would process the CIDR accordingly.
Can provide a PoC with the latter approach.

that will make this an event based instead of level based controller, we don't want to do that.

What I try to mean, is that when you need to Update, you have the old and the new object, so you can compare them and decide if you want to update or not.

[mneverov]

@aojea ptal.
I added the check reflect.DeepEqual(old, new) to the update func. With this change reconciliation only happens when there was an actual change in ClusterCIDR. Which should never happen because all ClusterCIDR fields are immutable. The only valid case I see is the metadata change (for example adding finalizers) that should not affect networking.
Other misc changes - logs improvements and ignoring duplicates.
Changed test to print test cases separately:

--- PASS: TestSyncClusterCIDRCreate (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv4_ClusterCIDR_with_no_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv4_ClusterCIDR_with_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv4_ClusterCIDR_with_overlapping_CIDRs (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv6_ClusterCIDR_with_no_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv6_ClusterCIDR_with_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_IPv6_ClusterCIDR_with_overlapping_CIDRs (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_Dualstack_ClusterCIDR_with_no_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_DualStack_ClusterCIDR_with_NodeSelector (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/valid_Dualstack_ClusterCIDR_with_overlapping_CIDRs (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/invalid_ClusterCIDR_with_both_IPv4_and_IPv6_CIDRs_nil (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/invalid_IPv4_ClusterCIDR (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/invalid_IPv6_ClusterCIDR (0.00s)
    --- PASS: TestSyncClusterCIDRCreate/invalid_dualstack_ClusterCIDR (0.00s)

[aojea]

SGTM, just use apiequality.Semantic.DeepEqual https://github.com/search?q=repo%3Akubernetes%2Fkubernetes%20apiequality.Semantic.DeepEqual&type=code

It differentiates between nil and empty per example https://github.com/kubernetes/kubernetes/blob/8f8c94a04d00e59d286fe4387197bc62c6a4f374/staging/src/k8s.io/apimachinery/pkg/api/equality/semantic.go#L27-L28

[mneverov]

nice, didn't know about it. Thanks!

[aojea]

@mneverov please rebase and sorry for the late response, totally slipped, feel free to ping me in slack if you need reviews

/lgtm

[aojea]

/lgtm
/approve

Thanks

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, mneverov

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 [aojea,mneverov]

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