Skip to content

Commit bd4ac11

Browse files
sarthypartysarthyparty
committed
fix reintroduced bug
1 parent 32dd41d commit bd4ac11

File tree

3 files changed

+202
-22
lines changed

3 files changed

+202
-22
lines changed

‎internal/controller/state/graph/policies.go

Lines changed: 77 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -455,33 +455,90 @@ func checkTargetRoutesForOverlap(
455455
// as a route referenced in a policy.
456456
func checkForRouteOverlap(route *L7Route, hostPortPaths map[string]string) *conditions.Condition {
457457
for _, parentRef := range route.ParentRefs {
458-
if parentRef.Attachment != nil {
459-
port := parentRef.Attachment.ListenerPort
460-
for _, hostname := range parentRef.Attachment.AcceptedHostnames {
461-
for _, rule := range route.Spec.Rules {
462-
for _, match := range rule.Matches {
463-
if match.Path != nil && match.Path.Value != nil {
464-
key := fmt.Sprintf("%s:%d%s", hostname, port, *match.Path.Value)
465-
if val, ok := hostPortPaths[key]; !ok {
466-
hostPortPaths[key] = fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName())
467-
} else {
468-
conflictingRouteName := fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName())
469-
msg := fmt.Sprintf("Policy cannot be applied to target %q since another "+
470-
"Route %q shares a hostname:port/path combination with this target", val, conflictingRouteName)
471-
cond := conditions.NewPolicyNotAcceptedTargetConflict(msg)
472-
473-
return &cond
474-
}
475-
}
476-
}
477-
}
458+
if parentRef.Attachment == nil {
459+
continue
460+
}
461+
462+
port := parentRef.Attachment.ListenerPort
463+
for _, hostnames := range parentRef.Attachment.AcceptedHostnames {
464+
mergedHostnames := getMergedHostnames(route, hostnames)
465+
if cond := checkHostnamesOverlap(route, mergedHostnames, port, hostPortPaths); cond != nil {
466+
return cond
478467
}
479468
}
480469
}
470+
return nil
471+
}
472+
473+
func getMergedHostnames(route *L7Route, hostnames []string) []string {
474+
mergedHostnames := make([]string, 0)
475+
for _, hostname := range hostnames {
476+
if len(route.Spec.Hostnames) > 0 {
477+
mergedHostnames = append(mergedHostnames, getHostnameMatches(hostname, route.Spec.Hostnames)...)
478+
} else {
479+
mergedHostnames = append(mergedHostnames, hostname)
480+
}
481+
}
482+
return mergedHostnames
483+
}
484+
485+
func getHostnameMatches(hostname string, routeHostnames []v1.Hostname) []string {
486+
foundExactMatch := false
487+
moreSpecificHostnames := make([]string, 0)
488+
489+
for _, h := range routeHostnames {
490+
if hostname == string(h) {
491+
foundExactMatch = true
492+
break
493+
}
494+
if Match(hostname, string(h)) {
495+
moreSpecificHostnames = append(moreSpecificHostnames, GetMoreSpecificHostname(hostname, string(h)))
496+
}
497+
}
481498

499+
if foundExactMatch {
500+
return []string{}
501+
}
502+
return moreSpecificHostnames
503+
}
504+
505+
func checkHostnamesOverlap(
506+
route *L7Route, hostnames []string, port v1.PortNumber, hostPortPaths map[string]string,
507+
) *conditions.Condition {
508+
for _, hostname := range hostnames {
509+
if cond := checkHostnamePathsOverlap(route, hostname, port, hostPortPaths); cond != nil {
510+
return cond
511+
}
512+
}
482513
return nil
483514
}
484515

516+
func checkHostnamePathsOverlap(
517+
route *L7Route, hostname string, port v1.PortNumber, hostPortPaths map[string]string,
518+
) *conditions.Condition {
519+
for _, rule := range route.Spec.Rules {
520+
for _, match := range rule.Matches {
521+
if match.Path != nil && match.Path.Value != nil {
522+
key := fmt.Sprintf("%s:%d%s", hostname, port, *match.Path.Value)
523+
if val, ok := hostPortPaths[key]; !ok {
524+
hostPortPaths[key] = fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName())
525+
} else {
526+
return createTargetConflictCondition(route, val)
527+
}
528+
}
529+
}
530+
}
531+
return nil
532+
}
533+
534+
func createTargetConflictCondition(route *L7Route, conflictingRoute string) *conditions.Condition {
535+
conflictingRouteName := fmt.Sprintf("%s/%s", route.Source.GetNamespace(), route.Source.GetName())
536+
msg := fmt.Sprintf("Policy cannot be applied to target %q since another "+
537+
"Route %q shares a hostname:port/path combination with this target", conflictingRoute, conflictingRouteName)
538+
cond := conditions.NewPolicyNotAcceptedTargetConflict(msg)
539+
return &cond
540+
}
541+
485542
// buildHostPortPaths uses the same logic as checkForRouteOverlap, except it's
486543
// simply initializing the hostPortPaths map with the route that's referenced in the Policy,
487544
// so it doesn't care about the return value.

