Skip to content

Commit c47cd00

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4206 from shuqz/main
[feat gw-api]bugfix and prevent backward generation route status update
2 parents 7399a96 + a02fbe2 commit c47cd00

20 files changed

+248
-39
lines changed

‎controllers/gateway/eventhandlers/grpc_route_events.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (h *enqueueRequestsForGRPCRouteEvent) Generic(ctx context.Context, e event.
5858
}
5959

6060
func (h *enqueueRequestsForGRPCRouteEvent) enqueueImpactedGateways(ctx context.Context, route *gatewayv1.GRPCRoute, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
61-
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Namespace, constants.ALBGatewayController)
61+
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Status.Parents, route.Namespace, constants.ALBGatewayController)
6262
if err != nil {
6363
h.logger.V(1).Info("ignoring unknown gateways referred by", "grpcroute", route.Name, "error", err)
6464
}

‎controllers/gateway/eventhandlers/http_route_events.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (h *enqueueRequestsForHTTPRouteEvent) Generic(ctx context.Context, e event.
5858
}
5959

6060
func (h *enqueueRequestsForHTTPRouteEvent) enqueueImpactedGateways(ctx context.Context, route *gatewayv1.HTTPRoute, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
61-
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Namespace, constants.ALBGatewayController)
61+
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Status.Parents, route.Namespace, constants.ALBGatewayController)
6262
if err != nil {
6363
h.logger.V(1).Info("ignoring unknown gateways referred by", "httproute", route.Name, "error", err)
6464
}

‎controllers/gateway/eventhandlers/tcp_route_events.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (h *enqueueRequestsForTCPRouteEvent) Generic(ctx context.Context, e event.T
5858
}
5959

6060
func (h *enqueueRequestsForTCPRouteEvent) enqueueImpactedGateways(ctx context.Context, route *gwalpha2.TCPRoute, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
61-
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Namespace, constants.NLBGatewayController)
61+
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Status.Parents, route.Namespace, constants.NLBGatewayController)
6262
if err != nil {
6363
h.logger.V(1).Info("ignoring unknown gateways referred by", "tcproute", route.Name, "error", err)
6464
}

‎controllers/gateway/eventhandlers/tls_route_events.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (h *enqueueRequestsForTLSRouteEvent) Generic(ctx context.Context, e event.T
5858
}
5959

6060
func (h *enqueueRequestsForTLSRouteEvent) enqueueImpactedGateways(ctx context.Context, route *gwalpha2.TLSRoute, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
61-
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Namespace, constants.NLBGatewayController)
61+
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Status.Parents, route.Namespace, constants.NLBGatewayController)
6262
if err != nil {
6363
h.logger.V(1).Info("ignoring unknown gateways referred by", "tlsroute", route.Name, "error", err)
6464
}

‎controllers/gateway/eventhandlers/udp_route_events.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func (h *enqueueRequestsForUDPRouteEvent) Generic(ctx context.Context, e event.T
5858
}
5959

6060
func (h *enqueueRequestsForUDPRouteEvent) enqueueImpactedGateways(ctx context.Context, route *gwalpha2.UDPRoute, queue workqueue.TypedRateLimitingInterface[reconcile.Request]) {
61-
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Namespace, constants.NLBGatewayController)
61+
gateways, err := GetImpactedGatewaysFromParentRefs(ctx, h.k8sClient, route.Spec.ParentRefs, route.Status.Parents, route.Namespace, constants.NLBGatewayController)
6262
if err != nil {
6363
h.logger.V(1).Info("ignoring unknown gateways referred by", "udproute", route.Name, "error", err)
6464
}

