fuzz: add unit tests to xDS verifier (envoyproxy#12246)

Commit Message: Add unit tests to xDS verifier
* add new file xds_verifier_test.cc with unit tests
* add tests for main paths through verifier

Signed-off-by: Sam Flattery <[email protected]>

Author: samflattery

test/server/config_validation/BUILD

@@ -129,6 +129,15 @@ envoy_cc_test_library(
     ],
 )
 
+envoy_cc_test(
+    name = "xds_verifier_test",
+    srcs = ["xds_verifier_test.cc"],
+    deps = [
+        ":xds_verifier_lib",
+        "//test/config:utility_lib",
+    ],
+)
+
 envoy_cc_test_library(
     name = "xds_fuzz_lib",
     srcs = ["xds_fuzz.cc"],

test/server/config_validation/xds_fuzz.cc

@@ -69,6 +69,9 @@ XdsFuzzTest::XdsFuzzTest(const test::server::config_validation::XdsTestCase& inp
   create_xds_upstream_ = true;
   tls_xds_upstream_ = false;
 
+  // avoid listeners draining during the test
+  drain_time_ = std::chrono::seconds(60);
+
   if (input.config().sotw_or_delta() == test::server::config_validation::Config::SOTW) {
     sotw_or_delta_ = Grpc::SotwOrDelta::Sotw;
   } else {
@@ -130,6 +133,7 @@ bool XdsFuzzTest::hasRoute(const std::string& route_name) {
  */
 void XdsFuzzTest::addListener(const std::string& listener_name, const std::string& route_name) {
   ENVOY_LOG_MISC(debug, "Adding {} with reference to {}", listener_name, route_name);
+  lds_update_success_++;
   bool removed = eraseListener(listener_name);
   auto listener = buildListener(listener_name, route_name);
   listeners_.push_back(listener);
@@ -154,6 +158,7 @@ void XdsFuzzTest::removeListener(const std::string& listener_name) {
   bool removed = eraseListener(listener_name);
 
   if (removed) {
+    lds_update_success_++;
     updateListener(listeners_, {}, {listener_name});
     EXPECT_TRUE(waitForAck(Config::TypeUrl::get().Listener, std::to_string(version_)));
     verifier_.listenerRemoved(listener_name);
@@ -165,19 +170,15 @@ void XdsFuzzTest::removeListener(const std::string& listener_name) {
  */
 void XdsFuzzTest::addRoute(const std::string& route_name) {
   ENVOY_LOG_MISC(debug, "Adding {}", route_name);
-  bool has_route = hasRoute(route_name);
   auto route = buildRouteConfig(route_name);
-  routes_.push_back(route);
 
-  if (has_route) {
-    // if the route was already in routes_, don't send a duplicate add in delta request
-    updateRoute(routes_, {}, {});
-    verifier_.routeUpdated(route);
-  } else {
-    updateRoute(routes_, {route}, {});
-    verifier_.routeAdded(route);
+  if (!hasRoute(route_name)) {
+    routes_.push_back(route);
   }
 
+  updateRoute(routes_, {route}, {});
+  verifier_.routeAdded(route);
+
   EXPECT_TRUE(waitForAck(Config::TypeUrl::get().RouteConfiguration, std::to_string(version_)));
 }
 
@@ -277,6 +278,7 @@ void XdsFuzzTest::replay() {
       test_server_->waitForCounterEq("listener_manager.listener_modified", verifier_.numModified());
       test_server_->waitForCounterEq("listener_manager.listener_added", verifier_.numAdded());
       test_server_->waitForCounterEq("listener_manager.listener_removed", verifier_.numRemoved());
+      test_server_->waitForCounterEq("listener_manager.lds.update_success", lds_update_success_);
     }
     ENVOY_LOG_MISC(debug, "warming {} ({}), active {} ({}), draining {} ({})",
                    verifier_.numWarming(),

test/server/config_validation/xds_fuzz.h

@@ -63,7 +63,7 @@ class XdsFuzzTest : public HttpIntegrationTest {
   std::vector<envoy::api::v2::RouteConfiguration> getRoutesConfigDump();
 
   bool eraseListener(const std::string& listener_name);
-  bool hasRoute(const std::string& route_num);
+  bool hasRoute(const std::string& route_name);
   AssertionResult waitForAck(const std::string& expected_type_url,
                              const std::string& expected_version);
 
@@ -77,6 +77,8 @@ class XdsFuzzTest : public HttpIntegrationTest {
   envoy::config::core::v3::ApiVersion api_version_;
 
   Network::Address::IpVersion ip_version_;
+
+  uint64_t lds_update_success_{0};
 };
 
 } // namespace Envoy

test/server/config_validation/xds_verifier.cc

@@ -29,11 +29,21 @@ std::string XdsVerifier::getRoute(const envoy::config::listener::v3::Listener& l
  * @return true iff the route listener refers to is in all_routes_
  */
 bool XdsVerifier::hasRoute(const envoy::config::listener::v3::Listener& listener) {
-  return all_routes_.contains(getRoute(listener));
+  return hasRoute(getRoute(listener));
 }
 
+bool XdsVerifier::hasRoute(const std::string& name) { return all_routes_.contains(name); }
+
 bool XdsVerifier::hasActiveRoute(const envoy::config::listener::v3::Listener& listener) {
-  return active_routes_.contains(getRoute(listener));
+  return hasActiveRoute(getRoute(listener));
+}
+
+bool XdsVerifier::hasActiveRoute(const std::string& name) { return active_routes_.contains(name); }
+
+bool XdsVerifier::hasListener(const std::string& name, ListenerState state) {
+  return std::any_of(listeners_.begin(), listeners_.end(), [&](const auto& rep) {
+    return rep.listener.name() == name && state == rep.state;
+  });
 }
 
 /**
@@ -68,7 +78,7 @@ void XdsVerifier::dumpState() {
  * updated listener
  */
 void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& listener) {
-  ENVOY_LOG_MISC(debug, "About to update listener {}", listener.name());
+  ENVOY_LOG_MISC(debug, "About to update listener {} to {}", listener.name(), getRoute(listener));
   dumpState();
 
   if (std::any_of(listeners_.begin(), listeners_.end(), [&](auto& rep) {
@@ -79,17 +89,25 @@ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& l
     return;
   }
 
-  for (unsigned long i = 0; i < listeners_.size(); ++i) {
-    const auto& rep = listeners_[i];
+  bool found = false;
+  for (auto it = listeners_.begin(); it != listeners_.end();) {
+    const auto& rep = *it;
+    ENVOY_LOG_MISC(debug, "checking {} for update", rep.listener.name());
     if (rep.listener.name() == listener.name()) {
-      if (rep.state == ACTIVE) {
+      // if we're updating a warming/active listener, num_modified_ must be incremented
+      if (rep.state != DRAINING && !found) {
         num_modified_++;
+        found = true;
+      }
+
+      if (rep.state == ACTIVE) {
         if (hasActiveRoute(listener)) {
           // if the new listener is ready to take traffic, the old listener will be removed
           // it seems to be directly removed without being added to the config dump as draining
           ENVOY_LOG_MISC(debug, "Removing {} after update", listener.name());
           num_active_--;
-          listeners_.erase(listeners_.begin() + i);
+          it = listeners_.erase(it);
+          continue;
         } else {
           // if the new listener has not gotten its route yet, the old listener will remain active
           // until that happens
@@ -99,9 +117,12 @@ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& l
         // if the old listener is warming, it will be removed and replaced with the new
         ENVOY_LOG_MISC(debug, "Removed warming listener {}", listener.name());
         num_warming_--;
-        listeners_.erase(listeners_.begin() + i);
+        it = listeners_.erase(it);
+        // don't increment it
+        continue;
       }
     }
+    ++it;
   }
   dumpState();
   listenerAdded(listener, true);
@@ -139,25 +160,28 @@ void XdsVerifier::listenerAdded(const envoy::config::listener::v3::Listener& lis
  */
 void XdsVerifier::listenerRemoved(const std::string& name) {
   bool found = false;
-  for (unsigned long i = 0; i < listeners_.size(); ++i) {
-    auto& rep = listeners_[i];
-    if (rep.listener.name() != name) {
-      continue;
-    }
 
-    if (rep.state == ACTIVE) {
-      // the listener will be drained before being removed
-      ENVOY_LOG_MISC(debug, "Changing {} to DRAINING", name);
-      num_removed_++;
-      num_active_--;
-      num_draining_++;
-      rep.state = DRAINING;
-    } else if (rep.state == WARMING) {
-      // the listener will be removed immediately
-      ENVOY_LOG_MISC(debug, "Removed warming listener {}", name);
-      listeners_.erase(listeners_.begin() + i);
-      num_warming_--;
+  for (auto it = listeners_.begin(); it != listeners_.end();) {
+    auto& rep = *it;
+    if (rep.listener.name() == name) {
+      if (rep.state == ACTIVE) {
+        // the listener will be drained before being removed
+        ENVOY_LOG_MISC(debug, "Changing {} to DRAINING", name);
+        found = true;
+        num_active_--;
+        num_draining_++;
+        rep.state = DRAINING;
+      } else if (rep.state == WARMING) {
+        // the listener will be removed immediately
+        ENVOY_LOG_MISC(debug, "Removed warming listener {}", name);
+        found = true;
+        num_warming_--;
+        it = listeners_.erase(it);
+        // don't increment it
+        continue;
+      }
     }
+    ++it;
   }
 
   if (found) {
@@ -236,7 +260,6 @@ void XdsVerifier::markForRemoval(ListenerRepresentation& rep) {
       // mark it as removed to remove it after the loop so as not to invalidate the iterator in
       // the caller function
       old_rep.state = REMOVED;
-      /* num_modified_++; */
       num_active_--;
     }
   }
@@ -271,17 +294,19 @@ void XdsVerifier::routeAdded(const envoy::config::route::v3::RouteConfiguration&
   // if an unreferenced route is sent in delta, it is ignored forever as it will not be sent in
   // future RDS updates, whereas in SOTW it will be present in all future RDS updates, so if a
   // listener that refers to it is added in the meantime, it will become active
+  if (!hasRoute(route.name())) {
+    all_routes_.insert({route.name(), route});
+  }
 
-  // in delta, active_routes_ and all_routes_ should be the same as we only send one route at a
-  // time, so it either becomes active or not
   if (sotw_or_delta_ == DELTA && std::any_of(listeners_.begin(), listeners_.end(), [&](auto& rep) {
         return getRoute(rep.listener) == route.name();
       })) {
-    active_routes_.insert({route.name(), route});
-    all_routes_.insert({route.name(), route});
+    if (!hasActiveRoute(route.name())) {
+      active_routes_.insert({route.name(), route});
+      updateDeltaListeners(route);
+    }
     updateDeltaListeners(route);
   } else if (sotw_or_delta_ == SOTW) {
-    all_routes_.insert({route.name(), route});
     updateSotwListeners();
   }
 }

test/server/config_validation/xds_verifier.h

@@ -49,12 +49,16 @@ class XdsVerifier {
 
   void dumpState();
 
+  bool hasListener(const std::string& name, ListenerState state);
+  bool hasRoute(const envoy::config::listener::v3::Listener& listener);
+  bool hasRoute(const std::string& name);
+  bool hasActiveRoute(const envoy::config::listener::v3::Listener& listener);
+  bool hasActiveRoute(const std::string& name);
+
 private:
   enum SotwOrDelta { SOTW, DELTA };
 
   std::string getRoute(const envoy::config::listener::v3::Listener& listener);
-  bool hasRoute(const envoy::config::listener::v3::Listener& listener);
-  bool hasActiveRoute(const envoy::config::listener::v3::Listener& listener);
   void updateSotwListeners();
   void updateDeltaListeners(const envoy::config::route::v3::RouteConfiguration& route);
   void markForRemoval(ListenerRepresentation& rep);