Skip to content

Commit

Permalink
build: add API_NO_BOOST* annotations. (envoyproxy#9480)
Browse files Browse the repository at this point in the history
These allow specific types, expressions and files to be excluded from
API boosting via annotation. API_NO_BOOST_FILE (anywhere in the file)
will skip boosting of the file, API_NO_BOOST() wrapped around an
expression or type will bypass boosting of that text.

The idea is that there are some types that we do not ever want to
upgrade as they are relevant to the cross-version wire impedance
matching, or testing v2 compat when the tree is v3alpha.

The sites identified in this PR were taken from my WiP in which I have
the entire tree boosted and tests passing. It's possible we might need
to tune some more later, but this should go a reasonable way towards
reducing the amount of manual fixups required after boosting to get
tests passing again.

Risk level: Low (macros are nops)
Testing: bazel test //test/..., new api_booster golden tests.

Part of envoyproxy#8082

Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Prakhar <prakhar_au@yahoo.com>
  • Loading branch information
htuch authored and prakhag1 committed Jan 3, 2020
1 parent 4f6566e commit 660f860
Show file tree
Hide file tree
Showing 39 changed files with 185 additions and 59 deletions.
5 changes: 5 additions & 0 deletions source/common/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "api_version_lib",
hdrs = ["api_version.h"],
)

