Skip to content

Commit

Permalink
fix filter and virtual host patch removal (istio#36404)
Browse files Browse the repository at this point in the history 
* fix listener patch removal

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* fix test

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* add test for listener

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* fix vhost patch and add test

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* remove unnecessary changes

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* remove unnecessary changes

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>

* lint

Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
  • Loading branch information
@ramaraochavali
ramaraochavali committed Dec 21, 2021
1 parent a3b61e2 commit 7decc18
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 33 deletions.
48 changes: 24 additions & 24 deletions pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import (
"istio.io/istio/pilot/pkg/model"
"istio.io/istio/pilot/pkg/networking/util"
"istio.io/istio/pilot/pkg/util/runtime"
"istio.io/istio/pilot/pkg/util/sets"
"istio.io/istio/pkg/config/xds"
"istio.io/pkg/log"
)
Expand Down Expand Up @@ -238,12 +239,11 @@ func mergeTransportSocketListener(fc *xdslistener.FilterChain, lp *model.EnvoyFi
func patchNetworkFilters(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
listener *xdslistener.Listener, fc *xdslistener.FilterChain) {
networkFiltersRemoved := false
removedNetworkFilters := sets.NewSet()
for i, filter := range fc.Filters {
if filter.Name == "" {
continue
if patchNetworkFilter(patchContext, patches, listener, fc, fc.Filters[i]) {
removedNetworkFilters.Insert(filter.Name)
}
patchNetworkFilter(patchContext, patches, listener, fc, fc.Filters[i], &networkFiltersRemoved)
}
for _, lp := range patches[networking.EnvoyFilter_NETWORK_FILTER] {
if !commonConditionMatch(patchContext, lp) ||
Expand Down Expand Up @@ -328,21 +328,24 @@ func patchNetworkFilters(patchContext networking.EnvoyFilter_PatchContext,
}
IncrementEnvoyFilterMetric(lp.Key(), NetworkFilter, applied)
}
if networkFiltersRemoved {
if len(removedNetworkFilters) > 0 {
tempArray := make([]*xdslistener.Filter, 0, len(fc.Filters))
for _, filter := range fc.Filters {
if filter.Name != "" {
tempArray = append(tempArray, filter)
if removedNetworkFilters.Contains(filter.Name) {
continue
}
tempArray = append(tempArray, filter)
}
fc.Filters = tempArray
}
}

// patchNetworkFilter patches passed in filter if it is MERGE operation.
// The return value indicates whether the filter has been removed for REMOVE operations.
func patchNetworkFilter(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
listener *xdslistener.Listener, fc *xdslistener.FilterChain,
filter *xdslistener.Filter, networkFilterRemoved *bool) {
filter *xdslistener.Filter) bool {
for _, lp := range patches[networking.EnvoyFilter_NETWORK_FILTER] {
if !commonConditionMatch(patchContext, lp) ||
!listenerMatch(listener, lp) ||
Expand All @@ -352,10 +355,7 @@ func patchNetworkFilter(patchContext networking.EnvoyFilter_PatchContext,
continue
}
if lp.Operation == networking.EnvoyFilter_Patch_REMOVE {
filter.Name = ""
*networkFilterRemoved = true
// nothing more to do in other patches as we removed this filter
return
return true
} else if lp.Operation == networking.EnvoyFilter_Patch_MERGE {
// proto merge doesn't work well when merging two filters with ANY typed configs
// especially when the incoming cp.Value is a struct that could contain the json config
Expand Down Expand Up @@ -399,6 +399,7 @@ func patchNetworkFilter(patchContext networking.EnvoyFilter_PatchContext,
if filter.Name == wellknown.HTTPConnectionManager {
patchHTTPFilters(patchContext, patches, listener, fc, filter)
}
return false
}

func patchHTTPFilters(patchContext networking.EnvoyFilter_PatchContext,
Expand All @@ -412,12 +413,11 @@ func patchHTTPFilters(patchContext networking.EnvoyFilter_PatchContext,
// as this loop will be called very frequently
}
}
httpFiltersRemoved := false
removedFilters := sets.Set{}
for _, httpFilter := range httpconn.HttpFilters {
if httpFilter.Name == "" {
continue
if patchHTTPFilter(patchContext, patches, listener, fc, filter, httpFilter) {
removedFilters.Insert(httpFilter.Name)
}
patchHTTPFilter(patchContext, patches, listener, fc, filter, httpFilter, &httpFiltersRemoved)
}
for _, lp := range patches[networking.EnvoyFilter_HTTP_FILTER] {
applied := false
Expand Down Expand Up @@ -506,11 +506,11 @@ func patchHTTPFilters(patchContext networking.EnvoyFilter_PatchContext,
}
IncrementEnvoyFilterMetric(lp.Key(), HttpFilter, applied)
}
if httpFiltersRemoved {
if len(removedFilters) > 0 {
tempArray := make([]*hcm.HttpFilter, 0, len(httpconn.HttpFilters))
for _, filter := range httpconn.HttpFilters {
if filter.Name != "" {
tempArray = append(tempArray, filter)
if removedFilters.Contains(filter.Name) {
continue
}
}
httpconn.HttpFilters = tempArray
Expand All @@ -521,10 +521,12 @@ func patchHTTPFilters(patchContext networking.EnvoyFilter_PatchContext,
}
}

// patchHTTPFilter patches passed in filter if it is MERGE operation.
// The return value indicates whether the filter has been removed for REMOVE operations.
func patchHTTPFilter(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
listener *xdslistener.Listener, fc *xdslistener.FilterChain, filter *xdslistener.Filter,
httpFilter *hcm.HttpFilter, httpFilterRemoved *bool) {
httpFilter *hcm.HttpFilter) bool {
for _, lp := range patches[networking.EnvoyFilter_HTTP_FILTER] {
applied := false
if !commonConditionMatch(patchContext, lp) ||
Expand All @@ -536,10 +538,7 @@ func patchHTTPFilter(patchContext networking.EnvoyFilter_PatchContext,
continue
}
if lp.Operation == networking.EnvoyFilter_Patch_REMOVE {
httpFilter.Name = ""
*httpFilterRemoved = true
// nothing more to do in other patches as we removed this filter
return
return true
} else if lp.Operation == networking.EnvoyFilter_Patch_MERGE {
// proto merge doesn't work well when merging two filters with ANY typed configs
// especially when the incoming cp.Value is a struct that could contain the json config
Expand Down Expand Up @@ -581,6 +580,7 @@ func patchHTTPFilter(patchContext networking.EnvoyFilter_PatchContext,
}
IncrementEnvoyFilterMetric(lp.Key(), HttpFilter, applied)
}
return false
}

func listenerMatch(listener *xdslistener.Listener, lp *model.EnvoyFilterConfigPatchWrapper) bool {
Expand Down
21 changes: 12 additions & 9 deletions pilot/pkg/networking/core/v1alpha3/envoyfilter/rc_patch.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pilot/pkg/model"
"istio.io/istio/pilot/pkg/util/runtime"
"istio.io/istio/pilot/pkg/util/sets"
"istio.io/pkg/log"
)

Expand Down Expand Up @@ -68,10 +69,12 @@ func ApplyRouteConfigurationPatches(
func patchVirtualHosts(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
routeConfiguration *route.RouteConfiguration, portMap model.GatewayPortMap) {
virtualHostsRemoved := false
removedVirtualHosts := sets.NewSet()
// first do removes/merges
for _, vhost := range routeConfiguration.VirtualHosts {
patchVirtualHost(patchContext, patches, routeConfiguration, vhost, &virtualHostsRemoved, portMap)
if patchVirtualHost(patchContext, patches, routeConfiguration, vhost, portMap) {
removedVirtualHosts.Insert(vhost.Name)
}
}

// now for the adds
Expand All @@ -87,10 +90,10 @@ func patchVirtualHosts(patchContext networking.EnvoyFilter_PatchContext,
IncrementEnvoyFilterMetric(rp.Key(), VirtualHost, false)
}
}
if virtualHostsRemoved {
if len(removedVirtualHosts) > 0 {
trimmedVirtualHosts := make([]*route.VirtualHost, 0, len(routeConfiguration.VirtualHosts))
for _, virtualHost := range routeConfiguration.VirtualHosts {
if virtualHost.Name == "" {
if removedVirtualHosts.Contains(virtualHost.Name) {
continue
}
trimmedVirtualHosts = append(trimmedVirtualHosts, virtualHost)
Expand All @@ -99,27 +102,27 @@ func patchVirtualHosts(patchContext networking.EnvoyFilter_PatchContext,
}
}

// patchVirtualHost patches passed in virtual host if it is MERGE operation.
// The return value indicates whether the virtual host has been removed for REMOVE operations.
func patchVirtualHost(patchContext networking.EnvoyFilter_PatchContext,
patches map[networking.EnvoyFilter_ApplyTo][]*model.EnvoyFilterConfigPatchWrapper,
routeConfiguration *route.RouteConfiguration, virtualHost *route.VirtualHost, virtualHostRemoved *bool, portMap model.GatewayPortMap) {
routeConfiguration *route.RouteConfiguration, virtualHost *route.VirtualHost, portMap model.GatewayPortMap) bool {
for _, rp := range patches[networking.EnvoyFilter_VIRTUAL_HOST] {
applied := false
if commonConditionMatch(patchContext, rp) &&
routeConfigurationMatch(patchContext, routeConfiguration, rp, portMap) &&
virtualHostMatch(virtualHost, rp) {
applied = true
if rp.Operation == networking.EnvoyFilter_Patch_REMOVE {
virtualHost.Name = ""
*virtualHostRemoved = true
// nothing more to do.
return
return true
} else if rp.Operation == networking.EnvoyFilter_Patch_MERGE {
proto.Merge(virtualHost, rp.Value)
}
}
IncrementEnvoyFilterMetric(rp.Key(), VirtualHost, applied)
}
patchHTTPRoutes(patchContext, patches, routeConfiguration, virtualHost, portMap)
return false
}

func hasRouteMatch(rp *model.EnvoyFilterConfigPatchWrapper) bool {
Expand Down
21 changes: 21 additions & 0 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 @@ -23,6 +23,7 @@ import (

networking "istio.io/api/networking/v1alpha3"
"istio.io/istio/pilot/pkg/model"
istionetworking "istio.io/istio/pilot/pkg/networking"
"istio.io/istio/pilot/pkg/serviceregistry/memory"
)

Expand Down Expand Up @@ -332,6 +333,22 @@ func TestApplyRouteConfigurationPatches(t *testing.T) {
Operation: networking.EnvoyFilter_Patch_REMOVE,
},
},
{
ApplyTo: networking.EnvoyFilter_VIRTUAL_HOST,
Match: &networking.EnvoyFilter_EnvoyConfigObjectMatch{
Context: networking.EnvoyFilter_SIDECAR_OUTBOUND,
ObjectTypes: &networking.EnvoyFilter_EnvoyConfigObjectMatch_RouteConfiguration{
RouteConfiguration: &networking.EnvoyFilter_RouteConfigurationMatch{
Vhost: &networking.EnvoyFilter_RouteConfigurationMatch_VirtualHostMatch{
Name: "allow_any",
},
},
},
},
Patch: &networking.EnvoyFilter_Patch{
Operation: networking.EnvoyFilter_Patch_REMOVE,
},
},
{
ApplyTo: networking.EnvoyFilter_VIRTUAL_HOST,
Patch: &networking.EnvoyFilter_Patch{
Expand Down Expand Up @@ -696,9 +713,13 @@ func TestApplyRouteConfigurationPatches(t *testing.T) {
want: patchedArrayInsert,
},
}
cav := istionetworking.BuildCatchAllVirtualHost(true, "")
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
efw := tt.args.push.EnvoyFilters(tt.args.proxy)
if tt.args.patchContext == networking.EnvoyFilter_SIDECAR_OUTBOUND {
tt.args.routeConfiguration.VirtualHosts = append(tt.args.routeConfiguration.VirtualHosts, cav)
}
got := ApplyRouteConfigurationPatches(tt.args.patchContext, tt.args.proxy,
efw, tt.args.routeConfiguration)
if diff := cmp.Diff(tt.want, got, protocmp.Transform()); diff != "" {
Expand Down

0 comments on commit 7decc18

Please sign in to comment.