Skip to content

🐛 properly handle deletion of IPAddressClaims in ipamutil.ClaimReconciler #329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

🐛 properly handle deletion of IPAddressClaims in ipamutil.ClaimReconciler #329

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c0c1fa
patch immediately after setting finalizer on IPAddressClaim
schrej Mar 24, 2025
0e182f3
fix ReleaseAddress not getting called during deletion
schrej Mar 24, 2025
fa644f4
adding err from handler.FetchPool to fmt.Errorf to not hide it
llxp Apr 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 32 additions & 30 deletions pkg/ipamutil/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,16 @@ type ProviderAdapter interface {
// ClaimHandler knows how to allocate and release IP addresses for a specific provider.
type ClaimHandler interface {
// FetchPool is called to fetch the pool referenced by the claim. The pool needs to be stored by the handler.
// This method is called before EnsureAddress and ReleaseAddress.
// Note that the ClaimReconciler will call the ReleaseAddress method regardless whether fetching the pool was
// successful or not.
FetchPool(ctx context.Context) (client.Object, *ctrl.Result, error)
// EnsureAddress is called to make sure that the IPAddress.Spec is correct and the address is allocated.
// It will only be called when FetchPool was successful.
EnsureAddress(ctx context.Context, address *ipamv1.IPAddress) (*ctrl.Result, error)
// ReleaseAddress is called to release the ip address that was allocated for the claim.
// It will be called even if FetchPool was not successful when reconciling a deleted claim, so there is no guarantee
// that the pool is available in the handler.
ReleaseAddress(ctx context.Context) (*ctrl.Result, error)
}

Expand Down Expand Up @@ -138,24 +144,18 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
cluster, err = clusterutil.GetClusterFromMetadata(ctx, r.Client, claim.ObjectMeta)
}
if err != nil {
if apierrors.IsNotFound(err) {
if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
patch := client.MergeFrom(claim.DeepCopy())
if err := r.reconcileDelete(ctx, claim); err != nil {
return ctrl.Result{}, fmt.Errorf("reconcile delete: %w", err)
}
// we'll need to explicitly patch the claim here since we haven't set up a patch helper yet.
if err := r.Client.Patch(ctx, claim, patch); err != nil {
return ctrl.Result{}, fmt.Errorf("patch after reconciling delete: %w", err)
}
return ctrl.Result{}, nil
}
if !apierrors.IsNotFound(err) {
log.Error(err, "error fetching cluster linked to IPAddressClaim")
return ctrl.Result{}, err
}
if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
// In case the claim is deleted, we'll process it even when we can't find the cluster. During cluster deletion it
// can happen that the Cluster object is released before all claims are cleaned up, which would block the claims indefinitely.
log.Info("IPAddressClaim linked to a cluster that is not found, unable to determine cluster's paused state, proceeding with deletion")
} else {
log.Info("IPAddressClaim linked to a cluster that is not found, unable to determine cluster's paused state, skipping reconciliation")
return ctrl.Result{}, nil
}

log.Error(err, "error fetching cluster linked to IPAddressClaim")
return ctrl.Result{}, err
}
if cluster != nil {
if annotations.IsPaused(cluster, cluster) {
Expand All @@ -176,7 +176,9 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
}
}()

controllerutil.AddFinalizer(claim, ReleaseAddressFinalizer)
if controllerutil.AddFinalizer(claim, ReleaseAddressFinalizer) {
return ctrl.Result{}, nil
}

var res *reconcile.Result
var pool client.Object
Expand All @@ -185,10 +187,10 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
handler := r.Adapter.ClaimHandlerFor(r.Client, claim)
if pool, res, err = handler.FetchPool(ctx); err != nil || res != nil {
if apierrors.IsNotFound(err) {
err := errors.New("pool not found")
err := fmt.Errorf("pool not found: %w", err)
log.Error(err, "the referenced pool could not be found")
if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
return ctrl.Result{}, r.reconcileDelete(ctx, claim)
return r.reconcileDelete(ctx, claim, handler)
}
return ctrl.Result{}, nil
}
Expand All @@ -206,12 +208,8 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
return ctrl.Result{}, nil
}

// If the claim is marked for deletion, release the address.
if !claim.ObjectMeta.DeletionTimestamp.IsZero() {
if res, err := handler.ReleaseAddress(ctx); err != nil {
return unwrapResult(res), err
}
return ctrl.Result{}, r.reconcileDelete(ctx, claim)
return r.reconcileDelete(ctx, claim, handler)
}

// We always ensure there is a valid address object passed to the handler.
Expand Down Expand Up @@ -261,34 +259,38 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct
return ctrl.Result{}, nil
}

func (r *ClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPAddressClaim) error {
func (r *ClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPAddressClaim, handler ClaimHandler) (ctrl.Result, error) {
if res, err := handler.ReleaseAddress(ctx); err != nil {
return unwrapResult(res), fmt.Errorf("release address: %w", err)
}

address := &ipamv1.IPAddress{}
namespacedName := types.NamespacedName{
Namespace: claim.Namespace,
Name: claim.Name,
}
if err := r.Client.Get(ctx, namespacedName, address); err != nil && !apierrors.IsNotFound(err) {
return errors.Wrap(err, "failed to fetch address")
return ctrl.Result{}, errors.Wrap(err, "failed to fetch address")
}

if address.Name != "" {
var err error
patch := client.MergeFrom(address.DeepCopy())
p := client.MergeFrom(address.DeepCopy())
if controllerutil.RemoveFinalizer(address, ProtectAddressFinalizer) {
if err = r.Client.Patch(ctx, address, patch); err != nil && !apierrors.IsNotFound(err) {
return errors.Wrap(err, "failed to remove address finalizer")
if err = r.Client.Patch(ctx, address, p); err != nil && !apierrors.IsNotFound(err) {
return ctrl.Result{}, errors.Wrap(err, "failed to remove address finalizer")
}
}

if err == nil {
if err := r.Client.Delete(ctx, address); err != nil && !apierrors.IsNotFound(err) {
return err
return ctrl.Result{}, err
}
}
}

controllerutil.RemoveFinalizer(claim, ReleaseAddressFinalizer)
return nil
return ctrl.Result{}, nil
}

func (r *ClaimReconciler) clusterToIPClaims(_ context.Context, a client.Object) []reconcile.Request {
Expand Down