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

ecds: add support for network filters #14696

Closed
kyessenov opened this issue Jan 14, 2021 · 6 comments · Fixed by #29240
Closed

ecds: add support for network filters #14696

kyessenov opened this issue Jan 14, 2021 · 6 comments · Fixed by #29240
Assignees
@kyessenov
Labels
area/configuration area/xds enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@kyessenov
Copy link
Contributor

Implement ECDS for network filters in listeners. API added here
@kyessenov kyessenov added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jan 14, 2021
@kyessenov
Copy link
Contributor Author

QQ: from a quick glance, https://github.com/envoyproxy/envoy/blob/master/source/common/filter/http/filter_config_discovery_impl.h would work just fine for network filters. However, it would need to be templatized by factory and factory callback classes. This would make it a header-only source file. Is that a reasonable approach style-wise?

@htuch
Copy link
Member

htuch commented Jan 14, 2021

@kyessenov if you can minimize what gets dragged into the template I think it would be good; we prefer not having massive header-only templates. E.g.. put a bunch of the shared logic in a shared base class.

@htuch htuch added area/configuration area/xds and removed triage Issue requires triage labels Jan 14, 2021
@mattklein123 mattklein123 added the help wanted Needs help! label Jan 14, 2021
@kyessenov kyessenov mentioned this issue Jan 15, 2021
Closed
@skiptomyliu skiptomyliu mentioned this issue Apr 8, 2021
Open
@tbarrella
Copy link
Contributor

tbarrella commented Aug 12, 2021 •
edited
Loading

I plan to redo and finish #14717 with these steps:

  • Move files (envoy/filter/http -> envoy/filter, etc.)
  • Create shared base classes
  • Templatize code
  • Make network filter code additions/changes/tests

@tbarrella tbarrella mentioned this issue Aug 12, 2021
Merged
htuch pushed a commit that referenced this issue Aug 16, 2021
@tbarrella
ecds: move filter config files out of http/ (#17701)
34910b6 
Refactoring in preparation for #14696 (see #14696 (comment))

Risk Level: Low
Testing: Existing

#14696

Signed-off-by: Taylor Barrella <[email protected]>
@tbarrella tbarrella mentioned this issue Aug 16, 2021
Merged
@tbarrella tbarrella mentioned this issue Aug 25, 2021
Merged
@mathetake mathetake mentioned this issue Sep 7, 2021
Open
@tbarrella tbarrella mentioned this issue Sep 9, 2021
Merged
htuch pushed a commit that referenced this issue Oct 28, 2021
@tbarrella
ecds: make onConfigUpdate generic over filter type (#18061)
4a2b01f 
Additional Description: Part of #14696 (comment). The goal is to templatize DynamicFilterConfigProviderImpl while having DynamicFilterConfigProviderImplBase, FilterConfigProviderManagerImplBase, and FilterConfigSubscription agnostic to HTTP vs. network filters. This is a step in that direction, following the approach of #14717. Because extra validation was added since that PR, the filter factory object must be created before onConfigUpdate in addition to during

Risk Level: Low
Testing: Existing (refactoring)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
#14696

Signed-off-by: Taylor Barrella <[email protected]>
@tbarrella tbarrella mentioned this issue Oct 29, 2021
Merged
lizan pushed a commit that referenced this issue Nov 5, 2021
@tbarrella
ecds: templatize FilterConfigProviderManagerImpl (#18832)
0264041 
Part of #14696 (comment). The refactoring that enables this is mainly within `FilterConfigSubscription::onConfigUpdate`. The rest is adding and removing template parameters.
Risk Level: Low
Testing: Existing (refactoring)
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
#14696

Signed-off-by: Taylor Barrella <[email protected]>
@tbarrella
Copy link
Contributor

I stopped work on this for now so @kyessenov or someone else can continue. Templatizing ECDS code so it can be used for more than just HTTP filters is done, but I did not start any work on network filters.

@yanjunxiang-google yanjunxiang-google mentioned this issue Feb 18, 2022
Closed
@zirain
Copy link
Member

zirain commented Jul 25, 2022

@kyessenov do you plan to bring back #14717?

@ramaraochavali ramaraochavali mentioned this issue Feb 9, 2023
Closed
@ohadvano
Copy link
Contributor

ohadvano commented Jul 14, 2023 •
edited
Loading

I plan to try and continue this work for adding network filters support with ECDS.

Reading the comments here it seems as @tbarrella did most of the refactoring and templatization for allowing this. Later on, @yanjunxiang-google did some more refactoring for listener filters ECDS support in #20049.

Breakdown of steps:

  • Adding network filters config provider equivalents in config_discovery_impl.h, unit tests
  • Adjusting listener_manager_impl.cc to support dynamic config providers for downstream network filters, integration tests, docs
  • Similar support dynamic config providers for upstream network filters, integration tests, docs

@ohadvano ohadvano mentioned this issue Jul 14, 2023
Merged
htuch pushed a commit that referenced this issue Jul 19, 2023
@ohadvano
ecds: add network filter dynamic config provider (#28407)
480765e 
Continuing the templatization work that has been done in order to support network filters for ECDS, as a similar feature to listener and HTTP filters. Linked issue: #14696. This PR adds dynamic config providers for network filters without listener consuming.

Risk Level: low
Testing: Unit tests

Signed-off-by: ohadvano <[email protected]>
@ohadvano ohadvano mentioned this issue Jul 19, 2023
Merged
@adisuissa adisuissa mentioned this issue Jul 20, 2023
Closed
@ohadvano ohadvano mentioned this issue Aug 6, 2023
Closed
This was referenced Aug 24, 2023
Closed
Merged
htuch pushed a commit that referenced this issue Aug 30, 2023
@ohadvano
ecds: support for upstream network filter (#29240)
41adbd5 
Support of upstream network filter with ECDS. Resolves #14696.

Risk Level: low
Testing: Integration tests
Docs Changes: ECDS docs

Signed-off-by: ohadvano <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kyessenov kyessenov

Labels
area/configuration area/xds enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Milestone
No milestone
6 participants
@zirain @mattklein123 @kyessenov @htuch @tbarrella @ohadvano