diff --git a/internal/controllers/ipaddressclaim.go b/internal/controllers/ipaddressclaim.go index e6b5a1d..068150c 100644 --- a/internal/controllers/ipaddressclaim.go +++ b/internal/controllers/ipaddressclaim.go @@ -11,6 +11,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/utils/pointer" ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1alpha1" "sigs.k8s.io/cluster-api/util/patch" "sigs.k8s.io/cluster-api/util/predicates" @@ -165,12 +166,27 @@ func (r *IPAddressClaimReconciler) reconcile(ctx context.Context, claim *ipamv1. address := poolutil.AddressByName(addressesInUse, claim.Name) if address == nil { var err error - address, err = r.allocateAddress(ctx, claim, pool, addressesInUse) + address, err = r.createAddress(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 create address") } + + log.Info("Allocated IPAddress for IPAddressClaim", + "IPAddressClaim", fmt.Sprintf("%s/%s", claim.Namespace, claim.Name), + "IPAddress", fmt.Sprintf("%s/%s %s", address.Namespace, address.Name, address.Spec.Address), + "InClusterIPPool", pool.GetName()) + } else { + err := r.patchAddressOwnerRefsAndFinalizer(ctx, address, claim, pool) + if err != nil { + return ctrl.Result{}, errors.Wrap(err, "failed to patch address ownerRefs and finalizer") + } + + log.Info("Found existing IPAddress for IPAddressClaim", + "IPAddressClaim", fmt.Sprintf("%s/%s", claim.Namespace, claim.Name), + "IPAddress", fmt.Sprintf("%s/%s %s", address.Namespace, address.Name, address.Spec.Address), + "InClusterIPPool", pool.GetName()) } + 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. @@ -182,6 +198,68 @@ func (r *IPAddressClaimReconciler) reconcile(ctx context.Context, claim *ipamv1. return ctrl.Result{}, nil } +func (r *IPAddressClaimReconciler) patchAddressOwnerRefsAndFinalizer(ctx context.Context, address *ipamv1.IPAddress, claim *ipamv1.IPAddressClaim, pool genericInClusterPool) error { + patchHelper, err := patch.NewHelper(address, r.Client) + if err != nil { + return errors.Wrap(err, "failed to initialize patch helper") + } + + if err := r.ensureOwnerRefsAndFinalizer(ctx, address, claim, pool); err != nil { + return errors.Wrap(err, "failed to ensure owner refs and finalizer on address") + } + + if err := patchHelper.Patch(ctx, address); err != nil { + return errors.Wrap(err, "failed to patch address") + } + return nil +} + +func (r *IPAddressClaimReconciler) createAddress(ctx context.Context, claim *ipamv1.IPAddressClaim, pool genericInClusterPool, addressesInUse []ipamv1.IPAddress) (*ipamv1.IPAddress, error) { + address, err := r.allocateAddress(ctx, claim, pool, addressesInUse) + if err != nil { + return nil, errors.Wrap(err, "failed to allocate address") + } + + if err := r.ensureOwnerRefsAndFinalizer(ctx, address, claim, pool); err != nil { + return nil, errors.Wrap(err, "failed to ensure owner refs and finalizer on address") + } + + if err := r.Client.Create(ctx, address); err != nil { + return nil, errors.Wrap(err, "failed to create IPAddress") + } + + return address, nil +} + +func (r *IPAddressClaimReconciler) ensureOwnerRefsAndFinalizer(ctx context.Context, address *ipamv1.IPAddress, claim *ipamv1.IPAddressClaim, pool genericInClusterPool) error { + if err := controllerutil.SetControllerReference(claim, address, r.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, r.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) + + _ = controllerutil.AddFinalizer(address, ProtectAddressFinalizer) + + return nil +} + func (r *IPAddressClaimReconciler) reconcileDelete(ctx context.Context, claim *ipamv1.IPAddressClaim, address *ipamv1.IPAddress) (ctrl.Result, error) { if address.Name != "" { var err error @@ -224,11 +302,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..c2b2c8b 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,124 @@ var _ = Describe("IPAddressClaimReconciler", func() { }) }) }) + + Context("When an existing IPAddress is missing finalizers and owner refs", 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/pkg/ipamutil/address.go b/pkg/ipamutil/address.go index 9ffe230..f35f714 100644 --- a/pkg/ipamutil/address.go +++ b/pkg/ipamutil/address.go @@ -4,7 +4,6 @@ package ipamutil import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/pointer" ipamv1 "sigs.k8s.io/cluster-api/exp/ipam/api/v1alpha1" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -17,17 +16,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{