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

Listener filter ECDS Support #21133

Merged
merged 30 commits into from
Jun 1, 2022
Merged

Listener filter ECDS Support #21133

merged 30 commits into from
Jun 1, 2022

Conversation

yanjunxiang-google
Copy link
Contributor

@yanjunxiang-google yanjunxiang-google commented May 3, 2022 •
edited
Loading

This PR support TCP listener filter ECDS functionalities.

This is the 4th PR to support TCP listener filter ECDS as in issue: #20049.

Note, the support of UDP listener ECDS will be done in a separate PR as in item 8 of issue #20049

Build:
passed

Testing:
passed

Release Notes:
N/A

Issues: ecds: add support for listener filters #20049

Signed-off-by: Yanjun Xiang yanjunxiang@google.com

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@yanjunxiang-google
Listener filter ECDS Support
a4125b6 
This PR support both TCP and UDP listener filter ECDS functionalities.

Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google yanjunxiang-google marked this pull request as draft May 3, 2022 20:45
@yanjunxiang-google
Copy link
Contributor Author

/wait

@yanjunxiang-google
Copy link
Contributor Author

yanjunxiang-google commented May 3, 2022 •
edited
Loading

This PR is WIP.

Need to add integration test for UDP listener filter ECDS.

@yanjunxiang-google
fixing format error
5e88f69 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Split UDP listener filter support to a separate PR.
dd55ab9 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google yanjunxiang-google marked this pull request as ready for review May 8, 2022 02:43
@yanjunxiang-google
Copy link
Contributor Author

/assign @adisuissa @kyessenov

@yanjunxiang-google yanjunxiang-google mentioned this pull request May 8, 2022
Closed
@yanjunxiang-google
fixing CI error
9f2b442 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
soulxu
soulxu suggested changes May 9, 2022
View reviewed changes
source/server/configuration_impl.cc Outdated Show resolved Hide resolved
source/server/configuration_impl.cc Outdated Show resolved Hide resolved
source/server/configuration_impl.cc Show resolved Hide resolved
adisuissa
adisuissa reviewed May 9, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa 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.
Here's a first pass.

envoy/filter/config_provider_manager.h Show resolved Hide resolved
source/server/configuration_impl.cc Show resolved Hide resolved
source/server/listener_impl.cc Outdated Show resolved Hide resolved
source/server/listener_impl.cc Outdated Show resolved Hide resolved
source/server/listener_manager_impl.cc Show resolved Hide resolved
source/server/listener_manager_impl.cc Outdated Show resolved Hide resolved
source/server/listener_manager_impl.cc Outdated Show resolved Hide resolved
source/server/listener_manager_impl.cc Outdated Show resolved Hide resolved
source/server/listener_manager_impl.cc Outdated Show resolved Hide resolved
source/server/listener_manager_impl.h Outdated Show resolved Hide resolved
@yanjunxiang-google
addressing comments
f9dd5a0 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
fixing CI error
b2eed54 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@kyessenov kyessenov mentioned this pull request May 10, 2022
Closed
@yanjunxiang-google
comments
3d8265c 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
fix CI error
bf16c3d 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
fix CI error
7011774 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
fixing server_test failure
e59f26f 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21133 (comment) was created by @yanjunxiang-google.

see: more, trace.

@yanjunxiang-google
fixing clangTidy error
7a8dc70 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

/assign @htuch @KBaichoo

@adisuissa
Copy link
Contributor

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21133 (comment) was created by @adisuissa.

see: more, trace.

adisuissa
adisuissa reviewed May 27, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks! Left one small comment.

envoy/config/extension_config_provider.h Show resolved Hide resolved
@yanjunxiang-google
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21133 (comment) was created by @yanjunxiang-google.

see: more, trace.

adisuissa
adisuissa previously approved these changes May 27, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa 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!
Let's make sure CI is green.
@soulxu in case you have more comments.

@yanjunxiang-google
fixing CI error due to .h not generated correctly
79679db 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

Created an issue to track the listener filter matcher integration test: #21484

@yanjunxiang-google
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #21133 (comment) was created by @yanjunxiang-google.

