diff --git a/internal/provider/kubernetes/status.go b/internal/provider/kubernetes/status.go index e9abf085a1..29ec951146 100644 --- a/internal/provider/kubernetes/status.go +++ b/internal/provider/kubernetes/status.go @@ -104,7 +104,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext panic(err) } hCopy := h.DeepCopy() - hCopy.Status.Parents = mergeRouteParentStatus(h.Namespace, h.Status.Parents, val.Parents) + hCopy.Status.Parents = mergeRouteParentStatus(h.Namespace, r.envoyGateway.Gateway.ControllerName, h.Status.Parents, val.Parents) return hCopy }), }) @@ -134,7 +134,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext panic(err) } gCopy := g.DeepCopy() - gCopy.Status.Parents = mergeRouteParentStatus(g.Namespace, g.Status.Parents, val.Parents) + gCopy.Status.Parents = mergeRouteParentStatus(g.Namespace, r.envoyGateway.Gateway.ControllerName, g.Status.Parents, val.Parents) return gCopy }), }) @@ -166,7 +166,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext panic(err) } tCopy := t.DeepCopy() - tCopy.Status.Parents = mergeRouteParentStatus(t.Namespace, t.Status.Parents, val.Parents) + tCopy.Status.Parents = mergeRouteParentStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Parents, val.Parents) return tCopy }), }) @@ -198,7 +198,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext panic(err) } tCopy := t.DeepCopy() - tCopy.Status.Parents = mergeRouteParentStatus(t.Namespace, t.Status.Parents, val.Parents) + tCopy.Status.Parents = mergeRouteParentStatus(t.Namespace, r.envoyGateway.Gateway.ControllerName, t.Status.Parents, val.Parents) return tCopy }), }) @@ -230,7 +230,7 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext panic(err) } uCopy := u.DeepCopy() - uCopy.Status.Parents = mergeRouteParentStatus(u.Namespace, u.Status.Parents, val.Parents) + uCopy.Status.Parents = mergeRouteParentStatus(u.Namespace, r.envoyGateway.Gateway.ControllerName, u.Status.Parents, val.Parents) return uCopy }), }) @@ -501,21 +501,41 @@ func (r *gatewayAPIReconciler) subscribeAndUpdateStatus(ctx context.Context, ext // mergeRouteParentStatus merges the old and new RouteParentStatus. // This is needed because the RouteParentStatus doesn't support strategic merge patch yet. -func mergeRouteParentStatus(ns string, old, new []gwapiv1.RouteParentStatus) []gwapiv1.RouteParentStatus { - merged := make([]gwapiv1.RouteParentStatus, len(old)) - _ = copy(merged, old) - for _, parent := range new { +func mergeRouteParentStatus(ns, controllerName string, old, new []gwapiv1.RouteParentStatus) []gwapiv1.RouteParentStatus { + // Allocating with worst-case capacity to avoid reallocation. + merged := make([]gwapiv1.RouteParentStatus, 0, len(old)+len(new)) + + // Range over old status parentRefs in order: + // 1. The parentRef exists in the new status: append the new one to the final status. + // 2. The parentRef doesn't exist in the new status and it's not our controller: append it to the final status. + // 3. The parentRef doesn't exist in the new status, and it is our controller: don't append it to the final status. + for _, oldP := range old { found := -1 - for i, existing := range old { - if isParentRefEqual(parent.ParentRef, existing.ParentRef, ns) { - found = i + for newI, newP := range new { + if isParentRefEqual(oldP.ParentRef, newP.ParentRef, ns) { + found = newI break } } if found >= 0 { - merged[found] = parent - } else { - merged = append(merged, parent) + merged = append(merged, new[found]) + } else if oldP.ControllerName != gwapiv1.GatewayController(controllerName) { + merged = append(merged, oldP) + } + } + + // Range over new status parentRefs and make sure every parentRef exists in the final status. If not, append it. + for _, newP := range new { + found := false + for _, mergedP := range merged { + if isParentRefEqual(newP.ParentRef, mergedP.ParentRef, ns) { + found = true + break + } + } + + if !found { + merged = append(merged, newP) } } return merged diff --git a/internal/provider/kubernetes/status_test.go b/internal/provider/kubernetes/status_test.go index 5e81c46135..63b7fdea7f 100644 --- a/internal/provider/kubernetes/status_test.go +++ b/internal/provider/kubernetes/status_test.go @@ -25,11 +25,11 @@ func Test_mergeRouteParentStatus(t *testing.T) { want []gwapiv1.RouteParentStatus }{ { - name: "merge old and new", + name: "old contains one parentRef of ours and one of another controller's, status of ours changed in new.", args: args{ old: []gwapiv1.RouteParentStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ControllerName: "istio.io/gateway-controller", ParentRef: gwapiv1.ParentReference{ Name: "gateway1", Namespace: ptr.To[gwapiv1.Namespace]("default"), @@ -49,6 +49,24 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, }, new: []gwapiv1.RouteParentStatus{ { @@ -59,6 +77,11 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, @@ -68,7 +91,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, want: []gwapiv1.RouteParentStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ControllerName: "istio.io/gateway-controller", ParentRef: gwapiv1.ParentReference{ Name: "gateway1", Namespace: ptr.To[gwapiv1.Namespace]("default"), @@ -96,6 +119,11 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, @@ -103,15 +131,17 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, - { - name: "override an existing parent", + name: "old contains one parentRef of ours and one of another controller's, status of ours changed in new with an additional parentRef of ours", args: args{ old: []gwapiv1.RouteParentStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ControllerName: "istio.io/gateway-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), }, Conditions: []metav1.Condition{ { @@ -129,8 +159,7 @@ func Test_mergeRouteParentStatus(t *testing.T) { { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", - Namespace: ptr.To[gwapiv1.Namespace]("default"), + Name: "gateway2", }, Conditions: []metav1.Condition{ { @@ -155,18 +184,44 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, }, }, want: []gwapiv1.RouteParentStatus{ { - ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ControllerName: "istio.io/gateway-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), }, Conditions: []metav1.Condition{ { @@ -189,22 +244,229 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + }, + { + name: "old contains one parentRef of ours and one of another controller's, ours gets dropped in new and a different parentRef of ours is added", + args: args{ + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "istio.io/gateway-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + }, + want: []gwapiv1.RouteParentStatus{ + { + ControllerName: "istio.io/gateway-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + }, + // Practically this will never occur, since having no parentRefs in the new + // status means the route doesn't attach (in the spec) to any of our gateways. + // + // But then we'd consider it irrelevant before ever computing such status for it, i.e, the + // route will forever have a dangling status parentRef referencing us that will not be removed. + // + // TODO: maybe this needs to be fixed. + { + name: "old contains one parentRef of ours and one of another controller's, ours gets dropped in new.", + args: args{ + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "istio.io/gateway-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{}, + }, + want: []gwapiv1.RouteParentStatus{ + { + ControllerName: "istio.io/gateway-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway1", + Namespace: ptr.To[gwapiv1.Namespace]("default"), + SectionName: ptr.To[gwapiv1.SectionName]("listener1"), + Port: ptr.To[gwapiv1.PortNumber](80), + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, }, }, { - name: "nothing changed", + name: "old contains one parentRef of ours, status of ours changed in new.", args: args{ old: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + Name: "gateway2", }, Conditions: []metav1.Condition{ { @@ -219,6 +481,8 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, + }, + new: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ @@ -227,12 +491,62 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, }, }, }, + }, + want: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionFalse, + Reason: "SomeReason", + }, + }, + }, + }, + }, + { + name: "old contains one parentRef of ours, status of ours changed in new with an additional parentRef of ours", + args: args{ + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, new: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", @@ -242,18 +556,59 @@ func Test_mergeRouteParentStatus(t *testing.T) { Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), Status: metav1.ConditionFalse, Reason: "SomeReason", }, }, }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, }, }, want: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway1", + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionFalse, + Reason: "SomeReason", + }, + }, + }, + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", }, Conditions: []metav1.Condition{ { @@ -268,25 +623,105 @@ func Test_mergeRouteParentStatus(t *testing.T) { }, }, }, + }, + }, + { + name: "old contains one parentRef of ours, ours gets dropped in new and a different parentRef of ours is added", + args: args{ + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + new: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway3", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + }, + want: []gwapiv1.RouteParentStatus{ { ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", ParentRef: gwapiv1.ParentReference{ - Name: "gateway2", + Name: "gateway3", }, Conditions: []metav1.Condition{ { Type: string(gwapiv1.RouteConditionAccepted), - Status: metav1.ConditionFalse, - Reason: "SomeReason", + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, + }, + }, + }, + }, + // Similar note about practicality of occurrence. + { + name: "old contains one parentRef of ours, and it gets dropped in new.", + args: args{ + old: []gwapiv1.RouteParentStatus{ + { + ControllerName: "gateway.envoyproxy.io/gatewayclass-controller", + ParentRef: gwapiv1.ParentReference{ + Name: "gateway2", + }, + Conditions: []metav1.Condition{ + { + Type: string(gwapiv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "Accepted", + }, + { + Type: string(gwapiv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + Reason: "ResolvedRefs", + }, }, }, }, + new: []gwapiv1.RouteParentStatus{}, }, + want: []gwapiv1.RouteParentStatus{}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := mergeRouteParentStatus("default", tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { + if got := mergeRouteParentStatus("default", "gateway.envoyproxy.io/gatewayclass-controller", tt.args.old, tt.args.new); !reflect.DeepEqual(got, tt.want) { t.Errorf("mergeRouteParentStatus() = %v, want %v", got, tt.want) } })