Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix filter and virtual host patch removal #36404

Merged
merged 8 commits into from
Dec 21, 2021
Merged

fix filter and virtual host patch removal #36404

merged 8 commits into from
Dec 21, 2021

Conversation

ramaraochavali
Copy link
Contributor

@ramaraochavali ramaraochavali commented Dec 7, 2021 •
edited
Loading

  • Configuration Infrastructure
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Policies and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@ramaraochavali
fix listener patch removal
2a8d784 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali ramaraochavali requested a review from a team as a code owner December 7, 2021 09:42
@istio-policy-bot istio-policy-bot added area/networking release-notes-none Indicates a PR that does not require release notes. labels Dec 7, 2021
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 7, 2021
@hzxuzhonghu
Copy link
Member

can you add some tests, since i can see the previous remove handling seems ok

	if networkFiltersRemoved {
		tempArray := make([]*xdslistener.Filter, 0, len(fc.Filters))
		for _, filter := range fc.Filters {
			if filter.Name != "" {
				tempArray = append(tempArray, filter)
			}
		}
		fc.Filters = tempArray
	}

@ramaraochavali ramaraochavali added the do-not-merge/hold Block automatic merging of a PR. label Dec 7, 2021
@ramaraochavali
Copy link
Contributor Author

Will add tests then we can review

howardjohn
howardjohn reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good but can we have a test?

howardjohn
howardjohn reviewed Dec 7, 2021
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VHOST remove is broken as well:

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: connect-match
  namespace: default
spec:
  configPatches:
  - applyTo: VIRTUAL_HOST
    match:
      context: SIDECAR_OUTBOUND
      routeConfiguration:
        vhost:
          name: allow_any
    patch:
      operation: REMOVE

@ramaraochavali
fix test
533e881 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
Merge remote-tracking branch 'upstream/master' into fix/listener_patch
759fba6
@ramaraochavali
add test for listener
af81d8c 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 10, 2021
@ramaraochavali ramaraochavali changed the title fix filter removal in listener patch add test case for listener patch removal Dec 10, 2021
@ramaraochavali
Copy link
Contributor Author

Listener patch removal seems to be working fine. So just added a test

@ramaraochavali
Copy link
Contributor Author

@howardjohn VHOST remove is broken as well - It may be because we are catching the catchAllVirtualHost? Or are you thinking it is because of some other problem?

@howardjohn
Copy link
Member

No I had the same issue I think? With empty name. But maybe also related to vhost cache. But if we remove it should not be a problem for the cache/

@ramaraochavali
Copy link
Contributor Author

Ok. Let me try that Envoy filter and see what is going on.

hzxuzhonghu
hzxuzhonghu reviewed Dec 13, 2021
View reviewed changes
pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch.go
@@ -524,7 +528,7 @@ func patchHTTPFilters(patchContext networking.EnvoyFilter_PatchContext,
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better document the meaning of return value

@ramaraochavali
Copy link
Contributor Author

I was able to reproduce virtual host removal issue in our environment. But not able to do it via unit test. It works correctly. I will continue to spend some time to reproduce it via unit test.

@ramaraochavali ramaraochavali changed the title add test case for listener patch removal fix filter and virtual host patch removal Dec 15, 2021
@ramaraochavali
fix vhost patch and add test
37d3a63 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
remove unnecessary changes
f4d4eef 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@ramaraochavali
remove unnecessary changes
8214d99 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 15, 2021
@ramaraochavali
Copy link
Contributor Author

@howardjohn @hzxuzhonghu Fixed the issue for vhost and listeners. Added test for vhost - with out this change, the last test case in TestApplyRouteConfigurationPatches will have virtual host with empty name.

@ramaraochavali
lint
daabeec 
Signed-off-by: Rama Chavali <rama.rao@salesforce.com>
@YonkaFang YonkaFang mentioned this pull request Dec 17, 2021
Open
@ramaraochavali ramaraochavali removed the do-not-merge/hold Block automatic merging of a PR. label Dec 21, 2021
@ramaraochavali
Copy link
Contributor Author

/test unit-tests_istio

@istio-testing istio-testing merged commit 7decc18 into istio:master Dec 21, 2021
@ramaraochavali ramaraochavali deleted the fix/listener_patch branch December 21, 2021 06:25
hzxuzhonghu
hzxuzhonghu approved these changes Dec 21, 2021
View reviewed changes
pilot/pkg/networking/core/v1alpha3/envoyfilter/listener_patch.go
@@ -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))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: len(httpconn.HttpFilters)-len(removedFilters)

This was referenced Dec 21, 2021
Merged
Closed
@ramaraochavali ramaraochavali mentioned this pull request Mar 14, 2022
Merged
11 tasks
@cywang1905 cywang1905 mentioned this pull request Mar 21, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu approved these changes

@howardjohn howardjohn howardjohn approved these changes

Assignees
No one assigned
Labels
area/networking release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ramaraochavali @hzxuzhonghu @howardjohn @istio-testing @istio-policy-bot