From 46a3b7df3b41924549fcf8f1d3fd21d6dccce49e Mon Sep 17 00:00:00 2001 From: "Hoanganh.Mai" Date: Mon, 23 Sep 2024 11:20:38 +0200 Subject: [PATCH 1/3] add check for tenant existence --- internal/controller/ipaddress_controller.go | 20 ++++++++++---------- internal/controller/prefix_controller.go | 20 ++++++++++---------- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index ad51a8cc..4818c467 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -78,7 +78,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // if being deleted if !o.ObjectMeta.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) { - if !o.Spec.PreserveInNetbox { + if o.Status.IpAddressId != 0 { err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId) if err != nil { return ctrl.Result{}, err @@ -101,6 +101,15 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, nil } + // if PreserveIpInNetbox flag is false then register finalizer if not yet registered + if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) { + debugLogger.Info("adding the finalizer") + controllerutil.AddFinalizer(o, IpAddressFinalizerName) + if err := r.Update(ctx, o); err != nil { + return ctrl.Result{}, err + } + } + // 1. try to lock lease of parent prefix if IpAddress status condition is not true // and IpAddress is owned by an IpAddressClaim or := o.ObjectMeta.OwnerReferences @@ -208,15 +217,6 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( r.Recorder.Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description") } - // if PreserveIpInNetbox flag is false then register finalizer if not yet registered - if !o.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) { - debugLogger.Info("adding the finalizer") - controllerutil.AddFinalizer(o, IpAddressFinalizerName) - if err := r.Update(ctx, o); err != nil { - return ctrl.Result{}, err - } - } - debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress)) err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, "") diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 7837d859..56571164 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -79,7 +79,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // if being deleted if !prefix.ObjectMeta.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { - if !prefix.Spec.PreserveInNetbox { + if prefix.Status.PrefixId != 0 { if err := r.NetboxClient.DeletePrefix(prefix.Status.PrefixId); err != nil { return ctrl.Result{}, err } @@ -99,6 +99,15 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } + // register finalizer if not yet registered + if !prefix.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { + debugLogger.Info("adding the finalizer") + controllerutil.AddFinalizer(prefix, PrefixFinalizerName) + if err := r.Update(ctx, prefix); err != nil { + return ctrl.Result{}, err + } + } + /* 1. try to lock the lease of the parent prefix if all of the following conditions are met - the prefix is owned by at least 1 prefixClaim @@ -203,15 +212,6 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr r.Recorder.Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description") } - // register finalizer if not yet registered - if !prefix.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { - debugLogger.Info("adding the finalizer") - controllerutil.AddFinalizer(prefix, PrefixFinalizerName) - if err := r.Update(ctx, prefix); err != nil { - return ctrl.Result{}, err - } - } - debugLogger.Info(fmt.Sprintf("reserved prefix in netbox, prefix: %s", prefix.Spec.Prefix)) if err = r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyTrue, corev1.EventTypeNormal, ""); err != nil { From 7c2b29e5eea310868e514031b68c112ec9303491 Mon Sep 17 00:00:00 2001 From: "Hoanganh.Mai" Date: Mon, 30 Sep 2024 11:37:58 +0200 Subject: [PATCH 2/3] update deletion for finalizers and move finalizer creation up --- internal/controller/ipaddress_controller.go | 11 ++++++++++- internal/controller/prefix_controller.go | 11 ++++++++++- pkg/netbox/api/ip_address.go | 8 ++++++++ pkg/netbox/api/prefix.go | 10 ++++++++++ 4 files changed, 38 insertions(+), 2 deletions(-) diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index 4818c467..c83fe7dd 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -30,6 +30,7 @@ import ( "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/api" "github.com/netbox-community/netbox-operator/pkg/netbox/models" + "github.com/netbox-community/netbox-operator/pkg/netbox/utils" "github.com/swisscom/leaselocker" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" @@ -78,9 +79,17 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // if being deleted if !o.ObjectMeta.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(o, IpAddressFinalizerName) { - if o.Status.IpAddressId != 0 { + if !o.Spec.PreserveInNetbox { err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId) if err != nil { + if errors.Is(err, utils.ErrNotFound) { + setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeWarning, err.Error()) + if setConditionErr != nil { + return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting IPAddress failed: %w", setConditionErr, err) + } + + return ctrl.Result{Requeue: true}, nil + } return ctrl.Result{}, err } } diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 56571164..fe1213fa 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -28,6 +28,7 @@ import ( "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/models" + "github.com/netbox-community/netbox-operator/pkg/netbox/utils" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -79,8 +80,16 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // if being deleted if !prefix.ObjectMeta.DeletionTimestamp.IsZero() { if controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { - if prefix.Status.PrefixId != 0 { + if !prefix.Spec.PreserveInNetbox { if err := r.NetboxClient.DeletePrefix(prefix.Status.PrefixId); err != nil { + if errors.Is(err, utils.ErrNotFound) { + setConditionErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, err.Error()) + if setConditionErr != nil { + return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting Prefix failed: %w", setConditionErr, err) + } + + return ctrl.Result{Requeue: true}, nil + } return ctrl.Result{}, err } } diff --git a/pkg/netbox/api/ip_address.go b/pkg/netbox/api/ip_address.go index 225d14d8..ffcb0bda 100644 --- a/pkg/netbox/api/ip_address.go +++ b/pkg/netbox/api/ip_address.go @@ -17,6 +17,7 @@ limitations under the License. package api import ( + "strings" "unicode/utf8" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" @@ -97,9 +98,16 @@ func (r *NetboxClient) UpdateIpAddress(ipAddressId int64, ipAddress *netboxModel } func (r *NetboxClient) DeleteIpAddress(ipAddressId int64) error { + // assuming id starts from 1 when IPAdress is created so 0 means that IPAddress doesn't exist + if ipAddressId == 0 { + return nil + } requestDeleteIp := ipam.NewIpamIPAddressesDeleteParams().WithID(ipAddressId) _, err := r.Ipam.IpamIPAddressesDelete(requestDeleteIp, nil) if err != nil { + if strings.Contains(err.Error(), "No IPAddress matches the given query.") { + return utils.NetboxNotFoundError("IPAddress") + } return utils.NetboxError("Failed to delete IP Address from Netbox", err) } return nil diff --git a/pkg/netbox/api/prefix.go b/pkg/netbox/api/prefix.go index 5d3def2e..cc344d50 100644 --- a/pkg/netbox/api/prefix.go +++ b/pkg/netbox/api/prefix.go @@ -17,6 +17,8 @@ limitations under the License. package api import ( + "strings" + "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" @@ -98,9 +100,17 @@ func (r *NetboxClient) UpdatePrefix(prefixId int64, prefix *netboxModels.Writabl } func (r *NetboxClient) DeletePrefix(prefixId int64) error { + // assuming id starts from 1 when Prefix is created so 0 means that Prefix doesn't exist + if prefixId == 0 { + return nil + } + requestDeletePrefix := ipam.NewIpamPrefixesDeleteParams().WithID(prefixId) _, err := r.Ipam.IpamPrefixesDelete(requestDeletePrefix, nil) if err != nil { + if strings.Contains(err.Error(), "No Prefix matches the given query.") { + return utils.NetboxNotFoundError("Prefix") + } return utils.NetboxError("failed to delete Prefix", err) } return nil From 8fa8024726356ad6dee3ac9eb8e4f40b3f55c4e9 Mon Sep 17 00:00:00 2001 From: "Hoanganh.Mai" Date: Mon, 30 Sep 2024 11:37:58 +0200 Subject: [PATCH 3/3] update deletion for finalizers and move finalizer creation up --- api/v1/ipaddress_types.go | 7 +++++++ api/v1/prefix_types.go | 7 +++++++ .../expected_netboxmock_calls_test.go | 15 +++++++++++++++ internal/controller/ipaddress_controller.go | 14 +++++--------- .../controller/ipaddress_controller_test.go | 1 + internal/controller/netbox_testdata_test.go | 4 ++++ internal/controller/prefix_controller.go | 14 +++++--------- pkg/netbox/api/ip_address.go | 16 ++++++++-------- pkg/netbox/api/prefix.go | 17 ++++++++--------- 9 files changed, 60 insertions(+), 35 deletions(-) diff --git a/api/v1/ipaddress_types.go b/api/v1/ipaddress_types.go index 06569808..c80106f8 100644 --- a/api/v1/ipaddress_types.go +++ b/api/v1/ipaddress_types.go @@ -101,3 +101,10 @@ var ConditionIpaddressReadyFalse = metav1.Condition{ Reason: "FailedToReserveIpInNetbox", Message: "Failed to reserve IP in NetBox", } + +var ConditionIpaddressReadyFalseDeletionFailed = metav1.Condition{ + Type: "Ready", + Status: "False", + Reason: "FailedToDeleteIpInNetbox", + Message: "Failed to delete IP in NetBox", +} diff --git a/api/v1/prefix_types.go b/api/v1/prefix_types.go index 96e31eac..d9d78b66 100644 --- a/api/v1/prefix_types.go +++ b/api/v1/prefix_types.go @@ -102,3 +102,10 @@ var ConditionPrefixReadyFalse = metav1.Condition{ Reason: "FailedToReservePrefixInNetbox", Message: "Failed to reserve prefix in NetBox", } + +var ConditionPrefixReadyFalseDeletionFailed = metav1.Condition{ + Type: "Ready", + Status: "False", + Reason: "FailedToDeletePrefixInNetbox", + Message: "Failed to delete prefix in Netbox", +} diff --git a/internal/controller/expected_netboxmock_calls_test.go b/internal/controller/expected_netboxmock_calls_test.go index 7d45c4c8..e8c049ac 100644 --- a/internal/controller/expected_netboxmock_calls_test.go +++ b/internal/controller/expected_netboxmock_calls_test.go @@ -139,6 +139,21 @@ func mockIpAddressesDelete(ipamMock *mock_interfaces.MockIpamInterface, catchUne }).MinTimes(1) } +func mockIpAddressesDeleteFail(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { + ipamMock.EXPECT().IpamIPAddressesDelete(gomock.Any(), nil). + DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamIPAddressesDeleteNoContent, error) { + got := params.(*ipam.IpamIPAddressesDeleteParams) + diff := deep.Equal(got, ExpectedDeleteFailParams) + if len(diff) > 0 { + err := fmt.Errorf("netboxmock: unexpected call to ipam.IpamIPAddressesDelete, diff to expected params diff: %+v", diff) + catchUnexpectedParams <- err + return &ipam.IpamIPAddressesDeleteNoContent{}, err + } + fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesDelete was called with mock input\n") + return nil, ipam.NewIpamIPAddressesDeleteDefault(404) + }).MinTimes(1) +} + func mockIpamIPAddressesUpdate(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { ipamMock.EXPECT().IpamIPAddressesUpdate(gomock.Any(), nil). DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamIPAddressesUpdateOK, error) { diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index c83fe7dd..c54d74ef 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -30,7 +30,6 @@ import ( "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/api" "github.com/netbox-community/netbox-operator/pkg/netbox/models" - "github.com/netbox-community/netbox-operator/pkg/netbox/utils" "github.com/swisscom/leaselocker" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" @@ -82,15 +81,12 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if !o.Spec.PreserveInNetbox { err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId) if err != nil { - if errors.Is(err, utils.ErrNotFound) { - setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeWarning, err.Error()) - if setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting IPAddress failed: %w", setConditionErr, err) - } - - return ctrl.Result{Requeue: true}, nil + setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err.Error()) + if setConditionErr != nil { + return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting IPAddress failed: %w", setConditionErr, err) } - return ctrl.Result{}, err + + return ctrl.Result{Requeue: true}, nil } } diff --git a/internal/controller/ipaddress_controller_test.go b/internal/controller/ipaddress_controller_test.go index a5dedfa0..b5f8f56c 100644 --- a/internal/controller/ipaddress_controller_test.go +++ b/internal/controller/ipaddress_controller_test.go @@ -146,6 +146,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { []func(*mock_interfaces.MockIpamInterface, chan error){ mockIpAddressListWithIpAddressFilter, mockIpamIPAddressesUpdateFail, + mockIpAddressesDeleteFail, }, []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, diff --git a/internal/controller/netbox_testdata_test.go b/internal/controller/netbox_testdata_test.go index b79570d3..670c5012 100644 --- a/internal/controller/netbox_testdata_test.go +++ b/internal/controller/netbox_testdata_test.go @@ -219,6 +219,7 @@ func mockedResponseTenancyTenantsList() *tenancy.TenancyTenantsListOKBody { var nsn = namespace + "/" + name + " // " var warningComment = " // managed by netbox-operator, please don't edit it in Netbox unless you know what you're doing" var expectedIpAddressID = int64(1) +var expectedIpAddressFailID = int64(0) var expectedIpToUpdate = &netboxModels.WritableIPAddress{ Address: &ipAddress, @@ -270,6 +271,9 @@ var ExpectedIpAddressesCreateWithHashParams = ipam.NewIpamIPAddressesCreateParam // expected inputs for ipam.IpamIPAddressesDelete method var ExpectedDeleteParams = ipam.NewIpamIPAddressesDeleteParams().WithID(expectedIpAddressID) +// expected inputs for ipam.IpamIPAddressesDelete method when update fails +var ExpectedDeleteFailParams = ipam.NewIpamIPAddressesDeleteParams().WithID(expectedIpAddressFailID) + var ExpectedIpAddressStatus = netboxv1.IpAddressStatus{IpAddressId: 1} var ExpectedIpAddressFailedStatus = netboxv1.IpAddressStatus{IpAddressId: 0} diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index fe1213fa..0f9d0ad1 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -28,7 +28,6 @@ import ( "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/models" - "github.com/netbox-community/netbox-operator/pkg/netbox/utils" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -82,15 +81,12 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { if !prefix.Spec.PreserveInNetbox { if err := r.NetboxClient.DeletePrefix(prefix.Status.PrefixId); err != nil { - if errors.Is(err, utils.ErrNotFound) { - setConditionErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, err.Error()) - if setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting Prefix failed: %w", setConditionErr, err) - } - - return ctrl.Result{Requeue: true}, nil + setConditionErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalseDeletionFailed, corev1.EventTypeWarning, err.Error()) + if setConditionErr != nil { + return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting Prefix failed: %w", setConditionErr, err) } - return ctrl.Result{}, err + + return ctrl.Result{Requeue: true}, nil } } diff --git a/pkg/netbox/api/ip_address.go b/pkg/netbox/api/ip_address.go index ffcb0bda..c9fbdbee 100644 --- a/pkg/netbox/api/ip_address.go +++ b/pkg/netbox/api/ip_address.go @@ -17,7 +17,7 @@ limitations under the License. package api import ( - "strings" + "net/http" "unicode/utf8" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" @@ -98,17 +98,17 @@ func (r *NetboxClient) UpdateIpAddress(ipAddressId int64, ipAddress *netboxModel } func (r *NetboxClient) DeleteIpAddress(ipAddressId int64) error { - // assuming id starts from 1 when IPAdress is created so 0 means that IPAddress doesn't exist - if ipAddressId == 0 { - return nil - } requestDeleteIp := ipam.NewIpamIPAddressesDeleteParams().WithID(ipAddressId) _, err := r.Ipam.IpamIPAddressesDelete(requestDeleteIp, nil) if err != nil { - if strings.Contains(err.Error(), "No IPAddress matches the given query.") { - return utils.NetboxNotFoundError("IPAddress") + switch typedErr := err.(type) { + case *ipam.IpamIPAddressesDeleteDefault: + if typedErr.IsCode(http.StatusNotFound) { + return nil + } + default: + return utils.NetboxError("Failed to delete IP Address from Netbox", err) } - return utils.NetboxError("Failed to delete IP Address from Netbox", err) } return nil } diff --git a/pkg/netbox/api/prefix.go b/pkg/netbox/api/prefix.go index cc344d50..46d58baa 100644 --- a/pkg/netbox/api/prefix.go +++ b/pkg/netbox/api/prefix.go @@ -17,7 +17,7 @@ limitations under the License. package api import ( - "strings" + "net/http" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" @@ -100,18 +100,17 @@ func (r *NetboxClient) UpdatePrefix(prefixId int64, prefix *netboxModels.Writabl } func (r *NetboxClient) DeletePrefix(prefixId int64) error { - // assuming id starts from 1 when Prefix is created so 0 means that Prefix doesn't exist - if prefixId == 0 { - return nil - } - requestDeletePrefix := ipam.NewIpamPrefixesDeleteParams().WithID(prefixId) _, err := r.Ipam.IpamPrefixesDelete(requestDeletePrefix, nil) if err != nil { - if strings.Contains(err.Error(), "No Prefix matches the given query.") { - return utils.NetboxNotFoundError("Prefix") + switch typedErr := err.(type) { + case *ipam.IpamPrefixesDeleteDefault: + if typedErr.IsCode(http.StatusNotFound) { + return nil + } + default: + return utils.NetboxError("Failed to delete IP Address from Netbox", err) } - return utils.NetboxError("failed to delete Prefix", err) } return nil }