runtime: deprecating legacy yaml (envoyproxy#38122)

Risk Level: low Testing: n/a Docs Changes: n/a Release Notes: yes fixes envoyproxy#38050 Signed-off-by: Alyssa Wilk <[email protected]>
fredyw · Jan 22, 2025 · 372f832 · 372f832
1 parent 1936f1b
commit 372f832
Show file tree

Hide file tree

Showing 5 changed files with 8 additions and 138 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -34,6 +34,9 @@ removed_config_or_runtime:
 - area: thread_local
   change: |
     Removed runtime guard  ``envoy.reloadable_features.allow_slot_destroy_on_worker_threads`` and legacy code paths.
+- area: runtime
+  change: |
+    Removed runtime flag ``envoy.reloadable_features.reject_invalid_yaml`` and legacy code paths.
 - area: dns
   change: |
     Removed runtime flag ``envoy.reloadable_features.dns_details`` and legacy code paths.

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -86,7 +86,6 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_al
 RUNTIME_GUARD(envoy_reloadable_features_quic_support_certificate_compression);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_socket_use_address_cache_for_read);
-RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml);
 RUNTIME_GUARD(envoy_reloadable_features_report_stream_reset_error_code);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_sni_in_access_log);
 RUNTIME_GUARD(envoy_reloadable_features_shadow_policy_inherit_trace_sampling);

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -284,82 +284,13 @@ bool parseEntryDoubleValue(Envoy::Runtime::Snapshot::Entry& entry) {
   return false;
 }
 
-// Handle an awful corner case where we explicitly shove a yaml percent in a proto string
-// value. Basically due to prior parsing logic we have to handle any combination
-// of numerator: #### [denominator Y] with quotes braces etc that could possibly be valid json.
-// E.g. "final_value": "{\"numerator\": 10000, \"denominator\": \"TEN_THOUSAND\"}",
-bool parseEntryFractionalPercentValue(Envoy::Runtime::Snapshot::Entry& entry) {
-  if (!absl::StrContains(entry.raw_string_value_, "numerator")) {
-    return false;
-  }
-
-  const re2::RE2 numerator_re(".*numerator[^\\d]+(\\d+)[^\\d]*");
-
-  std::string match_string;
-  if (!re2::RE2::FullMatch(entry.raw_string_value_.c_str(), numerator_re, &match_string)) {
-    return false;
-  }
-
-  uint32_t numerator;
-  if (!absl::SimpleAtoi(match_string, &numerator)) {
-    return false;
-  }
-  envoy::type::v3::FractionalPercent converted_fractional_percent;
-  converted_fractional_percent.set_numerator(numerator);
-  entry.fractional_percent_value_ = converted_fractional_percent;
-
-  if (!absl::StrContains(entry.raw_string_value_, "denominator")) {
-    return true;
-  }
-  if (absl::StrContains(entry.raw_string_value_, "TEN_THOUSAND")) {
-    entry.fractional_percent_value_->set_denominator(
-        envoy::type::v3::FractionalPercent::TEN_THOUSAND);
-  }
-  if (absl::StrContains(entry.raw_string_value_, "MILLION")) {
-    entry.fractional_percent_value_->set_denominator(envoy::type::v3::FractionalPercent::MILLION);
-  }
-  return true;
-}
-
-// Handle corner cases in non-yaml parsing: mixed case strings aren't parsed as booleans.
-bool parseEntryBooleanValue(Envoy::Runtime::Snapshot::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;
-}
-
 void SnapshotImpl::addEntry(Snapshot::EntryMap& values, const std::string& key,
                             const ProtobufWkt::Value& value, absl::string_view raw_string) {
-  const char* error_message = nullptr;
-  values.emplace(key, SnapshotImpl::createEntry(value, raw_string, error_message));
-  if (error_message != nullptr) {
-    IS_ENVOY_BUG(
-        absl::StrCat(error_message, "\n[ key:", key, ", value: ", value.DebugString(), "]"));
-  }
+  values.emplace(key, SnapshotImpl::createEntry(value, raw_string));
 }
 
-static const char* kBoolError =
-    "Runtime YAML appears to be setting booleans as strings. Support for this is planned "
-    "to removed in an upcoming release. If you can not fix your YAML and need this to continue "
-    "working "
-    "please ping on https://github.com/envoyproxy/envoy/issues/27434";
-static const char* kFractionError =
-    "Runtime YAML appears to be setting fractions as strings. Support for this is planned "
-    "to removed in an upcoming release. If you can not fix your YAML and need this to continue "
-    "working "
-    "please ping on https://github.com/envoyproxy/envoy/issues/27434";
-
 SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value,
