Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/android_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -110,5 +110,5 @@ jobs:
--test_output=all \
--build_tests_only \
$([ -z $GITHUB_TOKEN ] || echo "--config=remote-ci-linux-clang") \
--remote_header="Authorization=Bearer $GITHUB_TOKEN" \
--copt=-DALLOW_TCP_LISTENERS --remote_header="Authorization=Bearer $GITHUB_TOKEN" \

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be moved to a .bazelrc config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal here is to have E-M build without TCP listeners, but we exempt it for this CI due to the proxying tests. will comment inline.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then these tests will fail when run locally?

Why not add build test --copt=-DALLOW_TCP_LISTENERS to the .bazelrc so it works both on CI and locally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because then all tests will always run with TCP listeners, and we won't have regression testing for, say, the C++ tests running without TCP listeners.

What I'm hearing is you'd prefer I add it to the bazelrc, and explicitly not allow TCP listeners on all the non-legacy CI instead?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah I was thinking a config that we could pass when running these tests specifically, but then it doesn't make much difference if you have to pass a config or a --copt because either way ./bazelw test ... won't be sufficient.

I'm ok keeping this as-is then.

//test/kotlin/...
5 changes: 5 additions & 0 deletions envoy/server/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ class Options {
* responsibility of the caller to handle the duplicates.
*/
virtual const Stats::TagVector& statsTags() const PURE;

/**
* @return the type of listener manager to create.
*/
virtual const std::string& listenerManager() const PURE;
Comment thread
alyssawilk marked this conversation as resolved.
};

} // namespace Server
Expand Down
1 change: 1 addition & 0 deletions mobile/envoy_build_config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ envoy_cc_library(
"@envoy_mobile//library/common/extensions/filters/http/platform_bridge:config",
"@envoy_mobile//library/common/extensions/filters/http/route_cache_reset:config",
"@envoy_mobile//library/common/extensions/filters/http/socket_tag:config",
"@envoy_mobile//library/common/extensions/listener_managers/api_listener_manager:api_listener_manager_lib",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

similar to my other comment - should we include this dep only if the new compiler/build flag is set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends. if the long term plan is to have a bazel flag to have it optional, we can add it now. if long term we want to say E-M is a client library not a proxy, I'm inclined to not do the busy-work which I'd then have to unwind, and accept the additional include in the interim (we can remove it in-housE)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually having thought a bit more I think it is sensible to have a flag but I would like to add it when we flip the default, so have one for "enable TCP listener" where right now we have compile support for "enable API listener"

"@envoy_mobile//library/common/extensions/retry/options/network_configuration:config",
],
)
Expand Down
2 changes: 2 additions & 0 deletions mobile/envoy_build_config/extension_registry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include "library/common/extensions/filters/http/network_configuration/config.h"
#include "library/common/extensions/filters/http/platform_bridge/config.h"
#include "library/common/extensions/filters/http/route_cache_reset/config.h"
#include "library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h"
#include "library/common/extensions/retry/options/network_configuration/config.h"

