Skip to content

Commit d15dc18

Browse files
brueleabruelea
authored
Check restoration hash in NetBox before updating resource in NetBox (#285)
* check restoration hash when resource is update * update status if hash mismatch but id not nil
1 parent 27ae7c3 commit d15dc18

16 files changed

+449
-81
lines changed

‎api/v1/iprangeclaim_types.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ var ConditionIpRangeAssignedFalse = metav1.Condition{
181181
Message: "Failed to fetch new IP Range from NetBox",
182182
}
183183

184-
var ConditionIpRangeAssignedFalseSizeMissmatch = metav1.Condition{
184+
var ConditionIpRangeAssignedFalseSizeMismatch = metav1.Condition{
185185
Type: "IPRangeAssigned",
186186
Status: "False",
187187
Reason: "IPRangeCRNotCreated",

‎config/samples/netbox_v1_prefixclaim_parentprefixselector.yaml

+1-2
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ spec:
1414
prefixLength: "/31"
1515
parentPrefixSelector:
1616
tenant: "MY_TENANT"
17-
site: "DM-Buffalo"
1817
family: "IPv4"
1918
environment: "Production"
20-
poolName: "Pool 1"
19+
poolName: "Pool 1"

‎internal/controller/expected_netboxmock_calls_test.go

+17-1
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func mockIpAddressListWithIpAddressFilter(ipamMock *mock_interfaces.MockIpamInte
4141
return &ipam.IpamIPAddressesListOK{Payload: nil}, err
4242
}
4343
fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesList was called with expected input\n")
44-
return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressList()}, nil
44+
return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHash)}, nil
4545
}).MinTimes(1)
4646
}
4747

@@ -92,6 +92,22 @@ func mockIpAddressListWithHashFilter(ipamMock *mock_interfaces.MockIpamInterface
9292
}).MinTimes(1)
9393
}
9494

95+
func mockIpAddressListWithHashFilterMismatch(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) {
96+
ipamMock.EXPECT().IpamIPAddressesList(gomock.Any(), gomock.Any(), gomock.Any()).
97+
DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamIPAddressesListOK, error) {
98+
got := params.(*ipam.IpamIPAddressesListParams)
99+
diff := deep.Equal(got, ExpectedIpAddressListParamsWithIpAddressData)
100+
// skip check for the 3rd input parameter as it is a method, method is a non comparable type
101+
if len(diff) > 0 {
102+
err := fmt.Errorf("netboxmock: unexpected call to ipam.IpamIPAddressesList, diff to expected params diff: %+v", diff)
103+
catchUnexpectedParams <- err
104+
return &ipam.IpamIPAddressesListOK{Payload: nil}, err
105+
}
106+
fmt.Printf("NETBOXMOCK\t ipam.IpamIPAddressesList (empty reslut) was called with expected input,\n")
107+
return &ipam.IpamIPAddressesListOK{Payload: mockedResponseIPAddressListWithHash(customFieldsWithHashMismatch)}, nil
108+
}).MinTimes(1)
109+
}
110+
95111
func mockPrefixesListWithPrefixFilter(ipamMock *mock_interfaces.MockIpamInterface, catchUnexpectedParams chan error) {
96112
ipamMock.EXPECT().IpamPrefixesList(gomock.Any(), gomock.Any()).
97113
DoAndReturn(func(params interface{}, authInfo interface{}, opts ...interface{}) (*ipam.IpamPrefixesListOK, error) {

‎internal/controller/ipaddress_controller.go

+24-4
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,30 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
170170

171171
netboxIpAddressModel, err := r.NetboxClient.ReserveOrUpdateIpAddress(ipAddressModel)
172172
if err != nil {
173-
updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse,
174-
corev1.EventTypeWarning, o.Spec.IpAddress)
175-
return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+
176-
"after reservation of ip in netbox failed: %w", updateStatusErr, err)
173+
if errors.Is(err, api.ErrRestorationHashMismatch) && o.Status.IpAddressId == 0 {
174+
// if there is a restoration hash mismatch and the IpAddressId status field is not set,
175+
// delete the ip address so it can be recreated by the ip address claim controller
176+
// this will only affect resources that are created by a claim controller (and have a restoration hash custom field
177+
logger.Info("restoration hash mismatch, deleting ip address custom resource", "ipaddress", o.Spec.IpAddress)
178+
err = r.Client.Delete(ctx, o)
179+
if err != nil {
180+
if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse,
181+
corev1.EventTypeWarning, err.Error()); updateStatusErr != nil {
182+
return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+
183+
"after deletion of ip address cr failed: %w", updateStatusErr, err)
184+
}
185+
return ctrl.Result{Requeue: true}, nil
186+
}
187+
return ctrl.Result{}, nil
188+
189+
}
190+
191+
if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse,
192+
corev1.EventTypeWarning, err.Error()); updateStatusErr != nil {
193+
return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+
194+
"after reservation of ip in netbox failed: %w", updateStatusErr, err)
195+
}
196+
return ctrl.Result{Requeue: true}, nil
177197
}
178198

