RFC: reorganize repo

[mattklein123]

I'm going to fill this in with an actual plan, but the rough goal is to do the following:

Move all statically registered components (filters, access loggers, stats, etc.) into an "extensions" directory. Under this directory we would have "experimental" and "stable." This would also more easily allow us to compile extensions in and out more easily.

The goal here is to be able to more clearly separate out extensions from core, and have discrete OWNERS for extensions.

cc @envoyproxy/maintainers

[mattklein123]

OK finally coming back to this. Here is what I concretely propose:

/source/extensions/<maturity_level>/access_loggers
/source/extensions/<maturity_level>/http_filters
/source/extensions/<maturity_level>/http_tracers
/source/extensions/<maturity_level>/listener_filters
/source/extensions/<maturity_level>/network_filters
/source/extensions/<maturity_level>/resolvers
/source/extensions/<maturity_level>/stat_sinks

1. where maturity level is either experimental or production.
2. where there is a parallel tree under /test.
3. where all config registration is colocated with the extensions.
4. where every extension has its own directory. e.g.,

/source/extensions/production/network_filters/tcp_proxy

The benefits of this approach are:

1. Allows us to start having OWNER files for extensions different from core maintainers.
2. Much easier to discover status of extensions.
3. Experimental extensions are marked as such.
4. Extension writers now have single directory to look through to find the extensions to look for examples.

I considered having a different tree outside of /source and /test but I'm not sure it's really worth the extra effort of fixing all the resultant build issues that assume this is where code is.

@envoyproxy/maintainers any thoughts?

[htuch]

I like the overall plan. How do you propose dealing with filters that have libraries with mockable components? We've tended to use include/envoy and test/mock here, how do you envisage this mapping into the new filter space?

