envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
25.13k stars 4.82k forks source link

Separate out Envoy platform API from extension API #5126

Open htuch opened 6 years ago

htuch commented 6 years ago

We currently have a bit of conflation in api/ between those APIs that are provided a platform abstraction layer (mostly for the benefit of core Envoy, but also extensions) and the APIs that will one day act as a stable interface for extensions. For example:

https://github.com/envoyproxy/envoy/blob/2c0733ad3ffaf3e1630e3a77e822c5d76ad08f0b/include/envoy/api/api.h#L28

is clearly in the second category, and

https://github.com/envoyproxy/envoy/blob/2c0733ad3ffaf3e1630e3a77e822c5d76ad08f0b/include/envoy/api/api.h#L54

is in the first. This becomes even stranger when we have the ApiImpl require access to a stats object in

https://github.com/envoyproxy/envoy/blob/2c0733ad3ffaf3e1630e3a77e822c5d76ad08f0b/source/common/api/api_impl.h#L22

This is because the filesystem code is both a platform abstraction API and also providing higher level functionality like stats tracking.

Ideally, we split out the API into a api/platform and api/export, where we clearly delineate these two concerns. New platform ports of Envoy populate the interfaces in api/platform, extensions take dependencies on api/export.

CC @jmarantz @eziskind @mattklein123

mattklein123 commented 6 years ago

+1 from me. Also related to https://github.com/envoyproxy/envoy/issues/827 and https://github.com/envoyproxy/envoy/issues/3390.

jmarantz commented 6 years ago

I didn't realize the purpose of the API was just to provide a stable interface to extensions; I thought it also encapsulated platform functions on behalf of core code as well. If that's true should we think of a slightly different name from api/export?

But +1 on separating the platform API from the additions to Envoy. In the past I've used some variation on the decorator pattern. So for Filesystem we'd have:

Filesystem: abstract base class FilesystemImpl: wrapper providing OO wrapper around open/close/read/write FlushingFilesystem: ctor takes a Filesystem& and a ThreadFactory& and adds periodic flushing StatsFilesystem: takes a Filesystem& and Stats::Store& and adds stat-tracking.

The FilesytemImpl would be contained as part of the PlatformAPI object. Alternate FilesystemImpl implementations should be straightforward to plug in (e.g. that wrap the Windows API for file i/o or provide a pure in-memory filesystem for tests).

The aggregated flushing/stats-taking filesystem would be contained as part of the ExportAPI object.

Similarly I think it might be interesting to take stats on threads, in which case we'd want to do it in a decorator class rather than in ThreadFactoryImpl -- which will have multiple alternate impls (eg Windows, Posix)

jmarantz commented 6 years ago

Also I would propose we move TimeSystem/TimeSource into API. Many places where TImeSystem& or TimeSource& is plumbed through, API& is as well, and it would seem better to use API as a conduit.

jmarantz commented 5 years ago

@jstordeur fyi