‎controllers/gateway/eventhandlers/utils.go

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,12 @@ func GetGatewaysManagedByLBController(ctx context.Context, k8sClient client.Clie
6363

6464
// GetImpactedGatewaysFromParentRefs identifies Gateways affected by changes in parent references.
6565
// Returns Gateways that are impacted and managed by the LB controller.
66-
func GetImpactedGatewaysFromParentRefs(ctx context.Context, k8sClient client.Client, parentRefs []gwv1.ParentReference, resourceNamespace string, gwController string) ([]types.NamespacedName, error) {
66+
func GetImpactedGatewaysFromParentRefs(ctx context.Context, k8sClient client.Client, parentRefs []gwv1.ParentReference, originalParentRefsFromStatus []gwv1.RouteParentStatus, resourceNamespace string, gwController string) ([]types.NamespacedName, error) {
67+
for _, originalParentRef := range originalParentRefsFromStatus {
68+
parentRefs = append(parentRefs, originalParentRef.ParentRef)
69+
}
70+
parentRefs = removeDuplicateParentRefs(parentRefs, resourceNamespace)
71+
6772
if len(parentRefs) == 0 {
6873
return nil, nil
6974
}
@@ -129,3 +134,23 @@ func GetImpactedGatewaysFromLbConfig(ctx context.Context, k8sClient client.Clien
129134
}
130135
return impactedGateways
131136
}
137+
138+
// removeDuplicateParentRefs make sure parentRefs in list is unique
139+
func removeDuplicateParentRefs(parentRefs []gwv1.ParentReference, resourceNamespace string) []gwv1.ParentReference {
140+
result := make([]gwv1.ParentReference, 0, len(parentRefs))
141+
exist := sets.Set[types.NamespacedName]{}
142+
for _, parentRef := range parentRefs {
143+
if parentRef.Namespace != nil {
144+
resourceNamespace = string(*parentRef.Namespace)
145+
}
146+
namespacedName := types.NamespacedName{
147+
Namespace: resourceNamespace,
148+
Name: string(parentRef.Name),
149+
}
150+
if !exist.Has(namespacedName) {
151+
exist.Insert(namespacedName)
152+
result = append(result, parentRef)
153+
}
154+
}
155+
return result
156+
}

‎controllers/gateway/eventhandlers/utils_test.go

Lines changed: 142 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -218,11 +218,12 @@ func Test_GetGatewayClassesManagedByLBController(t *testing.T) {
218218

219219
func Test_GetImpactedGatewaysFromParentRefs(t *testing.T) {
220220
type args struct {
221-
parentRefs []gwv1.ParentReference
222-
resourceNS string
223-
gateways []*gwv1.Gateway
224-
gatewayClasses []*gwv1.GatewayClass
225-
gwController string
221+
parentRefs []gwv1.ParentReference
222+
originalParentRefsFromRouteStatus []gwv1.RouteParentStatus
223+
resourceNS string
224+
gateways []*gwv1.Gateway
225+
gatewayClasses []*gwv1.GatewayClass
226+
gwController string
226227
}
227228
tests := []struct {
228229
name string
@@ -271,6 +272,68 @@ func Test_GetImpactedGatewaysFromParentRefs(t *testing.T) {
271272
},
272273
wantErr: nil,
273274
},
275+
{
276+
name: "valid parent refs with managed gateways and originalParentRefsFromRouteStatus",
277+
args: args{
278+
parentRefs: []gwv1.ParentReference{
279+
{
280+
Name: "test-gw",
281+
Namespace: (*gwv1.Namespace)(ptr.To("test-ns")),
282+
},
283+
},
284+
resourceNS: "test-ns",
285+
originalParentRefsFromRouteStatus: []gwv1.RouteParentStatus{
286+
{
287+
ParentRef: gwv1.ParentReference{
288+
Name: "test-gw-1",
289+
Namespace: (*gwv1.Namespace)(ptr.To("test-ns")),
290+
},
291+
},
292+
},
293+
gateways: []*gwv1.Gateway{
294+
{
295+
ObjectMeta: metav1.ObjectMeta{
296+
Name: "test-gw",
297+
Namespace: "test-ns",
298+
},
299+
Spec: gwv1.GatewaySpec{
300+
GatewayClassName: "nlb-class",
301+
},
302+
},
303+
{
304+
ObjectMeta: metav1.ObjectMeta{
305+
Name: "test-gw-1",
306+
Namespace: "test-ns",
307+
},
308+
Spec: gwv1.GatewaySpec{
309+
GatewayClassName: "nlb-class",
310+
},
311+
},
312+
},
313+
gatewayClasses: []*gwv1.GatewayClass{
314+
{
315+
ObjectMeta: metav1.ObjectMeta{
316+
Name: "nlb-class",
317+
},
318+
Spec: gwv1.GatewayClassSpec{
319+
ControllerName: constants.NLBGatewayController,
320+
},
321+
},
322+
},
323+
gwController: constants.NLBGatewayController,
324+
},
325+
want: []types.NamespacedName{
326+
{
327+
Namespace: "test-ns",
328+
Name: "test-gw",
329+
},
330+
{
331+
Namespace: "test-ns",
332+
Name: "test-gw-1",
333+
},
334+
},
335+
wantErr: nil,
336+
},
274337
{
275338
name: "valid parent refs with unmanaged gateways",
276339
args: args{
@@ -454,7 +517,7 @@ func Test_GetImpactedGatewaysFromParentRefs(t *testing.T) {
454517
k8sClient.Create(context.Background(), gwClass)
455518
}
456519

457-
got, err := GetImpactedGatewaysFromParentRefs(context.Background(), k8sClient, tt.args.parentRefs, tt.args.resourceNS, tt.args.gwController)
520+
got, err := GetImpactedGatewaysFromParentRefs(context.Background(), k8sClient, tt.args.parentRefs, tt.args.originalParentRefsFromRouteStatus, tt.args.resourceNS, tt.args.gwController)
458521

459522
assert.Equal(t, err, tt.wantErr)
460523
assert.Equal(t, tt.want, got)
@@ -768,3 +831,76 @@ func Test_GetImpactedGatewaysFromLbConfig(t *testing.T) {
768831
})
769832
}
770833
}
834+
835+
func TestRemoveDuplicateParentRefs(t *testing.T) {
836+
namespace := "test-namespace"
837+
tests := []struct {
838+
name string
839+
parentRefs []gwv1.ParentReference
840+
resourceNamespace string
841+
want []gwv1.ParentReference
842+
}{
843+
{
844+
name: "no duplicates",
845+
parentRefs: []gwv1.ParentReference{
846+
{Name: "gateway1"},
847+
{Name: "gateway2"},
848+
},
849+
resourceNamespace: namespace,
850+
want: []gwv1.ParentReference{
851+
{Name: "gateway1"},
852+
{Name: "gateway2"},
853+
},
854+
},
855+
{
856+
name: "one duplicate",
857+
parentRefs: []gwv1.ParentReference{
858+
{Name: "gateway1"},
859+
{Name: "gateway1"},
860+
{Name: "gateway2"},
861+
},
862+
resourceNamespace: namespace,
863+
want: []gwv1.ParentReference{
864+
{Name: "gateway1"},
865+
{Name: "gateway2"},
866+
},
867+
},
868+
{
869+
name: "multiple duplicates",
870+
parentRefs: []gwv1.ParentReference{
871+
{Name: "gateway1"},
872+
{Name: "gateway2"},
873+
{Name: "gateway1"},
874+
{Name: "gateway2"},
875+
{Name: "gateway3"},
876+
{Name: "gateway1"},
877+
},
878+
resourceNamespace: namespace,
879+
want: []gwv1.ParentReference{
880+
{Name: "gateway1"},
881+
{Name: "gateway2"},
882+
{Name: "gateway3"},
883+
},
884+
},
885+
}
886+
887+
for _, tt := range tests {
888+
t.Run(tt.name, func(t *testing.T) {
889+
got := removeDuplicateParentRefs(tt.parentRefs, tt.resourceNamespace)
890+
891+
assert.Equal(t, len(tt.want), len(got))
892+
893+
actual := make(map[string]bool)
894+
for _, ref := range got {
895+
actual[string(ref.Name)] = true
896+
}
897+
898+
expected := make(map[string]bool)
899+
for _, ref := range tt.want {
900+
expected[string(ref.Name)] = true
901+
}
902+
903+
assert.Equal(t, expected, actual)
904+
})
905+
}
906+
}

‎controllers/gateway/route_reconciler.go

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,12 @@ func (d *routeReconcilerImpl) handleRouteStatusUpdate(routeData routeutils.Route
9898
return client.IgnoreNotFound(err)
9999
}
100100
routeOld := route.DeepCopyObject().(client.Object)
101+
101102
// update route with current status
102103
if err := d.updateRouteStatus(route, routeData); err != nil {
103104
return err
104105
}
106+
105107
// compare it with original status, patch if different
106108
if !d.isRouteStatusIdentical(routeOld, route) {
107109
if err := d.k8sClient.Status().Patch(context.Background(), route, client.MergeFrom(routeOld)); err != nil {
@@ -173,19 +175,20 @@ func (d *routeReconcilerImpl) updateRouteStatus(route client.Object, routeData r
173175
if parentRef.Namespace != nil {
174176
namespace = string(*parentRef.Namespace)
175177
}
176-
// for a given parentRef, if it has a statusInfo, this means condition is updated, override route condition based on route status info
177-
if namespace == routeData.ParentRefGateway.Namespace {
178-
if string(parentRef.Name) == routeData.ParentRefGateway.Name {
178+
179+
// do not allow backward generation update, Accepted and ResolvedRef always have same generation based on our implementation
180+
if (len(newRouteParentStatus.Conditions) != 0 && newRouteParentStatus.Conditions[0].ObservedGeneration <= routeData.RouteMetadata.RouteGeneration) || len(newRouteParentStatus.Conditions) == 0 {
181+
// for a given parentRef, if it has a statusInfo, this means condition is updated, override route condition based on route status info
182+
if namespace == routeData.ParentRefGateway.Namespace && string(parentRef.Name) == routeData.ParentRefGateway.Name {
179183
d.setConditionsWithRouteStatusInfo(route, &newRouteParentStatus, routeData.RouteStatusInfo)
180184
}
181-
}
182185

183-
// resolve ref Gateway, if parentRef does not have namespace, getting it from Route
184-
if _, err := d.resolveRefGateway(parentRef, route.GetNamespace()); err != nil {
185-
// set conditions if resolvedRef = false
186-
d.setConditionsBasedOnResolveRefGateway(route, &newRouteParentStatus, err)
186+
// resolve ref Gateway, if parentRef does not have namespace, getting it from Route
187+
if _, err := d.resolveRefGateway(parentRef, route.GetNamespace()); err != nil {
188+
// set conditions if resolvedRef = false
189+
d.setConditionsBasedOnResolveRefGateway(route, &newRouteParentStatus, err)
190+
}
187191
}
188-
189192
newRouteStatus = append(newRouteStatus, newRouteParentStatus)
190193
}
191194

‎pkg/gateway/routeutils/descriptor.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ type routeMetadataDescriptor interface {
1717
GetParentRefs() []gwv1.ParentReference
1818
GetRawRoute() interface{}
1919
GetBackendRefs() []gwv1.BackendRef
20+
GetRouteGeneration() int64
2021
}
2122

2223
// preLoadRouteDescriptor this object is used to represent a route description that has not loaded its child data (services, tg config)

‎pkg/gateway/routeutils/grpc.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,10 @@ func (grpcRoute *grpcRouteDescription) GetBackendRefs() []gwv1.BackendRef {
111111
return backendRefs
112112
}
113113

114+
func (grpcRoute *grpcRouteDescription) GetRouteGeneration() int64 {
115+
return grpcRoute.route.Generation
116+
}
117+
114118
var _ RouteDescriptor = &grpcRouteDescription{}
115119

116120
// Can we use an indexer here to query more efficiently?

‎pkg/gateway/routeutils/http.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,10 @@ func (httpRoute *httpRouteDescription) GetRouteKind() RouteKind {
8787
return HTTPRouteKind
8888
}
8989

90+
func (httpRoute *httpRouteDescription) GetRouteGeneration() int64 {
91+
return httpRoute.route.Generation
92+
}
93+
9094
func (httpRoute *httpRouteDescription) GetRouteNamespacedName() types.NamespacedName {
9195
return k8s.NamespacedName(httpRoute.route)
9296
}

‎pkg/gateway/routeutils/listener_attachment_helper.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(c
4141
}
4242
if !namespaceOK {
4343
deferredRouteReconciler.Enqueue(
44-
GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageNamespaceNotMatch, route, gw),
44+
GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageNamespaceNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw),
4545
)
4646

4747
return false, nil
@@ -51,7 +51,7 @@ func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(c
5151
kindOK := attachmentHelper.kindCheck(listener, route)
5252
if !kindOK {
5353
deferredRouteReconciler.Enqueue(
54-
GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route, gw),
54+
GenerateRouteData(false, true, string(gwv1.RouteReasonNotAllowedByListeners), RouteStatusInfoRejectedMessageKindNotMatch, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw),
5555
)
5656
return false, nil
5757
}
@@ -65,7 +65,7 @@ func (attachmentHelper *listenerAttachmentHelperImpl) listenerAllowsAttachment(c
6565
if !hostnameOK {
6666
// hostname is not ok, print out gwName and gwNamespace test-gw-alb gateway-alb
6767
deferredRouteReconciler.Enqueue(
68-
GenerateRouteData(false, true, string(gwv1.RouteReasonNoMatchingListenerHostname), RouteStatusInfoRejectedMessageNoMatchingHostname, route, gw),
68+
GenerateRouteData(false, true, string(gwv1.RouteReasonNoMatchingListenerHostname), RouteStatusInfoRejectedMessageNoMatchingHostname, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw),
6969
)
7070
return false, nil
7171
}

‎pkg/gateway/routeutils/loader.go

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -92,22 +92,27 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway,
9292
return nil, err
9393
}
9494

95+
// 3. Load the underlying resource(s) for each route that is configured.
96+
loadedRoute, err := l.loadChildResources(ctx, mappedRoutes)
97+
if err != nil {
98+
// TODO: add route status update for those with error
99+
return nil, err
100+
}
101+
95102
// update status for accepted routes
96-
for _, routeList := range mappedRoutes {
103+
for _, routeList := range loadedRoute {
97104
for _, route := range routeList {
98105
deferredRouteReconciler.Enqueue(
99-
GenerateRouteData(true, true, string(gwv1.RouteConditionAccepted), RouteStatusInfoAcceptedMessage, route, gw),
106+
GenerateRouteData(true, true, string(gwv1.RouteConditionAccepted), RouteStatusInfoAcceptedMessage, route.GetRouteNamespacedName(), route.GetRouteKind(), route.GetRouteGeneration(), gw),
100107
)
101108
}
102109
}
103-
104-
// 3. Load the underlying resource(s) for each route that is configured.
105-
return l.loadChildResources(ctx, mappedRoutes)
110+
return loadedRoute, nil
106111
}
107112

108113
// loadChildResources responsible for loading all resources that a route descriptor references.
109114
func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor) (map[int32][]RouteDescriptor, error) {
110-
// Cache to reduce duplicate route look ups.
115+
// Cache to reduce duplicate route lookups.
111116
// Kind -> [NamespacedName:Previously Loaded Descriptor]
112117
resourceCache := make(map[string]RouteDescriptor)
113118

0 commit comments

Comments
 (0)