Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement dynamic selection of parent prefix from a set of custom fields #90

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions ParentPrefixSelectorGuide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# A guide of `ParentPrefixSelector` in `PrefixClaim`

There are 2 ways to make a Prefix claim:
- provide a `parentPrefix`
- provide a `parentPrefixSelector`

In this documentation, we will focus on the `parentPrefixSelector` only.
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved

# CRD format

The following is a sample of utilizing the `parentPrefixSelector`:
```bash
apiVersion: netbox.dev/v1
kind: PrefixClaim
metadata:
labels:
app.kubernetes.io/name: netbox-operator
app.kubernetes.io/managed-by: kustomize
name: prefixclaim-customfields-sample
spec:
tenant: "MY_TENANT"
site: "DM-Akron"
description: "some description"
comments: "your comments"
preserveInNetbox: true
prefixLength: "/31"
parentPrefixSelector:
tenant: "MY_TENANT_2"
site: "DM-Bryon"
environment: "Production"
poolName: "Pool 1"
```

## `Spec.tenant` and `Spec.site`

Please provide the *name*, not the *slug* value

## `parentPrefixSelector`

The `parentPrefixSelector` is a key-value map, where all the entries are of data type `<string-string>`.

The `parentPrefixSelector` is a set of query conditions for selecting a set of prefixes that can be used as the parent prefix.

The fields that can be used as query conditions in the `parentPrefixSelector` are:
- `tenant` and `site`
- these 2 fields come by design with NetBox, so you do *not* need to create custom fields for them
- please provide the *name*, not the *slug* value
- if either of the value is missing, it will *not* inherit from the tenant and site from the Spec
- custom fields
- the data types tested and supported so far are `string`, `integer`, and `boolean`
- for `boolean` type, please use `true` and `false` as the value
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ Use Cases for this Restoration:
- Disaster Recovery: In case the cluster is lost, IP Addresses can be restored with the IPAddressClaim only
- Sticky IPs: Some services do not handle changes to IPs well. This ensures the IP/Prefix assigned to a Custom Resource is always the same.

# `ParentPrefixSelector` in `PrefixClaim`

Please read [ParentPrefixSelector guide] for more information!

[ParentPrefixSelector guide]: ./ParentPrefixSelectorGuide.md


# Project Distribution

