From 9fae5a9a992a1ca8e83ebb1114cb93a0e42178e5 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 16 Jul 2019 10:02:23 -0400 Subject: [PATCH 1/7] test: adding an option to hard-fail when deprecated config is used. Signed-off-by: Alyssa Wilk --- CONTRIBUTING.md | 6 +++++- bazel/BUILD | 5 +++++ bazel/README.md | 2 ++ bazel/envoy_internal.bzl | 3 +++ ci/do_ci.sh | 2 +- source/common/runtime/runtime_impl.cc | 5 +++++ test/common/protobuf/utility_test.cc | 2 ++ test/common/runtime/runtime_impl_test.cc | 9 +++++++++ 8 files changed, 32 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 345192d66ddf..f7bc4023cec8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,11 @@ maximize the chances of your PR being merged. could convert from the earlier API to the new API. A field may be deprecated if this tool would be able to perform the conversion. For example, removing a field to describe HTTP/2 window settings is valid if a more comprehensive - HTTP/2 protocol options field is being introduced to replace it. + HTTP/2 protocol options field is being introduced to replace it. The PR author + deprecating the old configuration is responsible for updating all tests and + canonical configuration, or guarding them with ifndef DISABLE_DEPRECATED_FEATURES. + This will be validated by the bazel.debug target, which will hard-fail when + deprecated configuration is used. * For configuration deprecations that are not covered by the above semantic replacement policy, any deprecation will only take place after community consultation on mailing lists, Slack and GitHub, over the period of diff --git a/bazel/BUILD b/bazel/BUILD index d449250d2390..e60a4ae9c06c 100755 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -120,6 +120,11 @@ config_setting( values = {"define": "object_dump_on_signal_trace=disabled"}, ) +config_setting( + name = "disable_deprecated_features", + values = {"define": "deprecated_features=disabled"}, +) + config_setting( name = "disable_hot_restart", values = {"define": "hot_restart=disabled"}, diff --git a/bazel/README.md b/bazel/README.md index 2631fbec01f3..24340fdd0fce 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -407,6 +407,8 @@ The following optional features can be disabled on the Bazel build command-line: * Backtracing on signals with `--define signal_trace=disabled` * Active stream state dump on signals with `--define signal_trace=disabled` or `--define disable_object_dump_on_signal_trace=disabled` * tcmalloc with `--define tcmalloc=disabled` +* deprecated features with `--define deprecated_features=disabled` + ## Enabling optional features diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index 325ea94f5596..94e2ce7f4fc3 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -56,6 +56,9 @@ def envoy_copts(repository, test = False): }) + select({ repository + "//bazel:disable_object_dump_on_signal_trace": [], "//conditions:default": ["-DENVOY_OBJECT_TRACE_ON_DUMP"], + }) + select({ + repository + "//bazel:disable_deprecated_features": ["-DDISABLE_DEPRECATED_FEATURES"], + "//conditions:default": [], }) + select({ repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"], "//conditions:default": [], diff --git a/ci/do_ci.sh b/ci/do_ci.sh index d526c87d1721..e8440e0bcc81 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -133,7 +133,7 @@ elif [[ "$CI_TARGET" == "bazel.debug" ]]; then echo "bazel debug build with tests..." bazel_binary_build debug echo "Testing ${TEST_TARGETS}" - bazel test ${BAZEL_BUILD_OPTIONS} -c dbg ${TEST_TARGETS} + bazel test ${BAZEL_BUILD_OPTIONS} --define deprecated_features=disabled -c dbg ${TEST_TARGETS} exit 0 elif [[ "$CI_TARGET" == "bazel.debug.server_only" ]]; then setup_clang_toolchain diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 3e86fcba3329..0d4569c133c8 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -182,9 +182,14 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const { // If either disallowed by default or configured off, the feature is not enabled. return false; } + // The feature is allowed. It is assumed this check is called when the feature // is about to be used, so increment the feature use stat. stats_.deprecated_feature_use_.inc(); +#ifdef DISABLE_DEPRECATED_FEATURES + return false; +#endif + return true; } diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 04fb200fc075..61f879a5eb6c 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -492,6 +492,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); } +#ifndef DISABLE_DEPRECATED_FEATURES TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); @@ -617,6 +618,7 @@ TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) { "'envoy.test.deprecation_test.Base.deprecated_repeated_message'", MessageUtil::checkForDeprecation(base)); } +#endif class TimestampUtilTest : public testing::Test, public ::testing::WithParamInterface {}; diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 82fedad731af..2c4e3d8276d2 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -179,6 +179,15 @@ TEST_F(DiskLoaderImplTest, All) { // test_feature_false is not in runtime_features.cc and so is false by default. EXPECT_EQ(false, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); + // Deprecation +#ifdef DISABLE_DEPRECATED_FEATURES + EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled")); +#else + EXPECT_EQ(true, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled")); +#endif + EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled( + "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal")); + // Feature defaults via helper function. EXPECT_EQ(false, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); EXPECT_EQ(true, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_true")); From 8c6d2b87ead498b41f365521748de4cda66b42a2 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Aug 2019 09:22:29 -0400 Subject: [PATCH 2/7] Envoy prefix Signed-off-by: Alyssa Wilk --- CONTRIBUTING.md | 4 ++-- bazel/envoy_internal.bzl | 2 +- ci/do_ci.sh | 6 +++--- source/common/runtime/runtime_impl.cc | 2 +- test/common/protobuf/utility_test.cc | 2 +- test/common/runtime/runtime_impl_test.cc | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f7bc4023cec8..32b72157f3f3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,8 +74,8 @@ maximize the chances of your PR being merged. field to describe HTTP/2 window settings is valid if a more comprehensive HTTP/2 protocol options field is being introduced to replace it. The PR author deprecating the old configuration is responsible for updating all tests and - canonical configuration, or guarding them with ifndef DISABLE_DEPRECATED_FEATURES. - This will be validated by the bazel.debug target, which will hard-fail when + canonical configuration, or guarding them with ifndef ENVOY_DISABLE_DEPRECATED_FEATURES. + This will be validated by the bazel.asan target, which will hard-fail when deprecated configuration is used. * For configuration deprecations that are not covered by the above semantic replacement policy, any deprecation will only take place after diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index 94e2ce7f4fc3..2562390311de 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -57,7 +57,7 @@ def envoy_copts(repository, test = False): repository + "//bazel:disable_object_dump_on_signal_trace": [], "//conditions:default": ["-DENVOY_OBJECT_TRACE_ON_DUMP"], }) + select({ - repository + "//bazel:disable_deprecated_features": ["-DDISABLE_DEPRECATED_FEATURES"], + repository + "//bazel:disable_deprecated_features": ["-DENVOY_DISABLE_DEPRECATED_FEATURES"], "//conditions:default": [], }) + select({ repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"], diff --git a/ci/do_ci.sh b/ci/do_ci.sh index e8440e0bcc81..1f28459a0570 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -133,7 +133,7 @@ elif [[ "$CI_TARGET" == "bazel.debug" ]]; then echo "bazel debug build with tests..." bazel_binary_build debug echo "Testing ${TEST_TARGETS}" - bazel test ${BAZEL_BUILD_OPTIONS} --define deprecated_features=disabled -c dbg ${TEST_TARGETS} + bazel test ${BAZEL_BUILD_OPTIONS} -c dbg ${TEST_TARGETS} exit 0 elif [[ "$CI_TARGET" == "bazel.debug.server_only" ]]; then setup_clang_toolchain @@ -144,10 +144,10 @@ elif [[ "$CI_TARGET" == "bazel.asan" ]]; then setup_clang_toolchain echo "bazel ASAN/UBSAN debug build with tests" echo "Building and testing envoy tests ${TEST_TARGETS}" - bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --config=clang-asan ${TEST_TARGETS} + bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --define deprecated_features=disabled --config=clang-asan ${TEST_TARGETS} echo "Building and testing envoy-filter-example tests..." pushd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" - bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --config=clang-asan \ + bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --define deprecated_features=disabled --config=clang-asan \ //:echo2_integration_test //:envoy_binary_test popd # Also validate that integration test traffic tapping (useful when debugging etc.) diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 0d4569c133c8..4162141f5920 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -186,7 +186,7 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const { // The feature is allowed. It is assumed this check is called when the feature // is about to be used, so increment the feature use stat. stats_.deprecated_feature_use_.inc(); -#ifdef DISABLE_DEPRECATED_FEATURES +#ifdef ENVOY_DISABLE_DEPRECATED_FEATURES return false; #endif diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 61f879a5eb6c..13bb4fb38aa2 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -492,7 +492,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); } -#ifndef DISABLE_DEPRECATED_FEATURES +#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 2c4e3d8276d2..55d9bf662744 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -180,7 +180,7 @@ TEST_F(DiskLoaderImplTest, All) { EXPECT_EQ(false, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); // Deprecation -#ifdef DISABLE_DEPRECATED_FEATURES +#ifdef ENVOY_DISABLE_DEPRECATED_FEATURES EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled")); #else EXPECT_EQ(true, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled")); From a427a9c64558c07cdcccb71b2d78cb761840f566 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Aug 2019 11:01:57 -0400 Subject: [PATCH 3/7] test fixes Signed-off-by: Alyssa Wilk --- .../clusters/redis/redis_cluster_integration_test.cc | 4 +++- .../filters/http/cors/cors_filter_integration_test.cc | 6 +++++- .../redis_proxy/redis_proxy_integration_test.cc | 10 +++++++--- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 91aecc412a3d..000928ef2441 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -36,7 +36,9 @@ const std::string& testConfig() { name: envoy.redis_proxy config: stat_prefix: redis_stats - cluster: cluster_0 + prefix_routes: + catch_all_route: + cluster: cluster_0 settings: op_timeout: 5s clusters: diff --git a/test/extensions/filters/http/cors/cors_filter_integration_test.cc b/test/extensions/filters/http/cors/cors_filter_integration_test.cc index bca2bcff0c63..6c78c1892313 100644 --- a/test/extensions/filters/http/cors/cors_filter_integration_test.cc +++ b/test/extensions/filters/http/cors/cors_filter_integration_test.cc @@ -34,7 +34,11 @@ class CorsFilterIntegrationTest : public testing::TestWithParamadd_routes(); route->mutable_match()->set_prefix("/no-cors"); route->mutable_route()->set_cluster("cluster_0"); - route->mutable_route()->mutable_cors()->mutable_enabled()->set_value(false); + route->mutable_route() + ->mutable_cors() + ->mutable_filter_enabled() + ->mutable_default_value() + ->set_numerator(0); } { diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index c9f8c2ce10f7..c8c2ecc5e74c 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -56,7 +56,9 @@ const std::string CONFIG = R"EOF( name: envoy.redis_proxy config: stat_prefix: redis_stats - cluster: cluster_0 + prefix_routes: + catch_all_route: + cluster: cluster_0 settings: op_timeout: 5s )EOF"; @@ -149,7 +151,8 @@ const std::string CONFIG_WITH_ROUTES_BASE = R"EOF( const std::string CONFIG_WITH_ROUTES = CONFIG_WITH_ROUTES_BASE + R"EOF( prefix_routes: - catch_all_cluster: cluster_0 + catch_all_route: + cluster: cluster_0 routes: - prefix: "foo:" cluster: cluster_1 @@ -250,7 +253,8 @@ const std::string CONFIG_WITH_ROUTES_AND_AUTH_PASSWORDS = R"EOF( settings: op_timeout: 5s prefix_routes: - catch_all_cluster: cluster_0 + catch_all_route: + cluster: cluster_0 routes: - prefix: "foo:" cluster: cluster_1 From f9d670c73350353ed5d383be89acc6b009aa70a9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Aug 2019 11:26:27 -0400 Subject: [PATCH 4/7] moving to compile options Signed-off-by: Alyssa Wilk --- CONTRIBUTING.md | 2 +- ci/do_ci.sh | 5 +++-- .../filters/http/cors/cors_filter_integration_test.cc | 6 +----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 32b72157f3f3..e52e15979e93 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -75,7 +75,7 @@ maximize the chances of your PR being merged. HTTP/2 protocol options field is being introduced to replace it. The PR author deprecating the old configuration is responsible for updating all tests and canonical configuration, or guarding them with ifndef ENVOY_DISABLE_DEPRECATED_FEATURES. - This will be validated by the bazel.asan target, which will hard-fail when + This will be validated by the bazel.compile_time_options target, which will hard-fail when deprecated configuration is used. * For configuration deprecations that are not covered by the above semantic replacement policy, any deprecation will only take place after diff --git a/ci/do_ci.sh b/ci/do_ci.sh index 1f28459a0570..db8bc0bdbd24 100755 --- a/ci/do_ci.sh +++ b/ci/do_ci.sh @@ -144,10 +144,10 @@ elif [[ "$CI_TARGET" == "bazel.asan" ]]; then setup_clang_toolchain echo "bazel ASAN/UBSAN debug build with tests" echo "Building and testing envoy tests ${TEST_TARGETS}" - bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --define deprecated_features=disabled --config=clang-asan ${TEST_TARGETS} + bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --config=clang-asan ${TEST_TARGETS} echo "Building and testing envoy-filter-example tests..." pushd "${ENVOY_FILTER_EXAMPLE_SRCDIR}" - bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --define deprecated_features=disabled --config=clang-asan \ + bazel_with_collection test ${BAZEL_BUILD_OPTIONS} -c dbg --config=clang-asan \ //:echo2_integration_test //:envoy_binary_test popd # Also validate that integration test traffic tapping (useful when debugging etc.) @@ -197,6 +197,7 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then --define log_debug_assert_in_release=enabled \ --define quiche=enabled \ --define path_normalization_by_default=true \ + --define deprecated_features=disabled \ " setup_clang_libcxx_toolchain # This doesn't go into CI but is available for developer convenience. diff --git a/test/extensions/filters/http/cors/cors_filter_integration_test.cc b/test/extensions/filters/http/cors/cors_filter_integration_test.cc index 6c78c1892313..bca2bcff0c63 100644 --- a/test/extensions/filters/http/cors/cors_filter_integration_test.cc +++ b/test/extensions/filters/http/cors/cors_filter_integration_test.cc @@ -34,11 +34,7 @@ class CorsFilterIntegrationTest : public testing::TestWithParamadd_routes(); route->mutable_match()->set_prefix("/no-cors"); route->mutable_route()->set_cluster("cluster_0"); - route->mutable_route() - ->mutable_cors() - ->mutable_filter_enabled() - ->mutable_default_value() - ->set_numerator(0); + route->mutable_route()->mutable_cors()->mutable_enabled()->set_value(false); } { From 5829dd55349b591361043d03f5409ef2e3f42eac Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 20 Aug 2019 13:00:11 -0400 Subject: [PATCH 5/7] reinstating test Signed-off-by: Alyssa Wilk --- .../filters/http/cors/cors_filter_integration_test.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/extensions/filters/http/cors/cors_filter_integration_test.cc b/test/extensions/filters/http/cors/cors_filter_integration_test.cc index bca2bcff0c63..6c78c1892313 100644 --- a/test/extensions/filters/http/cors/cors_filter_integration_test.cc +++ b/test/extensions/filters/http/cors/cors_filter_integration_test.cc @@ -34,7 +34,11 @@ class CorsFilterIntegrationTest : public testing::TestWithParamadd_routes(); route->mutable_match()->set_prefix("/no-cors"); route->mutable_route()->set_cluster("cluster_0"); - route->mutable_route()->mutable_cors()->mutable_enabled()->set_value(false); + route->mutable_route() + ->mutable_cors() + ->mutable_filter_enabled() + ->mutable_default_value() + ->set_numerator(0); } { From 4e0bb5efe1ad1d21edb7a334643b9b6bdb104f40 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 21 Aug 2019 17:10:42 -0400 Subject: [PATCH 6/7] macro Signed-off-by: Alyssa Wilk --- test/common/protobuf/utility_test.cc | 21 ++++++++++----------- test/test_common/utility.h | 13 +++++++++++++ 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 13bb4fb38aa2..62ce76981ef4 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -492,8 +492,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { EXPECT_EQ(0, runtime_deprecated_feature_use_.value()); } -#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES -TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) { +TEST_F(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. @@ -504,7 +503,7 @@ TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) { } // Use of a deprecated and disallowed field should result in an exception. -TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowed) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); EXPECT_THROW_WITH_REGEX( @@ -512,7 +511,8 @@ TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowed) { "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); } -TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowedWithRuntimeOverride) { +TEST_F(DeprecatedFieldsTest, + DEPRECATED_FEATURE_TEST(IndividualFieldDisallowedWithRuntimeOverride)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated_fatal("foo"); @@ -534,7 +534,7 @@ TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowedWithRuntimeOverride) { EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } -TEST_F(DeprecatedFieldsTest, DisallowViaRuntime) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); @@ -556,7 +556,7 @@ TEST_F(DeprecatedFieldsTest, 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, MixOfFatalAndWarnings) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) { envoy::test::deprecation_test::Base base; base.set_is_deprecated("foo"); base.set_is_deprecated_fatal("foo"); @@ -569,7 +569,7 @@ TEST_F(DeprecatedFieldsTest, MixOfFatalAndWarnings) { } // Present (unused) deprecated messages should be detected as deprecated. -TEST_F(DeprecatedFieldsTest, MessageDeprecated) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) { envoy::test::deprecation_test::Base base; base.mutable_deprecated_message(); EXPECT_LOG_CONTAINS( @@ -578,7 +578,7 @@ TEST_F(DeprecatedFieldsTest, MessageDeprecated) { EXPECT_EQ(1, runtime_deprecated_feature_use_.value()); } -TEST_F(DeprecatedFieldsTest, InnerMessageDeprecated) { +TEST_F(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 @@ -594,7 +594,7 @@ TEST_F(DeprecatedFieldsTest, InnerMessageDeprecated) { } // Check that repeated sub-messages get validated. -TEST_F(DeprecatedFieldsTest, SubMessageDeprecated) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.add_repeated_message(); base.add_repeated_message()->set_inner_deprecated("foo"); @@ -608,7 +608,7 @@ TEST_F(DeprecatedFieldsTest, SubMessageDeprecated) { } // Check that deprecated repeated messages trigger -TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) { +TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) { envoy::test::deprecation_test::Base base; base.add_deprecated_repeated_message(); @@ -618,7 +618,6 @@ TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) { "'envoy.test.deprecation_test.Base.deprecated_repeated_message'", MessageUtil::checkForDeprecation(base)); } -#endif class TimestampUtilTest : public testing::Test, public ::testing::WithParamInterface {}; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 7e33eeff7d1e..c66ba133c3bd 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -105,6 +105,19 @@ namespace Envoy { } \ } while (false) +// A convenience macro for testing Envoy deprecated features. This will disable the test when +// tests are built with --define deprecated_features=disabled to avoid the hard-failure mode for +// deprecated features. Sample usage is: +// +// TEST_F(FixtureName, DEPRECATED_FEATURE_TEST(TestName)) { +// ... +// } +#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES +#define DEPRECATED_FEATURE_TEST(X) X +#else +#define DEPRECATED_FEATURE_TEST(X) DISABLED_##X +#endif + // Random number generator which logs its seed to stderr. To repeat a test run with a non-zero seed // one can run the test with --test_arg=--gtest_random_seed=[seed] class TestRandomGenerator { From 74610058ac42177574332f04be216fb8d62aef43 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 22 Aug 2019 09:10:18 -0400 Subject: [PATCH 7/7] docs fix Signed-off-by: Alyssa Wilk --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e52e15979e93..23e9bd13a6e3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -74,7 +74,7 @@ maximize the chances of your PR being merged. field to describe HTTP/2 window settings is valid if a more comprehensive HTTP/2 protocol options field is being introduced to replace it. The PR author deprecating the old configuration is responsible for updating all tests and - canonical configuration, or guarding them with ifndef ENVOY_DISABLE_DEPRECATED_FEATURES. + canonical configuration, or guarding them with the DEPRECATED_FEATURE_TEST() macro. This will be validated by the bazel.compile_time_options target, which will hard-fail when deprecated configuration is used. * For configuration deprecations that are not covered by the above semantic