Skip to content

Commit afc40a6

Browse files
brueleabruelea
committed
improvements for review
1 parent 87bdd6c commit afc40a6

File tree

8 files changed

+128
-100
lines changed

8 files changed

+128
-100
lines changed

‎internal/controller/ipaddress_controller.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ import (
4545
)
4646

4747
const IpAddressFinalizerName = "ipaddress.netbox.dev/finalizer"
48-
const LastIpAddressManagedCustomFieldsAnnotationName = "ipaddress.netbox.dev/managed-custom-fields"
48+
const IPManagedCustomFieldsAnnotationName = "ipaddress.netbox.dev/managed-custom-fields"
4949

5050
// IpAddressReconciler reconciles a IpAddress object
5151
type IpAddressReconciler struct {
@@ -163,7 +163,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
163163
return ctrl.Result{}, err
164164
}
165165

166-
ipAddressModel, err := generateNetboxIpAddressModelFromIpAddressSpec(&o.Spec, req, annotations[LastIpAddressManagedCustomFieldsAnnotationName])
166+
ipAddressModel, err := generateNetboxIpAddressModelFromIpAddressSpec(&o.Spec, req, annotations[IPManagedCustomFieldsAnnotationName])
167167
if err != nil {
168168
return ctrl.Result{}, err
169169
}
@@ -193,7 +193,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
193193
annotations = make(map[string]string, 1)
194194
}
195195