Is there anything special we want to do to make it globally easy to condition in/out individual filters from the build in the general OSS distribution based on this organization? What about avoiding taking certain external dependencies that are irrelevant (LuaJIT, I'm looking at you!)?

I have a personal preference, which might not be universally shared, for shorter, more concise names where possible. Would a filters/listener, filters/network, etc. hierarchy make sense?

[mattklein123]

I like the overall plan. How do you propose dealing with filters that have libraries with mockable components? We've tended to use include/envoy and test/mock here, how do you envisage this mapping into the new filter space?

My thinking was to break this for extensions and just put abstract interfaces and mocks (if any) directly with the test code of the extension so that it is self contained. The one thing to think about is what if extensions end up having common code. We might need some type of common directory, allow extensions to refer to each other, or potentially require common code to go on "core" Envoy. Thoughts?

Is there anything special we want to do to make it globally easy to condition in/out individual filters from the build in the general OSS distribution based on this organization? What about avoiding taking certain external dependencies that are irrelevant (LuaJIT, I'm looking at you!)?

My thinking here is to work towards a place where every extension in the repo is governed by a bazel define that can turn it off. Thus, if no Lua filter is used, there doesn't need to be any Lua code. However, Lua already exposed the issue with common code. Does all Lua code go in extensions? Or just the filter parts?

I have a personal preference, which might not be universally shared, for shorter, more concise names where possible. Would a filters/listener, filters/network, etc. hierarchy make sense?

What do you mean exactly?

/source/extensions/<maturity_level>/access_loggers
/source/extensions/<maturity_level>/http_tracers
/source/extensions/<maturity_level>/filters/http
/source/extensions/<maturity_level>/filters/listener
/source/extensions/<maturity_level>/filters/network
/source/extensions/<maturity_level>/resolvers
/source/extensions/<maturity_level>/stat_sinks

? If so that sounds fine to me.

[alyssawilk]

Things I don't love but have no good solution for - I feel like it's going to be harder/more-annoying to find code, and I dislike files moving around when we promote filters. I don't see a good way around either so shrug

Mainly I have a bunch of questions :-)
Do we have definitions of what gets you over the bar to a production extension? Significant time in production by a heavy user? Active maintainer support? Do things move back out of production if they end up abandonware? I assume we're not lowering the bar for code / test for extensions directory but maybe we are if we allow a wider variety of OWNERS. Do we ever remove extensions from experimental if they don't get promoted? Maintenance cost is low if we don't extend APIs but if we have a policy anyone willing to own things can stick it in extensions, cost in subtle things like CI build time could end up being non-trivial.

[htuch]

My thinking here is to work towards a place where every extension in the repo is governed by a bazel define that can turn it off. Thus, if no Lua filter is used, there doesn't need to be any Lua code. However, Lua already exposed the issue with common code. Does all Lua code go in extensions? Or just the filter parts?

One thing to keep in mind is the growth of the binary itself. Do we want to keep Envoy suitable for very tight, resource constrained environments? E.g. in tiny containers or embedded applications? If so, it might be a good idea to have a plan to avoid unchecked growth in binary size. The model I'm thinking of is something closer to how the Linux kernel deals with drivers, with its config files. There are sensible minimal defaults which you can then opt-in on various features from.

I realize that if folks really want to strip it down they can today and in the future (just by creating distinct BUILD files), but making this convenient would be nice and could be accomplished somewhat structurally (e.g. by avoiding Lua code leaking into common) while we work on this specific issue.

[mattklein123]

Things I don't love but have no good solution for - I feel like it's going to be harder/more-annoying to find code, and I dislike files moving around when we promote filters. I don't see a good way around either so shrug

We can just drop <maturity_level> from the directory structure if we don't think it would provide enough value and handle in docs? That would be fine with me.

Do we have definitions of what gets you over the bar to a production extension? Significant time in production by a heavy user? Active maintainer support? Do things move back out of production if they end up abandonware? I assume we're not lowering the bar for code / test for extensions directory but maybe we are if we allow a wider variety of OWNERS. Do we ever remove extensions from experimental if they don't get promoted? Maintenance cost is low if we don't extend APIs but if we have a policy anyone willing to own things can stick it in extensions, cost in subtle things like CI build time could end up being non-trivial.

I agree these are all good questions which I don't really have answers for. My suggestion would be to just iterate what we have today into a new directory structure, and then go from there. We can initially do nothing different other than directory structure. If we decide to drop maturity level from the directory structure entirely, some of this becomes moot.

One thing to keep in mind is the growth of the binary itself. Do we want to keep Envoy suitable for very tight, resource constrained environments? E.g. in tiny containers or embedded applications? If so, it might be a good idea to have a plan to avoid unchecked growth in binary size. The model I'm thinking of is something closer to how the Linux kernel deals with drivers, with its config files. There are sensible minimal defaults which you can then opt-in on various features from.

I agree. Basically, as I said above IMO every extension in the future should have a bazel define. Some can even default to disabled if we want. I think the best course of action is to get the extensions into the new directory layout, and then start to figure out a general way to handle these types of defines. I'm guessing we can have some common Skylark that can help us here.

[dio]

Hi, I will try to play around with the proposal i.e. using the following pattern

/source/extensions/filters/http
/source/extensions/filters/network
...

and reflect that to the envoyproxy/envoy-filter-example, and will share the experience 😁

[mattklein123]

@dio thanks. FYI I'm planning on updating this issue this coming week with a final proposed plan.

[dio]

@mattklein123 thanks for the heads up. I saw that as one of the agenda for the next community call. Cool!

[alagalah]

Would this notion of "pluggable extensions" be extended to TransportSockets? (thinking about fd.io VPP integration, and could be applicable to SslTransportSocket?)

[ggreenway]

+1 on pluggable transport sockets

[mattklein123]

Yes definitely re: pluggable transport sockets. I will update the proposal to account for that also.

[mattklein123]

OK sorry for the radio silence here. The new proposal after chatting with various people is:

1. We will drop <maturity_level>. We can cover that via docs as needed. For now we will assume that anything in the core Envoy repo is sufficiently mature that it should be tested, adhere to standards, etc. In the future we might possible have some type of sandbox repo with additional extensions that are held to a lower standard.

2. There may still be a common directory inside extensions for code that is shared between extensions but is not used by core.

3. All config static registration will be colocated for easier inspection of how everything works.

4. Final directory layout:

/source/extensions/access_loggers
/source/extensions/http_tracers
/source/extensions/filters/http
/source/extensions/filters/listener
/source/extensions/filters/network
/source/extensions/resolvers
/source/extensions/stat_sinks
/source/extensions/transport_sockets

5. The general idea will be to allow every extension to have a bazel flag that can be used to disable compilation. This would allow for example easily not pulling in lightstep, OT, LuaJit, etc.

Any objections to this proposal? My plan is to get started on this once we release 1.6.0 early next week. I discussed offline with @jmarantz and he might also be interested in helping. I will coordinate with him.

[htuch]

@mattklein123 can you elaborate on (3)? Are you saying the factories for modules will still lives outside /source/extensions or are you only referring to the core factory code?

[alagalah]

IMHO dropping maturity level from the dir structure is a good idea. Less churn. Could be handled by build time defines I guess. i.e. "give me all the mature extensions" with still having per extension granularity. I'm happy to help if there is a particular task you want to farm out.

[mattklein123]

@htuch re: (3) my intention is to remove https://github.com/envoyproxy/envoy/tree/master/source/server/config and colocate the static registration with each extension. I think this will be a very nice improvement for readability.

@alagalah I had the same idea about build tagging. I think we should explore this in the future.

[ggreenway]

+1 on colocating registration with the extension.

[htuch]

+1 on proposal so far, but we need to also figure out what happens with test code. @dio has done some example implementation of this layout in https://github.com/envoyproxy/envoy-filter-example/pull/43/files. There are two features there of interest:

1. Source and test code are not colocated (same as Envoy structure today).
2. We now have two subtrees that reflect the extensions hierarchy, test/extensions and test/integration/extensions. The structuring of the integration tests is the first time we've imposed structure there. This is a nice cleanup IMHO.

I think this could work, and reflects existing Envoy source/test split. We could have OWNERS in the parallel trees. Call this the "core Envoy structure for extensions".

An alternative would be to have root level extensions/filters/http/foo/source, extensions/filters/http/foo/test, extensions/filters/http/foo/test/integration, etc. Call this "single root structure for extensions".

I think the single root structure has a nice property, where all code is colocated and under a single OWNERS. But, it's a weird inversion of the existing order.

I'm leaning towards existing structure as per envoyproxy/envoy-filter-example#43, but I wanted to throw it out there that we should clarify the test structure and consider the alternatives before locking the structure in.

[mattklein123]

In general, I have a preference for a "single root," but my concern was that it was going to break too many things which assume the code is in //source and //test. So I was planning on doing the former. Yes the integration cleanup sounds good to me. If people feel very strongly about "single root" we can look into what it would take to make that happen.

[alyssawilk]

FWIW I'm not averse to experimental / production, I really just didn't have a feel for what went there.
Most of the documented reasons for doing this were around the experimental / production split, with the last being around owners/reviewers. For that last issue, Would we have subdirectory-per-various filters and have /source/extensions//http_filters/lua owned by lua-savvy people or do more of per-file syntax?

I'd love if as a part of this move or a close follow-up we could have build rules / tools to make opting in and out of individual pluggable components easier, to head dependency bloat before we get there :-)

[htuch]

I'm not hearing any strong argument for single root. @mattklein123 @alyssawilk @ggreenway can you folks all take a look at envoyproxy/envoy-filter-example#43 and see if this matches what you envisage in the new layout? If so, we can merge that and we can move to applying this to the main repo.

[mattklein123]

Yes envoyproxy/envoy-filter-example#43 is essentially what I was planning on doing. I was planning on moving the echo filter first, since it's so simple. Probably on Friday. So perhaps I can do that and then we can look at that PR and envoyproxy/envoy-filter-example#43 together and then make a final call?

[ggreenway]

I have a mild preference for single root, with test code located near the thing it is testing, instead of a parallel directory structure for tests. But I don't feel at all strongly about it, so if everyone else is happy with this, I say go for it.

[alyssawilk]

I think the encoder/decoder split makes sense for the example code, but are we planning the same split for non-example code?

I'm with Greg that I mildly prefer code and tests to be colocated (test code is easy to flag in build rules with testonly) but will go with what folks prefer.

Otherwise 'tis fine by me

[mattklein123]

I think the encoder/decoder split makes sense for the example code, but are we planning the same split for non-example code?

I wasn't planning on it.

I'm with Greg that I mildly prefer code and tests to be colocated (test code is easy to flag in build rules with testonly) but will go with what folks prefer.

I don't have a strong preference on this one way or the other. The main issue I see with colocating test code with source code is that we don't do it anywhere else, so it's a bit strange. One middle ground would be to do:

/source/extensions/...
/test/extensions/...

But just put the integration tests colocated with the unit tests. That seems fine to me and would reduce one aspect of the split? Thoughts?

[alyssawilk]

Agree, if we wouldn't move the rest of the tests to be colocated it'd be sort of weird. I figured if we were moving around 80% of the code it was worth discussing the other 20% but no need to make this larger than it is :-P

I prefer keeping unit tests and integration tests separate, since integration tests often cover more than one thing, so of the two choices I'd lean towards keeping things as they're done in this PR.

[mattklein123]

Yeah I think this more like 20% code movement, vs. the other way around. OK SGTM. I will do the echo PR on Friday.

[mattklein123]

First stab at reorg done for echo filter here #2894. PTAL.

[htuch]

@alyssawilk @jmillikin-stripe and I just had our regular sync with the Bazel team and we covered this topic. The best suggestion so far is to continue our approach of using --define, but to create custom bazel.rc files that get fed into the Bazel CLI invocation to allow this to scale. The idea is that they would in turn be generated by a user facing configuration system or language (e.g. kernel config system, some YAML thing, whatever else is trendy) that generates the bazel.rc.

Making this work when Envoy is composed into something like envoy-filter-example and unifying the bazel.rc approach with explicit cc_binary deps for configuration is something that we should explore further.