Skip to content

filter: check if connection is open before and after filter read/write#7256

Merged
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
yxue:segfault
Jun 13, 2019
Merged

filter: check if connection is open before and after filter read/write#7256
mattklein123 merged 5 commits intoenvoyproxy:masterfrom
yxue:segfault

Conversation

@yxue
Copy link
Copy Markdown
Member

@yxue yxue commented Jun 13, 2019

Signed-off-by: Yan Xue yxyan@google.com

Description: Check if connection is open before and after filter read/write. In onData or onWrite, filter could close the underlying connection.
Risk Level: Low
Testing: unit test
Docs Changes: N/A
Release Notes: N/A
[Optional Fixes #Issue]
[Optional Deprecated:]

@yxue
Check if connection is open before and after filter read/write
2ccda96 
Signed-off-by: Yan Xue <yxyan@google.com>
@yxue yxue mentioned this pull request Jun 13, 2019
Merged
@mattklein123 mattklein123 self-assigned this Jun 13, 2019
mattklein123
mattklein123 reviewed Jun 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. While I think this change is generally correct, historically we have assumed that filters will stop iteration if they close the connection/stream. WDYT about just putting in asserts to this effect? it would be a much less scary change.

/wait-any

@yxue
Copy link
Copy Markdown
Member Author

yxue commented Jun 13, 2019 •
edited
Loading

Thanks for working on this. While I think this change is generally correct, historically we have assumed that filters will stop iteration if they close the connection/stream. WDYT about just putting in asserts to this effect? it would be a much less scary change.

/wait-any

I think the assumption is totally correct but sometimes users might not follow it or they are not intend to break the assumption. I found this bug from a filter extended by istio/mixer.

I think either of them is ok. I would prefer providing some tolerance when users don't follow the convention rather than terminating the process. WDYT?

@yxue yxue mentioned this pull request Jun 13, 2019
Closed
jplevyak
jplevyak reviewed Jun 13, 2019
View reviewed changes
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Jun 13, 2019

I think either of them is ok. I would prefer providing some tolerance when users don't follow the convention rather than terminating the process. WDYT?

Fair enough. Sounds good.

mattklein123
mattklein123 requested changes Jun 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks. My general preference here is to have a check after a filter runs, since I can see easily making that mistake, but asserts otherwise.

/wait

@yxue
resolve comments
530f75b 
Signed-off-by: Yan Xue <yxyan@google.com>
mattklein123
mattklein123 requested changes Jun 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, do you have test coverage for all of these new cases?

/wait

@yxue
Add check after filter call
810403f 
Signed-off-by: Yan Xue <yxyan@google.com>
@yxue
Copy link
Copy Markdown
Member Author

yxue commented Jun 13, 2019 •
edited
Loading

Thanks, do you have test coverage for all of these new cases?

/wait

I have 2 tests which should cover these cases.

@yxue
Rename test
9791a74 
Signed-off-by: Yan Xue <yxyan@google.com>
@mattklein123
Copy link
Copy Markdown
Member

mattklein123 commented Jun 13, 2019

If you were able to change the if statement after the filter call without changing your tests, I don't think your tests are complete. Please make them more robust to cover all cases.

/wait

@yxue
Add more tests
3559307 
Signed-off-by: Yan Xue <yxyan@google.com>
@yxue
Copy link
Copy Markdown
Member Author

yxue commented Jun 13, 2019

If you were able to change the if statement after the filter call without changing your tests, I don't think your tests are complete. Please make them more robust to cover all cases.

/wait

I didn't add test for onWrite. Now the test case should cover both read and write. @mattklein123 could you please take a look?

mattklein123
mattklein123 approved these changes Jun 13, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@yxue
Copy link
Copy Markdown
Member Author

yxue commented Jun 13, 2019

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Jun 13, 2019

🤷‍♀️ nothing to rebuild.

🐱

Caused by: a #7256 (comment) was created by @crazyxy.

see: more, trace.

@yxue
Copy link
Copy Markdown
Member Author

yxue commented Jun 13, 2019

/azp run

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines bot commented Jun 13, 2019

Commenter does not have sufficient privileges for PR 7256 in repo envoyproxy/envoy

@mattklein123 mattklein123 merged commit 3b4b500 into envoyproxy:master Jun 13, 2019
@yxue yxue deleted the segfault branch July 1, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

+1 more reviewer

@jplevyak jplevyak jplevyak left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@yxue @mattklein123 @jplevyak