Skip to content

Commit

Permalink
Update the partially match condition
Browse files Browse the repository at this point in the history
Signed-off-by: Lubron Zhan <[email protected]>
  • Loading branch information
lubronzhan committed Mar 1, 2024
1 parent 1d048a9 commit 725ce9b
Show file tree
Hide file tree
Showing 7 changed files with 229 additions and 39 deletions.
5 changes: 3 additions & 2 deletions internal/dag/dag.go
Original file line number Diff line number Diff line change
Expand Up @@ -761,9 +761,10 @@ func (v *VirtualHost) AddRoute(route *Route) {
}

// HasConflictRoute returns true if there is existing Path + Headers
// + QueryParams combination match this route candidate.
// + QueryParams combination match this route candidate and also they are same kind of Route.
func (v *VirtualHost) HasConflictRoute(route *Route) bool {
if _, ok := v.Routes[conditionsToString(route)]; ok {
// If match exist and kind is the same kind, return true.
if r, ok := v.Routes[conditionsToString(route)]; ok && r.Kind == route.Kind {
return true
}
return false
Expand Down
67 changes: 63 additions & 4 deletions internal/dag/dag_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -272,12 +272,13 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
rs []Route
expectConflict bool
}{
"2 different routes with same path and header, expect conflict": {
"2 different routes with same path and header, with same kind, expect conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
rs: []Route{
{
Kind: KindHTTPRoute,
Name: "a",
Namespace: "b",
PathMatchCondition: prefixSegment("/path1"),
Expand All @@ -297,7 +298,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
},
expectConflict: true,
},
"2 different routes with same path and header and query params, expect conflict": {
"2 different routes with same path and header and query params, with same kind, expect conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
Expand Down Expand Up @@ -329,7 +330,65 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
},
expectConflict: true,
},
"2 different routes with same path and header, but different query params, don't expect conflict": {
"2 different routes with same path and header, but different kind, expect no conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
rs: []Route{
{
Kind: KindTCPRoute,
Name: "a",
Namespace: "b",
PathMatchCondition: prefixSegment("/path1"),
HeaderMatchConditions: []HeaderMatchCondition{
{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
},
},
{
Kind: KindHTTPRoute,
Name: "b",
Namespace: "c",
PathMatchCondition: prefixSegment("/path1"),
HeaderMatchConditions: []HeaderMatchCondition{
{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
},
},
},
expectConflict: false,
},
"2 different routes with same path and header and query params, but different kind, expect no conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
rs: []Route{
{
Kind: KindHTTPRoute,
Name: "a",
Namespace: "b",
PathMatchCondition: prefixSegment("/path1"),
HeaderMatchConditions: []HeaderMatchCondition{
{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
},
QueryParamMatchConditions: []QueryParamMatchCondition{
{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
},
},
{
Kind: KindTCPRoute,
Name: "c",
Namespace: "d",
PathMatchCondition: prefixSegment("/path1"),
HeaderMatchConditions: []HeaderMatchCondition{
{Name: ":authority", MatchType: HeaderMatchTypeRegex, Value: "^[a-z0-9]([-a-z0-9]*[a-z0-9])?\\.example\\.com(:[0-9]+)?"},
},
QueryParamMatchConditions: []QueryParamMatchCondition{
{Name: "param-1", Value: "value-1", MatchType: QueryParamMatchTypeExact},
},
},
},
expectConflict: false,
},
"2 different routes with same path and header, but different query params, with same kind, don't expect conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
Expand Down Expand Up @@ -361,7 +420,7 @@ func TestHasConflictRouteForVirtualHost(t *testing.T) {
},
expectConflict: false,
},
"2 different routes with different path, don't expect conflict": {
"2 different routes with different path, with same kind, don't expect conflict": {
vHost: VirtualHost{
Routes: map[string]*Route{},
},
Expand Down
61 changes: 36 additions & 25 deletions internal/dag/gatewayapi_processor.go
Original file line number Diff line number Diff line change
Expand Up @@ -1152,7 +1152,8 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
listener *listenerInfo,
hosts sets.Set[string],
) {
routes := []*Route{}
// Count number of rules under this Route that are invalid.
invalidRuleCnt := 0
for ruleIndex, rule := range route.Spec.Rules {
// Get match conditions for the rule.
var matchconditions []*matchConditions
Expand Down Expand Up @@ -1391,15 +1392,14 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
// the same priority.
priority := uint8(ruleIndex)

// Get our list of routes under current rule based on whether it's a redirect
// or a cluster-backed route.
// Get our list of routes based on whether it's a redirect or a cluster-backed route.
// Note that we can end up with multiple routes here since the match conditions are
// logically "OR"-ed, which we express as multiple routes, each with one of the
// conditions, all with the same action.
var routesPerRule []*Route
var routes []*Route

if redirect != nil {
routesPerRule = p.redirectRoutes(
routes = p.redirectRoutes(
matchconditions,
requestHeaderPolicy,
responseHeaderPolicy,
Expand All @@ -1415,7 +1415,7 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
if !ok {
continue
}
routesPerRule = p.clusterRoutes(
routes = p.clusterRoutes(
matchconditions,
clusters,
totalWeight,
Expand All @@ -1430,33 +1430,44 @@ func (p *GatewayAPIProcessor) computeHTTPRouteForListener(
timeoutPolicy)
}

routes = append(routes, routesPerRule...)
// check all the routes whether there is conflict against previous rules
if !p.hasConflictRoute(listener, hosts, routes) {
// add the route if there is conflict
// Add each route to the relevant vhost(s)/svhosts(s).
for host := range hosts {
for _, route := range routes {
switch {
case listener.tlsSecret != nil:
svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host)
svhost.Secret = listener.tlsSecret
svhost.AddRoute(route)
default:
vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
vhost.AddRoute(route)
}
}
}
} else {
// Skip adding the routes under this rule.
invalidRuleCnt++
}
}

// check all the routes at once in case there is conflict
if p.hasConflictRoute(listener, hosts, routes) {
// skip adding the route to svhost or vhost since it's marked as conflict route
if invalidRuleCnt == len(route.Spec.Rules) {
// No rules under the route is valid, mark it as not accepted.
routeAccessor.AddCondition(
gatewayapi_v1.RouteConditionAccepted,
meta_v1.ConditionFalse,
status.ReasonRouteRuleMatchConflict,
"HTTPRoute's Match has conflict with other HTTPRoute's Match",
)
} else {
// Add each route to the relevant vhost(s)/svhosts(s).
for host := range hosts {
for _, route := range routes {
switch {
case listener.tlsSecret != nil:
svhost := p.dag.EnsureSecureVirtualHost(listener.dagListenerName, host)
svhost.Secret = listener.tlsSecret
svhost.AddRoute(route)
default:
vhost := p.dag.EnsureVirtualHost(listener.dagListenerName, host)
vhost.AddRoute(route)
}
}
}
} else if invalidRuleCnt > 0 {
routeAccessor.AddCondition(
gatewayapi_v1.RouteConditionPartiallyInvalid,
meta_v1.ConditionTrue,
status.ReasonRouteRuleMatchPartiallyConflict,
"Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)",

Check failure on line 1469 in internal/dag/gatewayapi_processor.go

View workflow job for this annotation

GitHub Actions / Codespell

droped ==> dropped
)
}
}

Expand Down
13 changes: 7 additions & 6 deletions internal/status/routeconditions.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ const (
)

const (
ReasonDegraded gatewayapi_v1.RouteConditionReason = "Degraded"
ReasonAllBackendRefsHaveZeroWeights gatewayapi_v1.RouteConditionReason = "AllBackendRefsHaveZeroWeights"
ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch"
ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch"
ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway"
ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict"
ReasonDegraded gatewayapi_v1.RouteConditionReason = "Degraded"
ReasonAllBackendRefsHaveZeroWeights gatewayapi_v1.RouteConditionReason = "AllBackendRefsHaveZeroWeights"
ReasonInvalidPathMatch gatewayapi_v1.RouteConditionReason = "InvalidPathMatch"
ReasonInvalidMethodMatch gatewayapi_v1.RouteConditionReason = "InvalidMethodMatch"
ReasonInvalidGateway gatewayapi_v1.RouteConditionReason = "InvalidGateway"
ReasonRouteRuleMatchConflict gatewayapi_v1.RouteConditionReason = "RuleMatchConflict"
ReasonRouteRuleMatchPartiallyConflict gatewayapi_v1.RouteConditionReason = "RuleMatchPartiallyConflict"
)

// RouteStatusUpdate represents an atomic update to a
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/gateway/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,8 @@ var _ = Describe("Gateway API", func() {
f.NamespacedTest("gateway-backend-tls-policy", testWithHTTPGateway(testBackendTLSPolicy))

f.NamespacedTest("gateway-httproute-conflict-match", testWithHTTPGateway(testHTTPRouteConflictMatch))

f.NamespacedTest("gateway-httproute-partially-conflict-match", testWithHTTPGateway(testHTTPRoutePartiallyConflictMatch))
})

Describe("Gateway with one HTTP listener and one HTTPS listener", func() {
Expand Down
99 changes: 99 additions & 0 deletions test/e2e/gateway/http_route_partially_conflict_match_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
// Copyright Project Contour Authors
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//go:build e2e

package gateway

import (
. "github.com/onsi/ginkgo/v2"
meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
gatewayapi_v1 "sigs.k8s.io/gateway-api/apis/v1"

"github.com/projectcontour/contour/internal/gatewayapi"
"github.com/projectcontour/contour/test/e2e"
)

func testHTTPRoutePartiallyConflictMatch(namespace string, gateway types.NamespacedName) {
Specify("Creates two http routes, second one has conflict match condition as the first one", func() {
By("create httproute-1 first")
route1 := &gatewayapi_v1.HTTPRoute{
ObjectMeta: meta_v1.ObjectMeta{
Namespace: namespace,
Name: "httproute-1",
},
Spec: gatewayapi_v1.HTTPRouteSpec{
Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"},
CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{
ParentRefs: []gatewayapi_v1.ParentReference{
gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name),
},
},
Rules: []gatewayapi_v1.HTTPRouteRule{
{
Matches: []gatewayapi_v1.HTTPRouteMatch{
{QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})},
},
BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1),
},
{
Matches: []gatewayapi_v1.HTTPRouteMatch{
{QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "dolphin"})},
},
BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1),
},
},
},
}
f.CreateHTTPRouteAndWaitFor(route1, e2e.HTTPRouteAccepted)

By("create httproute-2 with only partially conflicted matches")
route2 := &gatewayapi_v1.HTTPRoute{
ObjectMeta: meta_v1.ObjectMeta{
Namespace: namespace,
Name: "httproute-2",
},
Spec: gatewayapi_v1.HTTPRouteSpec{
Hostnames: []gatewayapi_v1.Hostname{"queryparams.gateway.projectcontour.io"},
CommonRouteSpec: gatewayapi_v1.CommonRouteSpec{
ParentRefs: []gatewayapi_v1.ParentReference{
gatewayapi.GatewayParentRef(gateway.Namespace, gateway.Name),
},
},
Rules: []gatewayapi_v1.HTTPRouteRule{
{
Matches: []gatewayapi_v1.HTTPRouteMatch{
{QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"animal": "whale"})},
},
BackendRefs: gatewayapi.HTTPBackendRef("echo-1", 80, 1),
},
{
Matches: []gatewayapi_v1.HTTPRouteMatch{
{QueryParams: gatewayapi.HTTPQueryParamMatches(map[string]string{"no conflict": "no joke", "no conflict again": "no joke"})},
},
BackendRefs: gatewayapi.HTTPBackendRef("echo-2", 80, 1),
},
},
},
}
f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRoutePartiallyAccepted)
// 1) Drop Rule(s): With this approach, implementations will drop the
// invalid Route Rule(s) until they are fully valid again. The message
// for this condition MUST start with the prefix "Dropped Rule" and
// include information about which Rules have been dropped. In this
// state, the "Accepted" condition MUST be set to "True" with the latest
// generation of the resource.
f.CreateHTTPRouteAndWaitFor(route2, e2e.HTTPRouteAccepted)
})
}
21 changes: 19 additions & 2 deletions test/e2e/gatewayapi_predicates.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,8 @@ func HTTPRouteAccepted(route *gatewayapi_v1.HTTPRoute) bool {
}

// HTTPRouteNotAcceptedDueToConflict returns true if the route has a .status.conditions
// entry of "Accepted: false" && "Reason: RouteConflict" && "Message: HTTPRoute's Match has
// conflict with other HTTPRoute's Match"
// entry of "Accepted: false" && "Reason: RouteMatchConflict" && "Message: HTTPRoute's Match has
// conflict with other HTTPRoute's Match".
func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) bool {
if route == nil {
return false
Expand All @@ -138,6 +138,23 @@ func HTTPRouteNotAcceptedDueToConflict(route *gatewayapi_v1.HTTPRoute) bool {
return false
}

// HTTPRoutePartiallyAccepted returns true if the route has a .status.conditions
// entry of "PartiallyInvalid: true" && "Reason: RuleMatchPartiallyConflict" && "Message:
// HTTPRoute's Match has partial conflict with other HTTPRoute's Match".
func HTTPRoutePartiallyAccepted(route *gatewayapi_v1.HTTPRoute) bool {
if route == nil {
return false
}

for _, gw := range route.Status.Parents {
if conditionExistsWithAllKeys(gw.Conditions, string(gatewayapi_v1.RouteConditionPartiallyInvalid), meta_v1.ConditionTrue, string(status.ReasonRouteRuleMatchPartiallyConflict), "Dropped Rule: HTTPRoute's rule(s) has(ve) been droped because of conflict against other HTTPRoute's rule(s)") {

Check failure on line 150 in test/e2e/gatewayapi_predicates.go

View workflow job for this annotation

GitHub Actions / Codespell

droped ==> dropped
return false
}
}

return false
}

// HTTPRouteIgnoredByContour returns true if the route has an empty .status.parents.conditions list
func HTTPRouteIgnoredByContour(route *gatewayapi_v1.HTTPRoute) bool {
if route == nil {
Expand Down

0 comments on commit 725ce9b

Please sign in to comment.