runtime: rejecting invalid yaml

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk · Jun 10, 2024 · 2ce2ab8 · 2ce2ab8
1 parent 9bfb2c9
commit 2ce2ab8
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 48 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -20,6 +20,12 @@ behavior_changes:
   change: |
     Passes HTTP/2 DATA frames through a different codec API. This behavior can be temporarily disabled by setting the runtime
     feature ``envoy.reloadable_features.http2_use_visitor_for_data`` to false.
+- area: runtime
+  change: |
+    Rejecting invalid yaml. This has been an ENVOY_BUG linked to https://github.com/envoyproxy/envoy/issues/27434
+    for over a year with no hard-blockers so should be Ok. This behavior can be temporarily disabled by setting
+    the runtime feature ``envoy.reloadable_features.reject_invalid_yaml`` to false but the runtime guard must be
+    parsed before any invalid yaml.
 - area: proxy_protocol
   change: |
     Populate typed metadata by default in proxy protocol listener. Typed metadata can be consumed as

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -84,6 +84,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_receive_ecn);
 // @danzh2010 or @RyanTheOptimist before removing.
 RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_all_clients);
 RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets);
+RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_te);
 RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value);
 RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request);

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -386,11 +386,13 @@ SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value,
     break;
   case ProtobufWkt::Value::kStringValue:
     parseEntryDoubleValue(entry);
-    if (parseEntryBooleanValue(entry)) {
-      error_message = kBoolError;
-    }
-    if (parseEntryFractionalPercentValue(entry)) {
-      error_message = kFractionError;
+    if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.reject_invalid_yaml")) {
+      if (parseEntryBooleanValue(entry)) {
+        error_message = kBoolError;
+      }
+      if (parseEntryFractionalPercentValue(entry)) {
+        error_message = kFractionError;
+      }
     }
   default:
     break;

diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc
@@ -1,6 +1,7 @@
 #include <memory>
 #include <string>
 
+#include "absl/flags/declare.h"
 #include "envoy/config/bootstrap/v3/bootstrap.pb.h"
 #include "envoy/config/core/v3/config_source.pb.h"
 #include "envoy/service/discovery/v3/discovery.pb.h"
@@ -31,6 +32,7 @@
 #ifdef ENVOY_ENABLE_QUIC
 #include "quiche/common/platform/api/quiche_flags.h"
 #endif
+ABSL_DECLARE_FLAG(bool, envoy_reloadable_features_reject_invalid_yaml);
 
 using testing::_;
 using testing::Invoke;
@@ -548,6 +550,7 @@ class StaticLoaderImplTest : public LoaderImplTest {
     THROW_IF_NOT_OK(loader.status());
     loader_ = std::move(loader.value());
   }
+  void testAllTheThings(bool allow_invalid_yaml);
 
   ProtobufWkt::Struct base_;
 };
@@ -604,8 +607,8 @@ TEST_F(StaticLoaderImplTest, ProtoParsingInvalidField) {
   EXPECT_THROW_WITH_MESSAGE(setup(), EnvoyException, "Invalid runtime entry value for file0");
 }
 
-// Validate proto parsing sanity.
-TEST_F(StaticLoaderImplTest, ProtoParsing) {
+void StaticLoaderImplTest::testAllTheThings(bool allow_invalid_yaml) {
+  // Validate proto parsing sanity.
   base_ = TestUtility::parseYaml<ProtobufWkt::Struct>(R"EOF(
     file1: hello override
     file2: world
@@ -758,48 +761,57 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
   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);
 
-  // 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());
+  testAllTheThings(true);
 }
 
 TEST_F(StaticLoaderImplTest, InvalidNumerator) {