-                                              absl::string_view raw_string,
-                                              const char*& error_message) {
+                                              absl::string_view raw_string) {
   Entry entry;
   entry.raw_string_value_ = value.string_value();
   if (!raw_string.empty()) {
@@ -392,14 +323,6 @@ SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value,
     break;
   case ProtobufWkt::Value::kStringValue:
     parseEntryDoubleValue(entry);
-    if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reject_invalid_yaml")) {
-      if (parseEntryBooleanValue(entry)) {
-        error_message = kBoolError;
-      }
-      if (parseEntryFractionalPercentValue(entry)) {
-        error_message = kFractionError;
-      }
-    }
     break;
   default:
     break;

diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h
@@ -88,8 +88,7 @@ class SnapshotImpl : public Snapshot, Logger::Loggable<Logger::Id::runtime> {
 
   const EntryMap& values() const;
 
-  static Entry createEntry(const ProtobufWkt::Value& value, absl::string_view raw_string,
-                           const char*& error_message);
+  static Entry createEntry(const ProtobufWkt::Value& value, absl::string_view raw_string);
   static void addEntry(Snapshot::EntryMap& values, const std::string& key,
                        const ProtobufWkt::Value& value, absl::string_view raw_string = "");
 

diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc
@@ -33,7 +33,6 @@
 #include "quiche/common/platform/api/quiche_flags.h"
 #endif
 ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_boolean_to_string_fix);
-ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_reject_invalid_yaml);
 
 using testing::_;
 using testing::Invoke;
@@ -551,7 +550,6 @@ class StaticLoaderImplTest : public LoaderImplTest {
     THROW_IF_NOT_OK(loader.status());
     loader_ = std::move(loader.value());
   }
-  void testAllTheThings(bool allow_invalid_yaml);
 
   ProtobufWkt::Struct base_;
 };
@@ -608,7 +606,7 @@ TEST_F(StaticLoaderImplTest, ProtoParsingInvalidField) {
   EXPECT_THROW_WITH_MESSAGE(setup(), EnvoyException, "Invalid runtime entry value for file0");
 }
 
-void StaticLoaderImplTest::testAllTheThings(bool allow_invalid_yaml) {
+TEST_F(StaticLoaderImplTest, ProtoParsing) {
   // Validate proto parsing sanity.
   base_ = TestUtility::parseYaml<ProtobufWkt::Struct>(R"EOF(
     file1: hello override
@@ -757,62 +755,10 @@ void StaticLoaderImplTest::testAllTheThings(bool allow_invalid_yaml) {
   EXPECT_EQ(22, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(2, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
 
-  const char* error = nullptr;
   // While null values are generally filtered out by walkProtoValue, test manually.
   ProtobufWkt::Value empty_value;
   const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-      .createEntry(empty_value, "", error);
-  if (allow_invalid_yaml) {
-    // Make sure the hacky fractional percent function works.
-    ProtobufWkt::Value fractional_value;
-    fractional_value.set_string_value(" numerator:  11 ");
-    auto entry = const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-                     .createEntry(fractional_value, "", error);
-    ASSERT_TRUE(entry.fractional_percent_value_.has_value());
-    EXPECT_EQ(entry.fractional_percent_value_->denominator(),
-              envoy::type::v3::FractionalPercent::HUNDRED);
-    EXPECT_EQ(entry.fractional_percent_value_->numerator(), 11);
-
-    // Make sure the hacky percent function works with numerator and denominator
-    fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"TEN_THOUSAND\"}");
-    entry = const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-                .createEntry(fractional_value, "", error);
-    ASSERT_TRUE(entry.fractional_percent_value_.has_value());
-    EXPECT_EQ(entry.fractional_percent_value_->denominator(),
-              envoy::type::v3::FractionalPercent::TEN_THOUSAND);
-    EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000);
-
-    // Make sure the hacky fractional percent function works with million
-    fractional_value.set_string_value("{\"numerator\": 10000, \"denominator\": \"MILLION\"}");
-    entry = const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-                .createEntry(fractional_value, "", error);
-    ASSERT_TRUE(entry.fractional_percent_value_.has_value());
-    EXPECT_EQ(entry.fractional_percent_value_->denominator(),
-              envoy::type::v3::FractionalPercent::MILLION);
-    EXPECT_EQ(entry.fractional_percent_value_->numerator(), 10000);
-
-    // Test atoi failure for the hacky fractional percent value function.
-    fractional_value.set_string_value(" numerator:  1.1 ");
-    entry = const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-                .createEntry(fractional_value, "", error);
-    ASSERT_FALSE(entry.fractional_percent_value_.has_value());
-
-    // Test legacy malformed boolean support
-    ProtobufWkt::Value boolean_value;
-    boolean_value.set_string_value("FaLsE");
-    entry = const_cast<SnapshotImpl&>(dynamic_cast<const SnapshotImpl&>(loader_->snapshot()))
-                .createEntry(boolean_value, "", error);
-    ASSERT_TRUE(entry.bool_value_.has_value());
-    ASSERT_FALSE(entry.bool_value_.value());
-  }
-}
-
-TEST_F(StaticLoaderImplTest, ProtoParsing) { testAllTheThings(false); }
-
-TEST_F(StaticLoaderImplTest, ProtoParsingLegacy) {
-  absl::SetFlag(&FLAGS_envoy_reloadable_features_reject_invalid_yaml, false);
-
-  testAllTheThings(true);
+      .createEntry(empty_value, "");
 }
 
 TEST_F(StaticLoaderImplTest, InvalidNumerator) {