xds: internal refactor using absl::span instead of Protobuf::Repeated…

…PtrField (#36316) Signed-off-by: Adi Suissa-Peleg <[email protected]>
envoyproxy · Oct 9, 2024 · e0ac5ac · e0ac5ac
1 parent 3732616
commit e0ac5ac
Show file tree

Hide file tree

Showing 8 changed files with 34 additions and 32 deletions.
diff --git a/envoy/config/subscription.h b/envoy/config/subscription.h
@@ -191,10 +191,10 @@ class UntypedConfigUpdateCallbacks {
    * being updated. Accepted changes have their version_info reflected in subsequent
    * requests.
    */
-  virtual void onConfigUpdate(
-      const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& added_resources,
-      const Protobuf::RepeatedPtrField<std::string>& removed_resources,
-      const std::string& system_version_info) PURE;
+  virtual void
+  onConfigUpdate(absl::Span<const envoy::service::discovery::v3::Resource* const> added_resources,
+                 const Protobuf::RepeatedPtrField<std::string>& removed_resources,
+                 const std::string& system_version_info) PURE;
 
   /**
    * Called when either the Subscription is unable to fetch a config update or when onConfigUpdate

diff --git a/envoy/config/xds_config_tracker.h b/envoy/config/xds_config_tracker.h
@@ -60,10 +60,10 @@ class XdsConfigTracker {
    * @param added_resources A list of decoded resources to add to the current state.
    * @param removed_resources A list of resources to remove from the current state.
    */
-  virtual void onConfigAccepted(
-      const absl::string_view type_url,
-      const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& added_resources,
-      const Protobuf::RepeatedPtrField<std::string>& removed_resources) PURE;
+  virtual void
+  onConfigAccepted(const absl::string_view type_url,
+                   absl::Span<const envoy::service::discovery::v3::Resource* const> added_resources,
+                   const Protobuf::RepeatedPtrField<std::string>& removed_resources) PURE;
 
   /**
    * Invoked when xds configs are rejected during xDS ingestion.

diff --git a/source/extensions/config_subscription/grpc/delta_subscription_state.cc b/source/extensions/config_subscription/grpc/delta_subscription_state.cc
@@ -218,12 +218,14 @@ void DeltaSubscriptionState::handleGoodResponse(
     }
   }
 
-  watch_map_.onConfigUpdate(non_heartbeat_resources, message.removed_resources(),
+  absl::Span<const envoy::service::discovery::v3::Resource* const> non_heartbeat_resources_span =
+      absl::MakeConstSpan(non_heartbeat_resources.data(), non_heartbeat_resources.size());
+  watch_map_.onConfigUpdate(non_heartbeat_resources_span, message.removed_resources(),
                             message.system_version_info());
 
   // Processing point when resources are successfully ingested.
   if (xds_config_tracker_.has_value()) {
-    xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources,
+    xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources_span,
                                           message.removed_resources());
   }
 

diff --git a/source/extensions/config_subscription/grpc/watch_map.cc b/source/extensions/config_subscription/grpc/watch_map.cc
@@ -216,7 +216,7 @@ void WatchMap::onConfigUpdate(const Protobuf::RepeatedPtrField<ProtobufWkt::Any>
 }
 
 void WatchMap::onConfigUpdate(
-    const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& added_resources,
+    absl::Span<const envoy::service::discovery::v3::Resource* const> added_resources,
     const Protobuf::RepeatedPtrField<std::string>& removed_resources,
     const std::string& system_version_info) {
   // Track any removals triggered by earlier watch updates.
@@ -232,15 +232,15 @@ void WatchMap::onConfigUpdate(
   // resources the watch map is interested in. Reserve the correct amount of
   // space for the vector for the good case.
   decoded_resources.reserve(added_resources.size());
-  for (const auto& r : added_resources) {
-    const absl::flat_hash_set<Watch*>& interested_in_r = watchesInterestedIn(r.name());
+  for (const auto* r : added_resources) {
+    const absl::flat_hash_set<Watch*>& interested_in_r = watchesInterestedIn(r->name());
     // If there are no watches, then we don't need to decode. If there are watches, they should all
     // be for the same resource type, so we can just use the callbacks of the first watch to decode.
     if (interested_in_r.empty()) {
       continue;
     }
     decoded_resources.emplace_back(
-        new DecodedResourceImpl((*interested_in_r.begin())->resource_decoder_, r));
+        new DecodedResourceImpl((*interested_in_r.begin())->resource_decoder_, *r));
     for (const auto& interested_watch : interested_in_r) {
       per_watch_added[interested_watch].emplace_back(*decoded_resources.back());
     }

diff --git a/source/extensions/config_subscription/grpc/watch_map.h b/source/extensions/config_subscription/grpc/watch_map.h
@@ -105,10 +105,10 @@ class WatchMap : public UntypedConfigUpdateCallbacks, public Logger::Loggable<Lo
   void onConfigUpdate(const std::vector<DecodedResourcePtr>& resources,
                       const std::string& version_info) override;
 
-  void onConfigUpdate(
-      const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& added_resources,
-      const Protobuf::RepeatedPtrField<std::string>& removed_resources,
-      const std::string& system_version_info) override;
+  void
+  onConfigUpdate(absl::Span<const envoy::service::discovery::v3::Resource* const> added_resources,
+                 const Protobuf::RepeatedPtrField<std::string>& removed_resources,
+                 const std::string& system_version_info) override;
   void onConfigUpdateFailed(ConfigUpdateFailureReason reason, const EnvoyException* e) override;
 
   WatchMap(const WatchMap&) = delete;

diff --git a/source/extensions/config_subscription/grpc/xds_mux/delta_subscription_state.cc b/source/extensions/config_subscription/grpc/xds_mux/delta_subscription_state.cc
@@ -188,12 +188,14 @@ void DeltaSubscriptionState::handleGoodResponse(
     }
   }
 
-  callbacks().onConfigUpdate(non_heartbeat_resources, message.removed_resources(),
+  absl::Span<const envoy::service::discovery::v3::Resource* const> non_heartbeat_resources_span =
+      absl::MakeConstSpan(non_heartbeat_resources.data(), non_heartbeat_resources.size());
+  callbacks().onConfigUpdate(non_heartbeat_resources_span, message.removed_resources(),
                              message.system_version_info());
 
   // Processing point when resources are successfully ingested.
   if (xds_config_tracker_.has_value()) {
-    xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources,
+    xds_config_tracker_->onConfigAccepted(message.type_url(), non_heartbeat_resources_span,
                                           message.removed_resources());
   }
 

diff --git a/test/integration/xds_config_tracker_integration_test.cc b/test/integration/xds_config_tracker_integration_test.cc
@@ -68,15 +68,14 @@ class TestXdsConfigTracker : public Config::XdsConfigTracker {
     }
   }
 
-  void onConfigAccepted(
-      const absl::string_view,
-      const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& resources,
-      const Protobuf::RepeatedPtrField<std::string>&) override {
+  void onConfigAccepted(const absl::string_view,
+                        absl::Span<const envoy::service::discovery::v3::Resource* const> resources,
+                        const Protobuf::RepeatedPtrField<std::string>&) override {
     stats_.on_config_accepted_.inc();
     test::envoy::config::xds::TestTrackerMetadata test_metadata;
-    for (const auto& resource : resources) {
-      if (resource.has_metadata()) {
-        const auto& config_typed_metadata = resource.metadata().typed_filter_metadata();
+    for (const auto* resource : resources) {
+      if (resource->has_metadata()) {
+        const auto& config_typed_metadata = resource->metadata().typed_filter_metadata();
         if (const auto& metadata_it = config_typed_metadata.find(kTestKey);
             metadata_it != config_typed_metadata.end()) {
           const auto status = Envoy::MessageUtil::unpackTo(metadata_it->second, test_metadata);

diff --git a/test/mocks/config/mocks.h b/test/mocks/config/mocks.h
@@ -56,11 +56,10 @@ class MockUntypedConfigUpdateCallbacks : public UntypedConfigUpdateCallbacks {
   MOCK_METHOD(void, onConfigUpdate,
               (const std::vector<DecodedResourcePtr>& resources, const std::string& version_info));
 
-  MOCK_METHOD(
-      void, onConfigUpdate,
-      (const Protobuf::RepeatedPtrField<envoy::service::discovery::v3::Resource>& added_resources,
-       const Protobuf::RepeatedPtrField<std::string>& removed_resources,
-       const std::string& system_version_info));
+  MOCK_METHOD(void, onConfigUpdate,
+              (absl::Span<const envoy::service::discovery::v3::Resource* const> added_resources,
+               const Protobuf::RepeatedPtrField<std::string>& removed_resources,
+               const std::string& system_version_info));
   MOCK_METHOD(void, onConfigUpdateFailed,
               (Envoy::Config::ConfigUpdateFailureReason reason, const EnvoyException* e));
 };