diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index de6cfbc6080b..46db864baef6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -33,6 +33,17 @@ maximize the chances of your PR being merged. deprecations between 1.3.0 and 1.4.0 will be deleted soon AFTER 1.5.0 is tagged and released (at the beginning of the 1.6.0 release cycle). This results in a three to six month window for migrating from deprecated code paths to new code paths. +* Unless the community and Envoy maintainer team agrees on an exception, during the + first release cycle after a feature has been deprecated, use of that feature + will cause a logged warning, and incrementing the + [runtime](https://www.envoyproxy.io/docs/envoy/latest/configuration/runtime#config-runtime) + runtime.deprecated_feature_use stat. + During the second release cycle, use of the deprecated configuration will + cause a configuration load failure, unless the feature in question is + explicitly overridden in + [runtime](https://www.envoyproxy.io/docs/envoy/latest/configuration/runtime#config-runtime) + config. Finally during the third release cycle the code and configuration will be removed + entirely. * This policy means that organizations deploying master should have some time to get ready for breaking changes, but we make no guarantees about the length of time. * The breaking change policy also applies to source level extensions (e.g., filters). Code that diff --git a/docs/root/configuration/runtime.rst b/docs/root/configuration/runtime.rst index 262dafdd0d6b..0c2dce87e4bb 100644 --- a/docs/root/configuration/runtime.rst +++ b/docs/root/configuration/runtime.rst @@ -79,6 +79,32 @@ old tree to the new runtime tree, using the equivalent of the following command: It's beyond the scope of this document how the file system data is deployed, garbage collected, etc. +Using runtime overrides for deprecated features +----------------------------------------------- + +The Envoy runtime is also a part of the Envoy feature deprecation process. + +As described in the Envoy :repo:`breaking change policy `, +feature deprecation in Envoy is in 3 phases: warn-by-default, fail-by-default, and code removal. + +In the first phase, Envoy logs a warning to the warning log that the feature is deprecated and +increments the :ref:`deprecated_feature_use ` runtime stat. +Users are encouraged to go to :repo:`DEPRECATED.md ` to see how to +migrate to the new code path and make sure it is suitable for their use case. + +In the second phase the message and filename will be added to +:repo:`runtime_features.h ` +and use of that configuration field will cause the config to be rejected by default. +This fail-by-default mode can be overridden in runtime configuration by setting +envoy.deprecated_features.filename.proto:fieldname to true. For example, for a deprecated field +``Foo.Bar.Eep`` in ``baz.proto`` set ``envoy.deprecated_features.baz.proto:Eep`` to +``true``. Use of this override is **strongly discouraged**. +Fatal-by-default configuration indicates that the removal of the old code paths is imminent. It is +far better for both Envoy users and for Envoy contributors if any bugs or feature gaps with the new +code paths are flushed out ahead of time, rather than after the code is removed! + +.. _runtime_stats: + Statistics ---------- @@ -92,4 +118,5 @@ The file system runtime provider emits some statistics in the *runtime.* namespa override_dir_not_exists, Counter, Total number of loads that did not use an override directory override_dir_exists, Counter, Total number of loads that did use an override directory load_success, Counter, Total number of load attempts that were successful + deprecated_feature_use, Counter, Total number of times deprecated features were used. num_keys, Gauge, Number of keys currently loaded diff --git a/include/envoy/runtime/BUILD b/include/envoy/runtime/BUILD index 073bc8559187..8e510aeb09dd 100644 --- a/include/envoy/runtime/BUILD +++ b/include/envoy/runtime/BUILD @@ -13,6 +13,8 @@ envoy_cc_library( hdrs = ["runtime.h"], external_deps = ["abseil_optional"], deps = [ + "//source/common/common:assert_lib", + "//source/common/singleton:threadsafe_singleton", "@envoy_api//envoy/type:percent_cc", ], ) diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index ef535cf8ffd3..6b424e74907a 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -9,6 +9,9 @@ #include "envoy/common/pure.h" #include "envoy/type/percent.pb.h" +#include "common/common/assert.h" +#include "common/singleton/threadsafe_singleton.h" + #include "absl/types/optional.h" namespace Envoy { @@ -46,6 +49,7 @@ class Snapshot { std::string raw_string_value_; absl::optional uint_value_; absl::optional fractional_percent_value_; + absl::optional bool_value_; }; typedef std::unordered_map EntryMap; @@ -71,6 +75,14 @@ class Snapshot { typedef std::unique_ptr OverrideLayerConstPtr; + // Returns true if a deprecated feature is allowed. + // + // Fundamentally, deprecated features are boolean values. + // They are allowed by default or with explicit configuration to "true" via runtime configuration. + // They can be disallowed either by inclusion in the hard-coded disallowed_features[] list, or by + // configuration of "false" in runtime config. + virtual bool deprecatedFeatureEnabled(const std::string& key) const PURE; + /** * Test if a feature is enabled using the built in random generator. This is done by generating * a random number in the range 0-99 and seeing if this number is < the value stored in the @@ -201,7 +213,17 @@ class Loader { virtual void mergeValues(const std::unordered_map& values) PURE; }; -typedef std::unique_ptr LoaderPtr; +using LoaderPtr = std::unique_ptr; + +// To make the runtime generally accessible, we make use of the dreaded +// singleton class. For Envoy, the runtime will be created and cleaned up by the +// Server::InstanceImpl initialize() and destructor, respectively. +// +// This makes it possible for call sites to easily make use of runtime values to +// determine if a given feature is on or off, as well as various deprecated configuration +// protos being enabled or disabled by default. +using LoaderSingleton = InjectableSingleton; +using ScopedLoaderSingleton = ScopedInjectableLoader; } // namespace Runtime } // namespace Envoy diff --git a/source/common/protobuf/BUILD b/source/common/protobuf/BUILD index c07212310aa7..a563c0d95c96 100644 --- a/source/common/protobuf/BUILD +++ b/source/common/protobuf/BUILD @@ -41,6 +41,7 @@ envoy_cc_library( deps = [ ":protobuf", "//include/envoy/api:api_interface", + "//include/envoy/runtime:runtime_interface", "//source/common/common:assert_lib", "//source/common/common:hash_lib", "//source/common/common:utility_lib", diff --git a/source/common/protobuf/utility.cc b/source/common/protobuf/utility.cc index d4d611cf0d06..a734d5394236 100644 --- a/source/common/protobuf/utility.cc +++ b/source/common/protobuf/utility.cc @@ -8,6 +8,18 @@ #include "absl/strings/match.h" namespace Envoy { +namespace { + +absl::string_view filenameFromPath(absl::string_view full_path) { + size_t index = full_path.rfind("/"); + if (index == std::string::npos || index == full_path.size()) { + return full_path; + } + return full_path.substr(index + 1, full_path.size()); +} + +} // namespace + namespace ProtobufPercentHelper { uint64_t checkAndReturnDefault(uint64_t default_value, uint64_t max_value) { @@ -106,7 +118,7 @@ void MessageUtil::loadFromFile(const std::string& path, Protobuf::Message& messa } } -void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool warn_only) { +void MessageUtil::checkForDeprecation(const Protobuf::Message& message, Runtime::Loader* runtime) { const Protobuf::Descriptor* descriptor = message.GetDescriptor(); const Protobuf::Reflection* reflection = message.GetReflection(); for (int i = 0; i < descriptor->field_count(); ++i) { @@ -118,17 +130,32 @@ void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool war continue; } + bool warn_only = true; + absl::string_view filename = filenameFromPath(field->file()->name()); + // Allow runtime to be null both to not crash if this is called before server initialization, + // and so proto validation works in context where runtime singleton is not set up (e.g. + // standalone config validation utilities) + if (runtime && !runtime->snapshot().deprecatedFeatureEnabled( + absl::StrCat("envoy.deprecated_features.", filename, ":", field->name()))) { + warn_only = false; + } + // If this field is deprecated, warn or throw an error. if (field->options().deprecated()) { std::string err = fmt::format( - "Using deprecated option '{}'. This configuration will be removed from Envoy soon. " - "Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md for " - "details.", - field->full_name()); + "Using deprecated option '{}' from file {}. This configuration will be removed from " + "Envoy soon. Please see https://github.com/envoyproxy/envoy/blob/master/DEPRECATED.md " + "for details.", + field->full_name(), filename); if (warn_only) { ENVOY_LOG_MISC(warn, "{}", err); } else { - throw ProtoValidationException(err, message); + const char fatal_error[] = + " If continued use of this field is absolutely necessary, see " + "https://www.envoyproxy.io/docs/envoy/latest/configuration/runtime" + "#using-runtime-overrides-for-deprecated-features for how to apply a temporary and" + "highly discouraged override."; + throw ProtoValidationException(err + fatal_error, message); } } @@ -137,10 +164,10 @@ void MessageUtil::checkForDeprecation(const Protobuf::Message& message, bool war if (field->is_repeated()) { const int size = reflection->FieldSize(message, field); for (int j = 0; j < size; ++j) { - checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), warn_only); + checkForDeprecation(reflection->GetRepeatedMessage(message, field, j), runtime); } } else { - checkForDeprecation(reflection->GetMessage(message, field), warn_only); + checkForDeprecation(reflection->GetMessage(message, field), runtime); } } } diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index e93a944d6e8b..dc3e17523c24 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -5,6 +5,7 @@ #include "envoy/api/api.h" #include "envoy/common/exception.h" #include "envoy/json/json_object.h" +#include "envoy/runtime/runtime.h" #include "envoy/type/percent.pb.h" #include "common/common/hash.h" @@ -192,11 +193,13 @@ class MessageUtil { /** * Checks for use of deprecated fields in message and all sub-messages. * @param message message to validate. - * @param warn_only if true, logs a warning rather than throwing an exception if deprecated fields - * are in use. - * @throw ProtoValidationException if deprecated fields are used and warn_only is false. + * @param loader optional a pointer to the runtime loader for live deprecation status. + * @throw ProtoValidationException if deprecated fields are used and listed + * Runtime::DisallowedFeatures */ - static void checkForDeprecation(const Protobuf::Message& message, bool warn_only); + static void + checkForDeprecation(const Protobuf::Message& message, + Runtime::Loader* loader = Runtime::LoaderSingleton::getExisting()); /** * Validate protoc-gen-validate constraints on a given protobuf. @@ -206,8 +209,8 @@ class MessageUtil { * @throw ProtoValidationException if the message does not satisfy its type constraints. */ template static void validate(const MessageType& message) { - // Log warnings if deprecated fields are in use. - checkForDeprecation(message, true); + // Log warnings or throw errors if deprecated fields are in use. + checkForDeprecation(message); std::string err; if (!Validate(message, &err)) { diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index 7fecd67d8a96..6cb7f339b36c 100644 --- a/source/common/runtime/BUILD +++ b/source/common/runtime/BUILD @@ -11,7 +11,10 @@ envoy_package() envoy_cc_library( name = "runtime_lib", srcs = ["runtime_impl.cc"], - hdrs = ["runtime_impl.h"], + hdrs = [ + "runtime_features.h", + "runtime_impl.h", + ], external_deps = ["ssl"], deps = [ "//include/envoy/event:dispatcher_interface", diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h new file mode 100644 index 000000000000..d57aab19c6d2 --- /dev/null +++ b/source/common/runtime/runtime_features.h @@ -0,0 +1,37 @@ +#pragma once + +#include + +#include "common/singleton/const_singleton.h" + +#include "absl/container/flat_hash_set.h" + +namespace Envoy { +namespace Runtime { + +const char* disallowed_features[] = { + // Acts as both a test entry for deprecated.proto and a marker for the Envoy + // deprecation scripts. + "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal", +}; + +class DisallowedFeatures { +public: + DisallowedFeatures() { + for (auto& feature : disallowed_features) { + disallowed_features_.insert(feature); + } + } + + bool disallowedByDefault(const std::string& feature) const { + return disallowed_features_.find(feature) != disallowed_features_.end(); + } + +private: + absl::flat_hash_set disallowed_features_; +}; + +using DisallowedFeaturesDefaults = ConstSingleton; + +} // namespace Runtime +} // namespace Envoy diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 2266a1418907..17c503122e9d 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -14,7 +14,9 @@ #include "common/common/utility.h" #include "common/filesystem/directory.h" #include "common/protobuf/utility.h" +#include "common/runtime/runtime_features.h" +#include "absl/strings/match.h" #include "openssl/rand.h" namespace Envoy { @@ -144,6 +146,25 @@ std::string RandomGeneratorImpl::uuid() { return std::string(uuid, UUID_LENGTH); } +bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const { + bool allowed = false; + // See if this value is explicitly set as a runtime boolean. + bool stored = getBoolean(key, allowed); + // If not, the default value is based on disallowedByDefault. + if (!stored) { + allowed = !DisallowedFeaturesDefaults::get().disallowedByDefault(key); + } + + if (!allowed) { + // 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(); + return true; +} + bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, uint64_t num_buckets) const { return random_value % num_buckets < std::min(getInteger(key, default_value), num_buckets); @@ -212,13 +233,22 @@ uint64_t SnapshotImpl::getInteger(const std::string& key, uint64_t default_value } } +bool SnapshotImpl::getBoolean(const std::string& key, bool& value) const { + auto entry = values_.find(key); + if (entry != values_.end() && entry->second.bool_value_.has_value()) { + value = entry->second.bool_value_.value(); + return true; + } + return false; +} + const std::vector& SnapshotImpl::getLayers() const { return layers_; } SnapshotImpl::SnapshotImpl(RandomGenerator& generator, RuntimeStats& stats, std::vector&& layers) - : layers_{std::move(layers)}, generator_{generator} { + : layers_{std::move(layers)}, generator_{generator}, stats_{stats} { for (const auto& layer : layers_) { for (const auto& kv : layer->values()) { values_.erase(kv.first); @@ -239,6 +269,20 @@ SnapshotImpl::Entry SnapshotImpl::createEntry(const std::string& value) { return entry; } +bool SnapshotImpl::parseEntryBooleanValue(Entry& entry) { + absl::string_view stripped = entry.raw_string_value_; + stripped = absl::StripAsciiWhitespace(stripped); + + if (absl::EqualsIgnoreCase(stripped, "true")) { + entry.bool_value_ = true; + return true; + } else if (absl::EqualsIgnoreCase(stripped, "false")) { + entry.bool_value_ = false; + return true; + } + return false; +} + bool SnapshotImpl::parseEntryUintValue(Entry& entry) { uint64_t converted_uint64; if (StringUtil::atoull(entry.raw_string_value_.c_str(), converted_uint64)) { diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 5fb8b85be727..3a4b9dcf9c58 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -17,12 +17,15 @@ #include "common/common/empty_string.h" #include "common/common/logger.h" #include "common/common/thread.h" +#include "common/singleton/threadsafe_singleton.h" #include "spdlog/spdlog.h" namespace Envoy { namespace Runtime { +using RuntimeSingleton = ThreadSafeSingleton; + /** * Implementation of RandomGenerator that uses per-thread RANLUX generators seeded with current * time. @@ -45,6 +48,7 @@ class RandomGeneratorImpl : public RandomGenerator { COUNTER(override_dir_not_exists) \ COUNTER(override_dir_exists) \ COUNTER(load_success) \ + COUNTER(deprecated_feature_use) \ GAUGE (num_keys) \ GAUGE (admin_overrides_active) // clang-format on @@ -67,6 +71,7 @@ class SnapshotImpl : public Snapshot, std::vector&& layers); // Runtime::Snapshot + bool deprecatedFeatureEnabled(const std::string& key) const override; bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, uint64_t num_buckets) const override; bool featureEnabled(const std::string& key, uint64_t default_value) const override; @@ -82,20 +87,29 @@ class SnapshotImpl : public Snapshot, static Entry createEntry(const std::string& value); + // Returns true and sets 'value' to the key if found. + // Returns false if the key is not a boolean value. + bool getBoolean(const std::string& key, bool& value) const; + private: static void resolveEntryType(Entry& entry) { + if (parseEntryBooleanValue(entry)) { + return; + } if (parseEntryUintValue(entry)) { return; } parseEntryFractionalPercentValue(entry); } + static bool parseEntryBooleanValue(Entry& entry); static bool parseEntryUintValue(Entry& entry); static void parseEntryFractionalPercentValue(Entry& entry); const std::vector layers_; EntryMap values_; RandomGenerator& generator_; + RuntimeStats& stats_; }; /** diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h index fbd3af0a28c3..b1b6ba76e149 100644 --- a/source/common/singleton/threadsafe_singleton.h +++ b/source/common/singleton/threadsafe_singleton.h @@ -1,5 +1,7 @@ #pragma once +#include + #include "absl/base/call_once.h" namespace Envoy { @@ -41,4 +43,42 @@ template absl::once_flag ThreadSafeSingleton::create_once_; template T* ThreadSafeSingleton::instance_ = nullptr; +// An instance of a singleton class which has the same thread safety properties +// as ThreadSafeSingleton, but must be created via initialize prior to access. +// +// As with ThreadSafeSingleton the use of this class is generally discouraged. +template class InjectableSingleton { +public: + static T& get() { + RELEASE_ASSERT(loader_ != nullptr, "InjectableSingleton used prior to initialization"); + return *loader_; + } + + static T* getExisting() { return loader_; } + + static void initialize(T* value) { + RELEASE_ASSERT(value != nullptr, "InjectableSingleton initialized with non-null value."); + RELEASE_ASSERT(loader_ == nullptr, "InjectableSingleton initialized multiple times."); + loader_ = value; + } + static void clear() { loader_ = nullptr; } + +protected: + static T* loader_; +}; + +template T* InjectableSingleton::loader_ = nullptr; + +template class ScopedInjectableLoader { +public: + ScopedInjectableLoader(std::unique_ptr&& instance) { + instance_ = std::move(instance); + InjectableSingleton::initialize(instance_.get()); + } + ~ScopedInjectableLoader() { InjectableSingleton::clear(); } + +private: + std::unique_ptr instance_; +}; + } // namespace Envoy diff --git a/source/server/server.cc b/source/server/server.cc index 91bc53289126..5fb45c8afd08 100644 --- a/source/server/server.cc +++ b/source/server/server.cc @@ -299,7 +299,8 @@ void InstanceImpl::initialize(const Options& options, // Runtime gets initialized before the main configuration since during main configuration // load things may grab a reference to the loader for later use. - runtime_loader_ = component_factory.createRuntime(*this, initial_config); + runtime_singleton_ = std::make_unique( + component_factory.createRuntime(*this, initial_config)); // Once we have runtime we can initialize the SSL context manager. ssl_context_manager_ = @@ -521,7 +522,7 @@ void InstanceImpl::terminate() { ENVOY_FLUSH_LOG(); } -Runtime::Loader& InstanceImpl::runtime() { return *runtime_loader_; } +Runtime::Loader& InstanceImpl::runtime() { return Runtime::LoaderSingleton::get(); } void InstanceImpl::shutdown() { shutdown_ = true; diff --git a/source/server/server.h b/source/server/server.h index e7d70741daeb..1b989c205aca 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -218,7 +218,7 @@ class InstanceImpl : Logger::Loggable, public Instance { Singleton::ManagerPtr singleton_manager_; Network::ConnectionHandlerPtr handler_; Runtime::RandomGeneratorPtr random_generator_; - Runtime::LoaderPtr runtime_loader_; + std::unique_ptr runtime_singleton_; std::unique_ptr ssl_context_manager_; ProdListenerComponentFactory listener_component_factory_; ProdWorkerFactory worker_factory_; diff --git a/test/common/protobuf/BUILD b/test/common/protobuf/BUILD index 54e44139d97c..7280fe87106f 100644 --- a/test/common/protobuf/BUILD +++ b/test/common/protobuf/BUILD @@ -15,6 +15,7 @@ envoy_cc_test( deps = [ "//source/common/protobuf:utility_lib", "//source/common/stats:isolated_store_lib", + "//test/mocks/server:server_mocks", "//test/proto:deprecated_proto_cc", "//test/test_common:environment_lib", "//test/test_common:logging_lib", diff --git a/test/common/protobuf/utility_test.cc b/test/common/protobuf/utility_test.cc index 0ec2bd3a5a41..10f0e5376552 100644 --- a/test/common/protobuf/utility_test.cc +++ b/test/common/protobuf/utility_test.cc @@ -5,8 +5,10 @@ #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" +#include "common/runtime/runtime_impl.h" #include "common/stats/isolated_store_impl.h" +#include "test/mocks/server/mocks.h" #include "test/proto/deprecated.pb.h" #include "test/test_common/environment.h" #include "test/test_common/logging.h" @@ -338,70 +340,157 @@ TEST(DurationUtilTest, OutOfRange) { } } -TEST(DeprecatedFields, NoErrorWhenDeprecatedFieldsUnused) { +class DeprecatedFieldsTest : public TestBase { +protected: + DeprecatedFieldsTest() + : loader_(new Runtime::ScopedLoaderSingleton( + Runtime::LoaderPtr{new Runtime::LoaderImpl(rand_, store_, tls_)})) {} + + NiceMock tls_; + Stats::IsolatedStoreImpl store_; + Runtime::MockRandomGenerator rand_; + std::unique_ptr loader_; +}; + +TEST_F(DeprecatedFieldsTest, NoCrashIfRuntimeMissing) { + loader_.reset(); + + envoy::test::deprecation_test::Base base; + base.set_not_deprecated("foo"); + // Fatal checks for a non-deprecated field should cause no problem. + MessageUtil::checkForDeprecation(base); +} + +TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) { envoy::test::deprecation_test::Base base; base.set_not_deprecated("foo"); // Fatal checks for a non-deprecated field should cause no problem. - MessageUtil::checkForDeprecation(base, true); + MessageUtil::checkForDeprecation(base); } -TEST(DeprecatedFields, IndividualFieldDeprecated) { +TEST_F(DeprecatedFieldsTest, 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. EXPECT_LOG_CONTAINS("warning", - "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'.", - MessageUtil::checkForDeprecation(base, true)); - // Fatal checks for a deprecated field should result in an exception. + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", + MessageUtil::checkForDeprecation(base)); +} + +// Use of a deprecated and disallowed field should result in an exception. +TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowed) { + envoy::test::deprecation_test::Base base; + base.set_is_deprecated_fatal("foo"); EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'."); + MessageUtil::checkForDeprecation(base), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); } -TEST(DeprecatedFields, MessageDeprecated) { +TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowedWithRuntimeOverride) { envoy::test::deprecation_test::Base base; - base.mutable_deprecated_message(); - // Fatal checks for a present (unused) deprecated message should result in an exception. + base.set_is_deprecated_fatal("foo"); + + // Make sure this is set up right. + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); + // The config will be rejected, so the feature will not be used. + EXPECT_EQ(0, store_.gauge("runtime.deprecated_feature_use").value()); + + // Now create a new snapshot with this feature allowed. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.deprecated_features.deprecated.proto:is_deprecated_fatal", "TrUe "}}); + + // Now the same deprecation check should only trigger a warning. + EXPECT_LOG_CONTAINS( + "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'", + MessageUtil::checkForDeprecation(base)); + EXPECT_EQ(1, store_.gauge("runtime.deprecated_feature_use").value()); +} + +TEST_F(DeprecatedFieldsTest, DisallowViaRuntime) { + envoy::test::deprecation_test::Base base; + base.set_is_deprecated("foo"); + + EXPECT_LOG_CONTAINS("warning", + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", + MessageUtil::checkForDeprecation(base)); + EXPECT_EQ(1, store_.gauge("runtime.deprecated_feature_use").value()); + + // Now create a new snapshot with this feature disallowed. + Runtime::LoaderSingleton::getExisting()->mergeValues( + {{"envoy.deprecated_features.deprecated.proto:is_deprecated", " false"}}); + EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'."); + MessageUtil::checkForDeprecation(base), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'"); + EXPECT_EQ(1, store_.gauge("runtime.deprecated_feature_use").value()); +} + +// 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) { + envoy::test::deprecation_test::Base base; + base.set_is_deprecated("foo"); + base.set_is_deprecated_fatal("foo"); + EXPECT_LOG_CONTAINS( + "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated'", { + EXPECT_THROW_WITH_REGEX( + MessageUtil::checkForDeprecation(base), ProtoValidationException, + "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'"); + }); } -TEST(DeprecatedFields, InnerMessageDeprecated) { +// Present (unused) deprecated messages should be detected as deprecated. +TEST_F(DeprecatedFieldsTest, MessageDeprecated) { + envoy::test::deprecation_test::Base base; + base.mutable_deprecated_message(); + EXPECT_LOG_CONTAINS( + "warning", "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_message'", + MessageUtil::checkForDeprecation(base)); + EXPECT_EQ(1, store_.gauge("runtime.deprecated_feature_use").value()); +} + +TEST_F(DeprecatedFieldsTest, InnerMessageDeprecated) { envoy::test::deprecation_test::Base base; base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo"); - // Non-fatal checks for a deprecated field shouldn't throw an exception. - MessageUtil::checkForDeprecation(base, true); + // Checks for a non-deprecated field shouldn't trigger warnings + EXPECT_LOG_NOT_CONTAINS("warning", "Using deprecated option", + MessageUtil::checkForDeprecation(base)); base.mutable_not_deprecated_message()->set_inner_deprecated("bar"); - // Fatal checks for a deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option " - "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); + // Checks for a deprecated sub-message should result in a warning. + EXPECT_LOG_CONTAINS( + "warning", + "Using deprecated option 'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'", + MessageUtil::checkForDeprecation(base)); } // Check that repeated sub-messages get validated. -TEST(DeprecatedFields, SubMessageDeprecated) { +TEST_F(DeprecatedFieldsTest, SubMessageDeprecated) { envoy::test::deprecation_test::Base base; base.add_repeated_message(); base.add_repeated_message()->set_inner_deprecated("foo"); base.add_repeated_message(); // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX(MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option " - "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'."); + EXPECT_LOG_CONTAINS("warning", + "Using deprecated option " + "'envoy.test.deprecation_test.Base.InnerMessage.inner_deprecated'", + MessageUtil::checkForDeprecation(base)); } // Check that deprecated repeated messages trigger -TEST(DeprecatedFields, RepeatedMessageDeprecated) { +TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) { envoy::test::deprecation_test::Base base; base.add_deprecated_repeated_message(); // Fatal checks for a repeated deprecated sub-message should result in an exception. - EXPECT_THROW_WITH_REGEX( - MessageUtil::checkForDeprecation(base, false), ProtoValidationException, - "Using deprecated option 'envoy.test.deprecation_test.Base.deprecated_repeated_message'."); + EXPECT_LOG_CONTAINS("warning", + "Using deprecated option " + "'envoy.test.deprecation_test.Base.deprecated_repeated_message'", + MessageUtil::checkForDeprecation(base)); } class TimestampUtilTest : public TestBase, public ::testing::WithParamInterface {}; diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index a01659a6ec07..06651402f281 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -106,6 +106,19 @@ TEST_F(DiskBackedLoaderImplTest, All) { EXPECT_EQ(2UL, loader->snapshot().getInteger("file3", 1)); EXPECT_EQ(123UL, loader->snapshot().getInteger("file4", 1)); + // Boolean getting. + bool value; + SnapshotImpl* snapshot = reinterpret_cast(&loader->snapshot()); + + EXPECT_EQ(true, snapshot->getBoolean("file11", value)); + EXPECT_EQ(true, value); + EXPECT_EQ(true, snapshot->getBoolean("file12", value)); + EXPECT_EQ(false, value); + EXPECT_EQ(true, snapshot->getBoolean("file13", value)); + EXPECT_EQ(true, value); + // File1 is not a boolean. + EXPECT_EQ(false, snapshot->getBoolean("file1", value)); + // Files with comments. EXPECT_EQ(123UL, loader->snapshot().getInteger("file5", 1)); EXPECT_EQ("/home#about-us", loader->snapshot().get("file6")); diff --git a/test/common/runtime/test_data/root/envoy/file11 b/test/common/runtime/test_data/root/envoy/file11 new file mode 100644 index 000000000000..27ba77ddaf61 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file11 @@ -0,0 +1 @@ +true diff --git a/test/common/runtime/test_data/root/envoy/file12 b/test/common/runtime/test_data/root/envoy/file12 new file mode 100644 index 000000000000..11bdefe7b29b --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file12 @@ -0,0 +1 @@ +FaLSe diff --git a/test/common/runtime/test_data/root/envoy/file13 b/test/common/runtime/test_data/root/envoy/file13 new file mode 100644 index 000000000000..8180953eec62 --- /dev/null +++ b/test/common/runtime/test_data/root/envoy/file13 @@ -0,0 +1,2 @@ + true + diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index 3c5e64c39b66..fe70e3e56f99 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -25,8 +25,9 @@ class MockRandomGenerator : public RandomGenerator { class MockSnapshot : public Snapshot { public: MockSnapshot(); - ~MockSnapshot(); + ~MockSnapshot() override; + MOCK_CONST_METHOD1(deprecatedFeatureEnabled, bool(const std::string& key)); MOCK_CONST_METHOD2(featureEnabled, bool(const std::string& key, uint64_t default_value)); MOCK_CONST_METHOD3(featureEnabled, bool(const std::string& key, uint64_t default_value, uint64_t random_value)); diff --git a/test/proto/deprecated.proto b/test/proto/deprecated.proto index f6d965ae971c..541cab6a9584 100644 --- a/test/proto/deprecated.proto +++ b/test/proto/deprecated.proto @@ -5,12 +5,14 @@ package envoy.test.deprecation_test; message Base { string not_deprecated = 1; string is_deprecated = 2 [deprecated = true]; + string is_deprecated_fatal = 3 [deprecated = true]; message InnerMessage { string inner_not_deprecated = 1; string inner_deprecated = 2 [deprecated = true]; + string inner_deprecated_fatal = 3 [deprecated = true]; } - InnerMessage deprecated_message = 3 [deprecated = true]; - InnerMessage not_deprecated_message = 4; - repeated InnerMessage repeated_message = 5; - repeated InnerMessage deprecated_repeated_message = 6 [deprecated = true]; + InnerMessage deprecated_message = 4 [deprecated = true]; + InnerMessage not_deprecated_message = 5; + repeated InnerMessage repeated_message = 6; + repeated InnerMessage deprecated_repeated_message = 7 [deprecated = true]; } diff --git a/test/server/http/admin_test.cc b/test/server/http/admin_test.cc index b2425ecdb065..223102b6838b 100644 --- a/test/server/http/admin_test.cc +++ b/test/server/http/admin_test.cc @@ -899,9 +899,11 @@ TEST_P(AdminInstanceTest, Runtime) { auto layer1 = std::make_unique>(); auto layer2 = std::make_unique>(); std::unordered_map entries2{ - {"string_key", {"override", {}, {}}}, {"extra_key", {"bar", {}, {}}}}; + {"string_key", {"override", {}, {}, {}}}, {"extra_key", {"bar", {}, {}, {}}}}; std::unordered_map entries1{ - {"string_key", {"foo", {}, {}}}, {"int_key", {"1", 1, {}}}, {"other_key", {"bar", {}, {}}}}; + {"string_key", {"foo", {}, {}, {}}}, + {"int_key", {"1", 1, {}, {}}}, + {"other_key", {"bar", {}, {}, {}}}}; ON_CALL(*layer1, name()).WillByDefault(testing::ReturnRefOfCopy(std::string{"layer1"})); ON_CALL(*layer1, values()).WillByDefault(testing::ReturnRef(entries1));