-
Notifications
You must be signed in to change notification settings - Fork 139
Listener Isolation for hostnames occupied by other listeners #3061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -337,22 +337,103 @@ func bindRoutesToListeners( | |
bindL7RouteToListeners(r, gw, namespaces) | ||
} | ||
|
||
var routes []*L4Route | ||
l7routes := make([]*L7Route, 0, len(l7Routes)) | ||
for _, r := range l7Routes { | ||
l7routes = append(l7routes, r) | ||
} | ||
|
||
isolateL7RouteListeners(l7routes, gw.Listeners) | ||
|
||
l4routes := make([]*L4Route, 0, len(l4Routes)) | ||
for _, r := range l4Routes { | ||
routes = append(routes, r) | ||
l4routes = append(l4routes, r) | ||
} | ||
|
||
// Sort the slice by timestamp and name so that we process the routes in the priority order | ||
sort.Slice(routes, func(i, j int) bool { | ||
return ngfSort.LessClientObject(routes[i].Source, routes[j].Source) | ||
sort.Slice(l4routes, func(i, j int) bool { | ||
return ngfSort.LessClientObject(l4routes[i].Source, l4routes[j].Source) | ||
}) | ||
|
||
// portHostnamesMap exists to detect duplicate hostnames on the same port | ||
portHostnamesMap := make(map[string]struct{}) | ||
|
||
for _, r := range routes { | ||
for _, r := range l4routes { | ||
bindL4RouteToListeners(r, gw, namespaces, portHostnamesMap) | ||
} | ||
|
||
isolateL4RouteListeners(l4routes, gw.Listeners) | ||
} | ||
|
||
// isolateL7RouteListeners ensures listener isolation for all L7Routes. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both functions are almost identical. Combine them into a single generic function to improve maintainability. You can refactor the logic into a single function to reduce redundancy. |
||
func isolateL7RouteListeners(routes []*L7Route, listeners []*Listener) { | ||
listenerHostnameMap := make(map[string]string, len(listeners)) | ||
for _, l := range listeners { | ||
listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) | ||
} | ||
|
||
for _, route := range routes { | ||
isolateHostnamesForParentRefs(route.ParentRefs, listenerHostnameMap) | ||
} | ||
} | ||
|
||
// isolateL4RouteListeners ensures listener isolation for all L4Routes. | ||
func isolateL4RouteListeners(routes []*L4Route, listeners []*Listener) { | ||
listenerHostnameMap := make(map[string]string, len(listeners)) | ||
for _, l := range listeners { | ||
listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) | ||
} | ||
|
||
for _, route := range routes { | ||
isolateHostnamesForParentRefs(route.ParentRefs, listenerHostnameMap) | ||
} | ||
} | ||
|
||
// isolateHostnamesForParentRefs iterates through the parentRefs of a route to identify the list of accepted hostnames | ||
// for each listener. If any accepted hostname belongs to another listener, | ||
// it removes those hostnames to ensure listener isolation. | ||
func isolateHostnamesForParentRefs(parentRef []ParentRef, listenerHostnameMap map[string]string) { | ||
for _, ref := range parentRef { | ||
acceptedHostnames := ref.Attachment.AcceptedHostnames | ||
|
||
hostnamesToRemoves := make([]string, 0, len(acceptedHostnames)) | ||
for listenerName, hostnames := range acceptedHostnames { | ||
if len(hostnames) == 0 { | ||
continue | ||
} | ||
for _, h := range hostnames { | ||
for lName, lHostname := range listenerHostnameMap { | ||
// skip comparison if it is a catch all listener block | ||
if lHostname == "" { | ||
continue | ||
} | ||
if h == lHostname && listenerName != lName { | ||
hostnamesToRemoves = append(hostnamesToRemoves, h) | ||
} | ||
} | ||
} | ||
|
||
isolatedHostnames := removeHostnames(hostnames, hostnamesToRemoves) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should be able to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So when using slices.Delete(arr, i, j) --> we would have to specify the index which panics if out of bounds. And wouldn't deleting elements while looping the array lead to similar issues with indexing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you would be deleting while looping, you build the list first, then delete afterwards like you do now. |
||
ref.Attachment.AcceptedHostnames[listenerName] = isolatedHostnames | ||
} | ||
} | ||
} | ||
|
||
// removeHostnames removes the hostnames that are part of toRemove slice. | ||
func removeHostnames(hostnames []string, toRemove []string) []string { | ||
result := make([]string, 0, len(hostnames)) | ||
for _, h := range hostnames { | ||
keep := true | ||
for _, r := range toRemove { | ||
if h == r { | ||
keep = false | ||
break | ||
} | ||
} | ||
if keep { | ||
result = append(result, h) | ||
} | ||
} | ||
return result | ||
} | ||
|
||
func validateParentRef( | ||
|
@@ -617,6 +698,11 @@ func tryToAttachL7RouteToListeners( | |
|
||
rk := CreateRouteKey(route.Source) | ||
|
||
listenerHostnameMap := make(map[string]string, len(attachableListeners)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't used anywhere. |
||
for _, l := range attachableListeners { | ||
listenerHostnameMap[l.Name] = getHostname(l.Source.Hostname) | ||
} | ||
|
||
bind := func(l *Listener) (allowed, attached bool) { | ||
if !isRouteNamespaceAllowedByListener(l, route.Source.GetNamespace(), gw.Source.Namespace, namespaces) { | ||
return false, false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name the l7routes map something different? Using the same name here is confusing at first glance.