WIP: config: Add FilterChain Discovery Service. #5385

ggreenway · 2018-12-20T23:39:12Z

Signed-off-by: Greg Greenway ggreenway@apple.com

Description: TODO
Risk Level: TODO
Testing: TODO
Docs Changes: TODO
Release Notes: TODO
Fixes: #4540

Fixes: 4540 Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2018-12-20T23:40:28Z

I've broken ground on this feature. I'll try to update this frequently for early feedback/comments on the design and high-level implementation.

PiotrSikora

Awesome! Thanks for starting this!

Nit: it should be really FDS and not FCDS.

cc @duderino

ggreenway · 2018-12-21T00:29:17Z

Nit: it should be really FDS and not FCDS.

Why? I don't feel strongly either way, but FilterChain is two words, so I gave the abbreviation two letters.

PiotrSikora · 2018-12-21T00:40:12Z

Why? I don't feel strongly either way, but FilterChain is two words, so I gave the abbreviation two letters.

Every other xDS is x DS, so it matches nicely with rest of the APIs... and it sounds slightly better.

But it's mostly a matter of preference, I don't have any better arguments here.

snowp · 2018-12-21T05:23:14Z

#4704 is introducing SRDS so having this be FCDS is probably fine

AndresGuedez · 2018-12-21T19:23:49Z

@ggreenway fyi -- #5243 is introducing a config provider framework that may be helpful for the FCDS implementation.

ggreenway · 2018-12-21T19:36:59Z

@ggreenway fyi -- #5243 is introducing a config provider framework that may be helpful for the FCDS implementation.

Thanks for the tip. I'll keep an eye on it.

…test Signed-off-by: Greg Greenway <ggreenway@apple.com>

htuch · 2018-12-24T12:24:40Z

FDS is fine for me as well. @snowp agreed that we now have precedent for this, but shorter is better where we can get away with it I reckon :)

htuch

Thanks for getting this moving, this will be an enabler for on-demand LDS.

htuch · 2018-12-24T12:25:41Z

api/envoy/api/v2/fcds.proto

+
+package envoy.api.v2;
+
+option java_generic_services = true;


Nit: Should this be down next to gogoproto decl? Seems like it makes sense to group options.

htuch · 2018-12-24T12:26:11Z

api/envoy/api/v2/fcds.proto

+
+// [#protodoc-title: Filter Chain Discovery Service]
+
+// The resource_names field in DiscoveryRequest specifies a listener.


How come a listener and not a filter chain?

My thought was that envoy would be telling the mgmt server which listener it needs the filter chains for. I believe this is similar to EDS, where the resource_names specify the clusters to get endpoints for.

The only downside to this would be if a user wants multiple listeners to reference the same filter chain, and then specify it within a listener by name. I'm not sure it matters that much in practice so seems fine either way.

May be I am missing something. In a situation where you have a single listener on original dst port having 100s of filter chains (one for each of the virtual listeners that we would otherwise be constructing with the ye ol bind-to-port), how would specifying the listener name be sufficient?

@rshriram Envoy is asking the mgmt server "Please give me all the filterchains/matches for listener Foo".

htuch · 2018-12-24T12:27:32Z

api/envoy/api/v2/lds.proto

+    core.ConfigSource config = 1 [(validate.rules).message.required = true];
+
+    // Optional alternative to the listener name to present to FCDS.
+    string filter_chain_name = 2;


I'm thinking we want to make filter chains first class named resource objects and name them directly, instead of making listeners pseudo-filterchains. This seems the cleanest thing to do, curious what the motivation was for the current approach you have in the PR?

I was trying to copy the semantics of EDS for consistency.

Are you suggesting that the filter chains should be entirely independent of the listeners? That would allow multiple listeners to share a set of filter-chains, but we'd have to think carefully about the lifetime issues.

I guess this is related to my comment above. I don't think @htuch is proposing actually sharing the chains (I think), but is suggesting that they have a first class name and we don't default to the listener name. As I said above I'm fine either way.

I agree with @htuch . FilterChains should be the resource that is being requested (mandatory and never optional). We can combine it with the listener name for precision. I am not sure sharing filter chains makes sense in the common case

As this is currently written, the set of filter chains can be given a first-class name; it just defaults to using the listener name if no name is specified. Is the suggestion here that this name be required instead of optional? Am I misunderstanding or missing something regarding this?

Also, note that the requested resource is always the set of filterchains/matches to use for a listener, not a single filter chain.

