Skip to content

Commit 7ad0887

Browse files
pleshakovsjberman
pleshakov
and
sjberman
authored
Fix attaching of HTTPRoutes to Gateway Listeners (#1275)
Problem: - Rules for traffic attachment of HTTPRoutes to Gateways were clarified in kubernetes-sigs/gateway-api#2396 -- successful attachment should depend only on parentRefs in an HTTPRoute and AllowedRoutes of a Listener, even if either or both of them are invalid. - The corresponding conformance test GatewayWithAttachedRoutes was added in kubernetes-sigs/gateway-api#2477 , which fails for NGINX Gateway Fabric. - NGINX Gateway Fabric will not try to attach an HTTPRoute to a Listener if either or both of them are invalid. Solution: - Make NGF compliant with the Gateway API and make the corresponding test pass. - Introduce Attachable fields for Listener and HTTPRoute types of the Graph in the graph package. - Update the validation logic: - NGF considers a Listener attachable if (a) its hostname is valid, (b) protocol is supported by NGF and (c) AllowedRoutes are valid. - NGF considers an HTTPRoute attachable if (d) its hostnames are valid. - Attach an HTTPRoute to a Listener if both are attachable. Note: (a), (b) and (d) are not mentioned in kubernetes-sigs/gateway-api#2396 However, they are necessary: For (b), NGF doesn't know how to attach to non-supported protocols like TCP. For (a), Listener hostname needed for HTTPRoute attaching, because it affects if an HTTPRoute can attach or not (per Gateway API spec). For (c), HTTPRoute hostnames are also needed, because they affect if an HTTPRoute can attach or not per Gateway API spec). See https://github.com/kubernetes-sigs/gateway-api/blob/52c2994ed9de1c287a37465490b91cfcf01bf16e/apis/v1/httproute_types.go#L71-L73 Testing: - Unit tests are updated and extended - Failing conformance test GatewayWithAttachedRoutes now passes. CLOSES #1148 Co-authored-by: Saylor Berman <s.berman@f5.com>
1 parent fdbe668 commit 7ad0887

9 files changed

+543
-209
lines changed

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

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -407,7 +407,8 @@ var _ = Describe("ChangeProcessor", func() {
407407
ValidFilters: true,
408408
},
409409
},
410-
Valid: true,
410+
Valid: true,
411+
Attachable: true,
411412
Conditions: []conditions.Condition{
412413
staticConds.NewRouteBackendRefRefBackendNotFound(
413414
"spec.rules[0].backendRefs[0].name: Not found: \"service\"",
@@ -434,8 +435,9 @@ var _ = Describe("ChangeProcessor", func() {
434435
Idx: 1,
435436
},
436437
},
437-
Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}},
438-
Valid: true,
438+
Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}},
439+
Valid: true,
440+
Attachable: true,
439441
}
440442

