Skip to content

Commit

Permalink
fix patching http_route affects other virtualhosts. (#44820) (#44823)
Browse files Browse the repository at this point in the history
  • Loading branch information
@believening
believening committed May 10, 2023
1 parent 4eae80b commit d1ec7e0
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 50 deletions.
19 changes: 15 additions & 4 deletions pilot/pkg/networking/core/v1alpha3/envoyfilter/rc_patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,11 @@ func patchHTTPRoutes(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
routeConfiguration *route.RouteConfiguration, virtualHost *route.VirtualHost, portMap model.GatewayPortMap,
) {
clonedVhostRoutes := false
routesRemoved := false
// Apply the route level removes/merges if any.
for index := range virtualHost.Routes {
patchHTTPRoute(patchContext, patches, routeConfiguration, virtualHost, index, &routesRemoved, portMap)
patchHTTPRoute(patchContext, patches, routeConfiguration, virtualHost, index, &routesRemoved, portMap, &clonedVhostRoutes)
}

// now for the adds
Expand Down Expand Up @@ -242,19 +243,24 @@ func patchHTTPRoutes(patchContext networking.EnvoyFilter_PatchContext,
func patchHTTPRoute(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
routeConfiguration *route.RouteConfiguration, virtualHost *route.VirtualHost, routeIndex int, routesRemoved *bool, portMap model.GatewayPortMap,
clonedVhostRoutes *bool,
) {
for _, rp := range patches[networking.EnvoyFilter_HTTP_ROUTE] {
applied := false
if commonConditionMatch(patchContext, rp) &&
routeConfigurationMatch(patchContext, routeConfiguration, rp, portMap) &&
virtualHostMatch(virtualHost, rp) &&
routeMatch(virtualHost.Routes[routeIndex], rp) {
if !*clonedVhostRoutes {
// different virtualHosts may share same routes pointer
virtualHost.Routes = cloneVhostRoutes(virtualHost.Routes)
*clonedVhostRoutes = true
}
if rp.Operation == networking.EnvoyFilter_Patch_REMOVE {
virtualHost.Routes[routeIndex] = nil
*routesRemoved = true
return
} else if rp.Operation == networking.EnvoyFilter_Patch_MERGE {
cloneVhostRouteByRouteIndex(virtualHost, routeIndex)
merge.Merge(virtualHost.Routes[routeIndex], rp.Value)
}
applied = true
Expand Down Expand Up @@ -389,6 +395,11 @@ func routeMatch(httpRoute *route.Route, rp *model.EnvoyFilterConfigPatchWrapper)
return true
}

func cloneVhostRouteByRouteIndex(virtualHost *route.VirtualHost, routeIndex int) {
virtualHost.Routes[routeIndex] = proto.Clone(virtualHost.Routes[routeIndex]).(*route.Route)
func cloneVhostRoutes(routes []*route.Route) []*route.Route {
out := make([]*route.Route, len(routes))
for i := 0; i < len(routes); i++ {
clone := proto.Clone(routes[i]).(*route.Route)
out[i] = clone
}
return out
}
117 changes: 71 additions & 46 deletions pilot/pkg/networking/core/v1alpha3/envoyfilter/rc_patch_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

route "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
"github.com/google/go-cmp/cmp"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/testing/protocmp"

networking "istio.io/api/networking/v1alpha3"
Expand Down Expand Up @@ -950,6 +951,16 @@ func Test_routeMatch(t *testing.T) {
}

func TestPatchHTTPRoute(t *testing.T) {
sharedVHostRoutes := []*route.Route{
{
Name: "shared",
Action: &route.Route_Route{
Route: &route.RouteAction{
PrefixRewrite: "/shared",
},
},
},
}
type args struct {
patchContext networking.EnvoyFilter_PatchContext
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper
Expand All @@ -958,6 +969,8 @@ func TestPatchHTTPRoute(t *testing.T) {
routeIndex int
routesRemoved *bool
portMap model.GatewayPortMap
clonedVhostRoutes bool
sharedRoutesVHost *route.VirtualHost
}
obj := &route.Route{}
if err := xds.StructToMessage(buildPatchStruct(`{"route": { "prefix_rewrite": "/foo"}}`), obj, false); err != nil {
Expand Down Expand Up @@ -1012,6 +1025,7 @@ func TestPatchHTTPRoute(t *testing.T) {
portMap: map[int]map[int]struct{}{
8080: {},
},
clonedVhostRoutes: false,
},
want: &route.VirtualHost{
Name: "normal",
Expand All @@ -1028,64 +1042,75 @@ func TestPatchHTTPRoute(t *testing.T) {
},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
patchHTTPRoute(tt.args.patchContext, tt.args.patches, tt.args.routeConfiguration,
tt.args.virtualHost, tt.args.routeIndex, tt.args.routesRemoved, tt.args.portMap)
if diff := cmp.Diff(tt.want, tt.args.virtualHost, protocmp.Transform()); diff != "" {
t.Errorf("PatchHTTPRoute(): %s mismatch (-want +got):\n%s", tt.name, diff)
}
})
}
}

func TestCloneVhostRouteByRouteIndex(t *testing.T) {
type args struct {
vh1 *route.VirtualHost
vh2 *route.VirtualHost
}
cloneRouter := route.Route{
Name: "clone",
Action: &route.Route_Route{
Route: &route.RouteAction{
PrefixRewrite: "/clone",
},
},
}
tests := []struct {
name string
args args
wantClone bool
}{
{
name: "clone",
name: "shared",
args: args{
vh1: &route.VirtualHost{
Name: "vh1",
Domains: []string{"*"},
Routes: []*route.Route{
&cloneRouter,
patchContext: networking.EnvoyFilter_GATEWAY,
patches: map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper{
networking.EnvoyFilter_HTTP_ROUTE: {
{
ApplyTo: networking.EnvoyFilter_HTTP_ROUTE,
Match: &networking.EnvoyFilter_EnvoyConfigObjectMatch{
ObjectTypes: &networking.EnvoyFilter_EnvoyConfigObjectMatch_RouteConfiguration{
RouteConfiguration: &networking.EnvoyFilter_RouteConfigurationMatch{
Vhost: &networking.EnvoyFilter_RouteConfigurationMatch_VirtualHostMatch{
Name: "shared",
Route: &networking.EnvoyFilter_RouteConfigurationMatch_RouteMatch{
Action: networking.EnvoyFilter_RouteConfigurationMatch_RouteMatch_ROUTE,
},
},
},
},
},
Operation: networking.EnvoyFilter_Patch_MERGE,
Value: obj,
},
},
},
vh2: &route.VirtualHost{
Name: "vh2",
routeConfiguration: &route.RouteConfiguration{Name: "normal"},
virtualHost: &route.VirtualHost{
Name: "shared",
Domains: []string{"*"},
Routes: []*route.Route{
&cloneRouter,
Routes: sharedVHostRoutes,
},
routeIndex: 0,
portMap: map[int]map[int]struct{}{
8080: {},
},
clonedVhostRoutes: false,
sharedRoutesVHost: &route.VirtualHost{
Name: "foo",
Domains: []string{"bar"},
Routes: sharedVHostRoutes,
},
},
want: &route.VirtualHost{
Name: "shared",
Domains: []string{"*"},
Routes: []*route.Route{
{
Name: "shared",
Action: &route.Route_Route{
Route: &route.RouteAction{
PrefixRewrite: "/foo",
},
},
},
},
},
wantClone: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cloneRouter.Name = "test"
cloneVhostRouteByRouteIndex(tt.args.vh1, 0)
if tt.args.vh1.Routes[0].Name != tt.args.vh2.Routes[0].Name && tt.wantClone {
t.Errorf("CloneVhostRouteByRouteIndex(): %s (-wantClone +got):%v", tt.name, tt.wantClone)
savedSharedVHost := proto.Clone(tt.args.sharedRoutesVHost).(*route.VirtualHost)
patchHTTPRoute(tt.args.patchContext, tt.args.patches, tt.args.routeConfiguration,
tt.args.virtualHost, tt.args.routeIndex, tt.args.routesRemoved, tt.args.portMap, &tt.args.clonedVhostRoutes)
if diff := cmp.Diff(tt.want, tt.args.virtualHost, protocmp.Transform()); diff != "" {
t.Errorf("PatchHTTPRoute(): %s mismatch (-want +got):\n%s", tt.name, diff)
}
if diff := cmp.Diff(savedSharedVHost, tt.args.sharedRoutesVHost, protocmp.Transform()); diff != "" {
t.Errorf("PatchHTTPRoute(): %s affect other virtualhosts (-want +got):\n%s", tt.name, diff)
}
})
}
Expand Down
8 changes: 8 additions & 0 deletions releasenotes/notes/44820.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
apiVersion: release-notes/v2
kind: bug-fix
area: traffic-management
issue:
- 44820
releaseNotes:
- |
**Fixed** the bug where patching http_route affects other virtualhosts.

0 comments on commit d1ec7e0

Please sign in to comment.