From ea07072b0df38149216e4022549b800577d9a9f7 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Thu, 13 Mar 2025 18:02:13 +0100 Subject: [PATCH 1/8] refactor event/status reporting adjust string separator add copyright header modified error return for prefixclaimcontroller implement feedback - logging message location only perform update call if the condition changed --- api/v1/ipaddress_types.go | 4 ++ api/v1/ipaddressclaim_types.go | 4 ++ api/v1/iprange_types.go | 4 ++ api/v1/iprangeclaim_types.go | 4 ++ api/v1/prefix_types.go | 4 ++ api/v1/prefixclaim_types.go | 4 ++ cmd/main.go | 72 +++++++++---------- internal/controller/interfaces.go | 27 +++++++ internal/controller/ipaddress_controller.go | 43 ++++------- .../controller/ipaddressclaim_controller.go | 42 ++++------- internal/controller/iprange_controller.go | 51 ++++--------- .../controller/iprangeclaim_controller.go | 61 +++++----------- internal/controller/prefix_controller.go | 46 ++++-------- internal/controller/prefixclaim_controller.go | 61 ++++++---------- internal/controller/suite_test.go | 12 ++-- internal/controller/utils.go | 40 +++++++++++ 16 files changed, 224 insertions(+), 255 deletions(-) create mode 100644 internal/controller/interfaces.go diff --git a/api/v1/ipaddress_types.go b/api/v1/ipaddress_types.go index 4af20c13..a038ac0e 100644 --- a/api/v1/ipaddress_types.go +++ b/api/v1/ipaddress_types.go @@ -99,6 +99,10 @@ type IpAddress struct { Status IpAddressStatus `json:"status,omitempty"` } +func (i *IpAddress) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + //+kubebuilder:object:root=true // IpAddressList contains a list of IpAddress diff --git a/api/v1/ipaddressclaim_types.go b/api/v1/ipaddressclaim_types.go index 641908d8..f290facb 100644 --- a/api/v1/ipaddressclaim_types.go +++ b/api/v1/ipaddressclaim_types.go @@ -104,6 +104,10 @@ type IpAddressClaim struct { Status IpAddressClaimStatus `json:"status,omitempty"` } +func (i *IpAddressClaim) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + //+kubebuilder:object:root=true // IpAddressClaimList contains a list of IpAddressClaim diff --git a/api/v1/iprange_types.go b/api/v1/iprange_types.go index b0ae287c..f521d0ab 100644 --- a/api/v1/iprange_types.go +++ b/api/v1/iprange_types.go @@ -108,6 +108,10 @@ type IpRange struct { Status IpRangeStatus `json:"status,omitempty"` } +func (i *IpRange) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + //+kubebuilder:object:root=true // IpRangeList contains a list of IpRange diff --git a/api/v1/iprangeclaim_types.go b/api/v1/iprangeclaim_types.go index fe859510..1fdd6600 100644 --- a/api/v1/iprangeclaim_types.go +++ b/api/v1/iprangeclaim_types.go @@ -133,6 +133,10 @@ type IpRangeClaim struct { Status IpRangeClaimStatus `json:"status,omitempty"` } +func (i *IpRangeClaim) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + //+kubebuilder:object:root=true // IpRangeClaimList contains a list of IpRangeClaim diff --git a/api/v1/prefix_types.go b/api/v1/prefix_types.go index 12d56d06..24586a66 100644 --- a/api/v1/prefix_types.go +++ b/api/v1/prefix_types.go @@ -104,6 +104,10 @@ type Prefix struct { Status PrefixStatus `json:"status,omitempty"` } +func (i *Prefix) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + // +kubebuilder:object:root=true // PrefixList contains a list of Prefix diff --git a/api/v1/prefixclaim_types.go b/api/v1/prefixclaim_types.go index f8d9facb..bed05b1c 100644 --- a/api/v1/prefixclaim_types.go +++ b/api/v1/prefixclaim_types.go @@ -132,6 +132,10 @@ type PrefixClaim struct { Status PrefixClaimStatus `json:"status,omitempty"` } +func (i *PrefixClaim) Conditions() *[]metav1.Condition { + return &i.Status.Conditions +} + // +kubebuilder:object:root=true // PrefixClaimList contains a list of PrefixClaim diff --git a/cmd/main.go b/cmd/main.go index 3fcb69ca..8b0e3cff 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -168,67 +168,67 @@ func main() { } if err = (&controller.IpAddressReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("ip-address-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("ip-address-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "IpAddress") os.Exit(1) } if err = (&controller.IpAddressClaimReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("ip-address-claim-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("ip-address-claim-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "IpAddressClaim") os.Exit(1) } if err = (&controller.PrefixReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("prefix-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("prefix-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "Prefix") os.Exit(1) } if err = (&controller.PrefixClaimReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("prefix-claim-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("prefix-claim-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "PrefixClaim") os.Exit(1) } if err = (&controller.IpRangeClaimReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("ip-range-claim-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("ip-range-claim-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "IpRangeClaim") os.Exit(1) } if err = (&controller.IpRangeReconciler{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), - Recorder: mgr.GetEventRecorderFor("ip-range-controller"), - NetboxClient: netboxClient, - OperatorNamespace: operatorNamespace, - RestConfig: mgr.GetConfig(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + EventStatusRecorder: controller.NewEventStatusRecorder(mgr.GetClient(), mgr.GetEventRecorderFor("ip-range-controller")), + NetboxClient: netboxClient, + OperatorNamespace: operatorNamespace, + RestConfig: mgr.GetConfig(), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "IpRange") os.Exit(1) diff --git a/internal/controller/interfaces.go b/internal/controller/interfaces.go new file mode 100644 index 00000000..5bff56b5 --- /dev/null +++ b/internal/controller/interfaces.go @@ -0,0 +1,27 @@ +/* +Copyright 2024 Swisscom (Schweiz) AG. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +type ObjectWithConditions interface { + client.Object + Conditions() *[]metav1.Condition +} diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index 24a98dd2..94eec6fc 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -33,11 +33,9 @@ import ( "github.com/swisscom/leaselocker" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -50,11 +48,11 @@ const IPManagedCustomFieldsAnnotationName = "ipaddress.netbox.dev/managed-custom // IpAddressReconciler reconciles a IpAddress object type IpAddressReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } //+kubebuilder:rbac:groups=netbox.dev,resources=ipaddresses,verbs=get;list;watch;create;update;patch;delete @@ -81,7 +79,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if !o.Spec.PreserveInNetbox { err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId) if err != nil { - setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err.Error()) + setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err) if setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting IPAddress failed: %w", setConditionErr, err) } @@ -148,7 +146,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( locked := ll.TryLock(lockCtx) if !locked { errorMsg := fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix) - r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -177,8 +175,8 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( 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 { + if updateStatusErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update ip address status: %w, "+ "after deletion of ip address cr failed: %w", updateStatusErr, err) } @@ -188,8 +186,8 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyFalse, - corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + if updateStatusErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, + corev1.EventTypeWarning, err, 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) } @@ -232,12 +230,12 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // check if created ip address contains entire description from spec _, found := strings.CutPrefix(netboxIpAddressModel.Description, req.NamespacedName.String()+" // "+o.Spec.Description) if !found { - r.Recorder.Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description") + r.EventStatusRecorder.rec.Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description") } debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress)) - err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, "") + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyTrue, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, err } @@ -254,21 +252,6 @@ func (r *IpAddressReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -func (r *IpAddressReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpAddress, condition metav1.Condition, eventType string, conditionMessageAppend string) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - err := r.Client.Status().Update(ctx, o) - if err != nil { - return err - } - return nil -} - func generateNetboxIpAddressModelFromIpAddressSpec(spec *netboxv1.IpAddressSpec, req ctrl.Request, lastIpAddressMetadata string) (*models.IPAddress, error) { // unmarshal lastIpAddressMetadata json string to map[string]string lastAppliedCustomFields := make(map[string]string) diff --git a/internal/controller/ipaddressclaim_controller.go b/internal/controller/ipaddressclaim_controller.go index 6eaeb1ae..620d3067 100644 --- a/internal/controller/ipaddressclaim_controller.go +++ b/internal/controller/ipaddressclaim_controller.go @@ -29,11 +29,9 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -43,11 +41,11 @@ import ( // IpAddressClaimReconciler reconciles a IpAddressClaim object type IpAddressClaimReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } //+kubebuilder:rbac:groups=netbox.dev,resources=ipaddressclaims,verbs=get;list;watch;create;update;patch;delete @@ -111,7 +109,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !locked { // lock for parent prefix was not available, rescheduling errorMsg := fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix) - r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -122,7 +120,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque h := generateIpAddressRestorationHash(o) ipAddressModel, err := r.NetboxClient.RestoreExistingIpByHash(h) if err != nil { - if setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err.Error()); setConditionErr != nil { + if setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, looking up ip by hash failed: %w", setConditionErr, err) } return ctrl.Result{Requeue: true}, nil @@ -139,7 +137,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque }, }) if err != nil { - if setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err.Error()); setConditionErr != nil { + if setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when assignment of ip address failed: %w", setConditionErr, err) } return ctrl.Result{Requeue: true}, nil @@ -160,14 +158,14 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque err = r.Client.Create(ctx, ipAddressResource) if err != nil { - setConditionErr := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, "") + setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err) if setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when creation of ip address object failed: %w", setConditionErr, err) } return ctrl.Result{}, err } - err = r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpAssignedTrue, corev1.EventTypeNormal, "") + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedTrue, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, err } @@ -202,13 +200,13 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque o.Status.IpAddress = ipAddress.Spec.IpAddress o.Status.IpAddressDotDecimal = strings.Split(ipAddress.Spec.IpAddress, "/")[0] o.Status.IpAddressName = ipAddress.Name - err := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpClaimReadyTrue, corev1.EventTypeNormal, "") + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpClaimReadyTrue, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, err } } else { debugLogger.Info("ipaddress status ready false") - err := r.SetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpClaimReadyFalse, corev1.EventTypeWarning, "") + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpClaimReadyFalse, corev1.EventTypeWarning, nil) if err != nil { return ctrl.Result{}, err } @@ -226,19 +224,3 @@ func (r *IpAddressClaimReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&netboxv1.IpAddress{}). Complete(r) } - -// SetConditionAndCreateEvent updates the condition and creates a log entry and event for this condition change -func (r *IpAddressClaimReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpAddressClaim, condition metav1.Condition, eventType string, conditionMessageAppend string) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - err := r.Client.Status().Update(ctx, o) - if err != nil { - return err - } - return nil -} diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 8b9ffa73..58886f71 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -32,11 +32,9 @@ import ( "github.com/swisscom/leaselocker" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -48,11 +46,11 @@ const IPRManagedCustomFieldsAnnotationName = "iprange.netbox.dev/managed-custom- // IpRangeReconciler reconciles a IpRange object type IpRangeReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } //+kubebuilder:rbac:groups=netbox.dev,resources=ipranges,verbs=get;list;watch;create;update;patch;delete @@ -78,8 +76,8 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if !o.Spec.PreserveInNetbox { err := r.NetboxClient.DeleteIpRange(o.Status.IpRangeId) if err != nil { - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed, - corev1.EventTypeWarning, "", err) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalseDeletionFailed, + corev1.EventTypeWarning, err) if err != nil { return ctrl.Result{}, err } @@ -122,7 +120,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct locked := ll.TryLock(lockCtx) if !locked { logger.Info(fmt.Sprintf("failed to lock parent prefix %s", parentPrefix)) - r.Recorder.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", + r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", parentPrefix) return ctrl.Result{Requeue: true}, nil } @@ -150,8 +148,8 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct 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 { + if err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + corev1.EventTypeWarning, err); err != nil { return ctrl.Result{}, err } return ctrl.Result{Requeue: true}, nil @@ -159,8 +157,8 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, nil } - if err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyFalse, - corev1.EventTypeWarning, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress), err); err != nil { + if err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalse, + corev1.EventTypeWarning, err, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress)); err != nil { return ctrl.Result{}, err } @@ -202,7 +200,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, err } - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeReadyTrue, corev1.EventTypeNormal, "", nil) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyTrue, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, err } @@ -219,27 +217,6 @@ func (r *IpRangeReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// logErrorSetConditionAndCreateEvent updates the condition and creates a log entry and event for this condition change and logs an error if errIn is not nil -func (r *IpRangeReconciler) logErrorSetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpRange, condition metav1.Condition, eventType string, conditionMessageAppend string, errIn error) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - - if errIn != nil { - // log errors here, so we do not need to aggregate them in the reconcile loop with the error of updating the status - logger := log.FromContext(ctx) - logger.Error(errIn, condition.Message+errIn.Error()) - condition.Message = condition.Message + ". Check the logs for more information." - } - - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - - return r.Client.Status().Update(ctx, o) -} - func (r *IpRangeReconciler) generateNetboxIpRangeModelFromIpRangeSpec(o *netboxv1.IpRange, req ctrl.Request, lastIpRangeMetadata string) (*models.IpRange, error) { // unmarshal lastIpRangeMetadata json string to map[string]string lastAppliedCustomFields := make(map[string]string) @@ -268,7 +245,7 @@ func (r *IpRangeReconciler) generateNetboxIpRangeModelFromIpRangeSpec(o *netboxv // check if created ip range contains entire description from spec _, found := strings.CutPrefix(description, req.NamespacedName.String()+" // "+o.Spec.Description) if !found { - r.Recorder.Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description") + r.EventStatusRecorder.rec.Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description") } return &models.IpRange{ diff --git a/internal/controller/iprangeclaim_controller.go b/internal/controller/iprangeclaim_controller.go index 4c48392d..3dad65f4 100644 --- a/internal/controller/iprangeclaim_controller.go +++ b/internal/controller/iprangeclaim_controller.go @@ -29,11 +29,9 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -45,11 +43,11 @@ const IpRangeClaimFinalizerName = "iprangeclaim.netbox.dev/finalizer" // IpRangeClaimReconciler reconciles a IpRangeClaim object type IpRangeClaimReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } //+kubebuilder:rbac:groups=netbox.dev,resources=iprangeclaims,verbs=get;list;watch;create;update;patch;delete @@ -124,15 +122,15 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request err = r.Client.Create(ctx, ipRangeResource) if err != nil { - errSetCondition := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, "", err) + errSetCondition := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err) if errSetCondition != nil { return ctrl.Result{}, errSetCondition } return ctrl.Result{}, err } - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedTrue, corev1.EventTypeNormal, - fmt.Sprintf(" , assigned ip range: %s-%s", ipRangeModel.StartAddress, ipRangeModel.EndAddress), nil) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedTrue, corev1.EventTypeNormal, + nil, fmt.Sprintf(" , assigned ip range: %s-%s", ipRangeModel.StartAddress, ipRangeModel.EndAddress)) if err != nil { return ctrl.Result{}, err } @@ -148,7 +146,7 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request } if !apismeta.IsStatusConditionTrue(ipRange.Status.Conditions, "Ready") { - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeClaimReadyFalse, corev1.EventTypeWarning, "", nil) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeClaimReadyFalse, corev1.EventTypeWarning, nil) if err != nil { return ctrl.Result{}, err } @@ -160,7 +158,7 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request o.Status, err = r.generateIpRangeClaimStatus(o, ipRange) if err != nil { logger.Error(err, "failed to generate ip range status") - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeClaimReadyFalseStatusGen, corev1.EventTypeWarning, "", err) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeClaimReadyFalseStatusGen, corev1.EventTypeWarning, err) if err != nil { return ctrl.Result{}, err } @@ -171,7 +169,7 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{}, err } - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeClaimReadyTrue, corev1.EventTypeNormal, "", nil) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeClaimReadyTrue, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, err @@ -188,32 +186,6 @@ func (r *IpRangeClaimReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// logErrorSetConditionAndCreateEvent updates the condition and creates a log entry and event for this condition change and logs an error if errIn is not nil -func (r *IpRangeClaimReconciler) logErrorSetConditionAndCreateEvent(ctx context.Context, o *netboxv1.IpRangeClaim, condition metav1.Condition, eventType string, conditionMessageAppend string, errIn error) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - - if errIn != nil { - // log errors here, so we do not need to aggregate them in the reconcile loop with the error of updating the status - logger := log.FromContext(ctx) - logger.Error(errIn, condition.Message+errIn.Error()) - condition.Message = condition.Message + ". Check the logs for more information." - } - - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - - err := r.Client.Status().Update(ctx, o) - if err != nil { - return err - } - - return nil -} - func (r *IpRangeClaimReconciler) tryLockOnParentPrefix(ctx context.Context, o *netboxv1.IpRangeClaim) (*leaselocker.LeaseLocker, ctrl.Result, error) { logger := log.FromContext(ctx) @@ -240,7 +212,7 @@ func (r *IpRangeClaimReconciler) tryLockOnParentPrefix(ctx context.Context, o *n 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", + r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", o.Spec.ParentPrefix) return nil, ctrl.Result{RequeueAfter: 2 * time.Second}, nil } @@ -290,7 +262,7 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte h := generateIpRangeRestorationHash(o) ipRangeModel, err := r.NetboxClient.RestoreExistingIpRangeByHash(h) if err != nil { - if err := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, "", err); err != nil { + if err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); err != nil { return nil, ctrl.Result{}, err } return nil, ctrl.Result{Requeue: true}, nil @@ -309,7 +281,7 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte }, ) if err != nil { - if err := r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, "", err); err != nil { + if err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); err != nil { return nil, ctrl.Result{}, err } return nil, ctrl.Result{Requeue: true}, nil @@ -320,9 +292,12 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte // check if the restored ip range has the size requested by the claim availableIpRanges, err := r.NetboxClient.GetAvailableIpAddressesByIpRange(ipRangeModel.Id) + if err != nil { + return nil, ctrl.Result{}, err + } if len(availableIpRanges.Payload) != o.Spec.Size { ll.Unlock() - err = r.logErrorSetConditionAndCreateEvent(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMismatch, corev1.EventTypeWarning, "", err) + err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalseSizeMismatch, corev1.EventTypeWarning, err) if err != nil { return nil, ctrl.Result{}, err } diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 6c3b7df2..fac50e6a 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -30,11 +30,9 @@ import ( "github.com/netbox-community/netbox-operator/pkg/netbox/models" corev1 "k8s.io/api/core/v1" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -51,11 +49,11 @@ const PXManagedCustomFieldsAnnotationName = "prefix.netbox.dev/managed-custom-fi // PrefixReconciler reconciles a Prefix object type PrefixReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } // +kubebuilder:rbac:groups=netbox.dev,resources=prefixes,verbs=get;list;watch;create;update;patch;delete @@ -81,7 +79,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { if !prefix.Spec.PreserveInNetbox { if err := r.NetboxClient.DeletePrefix(prefix.Status.PrefixId); err != nil { - setConditionErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalseDeletionFailed, corev1.EventTypeWarning, err.Error()) + setConditionErr := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalseDeletionFailed, corev1.EventTypeWarning, err) if setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting Prefix failed: %w", setConditionErr, err) } @@ -134,7 +132,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if prefixClaim.Status.SelectedParentPrefix == "" { // the parent prefix is not selected - if err := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, "the parent prefix is not selected"); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "the parent prefix is not selected")); err != nil { return ctrl.Result{}, err } return ctrl.Result{ @@ -161,7 +159,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // create lock if locked := ll.TryLock(lockCtx); !locked { errorMsg := fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Status.SelectedParentPrefix) - r.Recorder.Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.rec.Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -191,8 +189,8 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr 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 { + if updateStatusErr := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, + corev1.EventTypeWarning, err); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ "after deletion of prefix cr failed: %w", updateStatusErr, err) } @@ -201,8 +199,8 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } - if updateStatusErr := r.SetConditionAndCreateEvent(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, - corev1.EventTypeWarning, err.Error()); updateStatusErr != nil { + if updateStatusErr := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, + corev1.EventTypeWarning, err); updateStatusErr != nil { return ctrl.Result{}, fmt.Errorf("failed to update prefix status: %w, "+ "after reservation of prefix netbox failed: %w", updateStatusErr, err) } @@ -244,11 +242,11 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // check if the created prefix contains the entire description from spec if _, found := strings.CutPrefix(netboxPrefixModel.Description, req.NamespacedName.String()+" // "+prefix.Spec.Description); !found { - r.Recorder.Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description") + r.EventStatusRecorder.rec.Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description") } 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 { + if err = r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyTrue, corev1.EventTypeNormal, nil); err != nil { return ctrl.Result{}, err } @@ -264,22 +262,6 @@ func (r *PrefixReconciler) SetupWithManager(mgr ctrl.Manager) error { Complete(r) } -// TODO(henrybear327): Duplicated code, consider refactoring this -func (r *PrefixReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.Prefix, condition metav1.Condition, eventType string, conditionMessageAppend string) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - err := r.Client.Status().Update(ctx, o) - if err != nil { - return err - } - return nil -} - func generateNetboxPrefixModelFromPrefixSpec(spec *netboxv1.PrefixSpec, req ctrl.Request, lastPrefixMetadata string) (*models.Prefix, error) { // unmarshal lastPrefixMetadata json string to map[string]string lastAppliedCustomFields := make(map[string]string) diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index fa35f9b9..819c1f4e 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/internal/controller/prefixclaim_controller.go @@ -27,11 +27,9 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" apismeta "k8s.io/apimachinery/pkg/api/meta" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/rest" - "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -48,11 +46,11 @@ const ( // PrefixClaimReconciler reconciles a PrefixClaim object type PrefixClaimReconciler struct { client.Client - Scheme *runtime.Scheme - NetboxClient *api.NetboxClient - Recorder record.EventRecorder - OperatorNamespace string - RestConfig *rest.Config + Scheme *runtime.Scheme + NetboxClient *api.NetboxClient + EventStatusRecorder *EventStatusRecorder + OperatorNamespace string + RestConfig *rest.Config } // +kubebuilder:rbac:groups=netbox.dev,resources=prefixclaims,verbs=get;list;watch;create;update;patch;delete @@ -89,7 +87,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // set status, and condition field msg := fmt.Sprintf("parentPrefix is provided in CR: %v", prefixClaim.Status.SelectedParentPrefix) - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, msg); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); err != nil { return ctrl.Result{}, err } } else if len(prefixClaim.Spec.ParentPrefixSelector) > 0 { @@ -100,8 +98,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) h := generatePrefixRestorationHash(prefixClaim) canBeRestored, err := r.NetboxClient.RestoreExistingPrefixByHash(h) if err != nil { - msg := fmt.Sprintf("failed to look up prefix by hash: %v", err.Error()) - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, msg); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("failed to look up prefix by hash: %w", err)); err != nil { return ctrl.Result{}, err } @@ -139,7 +136,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // we write a special string in the ParentPrefix status field indicating the situation prefixClaim.Status.SelectedParentPrefix = msgCanNotInferParentPrefix - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, msgCanNotInferParentPrefix); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msgCanNotInferParentPrefix); err != nil { return ctrl.Result{}, err } } else { @@ -151,8 +148,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // fetch available prefixes from netbox parentPrefixCandidates, err := r.NetboxClient.GetAvailablePrefixByParentPrefixSelector(&prefixClaim.Spec) if err != nil || len(parentPrefixCandidates) == 0 { - errorMsg := fmt.Sprintf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %v, number of candidates = %v", err, len(parentPrefixCandidates)) - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, errorMsg); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %v, number of candidates = %v", err, len(parentPrefixCandidates))); err != nil { return ctrl.Result{}, err } @@ -166,13 +162,13 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // set status, and condition field msg := fmt.Sprintf("parentPrefix is selected: %v", prefixClaim.Status.SelectedParentPrefix) - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, msg); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); err != nil { return ctrl.Result{}, err } } } else { // this case should not be triggered anymore, as we have validation rules put in place on the CR - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, "either ParentPrefixSelector or ParentPrefix needs to be set"); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "either ParentPrefixSelector or ParentPrefix needs to be set")); err != nil { return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -214,7 +210,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) if !locked { // lock for parent prefix was not available, rescheduling errorMsg := fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Status.SelectedParentPrefix) - r.Recorder.Eventf(prefixClaim, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.rec.Eventf(prefixClaim, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -229,8 +225,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) h := generatePrefixRestorationHash(prefixClaim) prefixModel, err := r.NetboxClient.RestoreExistingPrefixByHash(h) if err != nil { - if setConditionErr := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err.Error()); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when look up of prefix by hash failed: %w", setConditionErr, err) + if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { + return ctrl.Result{}, setConditionErr } return ctrl.Result{Requeue: true}, nil } @@ -251,8 +247,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) }) if err != nil { if errors.Is(err, api.ErrParentPrefixExhausted) { - msg := fmt.Sprintf("%v, will restart the parent prefix selection process", err.Error()) - if setConditionErr := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, msg); setConditionErr != nil { + if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, fmt.Errorf("%w, will restart the parent prefix selection process", err)); setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when failed to get matching prefix: %w", setConditionErr, err) } @@ -262,7 +257,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{Requeue: true}, nil } - if setConditionErr := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err.Error()); setConditionErr != nil { + if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when failed to get matching prefix: %w", setConditionErr, err) } return ctrl.Result{Requeue: true}, nil @@ -283,13 +278,13 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) } err = r.Client.Create(ctx, prefixResource) if err != nil { - if setConditionErr := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, ""); setConditionErr != nil { + if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { return ctrl.Result{}, fmt.Errorf("error updating status: %w, when creation of prefix object failed: %w", setConditionErr, err) } return ctrl.Result{}, err } - if err = r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedTrue, corev1.EventTypeNormal, ""); err != nil { + if err = r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedTrue, corev1.EventTypeNormal, nil); err != nil { return ctrl.Result{}, err } } else { // Prefix object exists @@ -320,13 +315,13 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) prefixClaim.Status.Prefix = prefix.Spec.Prefix prefixClaim.Status.PrefixName = prefix.Name - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyTrue, corev1.EventTypeNormal, ""); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyTrue, corev1.EventTypeNormal, nil); err != nil { return ctrl.Result{}, err } } else { debugLogger.Info("prefix status ready false") - if err := r.SetConditionAndCreateEvent(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyFalse, corev1.EventTypeWarning, ""); err != nil { + if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyFalse, corev1.EventTypeWarning, nil); err != nil { return ctrl.Result{}, err } return ctrl.Result{Requeue: true}, nil @@ -344,19 +339,3 @@ func (r *PrefixClaimReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&netboxv1.Prefix{}). Complete(r) } - -// TODO(henrybear327): Duplicated code, consider refactoring this -func (r *PrefixClaimReconciler) SetConditionAndCreateEvent(ctx context.Context, o *netboxv1.PrefixClaim, condition metav1.Condition, eventType string, conditionMessageAppend string) error { - if len(conditionMessageAppend) > 0 { - condition.Message = condition.Message + ". " + conditionMessageAppend - } - conditionChanged := apismeta.SetStatusCondition(&o.Status.Conditions, condition) - if conditionChanged { - r.Recorder.Event(o, eventType, condition.Reason, condition.Message) - } - err := r.Client.Status().Update(ctx, o) - if err != nil { - return err - } - return nil -} diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index e992a65e..0579df86 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -118,9 +118,9 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred()) err = (&IpAddressReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - Recorder: k8sManager.GetEventRecorderFor("ip-address-claim-controller"), + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + EventStatusRecorder: NewEventStatusRecorder(k8sManager.GetClient(), k8sManager.GetEventRecorderFor("ip-address-controller")), NetboxClient: &api.NetboxClient{ Ipam: ipamMockIpAddress, Tenancy: tenancyMock, @@ -132,9 +132,9 @@ var _ = BeforeSuite(func() { Expect(err).ToNot(HaveOccurred()) err = (&IpAddressClaimReconciler{ - Client: k8sManager.GetClient(), - Scheme: k8sManager.GetScheme(), - Recorder: k8sManager.GetEventRecorderFor("ip-address-claim-controller"), + Client: k8sManager.GetClient(), + Scheme: k8sManager.GetScheme(), + EventStatusRecorder: NewEventStatusRecorder(k8sManager.GetClient(), k8sManager.GetEventRecorderFor("ip-address-claim-controller")), NetboxClient: &api.NetboxClient{ Ipam: ipamMockIpAddressClaim, Tenancy: tenancyMock, diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 8f3f9081..d793fbb1 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -22,6 +22,9 @@ import ( "fmt" "strings" + apismeta "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/tools/record" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" @@ -69,3 +72,40 @@ func addFinalizer(ctx context.Context, c client.Client, o client.Object, finaliz return nil } + +type EventStatusRecorder struct { + client client.Client + rec record.EventRecorder +} + +func NewEventStatusRecorder(client client.Client, rec record.EventRecorder) *EventStatusRecorder { + return &EventStatusRecorder{ + client: client, + rec: rec, + } +} + +func (esr *EventStatusRecorder) Report(ctx context.Context, o ObjectWithConditions, condition metav1.Condition, eventType string, errExt error, additionalMessages ...string) error { + logger := log.FromContext(ctx) + + if errExt != nil { + condition.Message = condition.Message + ", " + errExt.Error() + logger.Error(errExt, condition.Message) + } + + condition.Message = strings.Join(append([]string{condition.Message}, additionalMessages...), ", ") + condition.ObservedGeneration = o.GetGeneration() + + conditionChanged := apismeta.SetStatusCondition(o.Conditions(), condition) + if conditionChanged { + esr.rec.Event(o, eventType, condition.Reason, condition.Message) + logger.Info("Condition "+condition.Type+" changed to "+string(condition.Status), "Reason", condition.Reason, "Message", condition.Message) + + err := esr.client.Status().Update(ctx, o) + if err != nil { + return err + } + } + + return nil +} From c285549753d3dec582ece80e7e837ee3284acf0c Mon Sep 17 00:00:00 2001 From: Alexander North Date: Wed, 16 Apr 2025 10:20:32 +0200 Subject: [PATCH 2/8] don't use private fields of EventStatusRecorder --- internal/controller/ipaddress_controller.go | 4 ++-- internal/controller/ipaddressclaim_controller.go | 2 +- internal/controller/iprange_controller.go | 4 ++-- internal/controller/iprangeclaim_controller.go | 2 +- internal/controller/prefix_controller.go | 4 ++-- internal/controller/prefixclaim_controller.go | 2 +- internal/controller/utils.go | 4 ++++ 7 files changed, 13 insertions(+), 9 deletions(-) diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index 94eec6fc..2d60ef0d 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -146,7 +146,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( locked := ll.TryLock(lockCtx) if !locked { errorMsg := fmt.Sprintf("failed to lock parent prefix %s", ipAddressClaim.Spec.ParentPrefix) - r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -230,7 +230,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // check if created ip address contains entire description from spec _, found := strings.CutPrefix(netboxIpAddressModel.Description, req.NamespacedName.String()+" // "+o.Spec.Description) if !found { - r.EventStatusRecorder.rec.Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description") + r.EventStatusRecorder.Recorder().Event(o, corev1.EventTypeWarning, "IpDescriptionTruncated", "ip address was created with truncated description") } debugLogger.Info(fmt.Sprintf("reserved ip address in netbox, ip: %s", o.Spec.IpAddress)) diff --git a/internal/controller/ipaddressclaim_controller.go b/internal/controller/ipaddressclaim_controller.go index 620d3067..440248c8 100644 --- a/internal/controller/ipaddressclaim_controller.go +++ b/internal/controller/ipaddressclaim_controller.go @@ -109,7 +109,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque if !locked { // lock for parent prefix was not available, rescheduling errorMsg := fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix) - r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 58886f71..8e4e22a6 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -120,7 +120,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct locked := ll.TryLock(lockCtx) if !locked { logger.Info(fmt.Sprintf("failed to lock parent prefix %s", parentPrefix)) - r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", + r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", parentPrefix) return ctrl.Result{Requeue: true}, nil } @@ -245,7 +245,7 @@ func (r *IpRangeReconciler) generateNetboxIpRangeModelFromIpRangeSpec(o *netboxv // check if created ip range contains entire description from spec _, found := strings.CutPrefix(description, req.NamespacedName.String()+" // "+o.Spec.Description) if !found { - r.EventStatusRecorder.rec.Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description") + r.EventStatusRecorder.Recorder().Event(o, corev1.EventTypeWarning, "IpRangeDescriptionTruncated", "ip range was created with truncated description") } return &models.IpRange{ diff --git a/internal/controller/iprangeclaim_controller.go b/internal/controller/iprangeclaim_controller.go index 3dad65f4..fb7abcd0 100644 --- a/internal/controller/iprangeclaim_controller.go +++ b/internal/controller/iprangeclaim_controller.go @@ -212,7 +212,7 @@ func (r *IpRangeClaimReconciler) tryLockOnParentPrefix(ctx context.Context, o *n if !locked { // lock for parent prefix was not available, rescheduling logger.Info(fmt.Sprintf("failed to lock parent prefix %s", o.Spec.ParentPrefix)) - r.EventStatusRecorder.rec.Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", + r.EventStatusRecorder.Recorder().Eventf(o, corev1.EventTypeWarning, "FailedToLockParentPrefix", "failed to lock parent prefix %s", o.Spec.ParentPrefix) return nil, ctrl.Result{RequeueAfter: 2 * time.Second}, nil } diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index fac50e6a..20755fa1 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -159,7 +159,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // create lock if locked := ll.TryLock(lockCtx); !locked { errorMsg := fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Status.SelectedParentPrefix) - r.EventStatusRecorder.rec.Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.Recorder().Eventf(prefix, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil @@ -242,7 +242,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // check if the created prefix contains the entire description from spec if _, found := strings.CutPrefix(netboxPrefixModel.Description, req.NamespacedName.String()+" // "+prefix.Spec.Description); !found { - r.EventStatusRecorder.rec.Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description") + r.EventStatusRecorder.Recorder().Event(prefix, corev1.EventTypeWarning, "PrefixDescriptionTruncated", "prefix was created with truncated description") } debugLogger.Info(fmt.Sprintf("reserved prefix in netbox, prefix: %s", prefix.Spec.Prefix)) diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index 819c1f4e..278f6d44 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/internal/controller/prefixclaim_controller.go @@ -210,7 +210,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) if !locked { // lock for parent prefix was not available, rescheduling errorMsg := fmt.Sprintf("failed to lock parent prefix %s", prefixClaim.Status.SelectedParentPrefix) - r.EventStatusRecorder.rec.Eventf(prefixClaim, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) + r.EventStatusRecorder.Recorder().Eventf(prefixClaim, corev1.EventTypeWarning, "FailedToLockParentPrefix", errorMsg) return ctrl.Result{ RequeueAfter: 2 * time.Second, }, nil diff --git a/internal/controller/utils.go b/internal/controller/utils.go index d793fbb1..7480ae3e 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -109,3 +109,7 @@ func (esr *EventStatusRecorder) Report(ctx context.Context, o ObjectWithConditio return nil } + +func (esr *EventStatusRecorder) Recorder() record.EventRecorder { + return esr.rec +} From 6f4d3e0de342375e801ab84d1f698dd6cb8162e6 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Wed, 16 Apr 2025 11:13:52 +0200 Subject: [PATCH 3/8] set ready false at beginning of reconcile loop --- api/v1/shared_conditions.go | 10 ++++++++++ internal/controller/ipaddress_controller.go | 8 ++++++++ internal/controller/ipaddressclaim_controller.go | 8 ++++++++ internal/controller/iprange_controller.go | 8 ++++++++ internal/controller/iprangeclaim_controller.go | 8 ++++++++ internal/controller/prefix_controller.go | 8 ++++++++ internal/controller/prefixclaim_controller.go | 8 ++++++++ 7 files changed, 58 insertions(+) create mode 100644 api/v1/shared_conditions.go diff --git a/api/v1/shared_conditions.go b/api/v1/shared_conditions.go new file mode 100644 index 00000000..a669cc25 --- /dev/null +++ b/api/v1/shared_conditions.go @@ -0,0 +1,10 @@ +package v1 + +import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + +var ConditionReadyFalseNewResource = metav1.Condition{ + Type: "Ready", + Status: "False", + Reason: "NewResource", + Message: "Pending Reconciliation", +} diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index 2d60ef0d..d3bee4c2 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -113,6 +113,14 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } + // Set ready to false initially + if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + // 1. try to lock lease of parent prefix if IpAddress status condition is not true // and IpAddress is owned by an IpAddressClaim or := o.ObjectMeta.OwnerReferences diff --git a/internal/controller/ipaddressclaim_controller.go b/internal/controller/ipaddressclaim_controller.go index 440248c8..975424fe 100644 --- a/internal/controller/ipaddressclaim_controller.go +++ b/internal/controller/ipaddressclaim_controller.go @@ -74,6 +74,14 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, nil } + // Set ready to false initially + if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + // 1. check if matching IpAddress object already exists ipAddress := &netboxv1.IpAddress{} ipAddressName := o.ObjectMeta.Name diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 8e4e22a6..6b75d477 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -89,6 +89,14 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, removeFinalizer(ctx, r.Client, o, IpRangeFinalizerName) } + // Set ready to false initially + if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + // if PreserveIpInNetbox flag is false then register finalizer if not yet registered if !o.Spec.PreserveInNetbox { err = addFinalizer(ctx, r.Client, o, IpRangeFinalizerName) diff --git a/internal/controller/iprangeclaim_controller.go b/internal/controller/iprangeclaim_controller.go index fb7abcd0..3af74ea1 100644 --- a/internal/controller/iprangeclaim_controller.go +++ b/internal/controller/iprangeclaim_controller.go @@ -94,6 +94,14 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request return ctrl.Result{Requeue: true}, nil } + // Set ready to false initially + if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + err = r.Client.Get(ctx, ipRangeLookupKey, ipRange) if err != nil { // return error if not a notfound error diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 20755fa1..950c8029 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -102,6 +102,14 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } + // Set ready to false initially + if apismeta.FindStatusCondition(prefix.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + // register finalizer if not yet registered if !prefix.Spec.PreserveInNetbox && !controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { debugLogger.Info("adding the finalizer") diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index 278f6d44..698a0d77 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/internal/controller/prefixclaim_controller.go @@ -78,6 +78,14 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } + // Set ready to false initially + if apismeta.FindStatusCondition(prefixClaim.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { + err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) + } + } + /* 1. compute and assign the parent prefix if required */ // The current design will use prefixClaim.Status.ParentPrefix for storing the selected parent prefix, // and as the source of truth for future parent prefix references From 25646ecc7ee3d32bd093367cc3f81609819f02b4 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Wed, 16 Apr 2025 11:21:38 +0200 Subject: [PATCH 4/8] add licence header --- api/v1/shared_conditions.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/api/v1/shared_conditions.go b/api/v1/shared_conditions.go index a669cc25..7a77eac8 100644 --- a/api/v1/shared_conditions.go +++ b/api/v1/shared_conditions.go @@ -1,3 +1,19 @@ +/* +Copyright 2024 Swisscom (Schweiz) AG. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + package v1 import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" From 197dc991dd061aa3e8bbb8181ce80d2e78d0751c Mon Sep 17 00:00:00 2001 From: Alexander North Date: Wed, 16 Apr 2025 13:05:05 +0200 Subject: [PATCH 5/8] add information about kind image loading with containerd --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 0f1a44f4..796ad3c7 100644 --- a/README.md +++ b/README.md @@ -30,7 +30,10 @@ There are several ways to install NetBox Operator on your Cluster: ## Running both NetBox Operator and NetBox on a local kind cluster -Note: This requires Docker BuildKit. +> **Note:** This requires Docker BuildKit. + +> **Note:** There is currently a bug where if Docker uses containerd for pulling and storing images, kind fails to load the image into the cluster ([GitHub Issue](https://github.com/kubernetes-sigs/kind/issues/3795)). +To resolve this, temporarily disable this option in the Docker settings: "General > Use containerd for pulling and storing images" - Create kind cluster with a NetBox deployment: `make create-kind` - Deploy the NetBox Operator on the local kind cluster: `make deploy-kind` (In case you're using podman use `CONTAINER_TOOL=podman make deploy-kind`) From a454ddea7c8f2913d4180e2505c7894ad77f6de0 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Wed, 16 Apr 2025 16:16:30 +0200 Subject: [PATCH 6/8] fix match string for chainsaw tests --- internal/controller/iprange_controller.go | 2 +- internal/controller/utils.go | 2 +- .../chainsaw-test.yaml | 2 +- .../IPv4/prefixclaim-ipv4-prefixexhausted/chainsaw-test.yaml | 2 +- .../ipv4/ipaddressclaim-ipv4-prefixexhausted/chainsaw-test.yaml | 2 +- .../ipv6/ipaddressclaim-ipv6-prefixexhausted/chainsaw-test.yaml | 2 +- .../chainsaw-test.yaml | 2 +- .../chainsaw-test.yaml | 2 +- .../iprangeclaim-ipv4-invalid-parentprefix/chainsaw-test.yaml | 2 +- .../ipv4/iprangeclaim-ipv4-invalid-tenant/chainsaw-test.yaml | 2 +- .../ipv4/iprangeclaim-ipv4-prefixexhausted/chainsaw-test.yaml | 2 +- .../ipv6/iprangeclaim-ipv6-prefixexhausted/chainsaw-test.yaml | 2 +- 12 files changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 6b75d477..7051cdb5 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -166,7 +166,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } if err = r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeReadyFalse, - corev1.EventTypeWarning, err, fmt.Sprintf("%s-%s ", o.Spec.StartAddress, o.Spec.EndAddress)); err != nil { + corev1.EventTypeWarning, err, fmt.Sprintf("range: %s-%s", o.Spec.StartAddress, o.Spec.EndAddress)); err != nil { return ctrl.Result{}, err } diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 7480ae3e..5733c4ef 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -89,7 +89,7 @@ func (esr *EventStatusRecorder) Report(ctx context.Context, o ObjectWithConditio logger := log.FromContext(ctx) if errExt != nil { - condition.Message = condition.Message + ", " + errExt.Error() + condition.Message = condition.Message + ": " + errExt.Error() logger.Error(errExt, condition.Message) } diff --git a/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-invalid-parentprefixselector/chainsaw-test.yaml b/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-invalid-parentprefixselector/chainsaw-test.yaml index f1926c02..34a876ef 100644 --- a/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-invalid-parentprefixselector/chainsaw-test.yaml +++ b/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-invalid-parentprefixselector/chainsaw-test.yaml @@ -45,7 +45,7 @@ spec: reason: ParentPrefixNotSelected source: component: prefix-claim-controller - message: "The parent prefix was not able to be selected. no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = failed to fetch tenant 'niltenant': not found, number of candidates = 0" + message: "The parent prefix was not able to be selected: no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = failed to fetch tenant 'niltenant': not found, number of candidates = 0" involvedObject: apiVersion: netbox.dev/v1 kind: PrefixClaim diff --git a/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-prefixexhausted/chainsaw-test.yaml b/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-prefixexhausted/chainsaw-test.yaml index a1e14cef..a371ca73 100644 --- a/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-prefixexhausted/chainsaw-test.yaml +++ b/tests/e2e/Prefix/IPv4/prefixclaim-ipv4-prefixexhausted/chainsaw-test.yaml @@ -94,7 +94,7 @@ spec: reason: PrefixCRNotCreated source: component: prefix-claim-controller - message: Failed to fetch new Prefix from NetBox. parent prefix exhausted, will restart the parent prefix selection process + message: "Failed to fetch new Prefix from NetBox: parent prefix exhausted, will restart the parent prefix selection process" involvedObject: apiVersion: netbox.dev/v1 kind: PrefixClaim diff --git a/tests/e2e/ipaddress/ipv4/ipaddressclaim-ipv4-prefixexhausted/chainsaw-test.yaml b/tests/e2e/ipaddress/ipv4/ipaddressclaim-ipv4-prefixexhausted/chainsaw-test.yaml index bd9fd4a9..2c9cafe4 100644 --- a/tests/e2e/ipaddress/ipv4/ipaddressclaim-ipv4-prefixexhausted/chainsaw-test.yaml +++ b/tests/e2e/ipaddress/ipv4/ipaddressclaim-ipv4-prefixexhausted/chainsaw-test.yaml @@ -129,7 +129,7 @@ spec: reason: IPAddressCRNotCreated source: component: ip-address-claim-controller - message: Failed to fetch new IP from NetBox. parent prefix exhausted + message: "Failed to fetch new IP from NetBox: parent prefix exhausted" involvedObject: apiVersion: netbox.dev/v1 kind: IpAddressClaim diff --git a/tests/e2e/ipaddress/ipv6/ipaddressclaim-ipv6-prefixexhausted/chainsaw-test.yaml b/tests/e2e/ipaddress/ipv6/ipaddressclaim-ipv6-prefixexhausted/chainsaw-test.yaml index ddaafbd0..a5b0f503 100644 --- a/tests/e2e/ipaddress/ipv6/ipaddressclaim-ipv6-prefixexhausted/chainsaw-test.yaml +++ b/tests/e2e/ipaddress/ipv6/ipaddressclaim-ipv6-prefixexhausted/chainsaw-test.yaml @@ -129,7 +129,7 @@ spec: reason: IPAddressCRNotCreated source: component: ip-address-claim-controller - message: Failed to fetch new IP from NetBox. parent prefix exhausted + message: "Failed to fetch new IP from NetBox: parent prefix exhausted" involvedObject: apiVersion: netbox.dev/v1 kind: IpAddressClaim diff --git a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldnotexisting/chainsaw-test.yaml b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldnotexisting/chainsaw-test.yaml index 5a3e3e2e..e7f91f3e 100644 --- a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldnotexisting/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldnotexisting/chainsaw-test.yaml @@ -55,7 +55,7 @@ spec: reason: FailedToReserveIPRangeInNetbox source: component: ip-range-controller - message: Failed to reserve IP Range in NetBox. 3.2.3.1/32-3.2.3.30/32 . Check the logs for more information. + message: "Failed to reserve IP Range in NetBox: failed to reserve IP Range: [POST /ipam/ip-ranges/][400] ipam_ip-ranges_create default map[__all__:[Unknown field name 'justmade' in custom field data.]], range: 3.2.3.1/32-3.2.3.30/32" involvedObject: apiVersion: netbox.dev/v1 kind: IpRange diff --git a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldwrongdatatype/chainsaw-test.yaml b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldwrongdatatype/chainsaw-test.yaml index 29c26e14..cabb572a 100644 --- a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldwrongdatatype/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-customfieldwrongdatatype/chainsaw-test.yaml @@ -55,7 +55,7 @@ spec: reason: FailedToReserveIPRangeInNetbox source: component: ip-range-controller - message: Failed to reserve IP Range in NetBox. 3.2.3.1/32-3.2.3.30/32 . Check the logs for more information. + message: "Failed to reserve IP Range in NetBox: failed to reserve IP Range: [POST /ipam/ip-ranges/][400] ipam_ip-ranges_create default map[__all__:[Unknown field name 'cfDataTypeInteger' in custom field data.]], range: 3.2.3.1/32-3.2.3.30/32" involvedObject: apiVersion: netbox.dev/v1 kind: IpRange diff --git a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-parentprefix/chainsaw-test.yaml b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-parentprefix/chainsaw-test.yaml index 9c276496..641d343e 100644 --- a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-parentprefix/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-parentprefix/chainsaw-test.yaml @@ -30,7 +30,7 @@ spec: reason: IPRangeCRNotCreated source: component: ip-range-claim-controller - message: Failed to fetch new IP Range from NetBox. Check the logs for more information. + message: "Failed to fetch new IP Range from NetBox: failed to fetch parent prefix: not found" involvedObject: apiVersion: netbox.dev/v1 kind: IpRangeClaim diff --git a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-tenant/chainsaw-test.yaml b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-tenant/chainsaw-test.yaml index 57eda9c7..baf1024a 100644 --- a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-tenant/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-invalid-tenant/chainsaw-test.yaml @@ -36,7 +36,7 @@ spec: reason: IPRangeCRNotCreated source: component: ip-range-claim-controller - message: Failed to fetch new IP Range from NetBox. Check the logs for more information. + message: "Failed to fetch new IP Range from NetBox: failed to fetch tenant 'nonexistingtenant': not found" involvedObject: apiVersion: netbox.dev/v1 kind: IpRangeClaim diff --git a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-prefixexhausted/chainsaw-test.yaml b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-prefixexhausted/chainsaw-test.yaml index 53134a22..77691c69 100644 --- a/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-prefixexhausted/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv4/iprangeclaim-ipv4-prefixexhausted/chainsaw-test.yaml @@ -138,7 +138,7 @@ spec: reason: IPRangeCRNotCreated source: component: ip-range-claim-controller - message: Failed to fetch new IP Range from NetBox. Check the logs for more information. + message: "Failed to fetch new IP Range from NetBox: not enough consecutive IPs available" involvedObject: apiVersion: netbox.dev/v1 kind: IpRangeClaim diff --git a/tests/e2e/iprange/ipv6/iprangeclaim-ipv6-prefixexhausted/chainsaw-test.yaml b/tests/e2e/iprange/ipv6/iprangeclaim-ipv6-prefixexhausted/chainsaw-test.yaml index 3ed967a0..173d7bc1 100644 --- a/tests/e2e/iprange/ipv6/iprangeclaim-ipv6-prefixexhausted/chainsaw-test.yaml +++ b/tests/e2e/iprange/ipv6/iprangeclaim-ipv6-prefixexhausted/chainsaw-test.yaml @@ -139,7 +139,7 @@ spec: reason: IPRangeCRNotCreated source: component: ip-range-claim-controller - message: Failed to fetch new IP Range from NetBox. Check the logs for more information. + message: "Failed to fetch new IP Range from NetBox: not enough consecutive IPs available" involvedObject: apiVersion: netbox.dev/v1 kind: IpRangeClaim From 9ba9d02b62456f4ba442da6c0813442fed5a34e9 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Thu, 17 Apr 2025 14:18:48 +0200 Subject: [PATCH 7/8] implement feedback --- api/v1/prefix_types.go | 4 +- api/v1/prefixclaim_types.go | 4 +- internal/controller/ipaddress_controller.go | 8 ++- .../controller/ipaddressclaim_controller.go | 15 +++--- internal/controller/iprange_controller.go | 2 +- .../controller/iprangeclaim_controller.go | 13 +++-- internal/controller/prefix_controller.go | 11 ++-- internal/controller/prefixclaim_controller.go | 50 +++++++++---------- 8 files changed, 53 insertions(+), 54 deletions(-) diff --git a/api/v1/prefix_types.go b/api/v1/prefix_types.go index 24586a66..57663582 100644 --- a/api/v1/prefix_types.go +++ b/api/v1/prefix_types.go @@ -104,8 +104,8 @@ type Prefix struct { Status PrefixStatus `json:"status,omitempty"` } -func (i *Prefix) Conditions() *[]metav1.Condition { - return &i.Status.Conditions +func (p *Prefix) Conditions() *[]metav1.Condition { + return &p.Status.Conditions } // +kubebuilder:object:root=true diff --git a/api/v1/prefixclaim_types.go b/api/v1/prefixclaim_types.go index bed05b1c..a5616b25 100644 --- a/api/v1/prefixclaim_types.go +++ b/api/v1/prefixclaim_types.go @@ -132,8 +132,8 @@ type PrefixClaim struct { Status PrefixClaimStatus `json:"status,omitempty"` } -func (i *PrefixClaim) Conditions() *[]metav1.Condition { - return &i.Status.Conditions +func (p *PrefixClaim) Conditions() *[]metav1.Condition { + return &p.Status.Conditions } // +kubebuilder:object:root=true diff --git a/internal/controller/ipaddress_controller.go b/internal/controller/ipaddress_controller.go index d3bee4c2..03217331 100644 --- a/internal/controller/ipaddress_controller.go +++ b/internal/controller/ipaddress_controller.go @@ -79,11 +79,9 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( if !o.Spec.PreserveInNetbox { err := r.NetboxClient.DeleteIpAddress(o.Status.IpAddressId) if err != nil { - setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err) - if setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting IPAddress failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalseDeletionFailed, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } - return ctrl.Result{Requeue: true}, nil } } @@ -115,7 +113,7 @@ func (r *IpAddressReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Set ready to false initially if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } diff --git a/internal/controller/ipaddressclaim_controller.go b/internal/controller/ipaddressclaim_controller.go index 975424fe..1216d0e1 100644 --- a/internal/controller/ipaddressclaim_controller.go +++ b/internal/controller/ipaddressclaim_controller.go @@ -76,7 +76,7 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque // Set ready to false initially if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } @@ -128,8 +128,8 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque h := generateIpAddressRestorationHash(o) ipAddressModel, err := r.NetboxClient.RestoreExistingIpByHash(h) if err != nil { - if setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, looking up ip by hash failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil } @@ -145,8 +145,8 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque }, }) if err != nil { - if setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when assignment of ip address failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil } @@ -166,9 +166,8 @@ func (r *IpAddressClaimReconciler) Reconcile(ctx context.Context, req ctrl.Reque err = r.Client.Create(ctx, ipAddressResource) if err != nil { - setConditionErr := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err) - if setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when creation of ip address object failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{}, err } diff --git a/internal/controller/iprange_controller.go b/internal/controller/iprange_controller.go index 7051cdb5..4a80d53e 100644 --- a/internal/controller/iprange_controller.go +++ b/internal/controller/iprange_controller.go @@ -91,7 +91,7 @@ func (r *IpRangeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // Set ready to false initially if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } diff --git a/internal/controller/iprangeclaim_controller.go b/internal/controller/iprangeclaim_controller.go index 3af74ea1..4f48d8f0 100644 --- a/internal/controller/iprangeclaim_controller.go +++ b/internal/controller/iprangeclaim_controller.go @@ -96,7 +96,7 @@ func (r *IpRangeClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request // Set ready to false initially if apismeta.FindStatusCondition(o.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } @@ -270,8 +270,8 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte h := generateIpRangeRestorationHash(o) ipRangeModel, err := r.NetboxClient.RestoreExistingIpRangeByHash(h) if err != nil { - if err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); err != nil { - return nil, ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return nil, ctrl.Result{}, errReport } return nil, ctrl.Result{Requeue: true}, nil } @@ -289,8 +289,8 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte }, ) if err != nil { - if err := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); err != nil { - return nil, ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return nil, ctrl.Result{}, errReport } return nil, ctrl.Result{Requeue: true}, nil } @@ -301,6 +301,9 @@ func (r *IpRangeClaimReconciler) restoreOrAssignIpRangeAndSetCondition(ctx conte // check if the restored ip range has the size requested by the claim availableIpRanges, err := r.NetboxClient.GetAvailableIpAddressesByIpRange(ipRangeModel.Id) if err != nil { + if errReport := r.EventStatusRecorder.Report(ctx, o, netboxv1.ConditionIpRangeClaimReadyFalse, corev1.EventTypeWarning, err, "failed getting available IP Addresses By Range"); errReport != nil { + return nil, ctrl.Result{}, errReport + } return nil, ctrl.Result{}, err } if len(availableIpRanges.Payload) != o.Spec.Size { diff --git a/internal/controller/prefix_controller.go b/internal/controller/prefix_controller.go index 950c8029..2b132d3e 100644 --- a/internal/controller/prefix_controller.go +++ b/internal/controller/prefix_controller.go @@ -79,9 +79,8 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if controllerutil.ContainsFinalizer(prefix, PrefixFinalizerName) { if !prefix.Spec.PreserveInNetbox { if err := r.NetboxClient.DeletePrefix(prefix.Status.PrefixId); err != nil { - setConditionErr := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalseDeletionFailed, corev1.EventTypeWarning, err) - if setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when deleting Prefix failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalseDeletionFailed, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil @@ -104,7 +103,7 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // Set ready to false initially if apismeta.FindStatusCondition(prefix.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } @@ -140,8 +139,8 @@ func (r *PrefixReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr if prefixClaim.Status.SelectedParentPrefix == "" { // the parent prefix is not selected - if err := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "the parent prefix is not selected")); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefix, netboxv1.ConditionPrefixReadyFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "the parent prefix is not selected")); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{ Requeue: true, diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index 698a0d77..c4bcc933 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/internal/controller/prefixclaim_controller.go @@ -80,7 +80,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // Set ready to false initially if apismeta.FindStatusCondition(prefixClaim.Status.Conditions, netboxv1.ConditionReadyFalseNewResource.Type) == nil { - err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionIpaddressReadyFalse, corev1.EventTypeNormal, nil) + err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionReadyFalseNewResource, corev1.EventTypeNormal, nil) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to initialise Ready condition: %w, ", err) } @@ -95,8 +95,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // set status, and condition field msg := fmt.Sprintf("parentPrefix is provided in CR: %v", prefixClaim.Status.SelectedParentPrefix) - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); errReport != nil { + return ctrl.Result{}, errReport } } else if len(prefixClaim.Spec.ParentPrefixSelector) > 0 { // we first check if a prefix can be restored from the netbox @@ -106,8 +106,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) h := generatePrefixRestorationHash(prefixClaim) canBeRestored, err := r.NetboxClient.RestoreExistingPrefixByHash(h) if err != nil { - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("failed to look up prefix by hash: %w", err)); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("failed to look up prefix by hash: %w", err)); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil @@ -144,8 +144,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // we write a special string in the ParentPrefix status field indicating the situation prefixClaim.Status.SelectedParentPrefix = msgCanNotInferParentPrefix - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msgCanNotInferParentPrefix); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msgCanNotInferParentPrefix); errReport != nil { + return ctrl.Result{}, errReport } } else { // No, so we need to select one parent prefix from prefix candidates @@ -156,8 +156,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // fetch available prefixes from netbox parentPrefixCandidates, err := r.NetboxClient.GetAvailablePrefixByParentPrefixSelector(&prefixClaim.Spec) if err != nil || len(parentPrefixCandidates) == 0 { - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %v, number of candidates = %v", err, len(parentPrefixCandidates))); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %v, number of candidates = %v", err, len(parentPrefixCandidates))); errReport != nil { + return ctrl.Result{}, errReport } // we requeue as this might be a temporary prefix exhausation @@ -170,14 +170,14 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // set status, and condition field msg := fmt.Sprintf("parentPrefix is selected: %v", prefixClaim.Status.SelectedParentPrefix) - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedTrue, corev1.EventTypeNormal, nil, msg); errReport != nil { + return ctrl.Result{}, errReport } } } else { // this case should not be triggered anymore, as we have validation rules put in place on the CR - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "either ParentPrefixSelector or ParentPrefix needs to be set")); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("%s", "either ParentPrefixSelector or ParentPrefix needs to be set")); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{}, nil } @@ -233,8 +233,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) h := generatePrefixRestorationHash(prefixClaim) prefixModel, err := r.NetboxClient.RestoreExistingPrefixByHash(h) if err != nil { - if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { - return ctrl.Result{}, setConditionErr + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil } @@ -255,8 +255,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) }) if err != nil { if errors.Is(err, api.ErrParentPrefixExhausted) { - if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, fmt.Errorf("%w, will restart the parent prefix selection process", err)); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when failed to get matching prefix: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, fmt.Errorf("%w, will restart the parent prefix selection process", err)); errReport != nil { + return ctrl.Result{}, errReport } // we reset the selected parent prefix, since this one is already exhausted @@ -265,8 +265,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{Requeue: true}, nil } - if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when failed to get matching prefix: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil } @@ -286,8 +286,8 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) } err = r.Client.Create(ctx, prefixResource) if err != nil { - if setConditionErr := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); setConditionErr != nil { - return ctrl.Result{}, fmt.Errorf("error updating status: %w, when creation of prefix object failed: %w", setConditionErr, err) + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixAssignedFalse, corev1.EventTypeWarning, err); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{}, err } @@ -323,14 +323,14 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) prefixClaim.Status.Prefix = prefix.Spec.Prefix prefixClaim.Status.PrefixName = prefix.Name - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyTrue, corev1.EventTypeNormal, nil); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyTrue, corev1.EventTypeNormal, nil); errReport != nil { + return ctrl.Result{}, errReport } } else { debugLogger.Info("prefix status ready false") - if err := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyFalse, corev1.EventTypeWarning, nil); err != nil { - return ctrl.Result{}, err + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionPrefixClaimReadyFalse, corev1.EventTypeWarning, nil); errReport != nil { + return ctrl.Result{}, errReport } return ctrl.Result{Requeue: true}, nil } From 4451d1a468f5289acd23cbd2d7008d4c2f0750d2 Mon Sep 17 00:00:00 2001 From: Alexander North Date: Thu, 17 Apr 2025 14:47:11 +0200 Subject: [PATCH 8/8] change format string to wrap error --- internal/controller/prefixclaim_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/prefixclaim_controller.go b/internal/controller/prefixclaim_controller.go index c4bcc933..f2b9589b 100644 --- a/internal/controller/prefixclaim_controller.go +++ b/internal/controller/prefixclaim_controller.go @@ -156,7 +156,7 @@ func (r *PrefixClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) // fetch available prefixes from netbox parentPrefixCandidates, err := r.NetboxClient.GetAvailablePrefixByParentPrefixSelector(&prefixClaim.Spec) if err != nil || len(parentPrefixCandidates) == 0 { - if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %v, number of candidates = %v", err, len(parentPrefixCandidates))); errReport != nil { + if errReport := r.EventStatusRecorder.Report(ctx, prefixClaim, netboxv1.ConditionParentPrefixSelectedFalse, corev1.EventTypeWarning, fmt.Errorf("no parent prefix can be obtained with the query conditions set in ParentPrefixSelector, err = %w, number of candidates = %v", err, len(parentPrefixCandidates))); errReport != nil { return ctrl.Result{}, errReport }