Following are the steps to build the installer and distribute this project to users.
Expand Down
36 changes: 30 additions & 6 deletions api/v1/prefixclaim_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,31 @@ import (
// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

// PrefixClaimSpec defines the desired state of PrefixClaim
// TODO: The reason for using a workaround please see https://github.com/netbox-community/netbox-operator/pull/90#issuecomment-2402112475
// +kubebuilder:validation:XValidation:rule="!has(oldSelf.site) || has(self.site)", message="Site is required once set"
// +kubebuilder:validation:XValidation:rule="(!has(self.parentPrefix) && has(self.parentPrefixSelector)) || (has(self.parentPrefix) && !has(self.parentPrefixSelector))"
type PrefixClaimSpec struct {

// INSERT ADDITIONAL SPEC FIELDS - desired state of cluster
// Important: Run "make" to regenerate code after modifying this file

//+kubebuilder:validation:Required
//+kubebuilder:validation:Format=cidr
//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'parentPrefix' is immutable"
ParentPrefix string `json:"parentPrefix"`
ParentPrefix string `json:"parentPrefix,omitempty"`

//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'parentPrefixSelector' is immutable"
ParentPrefixSelector map[string]string `json:"parentPrefixSelector,omitempty"`
jstudler marked this conversation as resolved.
Show resolved Hide resolved

//+kubebuilder:validation:Required
//+kubebuilder:validation:Pattern=`^\/[0-9]|[1-9][0-9]|1[01][0-9]|12[0-8]$`
//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'prefixLength' is immutable"
PrefixLength string `json:"prefixLength"`

// Use the `name` value instead of the `slug` value
//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'site' is immutable"
Site string `json:"site,omitempty"`

// Use the `name` value instead of the `slug` value
//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="Field 'tenant' is immutable"
Tenant string `json:"tenant,omitempty"`

Expand All @@ -58,16 +65,19 @@ type PrefixClaimSpec struct {
type PrefixClaimStatus struct {
// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
// Important: Run "make" to regenerate code after modifying this file
// Prefix status: container, active, reserved , deprecated
Prefix string `json:"prefix,omitempty"`
PrefixName string `json:"prefixName,omitempty"`
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
// Prefix status: container, active, reserved, deprecated

ParentPrefix string `json:"parentPrefix,omitempty"` // Due to the fact that we can use ParentPrefixSelector to assign parent prefixes, we use this field to store exactly which parent prefix we are using for prefix allocation
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
Prefix string `json:"prefix,omitempty"`
PrefixName string `json:"prefixName,omitempty"`
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
}

// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:printcolumn:name="Prefix",type=string,JSONPath=`.status.prefix`
// +kubebuilder:printcolumn:name="PrefixAssigned",type=string,JSONPath=`.status.conditions[?(@.type=="PrefixAssigned")].status`
// +kubebuilder:printcolumn:name="ParentPrefix",type=string,JSONPath=`.status.parentPrefix`
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
// +kubebuilder:resource:shortName=pxc
Expand Down Expand Up @@ -120,3 +130,17 @@ var ConditionPrefixAssignedFalse = metav1.Condition{
Reason: "PrefixCRNotCreated",
Message: "Failed to fetch new Prefix from NetBox",
}

var ConditionParentPrefixComputedTrue = metav1.Condition{
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
Type: "ParentPrefixComputed",
Status: "True",
Reason: "ParentPrefixComputed",
Message: "The parent prefix was computed successfully",
}

var ConditionParentPrefixComputedFalse = metav1.Condition{
Type: "ParentPrefixComputed",
Status: "False",
Reason: "ParentPrefixNotComputed",
Message: "The parent prefix was not able to be computed",
}
7 changes: 7 additions & 0 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 19 additions & 6 deletions config/crd/bases/netbox.dev_prefixclaims.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ spec:
- jsonPath: .status.conditions[?(@.type=="PrefixAssigned")].status
name: PrefixAssigned
type: string
- jsonPath: .status.parentPrefix
name: ParentPrefix
type: string
- jsonPath: .status.conditions[?(@.type=="Ready")].status
name: Ready
type: string
Expand Down Expand Up @@ -52,7 +55,9 @@ spec:
metadata:
type: object
spec:
description: PrefixClaimSpec defines the desired state of PrefixClaim
description: |-
PrefixClaimSpec defines the desired state of PrefixClaim
TODO: The reason for using a workaround please see https://github.com/netbox-community/netbox-operator/pull/90#issuecomment-2402112475
properties:
comments:
type: string
Expand All @@ -68,6 +73,13 @@ spec:
x-kubernetes-validations:
- message: Field 'parentPrefix' is immutable
rule: self == oldSelf
parentPrefixSelector:
additionalProperties:
type: string
type: object
x-kubernetes-validations:
- message: Field 'parentPrefixSelector' is immutable
rule: self == oldSelf
prefixLength:
pattern: ^\/[0-9]|[1-9][0-9]|1[01][0-9]|12[0-8]$
type: string
Expand All @@ -77,22 +89,25 @@ spec:
preserveInNetbox:
type: boolean
site:
description: Use the `name` value instead of the `slug` value
type: string
x-kubernetes-validations:
- message: Field 'site' is immutable
rule: self == oldSelf
tenant:
description: Use the `name` value instead of the `slug` value
type: string
x-kubernetes-validations:
- message: Field 'tenant' is immutable
rule: self == oldSelf
required:
- parentPrefix
- prefixLength
type: object
x-kubernetes-validations:
- message: Site is required once set
rule: '!has(oldSelf.site) || has(self.site)'
- rule: (!has(self.parentPrefix) && has(self.parentPrefixSelector)) ||
(has(self.parentPrefix) && !has(self.parentPrefixSelector))
status:
description: PrefixClaimStatus defines the observed state of PrefixClaim
properties:
Expand Down Expand Up @@ -165,11 +180,9 @@ spec:
- type
type: object
type: array
parentPrefix:
type: string
prefix:
description: |-
INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
Important: Run "make" to regenerate code after modifying this file
Prefix status: container, active, reserved , deprecated
type: string
prefixName:
type: string
Expand Down
1 change: 1 addition & 0 deletions config/samples/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,4 +4,5 @@ resources:
- netbox_v1_ipaddressclaim.yaml
- netbox_v1_prefix.yaml
- netbox_v1_prefixclaim.yaml
- netbox_v1_prefixclaim_customfields.yaml
#+kubebuilder:scaffold:manifestskustomizesamples
19 changes: 19 additions & 0 deletions config/samples/netbox_v1_prefixclaim_customfields.yaml
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
apiVersion: netbox.dev/v1
kind: PrefixClaim
metadata:
labels:
app.kubernetes.io/name: netbox-operator
app.kubernetes.io/managed-by: kustomize
name: prefixclaim-customfields-sample
spec:
tenant: "MY_TENANT" # Use the `name` value instead of the `slug` value
site: "DM-Akron" # Use the `name` value instead of the `slug` value
description: "some description"
comments: "your comments"
preserveInNetbox: true
prefixLength: "/31"
parentPrefixSelector: # The keys and values are case-sensitive
environment: "Production"
poolName: "Pool 1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit confused here. The custom field name is poolName. Shouldn't the filter in here then be cf_poolName? Or how do you distinguish custom fields from built in fields? I think it would make sense to be consistent with the netbox filtering https://demo.netbox.dev/static/docs/rest-api/filtering/#filtering-by-custom-field

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually filter out tenant and site, so we don't force the user to build the netbox API query string in the CRs.

Should we have the user enter the prefix cf_ when adding the entries to save us some implementation hassle, or, we handle this for the user, which looks nicer on the UX standpoint. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine both ways. If we handle cf_ for the user it's one one side nice but on the other side also confusing. Depends a bit whether we want to stick to the UI (handle it for the user) or API (the user has to specify cf_ himself). wdyt @bruelea ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For spec.customFields the cf_ is handled for the use, so we could do the same for parentPrefixSelector.

# environment: "production"
# poolName: "pool 3"
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
28 changes: 28 additions & 0 deletions config/samples/netbox_v1_prefixclaim_customfields_bool_int.yaml
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
apiVersion: netbox.dev/v1
kind: PrefixClaim
metadata:
labels:
app.kubernetes.io/name: netbox-operator
app.kubernetes.io/managed-by: kustomize
name: prefixclaim-customfields-sample-4
spec:
tenant: "MY_TENANT" # Use the `name` value instead of the `slug` value
site: "DM-Akron" # Use the `name` value instead of the `slug` value
description: "some description"
comments: "your comments"
preserveInNetbox: true
prefixLength: "/31"
parentPrefixSelector: # The keys and values are case-sensitive
# should return a prefix in 3.0.0.0/24
environment: "Production"
poolName: "Pool 1"
# same condition as above, should return a prefix in 3.0.0.0/24
# cfDataTypeBool: "true"
# cfDataTypeInteger: "1"

# should return a prefix in 3.0.2.0/24
# environment: "Development"
# poolName: "Pool 1"
# same condition as above, should return a prefix in 3.0.0.0/24
# cfDataTypeBool: "false"
# cfDataTypeInteger: "2"
5 changes: 2 additions & 3 deletions internal/controller/ipaddress_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,8 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// create lock
locked := ll.TryLock(lockCtx)
if !locked {
logger.Info(fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix))
r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
ipAddressClaim.Spec.ParentPrefix)
errorMsg := fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix)
r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)
return ctrl.Result{
RequeueAfter: 2 * time.Second,
}, nil
Expand Down
8 changes: 3 additions & 5 deletions internal/controller/ipaddressclaim_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import (
"time"

netboxv1 "github.com/netbox-community/netbox-operator/api/v1"
"github.com/netbox-community/netbox-operator/pkg/config"
"github.com/netbox-community/netbox-operator/pkg/netbox/api"
"github.com/netbox-community/netbox-operator/pkg/netbox/models"
"github.com/swisscom/leaselocker"
Expand Down Expand Up @@ -111,9 +110,8 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque
locked := ll.TryLock(lockCtx)
if !locked {
// lock for parent prefix was not available, rescheduling
logger.Info(fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix))
r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
o.Spec.ParentPrefix)
errorMsg := fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix)
r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)
return ctrl.Result{
RequeueAfter: 2 * time.Second,
}, nil
Expand All @@ -122,7 +120,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque

