From a54b953ddcc54b5829fbde045035ac1383347b87 Mon Sep 17 00:00:00 2001 From: Andreas Fritzler Date: Wed, 27 Sep 2023 16:08:09 +0200 Subject: [PATCH] Fix ephemeral resource handling - Fix the cleanup of ephemeral resources {nic,prefix,volume,vip} - Update test cases Co-authored-by: Axel Christ --- api/common/v1alpha1/common_types.go | 7 ++++ ...ne_ephemeralnetworkinterface_controller.go | 13 ++++--- ...hemeralnetworkinterface_controller_test.go | 37 ++++++++++++++++++- .../machine_ephemeralvolume_controller.go | 15 ++++++-- ...machine_ephemeralvolume_controller_test.go | 30 +++++++++++++++ ...loadbalancer_ephemeralprefix_controller.go | 11 ++++-- ...alancer_ephemeralprefix_controller_test.go | 35 ++++++++++++++++++ ...orkinterface_ephemeralprefix_controller.go | 11 ++++-- ...terface_ephemeralprefix_controller_test.go | 36 +++++++++++++++++- ...interface_ephemeralvirtualip_controller.go | 13 ++++--- ...face_ephemeralvirtualip_controller_test.go | 36 +++++++++++++++++- utils/annotations/annotations.go | 13 +++++++ 12 files changed, 232 insertions(+), 25 deletions(-) diff --git a/api/common/v1alpha1/common_types.go b/api/common/v1alpha1/common_types.go index 4cec63579..2c2c85775 100644 --- a/api/common/v1alpha1/common_types.go +++ b/api/common/v1alpha1/common_types.go @@ -40,6 +40,13 @@ const ( // ManagedByAnnotation is an annotation that can be applied to resources to signify that // some external system is managing the resource. ManagedByAnnotation = "common.api.onmetal.de/managed-by" + + // EphemeralManagedByAnnotation is an annotation that can be applied to resources to signify that + // some ephemeral controller is managing the resource. + EphemeralManagedByAnnotation = "common.api.onmetal.de/ephemeral-managed-by" + + // DefaultEphemeralManager is the default onmetal-api ephemeral manager. + DefaultEphemeralManager = "ephemeral-manager" ) // ConfigMapKeySelector is a reference to a specific 'key' within a ConfigMap resource. diff --git a/internal/controllers/compute/machine_ephemeralnetworkinterface_controller.go b/internal/controllers/compute/machine_ephemeralnetworkinterface_controller.go index 3356d665f..60c04dbb9 100644 --- a/internal/controllers/compute/machine_ephemeralnetworkinterface_controller.go +++ b/internal/controllers/compute/machine_ephemeralnetworkinterface_controller.go @@ -19,6 +19,9 @@ import ( "errors" "fmt" + "github.com/onmetal/onmetal-api/utils/annotations" + "golang.org/x/exp/maps" + "github.com/go-logr/logr" commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1" @@ -73,22 +76,23 @@ func (r *MachineEphemeralNetworkInterfaceReconciler) ephemeralNetworkInterfaceBy Namespace: machine.Namespace, Name: nicName, Labels: ephemeral.NetworkInterfaceTemplate.Labels, - Annotations: ephemeral.NetworkInterfaceTemplate.Annotations, + Annotations: maps.Clone(ephemeral.NetworkInterfaceTemplate.Annotations), }, Spec: ephemeral.NetworkInterfaceTemplate.Spec, } + annotations.SetDefaultEphemeralManagedBy(nic) + _ = ctrl.SetControllerReference(machine, nic, r.Scheme()) nic.Spec.MachineRef = &commonv1alpha1.LocalUIDReference{ Name: machine.Name, UID: machine.UID, } - _ = ctrl.SetControllerReference(machine, nic, r.Scheme()) res[nicName] = nic } return res } func (r *MachineEphemeralNetworkInterfaceReconciler) handleExistingNetworkInterface(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, nic *networkingv1alpha1.NetworkInterface) error { - if metav1.IsControlledBy(nic, machine) { + if annotations.IsDefaultEphemeralControlledBy(nic, machine) { if shouldManage { log.V(1).Info("Ephemeral network interface is present and controlled by machine") return nil @@ -107,9 +111,8 @@ func (r *MachineEphemeralNetworkInterfaceReconciler) handleExistingNetworkInterf } if shouldManage { - return fmt.Errorf("network interface %s was not created for machine %s (machine is not owner)", nic.Name, machine.Name) + log.V(1).Info("Won't adopt unmanaged network interface") } - // Network interface is not desired but also not controlled by the machine. return nil } diff --git a/internal/controllers/compute/machine_ephemeralnetworkinterface_controller_test.go b/internal/controllers/compute/machine_ephemeralnetworkinterface_controller_test.go index 270bdf5e3..7f97cd91e 100644 --- a/internal/controllers/compute/machine_ephemeralnetworkinterface_controller_test.go +++ b/internal/controllers/compute/machine_ephemeralnetworkinterface_controller_test.go @@ -18,6 +18,7 @@ import ( commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1" networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1" + "github.com/onmetal/onmetal-api/utils/annotations" . "github.com/onmetal/onmetal-api/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -78,7 +79,7 @@ var _ = Describe("MachineEphemeralNetworkInterfaceController", func() { )) }) - It("should delete undesired ephemeral network interfaces", MustPassRepeatedly(10), func(ctx SpecContext) { + It("should delete undesired ephemeral network interfaces", func(ctx SpecContext) { By("creating a machine") machine := &computev1alpha1.Machine{ ObjectMeta: metav1.ObjectMeta{ @@ -104,10 +105,44 @@ var _ = Describe("MachineEphemeralNetworkInterfaceController", func() { }, }, } + annotations.SetDefaultEphemeralManagedBy(undesiredNic) Expect(ctrl.SetControllerReference(machine, undesiredNic, k8sClient.Scheme())).To(Succeed()) Expect(k8sClient.Create(ctx, undesiredNic)).To(Succeed()) By("waiting for the undesired network interface to be gone") Eventually(Get(undesiredNic)).Should(Satisfy(apierrors.IsNotFound)) }) + + It("should not delete an externally managed network interface", func(ctx SpecContext) { + By("creating a machine") + machine := &computev1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "machine-", + }, + Spec: computev1alpha1.MachineSpec{ + MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name}, + }, + } + Expect(k8sClient.Create(ctx, machine)).To(Succeed()) + + By("creating an externally managed network interface") + externalNic := &networkingv1alpha1.NetworkInterface{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "external-nic-", + }, + Spec: networkingv1alpha1.NetworkInterfaceSpec{ + NetworkRef: corev1.LocalObjectReference{Name: "my-network"}, + IPs: []networkingv1alpha1.IPSource{ + {Value: commonv1alpha1.MustParseNewIP("10.0.0.1")}, + }, + }, + } + Expect(ctrl.SetControllerReference(machine, externalNic, k8sClient.Scheme())).To(Succeed()) + Expect(k8sClient.Create(ctx, externalNic)).To(Succeed()) + + By("asserting the network interface is not being deleted") + Consistently(Object(externalNic)).Should(HaveField("DeletionTimestamp", BeNil())) + }) }) diff --git a/internal/controllers/compute/machine_ephemeralvolume_controller.go b/internal/controllers/compute/machine_ephemeralvolume_controller.go index d37062212..240236505 100644 --- a/internal/controllers/compute/machine_ephemeralvolume_controller.go +++ b/internal/controllers/compute/machine_ephemeralvolume_controller.go @@ -18,6 +18,9 @@ import ( "context" "errors" "fmt" + "maps" + + "github.com/onmetal/onmetal-api/utils/annotations" "github.com/go-logr/logr" computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1" @@ -33,6 +36,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) +const ( + MachineEphemeralVolumeManager = "machine-ephemeral-volume" +) + type MachineEphemeralVolumeReconciler struct { client.Client } @@ -73,10 +80,11 @@ func (r *MachineEphemeralVolumeReconciler) ephemeralMachineVolumeByName(machine Namespace: machine.Namespace, Name: volumeName, Labels: ephemeral.VolumeTemplate.Labels, - Annotations: ephemeral.VolumeTemplate.Annotations, + Annotations: maps.Clone(ephemeral.VolumeTemplate.Annotations), }, Spec: ephemeral.VolumeTemplate.Spec, } + annotations.SetDefaultEphemeralManagedBy(volume) _ = ctrl.SetControllerReference(machine, volume, r.Scheme()) res[volumeName] = volume } @@ -84,7 +92,7 @@ func (r *MachineEphemeralVolumeReconciler) ephemeralMachineVolumeByName(machine } func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Context, log logr.Logger, machine *computev1alpha1.Machine, shouldManage bool, volume *storagev1alpha1.Volume) error { - if metav1.IsControlledBy(volume, machine) { + if annotations.IsDefaultEphemeralControlledBy(volume, machine) { if shouldManage { log.V(1).Info("Ephemeral volume is present and controlled by machine") return nil @@ -103,9 +111,8 @@ func (r *MachineEphemeralVolumeReconciler) handleExistingVolume(ctx context.Cont } if shouldManage { - return fmt.Errorf("volume %s was not created for machine %s (machine is not owner)", volume.Name, machine.Name) + log.V(1).Info("Won't adopt unmanaged volume") } - // Volume is not desired but also not controlled by the machine. return nil } diff --git a/internal/controllers/compute/machine_ephemeralvolume_controller_test.go b/internal/controllers/compute/machine_ephemeralvolume_controller_test.go index ee1173fcc..fb41f0737 100644 --- a/internal/controllers/compute/machine_ephemeralvolume_controller_test.go +++ b/internal/controllers/compute/machine_ephemeralvolume_controller_test.go @@ -17,6 +17,7 @@ package compute import ( computev1alpha1 "github.com/onmetal/onmetal-api/api/compute/v1alpha1" storagev1alpha1 "github.com/onmetal/onmetal-api/api/storage/v1alpha1" + "github.com/onmetal/onmetal-api/utils/annotations" . "github.com/onmetal/onmetal-api/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -80,6 +81,7 @@ var _ = Describe("MachineEphemeralVolumeController", func() { }, Spec: storagev1alpha1.VolumeSpec{}, } + annotations.SetDefaultEphemeralManagedBy(undesiredControlledVolume) _ = ctrl.SetControllerReference(machine, undesiredControlledVolume, k8sClient.Scheme()) Expect(k8sClient.Create(ctx, undesiredControlledVolume)).To(Succeed()) @@ -98,4 +100,32 @@ var _ = Describe("MachineEphemeralVolumeController", func() { By("waiting for the undesired controlled volume to be gone") Eventually(Get(undesiredControlledVolume)).Should(Satisfy(apierrors.IsNotFound)) }) + + It("should not delete externally managed volumes for a machine", func(ctx SpecContext) { + By("creating a machine") + machine := &computev1alpha1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "machine-", + }, + Spec: computev1alpha1.MachineSpec{ + MachineClassRef: corev1.LocalObjectReference{Name: machineClass.Name}, + }, + } + Expect(k8sClient.Create(ctx, machine)).To(Succeed()) + + By("creating an undesired controlled volume") + externalVolume := &storagev1alpha1.Volume{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "external-volume-", + }, + Spec: storagev1alpha1.VolumeSpec{}, + } + _ = ctrl.SetControllerReference(machine, externalVolume, k8sClient.Scheme()) + Expect(k8sClient.Create(ctx, externalVolume)).To(Succeed()) + + By("asserting that the external volume is not being deleted") + Consistently(Object(externalVolume)).Should(HaveField("DeletionTimestamp", BeNil())) + }) }) diff --git a/internal/controllers/networking/loadbalancer_ephemeralprefix_controller.go b/internal/controllers/networking/loadbalancer_ephemeralprefix_controller.go index 81fb99c77..9b574a57f 100644 --- a/internal/controllers/networking/loadbalancer_ephemeralprefix_controller.go +++ b/internal/controllers/networking/loadbalancer_ephemeralprefix_controller.go @@ -18,6 +18,9 @@ import ( "context" "errors" "fmt" + "maps" + + "github.com/onmetal/onmetal-api/utils/annotations" "github.com/go-logr/logr" ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1" @@ -74,10 +77,11 @@ func (r *LoadBalancerEphemeralPrefixReconciler) ephemeralLoadBalancerPrefixByNam Namespace: loadBalancer.Namespace, Name: prefixName, Labels: ephemeral.PrefixTemplate.Labels, - Annotations: ephemeral.PrefixTemplate.Annotations, + Annotations: maps.Clone(ephemeral.PrefixTemplate.Annotations), }, Spec: ephemeral.PrefixTemplate.Spec, } + annotations.SetDefaultEphemeralManagedBy(prefix) _ = ctrl.SetControllerReference(loadBalancer, prefix, r.Scheme()) res[prefixName] = prefix } @@ -86,7 +90,7 @@ func (r *LoadBalancerEphemeralPrefixReconciler) ephemeralLoadBalancerPrefixByNam } func (r *LoadBalancerEphemeralPrefixReconciler) handleExistingPrefix(ctx context.Context, log logr.Logger, loadBalancer *networkingv1alpha1.LoadBalancer, shouldManage bool, prefix *ipamv1alpha1.Prefix) error { - if metav1.IsControlledBy(prefix, loadBalancer) { + if annotations.IsDefaultEphemeralControlledBy(prefix, loadBalancer) { if shouldManage { log.V(1).Info("Ephemeral prefix is present and controlled by load balancer") return nil @@ -105,9 +109,8 @@ func (r *LoadBalancerEphemeralPrefixReconciler) handleExistingPrefix(ctx context } if shouldManage { - return fmt.Errorf("prefix %s was not created for load balancer %s (load balancer is not owner)", prefix.Name, loadBalancer.Name) + log.V(1).Info("Won't adopt unmanaged prefix") } - // Prefix is not desired but also not controlled by the load balancer. return nil } diff --git a/internal/controllers/networking/loadbalancer_ephemeralprefix_controller_test.go b/internal/controllers/networking/loadbalancer_ephemeralprefix_controller_test.go index aea39e203..6d2f8c35f 100644 --- a/internal/controllers/networking/loadbalancer_ephemeralprefix_controller_test.go +++ b/internal/controllers/networking/loadbalancer_ephemeralprefix_controller_test.go @@ -19,6 +19,7 @@ import ( commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1" networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1" + "github.com/onmetal/onmetal-api/utils/annotations" . "github.com/onmetal/onmetal-api/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -100,10 +101,44 @@ var _ = Describe("LoadBalancerEphemeralPrefix", func() { Prefix: commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"), }, } + annotations.SetDefaultEphemeralManagedBy(prefix) Expect(ctrl.SetControllerReference(loadBalancer, prefix, k8sClient.Scheme())).To(Succeed()) Expect(k8sClient.Create(ctx, prefix)).To(Succeed()) By("waiting for the prefix to be marked for deletion") Eventually(Get(prefix)).Should(Satisfy(apierrors.IsNotFound)) }) + + It("should not delete externally managed prefix for a load balancer", MustPassRepeatedly(10), func(ctx SpecContext) { + By("creating a load balancer") + loadBalancer := &networkingv1alpha1.LoadBalancer{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "load-balancer-", + }, + Spec: networkingv1alpha1.LoadBalancerSpec{ + Type: networkingv1alpha1.LoadBalancerTypeInternal, + NetworkRef: corev1.LocalObjectReference{Name: "my-network"}, + IPs: []networkingv1alpha1.IPSource{{Value: commonv1alpha1.MustParseNewIP("10.0.0.1")}}, + }, + } + Expect(k8sClient.Create(ctx, loadBalancer)).To(Succeed()) + + By("creating an undesired prefix") + externalPrefix := &ipamv1alpha1.Prefix{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "external-prefix-", + }, + Spec: ipamv1alpha1.PrefixSpec{ + IPFamily: corev1.IPv4Protocol, + Prefix: commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"), + }, + } + Expect(ctrl.SetControllerReference(loadBalancer, externalPrefix, k8sClient.Scheme())).To(Succeed()) + Expect(k8sClient.Create(ctx, externalPrefix)).To(Succeed()) + + By("asserting that the external prefix is not being deleted") + Eventually(Object(externalPrefix)).Should(HaveField("DeletionTimestamp", BeNil())) + }) }) diff --git a/internal/controllers/networking/networkinterface_ephemeralprefix_controller.go b/internal/controllers/networking/networkinterface_ephemeralprefix_controller.go index 76597625f..d14e16e62 100644 --- a/internal/controllers/networking/networkinterface_ephemeralprefix_controller.go +++ b/internal/controllers/networking/networkinterface_ephemeralprefix_controller.go @@ -18,6 +18,9 @@ import ( "context" "errors" "fmt" + "maps" + + "github.com/onmetal/onmetal-api/utils/annotations" "github.com/go-logr/logr" ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1" @@ -74,10 +77,11 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) ephemeralNetworkInterfacePre Namespace: nic.Namespace, Name: prefixName, Labels: ephemeral.PrefixTemplate.Labels, - Annotations: ephemeral.PrefixTemplate.Annotations, + Annotations: maps.Clone(ephemeral.PrefixTemplate.Annotations), }, Spec: ephemeral.PrefixTemplate.Spec, } + annotations.SetDefaultEphemeralManagedBy(prefix) _ = ctrl.SetControllerReference(nic, prefix, r.Scheme()) res[prefixName] = prefix } @@ -106,7 +110,7 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) ephemeralNetworkInterfacePre } func (r *NetworkInterfaceEphemeralPrefixReconciler) handleExistingPrefix(ctx context.Context, log logr.Logger, nic *networkingv1alpha1.NetworkInterface, shouldManage bool, prefix *ipamv1alpha1.Prefix) error { - if metav1.IsControlledBy(prefix, nic) { + if annotations.IsDefaultEphemeralControlledBy(prefix, nic) { if shouldManage { log.V(1).Info("Ephemeral prefix is present and controlled by network interface") return nil @@ -125,9 +129,8 @@ func (r *NetworkInterfaceEphemeralPrefixReconciler) handleExistingPrefix(ctx con } if shouldManage { - return fmt.Errorf("prefix %s was not created for network interface %s (network interface is not owner)", prefix.Name, nic.Name) + log.V(1).Info("Won't adopt unmanaged prefix") } - // Prefix is not desired but also not controlled by the networkInterface. return nil } diff --git a/internal/controllers/networking/networkinterface_ephemeralprefix_controller_test.go b/internal/controllers/networking/networkinterface_ephemeralprefix_controller_test.go index 7925c7b33..06da1ef1a 100644 --- a/internal/controllers/networking/networkinterface_ephemeralprefix_controller_test.go +++ b/internal/controllers/networking/networkinterface_ephemeralprefix_controller_test.go @@ -19,6 +19,7 @@ import ( commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" ipamv1alpha1 "github.com/onmetal/onmetal-api/api/ipam/v1alpha1" networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1" + "github.com/onmetal/onmetal-api/utils/annotations" . "github.com/onmetal/onmetal-api/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -73,7 +74,7 @@ var _ = Describe("NetworkInterfaceEphemeralPrefix", func() { )) }) - It("should delete undesired prefixes for a network interface", MustPassRepeatedly(10), func(ctx SpecContext) { + It("should delete undesired prefixes for a network interface", func(ctx SpecContext) { By("creating a network interface") nic := &networkingv1alpha1.NetworkInterface{ ObjectMeta: metav1.ObjectMeta{ @@ -98,10 +99,43 @@ var _ = Describe("NetworkInterfaceEphemeralPrefix", func() { Prefix: commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"), }, } + annotations.SetDefaultEphemeralManagedBy(prefix) Expect(ctrl.SetControllerReference(nic, prefix, k8sClient.Scheme())).To(Succeed()) Expect(k8sClient.Create(ctx, prefix)).To(Succeed()) By("waiting for the prefix to be marked for deletion") Eventually(Get(prefix)).Should(Satisfy(apierrors.IsNotFound)) }) + + It("should not delete externally managed prefix for a network interface", func(ctx SpecContext) { + By("creating a network interface") + nic := &networkingv1alpha1.NetworkInterface{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "nic-", + }, + Spec: networkingv1alpha1.NetworkInterfaceSpec{ + NetworkRef: corev1.LocalObjectReference{Name: "my-network"}, + IPs: []networkingv1alpha1.IPSource{{Value: commonv1alpha1.MustParseNewIP("10.0.0.1")}}, + }, + } + Expect(k8sClient.Create(ctx, nic)).To(Succeed()) + + By("creating an undesired prefix") + externalPrefix := &ipamv1alpha1.Prefix{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "external-prefix-", + }, + Spec: ipamv1alpha1.PrefixSpec{ + IPFamily: corev1.IPv4Protocol, + Prefix: commonv1alpha1.MustParseNewIPPrefix("10.0.0.1/32"), + }, + } + Expect(ctrl.SetControllerReference(nic, externalPrefix, k8sClient.Scheme())).To(Succeed()) + Expect(k8sClient.Create(ctx, externalPrefix)).To(Succeed()) + + By("asserting that the prefix is not being deleted") + Eventually(Object(externalPrefix)).Should(HaveField("DeletionTimestamp", BeNil())) + }) }) diff --git a/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller.go b/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller.go index 89288cc3d..29c73677d 100644 --- a/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller.go +++ b/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller.go @@ -18,6 +18,9 @@ import ( "context" "errors" "fmt" + "maps" + + "github.com/onmetal/onmetal-api/utils/annotations" "github.com/go-logr/logr" commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" @@ -77,22 +80,23 @@ func (r *NetworkInterfaceEphemeralVirtualIPReconciler) ephemeralNetworkInterface Namespace: nic.Namespace, Name: virtualIPName, Labels: ephemeral.VirtualIPTemplate.Labels, - Annotations: ephemeral.VirtualIPTemplate.Annotations, + Annotations: maps.Clone(ephemeral.VirtualIPTemplate.Annotations), }, Spec: ephemeral.VirtualIPTemplate.Spec, } + annotations.SetDefaultEphemeralManagedBy(virtualIP) + _ = ctrl.SetControllerReference(nic, virtualIP, r.Scheme()) virtualIP.Spec.TargetRef = &commonv1alpha1.LocalUIDReference{ Name: nic.Name, UID: nic.UID, } - _ = ctrl.SetControllerReference(nic, virtualIP, r.Scheme()) res[virtualIPName] = virtualIP return res } func (r *NetworkInterfaceEphemeralVirtualIPReconciler) handleExistingVirtualIP(ctx context.Context, log logr.Logger, nic *networkingv1alpha1.NetworkInterface, shouldManage bool, virtualIP *networkingv1alpha1.VirtualIP) error { - if metav1.IsControlledBy(virtualIP, nic) { + if annotations.IsDefaultEphemeralControlledBy(virtualIP, nic) { if shouldManage { log.V(1).Info("Ephemeral virtual IP is present and controlled by network interface") return nil @@ -111,9 +115,8 @@ func (r *NetworkInterfaceEphemeralVirtualIPReconciler) handleExistingVirtualIP(c } if shouldManage { - return fmt.Errorf("virtual IP %s was not created for network interface %s (network interface is not owner)", virtualIP.Name, nic.Name) + log.V(1).Info("Won't adopt unmanaged virtual IP") } - // VirtualIP is not desired but also not controlled by the networkInterface. return nil } diff --git a/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller_test.go b/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller_test.go index e3fb2e584..a1ad8da59 100644 --- a/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller_test.go +++ b/internal/controllers/networking/networkinterface_ephemeralvirtualip_controller_test.go @@ -18,6 +18,7 @@ package networking import ( commonv1alpha1 "github.com/onmetal/onmetal-api/api/common/v1alpha1" networkingv1alpha1 "github.com/onmetal/onmetal-api/api/networking/v1alpha1" + "github.com/onmetal/onmetal-api/utils/annotations" . "github.com/onmetal/onmetal-api/utils/testing" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -80,7 +81,7 @@ var _ = Describe("NetworkInterfaceEphemeralVirtualIP", func() { )) }) - It("should delete undesired virtual IPs for a network interface", MustPassRepeatedly(10), func(ctx SpecContext) { + It("should delete undesired virtual IPs for a network interface", func(ctx SpecContext) { By("creating a network interface") nic := &networkingv1alpha1.NetworkInterface{ ObjectMeta: metav1.ObjectMeta{ @@ -105,10 +106,43 @@ var _ = Describe("NetworkInterfaceEphemeralVirtualIP", func() { IPFamily: corev1.IPv4Protocol, }, } + annotations.SetDefaultEphemeralManagedBy(vip) Expect(ctrl.SetControllerReference(nic, vip, k8sClient.Scheme())).To(Succeed()) Expect(k8sClient.Create(ctx, vip)).To(Succeed()) By("waiting for the virtual IP to be gone") Eventually(Get(vip)).Should(Satisfy(apierrors.IsNotFound)) }) + + It("should not delete externally managed virtual IPs for a network interface", func(ctx SpecContext) { + By("creating a network interface") + nic := &networkingv1alpha1.NetworkInterface{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "nic-", + }, + Spec: networkingv1alpha1.NetworkInterfaceSpec{ + NetworkRef: corev1.LocalObjectReference{Name: "my-network"}, + IPs: []networkingv1alpha1.IPSource{{Value: commonv1alpha1.MustParseNewIP("10.0.0.1")}}, + }, + } + Expect(k8sClient.Create(ctx, nic)).To(Succeed()) + + By("creating an undesired virtual IP") + externalVip := &networkingv1alpha1.VirtualIP{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns.Name, + GenerateName: "external-vip-", + }, + Spec: networkingv1alpha1.VirtualIPSpec{ + Type: networkingv1alpha1.VirtualIPTypePublic, + IPFamily: corev1.IPv4Protocol, + }, + } + Expect(ctrl.SetControllerReference(nic, externalVip, k8sClient.Scheme())).To(Succeed()) + Expect(k8sClient.Create(ctx, externalVip)).To(Succeed()) + + By("asserting that the virtual IP is not being deleted") + Eventually(Object(externalVip)).Should(HaveField("DeletionTimestamp", BeNil())) + }) }) diff --git a/utils/annotations/annotations.go b/utils/annotations/annotations.go index 225dcf825..1a5c465fb 100644 --- a/utils/annotations/annotations.go +++ b/utils/annotations/annotations.go @@ -38,6 +38,19 @@ func IsExternallyManaged(o metav1.Object) bool { return metautils.HasAnnotation(o, commonv1alpha1.ManagedByAnnotation) } +func IsEphemeralManagedBy(o metav1.Object, manager string) bool { + actual, ok := o.GetAnnotations()[commonv1alpha1.EphemeralManagedByAnnotation] + return ok && actual == manager +} + +func IsDefaultEphemeralControlledBy(o metav1.Object, owner metav1.Object) bool { + return metav1.IsControlledBy(o, owner) && IsEphemeralManagedBy(o, commonv1alpha1.DefaultEphemeralManager) +} + +func SetDefaultEphemeralManagedBy(o metav1.Object) { + metautils.SetAnnotation(o, commonv1alpha1.EphemeralManagedByAnnotation, commonv1alpha1.DefaultEphemeralManager) +} + func SetExternallyMangedBy(o metav1.Object, manager string) { metautils.SetAnnotation(o, commonv1alpha1.ManagedByAnnotation, manager) }