Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: adding an option to hard-fail when deprecated config is used. #7962

Merged
merged 7 commits into from
Aug 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 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
replacement policy, any deprecation will only take place after
community consultation on mailing lists, Slack and GitHub, over the period of
Expand Down
5 changes: 5 additions & 0 deletions bazel/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down
2 changes: 2 additions & 0 deletions bazel/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions bazel/envoy_internal.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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": ["-DENVOY_DISABLE_DEPRECATED_FEATURES"],
"//conditions:default": [],
}) + select({
repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"],
"//conditions:default": [],
Expand Down
1 change: 1 addition & 0 deletions ci/do_ci.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
5 changes: 5 additions & 0 deletions source/common/runtime/runtime_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 ENVOY_DISABLE_DEPRECATED_FEATURES
return false;
#endif

return true;
}

Expand Down
19 changes: 10 additions & 9 deletions test/common/protobuf/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) {
EXPECT_EQ(0, runtime_deprecated_feature_use_.value());
}

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.
Expand All @@ -503,15 +503,16 @@ 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(
MessageUtil::checkForDeprecation(base), ProtoValidationException,
"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");

Expand All @@ -533,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");

Expand All @@ -555,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");
Expand All @@ -568,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(
Expand All @@ -577,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
Expand All @@ -593,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");
Expand All @@ -607,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();

Expand Down
9 changes: 9 additions & 0 deletions test/common/runtime/runtime_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 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"));
#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"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const std::string& testConfig() {
name: envoy.redis_proxy
config:
stat_prefix: redis_stats
cluster: cluster_0
prefix_routes:
Copy link
Member

Choose a reason for hiding this comment

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

Q: are we losing coverage of the deprecated paths here and below with this change? Should these tests instead be wrapped or do we need additional tests?

Copy link
Contributor Author

@alyssawilk alyssawilk Aug 22, 2019

Choose a reason for hiding this comment

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

I can add unit tests for the old route / cors logic. Ok if I do it in a follow-up? I'd like to land this sooner rather than later, just so I don't have to do more test fix up :-)

Copy link
Member

Choose a reason for hiding this comment

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

Sure sounds good.

catch_all_route:
cluster: cluster_0
settings:
op_timeout: 5s
clusters:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,11 @@ class CorsFilterIntegrationTest : public testing::TestWithParam<Network::Address
auto* route = virtual_host->add_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);
}

{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions test/test_common/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down