Skip to content

listener: split listenerfilterfactorycontext and networkfilterfactorycontext#10491

Merged
mattklein123 merged 73 commits intoenvoyproxy:masterfrom
lambdai:lctxonly
Apr 3, 2020
Merged

listener: split listenerfilterfactorycontext and networkfilterfactorycontext#10491
mattklein123 merged 73 commits intoenvoyproxy:masterfrom
lambdai:lctxonly

Conversation

@lambdai
Copy link
Copy Markdown
Contributor

@lambdai lambdai commented Mar 23, 2020

Description:
Extract from #9773
The ultimate goal is to update network filter chains without destructing all the network filter chains in previous listener config.
Splitting lifetime of listener filter and network filter, so that we can reuse the references to the existing listener filters in old listener (will do in follow up PR or see #9773)
The next PR is to arm the connection handler with in place filter chain update.

Also eliminate the risk of triggering warmed up listener when destroying ListenerImpl.

Risk Level: LOW since the life time is equivalent with and without this PR
Testing: Fixed existing tests
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

lambdai added 30 commits January 8, 2020 19:59
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
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.

Generally looks OK, thanks. Lets get the other PRs merged and the small comments addressed and than I think we can ship. Thank you!

/wait

lambdai added 3 commits April 2, 2020 08:58
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai added 3 commits April 2, 2020 09:56
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Apr 2, 2020

merging master and resolving conflict with my other PR
Seems I hit another static initialized string issue :(

lambdai added 2 commits April 2, 2020 23:21
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
lambdai added 2 commits April 3, 2020 05:54
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>
@lambdai
Copy link
Copy Markdown
Contributor Author

lambdai commented Apr 3, 2020

This is the closest situation to all check passing during these days. @mattklein123 I addressed all your comments. Could you take another (hopefully last) look? Thanks!

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, thanks!

const Network::ConnectionSocket& socket) const;

const FilterChainManagerImpl* getOriginFilterChainManager() {
ASSERT(origin_.has_value());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: This will throw an exception/crash on the following line so this ASSERT doesn't add any value. Please remove in your next change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought we prefer prefer these precondition-ish ASSERT since the assertion failure is more friendly in debugging mode.
Sure, I will update in the next PR. Thank you for allowing me amending in follow up!

@mattklein123 mattklein123 merged commit 991f97c into envoyproxy:master Apr 3, 2020
@@ -21,7 +21,7 @@ class FilterChainFactoryContextCreator {
* Generate the filter chain factory context from proto. Note the caller does not own the filter
* chain context.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Quick drive-by comment: IIUC, the caller does own the filter chain context now, is that right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants