From 7dbd3f88f63935dd676a2bbfbbea1dc2c09bf98f Mon Sep 17 00:00:00 2001 From: Kevin Pollet Date: Mon, 29 Jul 2024 15:48:05 +0200 Subject: [PATCH] Do not update route status when nothing changed --- pkg/provider/kubernetes/gateway/client.go | 136 +++++++++++------- .../kubernetes/gateway/client_test.go | 2 +- pkg/provider/kubernetes/gateway/kubernetes.go | 32 +++-- pkg/provider/kubernetes/gateway/tcproute.go | 5 +- pkg/provider/kubernetes/gateway/tlsroute.go | 4 +- 5 files changed, 116 insertions(+), 63 deletions(-) diff --git a/pkg/provider/kubernetes/gateway/client.go b/pkg/provider/kubernetes/gateway/client.go index ce5619b72..fb1b4440c 100644 --- a/pkg/provider/kubernetes/gateway/client.go +++ b/pkg/provider/kubernetes/gateway/client.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "os" + "reflect" "slices" "time" @@ -53,7 +54,7 @@ func (reh *resourceEventHandler) OnDelete(obj interface{}) { type Client interface { WatchAll(namespaces []string, stopCh <-chan struct{}) (<-chan interface{}, error) UpdateGatewayStatus(ctx context.Context, gateway ktypes.NamespacedName, status gatev1.GatewayStatus) error - UpdateGatewayClassStatus(ctx context.Context, name string, condition metav1.Condition) error + UpdateGatewayClassStatus(ctx context.Context, name string, status gatev1.GatewayClassStatus) error UpdateHTTPRouteStatus(ctx context.Context, route ktypes.NamespacedName, status gatev1.HTTPRouteStatus) error UpdateTCPRouteStatus(ctx context.Context, route ktypes.NamespacedName, status gatev1alpha2.TCPRouteStatus) error UpdateTLSRouteStatus(ctx context.Context, route ktypes.NamespacedName, status gatev1alpha2.TLSRouteStatus) error @@ -378,7 +379,7 @@ func (c *clientWrapper) ListGatewayClasses() ([]*gatev1.GatewayClass, error) { return c.factoryGatewayClass.Gateway().V1().GatewayClasses().Lister().List(labels.Everything()) } -func (c *clientWrapper) UpdateGatewayClassStatus(ctx context.Context, name string, condition metav1.Condition) error { +func (c *clientWrapper) UpdateGatewayClassStatus(ctx context.Context, name string, status gatev1.GatewayClassStatus) error { err := retry.RetryOnConflict(retry.DefaultRetry, func() error { currentGatewayClass, err := c.factoryGatewayClass.Gateway().V1().GatewayClasses().Lister().Get(name) if err != nil { @@ -387,23 +388,12 @@ func (c *clientWrapper) UpdateGatewayClassStatus(ctx context.Context, name strin return err } - currentGatewayClass = currentGatewayClass.DeepCopy() - var newConditions []metav1.Condition - for _, cond := range currentGatewayClass.Status.Conditions { - // No update for identical condition. - if cond.Type == condition.Type && cond.Status == condition.Status && cond.ObservedGeneration == condition.ObservedGeneration { - return nil - } - - // Keep other condition types. - if cond.Type != condition.Type { - newConditions = append(newConditions, cond) - } + if conditionsEqual(currentGatewayClass.Status.Conditions, status.Conditions) { + return nil } - // Append the condition to update. - newConditions = append(newConditions, condition) - currentGatewayClass.Status.Conditions = newConditions + currentGatewayClass = currentGatewayClass.DeepCopy() + currentGatewayClass.Status = status if _, err = c.csGateway.GatewayV1().GatewayClasses().UpdateStatus(ctx, currentGatewayClass, metav1.UpdateOptions{}); err != nil { // We have to return err itself here (not wrapped inside another error) @@ -433,7 +423,7 @@ func (c *clientWrapper) UpdateGatewayStatus(ctx context.Context, gateway ktypes. return err } - if gatewayStatusEquals(currentGateway.Status, status) { + if gatewayStatusEqual(currentGateway.Status, status) { return nil } @@ -468,16 +458,22 @@ func (c *clientWrapper) UpdateHTTPRouteStatus(ctx context.Context, route ktypes. return err } - // TODO: keep statuses for gateways managed by other Traefik instances. - var parentStatuses []gatev1.RouteParentStatus - for _, currentParentStatus := range currentRoute.Status.Parents { - if currentParentStatus.ControllerName != controllerName { - parentStatuses = append(parentStatuses, currentParentStatus) + parentStatuses := make([]gatev1.RouteParentStatus, len(status.Parents)) + copy(parentStatuses, status.Parents) + + // keep statuses added by other gateway controllers. + // TODO: we should also keep statuses for gateways managed by other Traefik instances. + for _, parentStatus := range currentRoute.Status.Parents { + if parentStatus.ControllerName != controllerName { + parentStatuses = append(parentStatuses, parentStatus) continue } } - parentStatuses = append(parentStatuses, status.Parents...) + // do not update status when nothing has changed. + if routeParentStatusesEqual(currentRoute.Status.Parents, parentStatuses) { + return nil + } currentRoute = currentRoute.DeepCopy() currentRoute.Status = gatev1.HTTPRouteStatus{ @@ -514,16 +510,22 @@ func (c *clientWrapper) UpdateTCPRouteStatus(ctx context.Context, route ktypes.N return err } - // TODO: keep statuses for gateways managed by other Traefik instances. - var parentStatuses []gatev1alpha2.RouteParentStatus - for _, currentParentStatus := range currentRoute.Status.Parents { - if currentParentStatus.ControllerName != controllerName { - parentStatuses = append(parentStatuses, currentParentStatus) + parentStatuses := make([]gatev1.RouteParentStatus, len(status.Parents)) + copy(parentStatuses, status.Parents) + + // keep statuses added by other gateway controllers. + // TODO: we should also keep statuses for gateways managed by other Traefik instances. + for _, parentStatus := range currentRoute.Status.Parents { + if parentStatus.ControllerName != controllerName { + parentStatuses = append(parentStatuses, parentStatus) continue } } - parentStatuses = append(parentStatuses, status.Parents...) + // do not update status when nothing has changed. + if routeParentStatusesEqual(currentRoute.Status.Parents, parentStatuses) { + return nil + } currentRoute = currentRoute.DeepCopy() currentRoute.Status = gatev1alpha2.TCPRouteStatus{ @@ -560,16 +562,22 @@ func (c *clientWrapper) UpdateTLSRouteStatus(ctx context.Context, route ktypes.N return err } - // TODO: keep statuses for gateways managed by other Traefik instances. - var parentStatuses []gatev1alpha2.RouteParentStatus - for _, currentParentStatus := range currentRoute.Status.Parents { - if currentParentStatus.ControllerName != controllerName { - parentStatuses = append(parentStatuses, currentParentStatus) + parentStatuses := make([]gatev1.RouteParentStatus, len(status.Parents)) + copy(parentStatuses, status.Parents) + + // keep statuses added by other gateway controllers. + // TODO: we should also keep statuses for gateways managed by other Traefik instances. + for _, parentStatus := range currentRoute.Status.Parents { + if parentStatus.ControllerName != controllerName { + parentStatuses = append(parentStatuses, parentStatus) continue } } - parentStatuses = append(parentStatuses, status.Parents...) + // do not update status when nothing has changed. + if routeParentStatusesEqual(currentRoute.Status.Parents, parentStatuses) { + return nil + } currentRoute = currentRoute.DeepCopy() currentRoute.Status = gatev1alpha2.TLSRouteStatus{ @@ -675,12 +683,12 @@ func translateNotFoundError(err error) (bool, error) { return err == nil, err } -func gatewayStatusEquals(statusA, statusB gatev1.GatewayStatus) bool { +func gatewayStatusEqual(statusA, statusB gatev1.GatewayStatus) bool { if len(statusA.Listeners) != len(statusB.Listeners) { return false } - if !conditionsEquals(statusA.Conditions, statusB.Conditions) { + if !conditionsEqual(statusA.Conditions, statusB.Conditions) { return false } @@ -688,7 +696,7 @@ func gatewayStatusEquals(statusA, statusB gatev1.GatewayStatus) bool { for _, newListener := range statusB.Listeners { for _, oldListener := range statusA.Listeners { if newListener.Name == oldListener.Name { - if !conditionsEquals(newListener.Conditions, oldListener.Conditions) { + if !conditionsEqual(newListener.Conditions, oldListener.Conditions) { return false } @@ -704,22 +712,48 @@ func gatewayStatusEquals(statusA, statusB gatev1.GatewayStatus) bool { return listenerMatches == len(statusA.Listeners) } -func conditionsEquals(conditionsA, conditionsB []metav1.Condition) bool { - if len(conditionsA) != len(conditionsB) { +func routeParentStatusesEqual(routeParentStatusesA, routeParentStatusesB []gatev1alpha2.RouteParentStatus) bool { + if len(routeParentStatusesA) != len(routeParentStatusesB) { return false } - conditionMatches := 0 - for _, conditionA := range conditionsA { - for _, conditionB := range conditionsB { - if conditionA.Type == conditionB.Type { - if conditionA.Reason != conditionB.Reason || conditionA.Status != conditionB.Status || conditionA.Message != conditionB.Message || conditionA.ObservedGeneration != conditionB.ObservedGeneration { - return false - } - conditionMatches++ - } + for _, sA := range routeParentStatusesA { + if !slices.ContainsFunc(routeParentStatusesB, func(sB gatev1alpha2.RouteParentStatus) bool { + return routeParentStatusEqual(sB, sA) + }) { + return false } } - return conditionMatches == len(conditionsA) + for _, sB := range routeParentStatusesB { + if !slices.ContainsFunc(routeParentStatusesA, func(sA gatev1alpha2.RouteParentStatus) bool { + return routeParentStatusEqual(sA, sB) + }) { + return false + } + } + + return true +} + +func routeParentStatusEqual(sA, sB gatev1alpha2.RouteParentStatus) bool { + if !reflect.DeepEqual(sA.ParentRef, sB.ParentRef) { + return false + } + + if sA.ControllerName != sB.ControllerName { + return false + } + + return conditionsEqual(sA.Conditions, sB.Conditions) +} + +func conditionsEqual(conditionsA, conditionsB []metav1.Condition) bool { + return slices.EqualFunc(conditionsA, conditionsB, func(cA metav1.Condition, cB metav1.Condition) bool { + return cA.Type == cB.Type && + cA.Reason == cB.Reason && + cA.Status == cB.Status && + cA.Message == cB.Message && + cA.ObservedGeneration == cB.ObservedGeneration + }) } diff --git a/pkg/provider/kubernetes/gateway/client_test.go b/pkg/provider/kubernetes/gateway/client_test.go index 6b371b6fc..de013fbc0 100644 --- a/pkg/provider/kubernetes/gateway/client_test.go +++ b/pkg/provider/kubernetes/gateway/client_test.go @@ -268,7 +268,7 @@ func Test_gatewayStatusEquals(t *testing.T) { t.Run(test.desc, func(t *testing.T) { t.Parallel() - result := gatewayStatusEquals(test.statusA, test.statusB) + result := gatewayStatusEqual(test.statusA, test.statusB) assert.Equal(t, test.expected, result) }) diff --git a/pkg/provider/kubernetes/gateway/kubernetes.go b/pkg/provider/kubernetes/gateway/kubernetes.go index ce1e6d579..4203f3bde 100644 --- a/pkg/provider/kubernetes/gateway/kubernetes.go +++ b/pkg/provider/kubernetes/gateway/kubernetes.go @@ -318,15 +318,18 @@ func (p *Provider) loadConfigurationFromGateways(ctx context.Context) *dynamic.C gatewayClassNames[gatewayClass.Name] = struct{}{} - err := p.client.UpdateGatewayClassStatus(ctx, gatewayClass.Name, metav1.Condition{ - Type: string(gatev1.GatewayClassConditionStatusAccepted), - Status: metav1.ConditionTrue, - ObservedGeneration: gatewayClass.Generation, - Reason: "Handled", - Message: "Handled by Traefik controller", - LastTransitionTime: metav1.Now(), - }) - if err != nil { + status := gatev1.GatewayClassStatus{ + Conditions: upsertGatewayClassConditionAccepted(gatewayClass.Status.Conditions, metav1.Condition{ + Type: string(gatev1.GatewayClassConditionStatusAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: gatewayClass.Generation, + Reason: "Handled", + Message: "Handled by Traefik controller", + LastTransitionTime: metav1.Now(), + }), + } + + if err := p.client.UpdateGatewayClassStatus(ctx, gatewayClass.Name, status); err != nil { log.Ctx(ctx). Warn(). Err(err). @@ -1228,3 +1231,14 @@ func upsertRouteConditionResolvedRefs(conditions []metav1.Condition, condition m } return append(conds, condition) } + +func upsertGatewayClassConditionAccepted(conditions []metav1.Condition, condition metav1.Condition) []metav1.Condition { + var conds []metav1.Condition + for _, c := range conditions { + if c.Type == string(gatev1.GatewayClassConditionStatusAccepted) { + continue + } + conds = append(conds, c) + } + return append(conds, condition) +} diff --git a/pkg/provider/kubernetes/gateway/tcproute.go b/pkg/provider/kubernetes/gateway/tcproute.go index 89189ded4..23e7bcd80 100644 --- a/pkg/provider/kubernetes/gateway/tcproute.go +++ b/pkg/provider/kubernetes/gateway/tcproute.go @@ -27,7 +27,10 @@ func (p *Provider) loadTCPRoutes(ctx context.Context, gatewayListeners []gateway } for _, route := range routes { - logger := log.Ctx(ctx).With().Str("tcproute", route.Name).Str("namespace", route.Namespace).Logger() + logger := log.Ctx(ctx).With(). + Str("tcp_route", route.Name). + Str("namespace", route.Namespace). + Logger() var parentStatuses []gatev1alpha2.RouteParentStatus for _, parentRef := range route.Spec.ParentRefs { diff --git a/pkg/provider/kubernetes/gateway/tlsroute.go b/pkg/provider/kubernetes/gateway/tlsroute.go index 087b09e83..b8a853a44 100644 --- a/pkg/provider/kubernetes/gateway/tlsroute.go +++ b/pkg/provider/kubernetes/gateway/tlsroute.go @@ -24,7 +24,9 @@ func (p *Provider) loadTLSRoutes(ctx context.Context, gatewayListeners []gateway } for _, route := range routes { - logger := log.Ctx(ctx).With().Str("tlsroute", route.Name).Str("namespace", route.Namespace).Logger() + logger := log.Ctx(ctx).With(). + Str("tls_route", route.Name). + Str("namespace", route.Namespace).Logger() var parentStatuses []gatev1alpha2.RouteParentStatus for _, parentRef := range route.Spec.ParentRefs {