namespace Envoy {
Expand Down Expand Up @@ -75,6 +76,7 @@ void ExtensionRegistry::registerFactories() {
Router::forceRegisterUpstreamCodecFilterFactory();
Envoy::Network::forceRegisterGetAddrInfoDnsResolverFactory();
Envoy::Extensions::RequestId::forceRegisterUUIDRequestIDExtensionFactory();
Envoy::Server::forceRegisterApiListenerManagerFactoryImpl();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we force register in here only if DUSE_API_LISTENER is set?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to do that but I think it requires a bazel flag and additional plumbing to be useful given the library include


// TODO: add a "force initialize" function to the upstream code, or clean up the upstream code
// in such a way that does not depend on the statically initialized variable.
Expand Down
2 changes: 2 additions & 0 deletions mobile/envoy_build_config/extensions_build_config.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ EXTENSIONS = {
"envoy.transport_sockets.tls": "//source/extensions/transport_sockets/tls:config",
"envoy.http.stateful_header_formatters.preserve_case": "//source/extensions/http/header_formatters/preserve_case:config",
"envoy_mobile.cert_validator.platform_bridge_cert_validator": "@envoy_mobile//library/common/extensions/cert_validator/platform_bridge:config",
"envoy.listener_manager_impl.api": "@envoy_mobile//library/common/extensions/listener_managers/api_listener_manager:api_listener_manager_lib",

}
WINDOWS_EXTENSIONS = {}
LEGACY_ALWAYSLINK = 1
9 changes: 8 additions & 1 deletion mobile/library/common/engine_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,15 @@

namespace Envoy {

// Allows for legacy TCP listener support when building with --copt=-DALLOW_TCP_LISTENERS
#ifndef ALLOW_TCP_LISTENERS
const char* listener_type = "envoy.listener_manager_impl.api";
#else
const char* listener_type = "envoy.listener_manager_impl.default";
#endif

EngineCommon::EngineCommon(int argc, const char* const* argv)
: options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info),
: options_(argc, argv, &MainCommon::hotRestartVersion, spdlog::level::info, listener_type),
base_(options_, real_time_system_, default_listener_hooks_, prod_component_factory_,
std::make_unique<PlatformImpl>(), std::make_unique<Random::RandomGeneratorImpl>(),
nullptr) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
load(
"@envoy//bazel:envoy_build_system.bzl",
"envoy_cc_extension",
"envoy_extension_package",
)

licenses(["notice"]) # Apache 2

envoy_extension_package()

envoy_cc_extension(
name = "api_listener_manager_lib",
srcs = [
"api_listener_manager.cc",
],
hdrs = [
"api_listener_manager.h",
],
repository = "@envoy",
deps = [
"@envoy//envoy/server:instance_interface",
"@envoy//envoy/server:listener_manager_interface",
"@envoy//source/server:api_listener_lib",
"@envoy//source/server:listener_manager_factory_lib",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
#include "library/common/extensions/listener_managers/api_listener_manager/api_listener_manager.h"

#include "envoy/config/listener/v3/listener.pb.h"
#include "envoy/network/listener.h"

#include "source/common/common/assert.h"
#include "source/common/common/fmt.h"
#include "source/common/config/utility.h"
#include "source/server/api_listener_impl.h"

namespace Envoy {
namespace Server {

ApiListenerManagerImpl::ApiListenerManagerImpl(Instance& server) : server_(server) {}
bool ApiListenerManagerImpl::addOrUpdateListener(
const envoy::config::listener::v3::Listener& config, const std::string&, bool added_via_api) {
std::string name;
if (!config.name().empty()) {
name = config.name();
} else {
// TODO (soulxu): The random uuid name is bad for logging. We can use listening addresses in
// the log to improve that.
name = server_.api().randomGenerator().uuid();
}

// TODO(junr03): currently only one ApiListener can be installed via bootstrap to avoid having to
// build a collection of listeners, and to have to be able to warm and drain the listeners. In the
// future allow multiple ApiListeners, and allow them to be created via LDS as well as bootstrap.
if (config.has_api_listener()) {
if (config.has_internal_listener()) {
throw EnvoyException(fmt::format(
"error adding listener named '{}': api_listener and internal_listener cannot be both set",
name));
}
if (!api_listener_ && !added_via_api) {
// TODO(junr03): dispatch to different concrete constructors when there are other
// ApiListenerImplBase derived classes.
api_listener_ = std::make_unique<HttpApiListener>(config, server_, config.name());
return true;
} else {
ENVOY_LOG(warn, "listener {} can not be added because currently only one ApiListener is "
"allowed, and it can only be added via bootstrap configuration");
return false;
}
}
return false;
}

REGISTER_FACTORY(ApiListenerManagerFactoryImpl, ListenerManagerFactory);

} // namespace Server
} // namespace Envoy
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#pragma once

#include <memory>

#include "envoy/server/instance.h"
#include "envoy/server/listener_manager.h"

#include "source/server/listener_manager_factory.h"

namespace Envoy {
namespace Server {

/**
* Implementation of a lightweight ListenerManager for Envoy Mobile.
* This does not handle downstream TCP / UDP connections but only the API listener.
*/
class ApiListenerManagerImpl : public ListenerManager, Logger::Loggable<Logger::Id::config> {
public:
ApiListenerManagerImpl(Instance& server);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

explicit


// Server::ListenerManager
bool addOrUpdateListener(const envoy::config::listener::v3::Listener& config,
const std::string& version_info, bool added_via_api) override;
void createLdsApi(const envoy::config::core::v3::ConfigSource&,
const xds::core::v3::ResourceLocator*) override {}
std::vector<std::reference_wrapper<Network::ListenerConfig>> listeners(ListenerState) override {
return {};
}
uint64_t numConnections() const override { return 0; }
bool removeListener(const std::string&) override { return true; }
void startWorkers(GuardDog&, std::function<void()> callback) override { callback(); }
void stopListeners(StopListenersType) override {}
void stopWorkers() override {}
void beginListenerUpdate() override {}
void endListenerUpdate(FailureStates&&) override {}
bool isWorkerStarted() override { return true; }
Http::Context& httpContext() { return server_.httpContext(); }
ApiListenerOptRef apiListener() override {
return api_listener_ ? ApiListenerOptRef(std::ref(*api_listener_)) : absl::nullopt;
}

private:
Instance& server_;
ApiListenerPtr api_listener_;
};

class ApiListenerManagerFactoryImpl : public ListenerManagerFactory {
public:
std::unique_ptr<ListenerManager>
createListenerManager(Instance& server, std::unique_ptr<ListenerComponentFactory>&&,
WorkerFactory&, bool, Quic::QuicStatNames&) override {
return std::make_unique<ApiListenerManagerImpl>(server);
}
std::string name() const override { return "envoy.listener_manager_impl.api"; }
};

DECLARE_FACTORY(ApiListenerManagerFactoryImpl);

} // namespace Server
} // namespace Envoy
1 change: 1 addition & 0 deletions source/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ envoy_cc_library(
name = "listener_manager_factory_lib",
hdrs = ["listener_manager_factory.h"],
deps = [
":worker_lib",
"//envoy/server:factory_context_interface",
"//envoy/server:listener_manager_interface",
"//source/common/quic:quic_stat_names_lib",
Expand Down
10 changes: 7 additions & 3 deletions source/server/options_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,16 @@ std::vector<std::string> toArgsVector(int argc, const char* const* argv) {

OptionsImpl::OptionsImpl(int argc, const char* const* argv,
const HotRestartVersionCb& hot_restart_version_cb,
spdlog::level::level_enum default_log_level)
: OptionsImpl(toArgsVector(argc, argv), hot_restart_version_cb, default_log_level) {}
spdlog::level::level_enum default_log_level,
absl::string_view listener_manager)
: OptionsImpl(toArgsVector(argc, argv), hot_restart_version_cb, default_log_level,
listener_manager) {}

OptionsImpl::OptionsImpl(std::vector<std::string> args,
const HotRestartVersionCb& hot_restart_version_cb,
spdlog::level::level_enum default_log_level) {
spdlog::level::level_enum default_log_level,
absl::string_view listener_manager)
: listener_manager_(listener_manager) {
std::string log_levels_string = fmt::format("Log levels: {}", allowedLogLevels());
log_levels_string +=
fmt::format("\nDefault is [{}]", spdlog::level::level_string_views[default_log_level]);
Expand Down
8 changes: 6 additions & 2 deletions source/server/options_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
* value). The caller should call exit(1) after any necessary cleanup.
*/
OptionsImpl(int argc, const char* const* argv, const HotRestartVersionCb& hot_restart_version_cb,
spdlog::level::level_enum default_log_level);
spdlog::level::level_enum default_log_level,
absl::string_view listener_manager = "envoy.listener_manager_impl.default");

/**
* @throw NoServingException if Envoy has already done everything specified by the args (e.g.
Expand All @@ -42,7 +43,8 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
* value). The caller should call exit(1) after any necessary cleanup.
*/
OptionsImpl(std::vector<std::string> args, const HotRestartVersionCb& hot_restart_version_cb,
spdlog::level::level_enum default_log_level);
spdlog::level::level_enum default_log_level,
absl::string_view listener_manager = "envoy.listener_manager_impl.default");

// Default constructor; creates "reasonable" defaults, but desired values should be set
// explicitly.
Expand Down Expand Up @@ -157,6 +159,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
}
uint32_t count() const;
const std::string& socketPath() const override { return socket_path_; }
const std::string& listenerManager() const override { return listener_manager_; }
mode_t socketMode() const override { return socket_mode_; }

/**
Expand Down Expand Up @@ -217,6 +220,7 @@ class OptionsImpl : public Server::Options, protected Logger::Loggable<Logger::I
// enable_fine_grain_logging_.
bool enable_fine_grain_logging_ = false;
std::string socket_path_{"@envoy_domain_socket"};
std::string listener_manager_;
mode_t socket_mode_{0};
};

Expand Down
3 changes: 1 addition & 2 deletions source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,7 @@ void InstanceImpl::initialize(Network::Address::InstanceConstSharedPtr local_add

// Workers get created first so they register for thread local updates.
listener_manager_ =
Config::Utility::getAndCheckFactoryByName<ListenerManagerFactory>(
Config::ServerExtensionValues::get().DEFAULT_LISTENER)
Config::Utility::getAndCheckFactoryByName<ListenerManagerFactory>(options_.listenerManager())
.createListenerManager(*this, nullptr, worker_factory_,
bootstrap_.enable_dispatcher_stats(), quic_stat_names_);

Expand Down
1 change: 1 addition & 0 deletions test/mocks/server/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class MockOptions : public Options {
MOCK_METHOD(const std::string&, socketPath, (), (const));
MOCK_METHOD(mode_t, socketMode, (), (const));
MOCK_METHOD((const Stats::TagVector&), statsTags, (), (const));
MOCK_METHOD(const std::string&, listenerManager, (), (const));

std::string config_path_;
envoy::config::bootstrap::v3::Bootstrap config_proto_;
Expand Down