Skip to content

Commit

Permalink
refactor: refactor Gateway API's route parent status update code (#6877)
Browse files Browse the repository at this point in the history
  • Loading branch information
pmalek authored Dec 25, 2024
1 parent 7d4787b commit 3dce2d3
Show file tree
Hide file tree
Showing 14 changed files with 1,121 additions and 570 deletions.
148 changes: 148 additions & 0 deletions internal/controllers/gateway/conditions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
package gateway

import (
"context"

"github.com/samber/lo"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
gatewayv1 "sigs.k8s.io/gateway-api/apis/v1"

"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
)

func newCondition(
typ string,
status metav1.ConditionStatus,
reason string,
generation int64,
) metav1.Condition {
return metav1.Condition{
Type: typ,
Status: status,
Reason: reason,
ObservedGeneration: generation,
LastTransitionTime: metav1.Now(),
}
}

func newProgrammedConditionUnknown(obj interface{ GetGeneration() int64 }) metav1.Condition {
return newCondition(
ConditionTypeProgrammed,
metav1.ConditionUnknown,
string(ConditionReasonProgrammedUnknown),
obj.GetGeneration(),
)
}

// sameCondition returns true if the conditions in parameter has the same type, status, reason and message.
func sameCondition(a, b metav1.Condition) bool {
return a.Type == b.Type &&
a.Status == b.Status &&
a.Reason == b.Reason &&
a.Message == b.Message &&
a.ObservedGeneration == b.ObservedGeneration
}

func setRouteParentStatusCondition(parentStatus *gatewayapi.RouteParentStatus, newCondition metav1.Condition) bool {
var conditionFound, changed bool
for i, condition := range parentStatus.Conditions {
if condition.Type == newCondition.Type {
conditionFound = true
if !sameCondition(condition, newCondition) {
parentStatus.Conditions[i] = newCondition
changed = true
}
}
}

if !conditionFound {
parentStatus.Conditions = append(parentStatus.Conditions, newCondition)
changed = true
}
return changed
}

func parentStatusHasProgrammedCondition(parentStatus *gatewayapi.RouteParentStatus) bool {
return lo.ContainsBy(parentStatus.Conditions, func(c metav1.Condition) bool {
return c.Type == ConditionTypeProgrammed
})
}

// ensureParentsProgrammedCondition ensures that provided route's parent statuses
// have Programmed condition set properly. It returns a boolean flag indicating
// whether an update to the provided route has been performed.
//
// Use the condition argument to specify the Reason, Status and Message.
// Type will be set to Programmed whereas ObservedGeneration and LastTransitionTime
// will be set accordingly based on the route's generation and current time.
func ensureParentsProgrammedCondition[
routeT gatewayapi.RouteT,
](
ctx context.Context,
client client.SubResourceWriter,
route routeT,
routeParentStatuses []gatewayapi.RouteParentStatus,
gateways []supportedGatewayWithCondition,
condition metav1.Condition,
) (bool, error) {
// map the existing parentStatues to avoid duplications
parentStatuses := getParentStatuses(route, routeParentStatuses)

condition.Type = ConditionTypeProgrammed
condition.ObservedGeneration = route.GetGeneration()
condition.LastTransitionTime = metav1.Now()

statusChanged := false
for _, g := range gateways {
gateway := g.gateway

parentRefKey := routeParentStatusKey(route, g)
parentStatus, ok := parentStatuses[parentRefKey]
if ok {
// update existing parent in status.
changed := setRouteParentStatusCondition(parentStatus, condition)
if changed {
parentStatuses[parentRefKey] = parentStatus
setRouteParentInStatusForParent(route, *parentStatus, g)
}
statusChanged = statusChanged || changed
} else {
// add a new parent if the parent is not found in status.
newParentStatus := gatewayapi.RouteParentStatus{
ParentRef: gatewayapi.ParentReference{
Namespace: lo.ToPtr(gatewayapi.Namespace(gateway.Namespace)),
Name: gatewayapi.ObjectName(gateway.Name),
Kind: lo.ToPtr(gatewayapi.Kind("Gateway")),
Group: lo.ToPtr(gatewayapi.Group(gatewayv1.GroupName)),
// We don't need to check whether the listener matches route's spec
// because that should already be done via getSupportedGatewayForRoute
// at this point.
SectionName: lo.EmptyableToPtr(gatewayapi.SectionName(g.listenerName)),

// TODO: set port after gateway port matching implemented:
// https://github.com/Kong/kubernetes-ingress-controller/issues/3016
},
ControllerName: GetControllerName(),
Conditions: []metav1.Condition{
condition,
},
}
setRouteParentInStatusForParent(route, newParentStatus, g)

routeParentStatuses = append(routeParentStatuses, newParentStatus)
parentStatuses[parentRefKey] = &newParentStatus
statusChanged = true
}
}

// update status if needed.
if statusChanged {
if err := client.Update(ctx, route); err != nil {
return false, err
}
return true, nil
}
// no need to update if no status is changed.
return false, nil
}
75 changes: 6 additions & 69 deletions internal/controllers/gateway/grpcroute_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"time"

