diff --git a/internal/controllers/gateway/gateway_controller.go b/internal/controllers/gateway/gateway_controller.go index 89c3981e5f..f1215c6066 100644 --- a/internal/controllers/gateway/gateway_controller.go +++ b/internal/controllers/gateway/gateway_controller.go @@ -511,7 +511,9 @@ func (r *GatewayReconciler) reconcileUnmanagedGateway(ctx context.Context, log l gateway.Annotations = map[string]string{} } annotations.UpdateGatewayPublishService(gateway.Annotations, services) - return ctrl.Result{}, r.Update(ctx, gateway) + + err := r.Update(ctx, gateway) + return handleUpdateError(err, log, gateway) } serviceRefs := annotations.ExtractGatewayPublishService(gateway.Annotations) @@ -559,7 +561,9 @@ func (r *GatewayReconciler) reconcileUnmanagedGateway(ctx context.Context, log l Reason: string(gatewayapi.GatewayReasonPending), } setGatewayCondition(gateway, programmedCondition) - return ctrl.Result{}, r.Status().Update(ctx, pruneGatewayStatusConds(gateway)) + + err := r.Status().Update(ctx, pruneGatewayStatusConds(gateway)) + return handleUpdateError(err, log, gateway) } // When deployed on Kubernetes Kong can not be relied on for the address data needed for Gateway because @@ -622,17 +626,9 @@ func (r *GatewayReconciler) reconcileUnmanagedGateway(ctx context.Context, log l // Gateway status reflects the spec. As the status is simply a mirror of the Service, this is // a given and we can simply update spec to status. debug(log, gateway, "Updating the gateway status if necessary") - isChanged, err := r.updateAddressesAndListenersStatus(ctx, gateway, listenerStatuses, combinedAddresses) - if err != nil { - if apierrors.IsConflict(err) { - // if there's a conflict that's normal just requeue to retry, no need to make noise. - return ctrl.Result{Requeue: true}, nil - } - return ctrl.Result{}, err - } - if isChanged { - debug(log, gateway, "Gateway status updated") - return ctrl.Result{}, nil + res, err := r.updateAddressesAndListenersStatus(ctx, log, gateway, listenerStatuses, combinedAddresses) + if err != nil || !res.IsZero() { + return res, err } info(log, gateway, "Gateway provisioning complete") @@ -860,10 +856,11 @@ func (r *GatewayReconciler) determineListenersFromDataPlane( // If the addresses and listeners provided are the same as what exists, it is assumed that reconciliation is complete and a Programmed condition is posted. func (r *GatewayReconciler) updateAddressesAndListenersStatus( ctx context.Context, + log logr.Logger, gateway *gatewayapi.Gateway, listenerStatuses []gatewayapi.ListenerStatus, addresses []gatewayapi.GatewayStatusAddress, -) (bool, error) { +) (ctrl.Result, error) { if !isGatewayProgrammed(gateway) { gateway.Status.Listeners = listenerStatuses gateway.Status.Addresses = addresses @@ -875,11 +872,17 @@ func (r *GatewayReconciler) updateAddressesAndListenersStatus( Reason: string(gatewayapi.GatewayReasonProgrammed), } setGatewayCondition(gateway, programmedCondition) - return true, r.Status().Update(ctx, pruneGatewayStatusConds(gateway)) + + err := r.Status().Update(ctx, pruneGatewayStatusConds(gateway)) + return handleUpdateError(err, r.Log, gateway) } if !reflect.DeepEqual(gateway.Status.Listeners, listenerStatuses) { gateway.Status.Listeners = listenerStatuses - return true, r.Status().Update(ctx, gateway) + + err := r.Status().Update(ctx, gateway) + return handleUpdateError(err, r.Log, gateway) } - return false, nil + + info(log, gateway, "Gateway status updated") + return ctrl.Result{}, nil } diff --git a/internal/controllers/gateway/handle_error.go b/internal/controllers/gateway/handle_error.go new file mode 100644 index 0000000000..ebcaa21b6a --- /dev/null +++ b/internal/controllers/gateway/handle_error.go @@ -0,0 +1,20 @@ +package gateway + +import ( + "github.com/go-logr/logr" + apierrors "k8s.io/apimachinery/pkg/api/errors" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// handleUpdateError is a helper function to handle errors that occur when updating +// an object or its status. +// If the error is a conflict, it will return a requeue result. +// Otherwise, it will return the provided error. +func handleUpdateError(err error, log logr.Logger, obj client.Object) (ctrl.Result, error) { + if apierrors.IsConflict(err) { + debug(log, obj, "Conflict found when updating, retrying") + return ctrl.Result{Requeue: true}, nil + } + return ctrl.Result{}, err +} diff --git a/internal/controllers/gateway/httproute_controller.go b/internal/controllers/gateway/httproute_controller.go index bf86e6050f..d739b446f9 100644 --- a/internal/controllers/gateway/httproute_controller.go +++ b/internal/controllers/gateway/httproute_controller.go @@ -402,12 +402,14 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // Ensure we have no status for no-longer defined parentRefs. if wasAnyStatusRemoved := ensureNoStaleParentStatus(httproute); wasAnyStatusRemoved { - if err := r.Status().Update(ctx, httproute); err != nil { + err := r.Status().Update(ctx, httproute) + res, err := handleUpdateError(err, log, httproute) + if err != nil { return ctrl.Result{}, fmt.Errorf("failed to prune stale Gateway parent statuses from %s status: %w", client.ObjectKeyFromObject(httproute), err, ) } - return ctrl.Result{}, nil + return res, nil } // the referenced gateway object(s) for the HTTPRoute needs to be ready @@ -445,14 +447,16 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // we can update the object status to indicate that it's now properly linked // to the configured Gateways. debug(log, httproute, "Ensuring status contains Gateway associations") - statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, httproute, gateways...) + updated, res, err := r.ensureGatewayReferenceStatusAdded(ctx, httproute, gateways...) if err != nil { // don't proceed until the statuses can be updated appropriately return ctrl.Result{}, err } - if statusUpdated { + if !res.IsZero() { + return res, nil + } + if updated { // if the status was updated it will trigger a follow-up reconciliation - // so we don't need to do anything further here. return ctrl.Result{}, nil } @@ -521,7 +525,11 @@ var httprouteParentKind = "Gateway" // ensureGatewayReferenceStatus takes any number of Gateways that should be // considered "attached" to a given HTTPRoute and ensures that the status // for the HTTPRoute is updated appropriately. -func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Context, httproute *gatewayapi.HTTPRoute, gateways ...supportedGatewayWithCondition) (bool, error) { +// It returns true if controller should requeue the object. Either because +// the status update resulted in a conflict or because the status was updated. +func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded( + ctx context.Context, httproute *gatewayapi.HTTPRoute, gateways ...supportedGatewayWithCondition, +) (bool, ctrl.Result, error) { // map the existing parentStatues to avoid duplications parentStatuses := getParentStatuses(httproute, httproute.Status.Parents) @@ -571,7 +579,7 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont parentStatuses, resolvedRefsChanged, err := r.setRouteConditionResolvedRefsCondition(ctx, httproute, parentStatuses) if err != nil { - return false, err + return false, ctrl.Result{}, err } // initialize "programmed" condition to Unknown. @@ -593,7 +601,7 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont // if we didn't have to actually make any changes, no status update is needed if !statusChangesWereMade && !resolvedRefsChanged && !programmedConditionChanged { - return false, nil + return false, ctrl.Result{}, nil } // update the httproute status with the new status references @@ -603,12 +611,16 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont } // update the object status in the API - if err := r.Status().Update(ctx, httproute); err != nil { - return false, err + res, err := handleUpdateError(r.Status().Update(ctx, httproute), r.Log, httproute) + if err != nil { + return false, ctrl.Result{}, err + } + if !res.IsZero() { + return false, res, nil } // the status needed an update and it was updated successfully - return true, nil + return true, ctrl.Result{}, nil } // setRouteConditionResolvedRefsCondition sets a condition of type ResolvedRefs on the route status. diff --git a/internal/controllers/gateway/tcproute_controller.go b/internal/controllers/gateway/tcproute_controller.go index a030a7e9ce..f498ad1ec1 100644 --- a/internal/controllers/gateway/tcproute_controller.go +++ b/internal/controllers/gateway/tcproute_controller.go @@ -361,14 +361,16 @@ func (r *TCPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // we can update the object status to indicate that it's now properly linked // to the configured Gateways. debug(log, tcproute, "Ensuring status contains Gateway associations") - statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, tcproute, gateways...) + updated, res, err := r.ensureGatewayReferenceStatusAdded(ctx, tcproute, gateways...) if err != nil { // don't proceed until the statuses can be updated appropriately return ctrl.Result{}, err } - if statusUpdated { + if !res.IsZero() { + return res, nil + } + if updated { // if the status was updated it will trigger a follow-up reconciliation - // so we don't need to do anything further here. return ctrl.Result{}, nil } @@ -436,11 +438,13 @@ var tcprouteParentKind = "Gateway" // ensureGatewayReferenceStatus takes any number of Gateways that should be // considered "attached" to a given TCPRoute and ensures that the status // for the TCPRoute is updated appropriately. +// It returns true if controller should requeue the object. Either because +// the status update resulted in a conflict or because the status was updated. func (r *TCPRouteReconciler) ensureGatewayReferenceStatusAdded( ctx context.Context, tcproute *gatewayapi.TCPRoute, gateways ...supportedGatewayWithCondition, -) (bool, error) { +) (bool, ctrl.Result, error) { // map the existing parentStatues to avoid duplications parentStatuses := getParentStatuses(tcproute, tcproute.Status.Parents) @@ -503,7 +507,7 @@ func (r *TCPRouteReconciler) ensureGatewayReferenceStatusAdded( // if we didn't have to actually make any changes, no status update is needed if !statusChangesWereMade && !programmedConditionChanged { - return false, nil + return false, ctrl.Result{}, nil } // update the tcproute status with the new status references @@ -513,10 +517,14 @@ func (r *TCPRouteReconciler) ensureGatewayReferenceStatusAdded( } // update the object status in the API - if err := r.Status().Update(ctx, tcproute); err != nil { - return false, err + res, err := handleUpdateError(r.Status().Update(ctx, tcproute), r.Log, tcproute) + if err != nil { + return false, ctrl.Result{}, err + } + if !res.IsZero() { + return false, res, nil } // the status needed an update and it was updated successfully - return true, nil + return true, ctrl.Result{}, nil } diff --git a/internal/controllers/gateway/tlsroute_controller.go b/internal/controllers/gateway/tlsroute_controller.go index cdfee64182..51bfbd10f0 100644 --- a/internal/controllers/gateway/tlsroute_controller.go +++ b/internal/controllers/gateway/tlsroute_controller.go @@ -347,14 +347,16 @@ func (r *TLSRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // we can update the object status to indicate that it's now properly linked // to the configured Gateways. debug(log, tlsroute, "Ensuring status contains Gateway associations") - statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, tlsroute, gateways...) + updated, res, err := r.ensureGatewayReferenceStatusAdded(ctx, tlsroute, gateways...) if err != nil { // don't proceed until the statuses can be updated appropriately return ctrl.Result{}, err } - if statusUpdated { + if !res.IsZero() { + return res, nil + } + if updated { // if the status was updated it will trigger a follow-up reconciliation - // so we don't need to do anything further here. return ctrl.Result{}, nil } @@ -421,7 +423,11 @@ var tlsrouteParentKind = "Gateway" // ensureGatewayReferenceStatus takes any number of Gateways that should be // considered "attached" to a given TLSRoute and ensures that the status // for the TLSRoute is updated appropriately. -func (r *TLSRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Context, tlsroute *gatewayapi.TLSRoute, gateways ...supportedGatewayWithCondition) (bool, error) { +// It returns true if controller should requeue the object. Either because +// the status update resulted in a conflict or because the status was updated. +func (r *TLSRouteReconciler) ensureGatewayReferenceStatusAdded( + ctx context.Context, tlsroute *gatewayapi.TLSRoute, gateways ...supportedGatewayWithCondition, +) (bool, ctrl.Result, error) { // map the existing parentStatues to avoid duplications parentStatuses := getParentStatuses(tlsroute, tlsroute.Status.Parents) @@ -489,7 +495,7 @@ func (r *TLSRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte // if we didn't have to actually make any changes, no status update is needed if !statusChangesWereMade && !programmedConditionChanged { - return false, nil + return false, ctrl.Result{}, nil } // update the tlsroute status with the new status references @@ -499,10 +505,14 @@ func (r *TLSRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte } // update the object status in the API - if err := r.Status().Update(ctx, tlsroute); err != nil { - return false, err + res, err := handleUpdateError(r.Status().Update(ctx, tlsroute), r.Log, tlsroute) + if err != nil { + return false, ctrl.Result{}, err + } + if !res.IsZero() { + return false, res, nil } // the status needed an update and it was updated successfully - return true, nil + return true, ctrl.Result{}, nil } diff --git a/internal/controllers/gateway/udproute_controller.go b/internal/controllers/gateway/udproute_controller.go index 83fa3ea8c0..d2ea89cd0f 100644 --- a/internal/controllers/gateway/udproute_controller.go +++ b/internal/controllers/gateway/udproute_controller.go @@ -361,14 +361,16 @@ func (r *UDPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c // we can update the object status to indicate that it's now properly linked // to the configured Gateways. debug(log, udproute, "Ensuring status contains Gateway associations") - statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, udproute, gateways...) + updated, res, err := r.ensureGatewayReferenceStatusAdded(ctx, udproute, gateways...) if err != nil { // don't proceed until the statuses can be updated appropriately return ctrl.Result{}, err } - if statusUpdated { + if !res.IsZero() { + return res, nil + } + if updated { // if the status was updated it will trigger a follow-up reconciliation - // so we don't need to do anything further here. return ctrl.Result{}, nil } @@ -431,7 +433,11 @@ var udprouteParentKind = "Gateway" // ensureGatewayReferenceStatus takes any number of Gateways that should be // considered "attached" to a given UDPRoute and ensures that the status // for the UDPRoute is updated appropriately. -func (r *UDPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Context, udproute *gatewayapi.UDPRoute, gateways ...supportedGatewayWithCondition) (bool, error) { +// It returns true if controller should requeue the object. Either because +// the status update resulted in a conflict or because the status was updated. +func (r *UDPRouteReconciler) ensureGatewayReferenceStatusAdded( + ctx context.Context, udproute *gatewayapi.UDPRoute, gateways ...supportedGatewayWithCondition, +) (bool, ctrl.Result, error) { // map the existing parentStatues to avoid duplications parentStatuses := getParentStatuses(udproute, udproute.Status.Parents) @@ -494,7 +500,7 @@ func (r *UDPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte // if we didn't have to actually make any changes, no status update is needed if !statusChangesWereMade && !programmedConditionChanged { - return false, nil + return false, ctrl.Result{}, nil } // update the udproute status with the new status references @@ -504,10 +510,14 @@ func (r *UDPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Conte } // update the object status in the API - if err := r.Status().Update(ctx, udproute); err != nil { - return false, err + res, err := handleUpdateError(r.Status().Update(ctx, udproute), r.Log, udproute) + if err != nil { + return false, ctrl.Result{}, err + } + if !res.IsZero() { + return false, res, nil } // the status needed an update and it was updated successfully - return true, nil + return true, ctrl.Result{}, nil }