Ensure existing IPAddress's finalizer and ownerRefs on pre-existing IPAddresses #88
Conversation
adobley
force-pushed
the
ensure-refs-owners
branch
4 times, most recently
from
9280754 to
f67d96e
Compare
| for i, ownerRef := range address.GetOwnerReferences() { | ||
| if ownerRef.APIVersion == poolGVK.GroupVersion().String() && | ||
| ownerRef.Kind == poolGVK.Kind && | ||
| ownerRef.Name == pool.GetName() { | ||
| poolRefIdx = i | ||
| } | ||
| } | ||
|
|
||
| address.OwnerReferences[poolRefIdx].Controller = pointer.Bool(false) | ||
| address.OwnerReferences[poolRefIdx].BlockOwnerDeletion = pointer.Bool(true) |
There was a problem hiding this comment.
The helper functions that ensure the owner ref do not expose setting the Controller nor the BlockOwnerDeletion flag. The logic that creates the owner refs when the IPAddress is created sets these flags. We add those flags for consistency.
https://github.com/telekom/cluster-api-ipam-provider-in-cluster/blob/f67d96e92729d54fa51626d92d8adbb2d345527a/pkg/ipamutil/address.go#L27-L28
flawedmatrix
force-pushed
the
ensure-refs-owners
branch
2 times, most recently
from
b0ae7cf to
fc7516a
Compare
tylerschultz
changed the title
flawedmatrix
force-pushed
the
ensure-refs-owners
branch
3 times, most recently
from
ce54fb6 to
278b5c4
Compare
|
Hi @srm09, We made the refactor we discussed over a call. We stopped creating the IPAddress with ownerRefs, and instead made a call to the ensure function to add the ownerRefs. There is only one way that ownerRefs and finalizers are added to an address. We didn't go as with the refactor as you suggested. We didn't want to create an address, and then patch it right away. We wanted to avoid a second API call when creating an address. This is confusing to describe, let us know if you'd like to discuss further. Cheers. |
| address, err := r.allocateAddress(ctx, claim, pool, addressesInUse) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to allocate address") | ||
| } |
There was a problem hiding this comment.
allocateAddress still does the job of setting the owner Refs. The call to address := ipamutil.NewIPAddress(claim, pool) in the function still sets the owner reference on the IPAddress object.
Ideally,
- Fetch the IP address for the claim
- If not found,
allocateAddress=> just allocates a new unused IP address (does not make any API calls to persist the object, no owner ref logic in there either) - Use this IP address from above to set/reset finalizer and owner refs (making it clear that the controller owner ref is being set in the controller reconcile loop)
- Persist the object to the API server.
This logic works for either scenarios (new/existing address). All the fanciful owner ref and finalizer reconciliation logic stays in one place.
For the purpose of logging the presence of an existing IPAddress object, add an info logger after the fetch call for the IPAddress, since that was the main concern around the refactor.
There was a problem hiding this comment.
The patch helper is not able to create an address for the create case. I think there's something I'm missing.
There was a problem hiding this comment.
I get your point now, there is a way around it https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil#CreateOrPatch
But am gonna stop pushing for this one now. The code does what it is supposed to, just maintaining it will be difficult in the longer term. There is an easy chance that we might forget updating either of the places in the future.
There was a problem hiding this comment.
ipamutil.NewIPAddress was changed to no longer set the owner references. Maybe you've missed that change below?
I like it this way.
We could consider moving ensureOwnerRefs (without the finalizer part) to ipamutil as well. The idea is to potentially move ipamutils to CAPI to allow reusing some logic between providers. We might be able to improve it further with a second provider (e.g. the infoblox one) and turn it into some kind of frameworks. There should be quite a few similarities between ipam providers I think.
There was a problem hiding this comment.
@srm09 Ah, we got fixated on using the patch helper code, I didn't realize there was a CreateOrPatch. We've updated the logic as you've described. The result looks nice.
@schrej We've moved the ensureAddressOwnerReferences function to the pkg/ipam-util package.
I think we've updated everything to everyone's preference. Let us know if we've missed something.
christianang
force-pushed
the
ensure-refs-owners
branch
3 times, most recently
from
5b6be98 to
8753cb0
Compare
There was a problem hiding this comment.
Looks very clean now, thanks!
@tylerschultz could you fix the linter issue? It's a new problem now, I've fixed the gci import order thing that it was complaining about before.
Ensure IPAddresses have the expected finalizer and Owner References to their respective InClusterIPPool and IPAddress Claims. A case where this may happen is when a cluster has been restored from backup. Co-authored-by: Edwin Xie <[email protected]> Co-authored-by: Aidan Obley <[email protected]> Co-authored-by: Christian Ang <[email protected]> Signed-off-by: Tyler Schultz <[email protected]>
christianang
force-pushed
the
ensure-refs-owners
branch
from
8753cb0 to
2d45fb5
Compare
Ensure IPAddresses have the expected ProtectAddress finalizer and Owner References to their respective InClusterIPPool and IPAddress Claims. A case where this may happen is when a cluster has been restored from backup.