Feature : Listener Filter Chain Discovery Service #23096

rakeshdatta · 2022-09-13T23:17:50Z

Commit Message: Feature: Listener Filter Chain Discovery Service (SoTW)
Additional Description:
Representing tenants as filter-chains in envoy, its essential to be able to dynamically and independently load the tenant (filter chain) configs. To achieve that, this feature introduces anothe xDS called Filter Chain Discovery Service, which allows the filter chains inside a listener to be discovered dynamically.

This allows a tenant config to be added, deleted and modified on the fly, without impacting other tenant configs.

This is also an Implementation for the ask here: (#4540)
This feature would allow dynamic config update of filter chains.

Main envoy config yaml:

admin:
  address:
    socket_address:
      protocol: TCP
      address: 0.0.0.0
      port_value: 9090
node:
  id: envoy_01
  cluster: talon_01
static_resources:
  listeners:
  - name: https_listener
    address:
      socket_address:
        address: 0.0.0.0
        port_value: 9443
    listener_filters:
      - name: envoy.filters.listener.original_dst
      - name: envoy.filters.listener.tls_inspector
      - name: envoy.filters.listener.tenant_inspector
    fcds_cfg:
      name: "filter_chains_config_01"
      config_source:
        path: ./fcds.yaml

  clusters:
  - name: dynamic_forward_proxy_cluster
    connect_timeout: 1s
    lb_policy: CLUSTER_PROVIDED
    cluster_type:
      name: envoy.clusters.dynamic_forward_proxy
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.clusters.dynamic_forward_proxy.v3.ClusterConfig
        dns_cache_config:
          name: dynamic_forward_proxy_cache_config
          dns_lookup_family: V4_ONLY

fcds (dynamic) config yaml:

---
version_info: "0"
resources:
- "@type": type.googleapis.com/envoy.config.listener.v3.FilterChain
  name: "tenant_id_01"
  filter_chain_match:
    tenant_id: "tenant_id_01"
  filters:
    - name: envoy.filters.network.sni_filter
      <custom filter logic goes here>
    - name: envoy.filters.network.sni_dynamic_forward_proxy
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.filters.network.sni_dynamic_forward_proxy.v3.FilterConfig
        port_value: 443
        dns_cache_config:
          name: dynamic_forward_proxy_cache_config
          dns_lookup_family: V4_ONLY
    - name: envoy.tcp_proxy
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy
        stat_prefix: tcp
        cluster: dynamic_forward_proxy_cluster
- "@type": type.googleapis.com/envoy.config.listener.v3.FilterChain
  name: "tenant_id_02"
  filter_chain_match:
    tenant_id: "tenant_id_02"
  filters:
    - name: envoy.filters.network.sni_filter
      <custom filter config goes here>
    - name: envoy.filters.network.sni_dynamic_forward_proxy
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.filters.network.sni_dynamic_forward_proxy.v3.FilterConfig
        port_value: 443
        dns_cache_config:
          name: dynamic_forward_proxy_cache_config
          dns_lookup_family: V4_ONLY
    - name: envoy.tcp_proxy
      typed_config:
        "@type": type.googleapis.com/envoy.extensions.filters.network.tcp_proxy.v3.TcpProxy
        stat_prefix: tcp
        cluster: dynamic_forward_proxy_cluster

Risk Level: High

Testing: Locally tested with Inotify-based config updates.

Pending work: Test and implement filter-chain level draining

Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2022-09-13T23:17:53Z

Hi @rakeshdatta, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #23096 was opened by rakeshdatta.

see: more, trace.

repokitteh-read-only · 2022-09-13T23:17:58Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #23096 was opened by rakeshdatta.

see: more, trace.

mattklein123 · 2022-09-14T14:29:35Z

Assigning over to @htuch @adisuissa for first pass on API and general feature overview. Thank you!

adisuissa

Thanks for working on this!

I think DCO needs to be fixed.

High level question - what are the warming constraints this xDS service require?

adisuissa · 2022-09-14T16:35:35Z

api/envoy/config/listener/v3/listener.proto

+message Fcds{
+  string fcds_name = 1 [(validate.rules).string.min_len = 1];
+  envoy.config.core.v3.ConfigSource config_source = 2 [(validate.rules).message.required = true];
+}


This should probably be called ListenerFCDS, to differentiate between listener and http filter chains.

adisuissa · 2022-09-14T16:36:02Z

api/envoy/config/listener/v3/listener.proto

@@ -138,6 +139,9 @@ message Listener {
  // :ref:`FAQ entry <faq_how_to_setup_sni>`.
  repeated FilterChain filter_chains = 3;

+  // FCDS: Filter Chain Discovery Service config block. filter_chains and fcds are mutually exclusive
+  Fcds fcds = 34;


I think this needs to be promoted to oneof with filter_chains, and clearly comment that they are mutual exclusive.

Also, is this going to return a single filter-chain? Maybe it should return a glob collection of filter-chains.

I tried oneof when I started implementing this. Had some issues that I can't recall. Will give it another try and update this PR as needed.

FCDS block would encompass a collection of filter chains., which are under a listener Plz refer to the yaml samples in the PR description.

I tried oneof when I started implementing this. Had some issues that I can't recall. Will give it another try and update this PR as needed.

Changing to oneof breaks backwards compatibility. This should be promoted in future major version, and clearly stated in the comments.

FCDS block would encompass a collection of filter chains., which are under a listener Plz refer to the yaml samples in the PR description.

Looking here it implies that the returned type is a single FilterChain object for each request, not multiple ones. I think this should be a Glob collection and return a list of such elements.

Hi @adisuissa
I gave it a thought. I want to represent a single resource as a filter chain, not a list of filter chains. I think the name that I added in the proto, created confusion.

For file inotify, much like LDS, I wanted a single listener to point to one FCDS file which has a list of filter chains resources

Also, for GRPC, I want a listener to be able to subscribe to all(*) or a specific list of filter chains. The Control plane would respond back with a list of resources, where each resource is a filter chain.

In that case, much like LDS proto, I believe the resource type not being a list is fine. Is my understanding correct?

However, the challenge here is, how could a listener subscribe to a specific list of filter chain updates. Seems like thats a challenge that glob collection will solve? Is that the reason you suggested using glob collection?

Changing to oneof breaks backwards compatibility. This should be promoted in future major version, and clearly stated in the comments.

@adisuissa - Can you please show an example how to promote a proto change to a "Future major version"?

adisuissa · 2022-09-14T16:37:18Z

api/envoy/service/filter_chain/fcds.proto

+      returns (stream discovery.v3.DiscoveryResponse) {
+  }
+
+  rpc DeltaFilterChains(stream discovery.v3.DeltaDiscoveryRequest)


Not yet implemented in this PR.

Yes @adisuissa Delta is not implemented in this PR. I am working on it and will submit using a subsequent PR. Hope that works.

kyessenov · 2022-09-14T16:58:32Z

api/envoy/config/listener/v3/listener.proto

@@ -358,3 +362,8 @@ message Listener {
  // :ref:`global_downstream_max_connections <config_overload_manager_limiting_connections>`.
  bool ignore_global_conn_limit = 31;
 }
+
+message Fcds{
+  string fcds_name = 1 [(validate.rules).string.min_len = 1];


Every listener filter chain has a name so one can think of a listener as a collection of filter chains. Is this name referring to the collection name?

That's right @kyessenov. It is referring to the name of the collection of listener filter chains under a listener. It is not used in the code anywhere today though. It's just a name to refer to.

Can I have multiple of those such collections of listener filter-chains under the same listener? No.

Thanks for explaining. If two collections share a filter chain, then at the transport level, do we still have to get duplicate resources <fcds_name/filter_chain.name> if two listeners share a filter chain? I wonder if there is a way to have a global subscription instead of a per-listener subscription model.

Thanks for explaining. If two collections share a filter chain, then at the transport level, do we still have to get duplicate resources <fcds_name/filter_chain.name> if two listeners share a filter chain? I wonder if there is a way to have a global subscription instead of a per-listener subscription model.

Somewhat dependent on the use-case, but if you think of ADS, then there should be a single subscription to the management server, and it will notify the multiple "watchers".

adisuissa · 2022-09-21T10:43:33Z

/wait

rakeshdatta · 2022-09-23T20:29:10Z

/wait

I am working on the comments plus the draining logic. Will be back shortly. Thanks for reviewing the PR.

markdroth · 2022-09-23T20:35:40Z

api/envoy/service/filter_chain/fcds.proto

+// associated HTTP connection manager filters) to use different route
+// configurations. Each listener will bind its HTTP connection manager filter to
+// a route table via this identifier.
+service FcdsDiscoveryService {


Do we actually need this RPC service? I think we've agreed to move the xDS API to a model where new resource types can be added and used via ADS without having to add a new RPC service for each one.

Hi, @markdroth Thanks for the comment. This is new to me; I was not aware. As I am not sure about this, tagging @adisuissa to comment.

Also, wondering what is the downside of having new RPC APIs for this new XDS service. In case we don't have this, can u help me understand how we hook the FCDS back-end logic to the ADS RPC APIs?

Hi @markdroth @adisuissa Plz suggest if it is still acceptable to have these separate RPC endpoint for FCDS.

P.S. I resolved couple of bugs and handling another one on the draining (only the filter chains, without recreating a new listener) sequence now. I am testing off the file inotify way now, and should get to the grpc transport next.

From the wire protocol perspective, ADS already supports this; all you need to do is use the new resource type in the ADS stream.

I don't know how this would work from the perspective of Envoy's existing ADS implementation, although I would expect that this would get much easier once Envoy implements the xDS caching layer described in #13009. @adisuissa can advise further as to how to actually implement this today.

Signed-off-by: Rakesh Datta <[email protected]>

Signed-off-by: Rakesh Datta <[email protected]> Signed-off-by: Rakesh Datta <[email protected]>

jmarantz · 2022-11-23T13:41:01Z

@rakeshdatta thanks for all this. Can you take a look at https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md and in particular the stanza starting with "Once your PR is under review, please do not rebase it. "

Thank you!

Looks like CI is not healthy right now.

/wait

rakeshdatta · 2022-11-28T08:56:51Z

Hi @jmarantz, Thank you for sharing! I am aware of the rebasing requirement; however, I was actually trying to fix the DCO in the very first commit by rebasing the technique. That is not helping though. In case there is an easier way to the fix the DCO if the very first commit, could u plz suggest? Thanks again!

jmarantz · 2022-11-28T13:44:27Z

I know that it's possible to "repair" a PR with git magic (and losing PR comments in the process). But usually I find it easiest to simply open a new PR, and reference the old one from it. That way any comments in the old PR are not lost.

Maybe the most important thing, though, as you start to contribute to Envoy, is to set up a github hook to automatically add the signed-by line. There are instructions in https://github.com/envoyproxy/envoy/blob/main/DEVELOPER.md

jmarantz · 2022-11-28T13:44:37Z

/wait

github-actions · 2023-01-20T00:02:55Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2023-01-27T04:01:14Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Feature : Filter Chain Discovery Service

cacc9a2

repokitteh-read-only bot added the api label Sep 13, 2022

repokitteh-read-only bot assigned mattklein123 Sep 13, 2022

rakeshdatta changed the title ~~Feature : Filter Chain Discovery Service~~ Feature : Filter Chain Discovery Service (SoTW) Sep 13, 2022

rakeshdatta changed the title ~~Feature : Filter Chain Discovery Service (SoTW)~~ Feature : Filter Chain Discovery Service Sep 14, 2022

mattklein123 assigned adisuissa and htuch and unassigned mattklein123 Sep 14, 2022

adisuissa reviewed Sep 14, 2022

View reviewed changes

kyessenov reviewed Sep 14, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 21, 2022

markdroth reviewed Sep 23, 2022

View reviewed changes

repokitteh-read-only bot removed the waiting label Nov 17, 2022

rakeshdatta changed the title ~~Feature : Filter Chain Discovery Service~~ Feature : Listener Filter Chain Discovery Service Nov 17, 2022

rakeshdatta force-pushed the rakeshdatta_fcds_feature branch from 789b5d1 to d8d1085 Compare November 17, 2022 21:03

Adding support for filter chain only draining during FCDS

5bbb284

Signed-off-by: Rakesh Datta <[email protected]>

rakeshdatta force-pushed the rakeshdatta_fcds_feature branch 4 times, most recently from 0eb76c6 to 87757d7 Compare November 17, 2022 22:23

Feature : Listener Filter Chain Discovery Service

733fc2b

Signed-off-by: Rakesh Datta <[email protected]> Signed-off-by: Rakesh Datta <[email protected]>

rakeshdatta force-pushed the rakeshdatta_fcds_feature branch from 87757d7 to 733fc2b Compare November 17, 2022 22:28

repokitteh-read-only bot added the waiting label Nov 23, 2022

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 20, 2023

github-actions bot closed this Jan 27, 2023

rakeshdatta mentioned this pull request Jan 27, 2023

Feature : Listener Filter Chain Discovery Service rakeshdatta/envoy#1

Draft

rakeshdatta mentioned this pull request Feb 9, 2023

Feature : Listener Filter Chain Discovery Service #25473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature : Listener Filter Chain Discovery Service #23096

Feature : Listener Filter Chain Discovery Service #23096

rakeshdatta commented Sep 13, 2022 •

edited

Loading

repokitteh-read-only bot commented Sep 13, 2022

repokitteh-read-only bot commented Sep 13, 2022

mattklein123 commented Sep 14, 2022

adisuissa left a comment

adisuissa Sep 14, 2022

rakeshdatta Dec 1, 2022

adisuissa Sep 14, 2022

rakeshdatta Sep 14, 2022 •

edited

Loading

adisuissa Sep 14, 2022

rakeshdatta Dec 1, 2022 •

edited

Loading

ssripadham Dec 20, 2022

adisuissa Sep 14, 2022

rakeshdatta Sep 14, 2022

kyessenov Sep 14, 2022

rakeshdatta Sep 14, 2022

kyessenov Sep 14, 2022

adisuissa Sep 14, 2022

adisuissa commented Sep 21, 2022

rakeshdatta commented Sep 23, 2022

markdroth Sep 23, 2022

rakeshdatta Oct 20, 2022

rakeshdatta Nov 3, 2022

markdroth Nov 28, 2022

jmarantz commented Nov 23, 2022

rakeshdatta commented Nov 28, 2022

jmarantz commented Nov 28, 2022

jmarantz commented Nov 28, 2022

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 27, 2023

Feature : Listener Filter Chain Discovery Service #23096

Feature : Listener Filter Chain Discovery Service #23096

Conversation

rakeshdatta commented Sep 13, 2022 • edited Loading

repokitteh-read-only bot commented Sep 13, 2022

repokitteh-read-only bot commented Sep 13, 2022

mattklein123 commented Sep 14, 2022

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakeshdatta Sep 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakeshdatta Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa commented Sep 21, 2022

rakeshdatta commented Sep 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmarantz commented Nov 23, 2022

rakeshdatta commented Nov 28, 2022

jmarantz commented Nov 28, 2022

jmarantz commented Nov 28, 2022

github-actions bot commented Jan 20, 2023

github-actions bot commented Jan 27, 2023

rakeshdatta commented Sep 13, 2022 •

edited

Loading

rakeshdatta Sep 14, 2022 •

edited

Loading

rakeshdatta Dec 1, 2022 •

edited

Loading