441443
// This is the base case expected graph. Tests will manipulate this to add or remove elements
@@ -449,16 +451,18 @@ var _ = Describe("ChangeProcessor", func() {
449451
Source: gw1,
450452
Listeners: map[string]*graph.Listener{
451453
"listener-80-1": {
452-
Source: gw1.Spec.Listeners[0],
453-
Valid: true,
454+
Source: gw1.Spec.Listeners[0],
455+
Valid: true,
456+
Attachable: true,
454457
Routes: map[types.NamespacedName]*graph.Route{
455458
{Namespace: "test", Name: "hr-1"}: expRouteHR1,
456459
},
457460
SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}},
458461
},
459462
"listener-443-1": {
460-
Source: gw1.Spec.Listeners[1],
461-
Valid: true,
463+
Source: gw1.Spec.Listeners[1],
464+
Valid: true,
465+
Attachable: true,
462466
Routes: map[types.NamespacedName]*graph.Route{
463467
{Namespace: "test", Name: "hr-1"}: expRouteHR1,
464468
},
@@ -541,32 +545,40 @@ var _ = Describe("ChangeProcessor", func() {
541545
It("returns updated graph", func() {
542546
processor.CaptureUpsertChange(gc)
543547

544-
// no ref grant exists yet for gw1
545-
expGraph.Gateway.Listeners["listener-443-1"] = &graph.Listener{
546-
Source: gw1.Spec.Listeners[1],
547-
Valid: false,
548-
Routes: map[types.NamespacedName]*graph.Route{},
549-
Conditions: staticConds.NewListenerRefNotPermitted(
550-
"Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant",
551-
),
552-
SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}},
548+
// No ref grant exists yet for gw1
549+
// so the listener is not valid, but still attachable
550+
expGraph.Gateway.Listeners["listener-443-1"].Valid = false
551+
expGraph.Gateway.Listeners["listener-443-1"].ResolvedSecret = nil
552+
expGraph.Gateway.Listeners["listener-443-1"].Conditions = staticConds.NewListenerRefNotPermitted(
553+
"Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant",
554+
)
555+
556+
expAttachment80 := &graph.ParentRefAttachmentStatus{
557+
AcceptedHostnames: map[string][]string{
558+
"listener-80-1": {"foo.example.com"},
559+
},
560+
Attached: true,
553561
}
554562

555-
expAttachment := &graph.ParentRefAttachmentStatus{
556-
AcceptedHostnames: map[string][]string{},
557-
FailedCondition: staticConds.NewRouteInvalidListener(),
558-
Attached: false,
563+
expAttachment443 := &graph.ParentRefAttachmentStatus{
564+
AcceptedHostnames: map[string][]string{
565+
"listener-443-1": {"foo.example.com"},
566+
},
567+
Attached: true,
559568
}
560569

561-
expGraph.Gateway.Listeners["listener-80-1"].Routes[hr1Name].ParentRefs[1].Attachment = expAttachment
570+
expGraph.Gateway.Listeners["listener-80-1"].Routes[hr1Name].ParentRefs[0].Attachment = expAttachment80
571+
expGraph.Gateway.Listeners["listener-443-1"].Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443
562572

563573
// no ref grant exists yet for hr1
564-
expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment
565574
expGraph.Routes[hr1Name].Conditions = []conditions.Condition{
575+
staticConds.NewRouteInvalidListener(),
566576
staticConds.NewRouteBackendRefRefNotPermitted(
567577
"Backend ref to Service service-ns/service not permitted by any ReferenceGrant",
568578
),
569579
}
580+
expGraph.Routes[hr1Name].ParentRefs[0].Attachment = expAttachment80
581+
expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443
570582

571583
expGraph.ReferencedSecrets = nil
572584

‎internal/mode/static/state/dataplane/configuration.go

Lines changed: 65 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
apiv1 "k8s.io/api/core/v1"
99
"k8s.io/apimachinery/pkg/types"
10+
"sigs.k8s.io/controller-runtime/pkg/client"
1011
v1 "sigs.k8s.io/gateway-api/apis/v1"
1112

1213
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
@@ -202,70 +203,80 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) {
202203
hpr.httpsListeners = append(hpr.httpsListeners, l)
203204
}
204205

205-
for routeNsName, r := range l.Routes {
206-
var hostnames []string
207-
for _, p := range r.ParentRefs {
208-
if val, exist := p.Attachment.AcceptedHostnames[string(l.Source.Name)]; exist {
209-
hostnames = val
210-
}
206+
for _, r := range l.Routes {
207+
if !r.Valid {
208+
continue
211209
}
212210

213-
for _, h := range hostnames {
214-
if prevListener, exists := hpr.listenersForHost[h]; exists {
215-
// override the previous listener if the new one has a more specific hostname
216-
if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) {
217-
hpr.listenersForHost[h] = l
218-
}
219-
} else {
220-
hpr.listenersForHost[h] = l
221-
}
211+
hpr.upsertRoute(r, l)
212+
}
213+
}
222214

223-
if _, exist := hpr.rulesPerHost[h]; !exist {
224-
hpr.rulesPerHost[h] = make(map[pathAndType]PathRule)
225-
}
215+
func (hpr *hostPathRules) upsertRoute(route *graph.Route, listener *graph.Listener) {
216+
var hostnames []string
217+
for _, p := range route.ParentRefs {
218+
if val, exist := p.Attachment.AcceptedHostnames[string(listener.Source.Name)]; exist {
219+
hostnames = val
226220
}
221+
}
227222

228-
for i, rule := range r.Source.Spec.Rules {
229-
if !r.Rules[i].ValidMatches {
230-
continue
223+
for _, h := range hostnames {
224+
if prevListener, exists := hpr.listenersForHost[h]; exists {
225+
// override the previous listener if the new one has a more specific hostname
226+
if listenerHostnameMoreSpecific(listener.Source.Hostname, prevListener.Source.Hostname) {
227+
hpr.listenersForHost[h] = listener
231228
}
229+
} else {
230+
hpr.listenersForHost[h] = listener
231+
}
232232

233-
var filters HTTPFilters
234-
if r.Rules[i].ValidFilters {
235-
filters = createHTTPFilters(rule.Filters)
236-
} else {
237-
filters = HTTPFilters{
238-
InvalidFilter: &InvalidHTTPFilter{},
239-
}
233+
if _, exist := hpr.rulesPerHost[h]; !exist {
234+
hpr.rulesPerHost[h] = make(map[pathAndType]PathRule)
235+
}
236+
}
237+
238+
for i, rule := range route.Source.Spec.Rules {
239+
if !route.Rules[i].ValidMatches {
240+
continue
241+
}
242+
243+
var filters HTTPFilters
244+
if route.Rules[i].ValidFilters {
245+
filters = createHTTPFilters(rule.Filters)
246+
} else {
247+
filters = HTTPFilters{
248+
InvalidFilter: &InvalidHTTPFilter{},
240249
}
250+
}
241251

242-
for _, h := range hostnames {
243-
for _, m := range rule.Matches {
244-
path := getPath(m.Path)
252+
for _, h := range hostnames {
253+
for _, m := range rule.Matches {
254+
path := getPath(m.Path)
245255

246-
key := pathAndType{
247-
path: path,
248-
pathType: *m.Path.Type,
249-
}
256+
key := pathAndType{
257+
path: path,
258+
pathType: *m.Path.Type,
259+
}
250260

251-
rule, exist := hpr.rulesPerHost[h][key]
252-
if !exist {
253-
rule.Path = path
254-
rule.PathType = convertPathType(*m.Path.Type)
255-
}
261+
rule, exist := hpr.rulesPerHost[h][key]
262+
if !exist {
263+
rule.Path = path
264+
rule.PathType = convertPathType(*m.Path.Type)
265+
}
256266

257-
// create iteration variable inside the loop to fix implicit memory aliasing
258-
om := r.Source.ObjectMeta
267+
// create iteration variable inside the loop to fix implicit memory aliasing
268+
om := route.Source.ObjectMeta
259269

260-
rule.MatchRules = append(rule.MatchRules, MatchRule{
261-
Source: &om,
262-
BackendGroup: newBackendGroup(r.Rules[i].BackendRefs, routeNsName, i),
263-
Filters: filters,
264-
Match: convertMatch(m),
265-
})
270+
routeNsName := client.ObjectKeyFromObject(route.Source)
266271

267-
hpr.rulesPerHost[h][key] = rule
268-
}
272+
rule.MatchRules = append(rule.MatchRules, MatchRule{
273+
Source: &om,
274+
BackendGroup: newBackendGroup(route.Rules[i].BackendRefs, routeNsName, i),
275+
Filters: filters,
276+
Match: convertMatch(m),
277+
})
278+
279+
hpr.rulesPerHost[h][key] = rule
269280
}
270281
}
271282
}
@@ -371,6 +382,10 @@ func buildUpstreams(
371382
}
372383

373384
for _, route := range l.Routes {
385+
if !route.Valid {
386+
continue
387+
}
388+
374389
for _, rule := range route.Rules {
375390
if !rule.ValidMatches || !rule.ValidFilters {
376391
// don't generate upstreams for rules that have invalid matches or filters

0 commit comments

Comments
 (0)