// 4. try to reclaim ip address
h := generateIpAddressRestorationHash(o)
ipAddressModel, err := r.NetboxClient.RestoreExistingIpByHash(config.GetOperatorConfig().NetboxRestorationHashFieldName, h)
ipAddressModel, err := r.NetboxClient.RestoreExistingIpByHash(h)
if err != nil {
if setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err.Error()); setConditionErr != nil {
return ctrl.Result{}, fmt.Errorf("error updating status: %w, looking up ip by hash failed: %w", setConditionErr, err)
Expand Down
52 changes: 32 additions & 20 deletions internal/controller/prefix_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,29 +132,42 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
return ctrl.Result{}, err
}

// get the name of the parent prefix
leaseLockerNSN := types.NamespacedName{
Name: convertCIDRToLeaseLockName(prefixClaim.Spec.ParentPrefix),
Namespace: r.OperatorNamespace,
}
ll, err = leaselocker.NewLeaseLocker(r.RestConfig, leaseLockerNSN, req.NamespacedName.String())
if err != nil {
return ctrl.Result{}, err
if prefixClaim.Status.ParentPrefix == "" {
henrybear327 marked this conversation as resolved.
Show resolved Hide resolved
// the parent prefix is not computed
if err := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, "the parent prefix is not computed"); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{
Requeue: true,
}, nil
}

