From 9c0c1faabe0ee14ce4b074f9acf936ccb3dd5a72 Mon Sep 17 00:00:00 2001 From: Jakob Schrettenbrunner Date: Mon, 24 Mar 2025 18:03:29 +0100 Subject: [PATCH 1/3] patch immediately after setting finalizer on IPAddressClaim --- pkg/ipamutil/reconciler.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/ipamutil/reconciler.go b/pkg/ipamutil/reconciler.go index 430b3c3..368ebdb 100644 --- a/pkg/ipamutil/reconciler.go +++ b/pkg/ipamutil/reconciler.go @@ -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 From 0e182f39c712dd52d741d87ff34caf144f920e34 Mon Sep 17 00:00:00 2001 From: Jakob Schrettenbrunner Date: Mon, 24 Mar 2025 18:25:54 +0100 Subject: [PATCH 2/3] fix ReleaseAddress not getting called during deletion There were two cases where deletion of a claim was reconciled without calling ClaimHandler.ReleaseAddress, which causes issues for providers that manage addresses in external systems. --- pkg/ipamutil/reconciler.go | 56 +++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/pkg/ipamutil/reconciler.go b/pkg/ipamutil/reconciler.go index 368ebdb..b2abfcc 100644 --- a/pkg/ipamutil/reconciler.go +++ b/pkg/ipamutil/reconciler.go @@ -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) } @@ -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) { @@ -190,7 +190,7 @@ func (r *ClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ct err := errors.New("pool not found") 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 } @@ -208,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. @@ -263,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 { From fa644f493ec72035c0060c16fd2ae9a9c525ec57 Mon Sep 17 00:00:00 2001 From: "lukas.luedke@telekom.de" Date: Thu, 17 Apr 2025 16:34:10 +0200 Subject: [PATCH 3/3] adding err from handler.FetchPool to fmt.Errorf to not hide it --- pkg/ipamutil/reconciler.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/ipamutil/reconciler.go b/pkg/ipamutil/reconciler.go index b2abfcc..8c666ea 100644 --- a/pkg/ipamutil/reconciler.go +++ b/pkg/ipamutil/reconciler.go @@ -187,7 +187,7 @@ 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 r.reconcileDelete(ctx, claim, handler)