envoy_cc_library(
name = "config_provider_lib",
srcs = ["config_provider_impl.cc"],
Expand Down
7 changes: 7 additions & 0 deletions source/common/config/api_version.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#pragma once

// Use this to force a specific version of a given config proto, preventing API
// boosting from modifying it. E.g. API_NO_BOOST(envoy::api::v2::Cluster).
#define API_NO_BOOST(x) x

namespace Envoy {}
2 changes: 2 additions & 0 deletions source/common/config/type_to_endpoint.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@

#include "common/grpc/common.h"

// API_NO_BOOST_FILE

namespace Envoy {
namespace Config {

Expand Down
3 changes: 3 additions & 0 deletions source/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ envoy_cc_library(
"//include/envoy/thread_local:thread_local_interface",
"//source/common/common:assert_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/config:api_version_lib",
"//source/common/config:utility_lib",
"//source/common/init:target_lib",
"//source/common/protobuf:utility_lib",
Expand Down Expand Up @@ -160,6 +161,7 @@ envoy_cc_library(
"//source/common/common:callback_impl_lib",
"//source/common/common:cleanup_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/config:api_version_lib",
"//source/common/config:subscription_factory_lib",
"//source/common/config:utility_lib",
"//source/common/init:manager_lib",
Expand Down Expand Up @@ -205,6 +207,7 @@ envoy_cc_library(
"//source/common/common:assert_lib",
"//source/common/common:cleanup_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/config:api_version_lib",
"//source/common/config:config_provider_lib",
"//source/common/init:manager_lib",
"//source/common/init:watcher_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/common/router/rds_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/config/api_version.h"
#include "common/config/utility.h"
#include "common/protobuf/utility.h"
#include "common/router/config_impl.h"
Expand Down Expand Up @@ -73,7 +74,8 @@ RdsRouteConfigSubscription::RdsRouteConfigSubscription(
subscription_ =
factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource(
rds.config_source(),
Grpc::Common::typeUrl(envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name()),
Grpc::Common::typeUrl(
API_NO_BOOST(envoy::api::v2::RouteConfiguration)().GetDescriptor()->full_name()),
*scope_, *this);
config_update_info_ =
std::make_unique<RouteConfigUpdateReceiverImpl>(factory_context.timeSource(), validator);
Expand Down
6 changes: 4 additions & 2 deletions source/common/router/scoped_rds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "common/common/cleanup.h"
#include "common/common/logger.h"
#include "common/common/utility.h"
#include "common/config/api_version.h"
#include "common/config/resources.h"
#include "common/init/manager_impl.h"
#include "common/init/watcher_impl.h"
Expand Down Expand Up @@ -102,8 +103,9 @@ ScopedRdsConfigSubscription::ScopedRdsConfigSubscription(
subscription_ =
factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource(
scoped_rds.scoped_rds_config_source(),
Grpc::Common::typeUrl(
envoy::api::v2::ScopedRouteConfiguration().GetDescriptor()->full_name()),
Grpc::Common::typeUrl(API_NO_BOOST(envoy::api::v2::ScopedRouteConfiguration)()
.GetDescriptor()
->full_name()),
*scope_, *this);

initialize([scope_key_builder]() -> Envoy::Config::ConfigProvider::ConfigConstSharedPtr {
Expand Down
4 changes: 3 additions & 1 deletion source/common/router/vhds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/config/api_version.h"
#include "common/config/utility.h"
#include "common/protobuf/utility.h"
#include "common/router/config_impl.h"
Expand Down Expand Up @@ -42,7 +43,8 @@ VhdsSubscription::VhdsSubscription(RouteConfigUpdatePtr& config_update_info,
subscription_ =
factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource(
config_update_info_->routeConfiguration().vhds().config_source(),
Grpc::Common::typeUrl(envoy::api::v2::route::VirtualHost().GetDescriptor()->full_name()),
Grpc::Common::typeUrl(
API_NO_BOOST(envoy::api::v2::route::VirtualHost)().GetDescriptor()->full_name()),
*scope_, *this);
}

Expand Down
1 change: 1 addition & 0 deletions source/common/runtime/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ envoy_cc_library(
"//source/common/common:minimal_logger_lib",
"//source/common/common:thread_lib",
"//source/common/common:utility_lib",
"//source/common/config:api_version_lib",
"//source/common/filesystem:directory_lib",
"//source/common/grpc:common_lib",
"//source/common/init:target_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/common/runtime/runtime_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/common/utility.h"
#include "common/config/api_version.h"
#include "common/filesystem/directory.h"
#include "common/grpc/common.h"
#include "common/protobuf/message_validator_impl.h"
Expand Down Expand Up @@ -590,7 +591,8 @@ void RtdsSubscription::start() {
// instantiated in the server instance.
subscription_ = parent_.cm_->subscriptionFactory().subscriptionFromConfigSource(
config_source_,
Grpc::Common::typeUrl(envoy::service::discovery::v2::Runtime().GetDescriptor()->full_name()),
Grpc::Common::typeUrl(
API_NO_BOOST(envoy::service::discovery::v2::Runtime)().GetDescriptor()->full_name()),
store_, *this);
subscription_->start({resource_name_});
}
Expand Down
1 change: 1 addition & 0 deletions source/common/secret/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ envoy_cc_library(
"//include/envoy/stats:stats_interface",
"//source/common/common:callback_impl_lib",
"//source/common/common:cleanup_lib",
"//source/common/config:api_version_lib",
"//source/common/config:resources_lib",
"//source/common/config:utility_lib",
"//source/common/init:target_lib",
Expand Down
6 changes: 4 additions & 2 deletions source/common/secret/sds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/api/v2/core/config_source.pb.h"
#include "envoy/api/v2/discovery.pb.h"

#include "common/config/api_version.h"
#include "common/config/resources.h"
#include "common/protobuf/utility.h"

Expand Down Expand Up @@ -81,8 +82,9 @@ void SdsApi::validateUpdateSize(int num_resources) {
void SdsApi::initialize() {
subscription_ = subscription_factory_.subscriptionFromConfigSource(
sds_config_,
Grpc::Common::typeUrl(envoy::api::v2::auth::Secret().GetDescriptor()->full_name()), stats_,
*this);
Grpc::Common::typeUrl(
API_NO_BOOST(envoy::api::v2::auth::Secret)().GetDescriptor()->full_name()),
stats_, *this);
subscription_->start({sds_config_name_});
}

Expand Down
2 changes: 2 additions & 0 deletions source/common/upstream/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ envoy_cc_library(
"//include/envoy/local_info:local_info_interface",
"//source/common/common:cleanup_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/config:api_version_lib",
"//source/common/config:resources_lib",
"//source/common/config:utility_lib",
"//source/common/protobuf:utility_lib",
Expand Down Expand Up @@ -347,6 +348,7 @@ envoy_cc_library(
"//include/envoy/secret:secret_manager_interface",
"//include/envoy/upstream:cluster_factory_interface",
"//include/envoy/upstream:locality_lib",
"//source/common/config:api_version_lib",
"//source/common/config:metadata_lib",
"//source/common/config:subscription_factory_lib",
"//source/common/config:utility_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/common/upstream/cds_api_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "common/common/cleanup.h"
#include "common/common/utility.h"
#include "common/config/api_version.h"
#include "common/config/resources.h"
#include "common/config/utility.h"
#include "common/protobuf/utility.h"
Expand All @@ -30,7 +31,8 @@ CdsApiImpl::CdsApiImpl(const envoy::api::v2::core::ConfigSource& cds_config, Clu
: cm_(cm), scope_(scope.createScope("cluster_manager.cds.")),
validation_visitor_(validation_visitor) {
subscription_ = cm_.subscriptionFactory().subscriptionFromConfigSource(
cds_config, Grpc::Common::typeUrl(envoy::api::v2::Cluster().GetDescriptor()->full_name()),
cds_config,
Grpc::Common::typeUrl(API_NO_BOOST(envoy::api::v2::Cluster)().GetDescriptor()->full_name()),
*scope_, *this);
}

Expand Down
3 changes: 2 additions & 1 deletion source/common/upstream/eds.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/api/v2/eds.pb.validate.h"

#include "common/common/utility.h"
#include "common/config/api_version.h"

namespace Envoy {
namespace Upstream {
Expand Down Expand Up @@ -35,7 +36,7 @@ EdsClusterImpl::EdsClusterImpl(
factory_context.clusterManager().subscriptionFactory().subscriptionFromConfigSource(
eds_config,
Grpc::Common::typeUrl(
envoy::api::v2::ClusterLoadAssignment().GetDescriptor()->full_name()),
API_NO_BOOST(envoy::api::v2::ClusterLoadAssignment)().GetDescriptor()->full_name()),
info_->statsScope(), *this);
}

Expand Down
1 change: 1 addition & 0 deletions source/server/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ envoy_cc_library(
"//include/envoy/init:manager_interface",
"//include/envoy/server:listener_manager_interface",
"//source/common/common:cleanup_lib",
"//source/common/config:api_version_lib",
"//source/common/config:resources_lib",
"//source/common/config:utility_lib",
"//source/common/init:target_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/server/lds_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "envoy/stats/scope.h"

#include "common/common/cleanup.h"
#include "common/config/api_version.h"
#include "common/config/resources.h"
#include "common/config/utility.h"
#include "common/protobuf/utility.h"
Expand All @@ -27,7 +28,8 @@ LdsApiImpl::LdsApiImpl(const envoy::api::v2::core::ConfigSource& lds_config,
init_target_("LDS", [this]() { subscription_->start({}); }),
validation_visitor_(validation_visitor) {
subscription_ = cm.subscriptionFactory().subscriptionFromConfigSource(
lds_config, Grpc::Common::typeUrl(envoy::api::v2::Listener().GetDescriptor()->full_name()),
lds_config,
Grpc::Common::typeUrl(API_NO_BOOST(envoy::api::v2::Listener)().GetDescriptor()->full_name()),
*scope_, *this);
init_manager.add(init_target_);
}
Expand Down
3 changes: 3 additions & 0 deletions test/common/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ envoy_cc_test(
srcs = ["delta_subscription_impl_test.cc"],
deps = [
":delta_subscription_test_harness",
"//source/common/config:api_version_lib",
"//source/common/config:delta_subscription_lib",
"//source/common/stats:isolated_store_lib",
"//test/mocks:common_lib",
Expand Down Expand Up @@ -89,6 +90,7 @@ envoy_cc_test(
name = "grpc_mux_impl_test",
srcs = ["grpc_mux_impl_test.cc"],
deps = [
"//source/common/config:api_version_lib",
"//source/common/config:grpc_mux_lib",
"//source/common/config:protobuf_link_hacks",
"//source/common/config:resources_lib",
Expand Down Expand Up @@ -157,6 +159,7 @@ envoy_cc_test_library(
deps = [
":subscription_test_harness",
"//source/common/common:hash_lib",
"//source/common/config:api_version_lib",
"//source/common/config:grpc_subscription_lib",
"//source/common/config:resources_lib",
"//test/mocks/config:config_mocks",
Expand Down
3 changes: 2 additions & 1 deletion test/common/config/delta_subscription_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include "envoy/api/v2/eds.pb.h"

#include "common/buffer/zero_copy_input_stream_impl.h"
#include "common/config/api_version.h"

#include "test/common/config/delta_subscription_test_harness.h"

Expand Down Expand Up @@ -109,7 +110,7 @@ TEST_F(DeltaSubscriptionImplTest, PauseQueuesAcks) {
// All ACK sendMessage()s will happen upon calling resume().
EXPECT_CALL(async_stream_, sendMessageRaw_(_, _))
.WillRepeatedly(Invoke([this](Buffer::InstancePtr& buffer, bool) {
envoy::api::v2::DeltaDiscoveryRequest message;
API_NO_BOOST(envoy::api::v2::DeltaDiscoveryRequest) message;
EXPECT_TRUE(Grpc::Common::parseBufferInstance(std::move(buffer), message));
const std::string nonce = message.response_nonce();
if (!nonce.empty()) {
Expand Down
3 changes: 2 additions & 1 deletion test/common/config/grpc_mux_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "envoy/api/v2/eds.pb.h"

#include "common/common/empty_string.h"
#include "common/config/api_version.h"
#include "common/config/grpc_mux_impl.h"
#include "common/config/protobuf_link_hacks.h"
#include "common/config/resources.h"
Expand Down Expand Up @@ -68,7 +69,7 @@ class GrpcMuxImplTestBase : public testing::Test {
bool first = false, const std::string& nonce = "",
const Protobuf::int32 error_code = Grpc::Status::WellKnownGrpcStatus::Ok,
const std::string& error_message = "") {
envoy::api::v2::DiscoveryRequest expected_request;
API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_request;
if (first) {
expected_request.mutable_node()->CopyFrom(local_info_.node());
}
Expand Down
3 changes: 2 additions & 1 deletion test/common/config/grpc_subscription_test_harness.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/api/v2/eds.pb.h"

#include "common/common/hash.h"
#include "common/config/api_version.h"
#include "common/config/grpc_subscription_impl.h"
#include "common/config/resources.h"

Expand Down Expand Up @@ -62,7 +63,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness {
bool expect_node, const Protobuf::int32 error_code,
const std::string& error_message) {
UNREFERENCED_PARAMETER(expect_node);
envoy::api::v2::DiscoveryRequest expected_request;
API_NO_BOOST(envoy::api::v2::DiscoveryRequest) expected_request;
if (expect_node) {
expected_request.mutable_node()->CopyFrom(node_);
}
Expand Down
1 change: 1 addition & 0 deletions test/common/router/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ envoy_cc_test(
deps = [
"//include/envoy/config:subscription_interface",
"//include/envoy/init:manager_interface",
"//source/common/config:api_version_lib",
"//source/common/config:utility_lib",
"//source/common/http:message_lib",
"//source/common/json:json_loader_lib",
Expand Down
14 changes: 8 additions & 6 deletions test/common/router/scoped_rds_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "envoy/init/manager.h"
#include "envoy/stats/scope.h"

#include "common/config/api_version.h"
#include "common/config/grpc_mux_impl.h"
#include "common/router/scoped_rds.h"

Expand Down Expand Up @@ -122,12 +123,13 @@ class ScopedRdsTest : public ScopedRoutesTestBase {
subscriptionFromConfigSource(_, _, _, _))
.Times(AnyNumber());
// rds subscription
EXPECT_CALL(server_factory_context_.cluster_manager_.subscription_factory_,
subscriptionFromConfigSource(
_,
Eq(Grpc::Common::typeUrl(
envoy::api::v2::RouteConfiguration().GetDescriptor()->full_name())),
_, _))
EXPECT_CALL(
server_factory_context_.cluster_manager_.subscription_factory_,
subscriptionFromConfigSource(
_,
Eq(Grpc::Common::typeUrl(
API_NO_BOOST(envoy::api::v2::RouteConfiguration)().GetDescriptor()->full_name())),
_, _))
.Times(AnyNumber())
.WillRepeatedly(Invoke([this](const envoy::api::v2::core::ConfigSource&, absl::string_view,
Stats::Scope&,
Expand Down
5 changes: 5 additions & 0 deletions test/integration/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ envoy_cc_test(
],
deps = [
":http_integration_lib",
"//source/common/config:api_version_lib",
"//source/common/protobuf",
"//test/test_common:utility_lib",
"@envoy_api//envoy/api/v2:pkg_cc_proto",
Expand Down Expand Up @@ -482,6 +483,7 @@ envoy_cc_test_library(
"//source/common/buffer:zero_copy_input_stream_lib",
"//source/common/common:assert_lib",
"//source/common/common:minimal_logger_lib",
"//source/common/config:api_version_lib",
"//source/common/event:dispatcher_lib",
"//source/common/grpc:codec_lib",
"//source/common/grpc:common_lib",
Expand Down Expand Up @@ -769,6 +771,7 @@ envoy_cc_test(
],
deps = [
":http_integration_lib",
"//source/common/config:api_version_lib",
"//source/common/config:protobuf_link_hacks",
"//source/common/config:resources_lib",
"//source/common/event:dispatcher_includes",
Expand Down Expand Up @@ -801,6 +804,7 @@ envoy_cc_test(
],
deps = [
":integration_lib",
"//source/common/config:api_version_lib",
"//source/common/event:dispatcher_includes",
"//source/common/event:dispatcher_lib",
"//source/common/network:utility_lib",
Expand Down Expand Up @@ -956,6 +960,7 @@ envoy_cc_test(
],
deps = [
":http_integration_lib",
"//source/common/config:api_version_lib",
"//source/common/config:resources_lib",
"//source/common/event:dispatcher_includes",
"//source/common/event:dispatcher_lib",
Expand Down
Loading

0 comments on commit 660f860

Please sign in to comment.