Skip to content

Commit 56e43d6

Browse files
sjbermansjberman
authored
Allow empty sectionName in HTTPRoute parentRef (#626)
* Allow empty sectionName in HTTPRoute parentRef Support for empty sectionName in HTTPRoute parentRef. This will bind the route to all listeners with matching hostnames. If multiple listeners match, we'll choose the most specific one (mainly for TLS case to ensure proper certs are used). Either way, we should end up with a single nginx server for the hostname. We could end up with duplicate match rules if multiple listeners match, but nginx/njs will send traffic properly. At some point we'll need to figure out how to de-dupe these.
1 parent d62cf56 commit 56e43d6

File tree

7 files changed

+250
-48
lines changed

7 files changed

+250
-48
lines changed

‎docs/gateway-api-compatibility.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ Fields:
7979
8080
Fields:
8181
* `spec`
82-
* `parentRefs` - partially supported. `sectionName` must always be set.
82+
* `parentRefs` - partially supported. Port not supported.
8383
* `hostnames` - partially supported. Wildcard binding is not supported: a hostname like `example.com` will not bind to a listener with the hostname `*.example.com`. However, `example.com` will bind to a listener with the empty hostname.
8484
* `rules`
8585
* `matches`

‎examples/advanced-routing/cafe-routes.yaml‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ metadata:
55
spec:
66
parentRefs:
77
- name: gateway
8-
sectionName: http
98
hostnames:
109
- "cafe.example.com"
1110
rules:
@@ -40,7 +39,6 @@ metadata:
4039
spec:
4140
parentRefs:
4241
- name: gateway
43-
sectionName: http
4442
hostnames:
4543
- "cafe.example.com"
4644
rules:

‎internal/nginx/config/servers.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,8 @@ func createLocations(pathRules []dataplane.PathRule, listenerPort int) []http.Lo
144144
}
145145

