Skip to content

Commit

Permalink
Fix attaching of HTTPRoutes to Gateway Listeners (#1275)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@pleshakov @sjberman
pleshakov and sjberman authored Nov 27, 2023
1 parent fdbe668 commit 7ad0887
Show file tree
Hide file tree
Showing 9 changed files with 543 additions and 209 deletions.
56 changes: 34 additions & 22 deletions internal/mode/static/state/change_processor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,8 @@ var _ = Describe("ChangeProcessor", func() {
ValidFilters: true,
},
},
Valid: true,
Valid: true,
Attachable: true,
Conditions: []conditions.Condition{
staticConds.NewRouteBackendRefRefBackendNotFound(
"spec.rules[0].backendRefs[0].name: Not found: \"service\"",
Expand All @@ -434,8 +435,9 @@ var _ = Describe("ChangeProcessor", func() {
Idx: 1,
},
},
Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}},
Valid: true,
Rules: []graph.Rule{{ValidMatches: true, ValidFilters: true}},
Valid: true,
Attachable: true,
}

// This is the base case expected graph. Tests will manipulate this to add or remove elements
Expand All @@ -449,16 +451,18 @@ var _ = Describe("ChangeProcessor", func() {
Source: gw1,
Listeners: map[string]*graph.Listener{
"listener-80-1": {
Source: gw1.Spec.Listeners[0],
Valid: true,
Source: gw1.Spec.Listeners[0],
Valid: true,
Attachable: true,
Routes: map[types.NamespacedName]*graph.Route{
{Namespace: "test", Name: "hr-1"}: expRouteHR1,
},
SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}},
},
"listener-443-1": {
Source: gw1.Spec.Listeners[1],
Valid: true,
Source: gw1.Spec.Listeners[1],
Valid: true,
Attachable: true,
Routes: map[types.NamespacedName]*graph.Route{
{Namespace: "test", Name: "hr-1"}: expRouteHR1,
},
Expand Down Expand Up @@ -541,32 +545,40 @@ var _ = Describe("ChangeProcessor", func() {
It("returns updated graph", func() {
processor.CaptureUpsertChange(gc)

// no ref grant exists yet for gw1
expGraph.Gateway.Listeners["listener-443-1"] = &graph.Listener{
Source: gw1.Spec.Listeners[1],
Valid: false,
Routes: map[types.NamespacedName]*graph.Route{},
Conditions: staticConds.NewListenerRefNotPermitted(
"Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant",
),
SupportedKinds: []v1.RouteGroupKind{{Kind: "HTTPRoute"}},
// No ref grant exists yet for gw1
// so the listener is not valid, but still attachable
expGraph.Gateway.Listeners["listener-443-1"].Valid = false
expGraph.Gateway.Listeners["listener-443-1"].ResolvedSecret = nil
expGraph.Gateway.Listeners["listener-443-1"].Conditions = staticConds.NewListenerRefNotPermitted(
"Certificate ref to secret cert-ns/different-ns-tls-secret not permitted by any ReferenceGrant",
)

expAttachment80 := &graph.ParentRefAttachmentStatus{
AcceptedHostnames: map[string][]string{
"listener-80-1": {"foo.example.com"},
},
Attached: true,
}

expAttachment := &graph.ParentRefAttachmentStatus{
AcceptedHostnames: map[string][]string{},
FailedCondition: staticConds.NewRouteInvalidListener(),
Attached: false,
expAttachment443 := &graph.ParentRefAttachmentStatus{
AcceptedHostnames: map[string][]string{
"listener-443-1": {"foo.example.com"},
},
Attached: true,
}

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

// no ref grant exists yet for hr1
expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment
expGraph.Routes[hr1Name].Conditions = []conditions.Condition{
staticConds.NewRouteInvalidListener(),
staticConds.NewRouteBackendRefRefNotPermitted(
"Backend ref to Service service-ns/service not permitted by any ReferenceGrant",
),
}
expGraph.Routes[hr1Name].ParentRefs[0].Attachment = expAttachment80
expGraph.Routes[hr1Name].ParentRefs[1].Attachment = expAttachment443

expGraph.ReferencedSecrets = nil

Expand Down
115 changes: 65 additions & 50 deletions internal/mode/static/state/dataplane/configuration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (

apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
v1 "sigs.k8s.io/gateway-api/apis/v1"

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

for routeNsName, r := range l.Routes {
var hostnames []string
for _, p := range r.ParentRefs {
if val, exist := p.Attachment.AcceptedHostnames[string(l.Source.Name)]; exist {
hostnames = val
}
for _, r := range l.Routes {
if !r.Valid {
continue
}

for _, h := range hostnames {
if prevListener, exists := hpr.listenersForHost[h]; exists {
// override the previous listener if the new one has a more specific hostname
if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) {
hpr.listenersForHost[h] = l
}
} else {
hpr.listenersForHost[h] = l
}
hpr.upsertRoute(r, l)
}
}

if _, exist := hpr.rulesPerHost[h]; !exist {
hpr.rulesPerHost[h] = make(map[pathAndType]PathRule)
}
func (hpr *hostPathRules) upsertRoute(route *graph.Route, listener *graph.Listener) {
var hostnames []string
for _, p := range route.ParentRefs {
if val, exist := p.Attachment.AcceptedHostnames[string(listener.Source.Name)]; exist {
hostnames = val
}
}

for i, rule := range r.Source.Spec.Rules {
if !r.Rules[i].ValidMatches {
continue
for _, h := range hostnames {
if prevListener, exists := hpr.listenersForHost[h]; exists {
// override the previous listener if the new one has a more specific hostname
if listenerHostnameMoreSpecific(listener.Source.Hostname, prevListener.Source.Hostname) {
hpr.listenersForHost[h] = listener
}
} else {
hpr.listenersForHost[h] = listener
}

var filters HTTPFilters
if r.Rules[i].ValidFilters {
filters = createHTTPFilters(rule.Filters)
} else {
filters = HTTPFilters{
InvalidFilter: &InvalidHTTPFilter{},
}
if _, exist := hpr.rulesPerHost[h]; !exist {
hpr.rulesPerHost[h] = make(map[pathAndType]PathRule)
}
}

for i, rule := range route.Source.Spec.Rules {
if !route.Rules[i].ValidMatches {
continue
}

var filters HTTPFilters
if route.Rules[i].ValidFilters {
filters = createHTTPFilters(rule.Filters)
} else {
filters = HTTPFilters{
InvalidFilter: &InvalidHTTPFilter{},
}
}

for _, h := range hostnames {
for _, m := range rule.Matches {
path := getPath(m.Path)
for _, h := range hostnames {
for _, m := range rule.Matches {
path := getPath(m.Path)

key := pathAndType{
path: path,
pathType: *m.Path.Type,
}
key := pathAndType{
path: path,
pathType: *m.Path.Type,
}

rule, exist := hpr.rulesPerHost[h][key]
if !exist {
rule.Path = path
rule.PathType = convertPathType(*m.Path.Type)
}
rule, exist := hpr.rulesPerHost[h][key]
if !exist {
rule.Path = path
rule.PathType = convertPathType(*m.Path.Type)
}

// create iteration variable inside the loop to fix implicit memory aliasing
om := r.Source.ObjectMeta
// create iteration variable inside the loop to fix implicit memory aliasing
om := route.Source.ObjectMeta

rule.MatchRules = append(rule.MatchRules, MatchRule{
Source: &om,
BackendGroup: newBackendGroup(r.Rules[i].BackendRefs, routeNsName, i),
Filters: filters,
Match: convertMatch(m),
})
routeNsName := client.ObjectKeyFromObject(route.Source)

hpr.rulesPerHost[h][key] = rule
}
rule.MatchRules = append(rule.MatchRules, MatchRule{
Source: &om,
BackendGroup: newBackendGroup(route.Rules[i].BackendRefs, routeNsName, i),
Filters: filters,
Match: convertMatch(m),
})

hpr.rulesPerHost[h][key] = rule
}
}
}
Expand Down Expand Up @@ -371,6 +382,10 @@ func buildUpstreams(
}

for _, route := range l.Routes {
if !route.Valid {
continue
}

for _, rule := range route.Rules {
if !rule.ValidMatches || !rule.ValidFilters {
// don't generate upstreams for rules that have invalid matches or filters
Expand Down
Loading

0 comments on commit 7ad0887

Please sign in to comment.