refactor: update conflict error is not an error, don't report it but …

…requeue (#6833)

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
 }

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
+}

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.

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
 }

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
 }

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
 }