Skip to content

Commit

Permalink
improvements for review
Browse files Browse the repository at this point in the history
  • Loading branch information
@bruelea
bruelea committed Nov 28, 2024
1 parent 87bdd6c commit 9c96d13
Show file tree
Hide file tree
Showing 8 changed files with 128 additions and 99 deletions.
6 changes: 3 additions & 3 deletions internal/controller/ipaddress_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import (
)

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

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

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

annotations[LastIpAddressManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
annotations[IPManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
if err != nil {
logger.Error(err, "failed to update last metadata annotation")
return ctrl.Result{Requeue: true}, nil
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/iprange_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import (
)

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

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

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

annotations[LastIpRangeManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
annotations[IPRManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(o.Spec.CustomFields)
if err != nil {
logger.Error(err, "failed to update last metadata annotation")
return ctrl.Result{Requeue: true}, nil
Expand Down
146 changes: 80 additions & 66 deletions internal/controller/iprange_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,69 +16,83 @@ limitations under the License.

package controller

// import (
// "context"

// . "github.com/onsi/ginkgo/v2"
// . "github.com/onsi/gomega"
// "k8s.io/apimachinery/pkg/api/errors"
// "k8s.io/apimachinery/pkg/types"
// "sigs.k8s.io/controller-runtime/pkg/reconcile"

// metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

// netboxdevv1 "github.com/netbox-community/netbox-operator/api/v1"
// )

// var _ = Describe("IpRange Controller", func() {
// Context("When reconciling a resource", func() {
// const resourceName = "test-resource"

// ctx := context.Background()

// typeNamespacedName := types.NamespacedName{
// Name: resourceName,
// Namespace: "default", // TODO(user):Modify as needed
// }
// iprange := &netboxdevv1.IpRange{}

// BeforeEach(func() {
// By("creating the custom resource for the Kind IpRange")
// err := k8sClient.Get(ctx, typeNamespacedName, iprange)
// if err != nil && errors.IsNotFound(err) {
// resource := &netboxdevv1.IpRange{
// ObjectMeta: metav1.ObjectMeta{
// Name: resourceName,
// Namespace: "default",
// },
// // TODO(user): Specify other spec details if needed.
// }
// Expect(k8sClient.Create(ctx, resource)).To(Succeed())
// }
// })

// AfterEach(func() {
// // TODO(user): Cleanup logic after each test, like removing the resource instance.
// resource := &netboxdevv1.IpRange{}
// err := k8sClient.Get(ctx, typeNamespacedName, resource)
// Expect(err).NotTo(HaveOccurred())

// By("Cleanup the specific resource instance IpRange")
// Expect(k8sClient.Delete(ctx, resource)).To(Succeed())
// })
// It("should successfully reconcile the resource", func() {
// By("Reconciling the created resource")
// controllerReconciler := &IpRangeReconciler{
// Client: k8sClient,
// Scheme: k8sClient.Scheme(),
// }

// _, err := controllerReconciler.Reconcile(ctx, reconcile.Request{
// NamespacedName: typeNamespacedName,
// })
// Expect(err).NotTo(HaveOccurred())
// // TODO(user): Add more specific assertions depending on your controller's reconciliation logic.
// // Example: If you expect a certain status condition after reconciliation, verify it here.
// })
// })
// })
import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"github.com/netbox-community/netbox-operator/pkg/netbox/api"
"github.com/netbox-community/netbox-operator/pkg/netbox/models"

Check failure on line 24 in internal/controller/iprange_controller_test.go

View workflow job for this annotation

GitHub Actions / run 

ST1019: package "github.com/netbox-community/netbox-operator/pkg/netbox/models" is being imported more than once (stylecheck)
apimodel "github.com/netbox-community/netbox-operator/pkg/netbox/models"

Check failure on line 25 in internal/controller/iprange_controller_test.go

View workflow job for this annotation

GitHub Actions / run 

ST1019(related information): other import of "github.com/netbox-community/netbox-operator/pkg/netbox/models" (stylecheck)
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

netboxv1 "github.com/netbox-community/netbox-operator/api/v1"
)

var ipRangeRecondiler *IpRangeReconciler

var _ = Describe("IpRange Controller", func() {
Context("When generating NetBox IpRange Model form IpRangeSpec", func() {
// dummy reconciler
ipRangeRecondiler = &IpRangeReconciler{
NetboxClient: &api.NetboxClient{
Ipam: ipamMockIpAddress,
Tenancy: tenancyMock,
Dcim: dcimMock,
},
}

// default IpRange
ipRange := &netboxv1.IpRange{
ObjectMeta: metav1.ObjectMeta{
Namespace: "default",
},
Spec: netboxv1.IpRangeSpec{
StartAddress: "1.0.0.1/32",
EndAddress: "1.0.0.5/32",
Comments: "a comment",
Description: "a description",
Tenant: "a tenant",
CustomFields: map[string]string{"custom_field_2": "valueToBeSet"},
}}
ipRange.Name = "test-claim"

// default managedCustomFieldsAnnotation
managedCustomFieldsAnnotation := "{\"custom_field_1\":\"valueToBeRemoved\"}"

// default request
req := reconcile.Request{
NamespacedName: client.ObjectKey{
Name: "test-claim",
Namespace: "default",
},
}

It("should create the correct ip range model", func() {
ipRangeModel, err := ipRangeRecondiler.generateNetboxIpRangeModelFromIpRangeSpec(ipRange, req, managedCustomFieldsAnnotation)

Expect(ipRangeModel).To(Equal(&models.IpRange{
Metadata: &apimodel.NetboxMetadata{
Comments: "a comment",
Description: "default/test-claim // a description // managed by netbox-operator, please don't edit it in Netbox unless you know what you're doing",
Custom: map[string]string{"custom_field_2": "valueToBeSet", "custom_field_1": ""},
Tenant: "a tenant",
},
StartAddress: "1.0.0.1/32",
EndAddress: "1.0.0.5/32",
}))

Expect(err).To(BeNil())
})

It("should return error if parsing of annotation fails", func() {
invalidManagedCustomFieldsAnnotation := "{:\"valueToBeRemoved\"}"
ipRangeModel, err := ipRangeRecondiler.generateNetboxIpRangeModelFromIpRangeSpec(ipRange, req, invalidManagedCustomFieldsAnnotation)

Expect(ipRangeModel).To(BeNil())

Expect(err).To(HaveOccurred())
})
})
})
5 changes: 5 additions & 0 deletions internal/controller/iprangeclaim_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,11 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request
return ctrl.Result{}, err
}

err = addFinalizer(ctx, r.Client, o, IpRangeClaimFinalizerName)
if err != nil {
return ctrl.Result{}, err
}

err = r.Client.Create(ctx, ipRangeResource)
if err != nil {
errSetCondition := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, "", err)
Expand Down
6 changes: 3 additions & 3 deletions internal/controller/prefix_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ import (
)

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

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

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

annotations[LastPrefixManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(prefix.Spec.CustomFields)
annotations[PXManagedCustomFieldsAnnotationName], err = generateLastMetadataAnnotation(prefix.Spec.CustomFields)
if err != nil {
logger.Error(err, "failed to update last metadata annotation")
return ctrl.Result{Requeue: true}, nil
Expand Down
10 changes: 5 additions & 5 deletions internal/controller/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,14 @@ func convertCIDRToLeaseLockName(cidr string) string {
return strings.ReplaceAll(strings.ReplaceAll(cidr, "/", "-"), ":", "-")
}

func generateLastMetadataAnnotation(cusotmFields map[string]string) (string, error) {
if cusotmFields == nil {
cusotmFields = make(map[string]string)
func generateLastMetadataAnnotation(customFields map[string]string) (string, error) {
if customFields == nil {
customFields = make(map[string]string)
}

lastIpRangeMetadata, err := json.Marshal(cusotmFields)
lastIpRangeMetadata, err := json.Marshal(customFields)
if err != nil {
return "", fmt.Errorf("failed to marshal lastIpRangeMetadata annotation: %w", err)
return "", fmt.Errorf("failed to marshal custom fields to JSON: %w", err)
}

return string(lastIpRangeMetadata), nil
Expand Down
12 changes: 4 additions & 8 deletions pkg/netbox/api/ip_range_claim.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func searchAvailableIpRange(availableIps *ipam.IpamPrefixesAvailableIpsListOK, r
// it will search for the first available range of IPs with the required size
// it will return the start and end IP of the range
var startAddress, endAddress string
consecutiveCount := 0
consecutiveCount := 1

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

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

var previousIP net.IP
if i > 0 {
previousIP = ips[i-1]
}

if i == 0 || areConsecutiveIPs(previousIP, currentIp) {
if areConsecutiveIPs(previousIP, currentIp) {
consecutiveCount++
if consecutiveCount == requiredSize {
startAddress = ips[i-requiredSize+1].String()
Expand Down
36 changes: 25 additions & 11 deletions pkg/netbox/api/ip_range_claim_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ func TestIPRangeClaim(t *testing.T) {
mockTenancy := mock_interfaces.NewMockTenancyInterface(ctrl)

// test data for IPv4 ip range claim
parentPrefixIdV4 := int64(3)
parentPrefixId := int64(3)
parentPrefixV4 := "10.114.0.0"
requestedRangeSize := 3
ipRangeV4_1 := "10.112.140.1/24"
Expand All @@ -57,19 +57,19 @@ func TestIPRangeClaim(t *testing.T) {
Family: int64(IPv4Family),
},
{
Address: ipRangeV4_3,
Address: ipRangeV4_5,
Family: int64(IPv4Family),
},
{
Address: ipRangeV4_5,
Address: ipRangeV4_3,
Family: int64(IPv4Family),
},
{
Address: ipRangeV4_6,
Address: ipRangeV4_7,
Family: int64(IPv4Family),
},
{
Address: ipRangeV4_7,
Address: ipRangeV4_6,
Family: int64(IPv4Family),
},
{
Expand Down Expand Up @@ -111,14 +111,14 @@ func TestIPRangeClaim(t *testing.T) {
Payload: &ipam.IpamPrefixesListOKBody{
Results: []*netboxModels.Prefix{
{
ID: parentPrefixIdV4,
ID: parentPrefixId,
Prefix: &parentPrefixV4,
},
},
},
}

inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixIdV4)
inputIps := ipam.NewIpamPrefixesAvailableIpsListParams().WithID(parentPrefixId)
outputIps := &ipam.IpamPrefixesAvailableIpsListOK{
Payload: availableIpAdressesIPv4(),
}
Expand Down Expand Up @@ -148,7 +148,7 @@ func TestIPRangeClaim(t *testing.T) {
assert.Equal(t, expectedIp_7, actual.EndAddress)
})

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

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

Expand All @@ -159,14 +159,14 @@ func TestIPRangeClaim(t *testing.T) {
Payload: &ipam.IpamPrefixesListOKBody{
Results: []*netboxModels.Prefix{
{
ID: parentPrefixIdV4,
Prefix: &parentPrefixV4,
ID: parentPrefixId,
Prefix: &parentPrefixV6,
},
},
},
}

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

t.Run("Fail with invalid input when searching Available Ip Range", func(t *testing.T) {
payload := &ipam.IpamPrefixesAvailableIpsListOK{
Payload: []*netboxModels.AvailableIP{
{Address: "invalid ip address"},
},
}

startAddress, endAddress, err := searchAvailableIpRange(payload, 3, int64(4))

assert.Equal(t, "", startAddress)
assert.Equal(t, "", endAddress)
AssertError(t, err, "failed to parse IP address: invalid CIDR address: invalid ip address")
})

t.Run("Reclaim IP Range", func(t *testing.T) {

input := "dummy-hash"
Expand Down

0 comments on commit 9c96d13

Please sign in to comment.