"github.com/go-logr/logr"
"github.com/samber/lo"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -27,7 +26,6 @@ import (

"github.com/kong/kubernetes-ingress-controller/v3/internal/controllers"
"github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi"
"github.com/kong/kubernetes-ingress-controller/v3/internal/util"
k8sobj "github.com/kong/kubernetes-ingress-controller/v3/internal/util/kubernetes/object"
"github.com/kong/kubernetes-ingress-controller/v3/internal/util/kubernetes/object/status"
)
Expand Down Expand Up @@ -424,78 +422,17 @@ func (r *GRPCRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// GRPCRouteReconciler - Status Helpers
// -----------------------------------------------------------------------------

// grpcrouteParentKind indicates the only object KIND that this GRPCRoute
// implementation supports for route object parent references.
var grpcrouteParentKind = "Gateway"

// ensureGatewayReferenceStatus takes any number of Gateways that should be
// considered "attached" to a given GRPCRoute and ensures that the status
// for the GRPCRoute is updated appropriately.
func (r *GRPCRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Context, grpcroute *gatewayapi.GRPCRoute, gateways ...supportedGatewayWithCondition) (bool, error) {
// map the existing parentStatues to avoid duplications
parentStatuses := getParentStatuses(grpcroute, grpcroute.Status.Parents)

// overlay the parent ref statuses for all new gateway references
statusChangesWereMade := false
for _, gateway := range gateways {
// build a new status for the parent Gateway
gatewayParentStatus := &gatewayapi.RouteParentStatus{
ParentRef: gatewayapi.ParentReference{
Group: (*gatewayapi.Group)(&gatewayv1.GroupVersion.Group),
Kind: util.StringToGatewayAPIKindPtr(grpcrouteParentKind),
Namespace: (*gatewayapi.Namespace)(&gateway.gateway.Namespace),
Name: gatewayapi.ObjectName(gateway.gateway.Name),
},
ControllerName: GetControllerName(),
Conditions: []metav1.Condition{{
Type: string(gatewayapi.RouteConditionAccepted),
Status: metav1.ConditionTrue,
ObservedGeneration: grpcroute.Generation,
LastTransitionTime: metav1.Now(),
Reason: string(gatewayapi.RouteReasonAccepted),
}},
}

if gateway.listenerName != "" {
sectionName := gatewayapi.SectionName(gateway.listenerName)
gatewayParentStatus.ParentRef.SectionName = &sectionName
}

// if the reference already exists and doesn't require any changes
// then just leave it alone.
parentRefKey := fmt.Sprintf("%s/%s/%s", gateway.gateway.Namespace, gateway.gateway.Name, gateway.listenerName)
if existingGatewayParentStatus, exists := parentStatuses[parentRefKey]; exists {
// check if the parentRef and controllerName are equal, and whether the new condition is present in existing conditions
if reflect.DeepEqual(existingGatewayParentStatus.ParentRef, gatewayParentStatus.ParentRef) &&
existingGatewayParentStatus.ControllerName == gatewayParentStatus.ControllerName &&
lo.ContainsBy(existingGatewayParentStatus.Conditions, func(condition metav1.Condition) bool {
return sameCondition(gatewayParentStatus.Conditions[0], condition)
}) {
continue
}
}

// otherwise overlay the new status on top the list of parentStatuses
parentStatuses[parentRefKey] = gatewayParentStatus
statusChangesWereMade = true
}
parentStatuses, statusChangesWereMade := parentStatusesForRoute(
grpcroute,
grpcroute.Status.Parents,
gateways...,
)

// initialize "programmed" condition to Unknown.
// do not update the condition If a "Programmed" condition is already present.
programmedConditionChanged := false
programmedConditionUnknown := metav1.Condition{
Type: ConditionTypeProgrammed,
Status: metav1.ConditionUnknown,
Reason: string(ConditionReasonProgrammedUnknown),
ObservedGeneration: grpcroute.Generation,
LastTransitionTime: metav1.Now(),
}
for _, parentStatus := range parentStatuses {
if !parentStatusHasProgrammedCondition(parentStatus) {
programmedConditionChanged = true
parentStatus.Conditions = append(parentStatus.Conditions, programmedConditionUnknown)
}
}
programmedConditionChanged := initializeParentStatusesWithProgrammedCondition(grpcroute, parentStatuses)

// if we didn't have to actually make any changes, no status update is needed
if !statusChangesWereMade && !programmedConditionChanged {
Expand Down
73 changes: 6 additions & 67 deletions internal/controllers/gateway/httproute_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"time"

"github.com/go-logr/logr"
"github.com/samber/lo"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -518,10 +517,6 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
// HTTPRouteReconciler - Status Helpers
// -----------------------------------------------------------------------------

// httprouteParentKind indicates the only object KIND that this HTTPRoute
// implementation supports for route object parent references.
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.
Expand All @@ -530,74 +525,18 @@ var httprouteParentKind = "Gateway"
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)

// overlay the parent ref statuses for all new gateway references
statusChangesWereMade := false
for _, gateway := range gateways {
// build a new status for the parent Gateway
gatewayParentStatus := &gatewayapi.RouteParentStatus{
ParentRef: gatewayapi.ParentReference{
Group: (*gatewayapi.Group)(&gatewayv1.GroupVersion.Group),
Kind: util.StringToGatewayAPIKindPtr(httprouteParentKind),
Namespace: (*gatewayapi.Namespace)(&gateway.gateway.Namespace),
Name: gatewayapi.ObjectName(gateway.gateway.Name),
},
ControllerName: GetControllerName(),
Conditions: []metav1.Condition{{
Type: gateway.condition.Type,
Status: gateway.condition.Status,
ObservedGeneration: httproute.Generation,
LastTransitionTime: metav1.Now(),
Reason: gateway.condition.Reason,
}},
}
if gateway.listenerName != "" {
gatewayParentStatus.ParentRef.SectionName = lo.ToPtr(gatewayapi.SectionName(gateway.listenerName))
}

key := fmt.Sprintf("%s/%s/%s", gateway.gateway.Namespace, gateway.gateway.Name, gateway.listenerName)

// if the reference already exists and doesn't require any changes
// then just leave it alone.
if existingGatewayParentStatus, exists := parentStatuses[key]; exists {
// check if the parentRef and controllerName are equal, and whether the new condition is present in existing conditions
if reflect.DeepEqual(existingGatewayParentStatus.ParentRef, gatewayParentStatus.ParentRef) &&
existingGatewayParentStatus.ControllerName == gatewayParentStatus.ControllerName &&
lo.ContainsBy(existingGatewayParentStatus.Conditions, func(condition metav1.Condition) bool {
return sameCondition(gatewayParentStatus.Conditions[0], condition)
}) {
continue
}
}

// otherwise overlay the new status on top the list of parentStatuses
parentStatuses[key] = gatewayParentStatus
statusChangesWereMade = true
}
parentStatuses, statusChangesWereMade := parentStatusesForRoute(
httproute,
httproute.Status.Parents,
gateways...,
)

parentStatuses, resolvedRefsChanged, err := r.setRouteConditionResolvedRefsCondition(ctx, httproute, parentStatuses)
if err != nil {
return false, ctrl.Result{}, err
}

// initialize "programmed" condition to Unknown.
// do not update the condition If a "Programmed" condition is already present.
programmedConditionChanged := false
programmedConditionUnknown := metav1.Condition{
Type: ConditionTypeProgrammed,
Status: metav1.ConditionUnknown,
Reason: string(ConditionReasonProgrammedUnknown),
ObservedGeneration: httproute.Generation,
LastTransitionTime: metav1.Now(),
}
for _, parentStatus := range parentStatuses {
if !parentStatusHasProgrammedCondition(parentStatus) {
programmedConditionChanged = true
parentStatus.Conditions = append(parentStatus.Conditions, programmedConditionUnknown)
}
}
programmedConditionChanged := initializeParentStatusesWithProgrammedCondition(httproute, parentStatuses)

// if we didn't have to actually make any changes, no status update is needed
if !statusChangesWereMade && !resolvedRefsChanged && !programmedConditionChanged {
Expand Down
Loading

1 comment on commit 3dce2d3

@github-actions
Copy link

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Go Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 3dce2d3 Previous: 7d4787b Ratio
BenchmarkSanitizedCopy 1275701 ns/op 819447 B/op 5 allocs/op 463956 ns/op 819442 B/op 5 allocs/op 2.75
BenchmarkSanitizedCopy - ns/op 1275701 ns/op 463956 ns/op 2.75

This comment was automatically generated by workflow using github-action-benchmark.

CC: @Kong/k8s-maintainers

Please sign in to comment.