Add finalizer for AddressPool

docs/design.md

@@ -24,13 +24,14 @@ Design notes for Coil v2
   - [Implementation](#implementation)
 - [Garbage Collection](#garbage-collection)
   - [AddressBlock](#addressblock)
+  - [AddressPool](#addresspool)
   - [BlockRequest](#blockrequest)
 - [Upgrading from v1](#upgrading-from-v1)
 - [Diagrams](#diagrams)
   - [IPAM & Routing](#ipam--routing)
   - [On-demand NAT for Egress Traffics](#on-demand-nat-for-egress-traffics-1)
 - [Custom Resource Definitions (CRDs)](#custom-resource-definitions-crds)
-  - [AddressPool](#addresspool)
+  - [AddressPool](#addresspool-1)
   - [AddressBlock](#addressblock-1)
   - [BlockRequest](#blockrequest-1)
   - [Egress](#egress)
@@ -288,7 +289,7 @@ If you don't know much, read the following materials:
 ### AddressBlock
 
 **The owner reference of an `AddressBlock` is set to the `AddressPool`** from which the block was allocated.
-Therefore, when the owning `AddressPool` is deleted, all `AddressBlocks` from the pool is garbage collected automatically by Kubernetes.
+Therefore, when the deletion of the owning `AddressPool` is directed, all `AddressBlocks` from the pool is garbage collected automatically by Kubernetes.
 
 That said, an `AddressBlock` should not be deleted until there are no more Pods with an address in the block.
 For this purpose, Coil adds a finalizer to each `AddressBlock`.  **`coild` checks the usage of addresses in the block**, and once there are no more Pods using the addresses, it removes the finalizer to delete the `AddressBlock`.
@@ -299,6 +300,16 @@ For this purpose, Coil adds a finalizer to each `AddressBlock`.  **`coild` check
 
 Note that Coil does not include `Node` in the list of owner references of an `AddressBlock`.  This is because Kubernetes only deletes a resource after _all_ owners in the owner references of the resource are deleted.
 
+### AddressPool
+
+Similar to an `AddressBlock` and its addresses, an `AddressPool` should not be deleted until there are no more `AddressBlock`s derived from the pool.
+For this purpose, Coil adds a finalizer to each `AddressPool`.  `coil-controller` checks the usage of blocks in the pool.
+
+Note that `blockOwnerDeletion: true` in `AddressBlock`'s `ownerReferences` does not always block the deletion of the owning `AddressPool`.
+This directive has effect only when foreground cascading deletion is adopted.
+
+Also note that a finalizer for an `AddressPool` does not block the garbage collection on `AddressBlock`s.
+
 ### BlockRequest
 
 Normally, `coild` is responsible to delete `BlockRequest` created by itself.

v2/PROJECT

@@ -13,6 +13,7 @@ resources:
   path: github.com/cybozu-go/coil/v2/api/v2
   version: v2
   webhooks:
+    defaulting: true
     validation: true
     webhookVersion: v1
 - api:

v2/api/v2/addresspool_webhook.go

@@ -1,10 +1,12 @@
 package v2
 
 import (
+	"github.com/cybozu-go/coil/v2/pkg/constants"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
@@ -17,6 +19,15 @@ func (r *AddressPool) SetupWebhookWithManager(mgr ctrl.Manager) error {
 
 // EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 
+//+kubebuilder:webhook:path=/mutate-coil-cybozu-com-v2-addresspool,mutating=true,failurePolicy=fail,sideEffects=None,groups=coil.cybozu.com,resources=addresspools,verbs=create,versions=v2,name=maddresspool.kb.io,admissionReviewVersions={v1,v1beta1}
+
+var _ webhook.Defaulter = &AddressPool{}
+
+// Default implements webhook.Defaulter so a webhook will be registered for the type
+func (r *AddressPool) Default() {
+	controllerutil.AddFinalizer(r, constants.FinCoil)
+}
+
 // +kubebuilder:webhook:path=/validate-coil-cybozu-com-v2-addresspool,mutating=false,failurePolicy=fail,sideEffects=None,groups=coil.cybozu.com,resources=addresspools,verbs=create;update,versions=v2,name=vaddresspool.kb.io,admissionReviewVersions={v1,v1beta1}
 
 var _ webhook.Validator = &AddressPool{}

v2/api/v2/addresspool_webhook_test.go

@@ -3,24 +3,34 @@ package v2
 import (
 	"context"
 
+	"github.com/cybozu-go/coil/v2/pkg/constants"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 var _ = Describe("AddressPool Webhook", func() {
 	ctx := context.TODO()
 
 	BeforeEach(func() {
 		r := &AddressPool{}
-		r.Name = "test"
-		err := k8sClient.Delete(ctx, r)
-		if err == nil {
+		err := k8sClient.Get(ctx, client.ObjectKey{Name: "test"}, r)
+		if err != nil {
+			Expect(apierrors.IsNotFound(err)).To(BeTrue())
 			return
 		}
-		Expect(apierrors.IsNotFound(err)).To(BeTrue())
+
+		if controllerutil.ContainsFinalizer(r, constants.FinCoil) {
+			controllerutil.RemoveFinalizer(r, constants.FinCoil)
+			err = k8sClient.Update(ctx, r)
+			Expect(err).NotTo(HaveOccurred())
+		}
+
+		err = k8sClient.Delete(ctx, r)
+		Expect(err).NotTo(HaveOccurred())
 	})
 
 	It("should create an address pool with sane defaults", func() {
@@ -41,6 +51,7 @@ var _ = Describe("AddressPool Webhook", func() {
 		ap := &AddressPool{}
 		err = k8sClient.Get(ctx, client.ObjectKey{Name: "test"}, ap)
 		Expect(err).NotTo(HaveOccurred())
+		Expect(controllerutil.ContainsFinalizer(ap, constants.FinCoil)).To(BeTrue())
 		Expect(ap.Spec.BlockSizeBits).To(BeNumerically("==", 5))
 	})
 

v2/config/default/webhook_manifests_patch.yaml.tmpl

@@ -3,6 +3,9 @@ kind: MutatingWebhookConfiguration
 metadata:
   name: coilv2-mutating-webhook-configuration
 webhooks:
+- name: maddresspool.kb.io
+  clientConfig:
+    caBundle: "%CACERT%"
 - name: megress.kb.io
   clientConfig:
     caBundle: "%CACERT%"

v2/config/webhook/manifests.v1.yaml

@@ -6,6 +6,26 @@ metadata:
   creationTimestamp: null
   name: mutating-webhook-configuration
 webhooks:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-coil-cybozu-com-v2-addresspool
+  failurePolicy: Fail
+  name: maddresspool.kb.io
+  rules:
+  - apiGroups:
+    - coil.cybozu.com
+    apiVersions:
+    - v2
+    operations:
+    - CREATE
+    resources:
+    - addresspools
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   - v1beta1

v2/controllers/addresspool_controller.go

@@ -9,12 +9,14 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	coilv2 "github.com/cybozu-go/coil/v2/api/v2"
+	"github.com/cybozu-go/coil/v2/pkg/constants"
 	"github.com/cybozu-go/coil/v2/pkg/ipam"
 )
 
@@ -51,6 +53,27 @@ func (r *AddressPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	}
 
 	logger.Info("synchronized")
+
+	if ap.DeletionTimestamp == nil {
+		return ctrl.Result{}, nil
+	}
+
+	if !controllerutil.ContainsFinalizer(ap, constants.FinCoil) {
+		return ctrl.Result{}, nil
+	}
+
+	used, err := r.Manager.IsUsed(ctx, req.Name)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("IsUsed failed: %w", err)
+	}
+	if used {
+		return ctrl.Result{}, nil
+	}
+
+	controllerutil.RemoveFinalizer(ap, constants.FinCoil)
+	if err := r.Update(ctx, ap); err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to remove finalizer from address pool: %w", err)
+	}
 	return ctrl.Result{}, nil
 }
 

v2/controllers/addresspool_controller_test.go

@@ -2,13 +2,17 @@ package controllers
 
 import (
 	"context"
+	"errors"
 	"time"
 
 	coilv2 "github.com/cybozu-go/coil/v2/api/v2"
+	"github.com/cybozu-go/coil/v2/pkg/constants"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
 var _ = Describe("AddressPool reconciler", func() {
@@ -56,7 +60,7 @@ var _ = Describe("AddressPool reconciler", func() {
 		time.Sleep(10 * time.Millisecond)
 	})
 
-	It("works as expected", func() {
+	It("should synchronize pools", func() {
 		By("checking the synchronization status after startup")
 		Eventually(func() map[string]int {
 			return poolMgr.GetSynced()
@@ -102,4 +106,51 @@ var _ = Describe("AddressPool reconciler", func() {
 			"default": 1,
 		}))
 	})
+
+	It("should handle finalizers", func() {
+		By("adding the finalizer on behalf of webhook")
+		ap := &coilv2.AddressPool{}
+		err := k8sClient.Get(ctx, client.ObjectKey{Name: "default"}, ap)
+		Expect(err).To(Succeed())
+
+		controllerutil.AddFinalizer(ap, constants.FinCoil)
+		err = k8sClient.Update(ctx, ap)
+		Expect(err).To(Succeed())
+
+		By("trying to delete the pool while it is marked as used-by-AddressBlock")
+		poolMgr.SetUsed(true)
+		err = k8sClient.Delete(ctx, ap)
+		Expect(err).To(Succeed())
+		Consistently(func() error {
+			ap := &coilv2.AddressPool{}
+			return k8sClient.Get(ctx, client.ObjectKey{Name: "default"}, ap)
+		}).Should(Succeed())
+
+		By("marking the pool as not-used")
+		poolMgr.SetUsed(false)
+
+		// Update the pool to trigger reconciliation.
+		// In the real environment, reconciliation will be triggered by the deletion of dependent AddressBlocks.
+		err = k8sClient.Get(ctx, client.ObjectKey{Name: "default"}, ap)
+		Expect(err).To(Succeed())
+		if ap.Annotations == nil {
+			ap.Annotations = make(map[string]string)
+		}
+		ap.Annotations["foo"] = "bar"
+		err = k8sClient.Update(ctx, ap)
+		Expect(err).To(Succeed())
+
+		Eventually(func() error {
+			ap := &coilv2.AddressPool{}
+			err := k8sClient.Get(ctx, client.ObjectKey{Name: "default"}, ap)
+			if apierrors.IsNotFound(err) {
+				return nil
+			}
+			if err != nil {
+				return err
+			}
+
+			return errors.New("pool still exists")
+		}).Should(Succeed())
+	})
 })

v2/controllers/mock_test.go

@@ -17,6 +17,7 @@ type mockPoolManager struct {
 	dropped   map[string]int
 	synced    map[string]int
 	allocated int
+	used      bool
 }
 
 var _ ipam.PoolManager = &mockPoolManager{}
@@ -54,6 +55,13 @@ func (pm *mockPoolManager) AllocateBlock(ctx context.Context, poolName, nodeName
 	return block, nil
 }
 
+func (pm *mockPoolManager) IsUsed(ctx context.Context, name string) (bool, error) {
+	pm.mu.Lock()
+	defer pm.mu.Unlock()
+
+	return pm.used, nil
+}
+
 func (pm *mockPoolManager) GetDropped() map[string]int {
 	pm.mu.Lock()
 	defer pm.mu.Unlock()
@@ -81,6 +89,13 @@ func (pm *mockPoolManager) GetAllocated() int {
 	return pm.allocated
 }
 
+func (pm *mockPoolManager) SetUsed(used bool) {
+	pm.mu.Lock()
+	defer pm.mu.Unlock()
+
+	pm.used = used
+}
+
 type mockNodeIPAM struct {
 	mu       sync.Mutex
 	notified int

v2/pkg/ipam/pool.go

@@ -36,6 +36,9 @@ type PoolManager interface {
 	// AllocateBlock curves an AddressBlock out of the pool for a node.
 	// If the pool runs out of the free blocks, this returns ErrNoBlock.
 	AllocateBlock(ctx context.Context, poolName, nodeName string) (*coilv2.AddressBlock, error)
+
+	// IsUsed returns true if a pool is used by some AddressBlock.
+	IsUsed(ctx context.Context, name string) (bool, error)
 }
 
 var (
@@ -140,6 +143,14 @@ func (pm *poolManager) AllocateBlock(ctx context.Context, poolName, nodeName str
 	return p.AllocateBlock(ctx, nodeName)
 }
 
+func (pm *poolManager) IsUsed(ctx context.Context, name string) (bool, error) {
+	p, err := pm.getPool(ctx, name)
+	if err != nil {
+		return false, err
+	}
+	return p.IsUsed(ctx)
+}
+
 // pool manages the allocation of AddressBlock CR of an AddressPool CR.
 type pool struct {
 	name            string
@@ -273,3 +284,15 @@ func (p *pool) AllocateBlock(ctx context.Context, nodeName string) (*coilv2.Addr
 	p.log.Error(ErrNoBlock, "no available blocks")
 	return nil, ErrNoBlock
 }
+
+// IsUsed returns true if the pool is used by some AddressBlock.
+func (p *pool) IsUsed(ctx context.Context) (bool, error) {
+	blocks := &coilv2.AddressBlockList{}
+	err := p.reader.List(ctx, blocks, client.MatchingLabels{
+		constants.LabelPool: p.name,
+	})
+	if err != nil {
+		return false, err
+	}
+	return len(blocks.Items) > 0, nil
+}