runtime: remove yaml from the critical path (envoyproxy#26451)

Avoiding a yaml translation step when loading runtime . This also allows turning up e2e !yaml testing for Envoy Mobile (with limited API calls) so adding in regression testing via CI as well. Risk Level: high Testing: existing tests, new unit tests Docs Changes: n/a Release Notes: n/a part of envoyproxy#26450 Signed-off-by: Alyssa Wilk <[email protected]>
pchaseh · Apr 26, 2023 · 6d6661c · 6d6661c
1 parent a667db0
commit 6d6661c
Show file tree

Hide file tree

Showing 7 changed files with 152 additions and 90 deletions.
diff --git a/.github/workflows/mobile-compile_time_options.yml b/.github/workflows/mobile-compile_time_options.yml
@@ -112,4 +112,14 @@ jobs:
           --define=google_grpc=disabled \
           --define=envoy_yaml=disabled \
           --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor= \
-          //:android_dist
+      //:android_dist && ./bazelw test \
+          --fat_apk_cpu=x86_64 \
+          --define=admin_html=enabled \
+          --define=envoy_mobile_request_compression=disabled \
+          --define=envoy_enable_http_datagrams=disabled \
+          --define=google_grpc=disabled \
+          --define=envoy_yaml=disabled \
+          --@com_envoyproxy_protoc_gen_validate//bazel:template-flavor= \
+      //test/java/integration:android_engine_socket_tag_test \
+      //test/java/integration:android_engine_start_test \
+      //test/java/io/envoyproxy/envoymobile/utilities:certificate_verification_tests
diff --git a/mobile/library/cc/BUILD b/mobile/library/cc/BUILD
@@ -22,7 +22,6 @@ envoy_cc_library(
     deps = [
         ":envoy_engine_cc_lib_no_stamp",
         "@envoy//source/common/common:assert_lib",
-        "@envoy_mobile//library/common/types:matcher_data_lib",
         "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto",
         "@envoy_api//envoy/config/metrics/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/compression/brotli/decompressor/v3:pkg_cc_proto",
@@ -41,6 +40,7 @@ envoy_cc_library(
         "@envoy_mobile//library/common/extensions/filters/http/local_error:filter_cc_proto",
         "@envoy_mobile//library/common/extensions/filters/http/network_configuration:filter_cc_proto",
         "@envoy_mobile//library/common/extensions/filters/http/socket_tag:filter_cc_proto",
+        "@envoy_mobile//library/common/types:matcher_data_lib",
     ] + envoy_select_envoy_mobile_request_compression(
         [
             "@envoy_api//envoy/extensions/compression/brotli/compressor/v3:pkg_cc_proto",

diff --git a/mobile/library/common/jni/BUILD b/mobile/library/common/jni/BUILD
@@ -171,8 +171,8 @@ cc_library(
         "android_jni_utility.h",
     ],
     deps = [
-        "@envoy//source/common/common:assert_lib",
         "//library/common/types:c_types_lib",
+        "@envoy//source/common/common:assert_lib",
     ] + select({
         "@envoy//bazel:android": [
             ":jni_support_lib",

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -222,88 +222,126 @@ SnapshotImpl::SnapshotImpl(Random::RandomGenerator& generator, RuntimeStats& sta
   stats.num_keys_.set(values_.size());
 }
 
-SnapshotImpl::Entry SnapshotImpl::createEntry(const std::string& value) {
-  Entry entry;
-  entry.raw_string_value_ = value;
-
-  // As a perf optimization, attempt to parse the entry's string and store it inside the struct. If
-  // we don't succeed that's fine.
-  resolveEntryType(entry);
-
-  return entry;
-}
+void parseFractionValue(SnapshotImpl::Entry& entry, const ProtobufWkt::Struct& value) {
+  envoy::type::v3::FractionalPercent percent;
+  static_assert(envoy::type::v3::FractionalPercent::MILLION ==
+                envoy::type::v3::FractionalPercent::DenominatorType_MAX);
+  percent.set_denominator(envoy::type::v3::FractionalPercent::HUNDRED);
+  for (const auto& f : value.fields()) {
+    if (f.first == "numerator") {
+      if (f.second.has_number_value()) {
+        percent.set_numerator(f.second.number_value());
+      }
+    } else if (f.first == "denominator" && f.second.has_string_value()) {
+      if (f.second.string_value() == "HUNDRED") {
+        percent.set_denominator(envoy::type::v3::FractionalPercent::HUNDRED);
+      } else if (f.second.string_value() == "TEN_THOUSAND") {
+        percent.set_denominator(envoy::type::v3::FractionalPercent::TEN_THOUSAND);
+      } else if (f.second.string_value() == "MILLION") {
+        percent.set_denominator(envoy::type::v3::FractionalPercent::MILLION);
+      } else {
+        return;
+      }
+    } else {
+      return;
+    }
+  }
 
-SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value) {
-  // This isn't the smartest way to do it; we're round-tripping via YAML, this should be optimized
-  // if runtime parsing becomes performance sensitive.
-#ifdef ENVOY_ENABLE_YAML
-  return createEntry(MessageUtil::getYamlStringFromMessage(value, false, false));
-#else
-  IS_ENVOY_BUG("Runtime loading requires YAML support");
-  UNREFERENCED_PARAMETER(value);
-  return Entry();
-#endif
+  entry.fractional_percent_value_ = percent;
 }
 
-bool SnapshotImpl::parseEntryBooleanValue(Entry& entry) {
-  absl::string_view stripped = entry.raw_string_value_;
-  stripped = absl::StripAsciiWhitespace(stripped);
-
-  uint64_t parse_int;
-  if (absl::SimpleAtoi(stripped, &parse_int)) {
-    entry.bool_value_ = (parse_int != 0);
-    // This is really an integer, so return false here not because of failure, but so we continue to
-    // parse doubles/int.
-    return false;
-  } else if (absl::EqualsIgnoreCase(stripped, "true")) {
-    entry.bool_value_ = true;
-    return true;
-  } else if (absl::EqualsIgnoreCase(stripped, "false")) {
-    entry.bool_value_ = false;
-    return true;
+void setNumberValue(Envoy::Runtime::Snapshot::Entry& entry, double value) {
+  entry.double_value_ = value;
+  if (value < std::numeric_limits<int>::max() && value == static_cast<int>(value)) {
+    entry.bool_value_ = value != 0;
+  }
+  if (entry.double_value_ >= 0 && entry.double_value_ <= std::numeric_limits<uint64_t>::max()) {
+    // Valid uint values will always be parseable as doubles, so we assign the value to both the
+    // uint and double fields. In cases where the value is something like "3.1", we will floor the
+    // number by casting it to a uint and assigning the uint value.
+    entry.uint_value_ = entry.double_value_;
   }
-  return false;
 }
 
-bool SnapshotImpl::parseEntryDoubleValue(Entry& entry) {
+// Handle corner cases in parsing: negatives and decimals aren't always parsed as doubles.
+void parseEntryDoubleValue(Envoy::Runtime::Snapshot::Entry& entry) {
   double converted_double;
   if (absl::SimpleAtod(entry.raw_string_value_, &converted_double)) {
-    entry.double_value_ = converted_double;
-    return true;
+    setNumberValue(entry, converted_double);
   }
-  return false;
 }
 
-void SnapshotImpl::parseEntryFractionalPercentValue(Entry& entry) {
-  envoy::type::v3::FractionalPercent converted_fractional_percent;
-  TRY_ASSERT_MAIN_THREAD {
-#ifdef ENVOY_ENABLE_YAML
-    MessageUtil::loadFromYamlAndValidate(entry.raw_string_value_, converted_fractional_percent,
-                                         ProtobufMessage::getStrictValidationVisitor());
-#endif
+// Handle an absolutely awful corner case where we explicitly shove a yaml percent in a proto string
+// value.
+void parseEntryFractionalPercentValue(Envoy::Runtime::Snapshot::Entry& entry) {
+  if (!absl::StrContains(entry.raw_string_value_, "numerator:")) {
+    return;
   }
+  envoy::type::v3::FractionalPercent converted_fractional_percent;
+  TRY_ASSERT_MAIN_THREAD { entry.fractional_percent_value_ = converted_fractional_percent; }
   END_TRY
   catch (const ProtoValidationException& ex) {
-    ENVOY_LOG(error, "unable to validate fraction percent runtime proto: {}", ex.what());
     return;
   }
-  catch (const EnvoyException& ex) {
-    // An EnvoyException is thrown when we try to parse a bogus string as a protobuf. This is fine,
-    // since there was no expectation that the raw string was a valid proto.
-    return;
+}
+
+// Handle corner cases in non-yaml parsing: mixed case strings aren't parsed as booleans.
+void 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;
+  } else if (absl::EqualsIgnoreCase(stripped, "false")) {
+    entry.bool_value_ = false;
+  }
+}
+
+SnapshotImpl::Entry SnapshotImpl::createEntry(const ProtobufWkt::Value& value,
+                                              absl::string_view raw_string) {
+  Entry entry;
+  switch (value.kind_case()) {
+  case ProtobufWkt::Value::kNumberValue:
+    setNumberValue(entry, value.number_value());
+    break;
+  case ProtobufWkt::Value::kBoolValue:
+    entry.bool_value_ = value.bool_value();
+    break;
+  case ProtobufWkt::Value::kStructValue:
+    parseFractionValue(entry, value.struct_value());
+    break;
+  case ProtobufWkt::Value::kStringValue:
+    entry.raw_string_value_ = value.string_value();
+    parseEntryDoubleValue(entry);
+    // TODO(alyssawilk) after this PR lands and sticks, ENVOY_BUG these
+    // functions and see if we can remove the special casing.
+    parseEntryBooleanValue(entry);
+    parseEntryFractionalPercentValue(entry);
+    if (!raw_string.empty()) {
+      entry.raw_string_value_ = raw_string;
+    }
+  default:
+    break;
   }
 
-  entry.fractional_percent_value_ = converted_fractional_percent;
+  return entry;
 }
 
 void AdminLayer::mergeValues(const absl::node_hash_map<std::string, std::string>& values) {
+#ifdef ENVOY_ENABLE_YAML
   for (const auto& kv : values) {
     values_.erase(kv.first);
     if (!kv.second.empty()) {
-      values_.emplace(kv.first, SnapshotImpl::createEntry(kv.second));
+      values_.emplace(kv.first,
+                      SnapshotImpl::createEntry(ValueUtil::loadFromYaml(kv.second), kv.second));
     }
   }
   stats_.admin_overrides_active_.set(values_.empty() ? 0 : 1);
+#else
+  IS_ENVOY_BUG("Runtime admin reload requires YAML support");
+  UNREFERENCED_PARAMETER(values);
+  return;
+#endif
 }
 
 DiskLayer::DiskLayer(absl::string_view name, const std::string& path, Api::Api& api)
@@ -363,7 +401,14 @@ void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix
       // Separate erase/insert calls required due to the value type being constant; this prevents
       // the use of the [] operator. Can leverage insert_or_assign in C++17 in the future.
       values_.erase(full_prefix);
-      values_.insert({full_prefix, SnapshotImpl::createEntry(value)});
+#ifdef ENVOY_ENABLE_YAML
+      values_.insert(
+          {full_prefix, SnapshotImpl::createEntry(ValueUtil::loadFromYaml(value), value)});
+#else
+      IS_ENVOY_BUG("Runtime admin reload requires YAML support");
+      UNREFERENCED_PARAMETER(value);
+      return;
+#endif
     }
   }
 }
@@ -383,7 +428,7 @@ void ProtoLayer::walkProtoValue(const ProtobufWkt::Value& v, const std::string&
     throw EnvoyException(absl::StrCat("Invalid runtime entry value for ", prefix));
     break;
   case ProtobufWkt::Value::kStringValue:
-    values_.emplace(prefix, SnapshotImpl::createEntry(v.string_value()));
+    values_.emplace(prefix, SnapshotImpl::createEntry(v));
     break;
   case ProtobufWkt::Value::kNumberValue:
   case ProtobufWkt::Value::kBoolValue:

diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h
@@ -87,31 +87,9 @@ class SnapshotImpl : public Snapshot, Logger::Loggable<Logger::Id::runtime> {
 
   const EntryMap& values() const;
 
-  static Entry createEntry(const std::string& value);
-  static Entry createEntry(const ProtobufWkt::Value& value);
+  static Entry createEntry(const ProtobufWkt::Value& value, absl::string_view raw_string = "");
 
 private:
-  static void resolveEntryType(Entry& entry) {
-    if (parseEntryBooleanValue(entry)) {
-      return;
-    }
-
-    if (parseEntryDoubleValue(entry) && entry.double_value_ >= 0 &&
-        entry.double_value_ <= std::numeric_limits<uint64_t>::max()) {
-      // Valid uint values will always be parseable as doubles, so we assign the value to both the
-      // uint and double fields. In cases where the value is something like "3.1", we will floor the
-      // number by casting it to a uint and assigning the uint value.
-      entry.uint_value_ = entry.double_value_;
-      return;
-    }
-
-    parseEntryFractionalPercentValue(entry);
-  }
-
-  static bool parseEntryBooleanValue(Entry& entry);
-  static bool parseEntryDoubleValue(Entry& entry);
-  static void parseEntryFractionalPercentValue(Entry& entry);
-
   const std::vector<OverrideLayerConstPtr> layers_;
   EntryMap values_;
   Random::RandomGenerator& generator_;

diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc
@@ -578,6 +578,11 @@ TEST_F(StaticLoaderImplTest, RemovedFlags) {
   EXPECT_ENVOY_BUG(setup(), "envoy.reloadable_features.removed_foo");
 }
 
+TEST_F(StaticLoaderImplTest, ProtoParsingInvalidField) {
+  base_ = TestUtility::parseYaml<ProtobufWkt::Struct>("file0:");
+  EXPECT_THROW_WITH_MESSAGE(setup(), EnvoyException, "Invalid runtime entry value for file0");
+}
+
 // Validate proto parsing sanity.
 TEST_F(StaticLoaderImplTest, ProtoParsing) {
   base_ = TestUtility::parseYaml<ProtobufWkt::Struct>(R"EOF(
@@ -588,6 +593,12 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
     file8:
       numerator: 52
       denominator: HUNDRED
+    file80:
+      numerator: 52
+      denominator: TEN_THOUSAND
+    file800:
+      numerator: 52
+      denominator: MILLION
     file9:
       numerator: 100
       denominator: NONSENSE
@@ -666,10 +677,14 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
   // Fractional percent feature enablement
   envoy::type::v3::FractionalPercent fractional_percent;
   fractional_percent.set_numerator(5);
-  fractional_percent.set_denominator(envoy::type::v3::FractionalPercent::TEN_THOUSAND);
+  fractional_percent.set_denominator(envoy::type::v3::FractionalPercent::MILLION);
 
   EXPECT_CALL(generator_, random()).WillOnce(Return(50));
   EXPECT_TRUE(loader_->snapshot().featureEnabled("file8", fractional_percent)); // valid data
+  EXPECT_CALL(generator_, random()).WillOnce(Return(50));
+  EXPECT_TRUE(loader_->snapshot().featureEnabled("file80", fractional_percent)); // valid data
+  EXPECT_CALL(generator_, random()).WillOnce(Return(50));
+  EXPECT_TRUE(loader_->snapshot().featureEnabled("file800", fractional_percent)); // valid data
   EXPECT_CALL(generator_, random()).WillOnce(Return(60));
   EXPECT_FALSE(loader_->snapshot().featureEnabled("file8", fractional_percent)); // valid data
 
@@ -718,8 +733,13 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) {
 
   EXPECT_EQ(0, store_.counter("runtime.load_error").value());
   EXPECT_EQ(1, store_.counter("runtime.load_success").value());
-  EXPECT_EQ(21, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
+  EXPECT_EQ(23, store_.gauge("runtime.num_keys", Stats::Gauge::ImportMode::NeverImport).value());
   EXPECT_EQ(2, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value());
+
+  // 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);
 }
 
 TEST_F(StaticLoaderImplTest, InvalidNumerator) {

diff --git a/test/integration/utility.cc b/test/integration/utility.cc
@@ -128,7 +128,6 @@ class TestConnectionCallbacks : public Network::ConnectionCallbacks {
   bool connected_{false};
 };
 
-#ifdef ENVOY_ENABLE_YAML
 Network::UpstreamTransportSocketFactoryPtr
 IntegrationUtil::createQuicUpstreamTransportSocketFactory(Api::Api& api, Stats::Store& store,
                                                           Ssl::ContextManager& context_manager,
@@ -140,9 +139,15 @@ IntegrationUtil::createQuicUpstreamTransportSocketFactory(Api::Api& api, Stats::
   envoy::extensions::transport_sockets::quic::v3::QuicUpstreamTransport
       quic_transport_socket_config;
   auto* tls_context = quic_transport_socket_config.mutable_upstream_tls_context();
+#ifdef ENVOY_ENABLE_YAML
   initializeUpstreamTlsContextConfig(
       Ssl::ClientSslTransportOptions().setAlpn(true).setSan(san_to_match).setSni("lyft.com"),
       *tls_context);
+#else
+  UNREFERENCED_PARAMETER(tls_context);
+  UNREFERENCED_PARAMETER(san_to_match);
+  RELEASE_ASSERT(0, "unsupported");
+#endif // ENVOY_ENABLE_YAML
 
   envoy::config::core::v3::TransportSocket message;
   message.mutable_typed_config()->PackFrom(quic_transport_socket_config);
@@ -200,9 +205,14 @@ IntegrationUtil::makeSingleRequest(const Network::Address::InstanceConstSharedPt
   Network::TransportSocketOptionsConstSharedPtr options;
 
   std::shared_ptr<Upstream::MockClusterInfo> cluster{new NiceMock<Upstream::MockClusterInfo>()};
-  Upstream::HostDescriptionConstSharedPtr host_description{Upstream::makeTestHostDescription(
-      cluster, fmt::format("{}://127.0.0.1:80", (type == Http::CodecType::HTTP3 ? "udp" : "tcp")),
-      time_system)};
+  Upstream::HostDescriptionConstSharedPtr host_description =
+      std::make_shared<Upstream::HostDescriptionImpl>(
+          cluster, "",
+          Network::Utility::resolveUrl(
+              fmt::format("{}://127.0.0.1:80", (type == Http::CodecType::HTTP3 ? "udp" : "tcp"))),
+          nullptr, envoy::config::core::v3::Locality().default_instance(),
+          envoy::config::endpoint::v3::Endpoint::HealthCheckConfig::default_instance(), 0,
+          time_system);
 
   if (type <= Http::CodecType::HTTP2) {
     Http::CodecClientProd client(type,
@@ -259,7 +269,6 @@ IntegrationUtil::makeSingleRequest(uint32_t port, const std::string& method, con
       fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(ip_version), port));
   return makeSingleRequest(addr, method, url, body, type, host, content_type);
 }
-#endif // ENVOY_ENABLE_YAML
 
 RawConnectionDriver::RawConnectionDriver(uint32_t port, Buffer::Instance& request_data,
                                          ReadCallback response_data_callback,