lockCtx, cancel := context.WithCancel(ctx)
defer cancel()
if prefixClaim.Status.ParentPrefix != msgCanNotInferParentPrefix {
// we can't restore from the restoration hash

// create lock
if locked := ll.TryLock(lockCtx); !locked {
logger.Info(fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Spec.ParentPrefix))
r.Recorder.Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s",
prefixClaim.Spec.ParentPrefix)
return ctrl.Result{
RequeueAfter: 2 * time.Second,
}, nil
// get the name of the parent prefix
leaseLockerNSN := types.NamespacedName{
Name: convertCIDRToLeaseLockName(prefixClaim.Status.ParentPrefix),
Namespace: r.OperatorNamespace,
}
ll, err = leaselocker.NewLeaseLocker(r.RestConfig, leaseLockerNSN, req.NamespacedName.String())
if err != nil {
return ctrl.Result{}, err
}

lockCtx, cancel := context.WithCancel(ctx)
defer cancel()

// create lock
if locked := ll.TryLock(lockCtx); !locked {
errorMsg := fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Status.ParentPrefix)
r.Recorder.Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg)
return ctrl.Result{
RequeueAfter: 2 * time.Second,
}, nil
}
debugLogger.Info("successfully locked parent prefix %s", prefixClaim.Status.ParentPrefix)
}
debugLogger.Info("successfully locked parent prefix %s", prefixClaim.Spec.ParentPrefix)
}

/* 2. reserve or update Prefix in netbox */
Expand Down Expand Up @@ -218,7 +231,6 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
}

debugLogger.Info(fmt.Sprintf("reserved prefix in netbox, prefix: %s", prefix.Spec.Prefix))

if err = r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyTrue, corev1.EventTypeNormal, ""); err != nil {
return ctrl.Result{}, err
}
Expand Down
Loading