From b4db9c327ae2221a2398283d2d2d12ca1eae5f9d Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 14 Mar 2025 13:30:12 +0100 Subject: [PATCH 1/6] check restoration hash when resource is update --- ...x_v1_prefixclaim_parentprefixselector.yaml | 3 +- .../expected_netboxmock_calls_test.go | 18 ++- internal/controller/ipaddress_controller.go | 23 +++- .../controller/ipaddress_controller_test.go | 76 ++++++----- internal/controller/iprange_controller.go | 15 +++ internal/controller/netbox_testdata_test.go | 17 +++ internal/controller/prefix_controller.go | 21 +++- pkg/netbox/api/errors.go | 1 + pkg/netbox/api/ip_address.go | 35 ++++-- pkg/netbox/api/ip_address_test.go | 118 +++++++++++++++++- pkg/netbox/api/ip_range.go | 26 +++- pkg/netbox/api/ip_range_test.go | 48 +++++++ pkg/netbox/api/prefix.go | 27 +++- pkg/netbox/api/prefix_test.go | 39 ++++++ 14 files changed, 414 insertions(+), 53 deletions(-) diff --git a/config/samples/netbox_v1_prefixclaim_parentprefixselector.yaml b/config/samples/netbox_v1_prefixclaim_parentprefixselector.yaml index a437b0c2..8a9daea5 100644 --- a/config/samples/netbox_v1_prefixclaim_parentprefixselector.yaml +++ b/config/samples/netbox_v1_prefixclaim_parentprefixselector.yaml @@ -14,7 +14,6 @@ spec: prefixLength: "/31" parentPrefixSelector: tenant: "MY_TENANT" - site: "DM-Buffalo" family: "IPv4" environment: "Production" - poolName: "Pool 1" \ No newline at end of file + poolName: "Pool 1" diff --git a/internal/controller/expected_netboxmock_calls_test.go b/internal/controller/expected_netboxmock_calls_test.go index e8c049ac..55b7eee4 100644 --- a/internal/controller/expected_netboxmock_calls_test.go +++ b/internal/controller/expected_netboxmock_calls_test.go @@ -41,7 +41,7 @@ func mockIpAddressListWithIpAddressFilter(ipamMock *mock_interfaces.MockIpamInte return &ipam.IpamIPAddressesListOK{Payload: nil}, err } fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesList was called with expected input\n") - return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressList()}, nil + return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHash)}, nil }).MinTimes(1) } @@ -92,6 +92,22 @@ func mockIpAddressListWithHashFilter(ipamMock *mock_interfaces.MockIpamInterface }).MinTimes(1) } +func mockIpAddressListWithHashFilterMissmatch(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { + ipamMock.EXPECT().IpamIPAddressesList(gomock.Any(), gomock.Any(), gomock.Any()). + DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamIPAddressesListOK, error) { + got := params.(*ipam.IpamIPAddressesListParams) + diff := deep.Equal(got, ExpectedIpAddressListParamsWithIpAddressData) + // skip check for the 3rd input parameter as it is a method, method is a non comparable type + if len(diff) > 0 { + err := fmt.Errorf("netboxmock: unexpected call to ipam.IpamIPAddressesList, diff to expected params diff: %+v", diff) + catchUnexpectedParams <- err + return &ipam.IpamIPAddressesListOK{Payload: nil}, err + } + fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesList (empty reslut) was called with expected input,\n") + return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHashMissmatch)}, nil + }).MinTimes(1) +} + func mockPrefixesListWithPrefixFilter(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { ipamMock.EXPECT().IpamPrefixesList(gomock.Any(), gomock.Any()). DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamPrefixesListOK, error) { diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index 0e1b0900..f359b887 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -170,10 +170,25 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel) if err != nil { - updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, o.Spec.IpAddress) - return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ - "after reservation of ip in netbox failed: %w", updateStatusErr, err) + if errors.Is(err, api.ErrRestorationHashMissmatch) && o.Status.IpAddressId == 0 { + // if there is a restoration has missmatch and the IpAddressId status field is not set, + // delete the ip address so it can be recreated by the ip address claim controller + logger.Info("restoration hash missmatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress) + err = r.Client.Delete(ctx, o) + if err != nil { + updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err.Error()) + return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ + "after deletion of ip address cr failed: %w", updateStatusErr, err) + } + return ctrl.Result{}, nil + } + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, o.Spec.IpAddress); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ + "after reservation of ip in netbox failed: %w", updateStatusErr, err) + } + return ctrl.Result{}, nil } // 3. unlock lease of parent prefix diff --git a/internal/controller/ipaddress_controller_test.go b/internal/controller/ipaddress_controller_test.go index b5f8f56c..a08f581e 100644 --- a/internal/controller/ipaddress_controller_test.go +++ b/internal/controller/ipaddress_controller_test.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" netboxv1 "github.com/netbox-community/netbox-operator/api/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" ) var _ = Describe("IpAddress Controller", Ordered, func() { @@ -51,6 +52,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { cr *netboxv1.IpAddress, // our CR as typed object IpamMocksIpAddress []func(*mock_interfaces.MockIpamInterface, chan error), TenancyMocks []func(*mock_interfaces.MockTenancyInterface, chan error), + restorationHashMissmatch bool, // To check for deletion if restoration hash does not match expectedConditionReady bool, // Expected state of the ConditionReady condition expectedCRStatus netboxv1.IpAddressStatus, // Expected status of the CR ) { @@ -81,31 +83,40 @@ var _ = Describe("IpAddress Controller", Ordered, func() { By("Creating IpAddress CR") Eventually(k8sClient.Create(ctx, cr), timeout, interval).Should(Succeed()) - // check that reconcile loop did run a least once by checking that conditions are set createdCR := &netboxv1.IpAddress{} - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) - return err == nil && len(createdCR.Status.Conditions) > 0 - }, timeout, interval).Should(BeTrue()) - - // Now check if conditions are set as expected - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) - return err == nil && - apismeta.IsStatusConditionTrue(createdCR.Status.Conditions, netboxv1.ConditionIpaddressReadyTrue.Type) == expectedConditionReady - }, timeout, interval).Should(BeTrue()) - - // Check that the expected ip address is present in the status - Expect(createdCR.Status.IpAddressId).To(Equal(expectedCRStatus.IpAddressId)) - - // Cleanup the netbox resources - Expect(k8sClient.Delete(ctx, createdCR)).Should(Succeed()) - - // Wait until the resource is deleted to make sure that it will not interfere with the next test case - Eventually(func() bool { - err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) - return err != client.IgnoreNotFound(err) - }, timeout, interval).Should(BeTrue()) + + if restorationHashMissmatch { + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) + return apierrors.IsNotFound(err) + }, timeout, interval).Should(BeTrue()) + } else { + + // check that reconcile loop did run a least once by checking that conditions are set + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) + return err == nil && len(createdCR.Status.Conditions) > 0 + }, timeout, interval).Should(BeTrue()) + + // Now check if conditions are set as expected + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) + return err == nil && + apismeta.IsStatusConditionTrue(createdCR.Status.Conditions, netboxv1.ConditionIpaddressReadyTrue.Type) == expectedConditionReady + }, timeout, interval).Should(BeTrue()) + + // Check that the expected ip address is present in the status + Expect(createdCR.Status.IpAddressId).To(Equal(expectedCRStatus.IpAddressId)) + + // Cleanup the netbox resources + Expect(k8sClient.Delete(ctx, createdCR)).Should(Succeed()) + + // Wait until the resource is deleted to make sure that it will not interfere with the next test case + Eventually(func() bool { + err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) + return err != client.IgnoreNotFound(err) + }, timeout, interval).Should(BeTrue()) + } catchCtxCancel() }, @@ -119,7 +130,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, }, - true, ExpectedIpAddressStatus), + false, true, ExpectedIpAddressStatus), Entry("Create IpAddress CR, ip address already reserved in NetBox, preserved in netbox, ", defaultIpAddressCR(true), []func(*mock_interfaces.MockIpamInterface, chan error){ @@ -129,7 +140,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, }, - true, ExpectedIpAddressStatus), + false, true, ExpectedIpAddressStatus), Entry("Create IpAddress CR, ip address already reserved in NetBox", defaultIpAddressCR(false), []func(*mock_interfaces.MockIpamInterface, chan error){ @@ -140,7 +151,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, }, - true, ExpectedIpAddressStatus), + false, true, ExpectedIpAddressStatus), Entry("Create IpAddress CR, reserve or update failure", defaultIpAddressCR(false), []func(*mock_interfaces.MockIpamInterface, chan error){ @@ -151,6 +162,15 @@ var _ = Describe("IpAddress Controller", Ordered, func() { []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, }, - false, ExpectedIpAddressFailedStatus), + false, false, ExpectedIpAddressFailedStatus), + Entry("Create IpAddress CR, restoration hash missmatch", + defaultIpAddressCreatedByClaim(true), + []func(*mock_interfaces.MockIpamInterface, chan error){ + mockIpAddressListWithHashFilterMissmatch, + }, + []func(*mock_interfaces.MockTenancyInterface, chan error){ + mockTenancyTenancyTenantsList, + }, + true, false, nil), ) }) diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index f1e5c575..90256423 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -19,6 +19,7 @@ package controller import ( "context" "encoding/json" + "errors" "fmt" "maps" "strconv" @@ -142,6 +143,20 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel) if err != nil { + if errors.Is(err, api.ErrRestorationHashMissmatch) && o.Status.IpRangeId == 0 { + // if there is a restoration has missmatch and the IpRangeId status field is not set, + // delete the ip range so it can be recreated by the ip range claim controller + logger.Info("restoration hash missmatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress) + err = r.Client.Delete(ctx, o) + if err != nil { + if err := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + corev1.EventTypeWarning, "", err); err != nil { + return ctrl.Result{}, err + } + } + return ctrl.Result{}, nil + } + if loggingErr := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); loggingErr != nil { return ctrl.Result{}, fmt.Errorf("logging error: %w. Original error from ReserveOrUpdateIpRange: %w", loggingErr, err) diff --git a/internal/controller/netbox_testdata_test.go b/internal/controller/netbox_testdata_test.go index 670c5012..e3ad581f 100644 --- a/internal/controller/netbox_testdata_test.go +++ b/internal/controller/netbox_testdata_test.go @@ -55,6 +55,7 @@ var restorationHash = "6f6c67651f0b43b2969ba2ae35c74fc91815513b" var customFields = map[string]string{"example_field": "example value"} var customFieldsWithHash = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} +var customFieldsWithHashMissmatch = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} var netboxLabel = "Status" var value = "active" @@ -176,6 +177,22 @@ func mockedResponsePrefixList() *ipam.IpamPrefixesListOKBody { } } +func mockedResponseIPAddressListWithHash(customFields map[string]string) *ipam.IpamIPAddressesListOKBody { + return &ipam.IpamIPAddressesListOKBody{ + Results: []*netboxModels.IPAddress{ + { + ID: mockedResponseIPAddress().ID, + Address: mockedResponseIPAddress().Address, + Comments: mockedResponseIPAddress().Comments, + CustomFields: customFields, + Description: mockedResponseIPAddress().Description, + Display: mockedResponseIPAddress().Display, + Tenant: mockedResponseIPAddress().Tenant, + }, + }, + } +} + func mockedResponseIPAddressList() *ipam.IpamIPAddressesListOKBody { return &ipam.IpamIPAddressesListOKBody{ Results: []*netboxModels.IPAddress{ diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index f0b1f0b3..264c1699 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -184,8 +184,25 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr netboxPrefixModel, err := r.NetboxClient.ReserveOrUpdatePrefix(prefixModel) if err != nil { - updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, prefix.Spec.Prefix) - return ctrl.Result{}, fmt.Errorf("failed at update prefix status: %w, "+"after reservation of prefix in netbox failed: %w", updateStatusErr, err) + if errors.Is(err, api.ErrRestorationHashMissmatch) && prefix.Status.PrefixId == 0 { + // if there is a restoration has missmatch and the PrefixId status field is not set, + // delete the prefix so it can be recreated by the prefix claim controller + logger.Info("restoration hash missmatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix) + err = r.Client.Delete(ctx, prefix) + if err != nil { + updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err.Error()) + return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ + "after deletion of prefix cr failed: %w", updateStatusErr, err) + } + return ctrl.Result{}, nil + } + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, prefix.Spec.Prefix); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ + "after reservation of prefix netbox failed: %w", updateStatusErr, err) + } + return ctrl.Result{}, nil } /* 3. unlock lease of parent prefix */ diff --git a/pkg/netbox/api/errors.go b/pkg/netbox/api/errors.go index 302c8fbc..704002ac 100644 --- a/pkg/netbox/api/errors.go +++ b/pkg/netbox/api/errors.go @@ -23,4 +23,5 @@ var ( ErrParentPrefixNotFound = errors.New("parent prefix not found") ErrWrongMatchingPrefixSubnetFormat = errors.New("wrong matchingPrefix subnet format") ErrInvalidIpFamily = errors.New("invalid IP Family") + ErrRestorationHashMissmatch = errors.New("restoration hash missmatch") ) diff --git a/pkg/netbox/api/ip_address.go b/pkg/netbox/api/ip_address.go index ee3df7ad..c7004eb2 100644 --- a/pkg/netbox/api/ip_address.go +++ b/pkg/netbox/api/ip_address.go @@ -17,10 +17,12 @@ limitations under the License. package api import ( + "fmt" "net/http" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" + "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" @@ -33,14 +35,18 @@ func (r *NetboxClient) ReserveOrUpdateIpAddress(ipAddress *models.IPAddress) (*n } desiredIPAddress := &netboxModels.WritableIPAddress{ - Address: &ipAddress.IpAddress, - Comments: ipAddress.Metadata.Comments + warningComment, - CustomFields: ipAddress.Metadata.Custom, - Description: TruncateDescription(ipAddress.Metadata.Description), - Status: "active", + Address: &ipAddress.IpAddress, + Description: TruncateDescription(""), + Status: "active", } - if ipAddress.Metadata.Tenant != "" { + if ipAddress.Metadata != nil { + desiredIPAddress.CustomFields = ipAddress.Metadata.Custom + desiredIPAddress.Comments = ipAddress.Metadata.Comments + warningComment + desiredIPAddress.Description = TruncateDescription(ipAddress.Metadata.Description) + } + + if ipAddress.Metadata != nil && ipAddress.Metadata.Tenant != "" { tenantDetails, err := r.GetTenantDetails(ipAddress.Metadata.Tenant) if err != nil { return nil, err @@ -52,7 +58,22 @@ func (r *NetboxClient) ReserveOrUpdateIpAddress(ipAddress *models.IPAddress) (*n if len(responseIpAddress.Payload.Results) == 0 { return r.CreateIpAddress(desiredIPAddress) } - //update ip address since it does exist + + ipToUpdate := responseIpAddress.Payload.Results[0] + + // if the desired ip address has a restoration hash + // check that the ip address to update has the same restoration hash + restorationHashKey := config.GetOperatorConfig().NetboxRestorationHashFieldName + if ipAddress.Metadata != nil { + if restorationHash, ok := ipAddress.Metadata.Custom[restorationHashKey]; ok { + if ipToUpdate.CustomFields != nil && ipToUpdate.CustomFields.(map[string]interface{})[restorationHashKey] == restorationHash { + //update ip address since it does exist and the restoration hash matches + return r.UpdateIpAddress(ipToUpdate.ID, desiredIPAddress) + } + return nil, fmt.Errorf("%w, assigned ip address %s", ErrRestorationHashMissmatch, ipAddress.IpAddress) + } + } + ipAddressId := responseIpAddress.Payload.Results[0].ID return r.UpdateIpAddress(ipAddressId, desiredIPAddress) } diff --git a/pkg/netbox/api/ip_address_test.go b/pkg/netbox/api/ip_address_test.go index b2850936..a4493ac5 100644 --- a/pkg/netbox/api/ip_address_test.go +++ b/pkg/netbox/api/ip_address_test.go @@ -24,6 +24,7 @@ import ( "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" "github.com/netbox-community/go-netbox/v3/netbox/client/tenancy" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" + "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/models" "github.com/stretchr/testify/assert" "go.uber.org/mock/gomock" @@ -37,7 +38,7 @@ const ( ) func TestIPAddress(t *testing.T) { - ctrl := gomock.NewController(t) + ctrl := gomock.NewController(t, gomock.WithOverridableExpectations()) defer ctrl.Finish() mockIPAddress := mock_interfaces.NewMockIpamInterface(ctrl) mockPrefixTenancy := mock_interfaces.NewMockTenancyInterface(ctrl) @@ -61,14 +62,19 @@ func TestIPAddress(t *testing.T) { } } + customFields := map[string]string{ + config.GetOperatorConfig().NetboxRestorationHashFieldName: "fioaf9289rjfhaeuih", + } + // example output IP address expectedIPAddress := func() *netboxModels.IPAddress { return &netboxModels.IPAddress{ - ID: int64(1), - Address: &ipAddress, - Display: ipAddress, - Comments: Comments, - Description: Description, + ID: int64(1), + Address: &ipAddress, + Display: ipAddress, + Comments: Comments, + Description: Description, + CustomFields: customFields, Tenant: &netboxModels.NestedTenant{ ID: tenantId, }, @@ -93,6 +99,21 @@ func TestIPAddress(t *testing.T) { } } + ipAddressModel := func(restorationHash string) *models.IPAddress { + model := &models.IPAddress{ + IpAddress: ipAddress, + } + if restorationHash != "" { + if model.Metadata == nil { + model.Metadata = &models.NetboxMetadata{} + } + model.Metadata.Custom = map[string]string{ + config.GetOperatorConfig().NetboxRestorationHashFieldName: restorationHash, + } + } + return model + } + t.Run("Retrieve Existing static IP Address.", func(t *testing.T) { // id, address conversion from int64 to string @@ -254,4 +275,89 @@ func TestIPAddress(t *testing.T) { AssertNil(t, err) }) + t.Run("Check ReserveOrUpdate without hash", func(t *testing.T) { + inputList := ipam.NewIpamIPAddressesListParams().WithAddress(&ipAddress) + outputList := &ipam.IpamIPAddressesListOK{ + Payload: &ipam.IpamIPAddressesListOKBody{ + Results: []*netboxModels.IPAddress{ + { + ID: expectedIPAddress().ID, + Address: expectedIPAddress().Address, + Display: expectedIPAddress().Display, + }}, + }, + } + + outputUpdate := &ipam.IpamIPAddressesUpdateOK{ + Payload: expectedIPAddress(), + } + + mockIPAddress.EXPECT().IpamIPAddressesList(inputList, nil).Return(outputList, nil).AnyTimes() + // use gomock.Any() because the input contains a pointer + mockIPAddress.EXPECT().IpamIPAddressesUpdate(gomock.Any(), nil).Return(outputUpdate, nil) + + client := &NetboxClient{ + Ipam: mockIPAddress, + } + + ipAddressModel := ipAddressModel("") + _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) + AssertNil(t, err) + }) + + t.Run("Check ReserveOrUpdate with hash", func(t *testing.T) { + inputList := ipam.NewIpamIPAddressesListParams().WithAddress(&ipAddress) + outputList := &ipam.IpamIPAddressesListOK{ + Payload: &ipam.IpamIPAddressesListOKBody{ + Results: []*netboxModels.IPAddress{ + { + ID: expectedIPAddress().ID, + Address: expectedIPAddress().Address, + Display: expectedIPAddress().Display, + CustomFields: expectedIPAddress().CustomFields, + }}, + }, + } + + outputUpdate := &ipam.IpamIPAddressesUpdateOK{ + Payload: expectedIPAddress(), + } + + mockIPAddress.EXPECT().IpamIPAddressesList(inputList, nil).Return(outputList, nil).AnyTimes() + // use gomock.Any() because the input contains a pointer + mockIPAddress.EXPECT().IpamIPAddressesUpdate(gomock.Any(), nil).Return(outputUpdate, nil) + + client := &NetboxClient{ + Ipam: mockIPAddress, + } + + ipAddressModel := ipAddressModel("fioaf9289rjfhaeuih") + _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) + AssertNil(t, err) + }) + + t.Run("Check ReserveOrUpdate with hash missmatch", func(t *testing.T) { + inputList := ipam.NewIpamIPAddressesListParams().WithAddress(&ipAddress) + outputList := &ipam.IpamIPAddressesListOK{ + Payload: &ipam.IpamIPAddressesListOKBody{ + Results: []*netboxModels.IPAddress{ + { + ID: expectedIPAddress().ID, + Address: expectedIPAddress().Address, + Display: expectedIPAddress().Display, + CustomFields: expectedIPAddress().CustomFields, + }}, + }, + } + + mockIPAddress.EXPECT().IpamIPAddressesList(inputList, nil).Return(outputList, nil).AnyTimes() + + client := &NetboxClient{ + Ipam: mockIPAddress, + } + + ipAddressModel := ipAddressModel("iwfohs7v82fe9w0") + _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) + AssertError(t, err, "restoration hash missmatch, assigned ip address 10.112.140.0") + }) } diff --git a/pkg/netbox/api/ip_range.go b/pkg/netbox/api/ip_range.go index be00ff81..a38f5b48 100644 --- a/pkg/netbox/api/ip_range.go +++ b/pkg/netbox/api/ip_range.go @@ -17,10 +17,12 @@ limitations under the License. package api import ( + "fmt" "net/http" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" + "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" @@ -41,7 +43,13 @@ func (r *NetboxClient) ReserveOrUpdateIpRange(ipRange *models.IpRange) (*netboxM Status: "active", } - if ipRange.Metadata.Tenant != "" { + if ipRange.Metadata != nil { + desiredIpRange.CustomFields = ipRange.Metadata.Custom + desiredIpRange.Comments = ipRange.Metadata.Comments + warningComment + desiredIpRange.Description = TruncateDescription(ipRange.Metadata.Description) + } + + if ipRange.Metadata != nil && ipRange.Metadata.Tenant != "" { tenantDetails, err := r.GetTenantDetails(ipRange.Metadata.Tenant) if err != nil { return nil, err @@ -53,6 +61,22 @@ func (r *NetboxClient) ReserveOrUpdateIpRange(ipRange *models.IpRange) (*netboxM if len(responseIpRange.Payload.Results) == 0 { return r.CreateIpRange(desiredIpRange) } + + ipRangeToUpdate := responseIpRange.Payload.Results[0] + + // if the desired ip address has a restoration hash + // check that the ip address to update has the same restoration hash + restorationHashKey := config.GetOperatorConfig().NetboxRestorationHashFieldName + if ipRange.Metadata != nil { + if restorationHash, ok := ipRange.Metadata.Custom[restorationHashKey]; ok { + if ipRangeToUpdate.CustomFields != nil && ipRangeToUpdate.CustomFields.(map[string]interface{})[restorationHashKey] == restorationHash { + //update ip address since it does exist and the restoration hash matches + return r.UpdateIpRange(ipRangeToUpdate.ID, desiredIpRange) + } + return nil, fmt.Errorf("%w, assigned ip range %s-%s", ErrRestorationHashMissmatch, ipRange.StartAddress, ipRange.EndAddress) + } + } + //update ip range since it does exist ipRangeId := responseIpRange.Payload.Results[0].ID return r.UpdateIpRange(ipRangeId, desiredIpRange) diff --git a/pkg/netbox/api/ip_range_test.go b/pkg/netbox/api/ip_range_test.go index e08dd965..420026f1 100644 --- a/pkg/netbox/api/ip_range_test.go +++ b/pkg/netbox/api/ip_range_test.go @@ -24,6 +24,7 @@ import ( "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" "github.com/netbox-community/go-netbox/v3/netbox/client/tenancy" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" + "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/models" "github.com/stretchr/testify/assert" "go.uber.org/mock/gomock" @@ -183,6 +184,53 @@ func TestIpRange(t *testing.T) { assert.Equal(t, expectedIPRange().Tenant.Slug, actual.Tenant.Slug) }) + t.Run("ReserveOrUpdate, restoration hash missmatch", func(t *testing.T) { + + // ip range mock input + listInput := ipam.NewIpamIPRangesListParams(). + WithStartAddress(&startAddress). + WithEndAddress(&endAddress) + + // ip range mock output + listOutput := &ipam.IpamIPRangesListOK{ + Payload: &ipam.IpamIPRangesListOKBody{ + Results: []*netboxModels.IPRange{ + { + ID: expectedIPRange().ID, + StartAddress: &startAddress, + EndAddress: &endAddress, + CustomFields: map[string]string{ + config.GetOperatorConfig().NetboxRestorationHashFieldName: "different hash", + }, + Comments: expectedIPRange().Comments, + Description: expectedIPRange().Description, + Tenant: expectedIPRange().Tenant, + }, + }, + }, + } + + mockIpam.EXPECT().IpamIPRangesList(listInput, nil).Return(listOutput, nil).AnyTimes() + + // init client + client := &NetboxClient{ + Ipam: mockIpam, + } + + _, err := client.ReserveOrUpdateIpRange(&models.IpRange{ + StartAddress: startAddress, + EndAddress: endAddress, + Metadata: &models.NetboxMetadata{ + Custom: map[string]string{ + config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash", + }, + }, + }) + + // assert error return + AssertError(t, err, "restoration hash missmatch, assigned ip range 10.112.140.1-10.112.140.3") + }) + t.Run("ReserveOrUpdate, update existing ip range", func(t *testing.T) { // ip range mock input diff --git a/pkg/netbox/api/prefix.go b/pkg/netbox/api/prefix.go index 2dbdc827..48fc9a26 100644 --- a/pkg/netbox/api/prefix.go +++ b/pkg/netbox/api/prefix.go @@ -17,10 +17,12 @@ limitations under the License. package api import ( + "fmt" "net/http" "github.com/netbox-community/go-netbox/v3/netbox/client/ipam" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" + "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" @@ -43,7 +45,13 @@ func (r *NetboxClient) ReserveOrUpdatePrefix(prefix *models.Prefix) (*netboxMode Status: "active", } - if prefix.Metadata.Tenant != "" { + if prefix.Metadata != nil { + desiredPrefix.CustomFields = prefix.Metadata.Custom + desiredPrefix.Comments = prefix.Metadata.Comments + warningComment + desiredPrefix.Description = TruncateDescription(prefix.Metadata.Description) + } + + if prefix.Metadata != nil && prefix.Metadata.Tenant != "" { tenantDetails, err := r.GetTenantDetails(prefix.Metadata.Tenant) if err != nil { return nil, err @@ -51,7 +59,7 @@ func (r *NetboxClient) ReserveOrUpdatePrefix(prefix *models.Prefix) (*netboxMode desiredPrefix.Tenant = &tenantDetails.Id } - if prefix.Metadata.Site != "" { + if prefix.Metadata != nil && prefix.Metadata.Site != "" { siteDetails, err := r.GetSiteDetails(prefix.Metadata.Site) if err != nil { return nil, err @@ -64,6 +72,21 @@ func (r *NetboxClient) ReserveOrUpdatePrefix(prefix *models.Prefix) (*netboxMode return r.CreatePrefix(desiredPrefix) } + prefixToUpdate := responsePrefix.Payload.Results[0] + + // if the desired ip address has a restoration hash + // check that the ip address to update has the same restoration hash + restorationHashKey := config.GetOperatorConfig().NetboxRestorationHashFieldName + if prefix.Metadata != nil { + if restorationHash, ok := prefix.Metadata.Custom[restorationHashKey]; ok { + if prefixToUpdate.CustomFields != nil && prefixToUpdate.CustomFields.(map[string]interface{})[restorationHashKey] == restorationHash { + //update ip address since it does exist and the restoration hash matches + return r.UpdatePrefix(prefixToUpdate.ID, desiredPrefix) + } + return nil, fmt.Errorf("%w, assigned prefix %s", ErrRestorationHashMissmatch, prefix.Prefix) + } + } + //update ip address since it does exist prefixId := responsePrefix.Payload.Results[0].ID return r.UpdatePrefix(prefixId, desiredPrefix) diff --git a/pkg/netbox/api/prefix_test.go b/pkg/netbox/api/prefix_test.go index 0ab45ecd..a5e9a6b3 100644 --- a/pkg/netbox/api/prefix_test.go +++ b/pkg/netbox/api/prefix_test.go @@ -24,6 +24,7 @@ import ( "github.com/netbox-community/go-netbox/v3/netbox/client/tenancy" netboxModels "github.com/netbox-community/go-netbox/v3/netbox/models" "github.com/netbox-community/netbox-operator/gen/mock_interfaces" + "github.com/netbox-community/netbox-operator/pkg/config" "github.com/netbox-community/netbox-operator/pkg/netbox/models" "github.com/stretchr/testify/assert" "go.uber.org/mock/gomock" @@ -495,4 +496,42 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { // without manipulation by the code assert.Nil(t, err) }) + + t.Run("restoration hash missmatch", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockIpam := mock_interfaces.NewMockIpamInterface(ctrl) + + //prefix mock output + prefixListOutput := &ipam.IpamPrefixesListOK{ + Payload: &ipam.IpamPrefixesListOKBody{ + Results: []*netboxModels.Prefix{ + { + ID: prefixId, + CustomFields: map[string]string{config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash"}, + Display: prefix, + Prefix: &prefix, + }, + }, + }, + } + + mockIpam.EXPECT().IpamPrefixesList(prefixListRequestInput, nil).Return(prefixListOutput, nil) + + netboxClient := &NetboxClient{ + Ipam: mockIpam, + } + + prefixModel := models.Prefix{ + Prefix: prefix, + Metadata: &models.NetboxMetadata{ + Custom: map[string]string{config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash-not-matching"}, + }, + } + + _, err := netboxClient.ReserveOrUpdatePrefix(&prefixModel) + // skip assertion on retured values as the payload of IpamPrefixesCreate() is returened + // without manipulation by the code + AssertError(t, err, "restoration hash missmatch, assigned prefix 10.112.140.0/24") + }) } From 53ccfc0bf8109c248e119e234108d24fda174020 Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 14 Mar 2025 14:39:12 +0100 Subject: [PATCH 2/6] fix test --- internal/controller/netbox_testdata_test.go | 17 ++++++++++------- pkg/netbox/api/ip_address_test.go | 2 +- pkg/netbox/api/ip_range_test.go | 2 +- pkg/netbox/api/prefix_test.go | 12 +++++++----- 4 files changed, 19 insertions(+), 14 deletions(-) diff --git a/internal/controller/netbox_testdata_test.go b/internal/controller/netbox_testdata_test.go index e3ad581f..27ab18c5 100644 --- a/internal/controller/netbox_testdata_test.go +++ b/internal/controller/netbox_testdata_test.go @@ -53,9 +53,12 @@ var tenantSlug = "test-tenant-slug" var restorationHash = "6f6c67651f0b43b2969ba2ae35c74fc91815513b" -var customFields = map[string]string{"example_field": "example value"} -var customFieldsWithHash = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} -var customFieldsWithHashMissmatch = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} +var customFieldsCR = map[string]string{"example_field": "example value"} +var customFieldsWithHashCR = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} + +var customFields = map[string]interface{}{"example_field": "example value"} +var customFieldsWithHash = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} +var customFieldsWithHashMissmatch = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} var netboxLabel = "Status" var value = "active" @@ -73,7 +76,7 @@ func defaultIpAddressCR(preserveInNetbox bool) *netboxv1.IpAddress { Spec: netboxv1.IpAddressSpec{ IpAddress: ipAddress, Tenant: tenant, - CustomFields: customFields, + CustomFields: customFieldsCR, Comments: comments, Description: description, PreserveInNetbox: preserveInNetbox, @@ -90,7 +93,7 @@ func defaultIpAddressCreatedByClaim(preserveInNetbox bool) *netboxv1.IpAddress { Spec: netboxv1.IpAddressSpec{ IpAddress: ipAddress, Tenant: tenant, - CustomFields: customFieldsWithHash, + CustomFields: customFieldsWithHashCR, Comments: comments, Description: description, PreserveInNetbox: preserveInNetbox, @@ -107,7 +110,7 @@ func defaultIpAddressClaimCR() *netboxv1.IpAddressClaim { Spec: netboxv1.IpAddressClaimSpec{ ParentPrefix: parentPrefix, Tenant: tenant, - CustomFields: customFields, + CustomFields: customFieldsCR, Comments: comments, Description: description, PreserveInNetbox: false, @@ -177,7 +180,7 @@ func mockedResponsePrefixList() *ipam.IpamPrefixesListOKBody { } } -func mockedResponseIPAddressListWithHash(customFields map[string]string) *ipam.IpamIPAddressesListOKBody { +func mockedResponseIPAddressListWithHash(customFields map[string]interface{}) *ipam.IpamIPAddressesListOKBody { return &ipam.IpamIPAddressesListOKBody{ Results: []*netboxModels.IPAddress{ { diff --git a/pkg/netbox/api/ip_address_test.go b/pkg/netbox/api/ip_address_test.go index a4493ac5..35a28d32 100644 --- a/pkg/netbox/api/ip_address_test.go +++ b/pkg/netbox/api/ip_address_test.go @@ -62,7 +62,7 @@ func TestIPAddress(t *testing.T) { } } - customFields := map[string]string{ + customFields := map[string]interface{}{ config.GetOperatorConfig().NetboxRestorationHashFieldName: "fioaf9289rjfhaeuih", } diff --git a/pkg/netbox/api/ip_range_test.go b/pkg/netbox/api/ip_range_test.go index 420026f1..93324acb 100644 --- a/pkg/netbox/api/ip_range_test.go +++ b/pkg/netbox/api/ip_range_test.go @@ -199,7 +199,7 @@ func TestIpRange(t *testing.T) { ID: expectedIPRange().ID, StartAddress: &startAddress, EndAddress: &endAddress, - CustomFields: map[string]string{ + CustomFields: map[string]interface{}{ config.GetOperatorConfig().NetboxRestorationHashFieldName: "different hash", }, Comments: expectedIPRange().Comments, diff --git a/pkg/netbox/api/prefix_test.go b/pkg/netbox/api/prefix_test.go index a5e9a6b3..801d53ca 100644 --- a/pkg/netbox/api/prefix_test.go +++ b/pkg/netbox/api/prefix_test.go @@ -387,7 +387,7 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { prefixToCreate := &netboxModels.WritablePrefix{ Comments: comments + warningComment, Description: description + warningComment, - CustomFields: make(map[string]string), + CustomFields: make(map[string]interface{}), Prefix: prefixPtr, Site: &siteOutputId, Tenant: &tenantOutputId, @@ -507,10 +507,12 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { Payload: &ipam.IpamPrefixesListOKBody{ Results: []*netboxModels.Prefix{ { - ID: prefixId, - CustomFields: map[string]string{config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash"}, - Display: prefix, - Prefix: &prefix, + ID: prefixId, + CustomFields: map[string]interface{}{ + config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash", + }, + Display: prefix, + Prefix: &prefix, }, }, }, From 12bb17063a10c8b0d39bed5f8dad557c4ad90319 Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 14 Mar 2025 16:40:25 +0100 Subject: [PATCH 3/6] fix unit test and linting --- internal/controller/netbox_testdata_test.go | 1 - pkg/netbox/api/prefix_test.go | 19 ++----------------- 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/internal/controller/netbox_testdata_test.go b/internal/controller/netbox_testdata_test.go index 27ab18c5..bff24ed7 100644 --- a/internal/controller/netbox_testdata_test.go +++ b/internal/controller/netbox_testdata_test.go @@ -56,7 +56,6 @@ var restorationHash = "6f6c67651f0b43b2969ba2ae35c74fc91815513b" var customFieldsCR = map[string]string{"example_field": "example value"} var customFieldsWithHashCR = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} -var customFields = map[string]interface{}{"example_field": "example value"} var customFieldsWithHash = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} var customFieldsWithHashMissmatch = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} diff --git a/pkg/netbox/api/prefix_test.go b/pkg/netbox/api/prefix_test.go index 801d53ca..9fc47bfe 100644 --- a/pkg/netbox/api/prefix_test.go +++ b/pkg/netbox/api/prefix_test.go @@ -383,22 +383,6 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl) mockDcim := mock_interfaces.NewMockDcimInterface(ctrl) - //prefix mock input - prefixToCreate := &netboxModels.WritablePrefix{ - Comments: comments + warningComment, - Description: description + warningComment, - CustomFields: make(map[string]interface{}), - Prefix: prefixPtr, - Site: &siteOutputId, - Tenant: &tenantOutputId, - Status: "active", - } - - createPrefixInput := ipam. - NewIpamPrefixesCreateParams(). - WithDefaults(). - WithData(prefixToCreate) - //prefix mock output createPrefixOutput := &ipam.IpamPrefixesCreateCreated{ Payload: &netboxModels.Prefix{ @@ -419,7 +403,8 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { mockTenancy.EXPECT().TenancyTenantsList(tenantListRequestInput, nil).Return(tenantListRequestOutput, nil).AnyTimes() mockDcim.EXPECT().DcimSitesList(siteListRequestInput, nil).Return(siteListRequestOutput, nil).AnyTimes() mockIpam.EXPECT().IpamPrefixesList(prefixListRequestInput, nil).Return(emptyPrefixListOutput, nil) - mockIpam.EXPECT().IpamPrefixesCreate(createPrefixInput, nil).Return(createPrefixOutput, nil) + // use go mock Any as the input parameter contains pointers + mockIpam.EXPECT().IpamPrefixesCreate(gomock.Any(), nil).Return(createPrefixOutput, nil) netboxClient := &NetboxClient{ Ipam: mockIpam, From 65040413a6f1d3bbc3a25e44cd54c24027289ef0 Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 21 Mar 2025 13:38:35 +0100 Subject: [PATCH 4/6] update status if hash mismatch but id not nil --- api/v1/iprangeclaim_types.go | 2 +- .../expected_netboxmock_calls_test.go | 4 +- internal/controller/ipaddress_controller.go | 37 +++++++++++------ .../controller/ipaddress_controller_test.go | 8 ++-- internal/controller/iprange_controller.go | 34 ++++++++++------ .../controller/iprangeclaim_controller.go | 2 +- internal/controller/netbox_testdata_test.go | 2 +- internal/controller/prefix_controller.go | 40 ++++++++++++------- pkg/netbox/api/errors.go | 2 +- pkg/netbox/api/ip_address.go | 2 +- pkg/netbox/api/ip_address_test.go | 10 +++-- pkg/netbox/api/ip_range.go | 2 +- pkg/netbox/api/ip_range_test.go | 10 +++-- pkg/netbox/api/prefix.go | 2 +- pkg/netbox/api/prefix_test.go | 10 +++-- 15 files changed, 104 insertions(+), 63 deletions(-) diff --git a/api/v1/iprangeclaim_types.go b/api/v1/iprangeclaim_types.go index d3a7f7ea..fe859510 100644 --- a/api/v1/iprangeclaim_types.go +++ b/api/v1/iprangeclaim_types.go @@ -181,7 +181,7 @@ var ConditionIpRangeAssignedFalse = metav1.Condition{ Message: "Failed to fetch new IP Range from NetBox", } -var ConditionIpRangeAssignedFalseSizeMissmatch = metav1.Condition{ +var ConditionIpRangeAssignedFalseSizeMismatch = metav1.Condition{ Type: "IPRangeAssigned", Status: "False", Reason: "IPRangeCRNotCreated", diff --git a/internal/controller/expected_netboxmock_calls_test.go b/internal/controller/expected_netboxmock_calls_test.go index 55b7eee4..5837e7fd 100644 --- a/internal/controller/expected_netboxmock_calls_test.go +++ b/internal/controller/expected_netboxmock_calls_test.go @@ -92,7 +92,7 @@ func mockIpAddressListWithHashFilter(ipamMock *mock_interfaces.MockIpamInterface }).MinTimes(1) } -func mockIpAddressListWithHashFilterMissmatch(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { +func mockIpAddressListWithHashFilterMismatch(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) { ipamMock.EXPECT().IpamIPAddressesList(gomock.Any(), gomock.Any(), gomock.Any()). DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamIPAddressesListOK, error) { got := params.(*ipam.IpamIPAddressesListParams) @@ -104,7 +104,7 @@ func mockIpAddressListWithHashFilterMissmatch(ipamMock *mock_interfaces.MockIpam return &ipam.IpamIPAddressesListOK{Payload: nil}, err } fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesList (empty reslut) was called with expected input,\n") - return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHashMissmatch)}, nil + return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHashMismatch)}, nil }).MinTimes(1) } diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index f359b887..eed54b2f 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -170,25 +170,38 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMissmatch) && o.Status.IpAddressId == 0 { - // if there is a restoration has missmatch and the IpAddressId status field is not set, - // delete the ip address so it can be recreated by the ip address claim controller - logger.Info("restoration hash missmatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress) - err = r.Client.Delete(ctx, o) - if err != nil { - updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, err.Error()) + if errors.Is(err, api.ErrRestorationHashMismatch) { + if o.Status.IpAddressId == 0 { + // if there is a restoration hash mismatch and the IpAddressId status field is not set, + // delete the ip address so it can be recreated by the ip address claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress) + err = r.Client.Delete(ctx, o) + if err != nil { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ + "after deletion of ip address cr failed: %w", updateStatusErr, err) + } + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, nil + } + } else { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ - "after deletion of ip address cr failed: %w", updateStatusErr, err) + "after reservation of ip in netbox failed: %w", updateStatusErr, err) } - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, nil } + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, o.Spec.IpAddress); updateStatusErr != nil { + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ "after reservation of ip in netbox failed: %w", updateStatusErr, err) } - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, nil } // 3. unlock lease of parent prefix diff --git a/internal/controller/ipaddress_controller_test.go b/internal/controller/ipaddress_controller_test.go index a08f581e..4ecc5019 100644 --- a/internal/controller/ipaddress_controller_test.go +++ b/internal/controller/ipaddress_controller_test.go @@ -52,7 +52,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { cr *netboxv1.IpAddress, // our CR as typed object IpamMocksIpAddress []func(*mock_interfaces.MockIpamInterface, chan error), TenancyMocks []func(*mock_interfaces.MockTenancyInterface, chan error), - restorationHashMissmatch bool, // To check for deletion if restoration hash does not match + restorationHashMismatch bool, // To check for deletion if restoration hash does not match expectedConditionReady bool, // Expected state of the ConditionReady condition expectedCRStatus netboxv1.IpAddressStatus, // Expected status of the CR ) { @@ -85,7 +85,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() { createdCR := &netboxv1.IpAddress{} - if restorationHashMissmatch { + if restorationHashMismatch { Eventually(func() bool { err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR) return apierrors.IsNotFound(err) @@ -163,10 +163,10 @@ var _ = Describe("IpAddress Controller", Ordered, func() { mockTenancyTenancyTenantsList, }, false, false, ExpectedIpAddressFailedStatus), - Entry("Create IpAddress CR, restoration hash missmatch", + Entry("Create IpAddress CR, restoration hash mismatch", defaultIpAddressCreatedByClaim(true), []func(*mock_interfaces.MockIpamInterface, chan error){ - mockIpAddressListWithHashFilterMissmatch, + mockIpAddressListWithHashFilterMismatch, }, []func(*mock_interfaces.MockTenancyInterface, chan error){ mockTenancyTenancyTenantsList, diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 90256423..848abda6 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -143,27 +143,37 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMissmatch) && o.Status.IpRangeId == 0 { - // if there is a restoration has missmatch and the IpRangeId status field is not set, - // delete the ip range so it can be recreated by the ip range claim controller - logger.Info("restoration hash missmatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress) - err = r.Client.Delete(ctx, o) - if err != nil { - if err := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + if errors.Is(err, api.ErrRestorationHashMismatch) { + if o.Status.IpRangeId == 0 { + // if there is a restoration hash mismatch and the IpRangeId status field is not set, + // delete the ip range so it can be recreated by the ip range claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress) + err = r.Client.Delete(ctx, o) + if err != nil { + if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + corev1.EventTypeWarning, "", err); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, nil + } else { + if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, corev1.EventTypeWarning, "", err); err != nil { return ctrl.Result{}, err } + return ctrl.Result{Requeue: true}, nil } - return ctrl.Result{}, nil } - if loggingErr := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, - corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); loggingErr != nil { - return ctrl.Result{}, fmt.Errorf("logging error: %w. Original error from ReserveOrUpdateIpRange: %w", loggingErr, err) + if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); err != nil { + return ctrl.Result{}, err } // The decision to not return the error message (just logging it) is to not trigger printing the stacktrace on api errors - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, nil } // 3. unlock lease of parent prefix diff --git a/internal/controller/iprangeclaim_controller.go b/internal/controller/iprangeclaim_controller.go index c352bb8a..4c48392d 100644 --- a/internal/controller/iprangeclaim_controller.go +++ b/internal/controller/iprangeclaim_controller.go @@ -322,7 +322,7 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte availableIpRanges, err := r.NetboxClient.GetAvailableIpAddressesByIpRange(ipRangeModel.Id) if len(availableIpRanges.Payload) != o.Spec.Size { ll.Unlock() - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMissmatch, corev1.EventTypeWarning, "", err) + err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMismatch, corev1.EventTypeWarning, "", err) if err != nil { return nil, ctrl.Result{}, err } diff --git a/internal/controller/netbox_testdata_test.go b/internal/controller/netbox_testdata_test.go index bff24ed7..0bed0e22 100644 --- a/internal/controller/netbox_testdata_test.go +++ b/internal/controller/netbox_testdata_test.go @@ -57,7 +57,7 @@ var customFieldsCR = map[string]string{"example_field": "example value"} var customFieldsWithHashCR = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} var customFieldsWithHash = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash} -var customFieldsWithHashMissmatch = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} +var customFieldsWithHashMismatch = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"} var netboxLabel = "Status" var value = "active" diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 264c1699..cdff84b9 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -184,25 +184,37 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr netboxPrefixModel, err := r.NetboxClient.ReserveOrUpdatePrefix(prefixModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMissmatch) && prefix.Status.PrefixId == 0 { - // if there is a restoration has missmatch and the PrefixId status field is not set, - // delete the prefix so it can be recreated by the prefix claim controller - logger.Info("restoration hash missmatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix) - err = r.Client.Delete(ctx, prefix) - if err != nil { - updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, err.Error()) - return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ - "after deletion of prefix cr failed: %w", updateStatusErr, err) + if errors.Is(err, api.ErrRestorationHashMismatch) { + if prefix.Status.PrefixId == 0 { + // if there is a restoration hash mismatch and the PrefixId status field is not set, + // delete the prefix so it can be recreated by the prefix claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix) + err = r.Client.Delete(ctx, prefix) + if err != nil { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ + "after deletion of prefix cr failed: %w", updateStatusErr, err) + } + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, nil + } else { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ + "after deletion of prefix cr failed: %w", updateStatusErr, err) + } + return ctrl.Result{Requeue: true}, nil } - return ctrl.Result{}, nil } - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, prefix.Spec.Prefix); updateStatusErr != nil { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ "after reservation of prefix netbox failed: %w", updateStatusErr, err) } - return ctrl.Result{}, nil + return ctrl.Result{Requeue: true}, nil } /* 3. unlock lease of parent prefix */ diff --git a/pkg/netbox/api/errors.go b/pkg/netbox/api/errors.go index 704002ac..2492bc95 100644 --- a/pkg/netbox/api/errors.go +++ b/pkg/netbox/api/errors.go @@ -23,5 +23,5 @@ var ( ErrParentPrefixNotFound = errors.New("parent prefix not found") ErrWrongMatchingPrefixSubnetFormat = errors.New("wrong matchingPrefix subnet format") ErrInvalidIpFamily = errors.New("invalid IP Family") - ErrRestorationHashMissmatch = errors.New("restoration hash missmatch") + ErrRestorationHashMismatch = errors.New("restoration hash mismatch") ) diff --git a/pkg/netbox/api/ip_address.go b/pkg/netbox/api/ip_address.go index c7004eb2..3d13666b 100644 --- a/pkg/netbox/api/ip_address.go +++ b/pkg/netbox/api/ip_address.go @@ -70,7 +70,7 @@ func (r *NetboxClient) ReserveOrUpdateIpAddress(ipAddress *models.IPAddress) (*n //update ip address since it does exist and the restoration hash matches return r.UpdateIpAddress(ipToUpdate.ID, desiredIPAddress) } - return nil, fmt.Errorf("%w, assigned ip address %s", ErrRestorationHashMissmatch, ipAddress.IpAddress) + return nil, fmt.Errorf("%w, assigned ip address %s", ErrRestorationHashMismatch, ipAddress.IpAddress) } } diff --git a/pkg/netbox/api/ip_address_test.go b/pkg/netbox/api/ip_address_test.go index 35a28d32..0de7ea82 100644 --- a/pkg/netbox/api/ip_address_test.go +++ b/pkg/netbox/api/ip_address_test.go @@ -62,8 +62,10 @@ func TestIPAddress(t *testing.T) { } } + expectedHash := "fioaf9289rjfhaeuih" + customFields := map[string]interface{}{ - config.GetOperatorConfig().NetboxRestorationHashFieldName: "fioaf9289rjfhaeuih", + config.GetOperatorConfig().NetboxRestorationHashFieldName: expectedHash, } // example output IP address @@ -331,12 +333,12 @@ func TestIPAddress(t *testing.T) { Ipam: mockIPAddress, } - ipAddressModel := ipAddressModel("fioaf9289rjfhaeuih") + ipAddressModel := ipAddressModel(expectedHash) _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) AssertNil(t, err) }) - t.Run("Check ReserveOrUpdate with hash missmatch", func(t *testing.T) { + t.Run("Check ReserveOrUpdate with hash mismatch", func(t *testing.T) { inputList := ipam.NewIpamIPAddressesListParams().WithAddress(&ipAddress) outputList := &ipam.IpamIPAddressesListOK{ Payload: &ipam.IpamIPAddressesListOKBody{ @@ -358,6 +360,6 @@ func TestIPAddress(t *testing.T) { ipAddressModel := ipAddressModel("iwfohs7v82fe9w0") _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) - AssertError(t, err, "restoration hash missmatch, assigned ip address 10.112.140.0") + AssertError(t, err, "restoration hash mismatch, assigned ip address 10.112.140.0") }) } diff --git a/pkg/netbox/api/ip_range.go b/pkg/netbox/api/ip_range.go index a38f5b48..7d7cf9b4 100644 --- a/pkg/netbox/api/ip_range.go +++ b/pkg/netbox/api/ip_range.go @@ -73,7 +73,7 @@ func (r *NetboxClient) ReserveOrUpdateIpRange(ipRange *models.IpRange) (*netboxM //update ip address since it does exist and the restoration hash matches return r.UpdateIpRange(ipRangeToUpdate.ID, desiredIpRange) } - return nil, fmt.Errorf("%w, assigned ip range %s-%s", ErrRestorationHashMissmatch, ipRange.StartAddress, ipRange.EndAddress) + return nil, fmt.Errorf("%w, assigned ip range %s-%s", ErrRestorationHashMismatch, ipRange.StartAddress, ipRange.EndAddress) } } diff --git a/pkg/netbox/api/ip_range_test.go b/pkg/netbox/api/ip_range_test.go index 93324acb..8290ccd4 100644 --- a/pkg/netbox/api/ip_range_test.go +++ b/pkg/netbox/api/ip_range_test.go @@ -184,13 +184,14 @@ func TestIpRange(t *testing.T) { assert.Equal(t, expectedIPRange().Tenant.Slug, actual.Tenant.Slug) }) - t.Run("ReserveOrUpdate, restoration hash missmatch", func(t *testing.T) { + t.Run("ReserveOrUpdate, restoration hash mismatch", func(t *testing.T) { // ip range mock input listInput := ipam.NewIpamIPRangesListParams(). WithStartAddress(&startAddress). WithEndAddress(&endAddress) + wrongHash := "89hqvs0ud89qhdi" // ip range mock output listOutput := &ipam.IpamIPRangesListOK{ Payload: &ipam.IpamIPRangesListOKBody{ @@ -200,7 +201,7 @@ func TestIpRange(t *testing.T) { StartAddress: &startAddress, EndAddress: &endAddress, CustomFields: map[string]interface{}{ - config.GetOperatorConfig().NetboxRestorationHashFieldName: "different hash", + config.GetOperatorConfig().NetboxRestorationHashFieldName: wrongHash, }, Comments: expectedIPRange().Comments, Description: expectedIPRange().Description, @@ -217,18 +218,19 @@ func TestIpRange(t *testing.T) { Ipam: mockIpam, } + expectedHash := "ffjrep8b29fdaikb" _, err := client.ReserveOrUpdateIpRange(&models.IpRange{ StartAddress: startAddress, EndAddress: endAddress, Metadata: &models.NetboxMetadata{ Custom: map[string]string{ - config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash", + config.GetOperatorConfig().NetboxRestorationHashFieldName: expectedHash, }, }, }) // assert error return - AssertError(t, err, "restoration hash missmatch, assigned ip range 10.112.140.1-10.112.140.3") + AssertError(t, err, "restoration hash mismatch, assigned ip range 10.112.140.1-10.112.140.3") }) t.Run("ReserveOrUpdate, update existing ip range", func(t *testing.T) { diff --git a/pkg/netbox/api/prefix.go b/pkg/netbox/api/prefix.go index 48fc9a26..1c15830a 100644 --- a/pkg/netbox/api/prefix.go +++ b/pkg/netbox/api/prefix.go @@ -83,7 +83,7 @@ func (r *NetboxClient) ReserveOrUpdatePrefix(prefix *models.Prefix) (*netboxMode //update ip address since it does exist and the restoration hash matches return r.UpdatePrefix(prefixToUpdate.ID, desiredPrefix) } - return nil, fmt.Errorf("%w, assigned prefix %s", ErrRestorationHashMissmatch, prefix.Prefix) + return nil, fmt.Errorf("%w, assigned prefix %s", ErrRestorationHashMismatch, prefix.Prefix) } } diff --git a/pkg/netbox/api/prefix_test.go b/pkg/netbox/api/prefix_test.go index 9fc47bfe..3929903b 100644 --- a/pkg/netbox/api/prefix_test.go +++ b/pkg/netbox/api/prefix_test.go @@ -482,11 +482,12 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { assert.Nil(t, err) }) - t.Run("restoration hash missmatch", func(t *testing.T) { + t.Run("restoration hash mismatch", func(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mockIpam := mock_interfaces.NewMockIpamInterface(ctrl) + wrongHash := "89327r7fhui" //prefix mock output prefixListOutput := &ipam.IpamPrefixesListOK{ Payload: &ipam.IpamPrefixesListOKBody{ @@ -494,7 +495,7 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { { ID: prefixId, CustomFields: map[string]interface{}{ - config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash", + config.GetOperatorConfig().NetboxRestorationHashFieldName: wrongHash, }, Display: prefix, Prefix: &prefix, @@ -509,16 +510,17 @@ func TestPrefix_ReserveOrUpdate(t *testing.T) { Ipam: mockIpam, } + expectedHash := "jfioaw0e9gh" prefixModel := models.Prefix{ Prefix: prefix, Metadata: &models.NetboxMetadata{ - Custom: map[string]string{config.GetOperatorConfig().NetboxRestorationHashFieldName: "hash-not-matching"}, + Custom: map[string]string{config.GetOperatorConfig().NetboxRestorationHashFieldName: expectedHash}, }, } _, err := netboxClient.ReserveOrUpdatePrefix(&prefixModel) // skip assertion on retured values as the payload of IpamPrefixesCreate() is returened // without manipulation by the code - AssertError(t, err, "restoration hash missmatch, assigned prefix 10.112.140.0/24") + AssertError(t, err, "restoration hash mismatch, assigned prefix 10.112.140.0/24") }) } From 41bc58aae8948f7630920c4b85f71275487bb02f Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 21 Mar 2025 13:45:59 +0100 Subject: [PATCH 5/6] define variable for all hashes in tests --- pkg/netbox/api/ip_address_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/netbox/api/ip_address_test.go b/pkg/netbox/api/ip_address_test.go index 0de7ea82..b01fcb80 100644 --- a/pkg/netbox/api/ip_address_test.go +++ b/pkg/netbox/api/ip_address_test.go @@ -358,7 +358,8 @@ func TestIPAddress(t *testing.T) { Ipam: mockIPAddress, } - ipAddressModel := ipAddressModel("iwfohs7v82fe9w0") + expectedHash := "iwfohs7v82fe9w0" + ipAddressModel := ipAddressModel(expectedHash) _, err := client.ReserveOrUpdateIpAddress(ipAddressModel) AssertError(t, err, "restoration hash mismatch, assigned ip address 10.112.140.0") }) From 6ea119624d19909e6bf0f5605bac78846d689847 Mon Sep 17 00:00:00 2001 From: bruelea <166021996+bruelea@users.noreply.github.com> Date: Fri, 21 Mar 2025 13:59:32 +0100 Subject: [PATCH 6/6] remove duplicate code --- internal/controller/ipaddress_controller.go | 36 ++++++++------------- internal/controller/iprange_controller.go | 24 +++++--------- internal/controller/prefix_controller.go | 26 ++++++--------- 3 files changed, 31 insertions(+), 55 deletions(-) diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index eed54b2f..24a98dd2 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -170,30 +170,22 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMismatch) { - if o.Status.IpAddressId == 0 { - // if there is a restoration hash mismatch and the IpAddressId status field is not set, - // delete the ip address so it can be recreated by the ip address claim controller - // this will only affect resources that are created by a claim controller (and have a restoration hash custom field - logger.Info("restoration hash mismatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress) - err = r.Client.Delete(ctx, o) - if err != nil { - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { - return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ - "after deletion of ip address cr failed: %w", updateStatusErr, err) - } - return ctrl.Result{Requeue: true}, nil + if errors.Is(err, api.ErrRestorationHashMismatch) && o.Status.IpAddressId == 0 { + // if there is a restoration hash mismatch and the IpAddressId status field is not set, + // delete the ip address so it can be recreated by the ip address claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress) + err = r.Client.Delete(ctx, o) + if err != nil { + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ + "after deletion of ip address cr failed: %w", updateStatusErr, err) } - return ctrl.Result{}, nil - } - } else { - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { - return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ - "after reservation of ip in netbox failed: %w", updateStatusErr, err) + return ctrl.Result{Requeue: true}, nil } - return ctrl.Result{Requeue: true}, nil + return ctrl.Result{}, nil + } if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 848abda6..8b9ffa73 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -143,28 +143,20 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMismatch) { - if o.Status.IpRangeId == 0 { - // if there is a restoration hash mismatch and the IpRangeId status field is not set, - // delete the ip range so it can be recreated by the ip range claim controller - // this will only affect resources that are created by a claim controller (and have a restoration hash custom field - logger.Info("restoration hash mismatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress) - err = r.Client.Delete(ctx, o) - if err != nil { - if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, - corev1.EventTypeWarning, "", err); err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, nil - } else { + if errors.Is(err, api.ErrRestorationHashMismatch) && o.Status.IpRangeId == 0 { + // if there is a restoration hash mismatch and the IpRangeId status field is not set, + // delete the ip range so it can be recreated by the ip range claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress) + err = r.Client.Delete(ctx, o) + if err != nil { if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, corev1.EventTypeWarning, "", err); err != nil { return ctrl.Result{}, err } return ctrl.Result{Requeue: true}, nil } + return ctrl.Result{}, nil } if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index cdff84b9..6c3b7df2 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -184,23 +184,13 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr netboxPrefixModel, err := r.NetboxClient.ReserveOrUpdatePrefix(prefixModel) if err != nil { - if errors.Is(err, api.ErrRestorationHashMismatch) { - if prefix.Status.PrefixId == 0 { - // if there is a restoration hash mismatch and the PrefixId status field is not set, - // delete the prefix so it can be recreated by the prefix claim controller - // this will only affect resources that are created by a claim controller (and have a restoration hash custom field - logger.Info("restoration hash mismatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix) - err = r.Client.Delete(ctx, prefix) - if err != nil { - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, - corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { - return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ - "after deletion of prefix cr failed: %w", updateStatusErr, err) - } - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, nil - } else { + if errors.Is(err, api.ErrRestorationHashMismatch) && prefix.Status.PrefixId == 0 { + // if there is a restoration hash mismatch and the PrefixId status field is not set, + // delete the prefix so it can be recreated by the prefix claim controller + // this will only affect resources that are created by a claim controller (and have a restoration hash custom field + logger.Info("restoration hash mismatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix) + err = r.Client.Delete(ctx, prefix) + if err != nil { if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ @@ -208,7 +198,9 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr } return ctrl.Result{Requeue: true}, nil } + return ctrl.Result{}, nil } + if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+