From d64ef445b23137cfb8393e013e71d22cf554084c Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 26 Dec 2019 17:32:20 -0500 Subject: [PATCH 01/10] config: shadow-based upgrading/downgrading for versioned messages. This PR replaces the previous unknown field smuggling style of upgrading with shadow-based. Ultimately, we want to move away from shadows, but for 1.13.0 this simplifies by removing smuggling. In addition, we make two major changes that are backported from the WiP v3alpha branch: 1. Rather than make translateOpaqueConfig() smart and version/type aware, we switch to an iterative loadFromJson() that first attempts to load as v2 and then falls back to v3. This relies on the property that any v3 configuration that is ingestible as v2 has the same semantics in v2/v3, which holds due to the highly structured vN/v(N+1) mechanical transforms. 2. Support for downgrading to earlier versions is introduced to make it easier for tests that are within the v(N-1) subset of vN to continue to validate v(N-1), while still mostly using vN messages. This is particularly useful for ensuring that e2e integration tests are using v2 resources. As new v3 tests are introduced, tests will need to eschew downgrading, but this should be a viable approach for 1.13.0. Risk level: Low Testing: There's a bunch of new tests that need to be added for version converter, loadFroJson/unpackTo() and translateOpaqueConfig(). Putting this PR out for comment to get feedback. Part of #8082 Signed-off-by: Harvey Tuch --- source/common/config/BUILD | 6 +- source/common/config/api_type_oracle.cc | 49 +----- source/common/config/api_type_oracle.h | 18 +-- source/common/config/utility.cc | 27 +--- source/common/config/utility.h | 8 +- source/common/config/version_converter.cc | 148 +++--------------- source/common/config/version_converter.h | 39 +++-- source/common/protobuf/BUILD | 2 + source/common/protobuf/utility.cc | 117 ++++++++++---- source/common/router/config_impl.cc | 3 +- source/common/upstream/cluster_factory_impl.h | 5 +- source/common/upstream/upstream_impl.cc | 2 +- .../filters/network/dubbo_proxy/config.cc | 2 +- test/common/config/BUILD | 6 + test/common/config/api_type_oracle_test.cc | 20 ++- .../config/delta_subscription_test_harness.h | 3 +- test/common/config/grpc_mux_impl_test.cc | 35 +++-- .../config/grpc_subscription_test_harness.h | 5 +- test/common/config/new_grpc_mux_impl_test.cc | 8 +- test/common/config/utility_test.cc | 72 ++++++++- test/common/config/version_converter_test.cc | 2 + .../clusters/aggregate/cluster_test.cc | 3 +- .../dynamic_forward_proxy/cluster_test.cc | 3 +- .../clusters/redis/redis_cluster_test.cc | 9 +- test/integration/BUILD | 1 + test/integration/integration.h | 7 +- .../scoped_rds_integration_test.cc | 7 +- .../sds_dynamic_integration_test.cc | 2 +- 28 files changed, 283 insertions(+), 326 deletions(-) diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 9b61f27df4ca..3fa7358bb8de 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -13,10 +13,10 @@ envoy_cc_library( srcs = ["api_type_oracle.cc"], hdrs = ["api_type_oracle.h"], deps = [ + "//source/common/common:assert_lib", + "//source/common/common:logger_lib", "//source/common/protobuf", - "//source/common/protobuf:utility_lib", "@com_github_cncf_udpa//udpa/annotations:pkg_cc_proto", - "@com_github_cncf_udpa//udpa/type/v1:pkg_cc_proto", ], ) @@ -342,7 +342,7 @@ envoy_cc_library( srcs = ["version_converter.cc"], hdrs = ["version_converter.h"], deps = [ - "//source/common/common:assert_lib", + ":api_type_oracle_lib", "//source/common/protobuf", ], ) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 3381b4bdd3c7..b184c8320271 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -1,52 +1,19 @@ #include "common/config/api_type_oracle.h" -#include "common/protobuf/utility.h" +#include "common/common/assert.h" +#include "common/common/logger.h" #include "udpa/annotations/versioning.pb.h" -#include "udpa/type/v1/typed_struct.pb.h" namespace Envoy { namespace Config { -namespace { - -using V2ApiTypeMap = absl::flat_hash_map; - -const V2ApiTypeMap& v2ApiTypeMap() { - CONSTRUCT_ON_FIRST_USE(V2ApiTypeMap, - {"envoy.ip_tagging", "envoy.config.filter.http.ip_tagging.v2.IPTagging"}); -} - -} // namespace - const Protobuf::Descriptor* -ApiTypeOracle::inferEarlierVersionDescriptor(absl::string_view extension_name, - const ProtobufWkt::Any& typed_config, - absl::string_view target_type) { - ENVOY_LOG_MISC(trace, "Inferring earlier type for {} (extension {})", target_type, - extension_name); - // Determine what the type of configuration implied by typed_config is. - 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(trace, "Extracted embedded type {}", type); - } - - // If we can't find an explicit type, this is likely v2, so we need to consult - // a static map. - if (type.empty()) { - auto it = v2ApiTypeMap().find(extension_name); - if (it == v2ApiTypeMap().end()) { - ENVOY_LOG_MISC(trace, "Missing v2 API type map"); - return nullptr; - } - type = it->second; - } +ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) { + const std::string target_type = message.GetDescriptor()->full_name(); + ENVOY_LOG_MISC(trace, "Inferring earlier type for {} (extension {})", target_type); // Determine if there is an earlier API version for target_type. - std::string previous_target_type; const Protobuf::Descriptor* desc = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(std::string{target_type}); if (desc == nullptr) { @@ -54,11 +21,8 @@ ApiTypeOracle::inferEarlierVersionDescriptor(absl::string_view extension_name, return nullptr; } if (desc->options().HasExtension(udpa::annotations::versioning)) { - previous_target_type = + const std::string previous_target_type = desc->options().GetExtension(udpa::annotations::versioning).previous_message_type(); - } - - if (!previous_target_type.empty() && type != target_type) { const Protobuf::Descriptor* desc = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(previous_target_type); ASSERT(desc != nullptr); @@ -66,6 +30,7 @@ ApiTypeOracle::inferEarlierVersionDescriptor(absl::string_view extension_name, return desc; } + ENVOY_LOG_MISC(trace, "No earlier descriptor found for {}", target_type); return nullptr; } diff --git a/source/common/config/api_type_oracle.h b/source/common/config/api_type_oracle.h index b1694330fa07..e8ae307fe827 100644 --- a/source/common/config/api_type_oracle.h +++ b/source/common/config/api_type_oracle.h @@ -2,29 +2,21 @@ #include "common/protobuf/protobuf.h" -#include "absl/strings/string_view.h" - namespace Envoy { namespace Config { class ApiTypeOracle { 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 + * Based on a given message, determine if there exists an earlier version of + * this message. 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. + * @param message protobuf message. * @return const Protobuf::Descriptor* descriptor for earlier message version - * corresponding to config, if any, otherwise nullptr. + * corresponding to message, if any, otherwise nullptr. */ - static const Protobuf::Descriptor* - inferEarlierVersionDescriptor(absl::string_view extension_name, - const ProtobufWkt::Any& typed_config, - absl::string_view target_type); + static const Protobuf::Descriptor* getEarlierVersionDescriptor(const Protobuf::Message& message); }; } // namespace Config diff --git a/source/common/config/utility.cc b/source/common/config/utility.cc index f0abdce9e249..0221a4104716 100644 --- a/source/common/config/utility.cc +++ b/source/common/config/utility.cc @@ -248,33 +248,16 @@ envoy::api::v2::ClusterLoadAssignment Utility::translateClusterHosts( return load_assignment; } -void Utility::translateOpaqueConfig(absl::string_view extension_name, - const ProtobufWkt::Any& typed_config, +void Utility::translateOpaqueConfig(const ProtobufWkt::Any& typed_config, const ProtobufWkt::Struct& config, ProtobufMessage::ValidationVisitor& validation_visitor, Protobuf::Message& out_proto) { - const Protobuf::Descriptor* earlier_version_desc = ApiTypeOracle::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 = 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 absl::string_view type = TypeUtil::typeUrlToDescriptorFullName(typed_config.type_url()); @@ -286,12 +269,8 @@ void Utility::translateOpaqueConfig(absl::string_view extension_name, if (out_proto.GetDescriptor()->full_name() == struct_type) { out_proto.CopyFrom(typed_struct.value()); } else { - 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() + - "\nActual: " + std::string(type)); - } + // The typed struct might match out_proto, or some earlier version, let + // MessageUtil::jsonConvert sort this out. MessageUtil::jsonConvert(typed_struct.value(), validation_visitor, out_proto); } } // out_proto is expecting Struct, unpack directly diff --git a/source/common/config/utility.h b/source/common/config/utility.h index bbd25757d3e1..865f839b8b91 100644 --- a/source/common/config/utility.h +++ b/source/common/config/utility.h @@ -228,8 +228,8 @@ class Utility { // Fail in an obvious way if a plugin does not return a proto. RELEASE_ASSERT(config != nullptr, ""); - translateOpaqueConfig(factory.name(), enclosing_message.typed_config(), - enclosing_message.config(), validation_visitor, *config); + translateOpaqueConfig(enclosing_message.typed_config(), enclosing_message.config(), + validation_visitor, *config); return config; } @@ -271,14 +271,12 @@ 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(absl::string_view extension_name, - const ProtobufWkt::Any& typed_config, + static void translateOpaqueConfig(const ProtobufWkt::Any& typed_config, const ProtobufWkt::Struct& config, ProtobufMessage::ValidationVisitor& validation_visitor, Protobuf::Message& out_proto); diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index 4893f9a0794e..b810720dd15a 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -1,142 +1,32 @@ #include "common/config/version_converter.h" -#include "common/common/assert.h" - -// Protobuf reflection is on a per-scalar type basis, i.e. there are method to -// get/set uint32. So, we need to use macro magic to reduce boiler plate below -// when copying fields. -#define UPGRADE_SCALAR(type, method_fragment) \ - case Protobuf::FieldDescriptor::TYPE_##type: { \ - if (prev_field_descriptor->is_repeated()) { \ - const int field_size = prev_reflection->FieldSize(prev_message, prev_field_descriptor); \ - for (int n = 0; n < field_size; ++n) { \ - const auto& v = \ - prev_reflection->GetRepeated##method_fragment(prev_message, prev_field_descriptor, n); \ - target_reflection->Add##method_fragment(target_message, target_field_descriptor, v); \ - } \ - } else { \ - const auto v = prev_reflection->Get##method_fragment(prev_message, prev_field_descriptor); \ - target_reflection->Set##method_fragment(target_message, target_field_descriptor, v); \ - } \ - break; \ - } +#include "common/config/api_type_oracle.h" namespace Envoy { namespace Config { -// TODO(htuch): make the unknown field validators aware of this distinguished -// field, and don't reject if present. -constexpr uint32_t DeprecatedMessageFieldNumber = 100000; - void VersionConverter::upgrade(const Protobuf::Message& prev_message, Protobuf::Message& next_message) { - // Wow, why so complicated? Could we just do this conversion with: - // - // next_message.MergeFromString(prev_message.SerializeAsString()) - // - // and then some clever mangling of the UnknownFieldSet? - // - // Hold your horses! There's a few reasons that the approach below has been - // adopted: - // 1. We can ensure all unknown fields are placed in a distinguished - // DeprecatedMessageFieldNumber, so that the static/dynamic proto - // validators that look at unknown fields are capable of knowing the - // difference between deprecated fields smuggled in from previous versions - // and fields in the new version that are genuinely unknown by the Envoy. - // 2. We can do proto wire breaking changes between major versions. An example - // of this is promotion/demotion between wrapped (e.g. - // google.protobuf.UInt32) and unwrapped types (e.g. uint32). This isn't - // done below yet, but should be possible to automate via "next version" - // annotations on fields. - const Protobuf::Descriptor* next_descriptor = next_message.GetDescriptor(); - const Protobuf::Reflection* prev_reflection = prev_message.GetReflection(); - std::vector prev_field_descriptors; - prev_reflection->ListFields(prev_message, &prev_field_descriptors); - Protobuf::DynamicMessageFactory dmf; - std::unique_ptr deprecated_message; - - // Iterate over all the set fields in the previous version message. - for (const auto* prev_field_descriptor : prev_field_descriptors) { - const Protobuf::Reflection* target_reflection = next_message.GetReflection(); - Protobuf::Message* target_message = &next_message; - - // Does the field exist in the new version message? - const std::string& prev_name = prev_field_descriptor->name(); - const auto* target_field_descriptor = next_descriptor->FindFieldByName(prev_name); - // If we can't find this field in the next version, it must be deprecated. - // So, use deprecated_message and its reflection instead. - if (target_field_descriptor == nullptr) { - ASSERT(prev_field_descriptor->options().deprecated()); - if (!deprecated_message) { - deprecated_message.reset(dmf.GetPrototype(prev_message.GetDescriptor())->New()); - } - target_field_descriptor = prev_field_descriptor; - target_reflection = deprecated_message->GetReflection(); - target_message = deprecated_message.get(); - } - ASSERT(target_field_descriptor != nullptr); - - // These properties are guaranteed by protoxform. - ASSERT(prev_field_descriptor->type() == target_field_descriptor->type()); - ASSERT(prev_field_descriptor->number() == target_field_descriptor->number()); - ASSERT(prev_field_descriptor->type_name() == target_field_descriptor->type_name()); - ASSERT(prev_field_descriptor->is_repeated() == target_field_descriptor->is_repeated()); - - // Message fields need special handling, as we need to recurse. - if (prev_field_descriptor->type() == Protobuf::FieldDescriptor::TYPE_MESSAGE) { - if (prev_field_descriptor->is_repeated()) { - const int field_size = prev_reflection->FieldSize(prev_message, prev_field_descriptor); - for (int n = 0; n < field_size; ++n) { - const Protobuf::Message& prev_nested_message = - prev_reflection->GetRepeatedMessage(prev_message, prev_field_descriptor, n); - Protobuf::Message* target_nested_message = - target_reflection->AddMessage(target_message, target_field_descriptor); - upgrade(prev_nested_message, *target_nested_message); - } - } else { - const Protobuf::Message& prev_nested_message = - prev_reflection->GetMessage(prev_message, prev_field_descriptor); - Protobuf::Message* target_nested_message = - target_reflection->MutableMessage(target_message, target_field_descriptor); - upgrade(prev_nested_message, *target_nested_message); - } - } else { - // Scalar types. - switch (prev_field_descriptor->type()) { - UPGRADE_SCALAR(STRING, String) - UPGRADE_SCALAR(BYTES, String) - UPGRADE_SCALAR(INT32, Int32) - UPGRADE_SCALAR(INT64, Int64) - UPGRADE_SCALAR(UINT32, UInt32) - UPGRADE_SCALAR(UINT64, UInt64) - UPGRADE_SCALAR(DOUBLE, Double) - UPGRADE_SCALAR(FLOAT, Float) - UPGRADE_SCALAR(BOOL, Bool) - UPGRADE_SCALAR(ENUM, EnumValue) - default: - NOT_REACHED_GCOVR_EXCL_LINE; - } - } - } - - if (deprecated_message) { - const Protobuf::Reflection* next_reflection = next_message.GetReflection(); - auto* unknown_field_set = next_reflection->MutableUnknownFields(&next_message); - ASSERT(unknown_field_set->empty()); - std::string* s = unknown_field_set->AddLengthDelimited(DeprecatedMessageFieldNumber); - deprecated_message->SerializeToString(s); - } + std::string s; + prev_message.SerializeToString(&s); + next_message.ParseFromString(s); } -void VersionConverter::unpackDeprecated(const Protobuf::Message& upgraded_message, - Protobuf::Message& deprecated_message) { - const Protobuf::Reflection* reflection = upgraded_message.GetReflection(); - const auto& unknown_field_set = reflection->GetUnknownFields(upgraded_message); - ASSERT(unknown_field_set.field_count() == 1); - const auto& unknown_field = unknown_field_set.field(0); - ASSERT(unknown_field.number() == DeprecatedMessageFieldNumber); - const std::string& s = unknown_field.length_delimited(); - deprecated_message.ParseFromString(s); +DowngradedMessagePtr VersionConverter::downgrade(const Protobuf::Message& message) { + auto downgraded_message = std::make_unique(); + const Protobuf::Descriptor* prev_desc = ApiTypeOracle::getEarlierVersionDescriptor(message); + if (prev_desc != nullptr) { + downgraded_message->msg_.reset(downgraded_message->dmf_.GetPrototype(prev_desc)->New()); + std::string s; + message.SerializeToString(&s); + downgraded_message->msg_->ParseFromString(s); + return downgraded_message; + } + // Unnecessary copy.. + const Protobuf::Descriptor* desc = message.GetDescriptor(); + downgraded_message->msg_.reset(downgraded_message->dmf_.GetPrototype(desc)->New()); + downgraded_message->msg_->MergeFrom(message); + return downgraded_message; } } // namespace Config diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index d5e2db8af36f..e837a1cbc191 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -2,36 +2,41 @@ #include "common/protobuf/protobuf.h" +// Convenience macro for downgrading a message and obtaining a reference. +#define API_DOWNGRADE(msg) (*Config::VersionConverter::downgrade(msg)->msg_) + namespace Envoy { namespace Config { +// A message that has been downgraded, e.g. from v3alphato v2. +struct DowngradedMessage { + // The dynamic message factory must outlive the message. + Protobuf::DynamicMessageFactory dmf_; + + // Downgraded message. + std::unique_ptr msg_; +}; + +using DowngradedMessagePtr = std::unique_ptr; + class VersionConverter { public: /** * Upgrade a message from an earlier to later version of the Envoy API. This - * uses reflection techniques to perform the vN -> v(N+1) protobuf C++ type - * lifting which is the moral equivalent of what protoxform does on a - * migration. That is, most fields are by default unchanged in name, type, - * etc. and so can be trivially copied and then a series of mechanical - * transforms allows for a stylized, information preserving upgrade via - * reflection. For example, the UnknownFieldSet in next_message generated by - * protoxform is used to provide access to deprecated fields in the previous - * API version. + * performs a simple wire-level reinterpreation of the fields. As a result of + * shadow protos, earlier deprecated fields such as foo are rematerialized as + * hidden_envoy_deprecated_foo. * * @param prev_message previous version message input. * @param next_nmessage next version message to generate. */ static void upgrade(const Protobuf::Message& prev_message, Protobuf::Message& next_message); - /** - * Access the deprecated fields stored in the UnknownFieldSet of a message - * generated via VersionConverter::upgrade(). - * - * @param upggraded_message the result of VersionConverter::upgrade(). - * @param deprecated_message a copy of the deprecated fields. - */ - static void unpackDeprecated(const Protobuf::Message& upgraded_message, - Protobuf::Message& deprecated_message); + // Downgrade a message to the previous version. If no previous version exists, + // the given message is copied in the return value. This is not super + // efficient, most uses are expected to be tests and performance agnostic + // code. + static DowngradedMessagePtr downgrade(const Protobuf::Message& message); }; } // namespace Config diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index 6f41286ad2bd..3f7f69b3c010 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -65,6 +65,8 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/common:hash_lib", "//source/common/common:utility_lib", + "//source/common/config:api_type_oracle_lib", + "//source/common/config:version_converter_lib", "@envoy_api//envoy/type:pkg_cc_proto", ], ) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 561e424206b3..2b9643d0cfa1 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -8,6 +8,8 @@ #include "common/common/assert.h" #include "common/common/fmt.h" +#include "common/config/api_type_oracle.h" +#include "common/config/version_converter.h" #include "common/protobuf/message_validator_impl.h" #include "common/protobuf/protobuf.h" @@ -109,6 +111,49 @@ void jsonConvertInternal(const Protobuf::Message& source, MessageUtil::loadFromJson(json, dest, validation_visitor); } +// Apply a function transforming a message (e.g. loading JSON into the message). +// First we try with the message's earlier type, and if unsuccessful (or no +// earlier) type, then the current type. This allows us to take a v3 Envoy +// internal proto and ingest both v2 and v3 in methods such as loadFromJson. +// This relies on the property that any v3 configuration that is ingestible as +// v2 has the same semantics in v2/v3, which holds due to the highly structured +// vN/v(N+1) mechanical transforms. +void tryWithApiBoosting(std::function f, Protobuf::Message& message) { + const Protobuf::Descriptor* earlier_version_desc = + Config::ApiTypeOracle::getEarlierVersionDescriptor(message); + // If there is no earlier version of a message, just apply f directly. + if (earlier_version_desc == nullptr) { + f(message); + return; + } + Protobuf::DynamicMessageFactory dmf; + auto earlier_message = ProtobufTypes::MessagePtr(dmf.GetPrototype(earlier_version_desc)->New()); + ASSERT(earlier_message != nullptr); + try { + // Try apply f with an earlier version of the message, then upgrade the + // result. + f(*earlier_message); + Config::VersionConverter::upgrade(*earlier_message, message); + } catch (EnvoyException&) { + // If we fail at the earlier version, try f at the current version of the + // message. + bool newer_error = false; + try { + f(message); + } catch (EnvoyException&) { + // If we fail this time, we should throw. + // TODO(htuch): currently throwing the v2 error, rather than v3 error, to + // make it easier to handle existing v2 tests during API boosting. We + // probably want to provide a more details exception message contaiing + // both v2 and v3 exception info. + newer_error = true; + } + if (newer_error) { + throw; + } + } +} + } // namespace namespace ProtobufPercentHelper { @@ -174,38 +219,42 @@ size_t MessageUtil::hash(const Protobuf::Message& message) { void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { - Protobuf::util::JsonParseOptions options; - options.case_insensitive_enum_parsing = true; - // Let's first try and get a clean parse when checking for unknown fields; - // this should be the common case. - options.ignore_unknown_fields = false; - const auto strict_status = Protobuf::util::JsonStringToMessage(json, &message, options); - if (strict_status.ok()) { - // Success, no need to do any extra work. - return; - } - // If we fail, we see if we get a clean parse when allowing unknown fields. - // This is essentially a workaround - // for https://github.com/protocolbuffers/protobuf/issues/5967. - // TODO(htuch): clean this up when protobuf supports JSON/YAML unknown field - // detection directly. - options.ignore_unknown_fields = true; - const auto relaxed_status = Protobuf::util::JsonStringToMessage(json, &message, options); - // If we still fail with relaxed unknown field checking, the error has nothing - // to do with unknown fields. - if (!relaxed_status.ok()) { - throw EnvoyException("Unable to parse JSON as proto (" + relaxed_status.ToString() + - "): " + json); - } - // We know it's an unknown field at this point. - validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + - strict_status.ToString()); + tryWithApiBoosting( + [&json, &validation_visitor](Protobuf::Message& message) { + Protobuf::util::JsonParseOptions options; + options.case_insensitive_enum_parsing = true; + // Let's first try and get a clean parse when checking for unknown fields; + // this should be the common case. + options.ignore_unknown_fields = false; + const auto strict_status = Protobuf::util::JsonStringToMessage(json, &message, options); + if (strict_status.ok()) { + // Success, no need to do any extra work. + return; + } + // If we fail, we see if we get a clean parse when allowing unknown fields. + // This is essentially a workaround + // for https://github.com/protocolbuffers/protobuf/issues/5967. + // TODO(htuch): clean this up when protobuf supports JSON/YAML unknown field + // detection directly. + options.ignore_unknown_fields = true; + const auto relaxed_status = Protobuf::util::JsonStringToMessage(json, &message, options); + // If we still fail with relaxed unknown field checking, the error has nothing + // to do with unknown fields. + if (!relaxed_status.ok()) { + throw EnvoyException("Unable to parse JSON as proto (" + relaxed_status.ToString() + + "): " + json); + } + // We know it's an unknown field at this point. + validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + + strict_status.ToString()); + }, + message); } void MessageUtil::loadFromJson(const std::string& json, ProtobufWkt::Struct& message) { // No need to validate if converting to a Struct, since there are no unknown // fields possible. - return loadFromJson(json, message, ProtobufMessage::getNullValidationVisitor()); + loadFromJson(json, message, ProtobufMessage::getNullValidationVisitor()); } void MessageUtil::loadFromYaml(const std::string& yaml, Protobuf::Message& message, @@ -431,11 +480,15 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa } void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { - if (!any_message.UnpackTo(&message)) { - throw EnvoyException(fmt::format("Unable to unpack as {}: {}", - message.GetDescriptor()->full_name(), - any_message.DebugString())); - } + tryWithApiBoosting( + [&any_message](Protobuf::Message& message) { + if (!any_message.UnpackTo(&message)) { + throw EnvoyException(fmt::format("Unable to unpack as {}: {}", + message.GetDescriptor()->full_name(), + any_message.DebugString())); + } + }, + message); } void MessageUtil::jsonConvert(const Protobuf::Message& source, ProtobufWkt::Struct& dest) { diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc index 30646555f58a..9689bfb1940e 100644 --- a/source/common/router/config_impl.cc +++ b/source/common/router/config_impl.cc @@ -1189,8 +1189,7 @@ 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(name, typed_config, config, validator, - *proto_config); + Envoy::Config::Utility::translateOpaqueConfig(typed_config, config, validator, *proto_config); return factory.createRouteSpecificFilterConfig(*proto_config, factory_context, validator); } diff --git a/source/common/upstream/cluster_factory_impl.h b/source/common/upstream/cluster_factory_impl.h index 07ea992b4736..a064a8279cfd 100644 --- a/source/common/upstream/cluster_factory_impl.h +++ b/source/common/upstream/cluster_factory_impl.h @@ -174,9 +174,8 @@ template class ConfigurableClusterFactoryBase : public Clust Stats::ScopePtr&& stats_scope) override { ProtobufTypes::MessagePtr config = createEmptyConfigProto(); Config::Utility::translateOpaqueConfig( - cluster.cluster_type().name(), cluster.cluster_type().typed_config(), - ProtobufWkt::Struct::default_instance(), socket_factory_context.messageValidationVisitor(), - *config); + cluster.cluster_type().typed_config(), ProtobufWkt::Struct::default_instance(), + socket_factory_context.messageValidationVisitor(), *config); return createClusterWithConfig(cluster, MessageUtil::downcastAndValidate( *config, context.messageValidationVisitor()), diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index d359f1d5bf75..b7e20aed1647 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -143,7 +143,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(name, typed_config, config, validation_visitor, + Envoy::Config::Utility::translateOpaqueConfig(typed_config, config, validation_visitor, *proto_config); return factory->createProtocolOptionsConfig(*proto_config, validation_visitor); diff --git a/source/extensions/filters/network/dubbo_proxy/config.cc b/source/extensions/filters/network/dubbo_proxy/config.cc index 154915b0cbd9..12214fc7d244 100644 --- a/source/extensions/filters/network/dubbo_proxy/config.cc +++ b/source/extensions/filters/network/dubbo_proxy/config.cc @@ -146,7 +146,7 @@ void ConfigImpl::registerFilter(const DubboFilterConfig& proto_config) { Envoy::Config::Utility::getAndCheckFactory( string_name); ProtobufTypes::MessagePtr message = factory.createEmptyConfigProto(); - Envoy::Config::Utility::translateOpaqueConfig(string_name, proto_config.config(), + Envoy::Config::Utility::translateOpaqueConfig(proto_config.config(), ProtobufWkt::Struct::default_instance(), context_.messageValidationVisitor(), *message); DubboFilters::FilterFactoryCb callback = diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 8490bd6923b5..043f73685d68 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -94,6 +94,7 @@ envoy_cc_test( "//source/common/config:grpc_mux_lib", "//source/common/config:protobuf_link_hacks", "//source/common/config:resources_lib", + "//source/common/config:version_converter_lib", "//source/common/protobuf", "//source/common/stats:isolated_store_lib", "//test/mocks:common_lib", @@ -116,6 +117,7 @@ envoy_cc_test( "//source/common/config:new_grpc_mux_lib", "//source/common/config:protobuf_link_hacks", "//source/common/config:resources_lib", + "//source/common/config:version_converter_lib", "//source/common/protobuf", "//source/common/stats:isolated_store_lib", "//test/mocks:common_lib", @@ -162,6 +164,7 @@ envoy_cc_test_library( "//source/common/config:api_version_lib", "//source/common/config:grpc_subscription_lib", "//source/common/config:resources_lib", + "//source/common/config:version_converter_lib", "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", "//test/mocks/grpc:grpc_mocks", @@ -179,6 +182,7 @@ envoy_cc_test_library( deps = [ ":subscription_test_harness", "//source/common/config:delta_subscription_lib", + "//source/common/config:version_converter_lib", "//source/common/grpc:common_lib", "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", @@ -289,6 +293,7 @@ envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], deps = [ + "//source/common/config:api_version_lib", "//source/common/config:utility_lib", "//source/common/config:well_known_names", "//source/common/stats:stats_lib", @@ -302,6 +307,7 @@ envoy_cc_test( "@com_github_cncf_udpa//udpa/type/v1:pkg_cc_proto", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/api/v2/core:pkg_cc_proto", + "@envoy_api//envoy/api/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", ], ) diff --git a/test/common/config/api_type_oracle_test.cc b/test/common/config/api_type_oracle_test.cc index 4df740356b11..61b1740232f7 100644 --- a/test/common/config/api_type_oracle_test.cc +++ b/test/common/config/api_type_oracle_test.cc @@ -3,23 +3,26 @@ #include "common/config/api_type_oracle.h" -// For proto descriptors only - #include "gtest/gtest.h" -#include "udpa/type/v1/typed_struct.pb.h" + +// API_NO_BOOST_FILE namespace Envoy { namespace Config { namespace { TEST(ApiTypeOracleTest, All) { - // For proto descriptors only - static_cast(envoy::config::filter::http::ip_tagging::v2::IPTagging::RequestType()); - static_cast(envoy::config::filter::http::ip_tagging::v3alpha::IPTagging::RequestType()); + envoy::config::filter::http::ip_tagging::v2::IPTagging v2_config; + envoy::config::filter::http::ip_tagging::v3alpha::IPTagging v3_config; + ProtobufWkt::Any non_api_type; - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor("foo", {}, "")); - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor("envoy.ip_tagging", {}, "")); + EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(non_api_type)); + EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(v2_config)); + const auto* desc = ApiTypeOracle::getEarlierVersionDescriptor(v3_config); + EXPECT_EQ(envoy::config::filter::http::ip_tagging::v3alpha::IPTagging::descriptor()->name(), + desc->name()); +#if 0 // Struct upgrade to v3alpha. { const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( @@ -72,6 +75,7 @@ TEST(ApiTypeOracleTest, All) { EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v2.IPTagging")); +#endif } } // namespace diff --git a/test/common/config/delta_subscription_test_harness.h b/test/common/config/delta_subscription_test_harness.h index 2ed0609d7251..778a426dbae1 100644 --- a/test/common/config/delta_subscription_test_harness.h +++ b/test/common/config/delta_subscription_test_harness.h @@ -7,6 +7,7 @@ #include "envoy/api/v2/eds.pb.h" #include "common/config/delta_subscription_impl.h" +#include "common/config/version_converter.h" #include "common/grpc/common.h" #include "test/common/config/subscription_test_harness.h" @@ -140,7 +141,7 @@ class DeltaSubscriptionTestHarness : public SubscriptionTestHarness { auto* resource = response->add_resources(); resource->set_name(cluster); resource->set_version(version); - resource->mutable_resource()->PackFrom(*load_assignment); + resource->mutable_resource()->PackFrom(API_DOWNGRADE(*load_assignment)); } } Protobuf::RepeatedPtrField removed_resources; diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 2cb73cdda08b..20cbe450ee42 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -9,6 +9,7 @@ #include "common/config/protobuf_link_hacks.h" #include "common/config/resources.h" #include "common/config/utility.h" +#include "common/config/version_converter.h" #include "common/protobuf/protobuf.h" #include "common/stats/isolated_store_impl.h" @@ -71,7 +72,7 @@ class GrpcMuxImplTestBase : public testing::Test { const std::string& error_message = "") { API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_request; if (first) { - expected_request.mutable_node()->CopyFrom(local_info_.node()); + expected_request.mutable_node()->CopyFrom(API_DOWNGRADE(local_info_.node())); } for (const auto& resource : resource_names) { expected_request.add_resource_names(resource); @@ -254,14 +255,14 @@ TEST_F(GrpcMuxImplTest, WildcardWatch) { response->set_version_info("1"); envoy::api::v2::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); - response->add_resources()->PackFrom(load_assignment); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment)); EXPECT_CALL(callbacks_, onConfigUpdate(_, "1")) .WillOnce( Invoke([&load_assignment](const Protobuf::RepeatedPtrField& resources, const std::string&) { EXPECT_EQ(1, resources.size()); - envoy::api::v2::ClusterLoadAssignment expected_assignment; - resources[0].UnpackTo(&expected_assignment); + envoy::api::v2::ClusterLoadAssignment expected_assignment = + MessageUtil::anyConvert(resources[0]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); expectSendMessage(type_url, {}, "1"); @@ -290,15 +291,15 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { response->set_version_info("1"); envoy::api::v2::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); - response->add_resources()->PackFrom(load_assignment); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment)); EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "1")).Times(0); EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "1")) .WillOnce( Invoke([&load_assignment](const Protobuf::RepeatedPtrField& resources, const std::string&) { EXPECT_EQ(1, resources.size()); - envoy::api::v2::ClusterLoadAssignment expected_assignment; - resources[0].UnpackTo(&expected_assignment); + envoy::api::v2::ClusterLoadAssignment expected_assignment = + MessageUtil::anyConvert(resources[0]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); expectSendMessage(type_url, {"y", "z", "x"}, "1"); @@ -312,22 +313,23 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { response->set_version_info("2"); envoy::api::v2::ClusterLoadAssignment load_assignment_x; load_assignment_x.set_cluster_name("x"); - response->add_resources()->PackFrom(load_assignment_x); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_x)); envoy::api::v2::ClusterLoadAssignment load_assignment_y; load_assignment_y.set_cluster_name("y"); - response->add_resources()->PackFrom(load_assignment_y); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_y)); envoy::api::v2::ClusterLoadAssignment load_assignment_z; load_assignment_z.set_cluster_name("z"); - response->add_resources()->PackFrom(load_assignment_z); + response->add_resources()->PackFrom(API_DOWNGRADE(load_assignment_z)); EXPECT_CALL(bar_callbacks, onConfigUpdate(_, "2")) .WillOnce(Invoke( [&load_assignment_y, &load_assignment_z]( const Protobuf::RepeatedPtrField& resources, const std::string&) { EXPECT_EQ(2, resources.size()); - envoy::api::v2::ClusterLoadAssignment expected_assignment; - resources[0].UnpackTo(&expected_assignment); + envoy::api::v2::ClusterLoadAssignment expected_assignment = + MessageUtil::anyConvert(resources[0]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); - resources[1].UnpackTo(&expected_assignment); + expected_assignment = + MessageUtil::anyConvert(resources[1]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_z)); })); EXPECT_CALL(foo_callbacks, onConfigUpdate(_, "2")) @@ -335,10 +337,11 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { [&load_assignment_x, &load_assignment_y]( const Protobuf::RepeatedPtrField& resources, const std::string&) { EXPECT_EQ(2, resources.size()); - envoy::api::v2::ClusterLoadAssignment expected_assignment; - resources[0].UnpackTo(&expected_assignment); + envoy::api::v2::ClusterLoadAssignment expected_assignment = + MessageUtil::anyConvert(resources[0]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_x)); - resources[1].UnpackTo(&expected_assignment); + expected_assignment = + MessageUtil::anyConvert(resources[1]); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment_y)); })); expectSendMessage(type_url, {"y", "z", "x"}, "2"); diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index 08ac0165891e..b173e379700a 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -10,6 +10,7 @@ #include "common/config/api_version.h" #include "common/config/grpc_subscription_impl.h" #include "common/config/resources.h" +#include "common/config/version_converter.h" #include "test/common/config/subscription_test_harness.h" #include "test/mocks/config/mocks.h" @@ -65,7 +66,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { UNREFERENCED_PARAMETER(expect_node); API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_request; if (expect_node) { - expected_request.mutable_node()->CopyFrom(node_); + expected_request.mutable_node()->CopyFrom(API_DOWNGRADE(node_)); } for (const auto& cluster : cluster_names) { expected_request.add_resource_names(cluster); @@ -104,7 +105,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { last_cluster_names_.end()) { envoy::api::v2::ClusterLoadAssignment* load_assignment = typed_resources.Add(); load_assignment->set_cluster_name(cluster); - response->add_resources()->PackFrom(*load_assignment); + response->add_resources()->PackFrom(API_DOWNGRADE(*load_assignment)); } } EXPECT_CALL(callbacks_, onConfigUpdate(RepeatedProtoEq(response->resources()), version)) diff --git a/test/common/config/new_grpc_mux_impl_test.cc b/test/common/config/new_grpc_mux_impl_test.cc index 57b1373dab22..fd138174afdc 100644 --- a/test/common/config/new_grpc_mux_impl_test.cc +++ b/test/common/config/new_grpc_mux_impl_test.cc @@ -8,6 +8,7 @@ #include "common/config/protobuf_link_hacks.h" #include "common/config/resources.h" #include "common/config/utility.h" +#include "common/config/version_converter.h" #include "common/protobuf/protobuf.h" #include "common/stats/isolated_store_impl.h" @@ -97,15 +98,16 @@ TEST_F(NewGrpcMuxImplTest, DiscoveryResponseNonexistentSub) { response->set_system_version_info("1"); envoy::api::v2::ClusterLoadAssignment load_assignment; load_assignment.set_cluster_name("x"); - response->add_resources()->mutable_resource()->PackFrom(load_assignment); + response->add_resources()->mutable_resource()->PackFrom(API_DOWNGRADE(load_assignment)); EXPECT_CALL(callbacks_, onConfigUpdate(_, _, "1")) .WillOnce( Invoke([&load_assignment]( const Protobuf::RepeatedPtrField& added_resources, const Protobuf::RepeatedPtrField&, const std::string&) { EXPECT_EQ(1, added_resources.size()); - envoy::api::v2::ClusterLoadAssignment expected_assignment; - added_resources[0].resource().UnpackTo(&expected_assignment); + envoy::api::v2::ClusterLoadAssignment expected_assignment = + MessageUtil::anyConvert( + added_resources[0].resource()); EXPECT_TRUE(TestUtility::protoEqual(expected_assignment, load_assignment)); })); grpc_mux_->onDiscoveryResponse(std::move(response)); diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index be838813150b..a02f6f50094e 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -1,10 +1,12 @@ #include "envoy/api/v2/cds.pb.h" #include "envoy/api/v2/core/config_source.pb.h" #include "envoy/api/v2/core/grpc_service.pb.h" +#include "envoy/api/v3alpha/cds.pb.h" #include "envoy/common/exception.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" #include "common/common/fmt.h" +#include "common/config/api_version.h" #include "common/config/utility.h" #include "common/config/well_known_names.h" #include "common/protobuf/protobuf.h" @@ -272,7 +274,7 @@ TEST(UtilityTest, AnyWrongType) { typed_config.PackFrom(source_duration); ProtobufWkt::Timestamp out; EXPECT_THROW_WITH_REGEX( - Utility::translateOpaqueConfig("", typed_config, ProtobufWkt::Struct(), + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), ProtobufMessage::getStrictValidationVisitor(), out), EnvoyException, R"(Unable to unpack as google.protobuf.Timestamp: \[type.googleapis.com/google.protobuf.Duration\] .*)"); @@ -294,7 +296,7 @@ TEST(UtilityTest, TypedStructToStruct) { packTypedStructIntoAny(typed_config, untyped_struct); ProtobufWkt::Struct out; - Utility::translateOpaqueConfig("", typed_config, ProtobufWkt::Struct(), + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), ProtobufMessage::getStrictValidationVisitor(), out); EXPECT_THAT(out, ProtoEq(untyped_struct)); @@ -315,7 +317,7 @@ TEST(UtilityTest, TypedStructToBootstrap) { packTypedStructIntoAny(typed_config, bootstrap); envoy::config::bootstrap::v2::Bootstrap out; - Utility::translateOpaqueConfig("", typed_config, ProtobufWkt::Struct(), + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), ProtobufMessage::getStrictValidationVisitor(), out); EXPECT_THAT(out, ProtoEq(bootstrap)); } @@ -335,14 +337,68 @@ TEST(UtilityTest, TypedStructToInvalidType) { packTypedStructIntoAny(typed_config, bootstrap); ProtobufWkt::Any out; - EXPECT_THROW_WITH_MESSAGE( - Utility::translateOpaqueConfig("", typed_config, ProtobufWkt::Struct(), + EXPECT_THROW_WITH_REGEX( + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), ProtobufMessage::getStrictValidationVisitor(), out), - EnvoyException, - "Invalid proto type.\nExpected google.protobuf.Any\nActual: " - "envoy.config.bootstrap.v2.Bootstrap"); + EnvoyException, "Unable to parse JSON as proto"); } +// TODO(htuch): write a bunch of tests for translateOpaqueConfig that reflect +// the tests we used to have for ApiTypeOracle. +#if 0 + // Struct upgrade to v3alpha. + { + const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); + EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); + } + + // Any upgrade from v2 to v3alpha. + { + ProtobufWkt::Any typed_config; + typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); + const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); + EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); + } + + // There is no upgrade for same Any and target type URL. + { + ProtobufWkt::Any typed_config; + typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); + EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", typed_config, + "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); + } + + // TypedStruct upgrade from v2 to v3alpha. + { + ProtobufWkt::Any typed_config; + udpa::type::v1::TypedStruct typed_struct; + typed_struct.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); + typed_config.PackFrom(typed_struct); + const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); + EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); + } + + // There is no upgrade for same TypedStruct and target type URL. + { + ProtobufWkt::Any typed_config; + udpa::type::v1::TypedStruct typed_struct; + typed_struct.set_type_url( + "type.googleapis.com/envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); + typed_config.PackFrom(typed_struct); + EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", typed_config, + "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); + } + + // There is no upgrade for v2. + EXPECT_EQ(nullptr, + ApiTypeOracle::inferEarlierVersionDescriptor( + "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v2.IPTagging")); +#endif TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTypes) { envoy::api::v2::core::ConfigSource config; auto* api_config_source = config.mutable_api_config_source(); diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc index e880cf10a126..0beafcbda4d2 100644 --- a/test/common/config/version_converter_test.cc +++ b/test/common/config/version_converter_test.cc @@ -9,6 +9,7 @@ namespace Envoy { namespace Config { namespace { +#if 0 TEST(VersionConverterTest, All) { test::common::config::PreviousVersion previous_version; const std::string previous_version_yaml = R"EOF( @@ -85,6 +86,7 @@ TEST(VersionConverterTest, All) { next_version.enum_field_with_deprecated_value()); EXPECT_EQ(enum_with_deprecated_value, test::common::config::PREV_DEPRECATED_VALUE); } +#endif } // namespace } // namespace Config diff --git a/test/extensions/clusters/aggregate/cluster_test.cc b/test/extensions/clusters/aggregate/cluster_test.cc index 4552132c667d..af33b6d0a221 100644 --- a/test/extensions/clusters/aggregate/cluster_test.cc +++ b/test/extensions/clusters/aggregate/cluster_test.cc @@ -76,8 +76,7 @@ class AggregateClusterTest : public testing::Test { void initialize(const std::string& yaml_config) { envoy::api::v2::Cluster cluster_config = Upstream::parseClusterFromV2Yaml(yaml_config); envoy::config::cluster::aggregate::v2alpha::ClusterConfig config; - Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().name(), - cluster_config.cluster_type().typed_config(), + Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().typed_config(), ProtobufWkt::Struct::default_instance(), ProtobufMessage::getStrictValidationVisitor(), config); Stats::ScopePtr scope = stats_store_.createScope("cluster.name."); diff --git a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc index 05f1e5c70840..5e8a3aa8277d 100644 --- a/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc +++ b/test/extensions/clusters/dynamic_forward_proxy/cluster_test.cc @@ -30,8 +30,7 @@ class ClusterTest : public testing::Test, void initialize(const std::string& yaml_config, bool uses_tls) { envoy::api::v2::Cluster cluster_config = Upstream::parseClusterFromV2Yaml(yaml_config); envoy::config::cluster::dynamic_forward_proxy::v2alpha::ClusterConfig config; - Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().name(), - cluster_config.cluster_type().typed_config(), + Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().typed_config(), ProtobufWkt::Struct::default_instance(), ProtobufMessage::getStrictValidationVisitor(), config); Stats::ScopePtr scope = stats_store_.createScope("cluster.name."); diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 46812d6ccccf..ecc47b258352 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -96,8 +96,7 @@ class RedisClusterTest : public testing::Test, singleton_manager_, tls_, validation_visitor_, *api_); envoy::config::cluster::redis::RedisClusterConfig config; - Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().name(), - cluster_config.cluster_type().typed_config(), + Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().typed_config(), ProtobufWkt::Struct::default_instance(), ProtobufMessage::getStrictValidationVisitor(), config); cluster_callback_ = std::make_shared>(); @@ -127,9 +126,9 @@ class RedisClusterTest : public testing::Test, singleton_manager_, tls_, validation_visitor_, *api_); envoy::config::cluster::redis::RedisClusterConfig config; - Config::Utility::translateOpaqueConfig( - cluster_config.cluster_type().name(), cluster_config.cluster_type().typed_config(), - ProtobufWkt::Struct::default_instance(), validation_visitor_, config); + Config::Utility::translateOpaqueConfig(cluster_config.cluster_type().typed_config(), + ProtobufWkt::Struct::default_instance(), + validation_visitor_, config); NiceMock log_manager; NiceMock outlier_event_logger; diff --git a/test/integration/BUILD b/test/integration/BUILD index 6d01661a7e7c..495e9c64951b 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -484,6 +484,7 @@ envoy_cc_test_library( "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:api_version_lib", + "//source/common/config:version_converter_lib", "//source/common/event:dispatcher_lib", "//source/common/grpc:codec_lib", "//source/common/grpc:common_lib", diff --git a/test/integration/integration.h b/test/integration/integration.h index 1297379c5042..72b057b1ae19 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -10,6 +10,7 @@ #include "envoy/server/process_context.h" #include "common/config/api_version.h" +#include "common/config/version_converter.h" #include "common/http/codec_client.h" #include "test/common/grpc/grpc_client_integration.h" @@ -276,7 +277,7 @@ class BaseIntegrationTest : protected Logger::Loggable { discovery_response.set_version_info(version); discovery_response.set_type_url(type_url); for (const auto& message : messages) { - discovery_response.add_resources()->PackFrom(message); + discovery_response.add_resources()->PackFrom(API_DOWNGRADE(message)); } static int next_nonce_counter = 0; discovery_response.set_nonce(absl::StrCat("nonce", next_nonce_counter++)); @@ -300,10 +301,10 @@ class BaseIntegrationTest : protected Logger::Loggable { for (const auto& message : added_or_updated) { auto* resource = response.add_resources(); ProtobufWkt::Any temp_any; - temp_any.PackFrom(message); + temp_any.PackFrom(API_DOWNGRADE(message)); resource->set_name(TestUtility::xdsResourceName(temp_any)); resource->set_version(version); - resource->mutable_resource()->PackFrom(message); + resource->mutable_resource()->PackFrom(API_DOWNGRADE(message)); } *response.mutable_removed_resources() = {removed.begin(), removed.end()}; static int next_nonce_counter = 0; diff --git a/test/integration/scoped_rds_integration_test.cc b/test/integration/scoped_rds_integration_test.cc index e59342544aa3..81307ce10562 100644 --- a/test/integration/scoped_rds_integration_test.cc +++ b/test/integration/scoped_rds_integration_test.cc @@ -8,6 +8,7 @@ #include "common/config/api_version.h" #include "common/config/resources.h" +#include "common/config/version_converter.h" #include "test/common/grpc/grpc_client_integration.h" #include "test/integration/http_integration.h" @@ -166,7 +167,7 @@ class ScopedRdsIntegrationTest : public HttpIntegrationTest, response.set_type_url(Config::TypeUrl::get().RouteConfiguration); auto route_configuration = TestUtility::parseYaml(route_config); - response.add_resources()->PackFrom(route_configuration); + response.add_resources()->PackFrom(API_DOWNGRADE(route_configuration)); ASSERT(rds_upstream_info_.stream_by_resource_name_[route_configuration.name()] != nullptr); rds_upstream_info_.stream_by_resource_name_[route_configuration.name()]->sendGrpcMessage( response); @@ -201,7 +202,7 @@ class ScopedRdsIntegrationTest : public HttpIntegrationTest, auto resource = response.add_resources(); resource->set_name(scoped_route_proto.name()); resource->set_version(version); - resource->mutable_resource()->PackFrom(scoped_route_proto); + resource->mutable_resource()->PackFrom(API_DOWNGRADE(scoped_route_proto)); } scoped_rds_upstream_info_.stream_by_resource_name_[srds_config_name_]->sendGrpcMessage( response); @@ -218,7 +219,7 @@ class ScopedRdsIntegrationTest : public HttpIntegrationTest, for (const auto& resource_proto : resource_protos) { envoy::api::v2::ScopedRouteConfiguration scoped_route_proto; TestUtility::loadFromYaml(resource_proto, scoped_route_proto); - response.add_resources()->PackFrom(scoped_route_proto); + response.add_resources()->PackFrom(API_DOWNGRADE(scoped_route_proto)); } scoped_rds_upstream_info_.stream_by_resource_name_[srds_config_name_]->sendGrpcMessage( response); diff --git a/test/integration/sds_dynamic_integration_test.cc b/test/integration/sds_dynamic_integration_test.cc index fe91152ef23b..1ea3f1bc599b 100644 --- a/test/integration/sds_dynamic_integration_test.cc +++ b/test/integration/sds_dynamic_integration_test.cc @@ -121,7 +121,7 @@ class SdsDynamicIntegrationBaseTest : public Grpc::GrpcClientIntegrationParamTes API_NO_BOOST(envoy::api::v2::DiscoveryResponse) discovery_response; discovery_response.set_version_info("1"); discovery_response.set_type_url(Config::TypeUrl::get().Secret); - discovery_response.add_resources()->PackFrom(secret); + discovery_response.add_resources()->PackFrom(API_DOWNGRADE(secret)); xds_stream_->sendGrpcMessage(discovery_response); } From e7e5b46fc293f988ecfe9d872bdcf893f7c69df3 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 27 Dec 2019 10:46:47 -0500 Subject: [PATCH 02/10] Tests (and fixed bug in tryWithApiBoost()). Signed-off-by: Harvey Tuch --- source/common/protobuf/utility.cc | 25 ++- test/common/config/BUILD | 5 +- test/common/config/api_type_oracle_test.cc | 55 ------ test/common/config/utility_test.cc | 198 ++++++++++++------- test/common/config/version_converter_test.cc | 108 ++++------ test/common/protobuf/BUILD | 2 + test/common/protobuf/utility_test.cc | 91 +++++++++ 7 files changed, 275 insertions(+), 209 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 2b9643d0cfa1..2ef10b075008 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -118,12 +118,13 @@ void jsonConvertInternal(const Protobuf::Message& source, // This relies on the property that any v3 configuration that is ingestible as // v2 has the same semantics in v2/v3, which holds due to the highly structured // vN/v(N+1) mechanical transforms. -void tryWithApiBoosting(std::function f, Protobuf::Message& message) { +void tryWithApiBoosting(std::function f, + Protobuf::Message& message) { const Protobuf::Descriptor* earlier_version_desc = Config::ApiTypeOracle::getEarlierVersionDescriptor(message); // If there is no earlier version of a message, just apply f directly. if (earlier_version_desc == nullptr) { - f(message); + f(message, true); return; } Protobuf::DynamicMessageFactory dmf; @@ -132,14 +133,14 @@ void tryWithApiBoosting(std::function f, Protobuf::Mes try { // Try apply f with an earlier version of the message, then upgrade the // result. - f(*earlier_message); + f(*earlier_message, false); Config::VersionConverter::upgrade(*earlier_message, message); } catch (EnvoyException&) { // If we fail at the earlier version, try f at the current version of the // message. bool newer_error = false; try { - f(message); + f(message, true); } catch (EnvoyException&) { // If we fail this time, we should throw. // TODO(htuch): currently throwing the v2 error, rather than v3 error, to @@ -220,7 +221,7 @@ size_t MessageUtil::hash(const Protobuf::Message& message) { void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { tryWithApiBoosting( - [&json, &validation_visitor](Protobuf::Message& message) { + [&json, &validation_visitor](Protobuf::Message& message, bool latest_version) { Protobuf::util::JsonParseOptions options; options.case_insensitive_enum_parsing = true; // Let's first try and get a clean parse when checking for unknown fields; @@ -244,9 +245,15 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa throw EnvoyException("Unable to parse JSON as proto (" + relaxed_status.ToString() + "): " + json); } - // We know it's an unknown field at this point. - validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + - strict_status.ToString()); + // We know it's an unknown field at this point. If we're at the latest + // version, then it's definitely an unknown field, otherwise we try to + // load again at a later version. + if (latest_version) { + validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + + strict_status.ToString()); + } else { + throw EnvoyException("Unknown field, possibly a rename, try again."); + } }, message); } @@ -481,7 +488,7 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { tryWithApiBoosting( - [&any_message](Protobuf::Message& message) { + [&any_message](Protobuf::Message& message, bool) { if (!any_message.UnpackTo(&message)) { throw EnvoyException(fmt::format("Unable to unpack as {}: {}", message.GetDescriptor()->full_name(), diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 043f73685d68..201839300b6a 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -307,8 +307,8 @@ envoy_cc_test( "@com_github_cncf_udpa//udpa/type/v1:pkg_cc_proto", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/api/v2/core:pkg_cc_proto", - "@envoy_api//envoy/api/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", + "@envoy_api//envoy/config/bootstrap/v3alpha:pkg_cc_proto", ], ) @@ -376,8 +376,11 @@ envoy_cc_test( srcs = ["version_converter_test.cc"], deps = [ ":version_converter_proto_cc_proto", + "//source/common/config:api_version_lib", "//source/common/config:version_converter_lib", "//test/test_common:test_time_lib", "//test/test_common:utility_lib", + "@envoy_api//envoy/api/v2:pkg_cc_proto", + "@envoy_api//envoy/api/v3alpha:pkg_cc_proto", ], ) diff --git a/test/common/config/api_type_oracle_test.cc b/test/common/config/api_type_oracle_test.cc index 61b1740232f7..bb8049624850 100644 --- a/test/common/config/api_type_oracle_test.cc +++ b/test/common/config/api_type_oracle_test.cc @@ -21,61 +21,6 @@ TEST(ApiTypeOracleTest, All) { const auto* desc = ApiTypeOracle::getEarlierVersionDescriptor(v3_config); EXPECT_EQ(envoy::config::filter::http::ip_tagging::v3alpha::IPTagging::descriptor()->name(), desc->name()); - -#if 0 - // Struct upgrade to v3alpha. - { - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // Any upgrade from v2 to v3alpha. - { - ProtobufWkt::Any typed_config; - typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // There is no upgrade for same Any and target type URL. - { - ProtobufWkt::Any typed_config; - typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, - "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); - } - - // TypedStruct upgrade from v2 to v3alpha. - { - ProtobufWkt::Any typed_config; - udpa::type::v1::TypedStruct typed_struct; - typed_struct.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); - typed_config.PackFrom(typed_struct); - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // There is no upgrade for same TypedStruct and target type URL. - { - ProtobufWkt::Any typed_config; - udpa::type::v1::TypedStruct typed_struct; - typed_struct.set_type_url( - "type.googleapis.com/envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - typed_config.PackFrom(typed_struct); - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, - "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); - } - - // There is no upgrade for v2. - EXPECT_EQ(nullptr, - ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v2.IPTagging")); -#endif } } // namespace diff --git a/test/common/config/utility_test.cc b/test/common/config/utility_test.cc index a02f6f50094e..c384b4908ce5 100644 --- a/test/common/config/utility_test.cc +++ b/test/common/config/utility_test.cc @@ -1,9 +1,9 @@ #include "envoy/api/v2/cds.pb.h" #include "envoy/api/v2/core/config_source.pb.h" #include "envoy/api/v2/core/grpc_service.pb.h" -#include "envoy/api/v3alpha/cds.pb.h" #include "envoy/common/exception.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" +#include "envoy/config/bootstrap/v3alpha/bootstrap.pb.h" #include "common/common/fmt.h" #include "common/config/api_version.h" @@ -302,24 +302,140 @@ TEST(UtilityTest, TypedStructToStruct) { EXPECT_THAT(out, ProtoEq(untyped_struct)); } +// Verify that regular Struct can be translated into an arbitrary message of correct type +// (v2 API, no upgrading). +TEST(UtilityTest, StructToClusterV2) { + ProtobufWkt::Any typed_config; + API_NO_BOOST(envoy::api::v2::Cluster) cluster; + ProtobufWkt::Struct cluster_struct; + const std::string cluster_config_yaml = R"EOF( + drain_connections_on_host_removal: true + )EOF"; + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + TestUtility::loadFromYaml(cluster_config_yaml, cluster_struct); + + { + API_NO_BOOST(envoy::api::v2::Cluster) out; + Utility::translateOpaqueConfig({}, cluster_struct, ProtobufMessage::getNullValidationVisitor(), + out); + EXPECT_THAT(out, ProtoEq(cluster)); + } + { + API_NO_BOOST(envoy::api::v2::Cluster) out; + Utility::translateOpaqueConfig({}, cluster_struct, + ProtobufMessage::getStrictValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } +} + +// Verify that regular Struct can be translated into an arbitrary message of correct type +// (v3 API, upgrading). +TEST(UtilityTest, StructToClusterV3) { + ProtobufWkt::Any typed_config; + API_NO_BOOST(envoy::api::v3alpha::Cluster) cluster; + ProtobufWkt::Struct cluster_struct; + const std::string cluster_config_yaml = R"EOF( + ignore_health_on_host_removal: true + )EOF"; + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + TestUtility::loadFromYaml(cluster_config_yaml, cluster_struct); + + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) out; + Utility::translateOpaqueConfig({}, cluster_struct, ProtobufMessage::getNullValidationVisitor(), + out); + EXPECT_THAT(out, ProtoEq(cluster)); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) out; + Utility::translateOpaqueConfig({}, cluster_struct, + ProtobufMessage::getStrictValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } +} + // Verify that udpa.type.v1.TypedStruct can be translated into an arbitrary message of correct type -TEST(UtilityTest, TypedStructToBootstrap) { +// (v2 API, no upgrading). +TEST(UtilityTest, TypedStructToClusterV2) { ProtobufWkt::Any typed_config; - envoy::config::bootstrap::v2::Bootstrap bootstrap; - const std::string bootstrap_config_yaml = R"EOF( - admin: - access_log_path: /dev/null - address: - pipe: - path: "/" + API_NO_BOOST(envoy::api::v2::Cluster) cluster; + const std::string cluster_config_yaml = R"EOF( + drain_connections_on_host_removal: true )EOF"; - TestUtility::loadFromYaml(bootstrap_config_yaml, bootstrap); - packTypedStructIntoAny(typed_config, bootstrap); + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + packTypedStructIntoAny(typed_config, cluster); + + { + API_NO_BOOST(envoy::api::v2::Cluster) out; + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), + ProtobufMessage::getNullValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } + { + API_NO_BOOST(envoy::api::v2::Cluster) out; + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), + ProtobufMessage::getStrictValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } +} + +// Verify that udpa.type.v1.TypedStruct can be translated into an arbitrary message of correct type +// (v3 API, upgrading). +TEST(UtilityTest, TypedStructToClusterV3) { + ProtobufWkt::Any typed_config; + API_NO_BOOST(envoy::api::v3alpha::Cluster) cluster; + const std::string cluster_config_yaml = R"EOF( + ignore_health_on_host_removal: true + )EOF"; + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + packTypedStructIntoAny(typed_config, cluster); + + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) out; + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), + ProtobufMessage::getNullValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) out; + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), + ProtobufMessage::getStrictValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); + } +} + +// Verify that Any can be translated into an arbitrary message of correct type +// (v2 API, no upgrading). +TEST(UtilityTest, AnyToClusterV2) { + ProtobufWkt::Any typed_config; + API_NO_BOOST(envoy::api::v2::Cluster) cluster; + const std::string cluster_config_yaml = R"EOF( + drain_connections_on_host_removal: true + )EOF"; + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + typed_config.PackFrom(cluster); + + API_NO_BOOST(envoy::api::v2::Cluster) out; + Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), + ProtobufMessage::getStrictValidationVisitor(), out); + EXPECT_THAT(out, ProtoEq(cluster)); +} + +// Verify that Any can be translated into an arbitrary message of correct type +// (v3 API, upgrading). +TEST(UtilityTest, AnyToClusterV3) { + ProtobufWkt::Any typed_config; + API_NO_BOOST(envoy::api::v3alpha::Cluster) cluster; + const std::string cluster_config_yaml = R"EOF( + ignore_health_on_host_removal: true + )EOF"; + TestUtility::loadFromYaml(cluster_config_yaml, cluster); + typed_config.PackFrom(cluster); - envoy::config::bootstrap::v2::Bootstrap out; + API_NO_BOOST(envoy::api::v3alpha::Cluster) out; Utility::translateOpaqueConfig(typed_config, ProtobufWkt::Struct(), ProtobufMessage::getStrictValidationVisitor(), out); - EXPECT_THAT(out, ProtoEq(bootstrap)); + EXPECT_THAT(out, ProtoEq(cluster)); } // Verify that translation from udpa.type.v1.TypedStruct into message of incorrect type fails @@ -343,62 +459,6 @@ TEST(UtilityTest, TypedStructToInvalidType) { EnvoyException, "Unable to parse JSON as proto"); } -// TODO(htuch): write a bunch of tests for translateOpaqueConfig that reflect -// the tests we used to have for ApiTypeOracle. -#if 0 - // Struct upgrade to v3alpha. - { - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // Any upgrade from v2 to v3alpha. - { - ProtobufWkt::Any typed_config; - typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // There is no upgrade for same Any and target type URL. - { - ProtobufWkt::Any typed_config; - typed_config.set_type_url("envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, - "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); - } - - // TypedStruct upgrade from v2 to v3alpha. - { - ProtobufWkt::Any typed_config; - udpa::type::v1::TypedStruct typed_struct; - typed_struct.set_type_url("envoy.config.filter.http.ip_tagging.v2.IPTagging"); - typed_config.PackFrom(typed_struct); - const auto* desc = ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - EXPECT_EQ("envoy.config.filter.http.ip_tagging.v2.IPTagging", desc->full_name()); - } - - // There is no upgrade for same TypedStruct and target type URL. - { - ProtobufWkt::Any typed_config; - udpa::type::v1::TypedStruct typed_struct; - typed_struct.set_type_url( - "type.googleapis.com/envoy.config.filter.http.ip_tagging.v3alpha.IPTagging"); - typed_config.PackFrom(typed_struct); - EXPECT_EQ(nullptr, ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", typed_config, - "envoy.config.filter.http.ip_tagging.v3alpha.IPTagging")); - } - - // There is no upgrade for v2. - EXPECT_EQ(nullptr, - ApiTypeOracle::inferEarlierVersionDescriptor( - "envoy.ip_tagging", {}, "envoy.config.filter.http.ip_tagging.v2.IPTagging")); -#endif TEST(CheckApiConfigSourceSubscriptionBackingClusterTest, GrpcClusterTestAcrossTypes) { envoy::api::v2::core::ConfigSource config; auto* api_config_source = config.mutable_api_config_source(); diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc index 0beafcbda4d2..ce65a0fac1b6 100644 --- a/test/common/config/version_converter_test.cc +++ b/test/common/config/version_converter_test.cc @@ -1,3 +1,7 @@ +#include "envoy/api/v2/cds.pb.h" +#include "envoy/api/v3alpha/cds.pb.h" + +#include "common/config/api_version.h" #include "common/config/version_converter.h" #include "test/common/config/version_converter.pb.h" @@ -9,84 +13,38 @@ namespace Envoy { namespace Config { namespace { -#if 0 -TEST(VersionConverterTest, All) { - test::common::config::PreviousVersion previous_version; - const std::string previous_version_yaml = R"EOF( - string_field: foo - bytes_field: YWJjMTIzIT8kKiYoKSctPUB+ - int32_field: -1 - int64_field: -2 - uint32_field: 1 - uint64_field: 2 - double_field: 1.0 - float_field: 2.0 - bool_field: true - - enum_field: PREV_OTHER_VALUE - - nested_field: - any_field: - "@type": type.googleapis.com/google.protobuf.UInt32Value - value: 42 - - repeated_scalar_field: ["foo", "bar"] - repeated_nested_field: - - any_field: - "@type": type.googleapis.com/google.protobuf.UInt32Value - value: 1 - - any_field: - "@type": type.googleapis.com/google.protobuf.UInt64Value - value: 2 - - deprecated_field: 1 - enum_field_with_deprecated_value: PREV_DEPRECATED_VALUE - )EOF"; - TestUtility::loadFromYaml(previous_version_yaml, previous_version); - test::common::config::NextVersion next_version; - - VersionConverter::upgrade(previous_version, next_version); - - // Singleton scalars. - EXPECT_EQ(previous_version.string_field(), next_version.string_field()); - EXPECT_EQ(previous_version.bytes_field(), next_version.bytes_field()); - EXPECT_EQ(previous_version.int32_field(), next_version.int32_field()); - EXPECT_EQ(previous_version.int64_field(), next_version.int64_field()); - EXPECT_EQ(previous_version.uint32_field(), next_version.uint32_field()); - EXPECT_EQ(previous_version.uint64_field(), next_version.uint64_field()); - EXPECT_EQ(previous_version.double_field(), next_version.double_field()); - EXPECT_EQ(previous_version.float_field(), next_version.float_field()); - EXPECT_EQ(previous_version.bool_field(), next_version.bool_field()); - EXPECT_EQ(previous_version.enum_field(), next_version.enum_field()); - - // Singleton nested message. - EXPECT_THAT(previous_version.nested_field().any_field(), - ProtoEq(next_version.nested_field().any_field())); - - // Repeated entities. - EXPECT_EQ(previous_version.repeated_scalar_field().size(), - next_version.repeated_scalar_field().size()); - for (int n = 0; n < previous_version.repeated_scalar_field().size(); ++n) { - EXPECT_EQ(previous_version.repeated_scalar_field()[n], next_version.repeated_scalar_field()[n]); - } - EXPECT_EQ(previous_version.repeated_nested_field().size(), - next_version.repeated_nested_field().size()); - for (int n = 0; n < previous_version.repeated_nested_field().size(); ++n) { - EXPECT_THAT(previous_version.repeated_nested_field()[n].any_field(), - ProtoEq(next_version.repeated_nested_field()[n].any_field())); - } +// Wire-style upgrading between versions. +TEST(VersionConverterTest, Upgrade) { + API_NO_BOOST(envoy::api::v2::Cluster) source; + source.set_drain_connections_on_host_removal(true); + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + VersionConverter::upgrade(source, dst); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); +} - // Deprecations. - test::common::config::PreviousVersion deprecated; - VersionConverter::unpackDeprecated(next_version, deprecated); - EXPECT_EQ(previous_version.deprecated_field(), deprecated.deprecated_field()); +// Downgrading to an earlier version (where it exists). +TEST(VersionConverterTest, DowngradeEarlier) { + API_NO_BOOST(envoy::api::v3alpha::Cluster) source; + source.set_ignore_health_on_host_removal(true); + auto downgraded = VersionConverter::downgrade(source); + const Protobuf::Descriptor* desc = downgraded->msg_->GetDescriptor(); + const Protobuf::Reflection* reflection = downgraded->msg_->GetReflection(); + EXPECT_EQ("envoy.api.v2.Cluster", desc->full_name()); + EXPECT_EQ(true, reflection->GetBool(*downgraded->msg_, + desc->FindFieldByName("drain_connections_on_host_removal"))); +} - test::common::config::PreviousEnum enum_with_deprecated_value = - static_cast( - next_version.enum_field_with_deprecated_value()); - EXPECT_EQ(enum_with_deprecated_value, test::common::config::PREV_DEPRECATED_VALUE); +// Downgrading is idempotent if no earlier version. +TEST(VersionConverterTest, DowngradeSame) { + API_NO_BOOST(envoy::api::v2::Cluster) source; + source.set_drain_connections_on_host_removal(true); + auto downgraded = VersionConverter::downgrade(source); + const Protobuf::Descriptor* desc = downgraded->msg_->GetDescriptor(); + const Protobuf::Reflection* reflection = downgraded->msg_->GetReflection(); + EXPECT_EQ("envoy.api.v2.Cluster", desc->full_name()); + EXPECT_EQ(true, reflection->GetBool(*downgraded->msg_, + desc->FindFieldByName("drain_connections_on_host_removal"))); } -#endif } // namespace } // namespace Config diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index f437a1a4bac7..e1e04d13d144 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -23,6 +23,7 @@ envoy_cc_test( name = "utility_test", srcs = ["utility_test.cc"], deps = [ + "//source/common/config:api_version_lib", "//source/common/protobuf:utility_lib", "//source/common/stats:isolated_store_lib", "//test/mocks/init:init_mocks", @@ -34,6 +35,7 @@ envoy_cc_test( "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:pkg_cc_proto", + "@envoy_api//envoy/api/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto", "@envoy_api//envoy/type:pkg_cc_proto", ], diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index bb41d3e5d986..38041440d384 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -2,11 +2,13 @@ #include "envoy/api/v2/cds.pb.h" #include "envoy/api/v2/cds.pb.validate.h" +#include "envoy/api/v3alpha/cds.pb.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.h" #include "envoy/config/bootstrap/v2/bootstrap.pb.validate.h" #include "envoy/type/percent.pb.h" #include "common/common/base64.h" +#include "common/config/api_version.h" #include "common/protobuf/message_validator_impl.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" @@ -453,6 +455,95 @@ TEST_F(ProtobufUtilityTest, UnpackToWrongType) { R"(Unable to unpack as google.protobuf.Timestamp: \[type.googleapis.com/google.protobuf.Duration\] .*)"); } +// MessageUtility::unpackTo() with API message works at same version. +TEST_F(ProtobufUtilityTest, UnpackToSameVersion) { + { + API_NO_BOOST(envoy::api::v2::Cluster) source; + source.set_drain_connections_on_host_removal(true); + ProtobufWkt::Any source_any; + source_any.PackFrom(source); + API_NO_BOOST(envoy::api::v2::Cluster) dst; + MessageUtil::unpackTo(source_any, dst); + EXPECT_TRUE(dst.drain_connections_on_host_removal()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) source; + source.set_ignore_health_on_host_removal(true); + ProtobufWkt::Any source_any; + source_any.PackFrom(source); + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::unpackTo(source_any, dst); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); + } +} + +// MessageUtility::unpackTo() with API message works across version. +TEST_F(ProtobufUtilityTest, UnpackToNextVersion) { + API_NO_BOOST(envoy::api::v2::Cluster) source; + source.set_drain_connections_on_host_removal(true); + ProtobufWkt::Any source_any; + source_any.PackFrom(source); + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::unpackTo(source_any, dst); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); +} + +// MessageUtility::loadFromJson() with API message works at same version. +TEST_F(ProtobufUtilityTest, LoadFromJsonSameVersion) { + { + API_NO_BOOST(envoy::api::v2::Cluster) dst; + MessageUtil::loadFromJson("{drain_connections_on_host_removal: true}", dst, + ProtobufMessage::getNullValidationVisitor()); + EXPECT_TRUE(dst.drain_connections_on_host_removal()); + } + { + API_NO_BOOST(envoy::api::v2::Cluster) dst; + MessageUtil::loadFromJson("{drain_connections_on_host_removal: true}", dst, + ProtobufMessage::getStrictValidationVisitor()); + EXPECT_TRUE(dst.drain_connections_on_host_removal()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{ignore_health_on_host_removal: true}", dst, + ProtobufMessage::getNullValidationVisitor()); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{ignore_health_on_host_removal: true}", dst, + ProtobufMessage::getStrictValidationVisitor()); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); + } +} + +// MessageUtility::loadFromJson() with API message works across version. +TEST_F(ProtobufUtilityTest, LoadFromJsonNextVersion) { + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{use_tcp_for_dns_lookups: true}", dst, + ProtobufMessage::getNullValidationVisitor()); + EXPECT_TRUE(dst.use_tcp_for_dns_lookups()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{use_tcp_for_dns_lookups: true}", dst, + ProtobufMessage::getStrictValidationVisitor()); + EXPECT_TRUE(dst.use_tcp_for_dns_lookups()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{drain_connections_on_host_removal: true}", dst, + ProtobufMessage::getNullValidationVisitor()); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); + } + { + API_NO_BOOST(envoy::api::v3alpha::Cluster) dst; + MessageUtil::loadFromJson("{drain_connections_on_host_removal: true}", dst, + ProtobufMessage::getStrictValidationVisitor()); + EXPECT_TRUE(dst.ignore_health_on_host_removal()); + } +} + TEST_F(ProtobufUtilityTest, JsonConvertSuccess) { envoy::config::bootstrap::v2::Bootstrap source; source.set_flags_path("foo"); From 78b41828946b4bb5cd8cbeab762f5b3d3fa9a9ec Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Fri, 27 Dec 2019 15:46:15 -0500 Subject: [PATCH 03/10] Typos. Signed-off-by: Harvey Tuch --- source/common/config/version_converter.h | 8 ++++---- source/common/protobuf/utility.cc | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index e837a1cbc191..b9bb5a15b6e8 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -8,7 +8,7 @@ namespace Envoy { namespace Config { -// A message that has been downgraded, e.g. from v3alphato v2. +// A message that has been downgraded, e.g. from v3alpha to v2. struct DowngradedMessage { // The dynamic message factory must outlive the message. Protobuf::DynamicMessageFactory dmf_; @@ -23,12 +23,12 @@ class VersionConverter { public: /** * Upgrade a message from an earlier to later version of the Envoy API. This - * performs a simple wire-level reinterpreation of the fields. As a result of - * shadow protos, earlier deprecated fields such as foo are rematerialized as + * performs a simple wire-level reinterpretation of the fields. As a result of + * shadow protos, earlier deprecated fields such as foo are materialized as * hidden_envoy_deprecated_foo. * * @param prev_message previous version message input. - * @param next_nmessage next version message to generate. + * @param next_message next version message to generate. */ static void upgrade(const Protobuf::Message& prev_message, Protobuf::Message& next_message); diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 2ef10b075008..242abbc35f2a 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -115,7 +115,7 @@ void jsonConvertInternal(const Protobuf::Message& source, // First we try with the message's earlier type, and if unsuccessful (or no // earlier) type, then the current type. This allows us to take a v3 Envoy // internal proto and ingest both v2 and v3 in methods such as loadFromJson. -// This relies on the property that any v3 configuration that is ingestible as +// This relies on the property that any v3 configuration that is readable as // v2 has the same semantics in v2/v3, which holds due to the highly structured // vN/v(N+1) mechanical transforms. void tryWithApiBoosting(std::function f, @@ -145,7 +145,7 @@ void tryWithApiBoosting(std::function f, // If we fail this time, we should throw. // TODO(htuch): currently throwing the v2 error, rather than v3 error, to // make it easier to handle existing v2 tests during API boosting. We - // probably want to provide a more details exception message contaiing + // probably want to provide a more details exception message containing // both v2 and v3 exception info. newer_error = true; } From 3b05b15e790ff0fd901dc662085cc4e4eea095f4 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 30 Dec 2019 10:44:23 -0500 Subject: [PATCH 04/10] fix_format Signed-off-by: Harvey Tuch --- generated_api_shadow/envoy/admin/v2alpha/clusters.proto | 2 +- generated_api_shadow/envoy/admin/v3alpha/clusters.proto | 2 +- generated_api_shadow/envoy/api/v2/cds.proto | 2 +- generated_api_shadow/envoy/api/v2/core/config_source.proto | 3 ++- generated_api_shadow/envoy/api/v3alpha/cds.proto | 2 +- .../envoy/api/v3alpha/core/config_source.proto | 3 ++- 6 files changed, 8 insertions(+), 6 deletions(-) diff --git a/generated_api_shadow/envoy/admin/v2alpha/clusters.proto b/generated_api_shadow/envoy/admin/v2alpha/clusters.proto index dee414b48fc4..af3e9aa21087 100644 --- a/generated_api_shadow/envoy/admin/v2alpha/clusters.proto +++ b/generated_api_shadow/envoy/admin/v2alpha/clusters.proto @@ -141,6 +141,6 @@ message HostHealthStatus { // Health status as reported by EDS. Note: only HEALTHY and UNHEALTHY are currently supported // here. - // TODO(mrice32): pipe through remaining EDS health status possibilities. + // [#comment:TODO(mrice32): pipe through remaining EDS health status possibilities.] api.v2.core.HealthStatus eds_health_status = 3; } diff --git a/generated_api_shadow/envoy/admin/v3alpha/clusters.proto b/generated_api_shadow/envoy/admin/v3alpha/clusters.proto index 2b060a9fc26e..6829d5e24dfe 100644 --- a/generated_api_shadow/envoy/admin/v3alpha/clusters.proto +++ b/generated_api_shadow/envoy/admin/v3alpha/clusters.proto @@ -152,6 +152,6 @@ message HostHealthStatus { // Health status as reported by EDS. Note: only HEALTHY and UNHEALTHY are currently supported // here. - // TODO(mrice32): pipe through remaining EDS health status possibilities. + // [#comment:TODO(mrice32): pipe through remaining EDS health status possibilities.] api.v3alpha.core.HealthStatus eds_health_status = 3; } diff --git a/generated_api_shadow/envoy/api/v2/cds.proto b/generated_api_shadow/envoy/api/v2/cds.proto index d174d929dc49..298847e57d53 100644 --- a/generated_api_shadow/envoy/api/v2/cds.proto +++ b/generated_api_shadow/envoy/api/v2/cds.proto @@ -517,7 +517,7 @@ message Cluster { // *TransportSocketMatch* in this field. Other client Envoys receive CDS without // *transport_socket_match* set, and still send plain text traffic to the same cluster. // - // TODO(incfly): add a detailed architecture doc on intended usage. + // [#comment:TODO(incfly): add a detailed architecture doc on intended usage.] repeated TransportSocketMatch transport_socket_matches = 43; // Supplies the name of the cluster which must be unique across all clusters. diff --git a/generated_api_shadow/envoy/api/v2/core/config_source.proto b/generated_api_shadow/envoy/api/v2/core/config_source.proto index 00dae5bdf8f8..042a84789167 100644 --- a/generated_api_shadow/envoy/api/v2/core/config_source.proto +++ b/generated_api_shadow/envoy/api/v2/core/config_source.proto @@ -38,7 +38,8 @@ message ApiConfigSource { // with every update, the xDS server only sends what has changed since the last update. // // DELTA_GRPC is not yet entirely implemented! Initially, only CDS is available. - // Do not use for other xDSes. TODO(fredlas) update/remove this warning when appropriate. + // Do not use for other xDSes. + // [#comment:TODO(fredlas) update/remove this warning when appropriate.] DELTA_GRPC = 3; } diff --git a/generated_api_shadow/envoy/api/v3alpha/cds.proto b/generated_api_shadow/envoy/api/v3alpha/cds.proto index 798eddab27fb..f7e070452395 100644 --- a/generated_api_shadow/envoy/api/v3alpha/cds.proto +++ b/generated_api_shadow/envoy/api/v3alpha/cds.proto @@ -557,7 +557,7 @@ message Cluster { // *TransportSocketMatch* in this field. Other client Envoys receive CDS without // *transport_socket_match* set, and still send plain text traffic to the same cluster. // - // TODO(incfly): add a detailed architecture doc on intended usage. + // [#comment:TODO(incfly): add a detailed architecture doc on intended usage.] repeated TransportSocketMatch transport_socket_matches = 43; // Supplies the name of the cluster which must be unique across all clusters. diff --git a/generated_api_shadow/envoy/api/v3alpha/core/config_source.proto b/generated_api_shadow/envoy/api/v3alpha/core/config_source.proto index fe98e177c999..a14b34d2c80a 100644 --- a/generated_api_shadow/envoy/api/v3alpha/core/config_source.proto +++ b/generated_api_shadow/envoy/api/v3alpha/core/config_source.proto @@ -42,7 +42,8 @@ message ApiConfigSource { // with every update, the xDS server only sends what has changed since the last update. // // DELTA_GRPC is not yet entirely implemented! Initially, only CDS is available. - // Do not use for other xDSes. TODO(fredlas) update/remove this warning when appropriate. + // Do not use for other xDSes. + // [#comment:TODO(fredlas) update/remove this warning when appropriate.] DELTA_GRPC = 3; } From 5cf869386255de44bb2c0b1e0b29fe0d4fd1cc33 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 30 Dec 2019 13:35:44 -0500 Subject: [PATCH 05/10] Fix logging bug (was this causing coverage to fail?). Signed-off-by: Harvey Tuch --- source/common/config/api_type_oracle.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index b184c8320271..09c487fa9b0a 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -11,7 +11,7 @@ namespace Config { const Protobuf::Descriptor* ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) { const std::string target_type = message.GetDescriptor()->full_name(); - ENVOY_LOG_MISC(trace, "Inferring earlier type for {} (extension {})", target_type); + ENVOY_LOG_MISC(trace, "Inferring earlier type for {}", target_type); // Determine if there is an earlier API version for target_type. const Protobuf::Descriptor* desc = From 4270fbbf38274ee97d4d0f4cfe9402d3af58ce6c Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 30 Dec 2019 16:25:19 -0500 Subject: [PATCH 06/10] wip Signed-off-by: Harvey Tuch --- test/run_envoy_bazel_coverage.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/test/run_envoy_bazel_coverage.sh b/test/run_envoy_bazel_coverage.sh index 4c4af655bbf0..c96de551216d 100755 --- a/test/run_envoy_bazel_coverage.sh +++ b/test/run_envoy_bazel_coverage.sh @@ -29,6 +29,7 @@ BAZEL_USE_LLVM_NATIVE_COVERAGE=1 GCOV=llvm-profdata bazel coverage ${BAZEL_BUILD -c fastbuild --copt=-DNDEBUG --instrumentation_filter=//source/...,//include/... \ --test_timeout=2000 --cxxopt="-DENVOY_CONFIG_COVERAGE=1" --test_output=errors \ --test_arg="--log-path /dev/null" --test_arg="-l trace" --test_env=HEAPCHECK= \ + --show_timestamps \ //test/coverage:coverage_tests COVERAGE_DIR="${SRCDIR}"/generated/coverage From 253157a1e9c4fe31818224e7ba4e668619df098c Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Tue, 31 Dec 2019 17:26:03 -0500 Subject: [PATCH 07/10] Review feedback. Signed-off-by: Harvey Tuch --- source/common/config/version_converter.cc | 31 +++++++++++++++------- source/common/config/version_converter.h | 2 +- source/common/protobuf/utility.cc | 24 +++++++++++------ test/common/config/api_type_oracle_test.cc | 4 +-- 4 files changed, 41 insertions(+), 20 deletions(-) diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index b810720dd15a..0a85b618dc8b 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -5,26 +5,39 @@ namespace Envoy { namespace Config { +namespace { + +// Reinterpret a Protobuf message as another Protobuf message by converting to +// wire format and back. This only works for messages that can be effectively +// duck typed this way, e.g. with a subtype relationship modulo field name. +void wireCast(const Protobuf::Message& src, Protobuf::Message& dst) { + std::string s; + src.SerializeToString(&s); + dst.ParseFromString(s); +} + +} // namespace + void VersionConverter::upgrade(const Protobuf::Message& prev_message, Protobuf::Message& next_message) { - std::string s; - prev_message.SerializeToString(&s); - next_message.ParseFromString(s); + wireCast(prev_message, next_message); } DowngradedMessagePtr VersionConverter::downgrade(const Protobuf::Message& message) { auto downgraded_message = std::make_unique(); const Protobuf::Descriptor* prev_desc = ApiTypeOracle::getEarlierVersionDescriptor(message); if (prev_desc != nullptr) { - downgraded_message->msg_.reset(downgraded_message->dmf_.GetPrototype(prev_desc)->New()); - std::string s; - message.SerializeToString(&s); - downgraded_message->msg_->ParseFromString(s); + downgraded_message->msg_.reset( + downgraded_message->dynamic_msg_factory_.GetPrototype(prev_desc)->New()); + wireCast(message, *downgraded_message->msg_); return downgraded_message; } - // Unnecessary copy.. + // Unnecessary copy, since the existing message is being treated as + // "downgraded". However, we want to transfer an owned object, so this is the + // best we can do. const Protobuf::Descriptor* desc = message.GetDescriptor(); - downgraded_message->msg_.reset(downgraded_message->dmf_.GetPrototype(desc)->New()); + downgraded_message->msg_.reset( + downgraded_message->dynamic_msg_factory_.GetPrototype(desc)->New()); downgraded_message->msg_->MergeFrom(message); return downgraded_message; } diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index b9bb5a15b6e8..d39b7312c80a 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -11,7 +11,7 @@ namespace Config { // A message that has been downgraded, e.g. from v3alpha to v2. struct DowngradedMessage { // The dynamic message factory must outlive the message. - Protobuf::DynamicMessageFactory dmf_; + Protobuf::DynamicMessageFactory dynamic_msg_factory_; // Downgraded message. std::unique_ptr msg_; diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index 242abbc35f2a..aaea3035f919 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -111,6 +111,15 @@ void jsonConvertInternal(const Protobuf::Message& source, MessageUtil::loadFromJson(json, dest, validation_visitor); } +enum class MessageVersion { + // No known version of the message type exists at an earlier version. + EARLIEST_KNOWN, + // An earlier version of the message type exists. + NOT_EARLIEST, +}; + +using MessageXformFn = std::function; + // Apply a function transforming a message (e.g. loading JSON into the message). // First we try with the message's earlier type, and if unsuccessful (or no // earlier) type, then the current type. This allows us to take a v3 Envoy @@ -118,13 +127,12 @@ void jsonConvertInternal(const Protobuf::Message& source, // This relies on the property that any v3 configuration that is readable as // v2 has the same semantics in v2/v3, which holds due to the highly structured // vN/v(N+1) mechanical transforms. -void tryWithApiBoosting(std::function f, - Protobuf::Message& message) { +void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { const Protobuf::Descriptor* earlier_version_desc = Config::ApiTypeOracle::getEarlierVersionDescriptor(message); // If there is no earlier version of a message, just apply f directly. if (earlier_version_desc == nullptr) { - f(message, true); + f(message, MessageVersion::EARLIEST_KNOWN); return; } Protobuf::DynamicMessageFactory dmf; @@ -133,14 +141,14 @@ void tryWithApiBoosting(std::function f, try { // Try apply f with an earlier version of the message, then upgrade the // result. - f(*earlier_message, false); + f(*earlier_message, MessageVersion::NOT_EARLIEST); Config::VersionConverter::upgrade(*earlier_message, message); } catch (EnvoyException&) { // If we fail at the earlier version, try f at the current version of the // message. bool newer_error = false; try { - f(message, true); + f(message, MessageVersion::EARLIEST_KNOWN); } catch (EnvoyException&) { // If we fail this time, we should throw. // TODO(htuch): currently throwing the v2 error, rather than v3 error, to @@ -221,7 +229,7 @@ size_t MessageUtil::hash(const Protobuf::Message& message) { void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& message, ProtobufMessage::ValidationVisitor& validation_visitor) { tryWithApiBoosting( - [&json, &validation_visitor](Protobuf::Message& message, bool latest_version) { + [&json, &validation_visitor](Protobuf::Message& message, MessageVersion message_version) { Protobuf::util::JsonParseOptions options; options.case_insensitive_enum_parsing = true; // Let's first try and get a clean parse when checking for unknown fields; @@ -248,7 +256,7 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa // We know it's an unknown field at this point. If we're at the latest // version, then it's definitely an unknown field, otherwise we try to // load again at a later version. - if (latest_version) { + if (message_version == MessageVersion::EARLIEST_KNOWN) { validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + strict_status.ToString()); } else { @@ -488,7 +496,7 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) { tryWithApiBoosting( - [&any_message](Protobuf::Message& message, bool) { + [&any_message](Protobuf::Message& message, MessageVersion) { if (!any_message.UnpackTo(&message)) { throw EnvoyException(fmt::format("Unable to unpack as {}: {}", message.GetDescriptor()->full_name(), diff --git a/test/common/config/api_type_oracle_test.cc b/test/common/config/api_type_oracle_test.cc index bb8049624850..c264019a725c 100644 --- a/test/common/config/api_type_oracle_test.cc +++ b/test/common/config/api_type_oracle_test.cc @@ -19,8 +19,8 @@ TEST(ApiTypeOracleTest, All) { EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(non_api_type)); EXPECT_EQ(nullptr, ApiTypeOracle::getEarlierVersionDescriptor(v2_config)); const auto* desc = ApiTypeOracle::getEarlierVersionDescriptor(v3_config); - EXPECT_EQ(envoy::config::filter::http::ip_tagging::v3alpha::IPTagging::descriptor()->name(), - desc->name()); + EXPECT_EQ(envoy::config::filter::http::ip_tagging::v2::IPTagging::descriptor()->full_name(), + desc->full_name()); } } // namespace From bef539a8c50b6ea4b777cdfed5ebcecf01ff06aa Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 1 Jan 2020 09:47:22 -0500 Subject: [PATCH 08/10] Now with less confusing enum values. Signed-off-by: Harvey Tuch --- source/common/protobuf/utility.cc | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index aaea3035f919..3364c18eea98 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -112,10 +112,10 @@ void jsonConvertInternal(const Protobuf::Message& source, } enum class MessageVersion { - // No known version of the message type exists at an earlier version. - EARLIEST_KNOWN, - // An earlier version of the message type exists. - NOT_EARLIEST, + // This is an earlier version of a message, a later one exists. + EARLIER_VERSION, + // This is the latest version of a message. + LATEST_VERSION, }; using MessageXformFn = std::function; @@ -132,7 +132,7 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { Config::ApiTypeOracle::getEarlierVersionDescriptor(message); // If there is no earlier version of a message, just apply f directly. if (earlier_version_desc == nullptr) { - f(message, MessageVersion::EARLIEST_KNOWN); + f(message, MessageVersion::LATEST_VERSION); return; } Protobuf::DynamicMessageFactory dmf; @@ -141,14 +141,14 @@ void tryWithApiBoosting(MessageXformFn f, Protobuf::Message& message) { try { // Try apply f with an earlier version of the message, then upgrade the // result. - f(*earlier_message, MessageVersion::NOT_EARLIEST); + f(*earlier_message, MessageVersion::EARLIER_VERSION); Config::VersionConverter::upgrade(*earlier_message, message); } catch (EnvoyException&) { // If we fail at the earlier version, try f at the current version of the // message. bool newer_error = false; try { - f(message, MessageVersion::EARLIEST_KNOWN); + f(message, MessageVersion::LATEST_VERSION); } catch (EnvoyException&) { // If we fail this time, we should throw. // TODO(htuch): currently throwing the v2 error, rather than v3 error, to @@ -256,7 +256,7 @@ void MessageUtil::loadFromJson(const std::string& json, Protobuf::Message& messa // We know it's an unknown field at this point. If we're at the latest // version, then it's definitely an unknown field, otherwise we try to // load again at a later version. - if (message_version == MessageVersion::EARLIEST_KNOWN) { + if (message_version == MessageVersion::LATEST_VERSION) { validation_visitor.onUnknownField("type " + message.GetTypeName() + " reason " + strict_status.ToString()); } else { From cef6b0483806af25837122fac05b18faac5722f9 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 1 Jan 2020 22:04:55 -0500 Subject: [PATCH 09/10] Remove trace logs interfering with coverage. Signed-off-by: Harvey Tuch --- source/common/config/api_type_oracle.cc | 4 ---- test/run_envoy_bazel_coverage.sh | 1 - 2 files changed, 5 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 09c487fa9b0a..4b1bd680fca4 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -11,13 +11,11 @@ namespace Config { const Protobuf::Descriptor* ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) { const std::string target_type = message.GetDescriptor()->full_name(); - ENVOY_LOG_MISC(trace, "Inferring earlier type for {}", target_type); // Determine if there is an earlier API version for target_type. const Protobuf::Descriptor* desc = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(std::string{target_type}); if (desc == nullptr) { - ENVOY_LOG_MISC(trace, "No descriptor found for {}", target_type); return nullptr; } if (desc->options().HasExtension(udpa::annotations::versioning)) { @@ -26,11 +24,9 @@ ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) { const Protobuf::Descriptor* desc = Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(previous_target_type); ASSERT(desc != nullptr); - ENVOY_LOG_MISC(trace, "Inferred {}", desc->full_name()); return desc; } - ENVOY_LOG_MISC(trace, "No earlier descriptor found for {}", target_type); return nullptr; } diff --git a/test/run_envoy_bazel_coverage.sh b/test/run_envoy_bazel_coverage.sh index c96de551216d..4c4af655bbf0 100755 --- a/test/run_envoy_bazel_coverage.sh +++ b/test/run_envoy_bazel_coverage.sh @@ -29,7 +29,6 @@ BAZEL_USE_LLVM_NATIVE_COVERAGE=1 GCOV=llvm-profdata bazel coverage ${BAZEL_BUILD -c fastbuild --copt=-DNDEBUG --instrumentation_filter=//source/...,//include/... \ --test_timeout=2000 --cxxopt="-DENVOY_CONFIG_COVERAGE=1" --test_output=errors \ --test_arg="--log-path /dev/null" --test_arg="-l trace" --test_env=HEAPCHECK= \ - --show_timestamps \ //test/coverage:coverage_tests COVERAGE_DIR="${SRCDIR}"/generated/coverage From 0c56fc073179e6f4577e4ba967080fdde6e0e713 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 1 Jan 2020 22:12:03 -0500 Subject: [PATCH 10/10] Review feedback. Signed-off-by: Harvey Tuch --- source/common/config/api_type_oracle.cc | 11 +++++------ source/common/config/version_converter.cc | 4 +--- 2 files changed, 6 insertions(+), 9 deletions(-) diff --git a/source/common/config/api_type_oracle.cc b/source/common/config/api_type_oracle.cc index 4b1bd680fca4..0360e63fe9ce 100644 --- a/source/common/config/api_type_oracle.cc +++ b/source/common/config/api_type_oracle.cc @@ -19,12 +19,11 @@ ApiTypeOracle::getEarlierVersionDescriptor(const Protobuf::Message& message) { return nullptr; } if (desc->options().HasExtension(udpa::annotations::versioning)) { - const std::string previous_target_type = - desc->options().GetExtension(udpa::annotations::versioning).previous_message_type(); - const Protobuf::Descriptor* desc = - Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(previous_target_type); - ASSERT(desc != nullptr); - return desc; + const Protobuf::Descriptor* earlier_desc = + Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( + desc->options().GetExtension(udpa::annotations::versioning).previous_message_type()); + ASSERT(earlier_desc != nullptr); + return earlier_desc; } return nullptr; diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index 0a85b618dc8b..9dd563588bed 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -11,9 +11,7 @@ namespace { // wire format and back. This only works for messages that can be effectively // duck typed this way, e.g. with a subtype relationship modulo field name. void wireCast(const Protobuf::Message& src, Protobuf::Message& dst) { - std::string s; - src.SerializeToString(&s); - dst.ParseFromString(s); + dst.ParseFromString(src.SerializeAsString()); } } // namespace