‎internal/controller/state/graph/policies_test.go

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,6 +1297,43 @@ func TestProcessPolicies_RouteOverlap(t *testing.T) {
12971297
},
12981298
},
12991299
},
1300+
{
1301+
name: "no overlap different hostnames (hostname slice bug regression test)",
1302+
validator: &policiesfakes.FakeValidator{},
1303+
policies: map[PolicyKey]policies.Policy{
1304+
pol1Key: pol1,
1305+
pol3Key: pol3,
1306+
},
1307+
routes: map[RouteKey]*L7Route{
1308+
{
1309+
RouteType: RouteTypeHTTP,
1310+
NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"},
1311+
}: createTestRouteWithPathsAndHostname("hr-coffee", "app1.playground.testapis.net", "/"),
1312+
{
1313+
RouteType: RouteTypeHTTP,
1314+
NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"},
1315+
}: createTestRouteWithPathsAndHostname("hr-coffee-tea", "app2.playground.testapis.net", "/"),
1316+
},
1317+
valid: true,
1318+
},
1319+
{
1320+
name: "no overlap wildcard listener with specific route hostnames",
1321+
validator: &policiesfakes.FakeValidator{},
1322+
policies: map[PolicyKey]policies.Policy{
1323+
pol1Key: pol1, // Policy targeting first route only
1324+
},
1325+
routes: map[RouteKey]*L7Route{
1326+
{
1327+
RouteType: RouteTypeHTTP,
1328+
NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee"},
1329+
}: createTestRouteWithWildcardListener("hr-coffee", "*.example.com", []string{"api.example.com"}, "/"),
1330+
{
1331+
RouteType: RouteTypeHTTP,
1332+
NamespacedName: types.NamespacedName{Namespace: testNs, Name: "hr-coffee-tea"},
1333+
}: createTestRouteWithWildcardListener("hr-coffee-tea", "*.example.com", []string{"web.example.com"}, "/"),
1334+
},
1335+
valid: true,
1336+
},
13001337
}
13011338

13021339
gateways := map[types.NamespacedName]*Gateway{
@@ -1630,6 +1667,92 @@ func createTestRouteWithPaths(name string, paths ...string) *L7Route {
16301667
return route
16311668
}
16321669

1670+
func createTestRouteWithPathsAndHostname(name string, hostname string, paths ...string) *L7Route {
1671+
routeMatches := make([]v1.HTTPRouteMatch, 0, len(paths))
1672+
1673+
for _, path := range paths {
1674+
routeMatches = append(routeMatches, v1.HTTPRouteMatch{
1675+
Path: &v1.HTTPPathMatch{
1676+
Type: helpers.GetPointer(v1.PathMatchExact),
1677+
Value: helpers.GetPointer(path),
1678+
},
1679+
})
1680+
}
1681+
1682+
route := &L7Route{
1683+
Source: &v1.HTTPRoute{
1684+
ObjectMeta: metav1.ObjectMeta{
1685+
Name: name,
1686+
Namespace: testNs,
1687+
},
1688+
},
1689+
Spec: L7RouteSpec{
1690+
Rules: []RouteRule{
1691+
{Matches: routeMatches},
1692+
},
1693+
},
1694+
ParentRefs: []ParentRef{
1695+
{
1696+
Attachment: &ParentRefAttachmentStatus{
1697+
AcceptedHostnames: map[string][]string{"listener-1": {hostname}},
1698+
ListenerPort: 80,
1699+
},
1700+
},
1701+
},
1702+
}
1703+
1704+
return route
1705+
}
1706+
1707+
func createTestRouteWithWildcardListener(
1708+
name string,
1709+
listenerHostname string,
1710+
routeHostnames []string,
1711+
paths ...string,
1712+
) *L7Route {
1713+
routeMatches := make([]v1.HTTPRouteMatch, 0, len(paths))
1714+
1715+
for _, path := range paths {
1716+
routeMatches = append(routeMatches, v1.HTTPRouteMatch{
1717+
Path: &v1.HTTPPathMatch{
1718+
Type: helpers.GetPointer(v1.PathMatchExact),
1719+
Value: helpers.GetPointer(path),
1720+
},
1721+
})
1722+
}
1723+
1724+
// Convert string slice to v1.Hostname slice
1725+
specHostnames := make([]v1.Hostname, 0, len(routeHostnames))
1726+
for _, h := range routeHostnames {
1727+
specHostnames = append(specHostnames, v1.Hostname(h))
1728+
}
1729+
1730+
route := &L7Route{
1731+
Source: &v1.HTTPRoute{
1732+
ObjectMeta: metav1.ObjectMeta{
1733+
Name: name,
1734+
Namespace: testNs,
1735+
},
1736+
},
1737+
Spec: L7RouteSpec{
1738+
Hostnames: specHostnames,
1739+
Rules: []RouteRule{
1740+
{Matches: routeMatches},
1741+
},
1742+
},
1743+
ParentRefs: []ParentRef{
1744+
{
1745+
Attachment: &ParentRefAttachmentStatus{
1746+
AcceptedHostnames: map[string][]string{"listener-1": {listenerHostname}},
1747+
ListenerPort: 80,
1748+
},
1749+
},
1750+
},
1751+
}
1752+
1753+
return route
1754+
}
1755+
16331756
func getGatewayParentRef(gwNsName types.NamespacedName) v1.ParentReference {
16341757
return v1.ParentReference{
16351758
Group: helpers.GetPointer[v1.Group](v1.GroupName),

‎internal/controller/state/graph/route_common.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -926,15 +926,15 @@ func findAcceptedHostnames(listenerHostname *v1.Hostname, routeHostnames []v1.Ho
926926

927927
for _, h := range routeHostnames {
928928
routeHost := string(h)
929-
if match(hostname, routeHost) {
929+
if Match(hostname, routeHost) {
930930
result = append(result, GetMoreSpecificHostname(hostname, routeHost))
931931
}
932932
}
933933

934934
return result
935935
}
936936

937-
func match(listenerHost, routeHost string) bool {
937+
func Match(listenerHost, routeHost string) bool {
938938
if listenerHost == "" {
939939
return true
940940
}

0 commit comments

Comments
 (0)