179199
// 3. unlock lease of parent prefix

‎internal/controller/ipaddress_controller_test.go

+48-28
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"sigs.k8s.io/controller-runtime/pkg/client"
2929

3030
netboxv1 "github.com/netbox-community/netbox-operator/api/v1"
31+
apierrors "k8s.io/apimachinery/pkg/api/errors"
3132
)
3233

3334
var _ = Describe("IpAddress Controller", Ordered, func() {
@@ -51,6 +52,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
5152
cr *netboxv1.IpAddress, // our CR as typed object
5253
IpamMocksIpAddress []func(*mock_interfaces.MockIpamInterface, chan error),
5354
TenancyMocks []func(*mock_interfaces.MockTenancyInterface, chan error),
55+
restorationHashMismatch bool, // To check for deletion if restoration hash does not match
5456
expectedConditionReady bool, // Expected state of the ConditionReady condition
5557
expectedCRStatus netboxv1.IpAddressStatus, // Expected status of the CR
5658
) {
@@ -81,31 +83,40 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
8183
By("Creating IpAddress CR")
8284
Eventually(k8sClient.Create(ctx, cr), timeout, interval).Should(Succeed())
8385

84-
// check that reconcile loop did run a least once by checking that conditions are set
8586
createdCR := &netboxv1.IpAddress{}
86-
Eventually(func() bool {
87-
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
88-
return err == nil && len(createdCR.Status.Conditions) > 0
89-
}, timeout, interval).Should(BeTrue())
90-
91-
// Now check if conditions are set as expected
92-
Eventually(func() bool {
93-
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
94-
return err == nil &&
95-
apismeta.IsStatusConditionTrue(createdCR.Status.Conditions, netboxv1.ConditionIpaddressReadyTrue.Type) == expectedConditionReady
96-
}, timeout, interval).Should(BeTrue())
97-
98-
// Check that the expected ip address is present in the status
99-
Expect(createdCR.Status.IpAddressId).To(Equal(expectedCRStatus.IpAddressId))
100-
101-
// Cleanup the netbox resources
102-
Expect(k8sClient.Delete(ctx, createdCR)).Should(Succeed())
103-
104-
// Wait until the resource is deleted to make sure that it will not interfere with the next test case
105-
Eventually(func() bool {
106-
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
107-
return err != client.IgnoreNotFound(err)
108-
}, timeout, interval).Should(BeTrue())
87+
88+
if restorationHashMismatch {
89+
Eventually(func() bool {
90+
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
91+
return apierrors.IsNotFound(err)
92+
}, timeout, interval).Should(BeTrue())
93+
} else {
94+
95+
// check that reconcile loop did run a least once by checking that conditions are set
96+
Eventually(func() bool {
97+
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
98+
return err == nil && len(createdCR.Status.Conditions) > 0
99+
}, timeout, interval).Should(BeTrue())
100+
101+
// Now check if conditions are set as expected
102+
Eventually(func() bool {
103+
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
104+
return err == nil &&
105+
apismeta.IsStatusConditionTrue(createdCR.Status.Conditions, netboxv1.ConditionIpaddressReadyTrue.Type) == expectedConditionReady
106+
}, timeout, interval).Should(BeTrue())
107+
108+
// Check that the expected ip address is present in the status
109+
Expect(createdCR.Status.IpAddressId).To(Equal(expectedCRStatus.IpAddressId))
110+
111+
// Cleanup the netbox resources
112+
Expect(k8sClient.Delete(ctx, createdCR)).Should(Succeed())
113+
114+
// Wait until the resource is deleted to make sure that it will not interfere with the next test case
115+
Eventually(func() bool {
116+
err := k8sClient.Get(ctx, types.NamespacedName{Name: cr.GetName(), Namespace: cr.GetNamespace()}, createdCR)
117+
return err != client.IgnoreNotFound(err)
118+
}, timeout, interval).Should(BeTrue())
119+
}
109120

110121
catchCtxCancel()
111122
},
@@ -119,7 +130,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
119130
[]func(*mock_interfaces.MockTenancyInterface, chan error){
120131
mockTenancyTenancyTenantsList,
121132
},
122-
true, ExpectedIpAddressStatus),
133+
false, true, ExpectedIpAddressStatus),
123134
Entry("Create IpAddress CR, ip address already reserved in NetBox, preserved in netbox, ",
124135
defaultIpAddressCR(true),
125136
[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -129,7 +140,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
129140
[]func(*mock_interfaces.MockTenancyInterface, chan error){
130141
mockTenancyTenancyTenantsList,
131142
},
132-
true, ExpectedIpAddressStatus),
143+
false, true, ExpectedIpAddressStatus),
133144
Entry("Create IpAddress CR, ip address already reserved in NetBox",
134145
defaultIpAddressCR(false),
135146
[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -140,7 +151,7 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
140151
[]func(*mock_interfaces.MockTenancyInterface, chan error){
141152
mockTenancyTenancyTenantsList,
142153
},
143-
true, ExpectedIpAddressStatus),
154+
false, true, ExpectedIpAddressStatus),
144155
Entry("Create IpAddress CR, reserve or update failure",
145156
defaultIpAddressCR(false),
146157
[]func(*mock_interfaces.MockIpamInterface, chan error){
@@ -151,6 +162,15 @@ var _ = Describe("IpAddress Controller", Ordered, func() {
151162
[]func(*mock_interfaces.MockTenancyInterface, chan error){
152163
mockTenancyTenancyTenantsList,
153164
},
154-
false, ExpectedIpAddressFailedStatus),
165+
false, false, ExpectedIpAddressFailedStatus),
166+
Entry("Create IpAddress CR, restoration hash mismatch",
167+
defaultIpAddressCreatedByClaim(true),
168+
[]func(*mock_interfaces.MockIpamInterface, chan error){
169+
mockIpAddressListWithHashFilterMismatch,
170+
},
171+
[]func(*mock_interfaces.MockTenancyInterface, chan error){
172+
mockTenancyTenancyTenantsList,
173+
},
174+
true, false, nil),
155175
)
156176
})

