Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xds: use filter type URL as the primary way to discover extensions #9618

Merged
merged 32 commits into from
Jan 23, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
7442509
xds: use filter type URL as the primary way to discover extensions
kyessenov Jan 9, 2020
aad3af2
fix test registry
kyessenov Jan 9, 2020
ed020ad
merge fix
kyessenov Jan 9, 2020
151f640
merge fix
kyessenov Jan 9, 2020
f76c893
add unit tests
kyessenov Jan 9, 2020
224d5cf
fix asan
kyessenov Jan 10, 2020
2e7c1fa
review
kyessenov Jan 10, 2020
65113cb
asan fix
kyessenov Jan 10, 2020
6312737
hopefully fix coverage
kyessenov Jan 10, 2020
a4b77ae
coverage fails because of exception test for double registration
kyessenov Jan 10, 2020
ac5504b
change exception into a warning
kyessenov Jan 10, 2020
2e6515b
linkage-dependent test
kyessenov Jan 10, 2020
4b02241
document
kyessenov Jan 10, 2020
0c3dba7
blurb about typed struct
kyessenov Jan 10, 2020
cff30cd
typo
kyessenov Jan 10, 2020
9265f8e
issue with empty again
kyessenov Jan 10, 2020
c0d29bd
struct is ok
kyessenov Jan 10, 2020
18b57ca
fallback to name
kyessenov Jan 10, 2020
5d3342b
oops
kyessenov Jan 10, 2020
a0445ec
merge fix
kyessenov Jan 21, 2020
d546d26
fix tests
kyessenov Jan 21, 2020
6a1d486
section
kyessenov Jan 21, 2020
bc6f2bb
fix version
kyessenov Jan 21, 2020
402a86d
review
kyessenov Jan 21, 2020
7134d12
Merge remote-tracking branch 'upstream/master' into free-the-name
kyessenov Jan 22, 2020
36ad6ec
fix coverage build
kyessenov Jan 22, 2020
df32987
Merge remote-tracking branch 'upstream/master' into free-the-name
kyessenov Jan 22, 2020
858dfed
Merge remote-tracking branch 'upstream/master' into free-the-name
kyessenov Jan 22, 2020
0b875dd
trying to debug on CI since my local build passes
kyessenov Jan 22, 2020
a338edf
trying to debug on CI since my local build passes
kyessenov Jan 22, 2020
9f398d3
ODR violation
kyessenov Jan 23, 2020
1083c22
Merge remote-tracking branch 'upstream/master' into free-the-name
kyessenov Jan 23, 2020
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
63 changes: 63 additions & 0 deletions docs/root/configuration/overview/v2_overview.rst
Original file line number Diff line number Diff line change
Expand Up @@ -332,6 +332,69 @@ The management server could respond to EDS requests with:
address: 127.0.0.2
port_value: 1234

.. _config_overview_v2_extension_configuration:

Extension configuration
-----------------------
Copy link
Member

Choose a reason for hiding this comment

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

Can you add some discussion in here of the UDPA typed struct also? In general I would like to see a discussion of all the various acceptable ways of configuring extensions and why a user might want to choose them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. I didn't find any mention of typed struct, but can start sketching something in this section.


Each configuration resource in Envoy has a type URL in the `typed_config`. This
type corresponds to a versioned schema. If the type URL uniquely identifies an
extension capable of interpreting the configuration, then the extension is
selected regardless of the `name` field. In this case the `name` field becomes
optional and can be used as an identifier or as an annotation for the
particular instance of the extension configuration. For example, the following
filter configuration snippet is permitted:

.. code-block:: yaml

name: front-http-proxy
typed_config:
"@type": type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
stat_prefix: ingress_http
codec_type: AUTO
rds:
route_config_name: local_route
config_source:
api_config_source:
api_type: GRPC
grpc_services:
envoy_grpc:
cluster_name: xds_cluster
http_filters:
- name: front-router
typed_config:
"@type": type.googleapis.com/envoy.config.filter.http.router.v2.Router
dynamic_stats: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think something is injecting Protobuf::Empty when @type is specified and nothing else. Need to track that down, but that's a degenerate case.


In case the control plane lacks the schema definitions for an extension,
`udpa.type.v1.TypedStruct` should be used as a generic container. The type URL
inside it is then used by a client to convert the contents to a typed
configuration resource. For example, the above example could be written as
follows:

.. code-block:: yaml

name: front-http-proxy
typed_config:
"@type": type.googleapis.com/udpa.type.v1.TypedStruct
type_url: type.googleapis.com/envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager
value:
stat_prefix: ingress_http
codec_type: AUTO
rds:
route_config_name: local_route
config_source:
api_config_source:
api_type: GRPC
grpc_services:
envoy_grpc:
cluster_name: xds_cluster
http_filters:
- name: front-router
typed_config:
"@type": type.googleapis.com/udpa.type.v1.TypedStruct
type_url: type.googleapis.com/envoy.config.filter.http.router.v2.Router