196-
annotations[LastIpAddressManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
196+
annotations[IPManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
197197
if err != nil {
198198
logger.Error(err, "failed to update last metadata annotation")
199199
return ctrl.Result{Requeue: true}, nil

‎internal/controller/iprange_controller.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ import (
4242
)
4343

4444
const IpRangeFinalizerName = "iprange.netbox.dev/finalizer"
45-
const LastIpRangeManagedCustomFieldsAnnotationName = "iprange.netbox.dev/managed-custom-fields"
45+
const IPRManagedCustomFieldsAnnotationName = "iprange.netbox.dev/managed-custom-fields"
4646

4747
// IpRangeReconciler reconciles a IpRange object
4848
type IpRangeReconciler struct {
@@ -135,7 +135,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
135135
return ctrl.Result{}, err
136136
}
137137

138-
ipRangeModel, err := r.generateNetboxIpRangeModelFromIpRangeSpec(o, req, annotations[LastIpRangeManagedCustomFieldsAnnotationName])
138+
ipRangeModel, err := r.generateNetboxIpRangeModelFromIpRangeSpec(o, req, annotations[IPRManagedCustomFieldsAnnotationName])
139139
if err != nil {
140140
return ctrl.Result{}, err
141141
}
@@ -166,7 +166,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
166166
annotations = make(map[string]string, 1)
167167
}
168168

169-
annotations[LastIpRangeManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
169+
annotations[IPRManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
170170
if err != nil {
171171
logger.Error(err, "failed to update last metadata annotation")
172172
return ctrl.Result{Requeue: true}, nil

‎internal/controller/iprange_controller_test.go‎

Lines changed: 79 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -16,69 +16,82 @@ limitations under the License.
1616

1717
package controller
1818

19-
// import (
20-
// "context"
21-
22-
// . "github.com/onsi/ginkgo/v2"
23-
// . "github.com/onsi/gomega"
24-
// "k8s.io/apimachinery/pkg/api/errors"
25-
// "k8s.io/apimachinery/pkg/types"
26-
// "sigs.k8s.io/controller-runtime/pkg/reconcile"
27-
28-
// metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
29-
30-
// netboxdevv1 "github.com/netbox-community/netbox-operator/api/v1"
31-
// )
32-
33-
// var _ = Describe("IpRange Controller", func() {
34-
// Context("When reconciling a resource", func() {
35-
// const resourceName = "test-resource"
36-
37-
// ctx := context.Background()
38-
39-
// typeNamespacedName := types.NamespacedName{
40-
// Name: resourceName,
41-
// Namespace: "default", // TODO(user):Modify as needed
42-
// }
43-
// iprange := &netboxdevv1.IpRange{}
44-
45-
// BeforeEach(func() {
46-
// By("creating the custom resource for the Kind IpRange")
47-
// err := k8sClient.Get(ctx, typeNamespacedName, iprange)
48-
// if err != nil && errors.IsNotFound(err) {
49-
// resource := &netboxdevv1.IpRange{
50-
// ObjectMeta: metav1.ObjectMeta{
51-
// Name: resourceName,
52-
// Namespace: "default",
53-
// },
54-
// // TODO(user): Specify other spec details if needed.
55-
// }
56-
// Expect(k8sClient.Create(ctx, resource)).To(Succeed())
57-
// }
58-
// })
59-
60-
// AfterEach(func() {
61-
// // TODO(user): Cleanup logic after each test, like removing the resource instance.
62-
// resource := &netboxdevv1.IpRange{}
63-
// err := k8sClient.Get(ctx, typeNamespacedName, resource)
64-
// Expect(err).NotTo(HaveOccurred())
65-
66-
// By("Cleanup the specific resource instance IpRange")
67-
// Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
68-
// })
69-
// It("should successfully reconcile the resource", func() {
70-
// By("Reconciling the created resource")
71-
// controllerReconciler := &IpRangeReconciler{
72-
// Client: k8sClient,
73-
// Scheme: k8sClient.Scheme(),
74-
// }
75-
76-
// _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{
77-
// NamespacedName: typeNamespacedName,
78-
// })
79-
// Expect(err).NotTo(HaveOccurred())
80-
// // TODO(user): Add more specific assertions depending on your controller's reconciliation logic.
81-
// // Example: If you expect a certain status condition after reconciliation, verify it here.
82-
// })
83-
// })
84-
// })
19+
import (
20+
. "github.com/onsi/ginkgo/v2"
21+
. "github.com/onsi/gomega"
22+
23+
"github.com/netbox-community/netbox-operator/pkg/netbox/api"
24+
"github.com/netbox-community/netbox-operator/pkg/netbox/models"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26+
"sigs.k8s.io/controller-runtime/pkg/client"
27+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
28+
29+
netboxv1 "github.com/netbox-community/netbox-operator/api/v1"
30+
)
31+
32+
var ipRangeRecondiler *IpRangeReconciler
33+
34+
var _ = Describe("IpRange Controller", func() {
35+
Context("When generating NetBox IpRange Model form IpRangeSpec", func() {
36+
// dummy reconciler
37+
ipRangeRecondiler = &IpRangeReconciler{
38+
NetboxClient: &api.NetboxClient{
39+
Ipam: ipamMockIpAddress,
40+
Tenancy: tenancyMock,
41+
Dcim: dcimMock,
42+
},
43+
}
44+
45+
// default IpRange
46+
ipRange := &netboxv1.IpRange{
47+
ObjectMeta: metav1.ObjectMeta{
48+
Namespace: "default",
49+
},
50+
Spec: netboxv1.IpRangeSpec{
51+
StartAddress: "1.0.0.1/32",
52+
EndAddress: "1.0.0.5/32",
53+
Comments: "a comment",
54+
Description: "a description",
55+
Tenant: "a tenant",
56+
CustomFields: map[string]string{"custom_field_2": "valueToBeSet"},
57+
}}
58+
ipRange.Name = "test-claim"
59+
60+
// default managedCustomFieldsAnnotation
61+
managedCustomFieldsAnnotation := "{\"custom_field_1\":\"valueToBeRemoved\"}"
62+
63+
// default request
64+
req := reconcile.Request{
65+
NamespacedName: client.ObjectKey{
66+
Name: "test-claim",
67+
Namespace: "default",
68+
},
69+
}
70+
71+
It("should create the correct ip range model", func() {
72+
ipRangeModel, err := ipRangeRecondiler.generateNetboxIpRangeModelFromIpRangeSpec(ipRange, req, managedCustomFieldsAnnotation)
73+
74+
Expect(ipRangeModel).To(Equal(&models.IpRange{
75+
Metadata: &models.NetboxMetadata{
76+
Comments: "a comment",
77+
Description: "default/test-claim // a description // managed by netbox-operator, please don't edit it in Netbox unless you know what you're doing",
78+
Custom: map[string]string{"custom_field_2": "valueToBeSet", "custom_field_1": ""},
79+
Tenant: "a tenant",
80+
},
81+
StartAddress: "1.0.0.1/32",
82+
EndAddress: "1.0.0.5/32",
83+
}))
84+
85+
Expect(err).To(BeNil())
86+
})
87+
88+
It("should return error if parsing of annotation fails", func() {
89+
invalidManagedCustomFieldsAnnotation := "{:\"valueToBeRemoved\"}"
90+
ipRangeModel, err := ipRangeRecondiler.generateNetboxIpRangeModelFromIpRangeSpec(ipRange, req, invalidManagedCustomFieldsAnnotation)
91+
92+
Expect(ipRangeModel).To(BeNil())
93+
94+
Expect(err).To(HaveOccurred())
95+
})
96+
})
97+
})

‎internal/controller/iprangeclaim_controller.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
117117
return ctrl.Result{}, err
118118
}
119119

120+
err = addFinalizer(ctx, r.Client, o, IpRangeClaimFinalizerName)
121+
if err != nil {
122+
return ctrl.Result{}, err
123+
}
124+
120125
err = r.Client.Create(ctx, ipRangeResource)
121126
if err != nil {
122127
errSetCondition := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, "", err)

‎internal/controller/prefix_controller.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ import (
4646
)
4747

4848
const PrefixFinalizerName = "prefix.netbox.dev/finalizer"
49-
const LastPrefixManagedCustomFieldsAnnotationName = "prefix.netbox.dev/managed-custom-fields"
49+
const PXManagedCustomFieldsAnnotationName = "prefix.netbox.dev/managed-custom-fields"
5050

5151
// PrefixReconciler reconciles a Prefix object
5252
type PrefixReconciler struct {
@@ -177,7 +177,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
177177
return ctrl.Result{}, err
178178
}
179179

180-
prefixModel, err := generateNetboxPrefixModelFromPrefixSpec(&prefix.Spec, req, annotations[LastPrefixManagedCustomFieldsAnnotationName])
180+
prefixModel, err := generateNetboxPrefixModelFromPrefixSpec(&prefix.Spec, req, annotations[PXManagedCustomFieldsAnnotationName])
181181
if err != nil {
182182
return ctrl.Result{}, err
183183
}
@@ -205,7 +205,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
205205
annotations = make(map[string]string, 1)
206206
}
207207

208-
annotations[LastPrefixManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(prefix.Spec.CustomFields)
208+
annotations[PXManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(prefix.Spec.CustomFields)
209209
if err != nil {
210210
logger.Error(err, "failed to update last metadata annotation")
211211
return ctrl.Result{Requeue: true}, nil

‎internal/controller/utils.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,17 +31,17 @@ func convertCIDRToLeaseLockName(cidr string) string {
3131
return strings.ReplaceAll(strings.ReplaceAll(cidr, "/", "-"), ":", "-")
3232
}
3333

34-
func generateLastMetadataAnnotation(cusotmFields map[string]string) (string, error) {
35-
if cusotmFields == nil {
36-
cusotmFields = make(map[string]string)
34+
func generateLastMetadataAnnotation(customFields map[string]string) (string, error) {
35+
if customFields == nil {
36+
customFields = make(map[string]string)
3737
}
3838

39-
lastIpRangeMetadata, err := json.Marshal(cusotmFields)
39+
metadataJSON, err := json.Marshal(customFields)
4040
if err != nil {
41-
return "", fmt.Errorf("failed to marshal lastIpRangeMetadata annotation: %w", err)
41+
return "", fmt.Errorf("failed to marshal custom fields to JSON: %w", err)
4242
}
4343

44-
return string(lastIpRangeMetadata), nil
44+
return string(metadataJSON), nil
4545
}
4646

4747
func removeFinalizer(ctx context.Context, c client.Client, o client.Object, finalizerName string) error {

‎pkg/netbox/api/ip_range_claim.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ func searchAvailableIpRange(availableIps *ipam.IpamPrefixesAvailableIpsListOK, r
113113
// it will search for the first available range of IPs with the required size
114114
// it will return the start and end IP of the range
115115
var startAddress, endAddress string
116-
consecutiveCount := 0
116+
consecutiveCount := 1
117117

118118
ips := make([]net.IP, len(availableIps.Payload))
119119
var err error
@@ -129,15 +129,11 @@ func searchAvailableIpRange(availableIps *ipam.IpamPrefixesAvailableIpsListOK, r
129129
return bytes.Compare(ips[i], ips[j]) < 0
130130
})
131131

132-
for i := range ips {
132+
for i := 1; i < len(ips); i++ {
133133
currentIp := ips[i]
134+
previousIP := ips[i-1]
134135

135-
var previousIP net.IP
136-
if i > 0 {
137-
previousIP = ips[i-1]
138-
}
139-
140-
if i == 0 || areConsecutiveIPs(previousIP, currentIp) {
136+
if areConsecutiveIPs(previousIP, currentIp) {
141137
consecutiveCount++
142138
if consecutiveCount == requiredSize {
143139
startAddress = ips[i-requiredSize+1].String()

‎pkg/netbox/api/ip_range_claim_test.go‎

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ func TestIPRangeClaim(t *testing.T) {
3636
mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl)
3737

3838
// test data for IPv4 ip range claim
39-
parentPrefixIdV4 := int64(3)
39+
parentPrefixId := int64(3)
4040
parentPrefixV4 := "10.114.0.0"
4141
requestedRangeSize := 3
4242
ipRangeV4_1 := "10.112.140.1/24"
@@ -57,19 +57,19 @@ func TestIPRangeClaim(t *testing.T) {
5757
Family: int64(IPv4Family),
5858
},
5959
{
60-
Address: ipRangeV4_3,
60+
Address: ipRangeV4_5,
6161
Family: int64(IPv4Family),
6262
},
6363
{
64-
Address: ipRangeV4_5,
64+
Address: ipRangeV4_3,
6565
Family: int64(IPv4Family),
6666
},
6767
{
68-
Address: ipRangeV4_6,
68+
Address: ipRangeV4_7,
6969
Family: int64(IPv4Family),
7070
},
7171
{
72-
Address: ipRangeV4_7,
72+
Address: ipRangeV4_6,
7373
Family: int64(IPv4Family),
7474
},
7575
{
@@ -111,14 +111,14 @@ func TestIPRangeClaim(t *testing.T) {
111111
Payload: &ipam.IpamPrefixesListOKBody{
112112
Results: []*netboxModels.Prefix{
113113
{
114-
ID: parentPrefixIdV4,
114+
ID: parentPrefixId,
115115
Prefix: &parentPrefixV4,
116116
},
117117
},
118118
},
119119
}
120120

121-
inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixIdV4)
121+
inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixId)
122122
outputIps := &ipam.IpamPrefixesAvailableIpsListOK{
123123
Payload: availableIpAdressesIPv4(),
124124
}
@@ -148,7 +148,7 @@ func TestIPRangeClaim(t *testing.T) {
148148
assert.Equal(t, expectedIp_7, actual.EndAddress)
149149
})
150150

151-
t.Run("Fetch first available IP range by claim (IPv4).", func(t *testing.T) {
151+
t.Run("Fail first available IP range by claim (IPv6) if not enough consequiteve ips.", func(t *testing.T) {
152152

153153
inputTenant := tenancy.NewTenancyTenantsListParams().WithName(&tenantName)
154154

@@ -159,14 +159,14 @@ func TestIPRangeClaim(t *testing.T) {
159159
Payload: &ipam.IpamPrefixesListOKBody{
160160
Results: []*netboxModels.Prefix{
161161
{
162-
ID: parentPrefixIdV4,
163-
Prefix: &parentPrefixV4,
162+
ID: parentPrefixId,
163+
Prefix: &parentPrefixV6,
164164
},
165165
},
166166
},
167167
}
168168

169-
inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixIdV4)
169+
inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixId)
170170
outputIps := &ipam.IpamPrefixesAvailableIpsListOK{
171171
Payload: []*netboxModels.AvailableIP{
172172
{
@@ -201,6 +201,20 @@ func TestIPRangeClaim(t *testing.T) {
201201
AssertError(t, err, "not enough consecutive IPs available")
202202
})
203203

204+
t.Run("Fail with invalid input when searching Available Ip Range", func(t *testing.T) {
205+
payload := &ipam.IpamPrefixesAvailableIpsListOK{
206+
Payload: []*netboxModels.AvailableIP{
207+
{Address: "invalid ip address"},
208+
},
209+
}
210+
211+
startAddress, endAddress, err := searchAvailableIpRange(payload, 3, int64(4))
212+
213+
assert.Equal(t, "", startAddress)
214+
assert.Equal(t, "", endAddress)
215+
AssertError(t, err, "failed to parse IP address: invalid CIDR address: invalid ip address")
216+
})
217+
204218
t.Run("Reclaim IP Range", func(t *testing.T) {
205219

206220
input := "dummy-hash"

0 commit comments

Comments
 (0)