Skip to content

Commit 3ed9e17

Browse files
author
Kate Osborn
committed
Fix referenced services; we don't care about attached, we care that the service belongs to the winning gateway
1 parent c05c5bd commit 3ed9e17

File tree

8 files changed

+125
-250
lines changed

8 files changed

+125
-250
lines changed

‎internal/mode/static/nginx/config/upstreams_template.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ upstream {{ $u.Name }} {
1414
{{ if $u.ZoneSize -}}
1515
zone {{ $u.Name }} {{ $u.ZoneSize }};
1616
{{- end }}
17-
{{ range $server := $u.Servers }}
17+
{{ range $server := $u.Servers -}}
1818
server {{ $server.Address }};
1919
{{- end }}
2020
{{ if $u.KeepAlive.Connections -}}

‎internal/mode/static/state/change_processor_test.go

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -739,12 +739,8 @@ var _ = Describe("ChangeProcessor", func() {
739739
Routes: map[graph.RouteKey]*graph.L7Route{routeKey1: expRouteHR1},
740740
ReferencedSecrets: map[types.NamespacedName]*graph.Secret{},
741741
ReferencedServices: map[types.NamespacedName]*graph.ReferencedService{
742-
refSvc: {
743-
ParentGateways: []types.NamespacedName{{Namespace: "test", Name: "gateway-1"}},
744-
},
745-
refTLSSvc: {
746-
ParentGateways: []types.NamespacedName{{Namespace: "test", Name: "gateway-1"}},
747-
},
742+
refSvc: {},
743+
refTLSSvc: {},
748744
},
749745
}
750746
})
@@ -1257,9 +1253,6 @@ var _ = Describe("ChangeProcessor", func() {
12571253

12581254
delete(expGraph.ReferencedServices, expRouteHR1.Spec.Rules[0].BackendRefs[0].SvcNsName)
12591255
expRouteHR1.Spec.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{}
1260-
expGraph.ReferencedServices[refTLSSvc].ParentGateways = []types.NamespacedName{
1261-
client.ObjectKeyFromObject(gw2),
1262-
}
12631256

12641257
changed, graphCfg := processor.Process()
12651258
Expect(changed).To(Equal(state.ClusterStateChange))
@@ -1304,9 +1297,6 @@ var _ = Describe("ChangeProcessor", func() {
13041297

13051298
delete(expGraph.ReferencedServices, expRouteHR1.Spec.Rules[0].BackendRefs[0].SvcNsName)
13061299
expRouteHR1.Spec.Rules[0].BackendRefs[0].SvcNsName = types.NamespacedName{}
1307-
expGraph.ReferencedServices[refTLSSvc].ParentGateways = []types.NamespacedName{
1308-
client.ObjectKeyFromObject(gw2),
1309-
}
13101300

13111301
changed, graphCfg := processor.Process()
13121302
Expect(changed).To(Equal(state.ClusterStateChange))

‎internal/mode/static/state/graph/graph.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ func BuildGraph(
258258

259259
referencedNamespaces := buildReferencedNamespaces(state.Namespaces, gw)
260260

261-
referencedServices := buildReferencedServices(routes, l4routes)
261+
referencedServices := buildReferencedServices(routes, l4routes, gw)
262262

263263
// policies must be processed last because they rely on the state of the other resources in the graph
264264
processedPolicies := processPolicies(

‎internal/mode/static/state/graph/graph_test.go

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -894,12 +894,8 @@ func TestBuildGraph(t *testing.T) {
894894
client.ObjectKeyFromObject(ns): ns,
895895
},
896896
ReferencedServices: map[types.NamespacedName]*ReferencedService{
897-
client.ObjectKeyFromObject(svc): {
898-
ParentGateways: []types.NamespacedName{{Namespace: gw1.Namespace, Name: gw1.Name}},
899-
},
900-
client.ObjectKeyFromObject(svc1): {
901-
ParentGateways: []types.NamespacedName{{Namespace: gw1.Namespace, Name: gw1.Name}},
902-
},
897+
client.ObjectKeyFromObject(svc): {},
898+
client.ObjectKeyFromObject(svc1): {},
903899
},
904900
ReferencedCaCertConfigMaps: map[types.NamespacedName]*CaCertConfigMap{
905901
client.ObjectKeyFromObject(cm): {

‎internal/mode/static/state/graph/policies.go

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -102,33 +102,21 @@ func attachPolicyToService(
102102
gw *Gateway,
103103
ctlrName string,
104104
) {
105-
var ancestor *PolicyAncestor
106-
107-
for _, parentGw := range svc.ParentGateways {
108-
if parentGw != client.ObjectKeyFromObject(gw.Source) {
109-
continue
110-
}
111-
// Once we support multiple Gateways, this will turn into a list.
112-
ancestor = &PolicyAncestor{
113-
Ancestor: createParentReference(v1.GroupName, kinds.Gateway, parentGw),
114-
}
115-
}
116-
117-
if ancestor == nil {
105+
if ngfPolicyAncestorsFull(policy, ctlrName) {
118106
return
119107
}
120108

121-
if ngfPolicyAncestorsFull(policy, ctlrName) {
122-
return
109+
ancestor := PolicyAncestor{
110+
Ancestor: createParentReference(v1.GroupName, kinds.Gateway, client.ObjectKeyFromObject(gw.Source)),
123111
}
124112

125113
if !gw.Valid {
126114
ancestor.Conditions = []conditions.Condition{staticConds.NewPolicyTargetNotFound("Parent Gateway is invalid")}
127-
policy.Ancestors = append(policy.Ancestors, *ancestor)
115+
policy.Ancestors = append(policy.Ancestors, ancestor)
128116
return
129117
}
130118

131-
policy.Ancestors = append(policy.Ancestors, *ancestor)
119+
policy.Ancestors = append(policy.Ancestors, ancestor)
132120
svc.Policies = append(svc.Policies, policy)
133121
}
134122

‎internal/mode/static/state/graph/policies_test.go

Lines changed: 9 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,7 @@ func TestAttachPolicies(t *testing.T) {
158158

159159
getServices := func() map[types.NamespacedName]*ReferencedService {
160160
return map[types.NamespacedName]*ReferencedService{
161-
{Namespace: testNs, Name: "svc-1"}: {
162-
ParentGateways: []types.NamespacedName{
163-
{Namespace: testNs, Name: "gateway"},
164-
},
165-
},
161+
{Namespace: testNs, Name: "svc-1"}: {},
166162
}
167163
}
168164

@@ -542,15 +538,14 @@ func TestAttachPolicyToGateway(t *testing.T) {
542538
func TestAttachPolicyToService(t *testing.T) {
543539
t.Parallel()
544540

545-
winningGwNsName := types.NamespacedName{Namespace: testNs, Name: "gateway"}
546-
ignoredGwNsName := types.NamespacedName{Namespace: testNs, Name: "ignored-gateway"}
541+
gwNsname := types.NamespacedName{Namespace: testNs, Name: "gateway"}
547542

548543
getGateway := func(valid bool) *Gateway {
549544
return &Gateway{
550545
Source: &v1.Gateway{
551546
ObjectMeta: metav1.ObjectMeta{
552-
Name: winningGwNsName.Name,
553-
Namespace: winningGwNsName.Namespace,
547+
Name: gwNsname.Name,
548+
Namespace: gwNsname.Namespace,
554549
},
555550
},
556551
Valid: valid,
@@ -568,52 +563,32 @@ func TestAttachPolicyToService(t *testing.T) {
568563
{
569564
name: "attachment",
570565
policy: &Policy{Source: &policiesfakes.FakePolicy{}},
571-
svc: &ReferencedService{ParentGateways: []types.NamespacedName{winningGwNsName}},
572-
gw: getGateway(true /*valid*/),
573-
expAttached: true,
574-
expAncestors: []PolicyAncestor{
575-
{
576-
Ancestor: getGatewayParentRef(winningGwNsName),
577-
},
578-
},
579-
},
580-
{
581-
name: "attachment; multiple parent refs - one is winning gateway",
582-
policy: &Policy{Source: &policiesfakes.FakePolicy{}},
583-
svc: &ReferencedService{ParentGateways: []types.NamespacedName{ignoredGwNsName, winningGwNsName}},
566+
svc: &ReferencedService{},
584567
gw: getGateway(true /*valid*/),
585568
expAttached: true,
586569
expAncestors: []PolicyAncestor{
587570
{
588-
Ancestor: getGatewayParentRef(winningGwNsName),
571+
Ancestor: getGatewayParentRef(gwNsname),
589572
},
590573
},
591574
},
592-
{
593-
name: "no attachment; parent gateway is not winning gateway",
594-
policy: &Policy{Source: &policiesfakes.FakePolicy{}},
595-
svc: &ReferencedService{ParentGateways: []types.NamespacedName{ignoredGwNsName}},
596-
gw: getGateway(true /*valid*/),
597-
expAttached: false,
598-
expAncestors: nil,
599-
},
600575
{
601576
name: "no attachment; gateway is invalid",
602577
policy: &Policy{Source: &policiesfakes.FakePolicy{}},
603-
svc: &ReferencedService{ParentGateways: []types.NamespacedName{winningGwNsName}},
578+
svc: &ReferencedService{},
604579
gw: getGateway(false /*invalid*/),
605580
expAttached: false,
606581
expAncestors: []PolicyAncestor{
607582
{
608-
Ancestor: getGatewayParentRef(winningGwNsName),
583+
Ancestor: getGatewayParentRef(gwNsname),
609584
Conditions: []conditions.Condition{staticConds.NewPolicyTargetNotFound("Parent Gateway is invalid")},
610585
},
611586
},
612587
},
613588
{
614589
name: "no attachment; max ancestor",
615590
policy: &Policy{Source: createTestPolicyWithAncestors(16)},
616-
svc: &ReferencedService{ParentGateways: []types.NamespacedName{winningGwNsName}},
591+
svc: &ReferencedService{},
617592
gw: getGateway(true /*valid*/),
618593
expAttached: false,
619594
expAncestors: nil,

‎internal/mode/static/state/graph/service.go

Lines changed: 20 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -2,26 +2,31 @@ package graph
22

33
import (
44
"k8s.io/apimachinery/pkg/types"
5+
"sigs.k8s.io/controller-runtime/pkg/client"
56
)
67

7-
// A ReferencedService represents a Kubernetes Service that is referenced by a Route.
8-
// It does not contain the v1.Service object, because Services are resolved when building the dataplane.Configuration.
8+
// A ReferencedService represents a Kubernetes Service that is referenced by a Route and that belongs to the
9+
// winning Gateway. It does not contain the v1.Service object, because Services are resolved when building
10+
// the dataplane.Configuration.
911
type ReferencedService struct {
10-
// ParentGateways is a list of unique attached parent Gateways for the Routes that reference this Service.
11-
ParentGateways []types.NamespacedName
1212
// Policies is a list of NGF Policies that target this Service.
1313
Policies []*Policy
1414
}
1515

1616
func buildReferencedServices(
1717
l7routes map[RouteKey]*L7Route,
1818
l4Routes map[L4RouteKey]*L4Route,
19+
gw *Gateway,
1920
) map[types.NamespacedName]*ReferencedService {
21+
if gw == nil {
22+
return nil
23+
}
24+
2025
referencedServices := make(map[types.NamespacedName]*ReferencedService)
2126

22-
attached := func(parentRefs []ParentRef) bool {
23-
for _, ref := range parentRefs {
24-
if ref.Attachment != nil && ref.Attachment.Attached {
27+
belongsToWinningGw := func(refs []ParentRef) bool {
28+
for _, ref := range refs {
29+
if ref.Gateway == client.ObjectKeyFromObject(gw.Source) {
2530
return true
2631
}
2732
}
@@ -31,26 +36,23 @@ func buildReferencedServices(
3136

3237
// Processes both valid and invalid BackendRefs as invalid ones still have referenced services
3338
// we may want to track.
34-
35-
addServicesForL7Routes := func(routeRules []RouteRule, parentGateways []types.NamespacedName) {
39+
addServicesForL7Routes := func(routeRules []RouteRule) {
3640
for _, rule := range routeRules {
3741
for _, ref := range rule.BackendRefs {
3842
if ref.SvcNsName != (types.NamespacedName{}) {
3943
referencedServices[ref.SvcNsName] = &ReferencedService{
40-
ParentGateways: parentGateways,
41-
Policies: nil,
44+
Policies: nil,
4245
}
4346
}
4447
}
4548
}
4649
}
4750

48-
addServicesForL4Routes := func(route *L4Route, parentGateways []types.NamespacedName) {
51+
addServicesForL4Routes := func(route *L4Route) {
4952
nsname := route.Spec.BackendRef.SvcNsName
5053
if nsname != (types.NamespacedName{}) {
5154
referencedServices[nsname] = &ReferencedService{
52-
ParentGateways: parentGateways,
53-
Policies: nil,
55+
Policies: nil,
5456
}
5557
}
5658
}
@@ -63,25 +65,23 @@ func buildReferencedServices(
6365
continue
6466
}
6567

66-
// If none of the ParentRefs are attached to the Gateway, we want to skip the route.
67-
if !attached(route.ParentRefs) {
68+
if !belongsToWinningGw(route.ParentRefs) {
6869
continue
6970
}
7071

71-
addServicesForL7Routes(route.Spec.Rules, getUniqueAttachedParentGateways(route.ParentRefs))
72+
addServicesForL7Routes(route.Spec.Rules)
7273
}
7374

7475
for _, route := range l4Routes {
7576
if !route.Valid {
7677
continue
7778
}
7879

79-
// If none of the ParentRefs are attached to the Gateway, we want to skip the route.
80-
if !attached(route.ParentRefs) {
80+
if !belongsToWinningGw(route.ParentRefs) {
8181
continue
8282
}
8383

84-
addServicesForL4Routes(route, getUniqueAttachedParentGateways(route.ParentRefs))
84+
addServicesForL4Routes(route)
8585
}
8686

8787
if len(referencedServices) == 0 {
@@ -90,20 +90,3 @@ func buildReferencedServices(
9090

9191
return referencedServices
9292
}
93-
94-
func getUniqueAttachedParentGateways(parentRefs []ParentRef) []types.NamespacedName {
95-
gatewayMap := map[types.NamespacedName]struct{}{}
96-
for _, ref := range parentRefs {
97-
if ref.Attachment == nil || !ref.Attachment.Attached {
98-
continue
99-
}
100-
gatewayMap[ref.Gateway] = struct{}{}
101-
}
102-
103-
uniqueGateways := make([]types.NamespacedName, 0, len(gatewayMap))
104-
for gateway := range gatewayMap {
105-
uniqueGateways = append(uniqueGateways, gateway)
106-
}
107-
108-
return uniqueGateways
109-
}

0 commit comments

Comments
 (0)