.. _config_overview_v2_management_server:

xDS API endpoints
Expand Down
1 change: 1 addition & 0 deletions docs/root/intro/version_history.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Version history
* build: official released binary is now built against libc++.
* cluster: added :ref:`aggregate cluster <arch_overview_aggregate_cluster>` that allows load balancing between clusters.
* config: all category names of internal envoy extensions are prefixed with the 'envoy.' prefix to follow the reverse DNS naming notation.
* config: use type URL to select an extension whenever the config type URL (or its previous versions) uniquely identify a typed extension.
kyessenov marked this conversation as resolved.
Show resolved Hide resolved
* decompressor: remove decompressor hard assert failure and replace with an error flag.
* ext_authz: added :ref:`configurable ability<envoy_api_field_config.filter.http.ext_authz.v2.ExtAuthz.include_peer_certificate>` to send the :ref:`certificate<envoy_api_field_service.auth.v2.AttributeContext.Peer.certificate>` to the `ext_authz` service.
* health check: gRPC health checker sets the gRPC deadline to the configured timeout duration.
Expand Down
1 change: 1 addition & 0 deletions include/envoy/registry/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ envoy_cc_library(
hdrs = ["registry.h"],
deps = [
"//source/common/common:assert_lib",
"//source/common/config:api_type_oracle_lib",
"//source/common/protobuf:utility_lib",
"@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto",
],
Expand Down
62 changes: 61 additions & 1 deletion include/envoy/registry/registry.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/common/logger.h"
#include "common/config/api_type_oracle.h"
#include "common/protobuf/utility.h"

#include "absl/base/attributes.h"
Expand Down Expand Up @@ -188,6 +189,55 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id
return *deprecated_factory_names;
}

/**
* Lazily constructs a mapping from the configuration message type to a factory,
* including the deprecated configuration message types.
* Must be invoked after factory registration is completed.
*/
static absl::flat_hash_map<std::string, Base*>& factoriesByType() {
static absl::flat_hash_map<std::string, Base*>* factories_by_type =
[] {
auto mapping = std::make_unique<absl::flat_hash_map<std::string, Base*>>();

for (const auto& factory : factories()) {
if (factory.second == nullptr) {
continue;
}

// Skip untyped factories.
std::string config_type = factory.second->configType();
if (config_type.empty()) {
continue;
}

// Register config types in the mapping and traverse the deprecated message type chain.
while (true) {
auto it = mapping->find(config_type);
if (it != mapping->end() && it->second != factory.second) {
// Mark double-registered types with a nullptr.
// See issue https://github.com/envoyproxy/envoy/issues/9643.
ENVOY_LOG(warn, "Double registration for type: '{}' by '{}' and '{}'", config_type,
factory.second->name(), it->second ? it->second->name() : "");
it->second = nullptr;
} else {
mapping->emplace(std::make_pair(config_type, factory.second));
}

const Protobuf::Descriptor* previous =
Config::ApiTypeOracle::getEarlierVersionDescriptor(config_type);
if (previous == nullptr) {
break;
}
config_type = previous->full_name();
}
}
return mapping;
}()
.release();

return *factories_by_type;
}

/**
* instead_value are used when passed name was deprecated.
*/
Expand Down Expand Up @@ -262,6 +312,14 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id
return it->second;
}

static Base* getFactoryByType(absl::string_view type) {
auto it = factoriesByType().find(type);
if (it == factoriesByType().end()) {
return nullptr;
}
return it->second;
}

/**
* @return the canonical name of the factory. If the given name is a
* deprecated factory name, the canonical name is returned instead.
Expand Down Expand Up @@ -319,6 +377,7 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id

factories().emplace(factory.name(), &factory);
RELEASE_ASSERT(getFactory(factory.name()) == &factory, "");
factoriesByType().emplace(factory.configType(), &factory);

return displaced;
}
Expand All @@ -327,9 +386,10 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id
* Remove a factory by name. This method should only be used for testing purposes.
* @param name is the name of the factory to remove.
*/
static void removeFactoryForTest(absl::string_view name) {
static void removeFactoryForTest(absl::string_view name, absl::string_view config_type) {
auto result = factories().erase(name);
RELEASE_ASSERT(result == 1, "");
factoriesByType().erase(config_type);
}
};

