From 8753cb001a260d504f3daac7fda74f318816465c Mon Sep 17 00:00:00 2001 From: Tyler Schultz Date: Sat, 11 Mar 2023 00:34:40 +0000 Subject: [PATCH] Ensure existing IPAddresses finalizer and ownerRefs 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 Co-authored-by: Aidan Obley Co-authored-by: Christian Ang Signed-off-by: Tyler Schultz --- internal/controllers/ipaddressclaim.go | 28 ++- internal/controllers/ipaddressclaim_test.go | 223 ++++++++++++++++++++ internal/poolutil/pool.go | 2 +- pkg/ipamutil/address.go | 43 +++- 4 files changed, 275 insertions(+), 21 deletions(-) diff --git a/internal/controllers/ipaddressclaim.go b/internal/controllers/ipaddressclaim.go index e6b5a1d..96f951c 100644 --- a/internal/controllers/ipaddressclaim.go +++ b/internal/controllers/ipaddressclaim.go @@ -160,17 +160,33 @@ func (r *IPAddressClaimReconciler) reconcile(ctx context.Context, claim *ipamv1. return ctrl.Result{}, nil } - log = log.WithValues("pool name", pool.GetName()) + log = log.WithValues(pool.GetObjectKind().GroupVersionKind().Kind, fmt.Sprintf("%s/%s", pool.GetNamespace(), pool.GetName())) address := poolutil.AddressByName(addressesInUse, claim.Name) if address == nil { var err error address, err = r.allocateAddress(ctx, claim, pool, addressesInUse) if err != nil { - log.Error(err, "failed to allocate address") - return ctrl.Result{}, err + return ctrl.Result{}, errors.Wrap(err, "failed to allocate address") } } + + operationResult, err := controllerutil.CreateOrPatch(ctx, r.Client, address, func() error { + if err := ipamutil.EnsureIPAddressOwnerReferences(r.Scheme, address, claim, pool); err != nil { + return errors.Wrap(err, "failed to ensure owner references on address") + } + + _ = controllerutil.AddFinalizer(address, ProtectAddressFinalizer) + + return nil + }) + if err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to create or patch address") + } + + log.Info(fmt.Sprintf("IPAddress %s/%s (%s) has been %s", address.Namespace, address.Name, address.Spec.Address, operationResult), + "IPAddressClaim", fmt.Sprintf("%s/%s", claim.Namespace, claim.Name)) + if !address.DeletionTimestamp.IsZero() { // We prevent deleting IPAddresses while their corresponding IPClaim still exists since we cannot guarantee that the IP // wil remain the same when we recreate it. @@ -224,11 +240,5 @@ func (r *IPAddressClaimReconciler) allocateAddress(ctx context.Context, claim *i address.Spec.Gateway = poolSpec.Gateway address.Spec.Prefix = poolSpec.Prefix - controllerutil.AddFinalizer(&address, ProtectAddressFinalizer) - - if err := r.Client.Create(ctx, &address); err != nil { - return nil, fmt.Errorf("failed to create IPAddress: %w", err) - } - return &address, nil } diff --git a/internal/controllers/ipaddressclaim_test.go b/internal/controllers/ipaddressclaim_test.go index 825cdf7..dc66a58 100644 --- a/internal/controllers/ipaddressclaim_test.go +++ b/internal/controllers/ipaddressclaim_test.go @@ -19,6 +19,7 @@ var IgnoreUIDsOnIPAddress = IgnorePaths{ "TypeMeta", "ObjectMeta.OwnerReferences[0].UID", "ObjectMeta.OwnerReferences[1].UID", + "ObjectMeta.OwnerReferences[2].UID", "Spec.Claim.UID", "Spec.Pool.UID", } @@ -852,6 +853,228 @@ var _ = Describe("IPAddressClaimReconciler", func() { }) }) }) + + Context("When an existing IPAddress with no ownerReferences is missing finalizers and owner references", func() { + const poolName = "test-pool" + + BeforeEach(func() { + pool := v1alpha1.InClusterIPPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: poolName, + Namespace: namespace, + }, + Spec: v1alpha1.InClusterIPPoolSpec{ + First: "10.0.0.1", + Last: "10.0.0.254", + Prefix: 24, + Gateway: "10.0.0.2", + }, + } + Expect(k8sClient.Create(context.Background(), &pool)).To(Succeed()) + Eventually(Get(&pool)).Should(Succeed()) + }) + + AfterEach(func() { + deleteNamespacedPool(poolName, namespace) + deleteClaim("test", namespace) + }) + + It("should add the owner references and finalizer", func() { + claim := clusterv1.IPAddressClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + Spec: clusterv1.IPAddressClaimSpec{ + PoolRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("ipam.cluster.x-k8s.io"), + Kind: "InClusterIPPool", + Name: poolName, + }, + }, + } + + addressSpec := clusterv1.IPAddressSpec{ + ClaimRef: corev1.LocalObjectReference{ + Name: "test", + }, + PoolRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("ipam.cluster.x-k8s.io"), + Kind: "InClusterIPPool", + Name: poolName, + }, + Address: "10.0.0.1", + Prefix: 24, + Gateway: "10.0.0.2", + } + + address := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + Spec: addressSpec, + } + + Expect(k8sClient.Create(context.Background(), &address)).To(Succeed()) + Expect(k8sClient.Create(context.Background(), &claim)).To(Succeed()) + + expectedIPAddress := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + Finalizers: []string{ProtectAddressFinalizer}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "ipam.cluster.x-k8s.io/v1alpha1", + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + Kind: "IPAddressClaim", + Name: "test", + }, + { + APIVersion: "ipam.cluster.x-k8s.io/v1alpha1", + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(false), + Kind: "InClusterIPPool", + Name: poolName, + }, + }, + }, + Spec: addressSpec, + } + + actual := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + } + + Eventually(Object(&actual)). + WithTimeout(time.Second).WithPolling(100 * time.Millisecond).Should( + EqualObject(&expectedIPAddress, IgnoreAutogeneratedMetadata, IgnoreUIDsOnIPAddress), + ) + }) + }) + + Context("When an existing IPAddress with an unrelated ownerRef is missing finalizers and IPAM owner references", func() { + const poolName = "test-pool" + + BeforeEach(func() { + pool := v1alpha1.InClusterIPPool{ + ObjectMeta: metav1.ObjectMeta{ + Name: poolName, + Namespace: namespace, + }, + Spec: v1alpha1.InClusterIPPoolSpec{ + First: "10.0.0.1", + Last: "10.0.0.254", + Prefix: 24, + Gateway: "10.0.0.2", + }, + } + Expect(k8sClient.Create(context.Background(), &pool)).To(Succeed()) + Eventually(Get(&pool)).Should(Succeed()) + }) + + AfterEach(func() { + deleteNamespacedPool(poolName, namespace) + deleteClaim("test", namespace) + }) + + It("should add the owner references and finalizer", func() { + claim := clusterv1.IPAddressClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + Spec: clusterv1.IPAddressClaimSpec{ + PoolRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("ipam.cluster.x-k8s.io"), + Kind: "InClusterIPPool", + Name: poolName, + }, + }, + } + + addressSpec := clusterv1.IPAddressSpec{ + ClaimRef: corev1.LocalObjectReference{ + Name: "test", + }, + PoolRef: corev1.TypedLocalObjectReference{ + APIGroup: pointer.String("ipam.cluster.x-k8s.io"), + Kind: "InClusterIPPool", + Name: poolName, + }, + Address: "10.0.0.1", + Prefix: 24, + Gateway: "10.0.0.2", + } + + address := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "alpha-dummy", + Kind: "dummy-kind", + Name: "dummy-name", + UID: "abc-dummy-123", + }, + }, + }, + Spec: addressSpec, + } + + Expect(k8sClient.Create(context.Background(), &address)).To(Succeed()) + Expect(k8sClient.Create(context.Background(), &claim)).To(Succeed()) + + expectedIPAddress := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + Finalizers: []string{ProtectAddressFinalizer}, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "alpha-dummy", + Kind: "dummy-kind", + Name: "dummy-name", + UID: "abc-dummy-123", + }, + { + APIVersion: "ipam.cluster.x-k8s.io/v1alpha1", + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(true), + Kind: "IPAddressClaim", + Name: "test", + }, + { + APIVersion: "ipam.cluster.x-k8s.io/v1alpha1", + BlockOwnerDeletion: pointer.Bool(true), + Controller: pointer.Bool(false), + Kind: "InClusterIPPool", + Name: poolName, + }, + }, + }, + Spec: addressSpec, + } + + actual := clusterv1.IPAddress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: namespace, + }, + } + + Eventually(Object(&actual)). + WithTimeout(time.Second).WithPolling(100 * time.Millisecond).Should( + EqualObject(&expectedIPAddress, IgnoreAutogeneratedMetadata, IgnoreUIDsOnIPAddress), + ) + }) + }) }) func createNamespace() string { diff --git a/internal/poolutil/pool.go b/internal/poolutil/pool.go index 3e4a363..1433295 100644 --- a/internal/poolutil/pool.go +++ b/internal/poolutil/pool.go @@ -3,10 +3,10 @@ package poolutil import ( "context" - "errors" "fmt" "strings" + "github.com/pkg/errors" "inet.af/netaddr" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime/schema" diff --git a/pkg/ipamutil/address.go b/pkg/ipamutil/address.go index 9ffe230..2a13a33 100644 --- a/pkg/ipamutil/address.go +++ b/pkg/ipamutil/address.go @@ -2,11 +2,14 @@ package ipamutil import ( + "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/utils/pointer" ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) // NewIPAddress creates a new ipamv1.IPAddress with references to a pool and claim. @@ -17,17 +20,6 @@ func NewIPAddress(claim *ipamv1.IPAddressClaim, pool client.Object) ipamv1.IPAdd ObjectMeta: metav1.ObjectMeta{ Name: claim.Name, Namespace: claim.Namespace, - OwnerReferences: []metav1.OwnerReference{ - *metav1.NewControllerRef(claim, claim.GetObjectKind().GroupVersionKind()), - { - APIVersion: pool.GetObjectKind().GroupVersionKind().GroupVersion().String(), - Kind: pool.GetObjectKind().GroupVersionKind().Kind, - Name: pool.GetName(), - UID: pool.GetUID(), - BlockOwnerDeletion: pointer.Bool(true), - Controller: pointer.Bool(false), - }, - }, }, Spec: ipamv1.IPAddressSpec{ ClaimRef: corev1.LocalObjectReference{ @@ -41,3 +33,32 @@ func NewIPAddress(claim *ipamv1.IPAddressClaim, pool client.Object) ipamv1.IPAdd }, } } + +// EnsureIPAddressOwnerReferences ensures that an IPAddress has the +// IPAddressClaim and IPPool as an OwnerReference. +func EnsureIPAddressOwnerReferences(scheme *runtime.Scheme, address *ipamv1.IPAddress, claim *ipamv1.IPAddressClaim, pool client.Object) error { + if err := controllerutil.SetControllerReference(claim, address, scheme); err != nil { + if _, ok := err.(*controllerutil.AlreadyOwnedError); !ok { + return errors.Wrap(err, "Failed to update address's claim owner reference") + } + } + + if err := controllerutil.SetOwnerReference(pool, address, scheme); err != nil { + return errors.Wrap(err, "Failed to update address's pool owner reference") + } + + var poolRefIdx int + poolGVK := pool.GetObjectKind().GroupVersionKind() + 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) + + return nil +}