I think it makes more sense to name individual filter chains as first class for the purpose of incremental xDS. This will future proof FDS to a larger extent. Essentially, the Listener has a list of filter chain names that describe the resources that come with it. When we lazy load, we will request an additional named filter chain resource.

I don't think we need to share filter chains across listeners; as an implementation detail you might if you really wanted to save overhead and expect many different listeners to have the same filter chains, but that probably isn't the starting point.

If we did that, it would no longer solve the original problem: we want to be able to add/remove filter chains without modifying the listener (which would result in listener draining).

The answer may be to support both modes: either request "all filter chains for this listener", or request "filter chain X for this listener".

Won't most/all of the xDS APIs need to be augmented in a similar way to support lazy-loading?

As discussed offline, it seems reasonable to make listener changes non-draining for the filter chain list.

api/envoy/api/v2/lds.proto

rshriram

Thanks for taking up this feature @ggreenway
How do you plan to handle the draining logic when a filterChain changes? Its almost as if the filterChain gets the listener level properties.

the stats also become interesting. The stats emitted inside that filterChain should ideally use the filterChain's name instead of the shared parent listener's name, if we are to distinguish what happened at each filter chain level.

Something worth keeping in mind is that we want this to be a functional replacement for the bindToPort+VirtualListener stuff we do in Istio (and some others as well).

ggreenway · 2019-01-03T18:30:42Z

@rshriram

How do you plan to handle the draining logic when a filterChain changes? Its almost as if the filterChain gets the listener level properties.

My plan was make only connections on a modified/removed filter chain need draining. So the logic will need to match up connections to which filter chain they're using and which version of the filter chain config.

the stats also become interesting. The stats emitted inside that filterChain should ideally use the filterChain's name instead of the shared parent listener's name, if we are to distinguish what happened at each filter chain level.

I agree, but think this issue is unrelated to this change. This is no more or less needed with this change than it was before (because we could already have had many many filter chains on a listener).

Something worth keeping in mind is that we want this to be a functional replacement for the bindToPort+VirtualListener stuff we do in Istio (and some others as well).

This is not my goal. If there are design tweaks needed to make that happen, please comment/review as this progresses.

ggreenway · 2019-01-09T20:08:44Z

As discussed in the linked issue (#4540) I'm going to start with sorting out updating filterchains without full listener draining. FCDS can then be built on top of that. Closing this for now.

config: Add FilterChain Discovery Service.

f7aedf8

Fixes: 4540 Signed-off-by: Greg Greenway <ggreenway@apple.com>

PiotrSikora reviewed Dec 20, 2018

View reviewed changes

alyssawilk assigned PiotrSikora and htuch Dec 21, 2018

Wiring up config subscription, init manager, start of an integration …

f24d3f3

…test Signed-off-by: Greg Greenway <ggreenway@apple.com>

htuch suggested changes Dec 24, 2018

View reviewed changes

PiotrSikora mentioned this pull request Dec 27, 2018

Un-deprecate bind_to_port and use_original_dst. #5355

Closed

stale bot added stale stalebot believes this issue/PR has not been touched recently and removed stale stalebot believes this issue/PR has not been touched recently labels Dec 31, 2018

envoyproxy deleted a comment from stale bot Jan 2, 2019

mattklein123 reviewed Jan 3, 2019

View reviewed changes

api/envoy/api/v2/lds.proto Show resolved Hide resolved

rshriram reviewed Jan 3, 2019

View reviewed changes

ggreenway closed this Jan 9, 2019


		// [#protodoc-title: Filter Chain Discovery Service]

		// The resource_names field in DiscoveryRequest specifies a listener.

WIP: config: Add FilterChain Discovery Service. #5385

WIP: config: Add FilterChain Discovery Service. #5385

Uh oh!

Conversation

ggreenway commented Dec 20, 2018

Uh oh!

ggreenway commented Dec 20, 2018

Uh oh!

PiotrSikora left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Dec 21, 2018

Uh oh!

PiotrSikora commented Dec 21, 2018

Uh oh!

snowp commented Dec 21, 2018

Uh oh!

AndresGuedez commented Dec 21, 2018

Uh oh!

ggreenway commented Dec 21, 2018

Uh oh!

htuch commented Dec 24, 2018

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jan 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Jan 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ggreenway Jan 7, 2019 •

edited

Loading

ggreenway commented Jan 3, 2019 •

edited

Loading