From 0da7f832db986608ffb994a5500a1fba59896dc7 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 9 Jan 2020 08:48:15 -0500 Subject: [PATCH 01/10] config: support recovery of original message when upgrading. To better support config dump, deprecated field detection and other debug, it's helpful to leave a type name breadcrumb and be able to synthesize a Protobuf::Message that corresponds to what was delivered on the wire. TODO: plumb this to config dump, add some tests. Risk level: Low Testing: new version converter unit test. Fixes #9612 Signed-off-by: Harvey Tuch --- api/envoy/admin/v2alpha/BUILD | 2 - api/envoy/admin/v2alpha/config_dump.proto | 25 +- api/envoy/admin/v3alpha/BUILD | 4 - api/envoy/admin/v3alpha/config_dump.proto | 25 +- .../envoy/admin/v2alpha/BUILD | 2 - .../envoy/admin/v2alpha/config_dump.proto | 25 +- .../envoy/admin/v3alpha/BUILD | 4 - .../envoy/admin/v3alpha/config_dump.proto | 25 +- source/common/config/BUILD | 1 + source/common/config/version_converter.cc | 226 ++++++++++++------ source/common/config/version_converter.h | 25 +- source/common/local_info/BUILD | 1 + source/common/local_info/local_info_impl.h | 8 + source/common/protobuf/BUILD | 6 + source/common/protobuf/utility.cc | 26 +- source/common/protobuf/utility.h | 3 +- source/common/protobuf/well_known.h | 10 + source/common/router/BUILD | 2 + source/common/router/rds_impl.cc | 8 +- source/common/router/scoped_rds.cc | 7 +- source/common/secret/BUILD | 1 + source/common/secret/secret_manager_impl.cc | 59 +++-- source/common/upstream/BUILD | 3 + .../common/upstream/cluster_manager_impl.cc | 7 +- source/common/upstream/eds.cc | 4 + source/server/BUILD | 1 + source/server/http/admin.cc | 79 +++++- source/server/listener_manager_impl.cc | 7 +- test/common/config/BUILD | 2 + test/common/config/version_converter_test.cc | 77 ++++++ test/common/router/rds_impl_test.cc | 5 +- test/common/router/scoped_rds_test.cc | 17 +- .../common/secret/secret_manager_impl_test.cc | 17 ++ .../upstream/cluster_manager_impl_test.cc | 12 +- test/common/upstream/utility.h | 6 +- test/integration/BUILD | 1 + .../api_version_integration_test.cc | 18 +- test/integration/integration_admin_test.cc | 6 +- test/server/http/admin_test.cc | 23 +- test/server/listener_manager_impl_test.cc | 10 +- test/server/utility.h | 2 +- test/test_common/BUILD | 1 + test/test_common/utility.h | 20 +- 43 files changed, 589 insertions(+), 224 deletions(-) create mode 100644 source/common/protobuf/well_known.h diff --git a/api/envoy/admin/v2alpha/BUILD b/api/envoy/admin/v2alpha/BUILD index 38a121362b736..3ced653bc3282 100644 --- a/api/envoy/admin/v2alpha/BUILD +++ b/api/envoy/admin/v2alpha/BUILD @@ -6,8 +6,6 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ - "//envoy/api/v2:pkg", - "//envoy/api/v2/auth:pkg", "//envoy/api/v2/core:pkg", "//envoy/config/bootstrap/v2:pkg", "//envoy/service/tap/v2alpha:pkg", diff --git a/api/envoy/admin/v2alpha/config_dump.proto b/api/envoy/admin/v2alpha/config_dump.proto index d1e7d4afe60b5..5e3a14592ba22 100644 --- a/api/envoy/admin/v2alpha/config_dump.proto +++ b/api/envoy/admin/v2alpha/config_dump.proto @@ -2,11 +2,6 @@ syntax = "proto3"; package envoy.admin.v2alpha; -import "envoy/api/v2/auth/cert.proto"; -import "envoy/api/v2/cluster.proto"; -import "envoy/api/v2/listener.proto"; -import "envoy/api/v2/route.proto"; -import "envoy/api/v2/scoped_route.proto"; import "envoy/config/bootstrap/v2/bootstrap.proto"; import "google/protobuf/any.proto"; @@ -69,7 +64,7 @@ message ListenersConfigDump { // Describes a statically loaded listener. message StaticListener { // The listener config. - api.v2.Listener listener = 1; + google.protobuf.Any listener = 1; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 2; @@ -83,7 +78,7 @@ message ListenersConfigDump { string version_info = 1; // The listener config. - api.v2.Listener listener = 2; + google.protobuf.Any listener = 2; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 3; @@ -134,7 +129,7 @@ message ClustersConfigDump { // Describes a statically loaded cluster. message StaticCluster { // The cluster config. - api.v2.Cluster cluster = 1; + google.protobuf.Any cluster = 1; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 2; @@ -149,7 +144,7 @@ message ClustersConfigDump { string version_info = 1; // The cluster config. - api.v2.Cluster cluster = 2; + google.protobuf.Any cluster = 2; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 3; @@ -182,7 +177,7 @@ message ClustersConfigDump { message RoutesConfigDump { message StaticRouteConfig { // The route config. - api.v2.RouteConfiguration route_config = 1; + google.protobuf.Any route_config = 1; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 2; @@ -195,7 +190,7 @@ message RoutesConfigDump { string version_info = 1; // The route config. - api.v2.RouteConfiguration route_config = 2; + google.protobuf.Any route_config = 2; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 3; @@ -218,7 +213,7 @@ message ScopedRoutesConfigDump { string name = 1; // The scoped route configurations. - repeated api.v2.ScopedRouteConfiguration scoped_route_configs = 2; + repeated google.protobuf.Any scoped_route_configs = 2; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 3; @@ -234,7 +229,7 @@ message ScopedRoutesConfigDump { string version_info = 2; // The scoped route configurations. - repeated api.v2.ScopedRouteConfiguration scoped_route_configs = 3; + repeated google.protobuf.Any scoped_route_configs = 3; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 4; @@ -263,7 +258,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - api.v2.auth.Secret secret = 4; + google.protobuf.Any secret = 4; } // StaticSecret specifies statically loaded secret in bootstrap. @@ -277,7 +272,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - api.v2.auth.Secret secret = 3; + google.protobuf.Any secret = 3; } // The statically loaded secrets. diff --git a/api/envoy/admin/v3alpha/BUILD b/api/envoy/admin/v3alpha/BUILD index 5de70fa099853..32469e852fd49 100644 --- a/api/envoy/admin/v3alpha/BUILD +++ b/api/envoy/admin/v3alpha/BUILD @@ -8,12 +8,8 @@ api_proto_package( deps = [ "//envoy/admin/v2alpha:pkg", "//envoy/config/bootstrap/v3alpha:pkg", - "//envoy/config/cluster/v3alpha:pkg", "//envoy/config/core/v3alpha:pkg", - "//envoy/config/listener/v3alpha:pkg", - "//envoy/config/route/v3alpha:pkg", "//envoy/config/tap/v3alpha:pkg", - "//envoy/extensions/transport_sockets/tls/v3alpha:pkg", "//envoy/type/v3alpha:pkg", "@com_github_cncf_udpa//udpa/annotations:pkg", ], diff --git a/api/envoy/admin/v3alpha/config_dump.proto b/api/envoy/admin/v3alpha/config_dump.proto index af801e8d8c750..fc4c7960db7fd 100644 --- a/api/envoy/admin/v3alpha/config_dump.proto +++ b/api/envoy/admin/v3alpha/config_dump.proto @@ -3,11 +3,6 @@ syntax = "proto3"; package envoy.admin.v3alpha; import "envoy/config/bootstrap/v3alpha/bootstrap.proto"; -import "envoy/config/cluster/v3alpha/cluster.proto"; -import "envoy/config/listener/v3alpha/listener.proto"; -import "envoy/config/route/v3alpha/route.proto"; -import "envoy/config/route/v3alpha/scoped_route.proto"; -import "envoy/extensions/transport_sockets/tls/v3alpha/cert.proto"; import "google/protobuf/any.proto"; import "google/protobuf/timestamp.proto"; @@ -85,7 +80,7 @@ message ListenersConfigDump { "envoy.admin.v2alpha.ListenersConfigDump.StaticListener"; // The listener config. - config.listener.v3alpha.Listener listener = 1; + google.protobuf.Any listener = 1; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 2; @@ -103,7 +98,7 @@ message ListenersConfigDump { string version_info = 1; // The listener config. - config.listener.v3alpha.Listener listener = 2; + google.protobuf.Any listener = 2; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 3; @@ -164,7 +159,7 @@ message ClustersConfigDump { "envoy.admin.v2alpha.ClustersConfigDump.StaticCluster"; // The cluster config. - config.cluster.v3alpha.Cluster cluster = 1; + google.protobuf.Any cluster = 1; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 2; @@ -183,7 +178,7 @@ message ClustersConfigDump { string version_info = 1; // The cluster config. - config.cluster.v3alpha.Cluster cluster = 2; + google.protobuf.Any cluster = 2; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 3; @@ -223,7 +218,7 @@ message RoutesConfigDump { "envoy.admin.v2alpha.RoutesConfigDump.StaticRouteConfig"; // The route config. - config.route.v3alpha.RouteConfiguration route_config = 1; + google.protobuf.Any route_config = 1; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 2; @@ -240,7 +235,7 @@ message RoutesConfigDump { string version_info = 1; // The route config. - config.route.v3alpha.RouteConfiguration route_config = 2; + google.protobuf.Any route_config = 2; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 3; @@ -269,7 +264,7 @@ message ScopedRoutesConfigDump { string name = 1; // The scoped route configurations. - repeated config.route.v3alpha.ScopedRouteConfiguration scoped_route_configs = 2; + repeated google.protobuf.Any scoped_route_configs = 2; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 3; @@ -289,7 +284,7 @@ message ScopedRoutesConfigDump { string version_info = 2; // The scoped route configurations. - repeated config.route.v3alpha.ScopedRouteConfiguration scoped_route_configs = 3; + repeated google.protobuf.Any scoped_route_configs = 3; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 4; @@ -324,7 +319,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - envoy.extensions.transport_sockets.tls.v3alpha.Secret secret = 4; + google.protobuf.Any secret = 4; } // StaticSecret specifies statically loaded secret in bootstrap. @@ -341,7 +336,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - envoy.extensions.transport_sockets.tls.v3alpha.Secret secret = 3; + google.protobuf.Any secret = 3; } // The statically loaded secrets. diff --git a/generated_api_shadow/envoy/admin/v2alpha/BUILD b/generated_api_shadow/envoy/admin/v2alpha/BUILD index 38a121362b736..3ced653bc3282 100644 --- a/generated_api_shadow/envoy/admin/v2alpha/BUILD +++ b/generated_api_shadow/envoy/admin/v2alpha/BUILD @@ -6,8 +6,6 @@ licenses(["notice"]) # Apache 2 api_proto_package( deps = [ - "//envoy/api/v2:pkg", - "//envoy/api/v2/auth:pkg", "//envoy/api/v2/core:pkg", "//envoy/config/bootstrap/v2:pkg", "//envoy/service/tap/v2alpha:pkg", diff --git a/generated_api_shadow/envoy/admin/v2alpha/config_dump.proto b/generated_api_shadow/envoy/admin/v2alpha/config_dump.proto index d1e7d4afe60b5..5e3a14592ba22 100644 --- a/generated_api_shadow/envoy/admin/v2alpha/config_dump.proto +++ b/generated_api_shadow/envoy/admin/v2alpha/config_dump.proto @@ -2,11 +2,6 @@ syntax = "proto3"; package envoy.admin.v2alpha; -import "envoy/api/v2/auth/cert.proto"; -import "envoy/api/v2/cluster.proto"; -import "envoy/api/v2/listener.proto"; -import "envoy/api/v2/route.proto"; -import "envoy/api/v2/scoped_route.proto"; import "envoy/config/bootstrap/v2/bootstrap.proto"; import "google/protobuf/any.proto"; @@ -69,7 +64,7 @@ message ListenersConfigDump { // Describes a statically loaded listener. message StaticListener { // The listener config. - api.v2.Listener listener = 1; + google.protobuf.Any listener = 1; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 2; @@ -83,7 +78,7 @@ message ListenersConfigDump { string version_info = 1; // The listener config. - api.v2.Listener listener = 2; + google.protobuf.Any listener = 2; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 3; @@ -134,7 +129,7 @@ message ClustersConfigDump { // Describes a statically loaded cluster. message StaticCluster { // The cluster config. - api.v2.Cluster cluster = 1; + google.protobuf.Any cluster = 1; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 2; @@ -149,7 +144,7 @@ message ClustersConfigDump { string version_info = 1; // The cluster config. - api.v2.Cluster cluster = 2; + google.protobuf.Any cluster = 2; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 3; @@ -182,7 +177,7 @@ message ClustersConfigDump { message RoutesConfigDump { message StaticRouteConfig { // The route config. - api.v2.RouteConfiguration route_config = 1; + google.protobuf.Any route_config = 1; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 2; @@ -195,7 +190,7 @@ message RoutesConfigDump { string version_info = 1; // The route config. - api.v2.RouteConfiguration route_config = 2; + google.protobuf.Any route_config = 2; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 3; @@ -218,7 +213,7 @@ message ScopedRoutesConfigDump { string name = 1; // The scoped route configurations. - repeated api.v2.ScopedRouteConfiguration scoped_route_configs = 2; + repeated google.protobuf.Any scoped_route_configs = 2; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 3; @@ -234,7 +229,7 @@ message ScopedRoutesConfigDump { string version_info = 2; // The scoped route configurations. - repeated api.v2.ScopedRouteConfiguration scoped_route_configs = 3; + repeated google.protobuf.Any scoped_route_configs = 3; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 4; @@ -263,7 +258,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - api.v2.auth.Secret secret = 4; + google.protobuf.Any secret = 4; } // StaticSecret specifies statically loaded secret in bootstrap. @@ -277,7 +272,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - api.v2.auth.Secret secret = 3; + google.protobuf.Any secret = 3; } // The statically loaded secrets. diff --git a/generated_api_shadow/envoy/admin/v3alpha/BUILD b/generated_api_shadow/envoy/admin/v3alpha/BUILD index 5de70fa099853..32469e852fd49 100644 --- a/generated_api_shadow/envoy/admin/v3alpha/BUILD +++ b/generated_api_shadow/envoy/admin/v3alpha/BUILD @@ -8,12 +8,8 @@ api_proto_package( deps = [ "//envoy/admin/v2alpha:pkg", "//envoy/config/bootstrap/v3alpha:pkg", - "//envoy/config/cluster/v3alpha:pkg", "//envoy/config/core/v3alpha:pkg", - "//envoy/config/listener/v3alpha:pkg", - "//envoy/config/route/v3alpha:pkg", "//envoy/config/tap/v3alpha:pkg", - "//envoy/extensions/transport_sockets/tls/v3alpha:pkg", "//envoy/type/v3alpha:pkg", "@com_github_cncf_udpa//udpa/annotations:pkg", ], diff --git a/generated_api_shadow/envoy/admin/v3alpha/config_dump.proto b/generated_api_shadow/envoy/admin/v3alpha/config_dump.proto index af801e8d8c750..fc4c7960db7fd 100644 --- a/generated_api_shadow/envoy/admin/v3alpha/config_dump.proto +++ b/generated_api_shadow/envoy/admin/v3alpha/config_dump.proto @@ -3,11 +3,6 @@ syntax = "proto3"; package envoy.admin.v3alpha; import "envoy/config/bootstrap/v3alpha/bootstrap.proto"; -import "envoy/config/cluster/v3alpha/cluster.proto"; -import "envoy/config/listener/v3alpha/listener.proto"; -import "envoy/config/route/v3alpha/route.proto"; -import "envoy/config/route/v3alpha/scoped_route.proto"; -import "envoy/extensions/transport_sockets/tls/v3alpha/cert.proto"; import "google/protobuf/any.proto"; import "google/protobuf/timestamp.proto"; @@ -85,7 +80,7 @@ message ListenersConfigDump { "envoy.admin.v2alpha.ListenersConfigDump.StaticListener"; // The listener config. - config.listener.v3alpha.Listener listener = 1; + google.protobuf.Any listener = 1; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 2; @@ -103,7 +98,7 @@ message ListenersConfigDump { string version_info = 1; // The listener config. - config.listener.v3alpha.Listener listener = 2; + google.protobuf.Any listener = 2; // The timestamp when the Listener was last successfully updated. google.protobuf.Timestamp last_updated = 3; @@ -164,7 +159,7 @@ message ClustersConfigDump { "envoy.admin.v2alpha.ClustersConfigDump.StaticCluster"; // The cluster config. - config.cluster.v3alpha.Cluster cluster = 1; + google.protobuf.Any cluster = 1; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 2; @@ -183,7 +178,7 @@ message ClustersConfigDump { string version_info = 1; // The cluster config. - config.cluster.v3alpha.Cluster cluster = 2; + google.protobuf.Any cluster = 2; // The timestamp when the Cluster was last updated. google.protobuf.Timestamp last_updated = 3; @@ -223,7 +218,7 @@ message RoutesConfigDump { "envoy.admin.v2alpha.RoutesConfigDump.StaticRouteConfig"; // The route config. - config.route.v3alpha.RouteConfiguration route_config = 1; + google.protobuf.Any route_config = 1; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 2; @@ -240,7 +235,7 @@ message RoutesConfigDump { string version_info = 1; // The route config. - config.route.v3alpha.RouteConfiguration route_config = 2; + google.protobuf.Any route_config = 2; // The timestamp when the Route was last updated. google.protobuf.Timestamp last_updated = 3; @@ -269,7 +264,7 @@ message ScopedRoutesConfigDump { string name = 1; // The scoped route configurations. - repeated config.route.v3alpha.ScopedRouteConfiguration scoped_route_configs = 2; + repeated google.protobuf.Any scoped_route_configs = 2; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 3; @@ -289,7 +284,7 @@ message ScopedRoutesConfigDump { string version_info = 2; // The scoped route configurations. - repeated config.route.v3alpha.ScopedRouteConfiguration scoped_route_configs = 3; + repeated google.protobuf.Any scoped_route_configs = 3; // The timestamp when the scoped route config set was last updated. google.protobuf.Timestamp last_updated = 4; @@ -324,7 +319,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - envoy.extensions.transport_sockets.tls.v3alpha.Secret secret = 4; + google.protobuf.Any secret = 4; } // StaticSecret specifies statically loaded secret in bootstrap. @@ -341,7 +336,7 @@ message SecretsConfigDump { // The actual secret information. // Security sensitive information is redacted (replaced with "[redacted]") for // private keys and passwords in TLS certificates. - envoy.extensions.transport_sockets.tls.v3alpha.Secret secret = 3; + google.protobuf.Any secret = 3; } // The statically loaded secrets. diff --git a/source/common/config/BUILD b/source/common/config/BUILD index e3252a61e936e..78e485ab58c3e 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -368,6 +368,7 @@ envoy_cc_library( deps = [ ":api_type_oracle_lib", "//source/common/protobuf", + "//source/common/protobuf:well_known_lib", "@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto", ], ) diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index e1ce90b067d50..ae06561ceb491 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -2,6 +2,7 @@ #include "common/common/assert.h" #include "common/config/api_type_oracle.h" +#include "common/protobuf/well_known.h" #include "absl/strings/match.h" @@ -12,50 +13,41 @@ namespace { const char DeprecatedFieldShadowPrefix[] = "hidden_envoy_deprecated_"; -// TODO(htuch): refactor these message visitor patterns into utility.cc and share with -// MessageUtil::checkForUnexpectedFields. -void messageFieldVisitor(std::function - field_fn, - Protobuf::Message& message) { - const Protobuf::Descriptor* descriptor = message.GetDescriptor(); - const Protobuf::Reflection* reflection = message.GetReflection(); - for (int i = 0; i < descriptor->field_count(); ++i) { - const Protobuf::FieldDescriptor* field = descriptor->field(i); - field_fn(message, *field, *reflection); - // If this is a message, recurse to scrub deprecated fields in the sub-message. - if (field->cpp_type() == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { - if (field->is_repeated()) { - const int size = reflection->FieldSize(message, field); - for (int j = 0; j < size; ++j) { - messageFieldVisitor(field_fn, *reflection->MutableRepeatedMessage(&message, field, j)); - } - } else { - messageFieldVisitor(field_fn, *reflection->MutableMessage(&message, field)); - } - } +class ProtoVisitor { +public: + virtual ~ProtoVisitor() = default; + + // Invoked when a field is visited, with the message, field descriptor and + // context. Returns a new context for use when traversing the sub-message in a + // field. + virtual const void* onField(Protobuf::Message&, const Protobuf::FieldDescriptor&, + const void* ctxt) { + return ctxt; } -} -void constMessageFieldVisitor( - std::function - field_fn, - const Protobuf::Message& message) { + // Invoked when a message is visited, with the message and a context. + virtual void onMessage(Protobuf::Message&, const void*){}; +}; + +// TODO(htuch): refactor these message visitor patterns into utility.cc and share with +// MessageUtil::checkForUnexpectedFields. +void traverseMutableMessage(ProtoVisitor& visitor, Protobuf::Message& message, const void* ctxt) { + visitor.onMessage(message, ctxt); const Protobuf::Descriptor* descriptor = message.GetDescriptor(); const Protobuf::Reflection* reflection = message.GetReflection(); for (int i = 0; i < descriptor->field_count(); ++i) { const Protobuf::FieldDescriptor* field = descriptor->field(i); - field_fn(message, *field, *reflection); + const void* field_ctxt = visitor.onField(message, *field, ctxt); // If this is a message, recurse to scrub deprecated fields in the sub-message. if (field->cpp_type() == Protobuf::FieldDescriptor::CPPTYPE_MESSAGE) { if (field->is_repeated()) { const int size = reflection->FieldSize(message, field); for (int j = 0; j < size; ++j) { - constMessageFieldVisitor(field_fn, reflection->GetRepeatedMessage(message, field, j)); + traverseMutableMessage(visitor, *reflection->MutableRepeatedMessage(&message, field, j), + field_ctxt); } - } else { - constMessageFieldVisitor(field_fn, reflection->GetMessage(message, field)); + } else if (reflection->HasField(message, field)) { + traverseMutableMessage(visitor, *reflection->MutableMessage(&message, field), field_ctxt); } } } @@ -65,7 +57,75 @@ void constMessageFieldVisitor( // 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) { - dst.ParseFromString(src.SerializeAsString()); + // This should always succeed, since we should be supplying compatible + // messages here, but provide a RELEASE_ASSERT as this is off critical path + // and we want to learn if it fails. + RELEASE_ASSERT(dst.ParseFromString(src.SerializeAsString()), + "Unable to deserialize during wireCast()"); +} + +// Create a new dynamic message based on some message wire cast to the target +// descriptor. If the descriptor is null, a copy is performed. +DynamicMessagePtr createForDescriptorWithCast(const Protobuf::Message& message, + const Protobuf::Descriptor* desc) { + auto dynamic_message = std::make_unique(); + if (desc != nullptr) { + dynamic_message->msg_.reset(dynamic_message->dynamic_msg_factory_.GetPrototype(desc)->New()); + wireCast(message, *dynamic_message->msg_); + return dynamic_message; + } + // Unnecessary copy, since the existing message is being treated as + // "dynamic". However, we want to transfer an owned object, so this is the + // best we can do. + dynamic_message->msg_.reset(message.New()); + dynamic_message->msg_->MergeFrom(message); + return dynamic_message; +} + +// This needs to be recursive, since sub-messages are consumed and stored +// internally, we later want to recover their original types. +void annotateWithOriginalType(const Protobuf::Descriptor& prev_descriptor, + Protobuf::Message& next_message) { + class TypeAnnotatingProtoVisitor : public ProtoVisitor { + public: + void onMessage(Protobuf::Message& message, const void* ctxt) override { + const Protobuf::Descriptor* descriptor = message.GetDescriptor(); + const Protobuf::Reflection* reflection = message.GetReflection(); + const Protobuf::Descriptor& prev_descriptor = *static_cast(ctxt); + // If they are the same type, there's no possibility of any different type + // further down, so we're done. + if (descriptor->full_name() == prev_descriptor.full_name()) { + return; + } + auto* unknown_field_set = reflection->MutableUnknownFields(&message); + unknown_field_set->AddLengthDelimited(ProtobufWellKnown::OriginalTypeFieldNumber, + prev_descriptor.full_name()); + } + + const void* onField(Protobuf::Message&, const Protobuf::FieldDescriptor& field, + const void* ctxt) override { + const Protobuf::Descriptor& prev_descriptor = *static_cast(ctxt); + // TODO(htuch): This is a terrible hack, there should be no per-resource + // business logic in this file. The reason this is required is that + // endpoints, when captured in configuration such as inlined hosts in + // Clusters for config dump purposes, can potentially contribute a + // significant amount to memory consumption. stats_integration_test + // complains as a result if we increase any memory due to type annotations. + // In theory, we should be able to just clean up these annotations in + // ClusterManagerImpl with type erasure, but protobuf doesn't free up memory + // as expected, we probably need some arena level trick to address this. + if (prev_descriptor.full_name() == "envoy.api.v2.Cluster" && + (field.name() == "hosts" || field.name() == "load_assignment")) { + // This will cause the sub-message visit to abort early. + return field.message_type(); + } + const Protobuf::FieldDescriptor* prev_field = + prev_descriptor.FindFieldByNumber(field.number()); + return prev_field->message_type(); + } + }; + TypeAnnotatingProtoVisitor proto_visitor; + traverseMutableMessage(proto_visitor, next_message, &prev_descriptor); } } // namespace @@ -73,25 +133,45 @@ void wireCast(const Protobuf::Message& src, Protobuf::Message& dst) { void VersionConverter::upgrade(const Protobuf::Message& prev_message, Protobuf::Message& next_message) { wireCast(prev_message, next_message); + // Track original type to support recoverOriginal(). + annotateWithOriginalType(*prev_message.GetDescriptor(), next_message); +} + +void VersionConverter::eraseOriginalTypeInformation(Protobuf::Message& message) { + class TypeErasingProtoVisitor : public ProtoVisitor { + public: + void onMessage(Protobuf::Message& message, const void*) override { + const Protobuf::Reflection* reflection = message.GetReflection(); + auto* unknown_field_set = reflection->MutableUnknownFields(&message); + unknown_field_set->DeleteByNumber(ProtobufWellKnown::OriginalTypeFieldNumber); + } + }; + TypeErasingProtoVisitor proto_visitor; + traverseMutableMessage(proto_visitor, message, nullptr); +} + +DynamicMessagePtr VersionConverter::recoverOriginal(const Protobuf::Message& upgraded_message) { + const Protobuf::Reflection* reflection = upgraded_message.GetReflection(); + const auto& unknown_field_set = reflection->GetUnknownFields(upgraded_message); + for (int i = 0; i < unknown_field_set.field_count(); ++i) { + const auto& unknown_field = unknown_field_set.field(i); + if (unknown_field.number() == ProtobufWellKnown::OriginalTypeFieldNumber) { + ASSERT(unknown_field.type() == Protobuf::UnknownField::TYPE_LENGTH_DELIMITED); + const std::string& original_type = unknown_field.length_delimited(); + const Protobuf::Descriptor* original_descriptor = + Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName(original_type); + auto result = createForDescriptorWithCast(upgraded_message, original_descriptor); + // We should clear out the OriginalTypeFieldNumber in the recovered message. + eraseOriginalTypeInformation(*result->msg_); + return result; + } + } + return createForDescriptorWithCast(upgraded_message, nullptr); } DynamicMessagePtr 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->dynamic_msg_factory_.GetPrototype(prev_desc)->New()); - wireCast(message, *downgraded_message->msg_); - return downgraded_message; - } - // 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->dynamic_msg_factory_.GetPrototype(desc)->New()); - downgraded_message->msg_->MergeFrom(message); - return downgraded_message; + return createForDescriptorWithCast(message, prev_desc); } DynamicMessagePtr @@ -99,20 +179,22 @@ VersionConverter::reinterpret(const Protobuf::Message& message, envoy::config::core::v3alpha::ApiVersion api_version) { switch (api_version) { case envoy::config::core::v3alpha::ApiVersion::AUTO: - case envoy::config::core::v3alpha::ApiVersion::V2: + case envoy::config::core::v3alpha::ApiVersion::V2: { // TODO(htuch): this works as long as there are no new fields in the v3+ // DiscoveryRequest. When they are added, we need to do a full v2 conversion // and also discard unknown fields. Tracked at // https://github.com/envoyproxy/envoy/issues/9619. - return downgrade(message); + auto dynamic_message = downgrade(message); + eraseOriginalTypeInformation(*dynamic_message->msg_); + return dynamic_message; + } case envoy::config::core::v3alpha::ApiVersion::V3ALPHA: { // We need to scrub the hidden fields. auto non_shadowed_message = std::make_unique(); - const Protobuf::Descriptor* desc = message.GetDescriptor(); - non_shadowed_message->msg_.reset( - non_shadowed_message->dynamic_msg_factory_.GetPrototype(desc)->New()); + non_shadowed_message->msg_.reset(message.New()); non_shadowed_message->msg_->MergeFrom(message); VersionUtil::scrubHiddenEnvoyDeprecated(*non_shadowed_message->msg_); + eraseOriginalTypeInformation(*non_shadowed_message->msg_); return non_shadowed_message; } default: @@ -120,32 +202,20 @@ VersionConverter::reinterpret(const Protobuf::Message& message, } } -bool VersionUtil::hasHiddenEnvoyDeprecated(const Protobuf::Message& message) { - bool has_hidden = false; - constMessageFieldVisitor( - [&has_hidden](const Protobuf::Message& message, const Protobuf::FieldDescriptor& field, - const Protobuf::Reflection& reflection) { - if (absl::StartsWith(field.name(), DeprecatedFieldShadowPrefix)) { - if (field.is_repeated()) { - has_hidden |= reflection.FieldSize(message, &field) > 0; - } else { - has_hidden |= reflection.HasField(message, &field); - } - } - }, - message); - return has_hidden; -} - void VersionUtil::scrubHiddenEnvoyDeprecated(Protobuf::Message& message) { - messageFieldVisitor( - [](Protobuf::Message& message, const Protobuf::FieldDescriptor& field, - const Protobuf::Reflection& reflection) { - if (absl::StartsWith(field.name(), DeprecatedFieldShadowPrefix)) { - reflection.ClearField(&message, &field); - } - }, - message); + class HiddenFieldScrubbingProtoVisitor : public ProtoVisitor { + public: + void* onField(Protobuf::Message& message, const Protobuf::FieldDescriptor& field, + const void*) override { + const Protobuf::Reflection* reflection = message.GetReflection(); + if (absl::StartsWith(field.name(), DeprecatedFieldShadowPrefix)) { + reflection->ClearField(&message, &field); + } + return nullptr; + } + }; + HiddenFieldScrubbingProtoVisitor proto_visitor; + traverseMutableMessage(proto_visitor, message, nullptr); } } // namespace Config diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index deb81819370c7..e73ec86151da1 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -5,7 +5,10 @@ #include "common/protobuf/protobuf.h" // Convenience macro for downgrading a message and obtaining a reference. -#define API_DOWNGRADE(msg) (*Config::VersionConverter::downgrade(msg)->msg_) +#define API_DOWNGRADE(msg) (*Envoy::Config::VersionConverter::downgrade(msg)->msg_) + +// Convenience macro for recovering original message and obtaining a reference. +#define API_RECOVER_ORIGINAL(msg) (*Envoy::Config::VersionConverter::recoverOriginal(msg)->msg_) namespace Envoy { namespace Config { @@ -16,7 +19,7 @@ struct DynamicMessage { Protobuf::DynamicMessageFactory dynamic_msg_factory_; // Dynamic message. - std::unique_ptr msg_; + ProtobufTypes::MessagePtr msg_; }; using DynamicMessagePtr = std::unique_ptr; @@ -73,12 +76,28 @@ class VersionConverter { */ static DynamicMessagePtr reinterpret(const Protobuf::Message& message, envoy::config::core::v3alpha::ApiVersion api_version); + + /** + * For a message that may have been upgraded, recover the original message. + * This is useful for config dump, debug output etc. + * + * @param upgraded_message upgraded message input. + * + * @return DynamicMessagePtr original message (as a dynamic message). + */ + static DynamicMessagePtr recoverOriginal(const Protobuf::Message& upgraded_message); + + /** + * Remove original type information, when it's not needed, e.g. in tests. + * + * @param message upgraded message to scrub. + */ + static void eraseOriginalTypeInformation(Protobuf::Message& message); }; class VersionUtil { public: // Some helpers for working with earlier message version deprecated fields. - static bool hasHiddenEnvoyDeprecated(const Protobuf::Message& message); static void scrubHiddenEnvoyDeprecated(Protobuf::Message& message); }; diff --git a/source/common/local_info/BUILD b/source/common/local_info/BUILD index 8b73c33a441af..ca0a48b8c134b 100644 --- a/source/common/local_info/BUILD +++ b/source/common/local_info/BUILD @@ -13,6 +13,7 @@ envoy_cc_library( hdrs = ["local_info_impl.h"], deps = [ "//include/envoy/local_info:local_info_interface", + "//source/common/config:version_converter_lib", "@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto", ], ) diff --git a/source/common/local_info/local_info_impl.h b/source/common/local_info/local_info_impl.h index 9305b7c33bcea..ad1e7225bfbf1 100644 --- a/source/common/local_info/local_info_impl.h +++ b/source/common/local_info/local_info_impl.h @@ -5,6 +5,8 @@ #include "envoy/config/core/v3alpha/base.pb.h" #include "envoy/local_info/local_info.h" +#include "common/config/version_converter.h" + namespace Envoy { namespace LocalInfo { @@ -15,6 +17,12 @@ class LocalInfoImpl : public LocalInfo { absl::string_view zone_name, absl::string_view cluster_name, absl::string_view node_name) : node_(node), address_(address) { + // Whenever node is used to compare with wire contents, we don't want any + // original type information information. Also, node is likely never + // appearing in config dump or places where we need to recover the exact + // original. TODO(htuch): If we do need this at some point, add a + // nodeForDisplay() method. + Config::VersionConverter::eraseOriginalTypeInformation(node_); if (!zone_name.empty()) { node_.mutable_locality()->set_zone(std::string(zone_name)); } diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index b4bc060c0c40c..981fc375cb9f7 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -59,6 +59,7 @@ envoy_cc_library( deps = [ ":message_validator_lib", ":protobuf", + ":well_known_lib", "//include/envoy/api:api_interface", "//include/envoy/protobuf:message_validator_interface", "//include/envoy/runtime:runtime_interface", @@ -70,3 +71,8 @@ envoy_cc_library( "@envoy_api//envoy/type/v3alpha:pkg_cc_proto", ], ) + +envoy_cc_library( + name = "well_known_lib", + hdrs = ["well_known.h"], +) diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index b9608545236e4..97bc51663a053 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -12,6 +12,7 @@ #include "common/config/version_converter.h" #include "common/protobuf/message_validator_impl.h" #include "common/protobuf/protobuf.h" +#include "common/protobuf/well_known.h" #include "absl/strings/match.h" #include "yaml-cpp/yaml.h" @@ -311,6 +312,8 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } } +namespace { + void checkForDeprecatedNonRepeatedEnumValue(const Protobuf::Message& message, absl::string_view filename, const Protobuf::FieldDescriptor* field, @@ -356,21 +359,26 @@ void checkForDeprecatedNonRepeatedEnumValue(const Protobuf::Message& message, } } -void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message, - ProtobufMessage::ValidationVisitor& validation_visitor, - Runtime::Loader* runtime) { +void checkForUnexpectedFields(const Protobuf::Message& message, + ProtobufMessage::ValidationVisitor& validation_visitor, + Runtime::Loader* runtime) { // Reject unknown fields. const auto& unknown_fields = message.GetReflection()->GetUnknownFields(message); if (!unknown_fields.empty()) { std::string error_msg; for (int n = 0; n < unknown_fields.field_count(); ++n) { + if (unknown_fields.field(n).number() == ProtobufWellKnown::OriginalTypeFieldNumber) { + continue; + } error_msg += absl::StrCat(n > 0 ? ", " : "", unknown_fields.field(n).number()); } // We use the validation visitor but have hard coded behavior below for deprecated fields. // TODO(htuch): Unify the deprecated and unknown visitor handling behind the validation // visitor pattern. https://github.com/envoyproxy/envoy/issues/8092. - validation_visitor.onUnknownField("type " + message.GetTypeName() + - " with unknown field set {" + error_msg + "}"); + if (!error_msg.empty()) { + validation_visitor.onUnknownField("type " + message.GetTypeName() + + " with unknown field set {" + error_msg + "}"); + } } const Protobuf::Descriptor* descriptor = message.GetDescriptor(); @@ -438,6 +446,14 @@ void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message, } } +} // namespace + +void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message, + ProtobufMessage::ValidationVisitor& validation_visitor, + Runtime::Loader* runtime) { + ::Envoy::checkForUnexpectedFields(API_RECOVER_ORIGINAL(message), validation_visitor, runtime); +} + std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& message, const bool block_print, const bool always_print_primitive_fields) { diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 27ab0ce6f2f77..ee82b01f471fe 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -10,6 +10,7 @@ #include "common/common/hash.h" #include "common/common/utility.h" +#include "common/config/version_converter.h" #include "common/protobuf/protobuf.h" #include "common/singleton/const_singleton.h" @@ -251,7 +252,7 @@ class MessageUtil { std::string err; if (!Validate(message, &err)) { - throw ProtoValidationException(err, message); + throw ProtoValidationException(err, API_RECOVER_ORIGINAL(message)); } } diff --git a/source/common/protobuf/well_known.h b/source/common/protobuf/well_known.h new file mode 100644 index 0000000000000..cd84a5a1ef258 --- /dev/null +++ b/source/common/protobuf/well_known.h @@ -0,0 +1,10 @@ +#pragma once + +namespace Envoy { +namespace ProtobufWellKnown { + +// Used by VersionConverter to track the original type of an upgraded message. +constexpr uint32_t OriginalTypeFieldNumber = 100000; + +} // namespace ProtobufWellKnown +} // namespace Envoy diff --git a/source/common/router/BUILD b/source/common/router/BUILD index b0e044d842ee1..bb04839c65380 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -164,6 +164,7 @@ envoy_cc_library( "//source/common/config:api_version_lib", "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", + "//source/common/config:version_converter_lib", "//source/common/init:manager_lib", "//source/common/init:target_lib", "//source/common/init:watcher_lib", @@ -212,6 +213,7 @@ envoy_cc_library( "//source/common/common:minimal_logger_lib", "//source/common/config:api_version_lib", "//source/common/config:config_provider_lib", + "//source/common/config:version_converter_lib", "//source/common/init:manager_lib", "//source/common/init:watcher_lib", "@envoy_api//envoy/admin/v3alpha:pkg_cc_proto", diff --git a/source/common/router/rds_impl.cc b/source/common/router/rds_impl.cc index a1c1779d66e6e..18e04e7ed6abb 100644 --- a/source/common/router/rds_impl.cc +++ b/source/common/router/rds_impl.cc @@ -17,6 +17,7 @@ #include "common/common/fmt.h" #include "common/config/api_version.h" #include "common/config/utility.h" +#include "common/config/version_converter.h" #include "common/http/header_map_impl.h" #include "common/protobuf/utility.h" #include "common/router/config_impl.h" @@ -392,8 +393,8 @@ RouteConfigProviderManagerImpl::dumpRouteConfigs() const { if (subscription->routeConfigUpdate()->configInfo()) { auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add(); dynamic_config->set_version_info(subscription->routeConfigUpdate()->configVersion()); - dynamic_config->mutable_route_config()->MergeFrom( - subscription->routeConfigUpdate()->routeConfiguration()); + dynamic_config->mutable_route_config()->PackFrom( + API_RECOVER_ORIGINAL(subscription->routeConfigUpdate()->routeConfiguration())); TimestampUtil::systemClockToTimestamp(subscription->routeConfigUpdate()->lastUpdated(), *dynamic_config->mutable_last_updated()); } @@ -402,7 +403,8 @@ RouteConfigProviderManagerImpl::dumpRouteConfigs() const { for (const auto& provider : static_route_config_providers_) { ASSERT(provider->configInfo()); auto* static_config = config_dump->mutable_static_route_configs()->Add(); - static_config->mutable_route_config()->MergeFrom(provider->configInfo().value().config_); + static_config->mutable_route_config()->PackFrom( + API_RECOVER_ORIGINAL(provider->configInfo().value().config_)); TimestampUtil::systemClockToTimestamp(provider->lastUpdated(), *static_config->mutable_last_updated()); } diff --git a/source/common/router/scoped_rds.cc b/source/common/router/scoped_rds.cc index 4e4e5b522a6a0..0f94d20fb1094 100644 --- a/source/common/router/scoped_rds.cc +++ b/source/common/router/scoped_rds.cc @@ -16,6 +16,7 @@ #include "common/common/utility.h" #include "common/config/api_version.h" #include "common/config/resources.h" +#include "common/config/version_converter.h" #include "common/init/manager_impl.h" #include "common/init/watcher_impl.h" @@ -383,7 +384,8 @@ ProtobufTypes::MessagePtr ScopedRoutesConfigProviderManager::dumpConfigs() const dynamic_config->set_name(typed_subscription->name()); const ScopedRouteMap& scoped_route_map = typed_subscription->scopedRouteMap(); for (const auto& it : scoped_route_map) { - dynamic_config->mutable_scoped_route_configs()->Add()->MergeFrom(it.second->configProto()); + dynamic_config->mutable_scoped_route_configs()->Add()->PackFrom( + API_RECOVER_ORIGINAL(it.second->configProto())); } TimestampUtil::systemClockToTimestamp(subscription->lastUpdated(), *dynamic_config->mutable_last_updated()); @@ -397,7 +399,8 @@ ProtobufTypes::MessagePtr ScopedRoutesConfigProviderManager::dumpConfigs() const auto* inline_config = config_dump->mutable_inline_scoped_route_configs()->Add(); inline_config->set_name(static_cast(provider)->name()); for (const auto& config_proto : protos_info.value().config_protos_) { - inline_config->mutable_scoped_route_configs()->Add()->MergeFrom(*config_proto); + inline_config->mutable_scoped_route_configs()->Add()->PackFrom( + API_RECOVER_ORIGINAL(*config_proto)); } TimestampUtil::systemClockToTimestamp(provider->lastUpdated(), *inline_config->mutable_last_updated()); diff --git a/source/common/secret/BUILD b/source/common/secret/BUILD index 3055c18e2a939..637007f0bc31e 100644 --- a/source/common/secret/BUILD +++ b/source/common/secret/BUILD @@ -19,6 +19,7 @@ envoy_cc_library( "//include/envoy/server:transport_socket_config_interface", "//source/common/common:assert_lib", "//source/common/common:minimal_logger_lib", + "//source/common/config:version_converter_lib", "@envoy_api//envoy/admin/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto", "@envoy_api//envoy/extensions/transport_sockets/tls/v3alpha:pkg_cc_proto", diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc index 5fb060535acf4..3b6c940fbdfcf 100644 --- a/source/common/secret/secret_manager_impl.cc +++ b/source/common/secret/secret_manager_impl.cc @@ -8,6 +8,7 @@ #include "common/common/assert.h" #include "common/common/logger.h" +#include "common/config/version_converter.h" #include "common/secret/sds_api.h" #include "common/secret/secret_provider_impl.h" #include "common/ssl/certificate_validation_context_config_impl.h" @@ -155,6 +156,11 @@ void redactSecret(envoy::extensions::transport_sockets::tls::v3alpha::Secret* se } ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { + // TODO(htuch): unlike other config providers, we're recreating the original + // Secrets below. This makes it hard to support API_RECOVER_ORIGINAL()-style + // recovery of the original config message. As a result, for now we're + // providing v3 config dumps. For Secrets, the main deprecation of interest + // are the use of v2 Struct config() and verify_subject_alt_name. auto config_dump = std::make_unique(); // Handle static tls key/cert providers. for (const auto& cert_iter : static_tls_certificate_providers_) { @@ -162,10 +168,11 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { auto static_secret = config_dump->mutable_static_secrets()->Add(); static_secret->set_name(cert_iter.first); ASSERT(tls_cert != nullptr); - auto dump_secret = static_secret->mutable_secret(); - dump_secret->set_name(cert_iter.first); - dump_secret->mutable_tls_certificate()->MergeFrom(*tls_cert->secret()); - redactSecret(dump_secret); + envoy::extensions::transport_sockets::tls::v3alpha::Secret dump_secret; + dump_secret.set_name(cert_iter.first); + dump_secret.mutable_tls_certificate()->MergeFrom(*tls_cert->secret()); + redactSecret(&dump_secret); + static_secret->mutable_secret()->PackFrom(dump_secret); } // Handle static certificate validation context providers. @@ -174,9 +181,10 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { auto static_secret = config_dump->mutable_static_secrets()->Add(); static_secret->set_name(context_iter.first); ASSERT(validation_context != nullptr); - auto dump_secret = static_secret->mutable_secret(); - dump_secret->set_name(context_iter.first); - dump_secret->mutable_validation_context()->MergeFrom(*validation_context->secret()); + envoy::extensions::transport_sockets::tls::v3alpha::Secret dump_secret; + dump_secret.set_name(context_iter.first); + dump_secret.mutable_validation_context()->MergeFrom(*validation_context->secret()); + static_secret->mutable_secret()->PackFrom(dump_secret); } // Handle static session keys providers. @@ -185,12 +193,13 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { auto static_secret = config_dump->mutable_static_secrets()->Add(); static_secret->set_name(context_iter.first); ASSERT(session_ticket_keys != nullptr); - auto dump_secret = static_secret->mutable_secret(); - dump_secret->set_name(context_iter.first); + envoy::extensions::transport_sockets::tls::v3alpha::Secret dump_secret; + dump_secret.set_name(context_iter.first); for (const auto& key : session_ticket_keys->secret()->keys()) { - dump_secret->mutable_session_ticket_keys()->add_keys()->MergeFrom(key); + dump_secret.mutable_session_ticket_keys()->add_keys()->MergeFrom(key); } - redactSecret(dump_secret); + redactSecret(&dump_secret); + static_secret->mutable_secret()->PackFrom(dump_secret); } // Handle dynamic tls_certificate providers. @@ -206,17 +215,18 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { dump_secret = config_dump->mutable_dynamic_warming_secrets()->Add(); } dump_secret->set_name(secret_data.resource_name_); - auto secret = dump_secret->mutable_secret(); - secret->set_name(secret_data.resource_name_); + envoy::extensions::transport_sockets::tls::v3alpha::Secret secret; + secret.set_name(secret_data.resource_name_); ProtobufWkt::Timestamp last_updated_ts; TimestampUtil::systemClockToTimestamp(secret_data.last_updated_, last_updated_ts); dump_secret->set_version_info(secret_data.version_info_); *dump_secret->mutable_last_updated() = last_updated_ts; - secret->set_name(secret_data.resource_name_); + secret.set_name(secret_data.resource_name_); if (secret_ready) { - secret->mutable_tls_certificate()->MergeFrom(*tls_cert); + secret.mutable_tls_certificate()->MergeFrom(*tls_cert); } - redactSecret(secret); + redactSecret(&secret); + dump_secret->mutable_secret()->PackFrom(secret); } // Handling dynamic cert validation context providers. @@ -232,15 +242,16 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { dump_secret = config_dump->mutable_dynamic_warming_secrets()->Add(); } dump_secret->set_name(secret_data.resource_name_); - auto secret = dump_secret->mutable_secret(); - secret->set_name(secret_data.resource_name_); + envoy::extensions::transport_sockets::tls::v3alpha::Secret secret; + secret.set_name(secret_data.resource_name_); ProtobufWkt::Timestamp last_updated_ts; TimestampUtil::systemClockToTimestamp(secret_data.last_updated_, last_updated_ts); dump_secret->set_version_info(secret_data.version_info_); *dump_secret->mutable_last_updated() = last_updated_ts; if (secret_ready) { - secret->mutable_validation_context()->MergeFrom(*validation_context); + secret.mutable_validation_context()->MergeFrom(*validation_context); } + dump_secret->mutable_secret()->PackFrom(secret); } // Handle dynamic session keys providers providers. @@ -256,17 +267,17 @@ ProtobufTypes::MessagePtr SecretManagerImpl::dumpSecretConfigs() { dump_secret = config_dump->mutable_dynamic_warming_secrets()->Add(); } dump_secret->set_name(secret_data.resource_name_); - auto secret = dump_secret->mutable_secret(); - secret->set_name(secret_data.resource_name_); + envoy::extensions::transport_sockets::tls::v3alpha::Secret secret; + secret.set_name(secret_data.resource_name_); ProtobufWkt::Timestamp last_updated_ts; TimestampUtil::systemClockToTimestamp(secret_data.last_updated_, last_updated_ts); dump_secret->set_version_info(secret_data.version_info_); *dump_secret->mutable_last_updated() = last_updated_ts; - secret->set_name(secret_data.resource_name_); if (secret_ready) { - secret->mutable_session_ticket_keys()->MergeFrom(*tls_stek); + secret.mutable_session_ticket_keys()->MergeFrom(*tls_stek); } - redactSecret(secret); + redactSecret(&secret); + dump_secret->mutable_secret()->PackFrom(secret); } return config_dump; } diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index c92904d520e6c..61f5eb9a9489a 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -54,6 +54,7 @@ envoy_cc_library( "//source/common/config:grpc_mux_lib", "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", + "//source/common/config:version_converter_lib", "//source/common/grpc:async_client_manager_lib", "//source/common/http:async_client_lib", "//source/common/http/http1:conn_pool_lib", @@ -186,6 +187,7 @@ envoy_cc_library( "//include/envoy/stats:stats_macros", "//include/envoy/upstream:cluster_manager_interface", "//source/common/common:minimal_logger_lib", + "//source/common/config:version_converter_lib", "//source/common/grpc:async_client_lib", "@envoy_api//envoy/service/load_stats/v3alpha:pkg_cc_proto", ], @@ -354,6 +356,7 @@ envoy_cc_library( "//source/common/config:metadata_lib", "//source/common/config:subscription_factory_lib", "//source/common/config:utility_lib", + "//source/common/config:version_converter_lib", "//source/common/network:address_lib", "//source/common/network:resolver_lib", "//source/common/network:utility_lib", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 90e3711a5383e..34039b6269354 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -24,6 +24,7 @@ #include "common/config/new_grpc_mux_impl.h" #include "common/config/resources.h" #include "common/config/utility.h" +#include "common/config/version_converter.h" #include "common/grpc/async_client_manager_impl.h" #include "common/http/async_client_impl.h" #include "common/http/http1/conn_pool.h" @@ -875,13 +876,13 @@ ProtobufTypes::MessagePtr ClusterManagerImpl::dumpClusterConfigs() { const auto& cluster = *active_cluster_pair.second; if (!cluster.added_via_api_) { auto& static_cluster = *config_dump->mutable_static_clusters()->Add(); - static_cluster.mutable_cluster()->MergeFrom(cluster.cluster_config_); + static_cluster.mutable_cluster()->PackFrom(API_RECOVER_ORIGINAL(cluster.cluster_config_)); TimestampUtil::systemClockToTimestamp(cluster.last_updated_, *(static_cluster.mutable_last_updated())); } else { auto& dynamic_cluster = *config_dump->mutable_dynamic_active_clusters()->Add(); dynamic_cluster.set_version_info(cluster.version_info_); - dynamic_cluster.mutable_cluster()->MergeFrom(cluster.cluster_config_); + dynamic_cluster.mutable_cluster()->PackFrom(API_RECOVER_ORIGINAL(cluster.cluster_config_)); TimestampUtil::systemClockToTimestamp(cluster.last_updated_, *(dynamic_cluster.mutable_last_updated())); } @@ -891,7 +892,7 @@ ProtobufTypes::MessagePtr ClusterManagerImpl::dumpClusterConfigs() { const auto& cluster = *warming_cluster_pair.second; auto& dynamic_cluster = *config_dump->mutable_dynamic_warming_clusters()->Add(); dynamic_cluster.set_version_info(cluster.version_info_); - dynamic_cluster.mutable_cluster()->MergeFrom(cluster.cluster_config_); + dynamic_cluster.mutable_cluster()->PackFrom(API_RECOVER_ORIGINAL(cluster.cluster_config_)); TimestampUtil::systemClockToTimestamp(cluster.last_updated_, *(dynamic_cluster.mutable_last_updated())); } diff --git a/source/common/upstream/eds.cc b/source/common/upstream/eds.cc index 849eb177313cc..3e1ef1fbc74d8 100644 --- a/source/common/upstream/eds.cc +++ b/source/common/upstream/eds.cc @@ -11,6 +11,7 @@ #include "common/common/assert.h" #include "common/common/utility.h" #include "common/config/api_version.h" +#include "common/config/version_converter.h" namespace Envoy { namespace Upstream { @@ -121,6 +122,9 @@ void EdsClusterImpl::onConfigUpdate(const Protobuf::RepeatedPtrFieldenabled()) { diff --git a/source/server/BUILD b/source/server/BUILD index 89487314143cd..99f5a27778b7c 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -324,6 +324,7 @@ envoy_cc_library( "//include/envoy/server:transport_socket_config_interface", "//include/envoy/server:worker_interface", "//source/common/config:utility_lib", + "//source/common/config:version_converter_lib", "//source/common/init:manager_lib", "//source/common/network:listen_socket_lib", "//source/common/network:socket_option_factory_lib", diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index c187bfc9898e5..eb390fac4254f 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -49,6 +49,7 @@ #include "common/network/listen_socket_impl.h" #include "common/network/utility.h" #include "common/profiler/profiler.h" +#include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" #include "common/router/config_impl.h" #include "common/stats/histogram_impl.h" @@ -227,6 +228,80 @@ void setHealthFlag(Upstream::Host::HealthFlag flag, const Upstream::Host& host, break; } } + +// Apply a field mask to a resource message. These have a single Any field to +// hold the resource, and other fields with items such as name, time stamp, +// etc. We need to provide a way to trim with field masks that makes use of Any +// transparent, since native field masks don't work with Any. We simplify and +// assume at most one Any message in a resource. +// TODO(htuch): we could make field masks more powerful in future and generalize +// this to allow arbitrary indexing through Any fields. This is pretty +// complicated, we would need to build a FieldMask tree similar to how the C++ +// Protobuf library does this internally. +void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) { + const Protobuf::Descriptor* descriptor = message.GetDescriptor(); + const Protobuf::Reflection* reflection = message.GetReflection(); + // Figure out which paths cover Any fields. For each field, gather the paths to + // an inner mask, switch the outer mask to cover only the original field. + Protobuf::FieldMask outer_field_mask; + Protobuf::FieldMask inner_field_mask; + std::string any_field_name; + for (int i = 0; i < field_mask.paths().size(); ++i) { + const std::string& path = field_mask.paths(i); + std::vector frags = absl::StrSplit(path, "."); + if (frags.empty()) { + continue; + } + const Protobuf::FieldDescriptor* field = descriptor->FindFieldByName(frags[0]); + // Only a single Any field supported, repeated fields don't support further + // indexing. + // TODO(htuch): should add support for DynamicListener for multiple Any + // fields in the future. + if (field != nullptr && field->message_type() != nullptr && !field->is_repeated() && + field->message_type()->full_name() == "google.protobuf.Any") { + if (any_field_name.empty()) { + any_field_name = frags[0]; + } else { + // This should be structurally true due to the ConfigDump proto + // definition (but not for DynamicListener today). + ASSERT(any_field_name == frags[0], + "Only a single Any field in a config dump resource is supported."); + } + outer_field_mask.add_paths(frags[0]); + frags.erase(frags.begin()); + inner_field_mask.add_paths(absl::StrJoin(frags, ".")); + } else { + outer_field_mask.add_paths(path); + } + } + + if (!any_field_name.empty()) { + const Protobuf::FieldDescriptor* any_field = descriptor->FindFieldByName(any_field_name); + if (reflection->HasField(message, any_field)) { + ASSERT(any_field != nullptr); + // Unpack to a DynamicMessage. + ProtobufWkt::Any any_message; + any_message.MergeFrom(reflection->GetMessage(message, any_field)); + Protobuf::DynamicMessageFactory dmf; + const absl::string_view inner_type_name = + TypeUtil::typeUrlToDescriptorFullName(any_message.type_url()); + const Protobuf::Descriptor* inner_descriptor = + Protobuf::DescriptorPool::generated_pool()->FindMessageTypeByName( + static_cast(inner_type_name)); + ASSERT(inner_descriptor != nullptr); + std::unique_ptr inner_message; + inner_message.reset(dmf.GetPrototype(inner_descriptor)->New()); + MessageUtil::unpackTo(any_message, *inner_message); + // Trim message. + ProtobufUtil::FieldMaskUtil::TrimMessage(inner_field_mask, inner_message.get()); + // Pack it back into the Any resource. + any_message.PackFrom(*inner_message); + reflection->MutableMessage(&message, any_field)->CopyFrom(any_message); + } + } + ProtobufUtil::FieldMaskUtil::TrimMessage(outer_field_mask, &message); +} + } // namespace AdminFilter::AdminFilter(AdminImpl& parent) : parent_(parent) {} @@ -546,6 +621,8 @@ void AdminImpl::addAllConfigToDump(envoy::admin::v3alpha::ConfigDump& dump, if (mask.has_value()) { Protobuf::FieldMask field_mask; ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask); + // We don't use trimMessage() above here since masks don't support + // indexing through repeated fields. ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, message.get()); } @@ -578,7 +655,7 @@ AdminImpl::addResourceToDump(envoy::admin::v3alpha::ConfigDump& dump, if (mask.has_value()) { Protobuf::FieldMask field_mask; ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask); - ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, &msg); + trimResourceMessage(field_mask, msg); } auto* config = dump.add_configs(); config->PackFrom(msg); diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index c77e2ef1b9b7a..563c5917b6325 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -15,6 +15,7 @@ #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/config/utility.h" +#include "common/config/version_converter.h" #include "common/network/io_socket_handle_impl.h" #include "common/network/listen_socket_impl.h" #include "common/network/socket_option_factory.h" @@ -66,7 +67,7 @@ envoy::admin::v3alpha::ListenersConfigDump::DynamicListener* getOrCreateDynamicL void fillState(envoy::admin::v3alpha::ListenersConfigDump::DynamicListenerState& state, const ListenerImpl& listener) { state.set_version_info(listener.versionInfo()); - state.mutable_listener()->MergeFrom(listener.config()); + state.mutable_listener()->PackFrom(API_RECOVER_ORIGINAL(listener.config())); TimestampUtil::systemClockToTimestamp(listener.last_updated_, *(state.mutable_last_updated())); } @@ -260,7 +261,7 @@ ProtobufTypes::MessagePtr ListenerManagerImpl::dumpListenerConfigs() { for (const auto& listener : active_listeners_) { if (listener->blockRemove()) { auto& static_listener = *config_dump->mutable_static_listeners()->Add(); - static_listener.mutable_listener()->MergeFrom(listener->config()); + static_listener.mutable_listener()->PackFrom(API_RECOVER_ORIGINAL(listener->config())); TimestampUtil::systemClockToTimestamp(listener->last_updated_, *(static_listener.mutable_last_updated())); continue; @@ -336,7 +337,7 @@ bool ListenerManagerImpl::addOrUpdateListener( TimestampUtil::systemClockToTimestamp(server_.api().timeSource().systemTime(), *(it->second->mutable_last_update_attempt())); it->second->set_details(e.what()); - it->second->mutable_failed_configuration()->PackFrom(config); + it->second->mutable_failed_configuration()->PackFrom(API_RECOVER_ORIGINAL(config)); throw e; } error_state_tracker_.erase(it); diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 763424010c88b..ee7f0107f2a0f 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -417,9 +417,11 @@ envoy_cc_test( ":version_converter_proto_cc_proto", "//source/common/config:api_version_lib", "//source/common/config:version_converter_lib", + "//source/common/protobuf:well_known_lib", "//test/test_common:test_time_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:pkg_cc_proto", "@envoy_api//envoy/config/cluster/v3alpha:pkg_cc_proto", + "@envoy_api//envoy/service/discovery/v3alpha:pkg_cc_proto", ], ) diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc index aac510b8e9b8d..a4f1d3d6a61fa 100644 --- a/test/common/config/version_converter_test.cc +++ b/test/common/config/version_converter_test.cc @@ -1,8 +1,11 @@ #include "envoy/api/v2/cluster.pb.h" +#include "envoy/api/v2/discovery.pb.h" #include "envoy/config/cluster/v3alpha/cluster.pb.h" +#include "envoy/service/discovery/v3alpha/discovery.pb.h" #include "common/config/api_version.h" #include "common/config/version_converter.h" +#include "common/protobuf/well_known.h" #include "test/common/config/version_converter.pb.h" #include "test/test_common/utility.h" @@ -13,13 +16,87 @@ namespace Envoy { namespace Config { namespace { +bool hasOriginalTypeInformation(const Protobuf::Message& message) { + const Protobuf::Reflection* reflection = message.GetReflection(); + const auto& unknown_field_set = reflection->GetUnknownFields(message); + for (int i = 0; i < unknown_field_set.field_count(); ++i) { + const auto& unknown_field = unknown_field_set.field(i); + if (unknown_field.number() == ProtobufWellKnown::OriginalTypeFieldNumber) { + return true; + } + } + return false; +} + // Wire-style upgrading between versions. TEST(VersionConverterTest, Upgrade) { + // Create a v2 Cluster message with some fields set. API_NO_BOOST(envoy::api::v2::Cluster) source; + source.add_hosts(); + source.mutable_load_assignment()->set_cluster_name("bar"); + source.mutable_eds_cluster_config()->set_service_name("foo"); source.set_drain_connections_on_host_removal(true); + // Upgrade to a v3 Cluster. API_NO_BOOST(envoy::config::cluster::v3alpha::Cluster) dst; VersionConverter::upgrade(source, dst); + // Verify fields in v3 Cluster. + EXPECT_TRUE(hasOriginalTypeInformation(dst)); + EXPECT_FALSE(dst.hosts().empty()); + EXPECT_FALSE(hasOriginalTypeInformation(dst.hosts(0))); + EXPECT_EQ("bar", dst.load_assignment().cluster_name()); + EXPECT_FALSE(hasOriginalTypeInformation(dst.load_assignment())); + EXPECT_EQ("foo", dst.eds_cluster_config().service_name()); + EXPECT_TRUE(hasOriginalTypeInformation(dst.eds_cluster_config())); EXPECT_TRUE(dst.ignore_health_on_host_removal()); + // Recover a v2 Cluster from the v3 Cluster using original type information. + auto original_dynamic_msg = VersionConverter::recoverOriginal(dst); + const auto& original_msg = *original_dynamic_msg->msg_; + EXPECT_EQ("envoy.api.v2.Cluster", original_msg.GetDescriptor()->full_name()); + // Ensure that we erased any original type information and have the original + // message. + EXPECT_THAT(original_msg, ProtoEq(source)); + // Verify that sub-messages work with VersionConverter::recoverOriginal, i.e. + // we are propagating original type information. + auto original_dynamic_sub_msg = VersionConverter::recoverOriginal(dst.eds_cluster_config()); + const auto& original_sub_msg = *original_dynamic_sub_msg->msg_; + EXPECT_THAT(original_sub_msg, ProtoEq(source.eds_cluster_config())); +} + +// Verify that VersionUtil::scrubHiddenEnvoyDeprecated recursively scrubs any +// deprecated fields. +TEST(VersionConverterTest, ScrubHiddenEnvoyDeprecated) { + API_NO_BOOST(envoy::config::cluster::v3alpha::Cluster) msg; + msg.set_name("foo"); + msg.mutable_hidden_envoy_deprecated_tls_context(); + EXPECT_TRUE(msg.has_hidden_envoy_deprecated_tls_context()); + msg.mutable_load_balancing_policy()->add_policies()->mutable_hidden_envoy_deprecated_config(); + EXPECT_TRUE(msg.load_balancing_policy().policies(0).has_hidden_envoy_deprecated_config()); + VersionUtil::scrubHiddenEnvoyDeprecated(msg); + EXPECT_EQ("foo", msg.name()); + EXPECT_FALSE(msg.has_hidden_envoy_deprecated_tls_context()); + EXPECT_FALSE(msg.load_balancing_policy().policies(0).has_hidden_envoy_deprecated_config()); +} + +// Validate that we can sensible reinterpret messages such as DiscoveryRequest +// based on transport API version. +TEST(VersionConverter, Reinterpret) { + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request; + discovery_request.mutable_node()->set_hidden_envoy_deprecated_build_version("foo"); + discovery_request.mutable_node()->set_user_agent_name("bar"); + auto v2_discovery_request = VersionConverter::reinterpret( + discovery_request, envoy::config::core::v3alpha::ApiVersion::V2); + API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_v2_discovery_request; + expected_v2_discovery_request.mutable_node()->set_build_version("foo"); + expected_v2_discovery_request.mutable_node()->set_user_agent_name("bar"); + EXPECT_THAT(*v2_discovery_request->msg_, ProtoEq(expected_v2_discovery_request)); + auto auto_discovery_request = VersionConverter::reinterpret( + discovery_request, envoy::config::core::v3alpha::ApiVersion::AUTO); + EXPECT_THAT(*auto_discovery_request->msg_, ProtoEq(expected_v2_discovery_request)); + auto v3_discovery_request = VersionConverter::reinterpret( + discovery_request, envoy::config::core::v3alpha::ApiVersion::V3ALPHA); + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) expected_v3_discovery_request; + expected_v3_discovery_request.mutable_node()->set_user_agent_name("bar"); + EXPECT_THAT(*v3_discovery_request->msg_, ProtoEq(expected_v3_discovery_request)); } // Downgrading to an earlier version (where it exists). diff --git a/test/common/router/rds_impl_test.cc b/test/common/router/rds_impl_test.cc index 5f2ccc0058408..8e5ef3dd29ee1 100644 --- a/test/common/router/rds_impl_test.cc +++ b/test/common/router/rds_impl_test.cc @@ -342,7 +342,7 @@ class RouteConfigProviderManagerImplTest : public RdsTestBase { envoy::config::route::v3alpha::RouteConfiguration parseRouteConfigurationFromV2Yaml(const std::string& yaml) { envoy::config::route::v3alpha::RouteConfiguration route_config; - TestUtility::loadFromYaml(yaml, route_config); + TestUtility::loadFromYaml(yaml, route_config, true); return route_config; } @@ -386,6 +386,7 @@ name: foo TestUtility::loadFromYaml(R"EOF( static_route_configs: - route_config: + "@type": type.googleapis.com/envoy.api.v2.RouteConfiguration name: foo virtual_hosts: - name: bar @@ -432,6 +433,7 @@ name: foo TestUtility::loadFromYaml(R"EOF( static_route_configs: - route_config: + "@type": type.googleapis.com/envoy.api.v2.RouteConfiguration name: foo virtual_hosts: - name: bar @@ -445,6 +447,7 @@ name: foo dynamic_route_configs: - version_info: "1" route_config: + "@type": type.googleapis.com/envoy.api.v2.RouteConfiguration name: foo_route_config virtual_hosts: last_updated: diff --git a/test/common/router/scoped_rds_test.cc b/test/common/router/scoped_rds_test.cc index 5e77f0a4e9705..d88bacbb00649 100644 --- a/test/common/router/scoped_rds_test.cc +++ b/test/common/router/scoped_rds_test.cc @@ -44,7 +44,7 @@ using ::Envoy::Http::TestHeaderMapImpl; envoy::config::route::v3alpha::ScopedRouteConfiguration parseScopedRouteConfigurationFromYaml(const std::string& yaml) { envoy::config::route::v3alpha::ScopedRouteConfiguration scoped_route_config; - TestUtility::loadFromYaml(yaml, scoped_route_config); + TestUtility::loadFromYaml(yaml, scoped_route_config, true); return scoped_route_config; } @@ -57,7 +57,7 @@ envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConne parseHttpConnectionManagerFromYaml(const std::string& config_yaml) { envoy::extensions::filters::network::http_connection_manager::v3alpha::HttpConnectionManager http_connection_manager; - TestUtility::loadFromYaml(config_yaml, http_connection_manager); + TestUtility::loadFromYaml(config_yaml, http_connection_manager, true); return http_connection_manager; } @@ -752,10 +752,12 @@ stat_prefix: foo - name: foo-scoped-routes scoped_route_configs: - name: foo + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config key: fragments: { string_key: "172.10.10.10" } - name: foo2 + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config2 key: fragments: { string_key: "172.10.10.20" } @@ -765,7 +767,7 @@ stat_prefix: foo dynamic_scoped_route_configs: )EOF", expected_config_dump); - EXPECT_TRUE(TestUtility::protoEqual(expected_config_dump, scoped_routes_config_dump2)); + EXPECT_THAT(expected_config_dump, ProtoEq(scoped_routes_config_dump2)); // Now SRDS kicks off. Protobuf::RepeatedPtrField resources; @@ -784,10 +786,12 @@ route_configuration_name: dynamic-foo-route-config - name: foo-scoped-routes scoped_route_configs: - name: foo + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config key: fragments: { string_key: "172.10.10.10" } - name: foo2 + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config2 key: fragments: { string_key: "172.10.10.20" } @@ -798,6 +802,7 @@ route_configuration_name: dynamic-foo-route-config - name: foo_scoped_routes scoped_route_configs: - name: dynamic-foo + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: dynamic-foo-route-config key: fragments: { string_key: "172.30.30.10" } @@ -811,7 +816,7 @@ route_configuration_name: dynamic-foo-route-config const auto& scoped_routes_config_dump3 = TestUtility::downcastAndValidate( *message_ptr); - EXPECT_TRUE(TestUtility::protoEqual(expected_config_dump, scoped_routes_config_dump3)); + EXPECT_THAT(expected_config_dump, ProtoEq(scoped_routes_config_dump3)); resources.Clear(); srds_subscription_->onConfigUpdate(resources, "2"); @@ -820,10 +825,12 @@ route_configuration_name: dynamic-foo-route-config - name: foo-scoped-routes scoped_route_configs: - name: foo + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config key: fragments: { string_key: "172.10.10.10" } - name: foo2 + "@type": type.googleapis.com/envoy.api.v2.ScopedRouteConfiguration route_configuration_name: foo-route-config2 key: fragments: { string_key: "172.10.10.20" } @@ -842,7 +849,7 @@ route_configuration_name: dynamic-foo-route-config const auto& scoped_routes_config_dump4 = TestUtility::downcastAndValidate( *message_ptr); - EXPECT_TRUE(TestUtility::protoEqual(expected_config_dump, scoped_routes_config_dump4)); + EXPECT_THAT(expected_config_dump, ProtoEq(scoped_routes_config_dump4)); } // Tests that SRDS only allows creation of delta static config providers. diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc index 551f6f944f75d..d11d93319a808 100644 --- a/test/common/secret/secret_manager_impl_test.cc +++ b/test/common/secret/secret_manager_impl_test.cc @@ -396,6 +396,7 @@ name: "abc.com" seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" tls_certificate: certificate_chain: @@ -435,6 +436,7 @@ name: "abc.com.validation" seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" tls_certificate: certificate_chain: @@ -448,6 +450,7 @@ name: "abc.com.validation" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.validation" validation_context: trusted_ca: @@ -484,6 +487,7 @@ name: "abc.com.stek" seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" tls_certificate: certificate_chain: @@ -497,6 +501,7 @@ name: "abc.com.stek" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.validation" validation_context: trusted_ca: @@ -506,6 +511,7 @@ name: "abc.com.stek" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.stek" session_ticket_keys: keys: @@ -550,6 +556,7 @@ TEST_F(SecretManagerImplTest, ConfigDumpHandlerWarmingSecrets) { seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" )EOF"; checkConfigDump(expected_secrets_config_dump); @@ -566,12 +573,14 @@ TEST_F(SecretManagerImplTest, ConfigDumpHandlerWarmingSecrets) { seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" - name: "abc.com.validation" version_info: "uninitialized" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.validation" )EOF"; checkConfigDump(updated_config_dump); @@ -588,18 +597,21 @@ TEST_F(SecretManagerImplTest, ConfigDumpHandlerWarmingSecrets) { seconds: 1234567891 nanos: 234000000 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" - name: "abc.com.validation" version_info: "uninitialized" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.validation" - name: "abc.com.stek" version_info: "uninitialized" last_updated: seconds: 1234567899 secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.stek" )EOF"; checkConfigDump(updated_once_more_config_dump); @@ -657,6 +669,7 @@ name: "abc.com.nopassword" static_secrets: - name: "abc.com.nopassword" secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.nopassword" tls_certificate: certificate_chain: @@ -665,6 +678,7 @@ name: "abc.com.nopassword" inline_string: "[redacted]" - name: "abc.com" secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" tls_certificate: certificate_chain: @@ -716,6 +730,7 @@ name: "abc.com" static_secrets: - name: "abc.com" secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com" tls_certificate: certificate_chain: @@ -764,6 +779,7 @@ name: "abc.com.validation" static_secrets: - name: "abc.com.validation" secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.validation" validation_context: trusted_ca: @@ -810,6 +826,7 @@ name: "abc.com.stek" static_secrets: - name: "abc.com.stek" secret: + "@type": type.googleapis.com/envoy.extensions.transport_sockets.tls.v3alpha.Secret name: "abc.com.stek" session_ticket_keys: keys: diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index c2c61aa39b365..1f32857581dc9 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -22,7 +22,7 @@ namespace { envoy::config::bootstrap::v3alpha::Bootstrap parseBootstrapFromV2Yaml(const std::string& yaml) { envoy::config::bootstrap::v3alpha::Bootstrap bootstrap; - TestUtility::loadFromYaml(yaml, bootstrap); + TestUtility::loadFromYaml(yaml, bootstrap, true); return bootstrap; } @@ -189,6 +189,7 @@ TEST_F(ClusterManagerImplTest, MultipleProtocolCluster) { checkConfigDump(R"EOF( static_clusters: - cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: http12_cluster connect_timeout: 0.250s lb_policy: ROUND_ROBIN @@ -934,6 +935,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { version_info: version3 static_clusters: - cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "cds_cluster" type: "STATIC" connect_timeout: 0.25s @@ -945,6 +947,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { seconds: 1234567891 nanos: 234000000 - cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "fake_cluster" type: "STATIC" connect_timeout: 0.25s @@ -956,6 +959,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { seconds: 1234567891 nanos: 234000000 - cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "fake_cluster2" type: "STATIC" connect_timeout: 0.25s @@ -969,6 +973,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { dynamic_active_clusters: - version_info: "version1" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "cluster3" type: "STATIC" connect_timeout: 0.25s @@ -981,6 +986,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { nanos: 234000000 - version_info: "version2" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "cluster4" type: "STATIC" connect_timeout: 0.25s @@ -993,6 +999,7 @@ TEST_F(ClusterManagerImplTest, InitializeOrder) { nanos: 234000000 - version_info: "version3" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "cluster5" type: "STATIC" connect_timeout: 0.25s @@ -1106,6 +1113,7 @@ TEST_F(ClusterManagerImplTest, RemoveWarmingCluster) { dynamic_warming_clusters: - version_info: "version1" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "fake_cluster" type: STATIC connect_timeout: 0.25s @@ -1148,6 +1156,7 @@ TEST_F(ClusterManagerImplTest, ModifyWarmingCluster) { dynamic_warming_clusters: - version_info: "version1" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "fake_cluster" type: STATIC connect_timeout: 0.25s @@ -1180,6 +1189,7 @@ TEST_F(ClusterManagerImplTest, ModifyWarmingCluster) { dynamic_warming_clusters: - version_info: "version2" cluster: + "@type": type.googleapis.com/envoy.api.v2.Cluster name: "fake_cluster" type: STATIC connect_timeout: 0.25s diff --git a/test/common/upstream/utility.h b/test/common/upstream/utility.h index 43b426647caff..3d68faa7f04a8 100644 --- a/test/common/upstream/utility.h +++ b/test/common/upstream/utility.h @@ -46,20 +46,20 @@ inline std::string defaultStaticClusterJson(const std::string& name) { inline envoy::config::bootstrap::v3alpha::Bootstrap parseBootstrapFromV2Json(const std::string& json_string) { envoy::config::bootstrap::v3alpha::Bootstrap bootstrap; - TestUtility::loadFromJson(json_string, bootstrap); + TestUtility::loadFromJson(json_string, bootstrap, true); return bootstrap; } inline envoy::config::cluster::v3alpha::Cluster parseClusterFromV2Json(const std::string& json_string) { envoy::config::cluster::v3alpha::Cluster cluster; - TestUtility::loadFromJson(json_string, cluster); + TestUtility::loadFromJson(json_string, cluster, true); return cluster; } inline envoy::config::cluster::v3alpha::Cluster parseClusterFromV2Yaml(const std::string& yaml) { envoy::config::cluster::v3alpha::Cluster cluster; - TestUtility::loadFromYaml(yaml, cluster); + TestUtility::loadFromYaml(yaml, cluster, true); return cluster; } diff --git a/test/integration/BUILD b/test/integration/BUILD index 710fadc9c81df..4d49fbf846d99 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -394,6 +394,7 @@ envoy_cc_test( "@envoy_api//envoy/config/bootstrap/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/core/v3alpha:pkg_cc_proto", "@envoy_api//envoy/config/metrics/v3alpha:pkg_cc_proto", + "@envoy_api//envoy/config/route/v3alpha:pkg_cc_proto", ], ) diff --git a/test/integration/api_version_integration_test.cc b/test/integration/api_version_integration_test.cc index 30cd05d664b88..1350833e4623f 100644 --- a/test/integration/api_version_integration_test.cc +++ b/test/integration/api_version_integration_test.cc @@ -25,6 +25,16 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, skipPortUsageValidation(); } + static bool hasHiddenEnvoyDeprecated(const Protobuf::Message& message) { + // Do this the slow copy-based way, since this is just for test validation. + ProtobufTypes::MessagePtr mutable_clone; + mutable_clone.reset(message.New()); + mutable_clone->MergeFrom(message); + Config::VersionUtil::scrubHiddenEnvoyDeprecated(*mutable_clone); + return !TestUtility::protoEqual(message, *mutable_clone, + /*ignore_repeated_field_ordering=*/false); + } + static std::string paramsToString(const testing::TestParamInfo& p) { return fmt::format( "{}_{}_{}_Resource_{}_Transport_{}", @@ -119,7 +129,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, case envoy::config::core::v3alpha::ApiConfigSource::GRPC: { API_NO_BOOST(envoy::api::v2::DiscoveryRequest) discovery_request; VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); - EXPECT_TRUE(!Config::VersionUtil::hasHiddenEnvoyDeprecated(discovery_request)); + EXPECT_TRUE(!hasHiddenEnvoyDeprecated(discovery_request)); xds_stream_->startGrpcStream(); actual_type_url = discovery_request.type_url(); expected_endpoint = ads() ? ads_v2_sotw_endpoint : expected_v2_sotw_endpoint; @@ -128,7 +138,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, case envoy::config::core::v3alpha::ApiConfigSource::DELTA_GRPC: { API_NO_BOOST(envoy::api::v2::DeltaDiscoveryRequest) delta_discovery_request; VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, delta_discovery_request)); - EXPECT_TRUE(!Config::VersionUtil::hasHiddenEnvoyDeprecated(delta_discovery_request)); + EXPECT_TRUE(!hasHiddenEnvoyDeprecated(delta_discovery_request)); xds_stream_->startGrpcStream(); actual_type_url = delta_discovery_request.type_url(); expected_endpoint = expected_v2_delta_endpoint; @@ -154,7 +164,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, case envoy::config::core::v3alpha::ApiConfigSource::GRPC: { API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request; VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, discovery_request)); - EXPECT_TRUE(!Config::VersionUtil::hasHiddenEnvoyDeprecated(discovery_request)); + EXPECT_TRUE(!hasHiddenEnvoyDeprecated(discovery_request)); actual_type_url = discovery_request.type_url(); expected_endpoint = ads() ? ads_v3alpha_delta_endpoint : expected_v3alpha_sotw_endpoint; break; @@ -163,7 +173,7 @@ class ApiVersionIntegrationTest : public testing::TestWithParam, API_NO_BOOST(envoy::service::discovery::v3alpha::DeltaDiscoveryRequest) delta_discovery_request; VERIFY_ASSERTION(xds_stream_->waitForGrpcMessage(*dispatcher_, delta_discovery_request)); - EXPECT_TRUE(!Config::VersionUtil::hasHiddenEnvoyDeprecated(delta_discovery_request)); + EXPECT_TRUE(!hasHiddenEnvoyDeprecated(delta_discovery_request)); actual_type_url = delta_discovery_request.type_url(); expected_endpoint = expected_v3alpha_delta_endpoint; break; diff --git a/test/integration/integration_admin_test.cc b/test/integration/integration_admin_test.cc index 6a7c548344ff1..b343b5836599e 100644 --- a/test/integration/integration_admin_test.cc +++ b/test/integration/integration_admin_test.cc @@ -6,9 +6,11 @@ #include "envoy/config/bootstrap/v3alpha/bootstrap.pb.h" #include "envoy/config/core/v3alpha/base.pb.h" #include "envoy/config/metrics/v3alpha/stats.pb.h" +#include "envoy/config/route/v3alpha/route.pb.h" #include "envoy/http/header_map.h" #include "common/common/fmt.h" +#include "common/config/api_version.h" #include "common/profiler/profiler.h" #include "common/stats/stats_matcher_impl.h" @@ -363,7 +365,9 @@ TEST_P(IntegrationAdminTest, Admin) { // .. and that we can unpack one of the entries. envoy::admin::v3alpha::RoutesConfigDump route_config_dump; config_dump.configs(4).UnpackTo(&route_config_dump); - EXPECT_EQ("route_config_0", route_config_dump.static_route_configs(0).route_config().name()); + envoy::config::route::v3alpha::RouteConfiguration route_config; + EXPECT_TRUE(route_config_dump.static_route_configs(0).route_config().UnpackTo(&route_config)); + EXPECT_EQ("route_config_0", route_config.name()); envoy::admin::v3alpha::SecretsConfigDump secret_config_dump; config_dump.configs(5).UnpackTo(&secret_config_dump); diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index f2921f77fe9b5..6cee245017c87 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -947,8 +947,9 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByResource) { auto dyn_listener = msg->add_dynamic_listeners(); dyn_listener->set_name("foo"); auto stat_listener = msg->add_static_listeners(); - auto listener = stat_listener->mutable_listener(); - listener->set_name("bar"); + envoy::config::listener::v3alpha::Listener listener; + listener.set_name("bar"); + stat_listener->mutable_listener()->PackFrom(listener); return msg; }); const std::string expected_json = R"EOF({ @@ -977,8 +978,9 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByMask) { auto dyn_listener = msg->add_dynamic_listeners(); dyn_listener->set_name("foo"); auto stat_listener = msg->add_static_listeners(); - auto listener = stat_listener->mutable_listener(); - listener->set_name("bar"); + envoy::config::listener::v3alpha::Listener listener; + listener.set_name("bar"); + stat_listener->mutable_listener()->PackFrom(listener); return msg; }); const std::string expected_json = R"EOF({ @@ -1003,13 +1005,15 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByMask) { ProtobufTypes::MessagePtr testDumpClustersConfig() { auto msg = std::make_unique(); auto static_cluster = msg->add_static_clusters(); - auto inner_cluster = static_cluster->mutable_cluster(); - inner_cluster->set_name("foo"); - inner_cluster->set_ignore_health_on_host_removal(true); + envoy::config::cluster::v3alpha::Cluster inner_cluster; + inner_cluster.set_name("foo"); + inner_cluster.set_ignore_health_on_host_removal(true); + static_cluster->mutable_cluster()->PackFrom(inner_cluster); auto dyn_cluster = msg->add_dynamic_active_clusters(); - auto inner_dyn_cluster = dyn_cluster->mutable_cluster(); - inner_dyn_cluster->set_name("bar"); + envoy::config::cluster::v3alpha::Cluster inner_dyn_cluster; + inner_dyn_cluster.set_name("bar"); + dyn_cluster->mutable_cluster()->PackFrom(inner_dyn_cluster); return msg; } @@ -1024,6 +1028,7 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByResourceAndMask) { { "@type": "type.googleapis.com/envoy.admin.v3alpha.ClustersConfigDump.StaticCluster", "cluster": { + "@type": "type.googleapis.com/envoy.config.cluster.v3alpha.Cluster", "name": "foo" } } diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index d155c67570478..c899cce9ca727 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -627,6 +627,7 @@ name: foo checkConfigDump(R"EOF( static_listeners: listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: "foo" address: socket_address: @@ -701,6 +702,7 @@ version_info: version1 warming_state: version_info: version1 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: @@ -743,6 +745,7 @@ version_info: version2 warming_state: version_info: version2 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: @@ -787,6 +790,7 @@ version_info: version3 active_state: version_info: version3 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: @@ -799,6 +803,7 @@ version_info: version3 draining_state: version_info: version2 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: @@ -866,6 +871,7 @@ version_info: version5 active_state: version_info: version3 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: @@ -879,6 +885,7 @@ version_info: version5 active_state: version_info: version4 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: bar address: socket_address: @@ -892,6 +899,7 @@ version_info: version5 warming_state: version_info: version5 listener: + "@type": type.googleapis.com/envoy.api.v2.Listener name: baz address: socket_address: @@ -1172,7 +1180,7 @@ name: foo - name: foo error_state: failed_configuration: - "@type": type.googleapis.com/envoy.config.listener.v3alpha.Listener + "@type": type.googleapis.com/envoy.api.v2.Listener name: foo address: socket_address: diff --git a/test/server/utility.h b/test/server/utility.h index c3b7956ee3f3c..626b211ba3e3b 100644 --- a/test/server/utility.h +++ b/test/server/utility.h @@ -14,7 +14,7 @@ namespace { inline envoy::config::listener::v3alpha::Listener parseListenerFromV2Yaml(const std::string& yaml) { envoy::config::listener::v3alpha::Listener listener; - TestUtility::loadFromYaml(yaml, listener); + TestUtility::loadFromYaml(yaml, listener, true); return listener; } diff --git a/test/test_common/BUILD b/test/test_common/BUILD index 8c87e2b91e33a..f9ebd62946a8b 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -105,6 +105,7 @@ envoy_cc_test_library( "//source/common/common:thread_lib", "//source/common/common:utility_lib", "//source/common/config:resources_lib", + "//source/common/config:version_converter_lib", "//source/common/filesystem:directory_lib", "//source/common/filesystem:filesystem_lib", "//source/common/http:header_map_lib", diff --git a/test/test_common/utility.h b/test/test_common/utility.h index b7f2087e85810..2483603de41c8 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -18,6 +18,7 @@ #include "common/buffer/buffer_impl.h" #include "common/common/c_smart_ptr.h" #include "common/common/thread.h" +#include "common/config/version_converter.h" #include "common/http/header_map_impl.h" #include "common/protobuf/message_validator_impl.h" #include "common/protobuf/utility.h" @@ -510,20 +511,32 @@ class TestUtility { const std::vector>& gauges); // Strict variants of Protobuf::MessageUtil - static void loadFromJson(const std::string& json, Protobuf::Message& message) { + static void loadFromJson(const std::string& json, Protobuf::Message& message, + bool preserve_original_type = false) { MessageUtil::loadFromJson(json, message, ProtobufMessage::getStrictValidationVisitor()); + if (!preserve_original_type) { + Config::VersionConverter::eraseOriginalTypeInformation(message); + } } static void loadFromJson(const std::string& json, ProtobufWkt::Struct& message) { MessageUtil::loadFromJson(json, message); } - static void loadFromYaml(const std::string& yaml, Protobuf::Message& message) { + static void loadFromYaml(const std::string& yaml, Protobuf::Message& message, + bool preserve_original_type = false) { MessageUtil::loadFromYaml(yaml, message, ProtobufMessage::getStrictValidationVisitor()); + if (!preserve_original_type) { + Config::VersionConverter::eraseOriginalTypeInformation(message); + } } - static void loadFromFile(const std::string& path, Protobuf::Message& message, Api::Api& api) { + static void loadFromFile(const std::string& path, Protobuf::Message& message, Api::Api& api, + bool preserve_original_type = false) { MessageUtil::loadFromFile(path, message, ProtobufMessage::getStrictValidationVisitor(), api); + if (!preserve_original_type) { + Config::VersionConverter::eraseOriginalTypeInformation(message); + } } template @@ -535,6 +548,7 @@ class TestUtility { static void loadFromYamlAndValidate(const std::string& yaml, MessageType& message) { MessageUtil::loadFromYamlAndValidate(yaml, message, ProtobufMessage::getStrictValidationVisitor()); + Config::VersionConverter::eraseOriginalTypeInformation(message); } template static void validate(const MessageType& message) { From 53cee753431954d27bf9453fdf2d904b0547091f Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 12 Jan 2020 17:54:11 -0500 Subject: [PATCH 02/10] MessageUtil::checkForUnexpectedFields tests, fix bazel.gcc. Signed-off-by: Harvey Tuch --- source/common/config/version_converter.cc | 4 +- test/common/protobuf/utility_test.cc | 48 ++++++++++++++--------- test/proto/deprecated.proto | 29 ++++++++++++++ 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index ae06561ceb491..184b639eabc6c 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -205,8 +205,8 @@ VersionConverter::reinterpret(const Protobuf::Message& message, void VersionUtil::scrubHiddenEnvoyDeprecated(Protobuf::Message& message) { class HiddenFieldScrubbingProtoVisitor : public ProtoVisitor { public: - void* onField(Protobuf::Message& message, const Protobuf::FieldDescriptor& field, - const void*) override { + const void* onField(Protobuf::Message& message, const Protobuf::FieldDescriptor& field, + const void*) override { const Protobuf::Reflection* reflection = message.GetReflection(); if (absl::StartsWith(field.name(), DeprecatedFieldShadowPrefix)) { reflection->ClearField(&message, &field); diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index e656fec146c03..7546efd5c0b45 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -702,10 +702,10 @@ TEST(DurationUtilTest, OutOfRange) { } } -class DeprecatedFieldsTest : public testing::Test { +class DeprecatedFieldsTest : public testing::TestWithParam { protected: DeprecatedFieldsTest() - : api_(Api::createApiForTest(store_)), + : with_upgrade_(GetParam()), api_(Api::createApiForTest(store_)), runtime_deprecated_feature_use_(store_.counter("runtime.deprecated_feature_use")) { envoy::config::bootstrap::v3alpha::LayeredRuntime config; config.add_layers()->mutable_admin_layer(); @@ -714,6 +714,18 @@ class DeprecatedFieldsTest : public testing::Test { generator_, validation_visitor_, *api_)}); } + void checkForDeprecation(const Protobuf::Message& message) { + if (with_upgrade_) { + envoy::test::deprecation_test::UpgradedBase upgraded_message; + Config::VersionConverter::upgrade(message, upgraded_message); + MessageUtil::checkForUnexpectedFields(upgraded_message, + ProtobufMessage::getStrictValidationVisitor()); + } else { + MessageUtil::checkForUnexpectedFields(message, ProtobufMessage::getStrictValidationVisitor()); + } + } + + const bool with_upgrade_; Event::MockDispatcher dispatcher_; NiceMock tls_; Stats::IsolatedStoreImpl store_; @@ -727,11 +739,9 @@ class DeprecatedFieldsTest : public testing::Test { NiceMock validation_visitor_; }; -void checkForDeprecation(const Protobuf::Message& message) { - MessageUtil::checkForUnexpectedFields(message, ProtobufMessage::getStrictValidationVisitor()); -} +INSTANTIATE_TEST_SUITE_P(Versions, DeprecatedFieldsTest, testing::Values(false, true)); -TEST_F(DeprecatedFieldsTest, NoCrashIfRuntimeMissing) { +TEST_P(DeprecatedFieldsTest, NoCrashIfRuntimeMissing) { loader_.reset(); envoy::test::deprecation_test::Base base; @@ -740,7 +750,7 @@ TEST_F(DeprecatedFieldsTest, NoCrashIfRuntimeMissing) { checkForDeprecation(base); } -TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { +TEST_P(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { envoy::test::deprecation_test::Base base; base.set_not_deprecated("foo"); // Fatal checks for a non-deprecated field should cause no problem. @@ -748,7 +758,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); } -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDeprecated)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); // Non-fatal checks for a deprecated field should log rather than throw an exception. @@ -759,7 +769,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDeprecated)) } // Use of a deprecated and disallowed field should result in an exception. -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); EXPECT_THROW_WITH_REGEX( @@ -767,7 +777,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); } -TEST_F(DeprecatedFieldsTest, +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowedWithRuntimeOverride)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); @@ -790,7 +800,7 @@ TEST_F(DeprecatedFieldsTest, EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); @@ -812,7 +822,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { // Note that given how Envoy config parsing works, the first time we hit a // 'fatal' error and throw, we won't log future warnings. That said, this tests // the case of the warning occurring before the fatal error. -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); base.set_is_deprecated_fatal("foo"); @@ -825,7 +835,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { } // Present (unused) deprecated messages should be detected as deprecated. -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) { envoy::test::deprecation_test::Base base; base.mutable_deprecated_message(); EXPECT_LOG_CONTAINS( @@ -834,7 +844,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) { EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(InnerMessageDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(InnerMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); // Checks for a non-deprecated field shouldn't trigger warnings @@ -849,7 +859,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(InnerMessageDeprecated)) { } // Check that repeated sub-messages get validated. -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.add_repeated_message(); base.add_repeated_message()->set_inner_deprecated("foo"); @@ -863,7 +873,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) { } // Check that deprecated repeated messages trigger -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.add_deprecated_repeated_message(); @@ -875,7 +885,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) } // Check that deprecated enum values trigger for default values -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecatedDefault)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecatedDefault)) { envoy::test::deprecation_test::Base base; base.mutable_enum_container(); @@ -889,7 +899,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecatedDefault } // Check that deprecated enum values trigger for non-default values -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecated)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecated)) { envoy::test::deprecation_test::Base base; base.mutable_enum_container()->set_deprecated_enum( envoy::test::deprecation_test::Base::DEPRECATED_NOT_DEFAULT); @@ -904,7 +914,7 @@ TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(EnumValuesDeprecated)) { // Make sure the runtime overrides for protos work, by checking the non-fatal to // fatal option. -TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RuntimeOverrideEnumDefault)) { +TEST_P(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RuntimeOverrideEnumDefault)) { envoy::test::deprecation_test::Base base; base.mutable_enum_container(); diff --git a/test/proto/deprecated.proto b/test/proto/deprecated.proto index 444f85b56c1a6..ddc3849890a0d 100644 --- a/test/proto/deprecated.proto +++ b/test/proto/deprecated.proto @@ -28,3 +28,32 @@ message Base { } InnerMessageWithDeprecationEnum enum_container = 8; } + +// Same structure as base but with protoxform migration style transforms applied +// manually. +message UpgradedBase { + string not_deprecated = 1; + string hidden_envoy_deprecated_is_deprecated = 2 [deprecated = true]; + string hidden_envoy_deprecated_is_deprecated_fatal = 3 [deprecated = true]; + message InnerMessage { + string inner_not_deprecated = 1; + string hidden_envoy_deprecated_inner_deprecated = 2 [deprecated = true]; + string hidden_envoy_deprecated_inner_deprecated_fatal = 3 [deprecated = true]; + } + InnerMessage hidden_envoy_deprecated_deprecated_message = 4 [deprecated = true]; + InnerMessage not_deprecated_message = 5; + repeated InnerMessage repeated_message = 6; + repeated InnerMessage hidden_envoy_deprecated_deprecated_repeated_message = 7 [deprecated = true]; + + // For deprecated enum value testing, stick the enum in a container, to avoid + // the default instantiation of Base having a deprecated-by-default value. + enum DeprecationEnum { + hidden_envoy_deprecated_DEPRECATED_DEFAULT = 0 [deprecated = true]; + NOT_DEPRECATED = 1; + hidden_envoy_deprecated_DEPRECATED_NOT_DEFAULT = 2 [deprecated = true]; + } + message InnerMessageWithDeprecationEnum { + DeprecationEnum deprecated_enum = 1; + } + InnerMessageWithDeprecationEnum enum_container = 8; +} From 2ef8b5ab2de9330574be0a2b5a0f385ba8cb9358 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 12 Jan 2020 18:36:02 -0500 Subject: [PATCH 03/10] Some admin_test enhancements for logical coverage of field mask Any. Signed-off-by: Harvey Tuch --- test/server/http/admin_test.cc | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index 6cee245017c87..507cb211ef306 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -1004,15 +1004,19 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByMask) { ProtobufTypes::MessagePtr testDumpClustersConfig() { auto msg = std::make_unique(); - auto static_cluster = msg->add_static_clusters(); + auto* static_cluster = msg->add_static_clusters(); envoy::config::cluster::v3alpha::Cluster inner_cluster; inner_cluster.set_name("foo"); inner_cluster.set_ignore_health_on_host_removal(true); static_cluster->mutable_cluster()->PackFrom(inner_cluster); - auto dyn_cluster = msg->add_dynamic_active_clusters(); + auto* dyn_cluster = msg->add_dynamic_active_clusters(); + dyn_cluster->set_version_info("baz"); + dyn_cluster->mutable_last_updated()->set_seconds(5); envoy::config::cluster::v3alpha::Cluster inner_dyn_cluster; inner_dyn_cluster.set_name("bar"); + inner_dyn_cluster.set_ignore_health_on_host_removal(true); + inner_dyn_cluster.mutable_http2_protocol_options()->set_allow_connect(true); dyn_cluster->mutable_cluster()->PackFrom(inner_dyn_cluster); return msg; } @@ -1026,16 +1030,21 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByResourceAndMask) { const std::string expected_json = R"EOF({ "configs": [ { - "@type": "type.googleapis.com/envoy.admin.v3alpha.ClustersConfigDump.StaticCluster", + "@type": "type.googleapis.com/envoy.admin.v3alpha.ClustersConfigDump.DynamicCluster", + "version_info": "baz", "cluster": { "@type": "type.googleapis.com/envoy.config.cluster.v3alpha.Cluster", - "name": "foo" + "name": "bar", + "http2_protocol_options": { + "allow_connect": true + } } } ] } )EOF"; - EXPECT_EQ(Http::Code::OK, getCallback("/config_dump?resource=static_clusters&mask=cluster.name", + EXPECT_EQ(Http::Code::OK, getCallback("/config_dump?resource=dynamic_active_clusters&mask=" + "cluster.name,version_info,cluster.http2_protocol_options", header_map, response)); std::string output = response.toString(); EXPECT_EQ(expected_json, output); From 7210a0231a332a238ef43d92fb8db79296c39389 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 12 Jan 2020 19:05:47 -0500 Subject: [PATCH 04/10] Adjust cluster memory upper boundd for bazel.compile_time_options. Signed-off-by: Harvey Tuch --- test/integration/stats_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 192e74d093f7d..b6e4b540c8f0c 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -285,7 +285,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. EXPECT_MEMORY_EQ(m_per_cluster, 43637); // 104 bytes higher than a debug build. - EXPECT_MEMORY_LE(m_per_cluster, 44000); + EXPECT_MEMORY_LE(m_per_cluster, 44104); } TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { From 0e7686918a7908ea41b6700b24b264c00631d72d Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 12 Jan 2020 22:26:33 -0500 Subject: [PATCH 05/10] Adjust cluster memory bounds in stats_integration_test for new overhead. Signed-off-by: Harvey Tuch --- test/integration/stats_integration_test.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index b6e4b540c8f0c..0aaafed790a62 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -271,6 +271,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/01/07 9564 43445 44000 use RefcountPtr for CentralCache. // 2020/01/09 8889 43509 44000 api: add UpstreamHttpProtocolOptions message // 2020/01/09 9227 43637 44000 router: per-cluster histograms w/ timeout budget + // 2020/01/12 9633 43797 44104 config: support recovery of original message when + // upgrading. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -284,7 +286,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 43637); // 104 bytes higher than a debug build. + EXPECT_MEMORY_EQ(m_per_cluster, 43797); // 104 bytes higher than a debug build. EXPECT_MEMORY_LE(m_per_cluster, 44104); } @@ -323,6 +325,8 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/01/07 9564 35580 36000 RefcountPtr for CentralCache. // 2020/01/09 8889 35644 36000 api: add UpstreamHttpProtocolOptions message // 2019/01/09 9227 35772 36500 router: per-cluster histograms w/ timeout budget + // 2020/01/12 9633 35932 36500 config: support recovery of original message when + // upgrading. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI // 'release' builds, where we control the platform and tool-chain. So you @@ -336,7 +340,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // If you encounter a failure here, please see // https://github.com/envoyproxy/envoy/blob/master/source/docs/stats.md#stats-memory-tests // for details on how to fix. - EXPECT_MEMORY_EQ(m_per_cluster, 35772); // 104 bytes higher than a debug build. + EXPECT_MEMORY_EQ(m_per_cluster, 35932); // 104 bytes higher than a debug build. EXPECT_MEMORY_LE(m_per_cluster, 36500); } From 1a70e8914f1f424c1aa2f740fc201c8fff1d908a Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Sun, 12 Jan 2020 23:16:52 -0500 Subject: [PATCH 06/10] fix_format Signed-off-by: Harvey Tuch --- test/integration/stats_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/stats_integration_test.cc b/test/integration/stats_integration_test.cc index 0aaafed790a62..28a86a8fc43b3 100644 --- a/test/integration/stats_integration_test.cc +++ b/test/integration/stats_integration_test.cc @@ -271,7 +271,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithFakeSymbolTable) { // 2020/01/07 9564 43445 44000 use RefcountPtr for CentralCache. // 2020/01/09 8889 43509 44000 api: add UpstreamHttpProtocolOptions message // 2020/01/09 9227 43637 44000 router: per-cluster histograms w/ timeout budget - // 2020/01/12 9633 43797 44104 config: support recovery of original message when + // 2020/01/12 9633 43797 44104 config: support recovery of original message when // upgrading. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI @@ -325,7 +325,7 @@ TEST_P(ClusterMemoryTestRunner, MemoryLargeClusterSizeWithRealSymbolTable) { // 2020/01/07 9564 35580 36000 RefcountPtr for CentralCache. // 2020/01/09 8889 35644 36000 api: add UpstreamHttpProtocolOptions message // 2019/01/09 9227 35772 36500 router: per-cluster histograms w/ timeout budget - // 2020/01/12 9633 35932 36500 config: support recovery of original message when + // 2020/01/12 9633 35932 36500 config: support recovery of original message when // upgrading. // Note: when adjusting this value: EXPECT_MEMORY_EQ is active only in CI From 21641845b03d0d5abcb6a137c31ab8d35e66d911 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 13 Jan 2020 16:13:26 -0500 Subject: [PATCH 07/10] Restrucutre how wire content gets sanitized for gRPC and JSON. Signed-off-by: Harvey Tuch --- include/envoy/upstream/locality.h | 13 ++-- source/common/config/grpc_mux_impl.cc | 8 +-- .../common/config/http_subscription_impl.cc | 4 +- source/common/config/new_grpc_mux_impl.cc | 8 +-- source/common/config/version_converter.cc | 43 +++++++---- source/common/config/version_converter.h | 34 +++++---- source/common/local_info/local_info_impl.h | 6 -- source/common/upstream/BUILD | 1 + .../common/upstream/cluster_manager_impl.cc | 1 + .../upstream/health_discovery_service.cc | 14 ++-- .../upstream/health_discovery_service.h | 2 + source/common/upstream/load_stats_reporter.cc | 5 +- source/common/upstream/load_stats_reporter.h | 2 + source/server/server.cc | 6 +- test/common/config/version_converter_test.cc | 72 +++++++++++++++---- test/common/upstream/hds_test.cc | 2 +- .../upstream/load_stats_reporter_test.cc | 2 +- 17 files changed, 148 insertions(+), 75 deletions(-) diff --git a/include/envoy/upstream/locality.h b/include/envoy/upstream/locality.h index c074a942b2529..fb19e0c18d682 100644 --- a/include/envoy/upstream/locality.h +++ b/include/envoy/upstream/locality.h @@ -7,24 +7,29 @@ namespace Envoy { namespace Upstream { -// TODO(htuch): should these be templated in protobuf/utility.h? +// We use a tuple representation for hashing/equality/comparison, since this +// ensures we are not subject to proto nuances like unknown fields (e.g. from +// original type information annotations). +using LocalityTuple = std::tuple; + struct LocalityHash { size_t operator()(const envoy::config::core::v3alpha::Locality& locality) const { - return MessageUtil::hash(locality); + return absl::Hash()({locality.region(), locality.zone(), locality.sub_zone()}); } }; struct LocalityEqualTo { bool operator()(const envoy::config::core::v3alpha::Locality& lhs, const envoy::config::core::v3alpha::Locality& rhs) const { - return Protobuf::util::MessageDifferencer::Equivalent(lhs, rhs); + const LocalityTuple lhs_tuple = LocalityTuple(lhs.region(), lhs.zone(), lhs.sub_zone()); + const LocalityTuple rhs_tuple = LocalityTuple(rhs.region(), rhs.zone(), rhs.sub_zone()); + return lhs_tuple == rhs_tuple; } }; struct LocalityLess { bool operator()(const envoy::config::core::v3alpha::Locality& lhs, const envoy::config::core::v3alpha::Locality& rhs) const { - using LocalityTuple = std::tuple; const LocalityTuple lhs_tuple = LocalityTuple(lhs.region(), lhs.zone(), lhs.sub_zone()); const LocalityTuple rhs_tuple = LocalityTuple(rhs.region(), rhs.zone(), rhs.sub_zone()); return lhs_tuple < rhs_tuple; diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index a27744f4ba196..0a9cd2955a351 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -64,13 +64,7 @@ void GrpcMuxImpl::sendDiscoveryRequest(const std::string& type_url) { if (skip_subsequent_node_ && !first_stream_request_) { request.clear_node(); } - // TODO(htuch): this works as long as there are no new fields in the v3+ - // DiscoveryRequest. When they are added, we need to do a full v2 conversion - // and also discard unknown fields. Tracked at - // https://github.com/envoyproxy/envoy/issues/9619. - if (transport_api_version_ == envoy::config::core::v3alpha::ApiVersion::V3ALPHA) { - VersionUtil::scrubHiddenEnvoyDeprecated(request); - } + VersionConverter::prepareMessageForGrpcWire(request, transport_api_version_); ENVOY_LOG(trace, "Sending DiscoveryRequest for {}: {}", type_url, request.DebugString()); grpc_stream_.sendMessage(request); first_stream_request_ = false; diff --git a/source/common/config/http_subscription_impl.cc b/source/common/config/http_subscription_impl.cc index 34e985ff2a58c..d48665a93f427 100644 --- a/source/common/config/http_subscription_impl.cc +++ b/source/common/config/http_subscription_impl.cc @@ -68,8 +68,8 @@ void HttpSubscriptionImpl::createRequest(Http::Message& request) { stats_.update_attempt_.inc(); request.headers().setReferenceMethod(Http::Headers::get().MethodValues.Post); request.headers().setPath(path_); - request.body() = std::make_unique(MessageUtil::getJsonStringFromMessage( - *VersionConverter::reinterpret(request_, transport_api_version_)->msg_)); + request.body() = std::make_unique( + VersionConverter::getJsonStringFromMessage(request_, transport_api_version_)); request.headers().setReferenceContentType(Http::Headers::get().ContentTypeValues.Json); request.headers().setContentLength(request.body()->length()); } diff --git a/source/common/config/new_grpc_mux_impl.cc b/source/common/config/new_grpc_mux_impl.cc index 0db9523ee2edd..96d10b6121df2 100644 --- a/source/common/config/new_grpc_mux_impl.cc +++ b/source/common/config/new_grpc_mux_impl.cc @@ -192,13 +192,7 @@ void NewGrpcMuxImpl::trySendDiscoveryRequests() { } else { request = sub->second->sub_state_.getNextRequestAckless(); } - // TODO(htuch): this works as long as there are no new fields in the v3+ - // DiscoveryRequest. When they are added, we need to do a full v2 conversion - // and also discard unknown fields. Tracked at - // https://github.com/envoyproxy/envoy/issues/9619. - if (transport_api_version_ == envoy::config::core::v3alpha::ApiVersion::V3ALPHA) { - VersionUtil::scrubHiddenEnvoyDeprecated(request); - } + VersionConverter::prepareMessageForGrpcWire(request, transport_api_version_); grpc_stream_.sendMessage(request); } grpc_stream_.maybeUpdateQueueSizeStat(pausable_ack_queue_.size()); diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index 184b639eabc6c..2979fcb175b7f 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -174,9 +174,10 @@ DynamicMessagePtr VersionConverter::downgrade(const Protobuf::Message& message) return createForDescriptorWithCast(message, prev_desc); } -DynamicMessagePtr -VersionConverter::reinterpret(const Protobuf::Message& message, - envoy::config::core::v3alpha::ApiVersion api_version) { +std::string +VersionConverter::getJsonStringFromMessage(const Protobuf::Message& message, + envoy::config::core::v3alpha::ApiVersion api_version) { + DynamicMessagePtr dynamic_message; switch (api_version) { case envoy::config::core::v3alpha::ApiVersion::AUTO: case envoy::config::core::v3alpha::ApiVersion::V2: { @@ -184,22 +185,40 @@ VersionConverter::reinterpret(const Protobuf::Message& message, // DiscoveryRequest. When they are added, we need to do a full v2 conversion // and also discard unknown fields. Tracked at // https://github.com/envoyproxy/envoy/issues/9619. - auto dynamic_message = downgrade(message); - eraseOriginalTypeInformation(*dynamic_message->msg_); - return dynamic_message; + dynamic_message = downgrade(message); + break; } case envoy::config::core::v3alpha::ApiVersion::V3ALPHA: { // We need to scrub the hidden fields. - auto non_shadowed_message = std::make_unique(); - non_shadowed_message->msg_.reset(message.New()); - non_shadowed_message->msg_->MergeFrom(message); - VersionUtil::scrubHiddenEnvoyDeprecated(*non_shadowed_message->msg_); - eraseOriginalTypeInformation(*non_shadowed_message->msg_); - return non_shadowed_message; + dynamic_message = std::make_unique(); + dynamic_message->msg_.reset(message.New()); + dynamic_message->msg_->MergeFrom(message); + VersionUtil::scrubHiddenEnvoyDeprecated(*dynamic_message->msg_); + break; } default: NOT_REACHED_GCOVR_EXCL_LINE; } + eraseOriginalTypeInformation(*dynamic_message->msg_); + std::string json; + Protobuf::util::JsonPrintOptions json_options; + json_options.preserve_proto_field_names = true; + const auto status = Protobuf::util::MessageToJsonString(*dynamic_message->msg_, &json, json_options); + // This should always succeed unless something crash-worthy such as out-of-memory. + RELEASE_ASSERT(status.ok(), ""); + return json; +} + +void VersionConverter::prepareMessageForGrpcWire( + Protobuf::Message& message, envoy::config::core::v3alpha::ApiVersion api_version) { + // TODO(htuch): this works as long as there are no new fields in the v3+ + // DiscoveryRequest. When they are added, we need to do a full v2 conversion + // and also discard unknown fields. Tracked at + // https://github.com/envoyproxy/envoy/issues/9619. + if (api_version == envoy::config::core::v3alpha::ApiVersion::V3ALPHA) { + VersionUtil::scrubHiddenEnvoyDeprecated(message); + } + eraseOriginalTypeInformation(message); } void VersionUtil::scrubHiddenEnvoyDeprecated(Protobuf::Message& message) { diff --git a/source/common/config/version_converter.h b/source/common/config/version_converter.h index e73ec86151da1..8c93cb8d49f96 100644 --- a/source/common/config/version_converter.h +++ b/source/common/config/version_converter.h @@ -57,25 +57,35 @@ class VersionConverter { static DynamicMessagePtr downgrade(const Protobuf::Message& message); /** - * Reinterpret an Envoy internal API message at v3 based on a given transport API - * version. This will downgrade() to an earlier version or scrub the shadow - * deprecated fields in the existing one. + * Obtain JSON wire representation for an Envoy internal API message at v3 + * based on a given transport API version. This will downgrade() to an earlier + * version or scrub the shadow deprecated fields in the existing one. * - * This is typically used when Envoy is generating a wire message from some - * internally generated message, e.g. DiscoveryRequest, and we want to ensure - * it matches a specific API version. For example, a v3 DiscoveryRequest must - * have any deprecated v2 fields removed (they only exist because of - * shadowing) and a v2 DiscoveryRequest needs to have type + * This is typically used when Envoy is generating a JSON wire message from + * some internally generated message, e.g. DiscoveryRequest, and we want to + * ensure it matches a specific API version. For example, a v3 + * DiscoveryRequest must have any deprecated v2 fields removed (they only + * exist because of shadowing) and a v2 DiscoveryRequest needs to have type * envoy.api.v2.DiscoveryRequest to ensure JSON representations have the * correct field names (after renames/deprecations are reversed). * * @param message message input. * @param api_version target API version. - * @return DynamicMessagePtr with the reinterpreted message (and associated - * factory state). + * @return std::string JSON representation. + */ + static std::string getJsonStringFromMessage(const Protobuf::Message& message, + envoy::config::core::v3alpha::ApiVersion api_version); + + /** + * Modify a v3 message to make it suitable for sending as a gRPC message. This + * requires that a v3 message has hidden_envoy_deprecated_* fields removed, + * and that for all versions that original type information is removed. + * + * @param message message to modify. + * @param api_version target API version. */ - static DynamicMessagePtr reinterpret(const Protobuf::Message& message, - envoy::config::core::v3alpha::ApiVersion api_version); + static void prepareMessageForGrpcWire(Protobuf::Message& message, + envoy::config::core::v3alpha::ApiVersion api_version); /** * For a message that may have been upgraded, recover the original message. diff --git a/source/common/local_info/local_info_impl.h b/source/common/local_info/local_info_impl.h index ad1e7225bfbf1..07483a12216a7 100644 --- a/source/common/local_info/local_info_impl.h +++ b/source/common/local_info/local_info_impl.h @@ -17,12 +17,6 @@ class LocalInfoImpl : public LocalInfo { absl::string_view zone_name, absl::string_view cluster_name, absl::string_view node_name) : node_(node), address_(address) { - // Whenever node is used to compare with wire contents, we don't want any - // original type information information. Also, node is likely never - // appearing in config dump or places where we need to recover the exact - // original. TODO(htuch): If we do need this at some point, add a - // nodeForDisplay() method. - Config::VersionConverter::eraseOriginalTypeInformation(node_); if (!zone_name.empty()) { node_.mutable_locality()->set_zone(std::string(zone_name)); } diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 61f5eb9a9489a..5696c846b4a6b 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -211,6 +211,7 @@ envoy_cc_library( "//source/common/common:backoff_lib", "//source/common/common:minimal_logger_lib", "//source/common/config:utility_lib", + "//source/common/config:version_converter_lib", "//source/common/grpc:async_client_lib", "//source/common/network:resolver_lib", "//source/extensions/transport_sockets:well_known_names", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 34039b6269354..c8dce8c8ce482 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -350,6 +350,7 @@ ClusterManagerImpl::ClusterManagerImpl( Config::Utility::factoryForGrpcApiConfigSource( *async_client_manager_, load_stats_config, stats) ->create(), + load_stats_config.transport_api_version(), main_thread_dispatcher); } } diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index 35853b4599239..ef6656ad77dc2 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -8,6 +8,7 @@ #include "envoy/service/health/v3alpha/hds.pb.h" #include "envoy/stats/scope.h" +#include "common/config/version_converter.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -24,6 +25,7 @@ static constexpr uint32_t RetryInitialDelayMilliseconds = 1000; static constexpr uint32_t RetryMaxDelayMilliseconds = 30000; HdsDelegate::HdsDelegate(Stats::Scope& scope, Grpc::RawAsyncClientPtr async_client, + envoy::config::core::v3alpha::ApiVersion transport_api_version, Event::Dispatcher& dispatcher, Runtime::Loader& runtime, Envoy::Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, Runtime::RandomGenerator& random, ClusterInfoFactory& info_factory, @@ -34,11 +36,12 @@ HdsDelegate::HdsDelegate(Stats::Scope& scope, Grpc::RawAsyncClientPtr async_clie : stats_{ALL_HDS_STATS(POOL_COUNTER_PREFIX(scope, "hds_delegate."))}, service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.HealthDiscoveryService.StreamHealthCheck")), - async_client_(std::move(async_client)), dispatcher_(dispatcher), runtime_(runtime), - store_stats_(stats), ssl_context_manager_(ssl_context_manager), random_(random), - info_factory_(info_factory), access_log_manager_(access_log_manager), cm_(cm), - local_info_(local_info), admin_(admin), singleton_manager_(singleton_manager), tls_(tls), - validation_visitor_(validation_visitor), api_(api) { + async_client_(std::move(async_client)), transport_api_version_(transport_api_version), + dispatcher_(dispatcher), runtime_(runtime), store_stats_(stats), + ssl_context_manager_(ssl_context_manager), random_(random), info_factory_(info_factory), + access_log_manager_(access_log_manager), cm_(cm), local_info_(local_info), admin_(admin), + singleton_manager_(singleton_manager), tls_(tls), validation_visitor_(validation_visitor), + api_(api) { health_check_request_.mutable_health_check_request()->mutable_node()->MergeFrom( local_info_.node()); backoff_strategy_ = std::make_unique(RetryInitialDelayMilliseconds, @@ -77,6 +80,7 @@ void HdsDelegate::establishNewStream() { return; } + Config::VersionConverter::prepareMessageForGrpcWire(health_check_request_, transport_api_version_); ENVOY_LOG(debug, "Sending HealthCheckRequest {} ", health_check_request_.DebugString()); stream_->sendMessage(health_check_request_, false); stats_.responses_.inc(); diff --git a/source/common/upstream/health_discovery_service.h b/source/common/upstream/health_discovery_service.h index 4988e3b51d11c..2e8a8d79c346d 100644 --- a/source/common/upstream/health_discovery_service.h +++ b/source/common/upstream/health_discovery_service.h @@ -117,6 +117,7 @@ class HdsDelegate Logger::Loggable { public: HdsDelegate(Stats::Scope& scope, Grpc::RawAsyncClientPtr async_client, + envoy::config::core::v3alpha::ApiVersion transport_api_version, Event::Dispatcher& dispatcher, Runtime::Loader& runtime, Envoy::Stats::Store& stats, Ssl::ContextManager& ssl_context_manager, Runtime::RandomGenerator& random, ClusterInfoFactory& info_factory, AccessLog::AccessLogManager& access_log_manager, @@ -152,6 +153,7 @@ class HdsDelegate Grpc::AsyncClient async_client_; + const envoy::config::core::v3alpha::ApiVersion transport_api_version_; Grpc::AsyncStream stream_{}; Event::Dispatcher& dispatcher_; diff --git a/source/common/upstream/load_stats_reporter.cc b/source/common/upstream/load_stats_reporter.cc index 3132dc14c5a35..10fcbdad9f066 100644 --- a/source/common/upstream/load_stats_reporter.cc +++ b/source/common/upstream/load_stats_reporter.cc @@ -3,6 +3,7 @@ #include "envoy/service/load_stats/v3alpha/lrs.pb.h" #include "envoy/stats/scope.h" +#include "common/config/version_converter.h" #include "common/protobuf/protobuf.h" namespace Envoy { @@ -11,10 +12,11 @@ namespace Upstream { LoadStatsReporter::LoadStatsReporter(const LocalInfo::LocalInfo& local_info, ClusterManager& cluster_manager, Stats::Scope& scope, Grpc::RawAsyncClientPtr async_client, + envoy::config::core::v3alpha::ApiVersion transport_api_version, Event::Dispatcher& dispatcher) : cm_(cluster_manager), stats_{ALL_LOAD_REPORTER_STATS( POOL_COUNTER_PREFIX(scope, "load_reporter."))}, - async_client_(std::move(async_client)), + async_client_(std::move(async_client)), transport_api_version_(transport_api_version), service_method_(*Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.load_stats.v2.LoadReportingService.StreamLoadStats")), time_source_(dispatcher.timeSource()) { @@ -92,6 +94,7 @@ void LoadStatsReporter::sendLoadStatsRequest() { clusters_[cluster_name] = now; } + Config::VersionConverter::prepareMessageForGrpcWire(request_, transport_api_version_); ENVOY_LOG(trace, "Sending LoadStatsRequest: {}", request_.DebugString()); stream_->sendMessage(request_, false); stats_.responses_.inc(); diff --git a/source/common/upstream/load_stats_reporter.h b/source/common/upstream/load_stats_reporter.h index a3761f72f43df..911143fa68b3a 100644 --- a/source/common/upstream/load_stats_reporter.h +++ b/source/common/upstream/load_stats_reporter.h @@ -31,6 +31,7 @@ class LoadStatsReporter public: LoadStatsReporter(const LocalInfo::LocalInfo& local_info, ClusterManager& cluster_manager, Stats::Scope& scope, Grpc::RawAsyncClientPtr async_client, + envoy::config::core::v3alpha::ApiVersion transport_api_version, Event::Dispatcher& dispatcher); // Grpc::AsyncStreamCallbacks @@ -56,6 +57,7 @@ class LoadStatsReporter Grpc::AsyncClient async_client_; + const envoy::config::core::v3alpha::ApiVersion transport_api_version_; Grpc::AsyncStream stream_{}; const Protobuf::MethodDescriptor& service_method_; Event::TimerPtr retry_timer_; diff --git a/source/server/server.cc b/source/server/server.cc index e715fa7c5f888..27b3a41dae73a 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -418,9 +418,9 @@ void InstanceImpl::initialize(const Options& options, Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, hds_config, stats_store_) ->create(), - *dispatcher_, Runtime::LoaderSingleton::get(), stats_store_, *ssl_context_manager_, - *random_generator_, info_factory_, access_log_manager_, *config_.clusterManager(), - *local_info_, *admin_, *singleton_manager_, thread_local_, + hds_config.transport_api_version(), *dispatcher_, Runtime::LoaderSingleton::get(), + stats_store_, *ssl_context_manager_, *random_generator_, info_factory_, access_log_manager_, + *config_.clusterManager(), *local_info_, *admin_, *singleton_manager_, thread_local_, messageValidationContext().dynamicValidationVisitor(), *api_); } diff --git a/test/common/config/version_converter_test.cc b/test/common/config/version_converter_test.cc index a4f1d3d6a61fa..35de12e06a087 100644 --- a/test/common/config/version_converter_test.cc +++ b/test/common/config/version_converter_test.cc @@ -77,26 +77,70 @@ TEST(VersionConverterTest, ScrubHiddenEnvoyDeprecated) { EXPECT_FALSE(msg.load_balancing_policy().policies(0).has_hidden_envoy_deprecated_config()); } -// Validate that we can sensible reinterpret messages such as DiscoveryRequest -// based on transport API version. -TEST(VersionConverter, Reinterpret) { +// Validate that we can sensibly provide a JSON wire interpretation of messages +// such as DiscoveryRequest based on transport API version. +TEST(VersionConverter, GetJsonStringFromMessage) { API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request; discovery_request.mutable_node()->set_hidden_envoy_deprecated_build_version("foo"); discovery_request.mutable_node()->set_user_agent_name("bar"); - auto v2_discovery_request = VersionConverter::reinterpret( + const std::string v2_discovery_request = VersionConverter::getJsonStringFromMessage( discovery_request, envoy::config::core::v3alpha::ApiVersion::V2); - API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_v2_discovery_request; - expected_v2_discovery_request.mutable_node()->set_build_version("foo"); - expected_v2_discovery_request.mutable_node()->set_user_agent_name("bar"); - EXPECT_THAT(*v2_discovery_request->msg_, ProtoEq(expected_v2_discovery_request)); - auto auto_discovery_request = VersionConverter::reinterpret( + EXPECT_EQ("{\"node\":{\"build_version\":\"foo\",\"user_agent_name\":\"bar\"}}", + v2_discovery_request); + const std::string auto_discovery_request = VersionConverter::getJsonStringFromMessage( discovery_request, envoy::config::core::v3alpha::ApiVersion::AUTO); - EXPECT_THAT(*auto_discovery_request->msg_, ProtoEq(expected_v2_discovery_request)); - auto v3_discovery_request = VersionConverter::reinterpret( + EXPECT_EQ("{\"node\":{\"build_version\":\"foo\",\"user_agent_name\":\"bar\"}}", + auto_discovery_request); + const std::string v3_discovery_request = VersionConverter::getJsonStringFromMessage( discovery_request, envoy::config::core::v3alpha::ApiVersion::V3ALPHA); - API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) expected_v3_discovery_request; - expected_v3_discovery_request.mutable_node()->set_user_agent_name("bar"); - EXPECT_THAT(*v3_discovery_request->msg_, ProtoEq(expected_v3_discovery_request)); + EXPECT_EQ("{\"node\":{\"user_agent_name\":\"bar\"}}", v3_discovery_request); +} + +bool hasUnknownFields(const Protobuf::Message& message) { + const Protobuf::Reflection* reflection = message.GetReflection(); + const auto& unknown_field_set = reflection->GetUnknownFields(message); + return !unknown_field_set.empty(); +} + +// Validate that we can sensibly provide a gRPC wire interpretation of messages +// such as DiscoveryRequest based on transport API version. +TEST(VersionConverter, PrepareMessageForGrpcWire) { + API_NO_BOOST(envoy::api::v2::core::Node) v2_node; + v2_node.set_build_version("foo"); + v2_node.set_user_agent_name("bar"); + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request; + discovery_request.mutable_node()->set_hidden_envoy_deprecated_build_version("foo"); + VersionConverter::upgrade(v2_node, *discovery_request.mutable_node()); + { + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request_copy; + discovery_request_copy.MergeFrom(discovery_request); + VersionConverter::prepareMessageForGrpcWire(discovery_request_copy, + envoy::config::core::v3alpha::ApiVersion::V2); + API_NO_BOOST(envoy::api::v2::DiscoveryRequest) v2_discovery_request; + EXPECT_TRUE(v2_discovery_request.ParseFromString(discovery_request_copy.SerializeAsString())); + EXPECT_EQ("foo", v2_discovery_request.node().build_version()); + EXPECT_FALSE(hasUnknownFields(v2_discovery_request.node())); + } + { + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request_copy; + discovery_request_copy.MergeFrom(discovery_request); + VersionConverter::prepareMessageForGrpcWire(discovery_request_copy, + envoy::config::core::v3alpha::ApiVersion::AUTO); + API_NO_BOOST(envoy::api::v2::DiscoveryRequest) auto_discovery_request; + EXPECT_TRUE(auto_discovery_request.ParseFromString(discovery_request_copy.SerializeAsString())); + EXPECT_EQ("foo", auto_discovery_request.node().build_version()); + EXPECT_FALSE(hasUnknownFields(auto_discovery_request.node())); + } + { + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) discovery_request_copy; + discovery_request_copy.MergeFrom(discovery_request); + VersionConverter::prepareMessageForGrpcWire(discovery_request_copy, + envoy::config::core::v3alpha::ApiVersion::V3ALPHA); + API_NO_BOOST(envoy::service::discovery::v3alpha::DiscoveryRequest) v3_discovery_request; + EXPECT_TRUE(v3_discovery_request.ParseFromString(discovery_request_copy.SerializeAsString())); + EXPECT_EQ("", v3_discovery_request.node().hidden_envoy_deprecated_build_version()); + EXPECT_FALSE(hasUnknownFields(v3_discovery_request.node())); + } } // Downgrading to an earlier version (where it exists). diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc index d73c0bd023bfc..fae177602b656 100644 --- a/test/common/upstream/hds_test.cc +++ b/test/common/upstream/hds_test.cc @@ -69,7 +69,7 @@ class HdsTest : public testing::Test { return server_response_timer_; })); hds_delegate_ = std::make_unique( - stats_store_, Grpc::RawAsyncClientPtr(async_client_), dispatcher_, runtime_, stats_store_, + stats_store_, Grpc::RawAsyncClientPtr(async_client_), envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_, runtime_, stats_store_, ssl_context_manager_, random_, test_factory_, log_manager_, cm_, local_info_, admin_, singleton_manager_, tls_, validation_visitor_, *api_); } diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index 590f5f444e2e3..446db5605a4d0 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -44,7 +44,7 @@ class LoadStatsReporterTest : public testing::Test { return response_timer_; })); load_stats_reporter_ = std::make_unique( - local_info_, cm_, stats_store_, Grpc::RawAsyncClientPtr(async_client_), dispatcher_); + local_info_, cm_, stats_store_, Grpc::RawAsyncClientPtr(async_client_), envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_); } void expectSendMessage( From ad92f92557b066d6b8dd8b2d8b6b74f6d950aace Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 13 Jan 2020 18:01:05 -0500 Subject: [PATCH 08/10] Switch OriginalTypeFieldNumber magic to SHA-based. Signed-off-by: Harvey Tuch --- source/common/protobuf/well_known.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/source/common/protobuf/well_known.h b/source/common/protobuf/well_known.h index cd84a5a1ef258..86905f3b63c0d 100644 --- a/source/common/protobuf/well_known.h +++ b/source/common/protobuf/well_known.h @@ -4,7 +4,9 @@ namespace Envoy { namespace ProtobufWellKnown { // Used by VersionConverter to track the original type of an upgraded message. -constexpr uint32_t OriginalTypeFieldNumber = 100000; +// Magic number in this file derived from top 28bit of SHA256 digest of +// "original type". +constexpr uint32_t OriginalTypeFieldNumber = 183412668; } // namespace ProtobufWellKnown } // namespace Envoy From d7d59bc5d28fca749cdc94e01bc75dbe4002c912 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 13 Jan 2020 18:11:44 -0500 Subject: [PATCH 09/10] FieldMask comment improvements. Signed-off-by: Harvey Tuch --- source/server/http/admin.cc | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/source/server/http/admin.cc b/source/server/http/admin.cc index eb390fac4254f..f5479281843c9 100644 --- a/source/server/http/admin.cc +++ b/source/server/http/admin.cc @@ -229,11 +229,21 @@ void setHealthFlag(Upstream::Host::HealthFlag flag, const Upstream::Host& host, } } -// Apply a field mask to a resource message. These have a single Any field to -// hold the resource, and other fields with items such as name, time stamp, -// etc. We need to provide a way to trim with field masks that makes use of Any -// transparent, since native field masks don't work with Any. We simplify and -// assume at most one Any message in a resource. +// Apply a field mask to a resource message. A simple field mask might look +// like "cluster.name,cluster.alt_stat_name,last_updated" for a StaticCluster +// resource. Unfortunately, since the "cluster" field is Any and the in-built +// FieldMask utils can't mask inside an Any field, we need to do additional work +// below. +// +// We take advantage of the fact that for the most part (with the exception of +// DynamicListener) that ConfigDump resources have a single Any field where the +// embedded resources lives. This allows us to construct an inner field mask for +// the Any resource and an outer field mask for the enclosing message. In the +// above example, the inner field mask would be "name,alt_stat_name" and the +// outer field mask "cluster,last_updated". The masks are applied to their +// respective messages, with the Any resource requiring an unpack/mask/pack +// series of operations. +// // TODO(htuch): we could make field masks more powerful in future and generalize // this to allow arbitrary indexing through Any fields. This is pretty // complicated, we would need to build a FieldMask tree similar to how the C++ @@ -256,7 +266,8 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag // Only a single Any field supported, repeated fields don't support further // indexing. // TODO(htuch): should add support for DynamicListener for multiple Any - // fields in the future. + // fields in the future, see + // https://github.com/envoyproxy/envoy/issues/9669. if (field != nullptr && field->message_type() != nullptr && !field->is_repeated() && field->message_type()->full_name() == "google.protobuf.Any") { if (any_field_name.empty()) { From 75d1d9eb8e80b49304b0c3f4c9a16f8326cf11af Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Mon, 13 Jan 2020 18:18:35 -0500 Subject: [PATCH 10/10] fix_format Signed-off-by: Harvey Tuch --- source/common/config/version_converter.cc | 3 ++- source/common/upstream/cluster_manager_impl.cc | 13 ++++++------- source/common/upstream/health_discovery_service.cc | 3 ++- test/common/upstream/hds_test.cc | 3 ++- test/common/upstream/load_stats_reporter_test.cc | 3 ++- tools/check_format.py | 1 + 6 files changed, 15 insertions(+), 11 deletions(-) diff --git a/source/common/config/version_converter.cc b/source/common/config/version_converter.cc index 2979fcb175b7f..787ab1084bf69 100644 --- a/source/common/config/version_converter.cc +++ b/source/common/config/version_converter.cc @@ -203,7 +203,8 @@ VersionConverter::getJsonStringFromMessage(const Protobuf::Message& message, std::string json; Protobuf::util::JsonPrintOptions json_options; json_options.preserve_proto_field_names = true; - const auto status = Protobuf::util::MessageToJsonString(*dynamic_message->msg_, &json, json_options); + const auto status = + Protobuf::util::MessageToJsonString(*dynamic_message->msg_, &json, json_options); // This should always succeed unless something crash-worthy such as out-of-memory. RELEASE_ASSERT(status.ok(), ""); return json; diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index c8dce8c8ce482..8a60af10ab79a 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -345,13 +345,12 @@ ClusterManagerImpl::ClusterManagerImpl( if (cm_config.has_load_stats_config()) { const auto& load_stats_config = cm_config.load_stats_config(); - load_stats_reporter_ = - std::make_unique(local_info, *this, stats, - Config::Utility::factoryForGrpcApiConfigSource( - *async_client_manager_, load_stats_config, stats) - ->create(), - load_stats_config.transport_api_version(), - main_thread_dispatcher); + load_stats_reporter_ = std::make_unique( + local_info, *this, stats, + Config::Utility::factoryForGrpcApiConfigSource(*async_client_manager_, load_stats_config, + stats) + ->create(), + load_stats_config.transport_api_version(), main_thread_dispatcher); } } diff --git a/source/common/upstream/health_discovery_service.cc b/source/common/upstream/health_discovery_service.cc index ef6656ad77dc2..6a6e7c4abf41a 100644 --- a/source/common/upstream/health_discovery_service.cc +++ b/source/common/upstream/health_discovery_service.cc @@ -80,7 +80,8 @@ void HdsDelegate::establishNewStream() { return; } - Config::VersionConverter::prepareMessageForGrpcWire(health_check_request_, transport_api_version_); + Config::VersionConverter::prepareMessageForGrpcWire(health_check_request_, + transport_api_version_); ENVOY_LOG(debug, "Sending HealthCheckRequest {} ", health_check_request_.DebugString()); stream_->sendMessage(health_check_request_, false); stats_.responses_.inc(); diff --git a/test/common/upstream/hds_test.cc b/test/common/upstream/hds_test.cc index fae177602b656..f8218ec11341d 100644 --- a/test/common/upstream/hds_test.cc +++ b/test/common/upstream/hds_test.cc @@ -69,7 +69,8 @@ class HdsTest : public testing::Test { return server_response_timer_; })); hds_delegate_ = std::make_unique( - stats_store_, Grpc::RawAsyncClientPtr(async_client_), envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_, runtime_, stats_store_, + stats_store_, Grpc::RawAsyncClientPtr(async_client_), + envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_, runtime_, stats_store_, ssl_context_manager_, random_, test_factory_, log_manager_, cm_, local_info_, admin_, singleton_manager_, tls_, validation_visitor_, *api_); } diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index 446db5605a4d0..de7913c24591f 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -44,7 +44,8 @@ class LoadStatsReporterTest : public testing::Test { return response_timer_; })); load_stats_reporter_ = std::make_unique( - local_info_, cm_, stats_store_, Grpc::RawAsyncClientPtr(async_client_), envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_); + local_info_, cm_, stats_store_, Grpc::RawAsyncClientPtr(async_client_), + envoy::config::core::v3alpha::ApiVersion::AUTO, dispatcher_); } void expectSendMessage( diff --git a/tools/check_format.py b/tools/check_format.py index c774f8ef86d81..41ae732148e84 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -48,6 +48,7 @@ "./source/common/config/version_converter.cc", "./source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc", "./test/common/protobuf/utility_test.cc", + "./test/common/config/version_converter_test.cc", "./test/common/grpc/codec_test.cc", "./test/common/grpc/codec_fuzz_test.cc", )