diff --git a/docs/design.md b/docs/design.md index e1837ce1..1c54ae7d 100644 --- a/docs/design.md +++ b/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. diff --git a/v2/PROJECT b/v2/PROJECT index 4e483caa..d1381869 100644 --- a/v2/PROJECT +++ b/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: diff --git a/v2/api/v2/addresspool_webhook.go b/v2/api/v2/addresspool_webhook.go index 09c6787f..84479f0b 100644 --- a/v2/api/v2/addresspool_webhook.go +++ b/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{} diff --git a/v2/api/v2/addresspool_webhook_test.go b/v2/api/v2/addresspool_webhook_test.go index 263fdd9d..1186f3ac 100644 --- a/v2/api/v2/addresspool_webhook_test.go +++ b/v2/api/v2/addresspool_webhook_test.go @@ -3,11 +3,13 @@ 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() { @@ -15,12 +17,20 @@ var _ = Describe("AddressPool Webhook", func() { 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)) }) diff --git a/v2/config/default/webhook_manifests_patch.yaml.tmpl b/v2/config/default/webhook_manifests_patch.yaml.tmpl index b000460c..29096510 100644 --- a/v2/config/default/webhook_manifests_patch.yaml.tmpl +++ b/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%" diff --git a/v2/config/webhook/manifests.v1.yaml b/v2/config/webhook/manifests.v1.yaml index b1d5e6d9..a97b1d19 100644 --- a/v2/config/webhook/manifests.v1.yaml +++ b/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 diff --git a/v2/controllers/addresspool_controller.go b/v2/controllers/addresspool_controller.go index 6a4543a3..13637c1b 100644 --- a/v2/controllers/addresspool_controller.go +++ b/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 } diff --git a/v2/controllers/addresspool_controller_test.go b/v2/controllers/addresspool_controller_test.go index 121fdf6f..a0fc9d35 100644 --- a/v2/controllers/addresspool_controller_test.go +++ b/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()) + }) }) diff --git a/v2/controllers/mock_test.go b/v2/controllers/mock_test.go index 2ccc94a5..c825a370 100644 --- a/v2/controllers/mock_test.go +++ b/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 diff --git a/v2/pkg/ipam/pool.go b/v2/pkg/ipam/pool.go index a6de6572..aa9c863d 100644 --- a/v2/pkg/ipam/pool.go +++ b/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 +} diff --git a/v2/pkg/ipam/pool_test.go b/v2/pkg/ipam/pool_test.go index b2bc6a12..4084409f 100644 --- a/v2/pkg/ipam/pool_test.go +++ b/v2/pkg/ipam/pool_test.go @@ -25,6 +25,10 @@ var _ = Describe("PoolManager", func() { It("should allocate blocks", func() { pm := NewPoolManager(mgr.GetClient(), mgr.GetAPIReader(), ctrl.Log.WithName("PoolManager"), scheme) + used, err := pm.IsUsed(ctx, "default") + Expect(err).ToNot(HaveOccurred()) + Expect(used).To(BeFalse()) + blocks := make([]*coilv2.AddressBlock, 0, 6) block, err := pm.AllocateBlock(ctx, "default", "node1") Expect(err).ToNot(HaveOccurred()) @@ -37,6 +41,10 @@ var _ = Describe("PoolManager", func() { Expect(block.Labels[constants.LabelPool]).To(Equal("default")) Expect(controllerutil.ContainsFinalizer(block, constants.FinCoil)).To(BeTrue()) + used, err = pm.IsUsed(ctx, "default") + Expect(err).ToNot(HaveOccurred()) + Expect(used).To(BeTrue()) + verify := &coilv2.AddressBlock{} err = k8sClient.Get(ctx, client.ObjectKey{Name: block.Name}, verify) Expect(err).ToNot(HaveOccurred()) @@ -77,6 +85,18 @@ var _ = Describe("PoolManager", func() { Expect(block.IPv6).To(Equal(strPtr("fd02::300/127"))) Expect(block.Labels[constants.LabelNode]).To(Equal("node2")) Expect(block.Labels[constants.LabelPool]).To(Equal("default")) + + blocks[4] = block + for _, b := range blocks { + controllerutil.RemoveFinalizer(b, constants.FinCoil) + err = k8sClient.Update(ctx, b) + Expect(err).ToNot(HaveOccurred()) + err = k8sClient.Delete(ctx, b) + Expect(err).ToNot(HaveOccurred()) + } + used, err = pm.IsUsed(ctx, "default") + Expect(err).ToNot(HaveOccurred()) + Expect(used).To(BeFalse()) }) })