Listener: Add listener filters. by jrajahalme · Pull Request #2346 · envoyproxy/envoy

jrajahalme · 2018-01-11T01:18:31Z

Network::ListenerImpl class implements an libevent socket
listener. Factor out all functionality that is not directly libevent
related to ConnectionHandlerImpl, which both reduces overall
complexity and makes it possible to refactor configurable listener
behavior to a new class of listener filters in the following patches.

This patch also reduces the special handling for SSL connections by
introducing a new dispatcher member for creating server
connections. This is used by the refactored code as the connection
creation is now shifted from Network::Listener to the connection
handler.

To further simplify the resulting code, this patch removes
Network::ListenerOptions class, as the only option that now needs to
be passed to Network::Listener is the "bind to port" boolean, which is
now passed in explicitly. All other listener options are handled by
passing along Server::Listener instead.

Add support for listener filters and refactor listener original dst
and proxy protocol features as listener filters.

Existing use_proxy_protocol and use_original_dst listener flags are
still supported by internally converting them to listener filters.

Signed-off-by: Jarno Rajahalme jarno@covalent.io

Risk Level: High
Testing: Existing unit tests are converted to use the new filter framework where applicable.
Docs Changes: TBD
Release Notes: TBD
API Changes: [Data Plane PR #400][https://github.com/envoyproxy/data-plane-api/pull/400]
Deprecated: TBD if the legacy flags should be removed from v2 API

Network::ListenerImpl class implements an libevent socket listener. Factor out all functionality that is not directly libevent related to ConnectionHandlerImpl, which both reduces overall complexity and makes it possible to refactor configurable listener behavior to a new class of listener filters in the following patches. This patch also reduces the special handling for SSL connections by introducing a new dispatcher member for creating server connections. This is used by the refactored code as the connection creation is now shifted from Network::Listener to the connection handler. To further simplify the resulting code, this patch removes Network::ListenerOptions class, as the only option that now needs to be passed to Network::Listener is the "bind to port" boolean, which is now passed in explicitly. All other listener options are handled by passing along Server::Listener instead. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Add support for listener filters and refactor listener original dst and proxy protocol features as listener filters. Existing use_proxy_protocol and use_original_dst listener flags are still supported by internally converting them to listener filters. As a further step we could remoe the legacy flags from the v2 APIs. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 · 2018-01-11T02:23:21Z

@PiotrSikora FYI this will have implications for the changes we were discussing today. I would wait for this to land first, otherwise you will need to rebase again. I will review this tomorrow.

PiotrSikora · 2018-01-11T02:34:14Z

@lizan Could you take a look and see if this conflicts or helps with your work on transport security?

jrajahalme · 2018-01-11T03:08:08Z

On Jan 10, 2018, at 6:34 PM, Piotr Sikora ***@***.***> wrote: @lizan <https://github.com/lizan> Could you take a look and see if this conflicts or helps with your work on transport security?

I briefly looked into the possibility of implementing SSL as a listener filter. IMO it seems possible using https://github.com/libevent/libevent/blob/master/bufferevent_openssl.c <https://github.com/libevent/libevent/blob/master/bufferevent_openssl.c> as long as Buffers will be evbuffers. Jarno

mattklein123 · 2018-01-11T03:16:36Z

I briefly looked into the possibility of implementing SSL as a listener filter. IMO it seems possible using https://github.com/libevent/libevent/blob/master/bufferevent_openssl.c https://github.com/libevent/libevent/blob/master/bufferevent_openssl.c as long as Buffers will be evbuffers.

We won't be going back to bufferevent, but @lizan is working on a bunch of refactoring around pluggable transport layers, and @PiotrSikora is working on sniffing TLS so that we can do filter chain matching based on sniffed data. (Basically have a listener support both plain-text and TLS). Then I'm working on v2 filter chain matching.

We just need to be careful here to line everything up. I'm hopeful we can sort it out in comments but it's possible we will need to have a quick meeting. Let's see how things go in comments tomorrow.

lizan · 2018-01-11T19:25:15Z

Took a rough pass on the PR, some of this PR (e.g. deleting addSslListener from dispatcher) will help me for upstream transport socket, and I don't think it really add difficulty. One thing that needs clarification is when a listener filter read/write from/to the fd, it might conflict with what the transport socket do later.

Though this certainly conflicts with my local working directory. I'd like to know how long this is going to take to merge, to better lining things up.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 · 2018-01-11T20:01:48Z

@lizan I would like to move this along since it needs to come in front of a bunch of other items we are all working on. I've gotten derailed by a few things today so reviewing this might not happen but I will review later or tomorrow. We will aim to iterate and merge this early next week.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

lizan · 2018-01-11T21:23:40Z

@mattklein123 SGTM, I'll hold my changes for a few days :)

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-12T03:55:31Z

I still have some fixing left due to the rebase, will sort out tomorrow. Should not block reviewing, though.

Change info to debubg level. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

@jrajahalme this is a pretty epic change! In general, I really like the direction. Thanks for working on this. This is going to take multiple passes. I have some comments to get started. I would like to figure out interface stuff first and kill v1 support from the PR. Once we sort out interfaces, we can dive into the implementation. I think it would be helpful to actually provide a bit of a more detailed overview of what the new interface hierarchy is, as it would help with review and sorting out the common/server interface issues.

mattklein123 · 2018-01-12T17:00:18Z

include/envoy/event/dispatcher.h

+   * @param accept_socket supplies a socket with an open file descriptor and connection metadata
+   *        to use for the connection. Takes ownership of the accept_socket.
+   * @param ssl_ctx supplies the SSL context to use, if not nullptr.
+   * @return Network::ConnectionPtr a client connection that is owned by the caller.


"a server connection"

mattklein123 · 2018-01-12T17:00:34Z

include/envoy/event/dispatcher.h

+   * @param ssl_ctx supplies the SSL context to use, if not nullptr.
+   * @return Network::ConnectionPtr a client connection that is owned by the caller.
+   */
+  virtual Network::ConnectionPtr createConnection(Network::AcceptSocketPtr&& accept_socket,


I would probably call this createServerConnection to match createClientConnection

Will change.

There is no ServerConnection class, ClientConnection inherits from Connection, so from that viewpoint createConnection() is correct. Hosts also have a createConnection method, so createServerConnection makes grepping the source easier.

mattklein123 · 2018-01-12T17:01:09Z

include/envoy/event/dispatcher.h

-  createSslListener(Network::ConnectionHandler& conn_handler, Ssl::ServerContext& ssl_ctx,
-                    Network::ListenSocket& socket, Network::ListenerCallbacks& cb,
-                    Stats::Scope& scope, const Network::ListenerOptions& listener_options) PURE;
+  virtual Network::ListenerPtr createListener(Network::ListenSocket& socket,


please fix doc comments above

OK, sorry for the sloppiness...

mattklein123 · 2018-01-12T17:01:59Z

include/envoy/network/connection_handler.h

+namespace Server {
+class Listener;
+}
+// XXX: Move to Server namespace?


yeah, from an abstraction level in general we try to make server code depend on common code, but not the other way around. Why was this necessary?

Listener config is defined in the Server namespace (Server::Listener), and the ConnectionHandler's addListener() now takes it as an argument. I did not want to include server headers from here, so I used a bare class declaration (in the correct namespace) to circumvent the need for the include.

Could also see if the Listener config class could be moved to the common code.

Yeah, let's just move Server::Listener (that interface only) into the Network namespace. That interface doesn't depend on anything else in Server. Please do that in a dedicated PR since it will be just code movement. We can review/merge that quickly and then rebase this.

Network namespace already has a Listener (the abstract base class, so something needs to be renamed. How about renaming the Server::Listener as Network::ListenerConfig, as it is just that, "A configuration for an individual listener", as the comment says?

Will do this and the filter directory split tomorrow, hopefully. In the meanwhile I updated this PR so that you may continue the review as you please :-)

mattklein123 · 2018-01-12T17:02:53Z

include/envoy/network/filter.h

+  virtual Event::Dispatcher& dispatcher() PURE;
+
+  /**
+   * If an filter stopped filter iteration by returning FilterStatus::StopIteration,


mattklein123 · 2018-01-12T17:14:43Z

source/common/config/well_known_names.h

+  const std::string PROXY_PROTOCOL = "envoy.proxy_protocol";
+
+  // Converts names from v1 to v2
+  const V1Converter v1_converter_;


v2 only please

mattklein123 · 2018-01-12T17:20:29Z

source/common/network/listen_socket_impl.h

+    return fd;
+  }
+
+  void clearReset() override { local_address_reset_ = false; }


This existence/semantics of the clearReset() function is strange to me. It doesn't apply to remote, and it leaves the addresses to what they were reset to. This feels like a hack to me. Can you explain why this is needed and if there is a cleaner way to handle the need?

I was never happy with the name of this function either.

This is needed for handing off the accepted socket from one listener to another, which is needed for backwards compatibility in the case where the first listener listens on a redirected port and the second listener matches the original local address but does not bind.

Clearing of the local_address_reset_ bit is needed so that the second listener would not try to repeat the same check again (in an infinite recursion). See the call site in source/server/connection_handler_impl.cc.

This implementation also allows the second listener have and run it's own set of listener filters on the accepted socket it is given.

So the functionality is correct and needed. We should rename this function to lessen the likelihood of confusion in the future, though.

OK. I would probably switch reset with "change" and use the following function names (or some variant of them):

changeLocalAddress(...)

localAddressChanged()

forgetOriginalLocalAddress() (This should ASSERT that the local address was changed when called).

Etc.

And more heavily comment the interface. Feel free to iterate on my suggestions above, but that is more clear to me.

Just realized that the resetting the local_address_reset_ bit when passing the socket from one listener to another would break the original dst cluster from being able to forward the connection, so I'll have to figure alternative way of not recursing from the second listener. I don't see a need for using an original destination cluster in conjunction of non-bound listeners like this, but I don't like the way this is now anyway, so I'll spend some time on this. I'll try to come up with something that also addresses your concerns here and does not need heavy commenting :-)

mattklein123 · 2018-01-12T17:21:15Z

source/server/config/network/original_dst.cc

@@ -0,0 +1,47 @@
+#include <string>


Please move the configs to a listener folder, like the filters.

mattklein123 · 2018-01-12T17:22:48Z

source/server/connection_handler_impl.h

    ~ActiveListener();

+    // Network::ListenerCallbacks
+    /**


nit: don't need to duplicate doc comments here

Made them more specific.

mattklein123 · 2018-01-12T17:23:55Z

source/server/listener_manager_impl.cc

+            string_name);
+    if (factory != nullptr) {
+      Configuration::ListenerFilterFactoryCb callback;
+      if (filter_config->getBoolean("deprecated_v1", false)) {


v2 only please

ggreenway · 2018-01-12T17:39:23Z

I looked through this, and I wonder if it should be reworked so that there is a connection object of some type (a thing to hold the fd, and handle reading from it) is created as soon as the connection is accepted. It would then call the new set of filters, then match which logical listener and/or filter chain to use, then proceed.

The piece about this I dislike, which made me think of the above, is that the proxy protocol filter has to implement reading from the socket directly. I imagine other listener filters will need to do the same.

The other thing I think we'll want eventually, is for one of these new filters to read some data, look at it, and then pass it along (modified or not) to the network filters. This could be done with MSG_PEEK, but that seems like a hack.

Thoughts?

mattklein123 · 2018-01-12T17:49:09Z

The piece about this I dislike, which made me think of the above, is that the proxy protocol filter has to implement reading from the socket directly. I imagine other listener filters will need to do the same.

Yes hiding the fd behind some interface (possibly not the full connection interface, something like PreConnectionHandle or something SGTM. I don't think though that it changes the overall architecture of the PR, but I agree it would be an improvement over what we were doing previously.

lizan · 2018-01-12T22:59:53Z

include/envoy/server/filter_config.h

+   * @param context supplies the filter's context.
+   * @return ListenerFilterFactoryCb the factory creation function.
+   */
+  virtual ListenerFilterFactoryCb createFilterFactory(const Json::Object& config,


If we go v2 only, no necessary of this method

OK, removed.

lizan · 2018-01-12T23:00:04Z

include/envoy/server/filter_config.h

+   *         JSON and then parsed into this empty proto. Optional today, will be compulsory when v1
+   *         is deprecated.
+   */
+  virtual ProtobufTypes::MessagePtr createEmptyConfigProto() { return nullptr; }


this should be a PURE method.

Right, fixed!

lizan · 2018-01-12T23:05:16Z

source/server/config/network/original_dst.cc

+  }
+
+  ProtobufTypes::MessagePtr createEmptyConfigProto() override {
+    return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Empty()};


nit: std::make_unique

Envoy::ProtobufWkt::Empty() is a more specific type than ProtobufTypes::Message, do you think using std::make_unique would make this easier on the eyes?

lizan · 2018-01-12T23:06:08Z

source/server/connection_handler_impl.cc

-  ActiveListenerPtr l(new SslActiveListener(*this, ssl_ctx, socket, factory, scope, listener_tag,
-                                            listener_options));
-  listeners_.emplace_back(socket.localAddress(), std::move(l));
+void ConnectionHandlerImpl::addListener(Listener& config) {


Thanks for consolidating addListner and addSslListener, neat!

lizan · 2018-01-12T23:08:26Z

source/server/connection_handler_impl.h

+  /**
+   * Wrapper for an active accept socket owned by this handler.
+   */
+  struct ActiveSocket : public Network::ListenerFilterManager,


nit: this still have to be a class, per Google Style Guide (no deviation in envoy style for this):

Use a struct only for passive objects that carry data; everything else is a class.

I tried this but this happened even when I made all members public:

source/server/connection_handler_impl.cc: In destructor 'virtual Envoy::Server::ConnectionHandlerImpl::ActiveListener::~ActiveListener()': source/server/connection_handler_impl.cc:72:72: error: 'std::unique_ptr<_Tp> Envoy::LinkedObject<T>::removeFromList(Envoy::LinkedObject<T>::ListType&) [with T = Envoy::Server::ConnectionHandlerImpl::ActiveSocket; Envoy::LinkedObject<T>::ListType = std::__cxx11::list<std::unique_ptr<Envoy::Server::ConnectionHandlerImpl::ActiveSocket> >]' is inaccessible within this context ActiveSocketPtr removed = sockets_.front()->removeFromList(sockets_);

Maybe there is a reason other structs like this also exist in this file?

lizan · 2018-01-12T23:09:56Z

source/server/listener_manager_impl.cc

+    Configuration::FactoryContext& context) {
+  std::vector<Configuration::ListenerFilterFactoryCb> ret;
+  for (ssize_t i = 0; i < filters.size(); i++) {
+    const std::string string_type = filters[i].deprecated_v1().type();


nit: ProtobufTypes::String

lizan · 2018-01-12T23:11:25Z

source/server/listener_manager_impl.h

-  // Server::ListenSocketFactory
+  // Server::ListenerComponentFactory
  std::vector<Configuration::NetworkFilterFactoryCb>
  createFilterFactoryList(const Protobuf::RepeatedPtrField<envoy::api::v2::Filter>& filters,


rename this to createNetworkFilterFactoryList?

lizan · 2018-01-12T23:13:17Z

include/envoy/network/filter.h

+   * @return true if filter chain was created successfully. Otherwise
+   *   false.
+   */
+  virtual bool createFilterChain(ListenerFilterManager& listener) PURE;


createListnerFilterChain for this and createNetworkFilterChain for the one above, this shouldn't be an overloaded.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Sync with the API change to a separate ListenerFilter type. Drop API v1 support. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Rename createConnection() as createServerConnection(), rhymes with createClientConnection(). Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Some of the tests depended on the timing of data arraving to the socket read by the proxy protocol filter. Make all tests run the dispatcher in blocking mode, and stop the dispatcher when we know that the proxy protocol processing has finished. These changes allow these tests to pass on OSX, and also prevent spurious test failures on other platforms. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-20T05:51:28Z

Proxy protocol tests depended on OS timing w.r.t socket buffering and I/O. Converted tests that did this to run the dispatched until the read filter gets callbacks to make the test runs deterministic. With this I got the proxy protocol test succeed locally on Mac as well as Linux.

jrajahalme · 2018-01-22T19:47:58Z

On Jan 19, 2018, at 21:51, jrajahalme ***@***.***> wrote: Proxy protocol tests depended on OS timing w.r.t socket buffering and I/O. Converted tests that did this to run the dispatched until the read filter gets callbacks to make the test runs deterministic. With this I got the proxy protocol test succeed locally on Mac as well as Linux.

Should mention that this test likely has been failing or flaky for MAC on the master branch as well for the same reasons.

mattklein123

Thanks, this is a really awesome change. I love the way that it cleans up a bunch of special case logic. I just have a random assortment of minor comments. Let's finalize the data-plane-api PR and then we can close this out and get it merged!

mattklein123 · 2018-01-22T21:03:30Z

include/envoy/network/listen_socket.h

+   *        different from the one the socket was initially accepted at. This should only be set
+   *        to 'true' when restoring the original destination address of a connection redirected
+   *        by iptables REDIRECT. The caller is responsible for making sure the new address is
+   *        actually different when passing restored as 'true'.


Can you also give an example of when someone would call this function but set restored to false? I see multiple examples in this PR but just from reading the interfaces it's not completely clear when one would do that.

I reverted the meaning of this boolean and renamed it as "hand_off_restored_destinations", should be clearer now, see the new commit.

mattklein123 · 2018-01-22T21:12:11Z

source/common/filter/listener/proxy_protocol.h

+   */
+  bool readLine(int fd, std::string& s);
+
+  Network::ListenerFilterCallbacks* cb_;


initialize to {}

mattklein123 · 2018-01-22T21:12:58Z

source/common/filter/listener/proxy_protocol.h

+  size_t buf_off_{};
+
+  // The index in buf_ where the search for '\r\n' should continue from
+  size_t search_index_;


not your code but I would move the {1} initializer here for consistency.

mattklein123 · 2018-01-22T21:13:38Z

source/common/network/connection_impl.cc

+    : ConnectionImpl(dispatcher, std::make_unique<ClientSocketImpl>(remote_address),
+                     std::move(transport_socket), false) {
+  if (source_address != nullptr) {
+    int rc = source_address->bind(fd());


mattklein123 · 2018-01-22T21:14:05Z

source/common/network/connection_impl.cc

+
+void ClientConnectionImpl::connect() {
+  ENVOY_CONN_LOG(debug, "connecting to {}", *this, socket_->remoteAddress()->asString());
+  int rc = socket_->remoteAddress()->connect(fd());


mattklein123 · 2018-01-22T21:22:32Z

source/server/connection_handler_impl.cc

+  // Purge sockets that have not progressed to connections. This should only happen when
+  // a listener filter stops iteration and never resumes.
+  while (!sockets_.empty()) {
+    ActiveSocketPtr removed = sockets_.front()->removeFromList(sockets_);


Do you have test coverage for this case?

proxy_protocol_test.cc should exercise this, it has tests for invalid/truncated proxy protocol messages.

mattklein123 · 2018-01-22T21:30:41Z

source/server/connection_handler_impl.h

+
+    ActiveListener& listener_;
+    Network::ConnectionSocketPtr socket_;
+    bool redirected_;


mattklein123 · 2018-01-22T21:33:27Z

source/server/listener_manager_impl.cc

  // filter chain #1308.
  ASSERT(config.filter_chains().size() >= 1);

+  if (config.listener_filters().size()) {


nit: !config.listener_filters().empty()

mattklein123 · 2018-01-22T21:34:24Z

source/server/listener_manager_impl.cc

+    listener_filter_factories_ =
+        parent_.factory_.createListenerFilterFactoryList(config.listener_filters(), *this);
+  }
+  // Add original dst listener filter if 'use_original_dst' flag is set.


Per my comment in the data-plane-api repo, can we deprecate the use_original_dst field in DEPRECATED.md and docs, and push people towards using the filter directly.

mattklein123 · 2018-01-22T21:36:44Z

source/server/listener_manager_impl.cc

+  //                   is implemented, this needs to be run after the filter chain has been
+  //                   selected.
+  if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config.filter_chains()[0], use_proxy_proto, false)) {
+    auto& factory =


I'm wondering if we should even register proxy proto as a factory, given that in the future we aren't going to use it as a listener filter. I don't think it's a big deal either way, but just throwing it out there for thoughts.

IMO, I would leave the door open to use it as a listener filter as well, in case someone must do filter chain matching after proxy protocol has restored the destination address.

jrajahalme · 2018-01-23T02:11:07Z

@mattklein123 Thanks for your thorough review! I'll post commits addressing your comments right after the test run I'm doing locally finishes.

jrajahalme · 2018-01-23T02:13:00Z

Will also merge master to resolve the conflict.

Deprecate 'use_proxy_protocol' boolean from the v2 LDS API, as the corresponding functionality can be attained with listener filters and filter chain matching. Handing off from one listener to another is supported for v2 API only until 'use_proxy_protocol' flag is removed. After that v2 API users have to use filter chain matching to implement filter chain selection. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-01-23T02:47:08Z

Don't have the time to wait for the test run locally, hoping this tests OK.

mattklein123

LGTM, a few small comments. Please merge master in with the real data-plane-api change.

mattklein123 · 2018-01-23T17:58:32Z

source/common/network/listener_impl.h


  Address::InstanceConstSharedPtr local_address_;
  ListenerCallbacks& cb_;
+  bool hand_off_restored_destinations_;


I have to learn this someday :-) Btw. this factors in to the discussion about this being plural or not in the other comment..

mattklein123 · 2018-01-23T17:58:53Z

include/envoy/network/listener.h

+  /**
+   * @return bool if a connection should be handed off to another Listener after the original
+   *         destination address has been restored. 'true' when 'use_original_dst' flag in listener
+   *         configuration is set, false otherwise. Note that this flag will be removed from the v2


can you explicitly use the word "deprecated" in here somewhere so that when we search for deprecated things this comes up?

mattklein123 · 2018-01-23T18:00:49Z

include/envoy/network/listener.h

+   *         configuration is set, false otherwise. Note that this flag will be removed from the v2
+   *         API.
+   */
+  virtual bool handOffRestoredDestinations() const PURE;


Should this be not plural? handOffRestoredDestination() ? And same with all variables similarly named?

A listener accepts many connections, each of which has a destination address, and the listener will either hand all restored destinations off to another listener (if found) or not. To me this calls for a plural, but I'm happy to change it you don't buy this line of reasoning.

OK makes sense. I guess I would probably name it handOffRestoredDestinationConnections(), but I don't feel that strongly about it.

OK, I'll change this, hold on for a while :-)

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

…nections(). Remame handOffRestoredDestinations() as handOffRestoredDestinationConnections(). Corresponding rename for parameter and member variable names, too. Constify the 'bool hand_off_restored_destination_connections_' member variable in tests. Suggested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123

Awesome work. Thank you!

jrajahalme · 2018-01-23T23:23:22Z

I've been running the starting point of this PR for a couple of months now, but still positively surprised by the code quality & readability increase during the review process! Thanks for keeping a to it for the last two weeks!

mattklein123 · 2018-01-23T23:35:44Z

I've been running the starting point of this PR for a couple of months now, but still positively surprised by the code quality & readability increase during the review process! Thanks for keeping a to it for the last two weeks!

Thank you for being a good sport! :)

We should not have default parameter values on virtual functions, but this one slipped through. Fixes: eb02089 ("Listener: Add listener filters. (envoyproxy#2346)") Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

…2573) We should not have default parameter values on virtual functions, but this one slipped through. Fixes: eb02089 ("Listener: Add listener filters. (#2346)") Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

To pull in bazelbuild/rules_apple#1488, which should fix our CocoaPods integration and allow us to re-add it. Risk Level: Medium for iOS users who use the xcframework, low for everyone else Testing: There's a recent CI job that validates SwiftPM integration that acts as a good integration test for this Docs Changes: None for now, this isn't really a user-facing change, as Xcode/SwiftPM handles both the old and new xcframework format/layout transparently Release Notes: None for now, this isn't really a user-facing change Signed-off-by: JP Simard <jp@jpsim.com>

jrajahalme added 2 commits January 10, 2018 14:43

jrajahalme mentioned this pull request Jan 11, 2018

lds: Add listener_filter_chain. envoyproxy/data-plane-api#400

Merged

jrajahalme added 2 commits January 11, 2018 11:41

Merge branch 'master' into listener-filters

ee60ccf

Style fixes.

6874568

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme added 2 commits January 11, 2018 12:28

test: Add missing 'override' keyword.

bc51bf8

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

test: Remove unused 'this' lambda capture.

0a8fd6f

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

test: Fix format.

da0c39b

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme added 2 commits January 11, 2018 19:58

filter/original_dst: Increase log level.

044a089

Change info to debubg level. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

listener filters: Implement proto initializers.

d97257a

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 12, 2018

View reviewed changes

mattklein123 self-assigned this Jan 12, 2018

lizan reviewed Jan 12, 2018

View reviewed changes

jrajahalme added 7 commits January 12, 2018 19:14

Merge branch 'master' into listener-filters

efdf2f1

Merge branch 'master' into listener-filters

31ea8a0

test: Add Listener filter mocks.

c6f4fb2

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

listener: Use ListenerFilter type, drop v1 support.

a7845db

Sync with the API change to a separate ListenerFilter type. Drop API v1 support. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

include: Fix comments.

f9c5650

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

dispatcher: Rename createConnection() as createServerConnection().

d42ce02

Rename createConnection() as createServerConnection(), rhymes with createClientConnection(). Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Listener: Rename AcceptSocket class as AcceptedSocket

aa81333

Requested-by: Matt Klein <mklein@lyft.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 22, 2018

View reviewed changes

jrajahalme added 3 commits January 22, 2018 18:29

Merge branch 'master' into listener-filters

1021213

Review fixes.

ae07e0f

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

mattklein123 reviewed Jan 23, 2018

View reviewed changes

jrajahalme added 3 commits January 23, 2018 12:09

Merge branch 'master' into listener-filters

393e2bc

Last nits.

f5c19a9

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

lizan mentioned this pull request Jan 23, 2018

network: add DownstreamTransportSocketConfigFactory and registry #2437

Merged

mattklein123 approved these changes Jan 23, 2018

View reviewed changes

mattklein123 merged commit eb02089 into envoyproxy:master Jan 23, 2018

jrajahalme mentioned this pull request Feb 9, 2018

listen_socket: Remove default value from virtual function parameter. #2573

Merged

htuch mentioned this pull request Mar 26, 2018

[v1.6.0 deprecation] Remove features marked deprecated in #2346 #2901

Closed

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

Update Envoy-WASM SHA to latest. (envoyproxy#2346)

048890f

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Conversation

jrajahalme commented Jan 11, 2018

Uh oh!

mattklein123 commented Jan 11, 2018

Uh oh!

PiotrSikora commented Jan 11, 2018

Uh oh!

jrajahalme commented Jan 11, 2018 via email

Uh oh!

mattklein123 commented Jan 11, 2018

Uh oh!

lizan commented Jan 11, 2018

Uh oh!

mattklein123 commented Jan 11, 2018

Uh oh!

lizan commented Jan 11, 2018

Uh oh!

jrajahalme commented Jan 12, 2018

Uh oh!

mattklein123 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jan 12, 2018

Uh oh!

mattklein123 commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mattklein123 commented Jan 12, 2018 •

edited

Loading