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.

Sign up for GitHub

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

Jump to bottom

config: upgrade envoy.ip_tagging to v3alpha. #9150

Merged
merged 4 commits into from
Nov 27, 2019
Merged

config: upgrade envoy.ip_tagging to v3alpha. #9150

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
485580b
config: upgrade envoy.ip_tagging to v3alpha.
htuch Nov 25, 2019
037dcff
Nit.
htuch Nov 27, 2019
070a6b2
Use factory.name() to simplify translateToFactoryConfig().
htuch Nov 27, 2019
60d80e0
DEPRECATED_FEATURE_TEST
htuch Nov 27, 2019
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
13 changes: 13 additions & 0 deletions source/common/config/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,17 @@ load(

envoy_package()

envoy_cc_library(
name = "api_type_db_lib",
htuch marked this conversation as resolved.
Show resolved Hide resolved
srcs = ["api_type_db.cc"],
hdrs = ["api_type_db.h"],
deps = [
"//source/common/protobuf",
"//source/common/protobuf:utility_lib",
"@com_github_cncf_udpa//udpa/type/v1:typed_struct_cc",
],
)

envoy_cc_library(
name = "config_provider_lib",
srcs = ["config_provider_impl.cc"],
Expand Down Expand Up @@ -289,7 +300,9 @@ envoy_cc_library(
srcs = ["utility.cc"],
hdrs = ["utility.h"],
deps = [
":api_type_db_lib",
":resources_lib",
":version_converter_lib",
"//include/envoy/config:grpc_mux_interface",
"//include/envoy/config:subscription_interface",
"//include/envoy/local_info:local_info_interface",
Expand Down
42 changes: 42 additions & 0 deletions source/common/config/api_type_db.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#include "common/config/api_type_db.h"

#include "common/protobuf/utility.h"

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

namespace Envoy {
namespace Config {

const Protobuf::Descriptor*
ApiTypeDb::inferEarlierVersionDescriptor(absl::string_view extension_name,
const ProtobufWkt::Any& typed_config,
absl::string_view target_type) {
// This is just a placeholder for where we will load from the API type
// database.
absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url());

udpa::type::v1::TypedStruct typed_struct;
if (type == udpa::type::v1::TypedStruct::default_instance().GetDescriptor()->full_name()) {
MessageUtil::unpackTo(typed_config, typed_struct);
type = TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url());
ENVOY_LOG_MISC(debug, "Extracted embedded type {}", type);
}

// We have an earlier version if (i) we know about the extension (ii) its type
// is different (v3alpha vs. v2) and (iii) we haven't reached the end of the
// upgrade chain.
if (extension_name == "envoy.ip_tagging" && type != target_type &&
target_type != "envoy.config.filter.http.ip_tagging.v2.IPTagging") {
const Protobuf::Descriptor* desc =
Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(
"envoy.config.filter.http.ip_tagging.v2.IPTagging");
ASSERT(desc != nullptr);
ENVOY_LOG_MISC(debug, "Inferred {}", desc->full_name());
return desc;
}

return nullptr;
}

} // namespace Config
} // namespace Envoy
31 changes: 31 additions & 0 deletions source/common/config/api_type_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#pragma once

#include "common/protobuf/protobuf.h"

#include "absl/strings/string_view.h"

namespace Envoy {
namespace Config {

class ApiTypeDb {
public:
/**
* Based on the presented extension config and name, determine if this is
* configuration for an earlier version than the latest alpha version
* supported by Envoy internally. If so, return the descriptor for the earlier
* message, to support upgrading via VersionConverter::upgrade().
*
* @param extension_name name of extension corresponding to config.
* @param typed_config opaque config packed in google.protobuf.Any.
* @param target_type target type of conversion.
* @return const Protobuf::Descriptor* descriptor for earlier message version
* corresponding to config, if any, otherwise nullptr.
*/
static const Protobuf::Descriptor*
inferEarlierVersionDescriptor(absl::string_view extension_name,
const ProtobufWkt::Any& typed_config,
absl::string_view target_type);
};

} // namespace Config
} // namespace Envoy
34 changes: 21 additions & 13 deletions source/common/config/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
#include "common/common/fmt.h"
#include "common/common/hex.h"
#include "common/common/utility.h"
#include "common/config/api_type_db.h"
#include "common/config/resources.h"
#include "common/config/version_converter.h"
#include "common/config/well_known_names.h"
#include "common/protobuf/protobuf.h"
#include "common/protobuf/utility.h"
Expand Down Expand Up @@ -240,20 +242,26 @@ envoy::api::v2::ClusterLoadAssignment Utility::translateClusterHosts(
return load_assignment;
}

namespace {
absl::string_view protoTypeUrlToDescriptorFullName(absl::string_view type_url) {
size_t pos = type_url.find_last_of('/');
if (pos != absl::string_view::npos) {
type_url = type_url.substr(pos + 1);
}
return type_url;
}
} // namespace

void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
void Utility::translateOpaqueConfig(absl::string_view extension_name,
const ProtobufWkt::Any& typed_config,
const ProtobufWkt::Struct& config,
ProtobufMessage::ValidationVisitor& validation_visitor,
Protobuf::Message& out_proto) {
const Protobuf::Descriptor* earlier_version_desc = ApiTypeDb::inferEarlierVersionDescriptor(
extension_name, typed_config, out_proto.GetDescriptor()->full_name());

if (earlier_version_desc != nullptr) {
Protobuf::DynamicMessageFactory dmf;
// Create a previous version message.
auto message = ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New());
ASSERT(message != nullptr);
// Recurse and translateOpaqueConfig for previous version.
translateOpaqueConfig(extension_name, typed_config, config, validation_visitor, *message);
// Update from previous version to current version.
VersionConverter::upgrade(*message, out_proto);
return;
}

static const std::string struct_type =
ProtobufWkt::Struct::default_instance().GetDescriptor()->full_name();
static const std::string typed_struct_type =
Expand All @@ -263,7 +271,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,

// 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
absl::string_view type = protoTypeUrlToDescriptorFullName(typed_config.type_url());
absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url());

if (type == typed_struct_type) {
udpa::type::v1::TypedStruct typed_struct;
Expand All @@ -272,7 +280,7 @@ void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
if (out_proto.GetDescriptor()->full_name() == struct_type) {
out_proto.CopyFrom(typed_struct.value());
} else {
type = protoTypeUrlToDescriptorFullName(typed_struct.type_url());
type = TypeUtil::typeUrlToDescriptorFullName(typed_struct.type_url());
if (type != out_proto.GetDescriptor()->full_name()) {
throw EnvoyException("Invalid proto type.\nExpected " +
out_proto.GetDescriptor()->full_name() +
Expand Down
9 changes: 6 additions & 3 deletions source/common/config/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,7 @@ class Utility {

/**
* Translate a nested config into a proto message provided by the implementation factory.
* @param extension_name name of extension corresponding to config.
* @param enclosing_message proto that contains a field 'config'. Note: the enclosing proto is
* provided because for statically registered implementations, a custom config is generally
* optional, which means the conversion must be done conditionally.
Expand All @@ -225,8 +226,8 @@ class Utility {
// Fail in an obvious way if a plugin does not return a proto.
RELEASE_ASSERT(config != nullptr, "");

translateOpaqueConfig(enclosing_message.typed_config(), enclosing_message.config(),
validation_visitor, *config);
translateOpaqueConfig(factory.name(), enclosing_message.typed_config(),
enclosing_message.config(), validation_visitor, *config);

return config;
}
Expand Down Expand Up @@ -268,12 +269,14 @@ class Utility {
/**
* Translate opaque config from google.protobuf.Any or google.protobuf.Struct to defined proto
* message.
* @param extension_name name of extension corresponding to config.
* @param typed_config opaque config packed in google.protobuf.Any
* @param config the deprecated google.protobuf.Struct config, empty struct if doesn't exist.
* @param validation_visitor message validation visitor instance.
* @param out_proto the proto message instantiated by extensions
*/
static void translateOpaqueConfig(const ProtobufWkt::Any& typed_config,
static void translateOpaqueConfig(absl::string_view extension_name,
const ProtobufWkt::Any& typed_config,
const ProtobufWkt::Struct& config,
ProtobufMessage::ValidationVisitor& validation_visitor,
Protobuf::Message& out_proto);
Expand Down
1 change: 1 addition & 0 deletions source/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:utility_lib",
"@envoy_api//envoy/api/v2/core:pkg_cc_proto",
"@envoy_api//envoy/api/v3alpha/core:pkg_cc_proto",
],
)

Expand Down
4 changes: 4 additions & 0 deletions source/common/network/cidr_range.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ CidrRange CidrRange::create(const envoy::api::v2::core::CidrRange& cidr) {
return create(Utility::parseInternetAddress(cidr.address_prefix()), cidr.prefix_len().value());
}

CidrRange CidrRange::create(const envoy::api::v3alpha::core::CidrRange& cidr) {
return create(Utility::parseInternetAddress(cidr.address_prefix()), cidr.prefix_len().value());
}

// static
CidrRange CidrRange::create(const std::string& range) {
const auto parts = StringUtil::splitToken(range, "/");
Expand Down
4 changes: 4 additions & 0 deletions source/common/network/cidr_range.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include <vector>

#include "envoy/api/v2/core/address.pb.h"
#include "envoy/api/v3alpha/core/address.pb.h"
#include "envoy/json/json_object.h"
#include "envoy/network/address.h"

Expand Down Expand Up @@ -99,6 +100,9 @@ class CidrRange {
* TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges.
*/
static CidrRange create(const envoy::api::v2::core::CidrRange& cidr);
// The ::v2 and ::v3alpha variants will merge once we land API boosting
// automation. TODO(htuch): make sure this happens.
static CidrRange create(const envoy::api::v3alpha::core::CidrRange& cidr);

/**
* Given an IP address and a length of high order bits to keep, returns an address
Expand Down
8 changes: 8 additions & 0 deletions source/common/protobuf/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,4 +613,12 @@ void TimestampUtil::systemClockToTimestamp(const SystemTime system_clock_time,
.count()));
}

absl::string_view TypeUtil::typeUrlToDescriptorFullName(absl::string_view type_url) {
const size_t pos = type_url.rfind('/');
if (pos != absl::string_view::npos) {
type_url = type_url.substr(pos + 1);
}
return type_url;
}

} // namespace Envoy
5 changes: 5 additions & 0 deletions source/common/protobuf/utility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ class MissingFieldException : public EnvoyException {
MissingFieldException(const std::string& field_name, const Protobuf::Message& message);
};

class TypeUtil {
public:
static absl::string_view typeUrlToDescriptorFullName(absl::string_view type_url);
};

class RepeatedPtrUtil {
public:
static std::string join(const Protobuf::RepeatedPtrField<std::string>& source,
Expand Down
3 changes: 2 additions & 1 deletion source/common/router/config_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,8 @@ createRouteSpecificFilterConfig(const std::string& name, const ProtobufWkt::Any&
auto& factory = Envoy::Config::Utility::getAndCheckFactory<
Server::Configuration::NamedHttpFilterConfigFactory>(name);
ProtobufTypes::MessagePtr proto_config = factory.createEmptyRouteConfigProto();
Envoy::Config::Utility::translateOpaqueConfig(typed_config, config, validator, *proto_config);
Envoy::Config::Utility::translateOpaqueConfig(name, typed_config, config, validator,
*proto_config);
return factory.createRouteSpecificFilterConfig(*proto_config, factory_context, validator);
}

Expand Down
5 changes: 3 additions & 2 deletions source/common/upstream/cluster_factory_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,9 @@ template <class ConfigProto> class ConfigurableClusterFactoryBase : public Clust
Stats::ScopePtr&& stats_scope) override {
ProtobufTypes::MessagePtr config = createEmptyConfigProto();
Config::Utility::translateOpaqueConfig(
cluster.cluster_type().typed_config(), ProtobufWkt::Struct::default_instance(),
socket_factory_context.messageValidationVisitor(), *config);
cluster.cluster_type().name(), cluster.cluster_type().typed_config(),
ProtobufWkt::Struct::default_instance(), socket_factory_context.messageValidationVisitor(),
*config);
return createClusterWithConfig(cluster,
MessageUtil::downcastAndValidate<const ConfigProto&>(
*config, context.messageValidationVisitor()),
Expand Down
2 changes: 1 addition & 1 deletion source/common/upstream/upstream_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ createProtocolOptionsConfig(const std::string& name, const ProtobufWkt::Any& typ
throw EnvoyException(fmt::format("filter {} does not support protocol options", name));
}

Envoy::Config::Utility::translateOpaqueConfig(typed_config, config, validation_visitor,
Envoy::Config::Utility::translateOpaqueConfig(name, typed_config, config, validation_visitor,
*proto_config);

return factory->createProtocolOptionsConfig(*proto_config, validation_visitor);
Expand Down
3 changes: 2 additions & 1 deletion source/extensions/filters/http/ip_tagging/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ envoy_cc_library(
"//source/common/http:headers_lib",
"//source/common/network:lc_trie_lib",
"//source/common/stats:symbol_table_lib",
"@envoy_api//envoy/config/filter/http/ip_tagging/v2:pkg_cc_proto",
"@envoy_api//envoy/config/filter/http/ip_tagging/v3alpha:pkg_cc_proto",
],
)

Expand All @@ -40,5 +40,6 @@ envoy_cc_extension(
"//source/extensions/filters/http/common:factory_base_lib",
"//source/extensions/filters/http/ip_tagging:ip_tagging_filter_lib",
"@envoy_api//envoy/config/filter/http/ip_tagging/v2:pkg_cc_proto",
"@envoy_api//envoy/config/filter/http/ip_tagging/v3alpha:pkg_cc_proto",
],
)
4 changes: 2 additions & 2 deletions source/extensions/filters/http/ip_tagging/config.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#include "extensions/filters/http/ip_tagging/config.h"

#include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.validate.h"
#include "envoy/config/filter/http/ip_tagging/v3alpha/ip_tagging.pb.validate.h"
#include "envoy/registry/registry.h"

#include "common/protobuf/utility.h"
Expand All @@ -13,7 +13,7 @@ namespace HttpFilters {
namespace IpTagging {

Http::FilterFactoryCb IpTaggingFilterFactory::createFilterFactoryFromProtoTyped(
const envoy::config::filter::http::ip_tagging::v2::IPTagging& proto_config,
const envoy::config::filter::http::ip_tagging::v3alpha::IPTagging& proto_config,
const std::string& stat_prefix, Server::Configuration::FactoryContext& context) {

IpTaggingFilterConfigSharedPtr config(
Expand Down
9 changes: 5 additions & 4 deletions source/extensions/filters/http/ip_tagging/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
#pragma once

#include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.h"
#include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.validate.h"
#include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.h" // For proto descriptors only
Copy link
Member

Choose a reason for hiding this comment

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

Will we potentially run into linking dropping needed descriptors like has happened previously when we move to a fully automated/reflection version? Especially in library situations like Envoy Mobile? It would be nice to make sure we avoid this at a system level somehow and don't run into confusing runtime problems? (This is orthogonal to the other discussion about being able to completely disable conversion via compile time option potentially)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I want to take this on as a separate PR. Let's say we don't have the API type DB, and we embed annotations inside the v3 APIs to record some of the history. Do we want to be able to avoid including v2 for Envoy Mobile still? I.e. save the space of the proto descriptors.

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 that's fine in that case, but I still worry about dropping descriptors that we need in confusing ways, but I think we can figure that out later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we might need to generate some dummy file to reference all of these. As we encounter this during the conversion work, I will add something to do this. Basically, I envisage a tool that is part of fix_format that uses the API type DB (which will be available at format time) that figures out the entire set of API build targets and headers, that generates some C++ file with lots of void casted references to things in these targets. Or we can try and solve the problem of the lost proto descriptors. Either way.

#include "envoy/config/filter/http/ip_tagging/v3alpha/ip_tagging.pb.h"
#include "envoy/config/filter/http/ip_tagging/v3alpha/ip_tagging.pb.validate.h"

#include "extensions/filters/http/common/factory_base.h"
#include "extensions/filters/http/well_known_names.h"
Expand All @@ -15,13 +16,13 @@ namespace IpTagging {
* Config registration for the router filter. @see NamedHttpFilterConfigFactory.
*/
class IpTaggingFilterFactory
: public Common::FactoryBase<envoy::config::filter::http::ip_tagging::v2::IPTagging> {
: public Common::FactoryBase<envoy::config::filter::http::ip_tagging::v3alpha::IPTagging> {
public:
IpTaggingFilterFactory() : FactoryBase(HttpFilterNames::get().IpTagging) {}

private:
Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
const envoy::config::filter::http::ip_tagging::v2::IPTagging& proto_config,
const envoy::config::filter::http::ip_tagging::v3alpha::IPTagging& proto_config,
const std::string& stats_prefix, Server::Configuration::FactoryContext& context) override;
};

Expand Down
4 changes: 2 additions & 2 deletions source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace HttpFilters {
namespace IpTagging {

IpTaggingFilterConfig::IpTaggingFilterConfig(
const envoy::config::filter::http::ip_tagging::v2::IPTagging& config,
const envoy::config::filter::http::ip_tagging::v3alpha::IPTagging& config,
const std::string& stat_prefix, Stats::Scope& scope, Runtime::Loader& runtime)
: request_type_(requestTypeEnum(config.request_type())), scope_(scope), runtime_(runtime),
stat_name_set_(scope.symbolTable().makeSet("IpTagging")),
Expand All @@ -33,7 +33,7 @@ IpTaggingFilterConfig::IpTaggingFilterConfig(
for (const auto& ip_tag : config.ip_tags()) {
std::vector<Network::Address::CidrRange> cidr_set;
cidr_set.reserve(ip_tag.ip_list().size());
for (const envoy::api::v2::core::CidrRange& entry : ip_tag.ip_list()) {
for (const envoy::api::v3alpha::core::CidrRange& entry : ip_tag.ip_list()) {

// Currently, CidrRange::create doesn't guarantee that the CidrRanges are valid.
Network::Address::CidrRange cidr_entry = Network::Address::CidrRange::create(entry);
Expand Down
Loading