‎internal/controller/iprange_controller.go

+21-4
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package controller
1919
import (
2020
"context"
2121
"encoding/json"
22+
"errors"
2223
"fmt"
2324
"maps"
2425
"strconv"
@@ -142,13 +143,29 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
142143

143144
netboxIpRangeModel, err := r.NetboxClient.ReserveOrUpdateIpRange(ipRangeModel)
144145
if err != nil {
145-
if loggingErr := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
146-
corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); loggingErr != nil {
147-
return ctrl.Result{}, fmt.Errorf("logging error: %w. Original error from ReserveOrUpdateIpRange: %w", loggingErr, err)
146+
if errors.Is(err, api.ErrRestorationHashMismatch) && o.Status.IpRangeId == 0 {
147+
// if there is a restoration hash mismatch and the IpRangeId status field is not set,
148+
// delete the ip range so it can be recreated by the ip range claim controller
149+
// this will only affect resources that are created by a claim controller (and have a restoration hash custom field
150+
logger.Info("restoration hash mismatch, deleting ip range custom resource", "ip-range-start", o.Spec.StartAddress, "ip-range-end", o.Spec.EndAddress)
151+
err = r.Client.Delete(ctx, o)
152+
if err != nil {
153+
if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
154+
corev1.EventTypeWarning, "", err); err != nil {
155+
return ctrl.Result{}, err
156+
}
157+
return ctrl.Result{Requeue: true}, nil
158+
}
159+
return ctrl.Result{}, nil
160+
}
161+
162+
if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse,
163+
corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); err != nil {
164+
return ctrl.Result{}, err
148165
}
149166

150167
// The decision to not return the error message (just logging it) is to not trigger printing the stacktrace on api errors
151-
return ctrl.Result{}, nil
168+
return ctrl.Result{Requeue: true}, nil
152169
}
153170

154171
// 3. unlock lease of parent prefix

‎internal/controller/iprangeclaim_controller.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -322,7 +322,7 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte
322322
availableIpRanges, err := r.NetboxClient.GetAvailableIpAddressesByIpRange(ipRangeModel.Id)
323323
if len(availableIpRanges.Payload) != o.Spec.Size {
324324
ll.Unlock()
325-
err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMissmatch, corev1.EventTypeWarning, "", err)
325+
err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMismatch, corev1.EventTypeWarning, "", err)
326326
if err != nil {
327327
return nil, ctrl.Result{}, err
328328
}

‎internal/controller/netbox_testdata_test.go

+24-5
Original file line numberDiff line numberDiff line change
@@ -53,8 +53,11 @@ var tenantSlug = "test-tenant-slug"
5353

5454
var restorationHash = "6f6c67651f0b43b2969ba2ae35c74fc91815513b"
5555

56-
var customFields = map[string]string{"example_field": "example value"}
57-
var customFieldsWithHash = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash}
56+
var customFieldsCR = map[string]string{"example_field": "example value"}
57+
var customFieldsWithHashCR = map[string]string{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash}
58+
59+
var customFieldsWithHash = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": restorationHash}
60+
var customFieldsWithHashMismatch = map[string]interface{}{"example_field": "example value", "netboxOperatorRestorationHash": "different hash"}
5861