Expand Down
8 changes: 3 additions & 5 deletions source/common/config/api_type_oracle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,10 @@ namespace Envoy {
namespace Config {

const Protobuf::Descriptor*
ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) {
const std::string target_type = message.GetDescriptor()->full_name();

// Determine if there is an earlier API version for target_type.
ApiTypeOracle::getEarlierVersionDescriptor(const std::string& message_type) {
// Determine if there is an earlier API version for message_type.
const Protobuf::Descriptor* desc =
Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(std::string{target_type});
Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(std::string{message_type});
if (desc == nullptr) {
return nullptr;
}
Expand Down
4 changes: 2 additions & 2 deletions source/common/config/api_type_oracle.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ class ApiTypeOracle {
* this message. If so, return the descriptor for the earlier
* message, to support upgrading via VersionConverter::upgrade().
*
* @param message protobuf message.
* @param message_type protobuf message type
* @return const Protobuf::Descriptor* descriptor for earlier message version
* corresponding to message, if any, otherwise nullptr.
*/
static const Protobuf::Descriptor* getEarlierVersionDescriptor(const Protobuf::Message& message);
static const Protobuf::Descriptor* getEarlierVersionDescriptor(const std::string& message_type);
};

} // namespace Config
Expand Down
2 changes: 0 additions & 2 deletions source/common/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
#include "common/stats/stats_matcher_impl.h"
#include "common/stats/tag_producer_impl.h"

#include "udpa/type/v1/typed_struct.pb.h"

namespace Envoy {
namespace Config {

Expand Down
36 changes: 36 additions & 0 deletions source/common/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
#include "common/protobuf/utility.h"
#include "common/singleton/const_singleton.h"

#include "udpa/type/v1/typed_struct.pb.h"

namespace Envoy {
namespace Config {

Expand Down Expand Up @@ -209,13 +211,47 @@ class Utility {
return *factory;
}

template <class Factory> static Factory& getAndCheckFactoryByType(absl::string_view type) {
if (type.empty()) {
throw EnvoyException("Provided type for static registration lookup was empty.");
kyessenov marked this conversation as resolved.
Show resolved Hide resolved
}
Factory* factory = Registry::FactoryRegistry<Factory>::getFactoryByType(type);
if (factory == nullptr) {
throw EnvoyException(
fmt::format("Didn't find a registered implementation for type: '{}'", type));
}
return *factory;
}

/**
* Get a Factory from the registry with error checking to ensure the name and the factory are
* valid.
* @param message proto that contains fields 'name' and 'typed_config'.
*/
template <class Factory, class ProtoMessage>
static Factory& getAndCheckFactory(const ProtoMessage& message) {
const ProtobufWkt::Any& typed_config = message.typed_config();
static const std::string& struct_type =
Copy link
Member

Choose a reason for hiding this comment

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

Why can't Struct be treated the same way as any double registered type URL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are two cases:

  • case 1: Struct inside TypedStruct inside typed_config -> handled just like a double registered type URL;
  • case 2: Struct inside typed_config -> ignored, treated as if config field was set instead of typed_config.

Both cases are abnormal IMHO. Do you want to make case 2 behave like case 1?

Copy link
Member

Choose a reason for hiding this comment

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

I think we might want that, to make it easier for folks to move to TypedStruct even without fixing their config uniqueness issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, made it fall back to name if a factory cannot be found because either it is untyped, or double-registered for a type.

ProtobufWkt::Struct::default_instance().GetDescriptor()->full_name();
static const std::string& typed_struct_type =
udpa::type::v1::TypedStruct::default_instance().GetDescriptor()->full_name();

if (!typed_config.value().empty()) {
// Unpack methods will only use the fully qualified type name after the last '/'.
// https://github.com/protocolbuffers/protobuf/blob/3.6.x/src/google/protobuf/any.proto#L87
const absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url());

if (type == typed_struct_type) {
udpa::type::v1::TypedStruct typed_struct;
MessageUtil::unpackTo(typed_config, typed_struct);
// Not handling nested structs or typed structs in typed structs
return Utility::getAndCheckFactoryByType<Factory>(
TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url()));
} else if (type != struct_type) {
return Utility::getAndCheckFactoryByType<Factory>(type);
}
}

return Utility::getAndCheckFactoryByName<Factory>(message.name());
}

Expand Down
3 changes: 2 additions & 1 deletion source/common/config/version_converter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ void VersionConverter::upgrade(const Protobuf::Message& prev_message,

DowngradedMessagePtr VersionConverter::downgrade(const Protobuf::Message& message) {
auto downgraded_message = std::make_unique<DowngradedMessage>();
const Protobuf::Descriptor* prev_desc = ApiTypeOracle::getEarlierVersionDescriptor(message);
const Protobuf::Descriptor* prev_desc =
ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name());
if (prev_desc != nullptr) {
downgraded_message->msg_.reset(
downgraded_message->dynamic_msg_factory_.GetPrototype(prev_desc)->New());
Expand Down
4 changes: 2 additions & 2 deletions source/common/protobuf/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class ApiBoostRetryException : public EnvoyException {
// vN/v(N+1) mechanical transforms.
void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) {
const Protobuf::Descriptor* earlier_version_desc =
Config::ApiTypeOracle::getEarlierVersionDescriptor(message);
Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name());
// If there is no earlier version of a message, just apply f directly.
if (earlier_version_desc == nullptr) {
f(message, MessageVersion::LATEST_VERSION);
Expand Down Expand Up @@ -493,7 +493,7 @@ void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Messag
TypeUtil::typeUrlToDescriptorFullName(any_message.type_url());
if (any_full_name != message.GetDescriptor()->full_name()) {
const Protobuf::Descriptor* earlier_version_desc =
Config::ApiTypeOracle::getEarlierVersionDescriptor(message);
Config::ApiTypeOracle::getEarlierVersionDescriptor(message.GetDescriptor()->full_name());
// If the earlier version matches, unpack and upgrade.
if (earlier_version_desc != nullptr && any_full_name == earlier_version_desc->full_name()) {
Protobuf::DynamicMessageFactory dmf;
Expand Down
9 changes: 6 additions & 3 deletions test/common/config/api_type_oracle_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@ TEST(ApiTypeOracleTest, All) {
envoy::extensions::filters::http::ip_tagging::v3alpha::IPTagging v3_config;
ProtobufWkt::Any non_api_type;

EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(non_api_type));
EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(v2_config));
const auto* desc = ApiTypeOracle::getEarlierVersionDescriptor(v3_config);
EXPECT_EQ(nullptr,
ApiTypeOracle::getEarlierVersionDescriptor(non_api_type.GetDescriptor()->full_name()));
EXPECT_EQ(nullptr,
ApiTypeOracle::getEarlierVersionDescriptor(v2_config.GetDescriptor()->full_name()));
const auto* desc =
ApiTypeOracle::getEarlierVersionDescriptor(v3_config.GetDescriptor()->full_name());
EXPECT_EQ(envoy::config::filter::http::ip_tagging::v2::IPTagging::descriptor()->full_name(),
desc->full_name());
}
Expand Down
31 changes: 15 additions & 16 deletions test/extensions/access_loggers/file/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,26 @@ TEST(FileAccessLogConfigTest, ValidateFail) {
TEST(FileAccessLogConfigTest, ConfigureFromProto) {
envoy::config::accesslog::v3alpha::AccessLog config;

NiceMock<Server::Configuration::MockFactoryContext> context;
EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Provided name for static registration lookup was empty.");

config.set_name("INVALID");

EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Didn't find a registered implementation for name: 'INVALID'");

envoy::extensions::access_loggers::file::v3alpha::FileAccessLog fal_config;
fal_config.set_path("/dev/null");

config.mutable_typed_config()->PackFrom(fal_config);

NiceMock<Server::Configuration::MockFactoryContext> context;
EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Provided name for static registration lookup was empty.");

config.set_name(AccessLogNames::get().File);

AccessLog::InstanceSharedPtr log = AccessLog::AccessLogFactory::fromProto(config, context);

EXPECT_NE(nullptr, log);
EXPECT_NE(nullptr, dynamic_cast<FileAccessLog*>(log.get()));

config.set_name("INVALID");

EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Didn't find a registered implementation for name: 'INVALID'");
}

TEST(FileAccessLogConfigTest, FileAccessLogTest) {
Expand Down Expand Up @@ -92,23 +92,22 @@ TEST(FileAccessLogConfigTest, FileAccessLogJsonTest) {
EXPECT_EQ(fal_config.access_log_format_case(),
envoy::extensions::access_loggers::file::v3alpha::FileAccessLog::AccessLogFormatCase::
kJsonFormat);
config.mutable_typed_config()->PackFrom(fal_config);

NiceMock<Server::Configuration::MockFactoryContext> context;
EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Provided name for static registration lookup was empty.");

config.set_name("INVALID");

EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Didn't find a registered implementation for name: 'INVALID'");
config.mutable_typed_config()->PackFrom(fal_config);

config.set_name(AccessLogNames::get().File);

AccessLog::InstanceSharedPtr log = AccessLog::AccessLogFactory::fromProto(config, context);

EXPECT_NE(nullptr, log);
EXPECT_NE(nullptr, dynamic_cast<FileAccessLog*>(log.get()));

config.set_name("INVALID");

EXPECT_THROW_WITH_MESSAGE(AccessLog::AccessLogFactory::fromProto(config, context), EnvoyException,
"Didn't find a registered implementation for name: 'INVALID'");
}

TEST(FileAccessLogConfigTest, FileAccessLogTypedJsonTest) {
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -905,6 +905,7 @@ envoy_cc_test(
deps = [
":http_integration_lib",
"//source/common/stats:stats_lib",
"//test/test_common:registry_lib",
"@envoy_api//envoy/extensions/filters/network/tcp_proxy/v3alpha:pkg_cc_proto",
],
)
Expand Down
Loading