146146
if len(matches) > 0 {
147+
// FIXME(sberman): De-dupe matches and associated locations
148+
// so we don't need nginx/njs to perform unnecessary matching.
147149
b, err := json.Marshal(matches)
148150
if err != nil {
149151
// panic is safe here because we should never fail to marshal the match unless we constructed it incorrectly.

‎internal/state/dataplane/configuration.go‎

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,14 @@ func (hpr *hostPathRules) upsertListener(l *graph.Listener) {
230230
}
231231

232232
for _, h := range hostnames {
233-
hpr.listenersForHost[h] = l
233+
if prevListener, exists := hpr.listenersForHost[h]; exists {
234+
// override the previous listener if the new one has a more specific hostname
235+
if listenerHostnameMoreSpecific(l.Source.Hostname, prevListener.Source.Hostname) {
236+
hpr.listenersForHost[h] = l
237+
}
238+
} else {
239+
hpr.listenersForHost[h] = l
240+
}
234241

235242
if _, exist := hpr.rulesPerHost[h]; !exist {
236243
hpr.rulesPerHost[h] = make(map[pathAndType]PathRule)
@@ -319,7 +326,8 @@ func (hpr *hostPathRules) buildServers() []VirtualServer {
319326

320327
for _, l := range hpr.httpsListeners {
321328
hostname := getListenerHostname(l.Source.Hostname)
322-
// generate a 404 ssl server block for listeners with no routes or listeners with wildcard (match-all) routes
329+
// Generate a 404 ssl server block for listeners with no routes or listeners with wildcard (match-all) routes.
330+
// This server overrides the default ssl server.
323331
// FIXME(kate-osborn): when we support regex hostnames (e.g. *.example.com)
324332
// we will have to modify this check to catch regex hostnames.
325333
if len(l.Routes) == 0 || hostname == wildcardHostname {
@@ -434,3 +442,30 @@ func convertPathType(pathType v1beta1.PathMatchType) PathType {
434442
panic(fmt.Sprintf("unsupported path type: %s", pathType))
435443
}
436444
}
445+
446+
// listenerHostnameMoreSpecific returns true if host1 is more specific than host2 (using length).
447+
//
448+
// FIXME(sberman): Since the only caller of this function specifies listener hostnames that are both
449+
// bound to the same route hostname, this function assumes that host1 and host2 match, either
450+
// exactly or as a substring.
451+
//
452+
// For example:
453+
// - foo.example.com and "" (host1 wins)
454+
// Non-example:
455+
// - foo.example.com and bar.example.com (should not be given to this function)
456+
//
457+
// As we add regex support, we should put in the proper
458+
// validation and error handling for this function to ensure that the hostnames are actually matching,
459+
// to avoid the unintended inputs above for the invalid case.
460+
func listenerHostnameMoreSpecific(host1, host2 *v1beta1.Hostname) bool {
461+
var host1Str, host2Str string
462+
if host1 != nil {
463+
host1Str = string(*host1)
464+
}
465+
466+
if host2 != nil {
467+
host2Str = string(*host2)
468+
}
469+
470+
return len(host1Str) >= len(host2Str)
471+
}

‎internal/state/dataplane/configuration_test.go‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1057,6 +1057,86 @@ func TestBuildConfiguration(t *testing.T) {
10571057
},
10581058
msg: "duplicate paths with different types",
10591059
},
1060+
{
1061+
graph: &graph.Graph{
1062+
GatewayClass: &graph.GatewayClass{
1063+
Source: &v1beta1.GatewayClass{},
1064+
Valid: true,
1065+
},
1066+
Gateway: &graph.Gateway{
1067+
Source: &v1beta1.Gateway{},
1068+
Listeners: map[string]*graph.Listener{
1069+
"listener-443-with-hostname": {
1070+
Source: listener443WithHostname,
1071+
Valid: true,
1072+
SecretPath: "secret-path-https-listener-2",
1073+
Routes: map[types.NamespacedName]*graph.Route{
1074+
{Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5,
1075+
},
1076+
AcceptedHostnames: map[string]struct{}{
1077+
"example.com": {},
1078+
},
1079+
},
1080+
"listener-443-1": {
1081+
Source: listener443,
1082+
Valid: true,
1083+
SecretPath: secretPath,
1084+
Routes: map[types.NamespacedName]*graph.Route{
1085+
{Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5,
1086+
},
1087+
AcceptedHostnames: map[string]struct{}{
1088+
"example.com": {},
1089+
},
1090+
},
1091+
},
1092+
},
1093+
Routes: map[types.NamespacedName]*graph.Route{
1094+
{Namespace: "test", Name: "https-hr-5"}: httpsRouteHR5,
1095+
},
1096+
},
1097+
expConf: Configuration{
1098+
HTTPServers: []VirtualServer{},
1099+
SSLServers: []VirtualServer{
1100+
{
1101+
IsDefault: true,
1102+
},
1103+
{
1104+
Hostname: "example.com",
1105+
PathRules: []PathRule{
1106+
{
1107+
Path: "/",
1108+
PathType: PathTypePrefix,
1109+
MatchRules: []MatchRule{
1110+
// duplicate match rules since two listeners both match this route's hostname
1111+
{
1112+
MatchIdx: 0,
1113+
RuleIdx: 0,
1114+
BackendGroup: httpsHR5Groups[0],
1115+
Source: httpsHR5,
1116+
},
1117+
{
1118+
MatchIdx: 0,
1119+
RuleIdx: 0,
1120+
BackendGroup: httpsHR5Groups[0],
1121+
Source: httpsHR5,
1122+
},
1123+
},
1124+
},
1125+
},
1126+
SSL: &SSL{
1127+
CertificatePath: "secret-path-https-listener-2",
1128+
},
1129+
},
1130+
{
1131+
Hostname: wildcardHostname,
1132+
SSL: &SSL{CertificatePath: secretPath},
1133+
},
1134+
},
1135+
Upstreams: []Upstream{fooUpstream},
1136+
BackendGroups: []graph.BackendGroup{httpsHR5Groups[0]},
1137+
},
1138+
msg: "two https listeners with different hostnames but same route; chooses listener with more specific hostname",
1139+
},
10601140
}
10611141

10621142
for _, test := range tests {
@@ -1626,3 +1706,49 @@ func TestConvertPathType(t *testing.T) {
16261706
}
16271707
}
16281708
}
1709+
1710+
func TestHostnameMoreSpecific(t *testing.T) {
1711+
g := NewGomegaWithT(t)
1712+
1713+
tests := []struct {
1714+
host1 *v1beta1.Hostname
1715+
host2 *v1beta1.Hostname
1716+
msg string
1717+
host1Wins bool
1718+
}{
1719+
{
1720+
host1: nil,
1721+
host2: helpers.GetPointer(v1beta1.Hostname("")),
1722+
host1Wins: true,
1723+
msg: "host1 nil; host2 empty",
1724+
},
1725+
{
1726+
host1: helpers.GetPointer(v1beta1.Hostname("")),
1727+
host2: nil,
1728+
host1Wins: true,
1729+
msg: "host1 empty; host2 nil",
1730+
},
1731+
{
1732+
host1: helpers.GetPointer(v1beta1.Hostname("")),
1733+
host2: helpers.GetPointer(v1beta1.Hostname("")),
1734+
host1Wins: true,
1735+
msg: "both hosts empty",
1736+
},
1737+
{
1738+
host1: helpers.GetPointer(v1beta1.Hostname("example.com")),
1739+
host2: helpers.GetPointer(v1beta1.Hostname("")),
1740+
host1Wins: true,
1741+
msg: "host1 has value; host2 empty",
1742+
},
1743+
{
1744+
host1: helpers.GetPointer(v1beta1.Hostname("example.com")),
1745+
host2: helpers.GetPointer(v1beta1.Hostname("foo.example.com")),
1746+
host1Wins: false,
1747+
msg: "host2 longer than host1",
1748+
},
1749+
}
1750+
1751+
for _, tc := range tests {
1752+
g.Expect(listenerHostnameMoreSpecific(tc.host1, tc.host2)).To(Equal(tc.host1Wins), tc.msg)
1753+
}
1754+
}

‎internal/state/graph/httproute.go‎

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -298,12 +298,6 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
298298

299299
// Case 1: Attachment is not possible due to unsupported configuration
300300

301-
if routeRef.SectionName == nil || *routeRef.SectionName == "" {
302-
valErr := field.Required(path.Child("sectionName"), "cannot be empty")
303-
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
304-
continue
305-
}
306-
307301
if routeRef.Port != nil {
308302
valErr := field.Forbidden(path.Child("port"), "cannot be set")
309303
attachment.FailedCondition = conditions.NewRouteUnsupportedValue(valErr.Error())
@@ -326,40 +320,49 @@ func bindRouteToListeners(r *Route, gw *Gateway) {
326320
// FIXME(pleshakov)
327321
// For now, let's do simple matching.
328322
// However, we need to also support wildcard matching.
329-
// More over, we need to handle cases when a Route host matches multiple HTTP listeners on the same port
330-
// when sectionName is empty and only choose one listener.
331-
// For example:
332-
// - Route with host foo.example.com;
333-
// - listener 1 for port 80 with hostname foo.example.com
334-
// - listener 2 for port 80 with hostname *.example.com;
335-
// In this case, the Route host foo.example.com should choose listener 1, as it is a more specific match.
336-
337-
l, exists := gw.Listeners[string(*routeRef.SectionName)]
338-
if !exists {
339-
// FIXME(pleshakov): Add a proper condition once it is available in the Gateway API.
340-
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306
341-
attachment.FailedCondition = conditions.NewTODO("listener is not found")
342-
continue
323+
324+
bind := func(l *Listener) (valid bool) {
325+
if !l.Valid {
326+
return false
327+
}
328+
329+
hostnames := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames)
330+
if len(hostnames) == 0 {
331+
return true // listener is valid, but return without attaching due to no matching hostnames
332+
}
333+
334+
attachment.Attached = true
335+
for _, h := range hostnames {
336+
l.AcceptedHostnames[h] = struct{}{}
337+
}
338+
l.Routes[client.ObjectKeyFromObject(r.Source)] = r
339+
340+
return true
343341
}
344342

345-
if !l.Valid {
343+
var validListener bool
344+
if getSectionName(routeRef.SectionName) == "" {
345+
for _, l := range gw.Listeners {
346+
validListener = bind(l) || validListener
347+
}
348+
} else {
349+
l, exists := gw.Listeners[string(*routeRef.SectionName)]
350+
if !exists {
351+
// FIXME(pleshakov): Add a proper condition once it is available in the Gateway API.
352+
// https://github.com/nginxinc/nginx-kubernetes-gateway/issues/306
353+
attachment.FailedCondition = conditions.NewTODO("listener is not found")
354+
continue
355+
}
356+
357+
validListener = bind(l)
358+
}
359+
if !validListener {
346360
attachment.FailedCondition = conditions.NewRouteInvalidListener()
347361
continue
348362
}
349-
350-
accepted := findAcceptedHostnames(l.Source.Hostname, r.Source.Spec.Hostnames)
351-
352-
if len(accepted) == 0 {
363+
if !attachment.Attached {
353364
attachment.FailedCondition = conditions.NewRouteNoMatchingListenerHostname()
354-
continue
355365
}
356-
357-
attachment.Attached = true
358-
359-
for _, h := range accepted {
360-
l.AcceptedHostnames[h] = struct{}{}
361-
}
362-
l.Routes[client.ObjectKeyFromObject(r.Source)] = r
363366
}
364367
}
365368

@@ -391,6 +394,13 @@ func getHostname(h *v1beta1.Hostname) string {
391394
return string(*h)
392395
}
393396

397+
func getSectionName(s *v1beta1.SectionName) string {
398+
if s == nil {
399+
return ""
400+
}
401+
return string(*s)
402+
}
403+
394404
func validateHostnames(hostnames []v1beta1.Hostname, path *field.Path) error {
395405
var allErrs field.ErrorList
396406

0 commit comments

Comments
 (0)