see: more, trace.

@soulxu
Copy link
Member

soulxu commented May 27, 2022

LGTM, thanks! Let's make sure CI is green. @soulxu in case you have more comments.

previously one thing come to my mind, I think the current is the right way. But just put it here in case people have different thoughts. I'm thinking about whether we should enable the listener matcher for MissConfigFilter. Should we skip the connection close if the connection doesn't match the listener matcher? But I think the current code is right, since it is an invalid config case, then closing the connection directly without considering the listener matcher is right. And it is a little strange for some connection successes due to not matching the matcher, some connections are failed.

Anyway, I think this is LGTM now, thanks!

soulxu
soulxu previously approved these changes May 27, 2022
View reviewed changes
@yanjunxiang-google
adding integration tests for listener filter matcher
2864e0b 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

yanjunxiang-google commented May 30, 2022 •
edited
Loading

LGTM, thanks! Let's make sure CI is green. @soulxu in case you have more comments.

previously one thing come to my mind, I think the current is the right way. But just put it here in case people have different thoughts. I'm thinking about whether we should enable the listener matcher for MissConfigFilter. Should we skip the connection close if the connection doesn't match the listener matcher? But I think the current code is right, since it is an invalid config case, then closing the connection directly without considering the listener matcher is right. And it is a little strange for some connection successes due to not matching the matcher, some connections are failed.

Anyway, I think this is LGTM now, thanks!

Thanks for sharing the thoughts. What you described make sense to me, i.e, if a listener filter config is invalid, the filter chain match predicate value should be ignored.

@yanjunxiang-google
adding matcher integration tests for static filter
fc1e9c1 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
@yanjunxiang-google
Copy link
Contributor Author

/retest

1 similar comment
@adisuissa
Copy link
Contributor

/retest

@yanjunxiang-google
Copy link
Contributor Author

Hi @mattklein123 , do you have further comments? After you took the pass last time, what we changed is adding the missing_config counter and the integration tests for listener filter matcher.
If no further comments, could you please approve?

@mattklein123
Copy link
Member

I already approved this. Please have the others doing active reviews review any new code and merge. Thank you!

@yanjunxiang-google
Copy link
Contributor Author

I already approved this. Please have the others doing active reviews review any new code and merge. Thank you!

Thanks @mattklein123

adisuissa
adisuissa approved these changes Jun 1, 2022
View reviewed changes
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@adisuissa adisuissa merged commit c98dc9f into envoyproxy:main Jun 1, 2022
@yanjunxiang-google yanjunxiang-google mentioned this pull request Jun 2, 2022
Merged
@yanjunxiang-google yanjunxiang-google deleted the ecds_hook branch June 2, 2022 21:36
@yanjunxiang-google yanjunxiang-google mentioned this pull request Jun 9, 2022
Merged
tyxia pushed a commit to tyxia/envoy that referenced this pull request Jun 14, 2022
@yanjunxiang-google @tyxia
ECDS: Add Listener filter ECDS Support (envoyproxy#21133)
af4f5da 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Amila-Rukshan pushed a commit to Amila-Rukshan/envoy that referenced this pull request Jun 28, 2022
@yanjunxiang-google @Amila-Rukshan
ECDS: Add Listener filter ECDS Support (envoyproxy#21133)
aae8f1b 
Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
Signed-off-by: Amila Senadheera <amila.15@cse.mrt.ac.lk>
@zirain zirain mentioned this pull request Feb 10, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kyessenov kyessenov kyessenov left review comments

@KBaichoo KBaichoo KBaichoo left review comments

@adisuissa adisuissa adisuissa approved these changes

@mattklein123 mattklein123 mattklein123 left review comments

@soulxu soulxu soulxu left review comments

@alyssawilk alyssawilk Awaiting requested review from alyssawilk alyssawilk is a code owner

Assignees

@KBaichoo KBaichoo

@adisuissa adisuissa

@mattklein123 mattklein123

@kyessenov kyessenov

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yanjunxiang-google @htuch @mattklein123 @adisuissa @soulxu @KBaichoo @kyessenov