5962
var netboxLabel = "Status"
6063
var value = "active"
@@ -72,7 +75,7 @@ func defaultIpAddressCR(preserveInNetbox bool) *netboxv1.IpAddress {
7275
Spec: netboxv1.IpAddressSpec{
7376
IpAddress: ipAddress,
7477
Tenant: tenant,
75-
CustomFields: customFields,
78+
CustomFields: customFieldsCR,
7679
Comments: comments,
7780
Description: description,
7881
PreserveInNetbox: preserveInNetbox,
@@ -89,7 +92,7 @@ func defaultIpAddressCreatedByClaim(preserveInNetbox bool) *netboxv1.IpAddress {
8992
Spec: netboxv1.IpAddressSpec{
9093
IpAddress: ipAddress,
9194
Tenant: tenant,
92-
CustomFields: customFieldsWithHash,
95+
CustomFields: customFieldsWithHashCR,
9396
Comments: comments,
9497
Description: description,
9598
PreserveInNetbox: preserveInNetbox,
@@ -106,7 +109,7 @@ func defaultIpAddressClaimCR() *netboxv1.IpAddressClaim {
106109
Spec: netboxv1.IpAddressClaimSpec{
107110
ParentPrefix: parentPrefix,
108111
Tenant: tenant,
109-
CustomFields: customFields,
112+
CustomFields: customFieldsCR,
110113
Comments: comments,
111114
Description: description,
112115
PreserveInNetbox: false,
@@ -176,6 +179,22 @@ func mockedResponsePrefixList() *ipam.IpamPrefixesListOKBody {
176179
}
177180
}
178181

182+
func mockedResponseIPAddressListWithHash(customFields map[string]interface{}) *ipam.IpamIPAddressesListOKBody {
183+
return &ipam.IpamIPAddressesListOKBody{
184+
Results: []*netboxModels.IPAddress{
185+
{
186+
ID: mockedResponseIPAddress().ID,
187+
Address: mockedResponseIPAddress().Address,
188+
Comments: mockedResponseIPAddress().Comments,
189+
CustomFields: customFields,
190+
Description: mockedResponseIPAddress().Description,
191+
Display: mockedResponseIPAddress().Display,
192+
Tenant: mockedResponseIPAddress().Tenant,
193+
},
194+
},
195+
}
196+
}
197+
179198
func mockedResponseIPAddressList() *ipam.IpamIPAddressesListOKBody {
180199
return &ipam.IpamIPAddressesListOKBody{
181200
Results: []*netboxModels.IPAddress{

‎internal/controller/prefix_controller.go

+23-2
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,29 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
184184

185185
netboxPrefixModel, err := r.NetboxClient.ReserveOrUpdatePrefix(prefixModel)
186186
if err != nil {
187-
updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, prefix.Spec.Prefix)
188-
return ctrl.Result{}, fmt.Errorf("failed at update prefix status: %w, "+"after reservation of prefix in netbox failed: %w", updateStatusErr, err)
187+
if errors.Is(err, api.ErrRestorationHashMismatch) && prefix.Status.PrefixId == 0 {
188+
// if there is a restoration hash mismatch and the PrefixId status field is not set,
189+
// delete the prefix so it can be recreated by the prefix claim controller
190+
// this will only affect resources that are created by a claim controller (and have a restoration hash custom field
191+
logger.Info("restoration hash mismatch, deleting prefix custom resource", "prefix", prefix.Spec.Prefix)
192+
err = r.Client.Delete(ctx, prefix)
193+
if err != nil {
194+
if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse,
195+
corev1.EventTypeWarning, err.Error()); updateStatusErr != nil {
196+
return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+
197+
"after deletion of prefix cr failed: %w", updateStatusErr, err)
198+
}
199+
return ctrl.Result{Requeue: true}, nil
200+
}
201+
return ctrl.Result{}, nil
202+
}
203+
204+
if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse,
205+
corev1.EventTypeWarning, err.Error()); updateStatusErr != nil {
206+
return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+
207+
"after reservation of prefix netbox failed: %w", updateStatusErr, err)
208+
}
209+
return ctrl.Result{Requeue: true}, nil
189210
}
190211

191212
/* 3. unlock lease of parent prefix */

‎pkg/netbox/api/errors.go

+1
Original file line numberDiff line numberDiff line change
@@ -23,4 +23,5 @@ var (
2323
ErrParentPrefixNotFound = errors.New("parent prefix not found")
2424
ErrWrongMatchingPrefixSubnetFormat = errors.New("wrong matchingPrefix subnet format")
2525
ErrInvalidIpFamily = errors.New("invalid IP Family")
26+
ErrRestorationHashMismatch = errors.New("restoration hash mismatch")
2627
)

0 commit comments

Comments
 (0)