diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc index a257b69b36e5..ea735ffb41d5 100644 --- a/source/server/listener_manager_impl.cc +++ b/source/server/listener_manager_impl.cc @@ -393,6 +393,17 @@ bool ListenerManagerImpl::addOrUpdateListenerInternal( auto existing_active_listener = getListenerByName(active_listeners_, name); auto existing_warming_listener = getListenerByName(warming_listeners_, name); + // The listener should be updated back to its original state and the warming listener should be + // removed. + if (existing_warming_listener != warming_listeners_.end() && + existing_active_listener != active_listeners_.end() && + (*existing_active_listener)->blockUpdate(hash)) { + warming_listeners_.erase(existing_warming_listener); + updateWarmingActiveGauges(); + stats_.listener_modified_.inc(); + return true; + } + // Do a quick blocked update check before going further. This check needs to be done against both // warming and active. if ((existing_warming_listener != warming_listeners_.end() && diff --git a/test/server/config_validation/xds_corpus/clusterfuzz-testcase-xds_fuzz_test-6589246463541248 b/test/server/config_validation/xds_corpus/clusterfuzz-testcase-xds_fuzz_test-6589246463541248 new file mode 100644 index 000000000000..bd3491d65306 --- /dev/null +++ b/test/server/config_validation/xds_corpus/clusterfuzz-testcase-xds_fuzz_test-6589246463541248 @@ -0,0 +1,118 @@ +actions { + add_listener { + listener_num: 1024 + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_listener { + } +} +actions { + add_listener { + listener_num: 1851719680 + } +} +actions { + add_listener { + listener_num: 4 + route_num: 1667301376 + } +} +actions { + add_listener { + listener_num: 268435456 + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_listener { + listener_num: 17 + route_num: 20992 + } +} +actions { + remove_listener { + listener_num: 16711680 + } +} +actions { + add_listener { + listener_num: 1 + route_num: 42 + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_listener { + } +} +actions { + add_listener { + listener_num: 8 + route_num: 1 + } +} +actions { + add_route { + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_listener { + listener_num: 17 + route_num: 20992 + } +} +actions { + remove_listener { + listener_num: 16711680 + } +} +actions { + add_listener { + listener_num: 822083584 + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + add_route { + } +} +actions { + add_route { + route_num: 1 + } +} +actions { + remove_listener { + listener_num: 16711680 + } +} +config { +} diff --git a/test/server/config_validation/xds_verifier.cc b/test/server/config_validation/xds_verifier.cc index 5623ceed35a3..90b6a8821d8e 100644 --- a/test/server/config_validation/xds_verifier.cc +++ b/test/server/config_validation/xds_verifier.cc @@ -16,7 +16,7 @@ XdsVerifier::XdsVerifier(test::server::config_validation::Config::SotwOrDelta so } /** - * get the route referenced by a listener + * Get the route referenced by a listener. */ std::string XdsVerifier::getRoute(const envoy::config::listener::v3::Listener& listener) { envoy::config::listener::v3::Filter filter0 = listener.filter_chains()[0].filters()[0]; @@ -47,7 +47,7 @@ bool XdsVerifier::hasListener(const std::string& name, ListenerState state) { } /** - * prints the currently stored listeners and their states + * Pretty prints the currently stored listeners and their states. */ void XdsVerifier::dumpState() { ENVOY_LOG_MISC(debug, "Listener Dump:"); @@ -58,33 +58,60 @@ void XdsVerifier::dumpState() { } /* - * if a listener is added for the first time, it will be added as active/warming depending on if - * envoy knows about its route config + * If a listener is added for the first time, it will be added as active/warming depending on if + * Envoy knows about its route config. * - * if a listener is updated (i.e. there is a already a listener by this name), there are 3 cases: - * 1. the old listener is active and the new is warming: - * - old will remain active - * - new will be added as warming, to replace the old when it gets its route - * 2. the old listener is active and new is active: - * - old is drained (seemingly instantaneously) - * - new is added as active - * 3. the old listener is warming and new is active/warming: - * - old is completely removed - * - new is added as warming/active as normal + * If a listener is updated (i.e. there is a already a listener by this name), there are 3 cases: + * 1. The old listener is active and the new is warming: + * - Old will remain active + * - New will be added as warming, to replace the old when it gets its route + * 2. The old listener is active and new is active: + * - Old is drained (seemingly instantaneously) + * - New is added as active + * 3. The old listener is warming and new is active/warming: + * - Old is completely removed + * - New is added as warming/active as normal */ /** - * update a listener when its route is changed, draining/removing the old listener and adding the - * updated listener + * Update a listener when its route is changed, draining/removing the old listener and adding the + * updated listener. */ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& listener) { ENVOY_LOG_MISC(debug, "About to update listener {} to {}", listener.name(), getRoute(listener)); dumpState(); - if (std::any_of(listeners_.begin(), listeners_.end(), [&](auto& rep) { + const auto existing_warming = + std::find_if(listeners_.begin(), listeners_.end(), [&](const auto& rep) { return rep.listener.name() == listener.name() && - getRoute(listener) == getRoute(rep.listener) && rep.state != DRAINING; - })) { + getRoute(rep.listener) == getRoute(listener) && rep.state == WARMING; + }); + const auto existing_active = + std::find_if(listeners_.begin(), listeners_.end(), [&](const auto& rep) { + return rep.listener.name() == listener.name() && + getRoute(rep.listener) == getRoute(listener) && rep.state == ACTIVE; + }); + + // Return early if the listener is a duplicate. + if (existing_active != listeners_.end()) { + const auto warming_to_remove = + std::find_if(listeners_.begin(), listeners_.end(), [&](const auto& rep) { + return rep.listener.name() == listener.name() && rep.state == WARMING; + }); + + // The listener should be updated back to its original state and the warming removed. + if (warming_to_remove != listeners_.end()) { + ENVOY_LOG_MISC(debug, "Removing warming listener {} after update", listener.name()); + num_modified_++; + num_warming_--; + listeners_.erase(warming_to_remove); + return; + } + ENVOY_LOG_MISC(debug, "Ignoring duplicate add of {}", listener.name()); + return; + } + + if (existing_warming != listeners_.end()) { ENVOY_LOG_MISC(debug, "Ignoring duplicate add of {}", listener.name()); return; } @@ -94,7 +121,7 @@ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& l const auto& rep = *it; ENVOY_LOG_MISC(debug, "checking {} for update", rep.listener.name()); if (rep.listener.name() == listener.name()) { - // if we're updating a warming/active listener, num_modified_ must be incremented + // If we're updating a warming/active listener, num_modified_ must be incremented. if (rep.state != DRAINING && !found) { num_modified_++; found = true; @@ -102,23 +129,23 @@ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& l 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 + // 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_--; 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 + // If the new listener has not gotten its route yet, the old listener will remain active + // until that happens. ENVOY_LOG_MISC(debug, "Keeping {} as ACTIVE", listener.name()); } } else if (rep.state == WARMING) { - // if the old listener is warming, it will be removed and replaced with the new + // 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_--; it = listeners_.erase(it); - // don't increment it + // Don't increment it. continue; } } @@ -129,7 +156,7 @@ void XdsVerifier::listenerUpdated(const envoy::config::listener::v3::Listener& l } /** - * add a new listener to listeners_ in either an active or warming state + * Add a new listener to listeners_ in either an active or warming state. * @param listener the listener to be added * @param from_update whether this function was called from listenerUpdated, in which case * num_added_ should not be incremented @@ -155,7 +182,7 @@ void XdsVerifier::listenerAdded(const envoy::config::listener::v3::Listener& lis } /** - * remove a listener and drain it if it was active + * Remove a listener and drain it if it was active. * @param name the name of the listener to be removed */ void XdsVerifier::listenerRemoved(const std::string& name) { @@ -165,19 +192,19 @@ void XdsVerifier::listenerRemoved(const std::string& name) { auto& rep = *it; if (rep.listener.name() == name) { if (rep.state == ACTIVE) { - // the listener will be drained before being removed + // 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 + // 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 + // Don't increment it. continue; } } @@ -190,18 +217,18 @@ void XdsVerifier::listenerRemoved(const std::string& name) { } /** - * after a SOTW update, see if any listeners that are currently warming can become active + * After a SOTW update, see if any listeners that are currently warming can become active. */ void XdsVerifier::updateSotwListeners() { ASSERT(sotw_or_delta_ == SOTW); for (auto& rep : listeners_) { - // check all_routes_, not active_routes_ since this is SOTW, so any inactive routes will become - // active if this listener refers to them + // Check all_routes_, not active_routes_ since this is SOTW, so any inactive routes will become + // active if this listener refers to them. if (hasRoute(rep.listener) && rep.state == WARMING) { - // it should successfully warm now + // It should successfully warm now. ENVOY_LOG_MISC(debug, "Moving {} to ACTIVE state", rep.listener.name()); - // if the route was not originally added as active, change it now + // If the route was not originally added as active, change it now. if (!hasActiveRoute(rep.listener)) { std::string route_name = getRoute(rep.listener); auto it = all_routes_.find(route_name); @@ -210,8 +237,8 @@ void XdsVerifier::updateSotwListeners() { active_routes_.insert({route_name, it->second}); } - // if there were any active listeners that were waiting to be updated, they will now be - // removed and the warming listener will take their place + // If there were any active listeners that were waiting to be updated, they will now be + // removed and the warming listener will take their place. markForRemoval(rep); num_warming_--; num_active_++; @@ -224,16 +251,16 @@ void XdsVerifier::updateSotwListeners() { } /** - * after a delta update, update any listeners that refer to the added route + * After a delta update, update any listeners that refer to the added route. */ void XdsVerifier::updateDeltaListeners(const envoy::config::route::v3::RouteConfiguration& route) { for (auto& rep : listeners_) { if (getRoute(rep.listener) == route.name() && rep.state == WARMING) { - // it should successfully warm now + // It should successfully warm now. ENVOY_LOG_MISC(debug, "Moving {} to ACTIVE state", rep.listener.name()); - // if there were any active listeners that were waiting to be updated, they will now be - // removed and the warming listener will take their place + // If there were any active listeners that were waiting to be updated, they will now be + // removed and the warming listener will take their place. markForRemoval(rep); num_warming_--; num_active_++; @@ -253,12 +280,12 @@ void XdsVerifier::updateDeltaListeners(const envoy::config::route::v3::RouteConf */ void XdsVerifier::markForRemoval(ListenerRepresentation& rep) { ASSERT(rep.state == WARMING); - // find the old listener and mark it for removal + // Find the old listener and mark it for removal. for (auto& old_rep : listeners_) { if (old_rep.listener.name() == rep.listener.name() && getRoute(old_rep.listener) != getRoute(rep.listener) && old_rep.state == ACTIVE) { - // mark it as removed to remove it after the loop so as not to invalidate the iterator in - // the caller function + // 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_active_--; } @@ -266,14 +293,14 @@ void XdsVerifier::markForRemoval(ListenerRepresentation& rep) { } /** - * add a new route and update any listeners that refer to this route + * Add a new route and update any listeners that refer to this route. */ void XdsVerifier::routeAdded(const envoy::config::route::v3::RouteConfiguration& route) { - // routes that are not referenced by any resource are ignored, so this creates a distinction - // between SOTW and delta - // if an unreferenced route is sent in delta, it is ignored forever as it will not be sent in + // Routes that are not referenced by any resource are ignored, so this creates a distinction + // between SOTW and delta. + // 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 + // listener that refers to it is added in the meantime, it will become active. if (!hasRoute(route.name())) { all_routes_.insert({route.name(), route}); } @@ -292,7 +319,7 @@ void XdsVerifier::routeAdded(const envoy::config::route::v3::RouteConfiguration& } /** - * called after draining a listener, will remove it from listeners_ + * Called after draining a listener, will remove it from listeners_. */ void XdsVerifier::drainedListener(const std::string& name) { for (auto it = listeners_.begin(); it != listeners_.end(); ++it) { diff --git a/test/server/config_validation/xds_verifier_test.cc b/test/server/config_validation/xds_verifier_test.cc index 8dc1a28d2210..3e65220687e7 100644 --- a/test/server/config_validation/xds_verifier_test.cc +++ b/test/server/config_validation/xds_verifier_test.cc @@ -16,7 +16,7 @@ envoy::config::route::v3::RouteConfiguration buildRoute(const std::string& route envoy::config::core::v3::ApiVersion::V3); } -// add, warm, drain and remove a listener +// Add, warm, drain and remove a listener. TEST(XdsVerifier, Basic) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); verifier.listenerAdded(buildListener("listener_0", "route_config_0")); @@ -44,19 +44,19 @@ TEST(XdsVerifier, Basic) { TEST(XdsVerifier, RouteBeforeListenerSOTW) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // send a route first, so envoy will not accept it + // Send a route first, so envoy will not accept it. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasRoute("route_config_0")); EXPECT_FALSE(verifier.hasActiveRoute("route_config_0")); - // envoy still doesn't know about the route, so this will warm + // Envoy still doesn't know about the route, so this will warm. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); EXPECT_EQ(verifier.numAdded(), 1); EXPECT_EQ(verifier.numWarming(), 1); - // send a new route, which will include route_config_0 since SOTW, so route_config_0 will become - // active + // Send a new route, which will include route_config_0 since SOTW, so route_config_0 will become + // active. verifier.routeAdded(buildRoute("route_config_1")); EXPECT_TRUE(verifier.hasRoute("route_config_1")); EXPECT_FALSE(verifier.hasActiveRoute("route_config_1")); @@ -67,18 +67,18 @@ TEST(XdsVerifier, RouteBeforeListenerSOTW) { TEST(XdsVerifier, RouteBeforeListenerDelta) { XdsVerifier verifier(test::server::config_validation::Config::DELTA); - // send a route first, so envoy will not accept it + // Send a route first, so envoy will not accept it. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_FALSE(verifier.hasActiveRoute("route_config_0")); - // envoy still doesn't know about the route, so this will warm + // Envoy still doesn't know about the route, so this will warm. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); EXPECT_EQ(verifier.numAdded(), 1); EXPECT_EQ(verifier.numWarming(), 1); - // send a new route, which will not include route_config_0 since SOTW, so route_config_0 will not - // become active + // Send a new route, which will not include route_config_0 since SOTW, so route_config_0 will not + // become active. verifier.routeAdded(buildRoute("route_config_1")); EXPECT_FALSE(verifier.hasActiveRoute("route_config_1")); EXPECT_FALSE(verifier.hasActiveRoute("route_config_0")); @@ -90,15 +90,15 @@ TEST(XdsVerifier, UpdateWarmingListener) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.listenerUpdated(buildListener("listener_0", "route_config_1")); - // the new listener should directly replace the old listener since it's warming + // The new listener should directly replace the old listener since it's warming. EXPECT_EQ(verifier.numModified(), 1); EXPECT_EQ(verifier.numAdded(), 1); - // send the route for the old listener, which should have been replaced with the update + // Send the route for the old listener, which should have been replaced with the update. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_FALSE(verifier.hasListener("listener_0", verifier.ACTIVE)); - // now the new should become active + // Now the new should become active. verifier.routeAdded(buildRoute("route_config_1")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); } @@ -106,18 +106,18 @@ TEST(XdsVerifier, UpdateWarmingListener) { TEST(XdsVerifier, UpdateActiveListener) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // add an active listener + // Add an active listener. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); - // send an update, which should keep the old listener active until the new warms + // Send an update, which should keep the old listener active until the new warms. verifier.listenerUpdated(buildListener("listener_0", "route_config_1")); EXPECT_EQ(verifier.numModified(), 1); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); - // warm the new listener, which should remove the old + // Warm the new listener, which should remove the old. verifier.routeAdded(buildRoute("route_config_1")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); EXPECT_FALSE(verifier.hasListener("listener_0", verifier.DRAINING)); @@ -129,19 +129,19 @@ TEST(XdsVerifier, UpdateActiveListener) { TEST(XdsVerifier, UpdateActiveToActive) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // add two active listeners to different routes + // Add two active listeners to different routes. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); - // add an active listener + // Add an active listener. verifier.listenerAdded(buildListener("listener_1", "route_config_1")); verifier.routeAdded(buildRoute("route_config_1")); EXPECT_TRUE(verifier.hasListener("listener_1", verifier.ACTIVE)); EXPECT_EQ(verifier.numAdded(), 2); - // send an update, which should make the new listener active straight away and remove the old - // since its route is already active + // Send an update, which should make the new listener active straight away and remove the old + // since its route is already active. verifier.listenerUpdated(buildListener("listener_0", "route_config_1")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); EXPECT_FALSE(verifier.hasListener("listener_0", verifier.WARMING)); @@ -151,11 +151,11 @@ TEST(XdsVerifier, UpdateActiveToActive) { TEST(XdsVerifier, WarmMultipleListenersSOTW) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // add two warming listener to the same route + // Add two warming listener to the same route. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.listenerAdded(buildListener("listener_1", "route_config_0")); - // send the route, make sure both are active + // Send the route, make sure both are active. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); EXPECT_TRUE(verifier.hasListener("listener_1", verifier.ACTIVE)); @@ -165,11 +165,11 @@ TEST(XdsVerifier, WarmMultipleListenersSOTW) { TEST(XdsVerifier, WarmMultipleListenersDelta) { XdsVerifier verifier(test::server::config_validation::Config::DELTA); - // add two warming listener to the same route + // Add two warming listener to the same route. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.listenerAdded(buildListener("listener_1", "route_config_0")); - // send the route, make sure both are active + // Send the route, make sure both are active. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); EXPECT_TRUE(verifier.hasListener("listener_1", verifier.ACTIVE)); @@ -179,14 +179,14 @@ TEST(XdsVerifier, WarmMultipleListenersDelta) { TEST(XdsVerifier, ResendRouteSOTW) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // send a route that will be ignored + // Send a route that will be ignored. verifier.routeAdded(buildRoute("route_config_0")); - // add a warming listener that refers to this route + // Add a warming listener that refers to this route. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); - // send the same route again, make sure listener becomes active + // Send the same route again, make sure listener becomes active. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); } @@ -194,14 +194,14 @@ TEST(XdsVerifier, ResendRouteSOTW) { TEST(XdsVerifier, ResendRouteDelta) { XdsVerifier verifier(test::server::config_validation::Config::DELTA); - // send a route that will be ignored + // Send a route that will be ignored. verifier.routeAdded(buildRoute("route_config_0")); - // add a warming listener that refers to this route + // Add a warming listener that refers to this route. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); - // send the same route again, make sure listener becomes active + // Send the same route again, make sure listener becomes active. verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); } @@ -209,19 +209,38 @@ TEST(XdsVerifier, ResendRouteDelta) { TEST(XdsVerifier, RemoveThenAddListener) { XdsVerifier verifier(test::server::config_validation::Config::SOTW); - // add an active listener + // Add an active listener. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); verifier.routeAdded(buildRoute("route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); - // remove it + // Remove it. verifier.listenerRemoved("listener_0"); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.DRAINING)); - // and add it back, it should now be draining and active + // And add it back, it should now be draining and active. verifier.listenerAdded(buildListener("listener_0", "route_config_0")); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.DRAINING)); EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); } +TEST(XdsVerifier, UpdateBackToOriginal) { + XdsVerifier verifier(test::server::config_validation::Config::SOTW); + + // Add an active listener. + verifier.listenerAdded(buildListener("listener_0", "route_config_0")); + verifier.routeAdded(buildRoute("route_config_0")); + EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); + + // Update it to a new route, should warm. + verifier.listenerUpdated(buildListener("listener_0", "route_config_1")); + EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); + EXPECT_TRUE(verifier.hasListener("listener_0", verifier.WARMING)); + + // Update it back to original, should remove warming listener. + verifier.listenerUpdated(buildListener("listener_0", "route_config_0")); + EXPECT_FALSE(verifier.hasListener("listener_0", verifier.WARMING)); + EXPECT_TRUE(verifier.hasListener("listener_0", verifier.ACTIVE)); +} + } // namespace Envoy diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index a1ccd242cdd3..4495e0b125ed 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1255,6 +1255,67 @@ name: baz EXPECT_CALL(*listener_baz_update1, onDestroy()); } +TEST_F(ListenerManagerImplTest, UpdateActiveToWarmAndBack) { + InSequence s; + + EXPECT_CALL(*worker_, start(_)); + manager_->startWorkers(guard_dog_); + + // Add and initialize foo listener. + const std::string listener_foo_yaml = R"EOF( +name: foo +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +filter_chains: +- filters: [] + )EOF"; + + ListenerHandle* listener_foo = expectListenerCreate(true, true); + EXPECT_CALL(listener_factory_, createListenSocket(_, _, _, {true})); + EXPECT_CALL(listener_foo->target_, initialize()); + EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true)); + checkStats(__LINE__, 1, 0, 0, 1, 0, 0, 0); + EXPECT_CALL(*worker_, addListener(_, _, _)); + listener_foo->target_.ready(); + worker_->callAddCompletion(true); + EXPECT_EQ(1UL, manager_->listeners().size()); + checkStats(__LINE__, 1, 0, 0, 0, 1, 0, 0); + + // Update foo into warming. + const std::string listener_foo_update1_yaml = R"EOF( +name: foo +address: + socket_address: + address: 127.0.0.1 + port_value: 1234 +per_connection_buffer_limit_bytes: 999 +filter_chains: +- filters: [] + )EOF"; + + ListenerHandle* listener_foo_update1 = expectListenerCreate(true, true); + EXPECT_CALL(listener_foo_update1->target_, initialize()); + EXPECT_TRUE( + manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_update1_yaml), "", true)); + + // Should be both active and warming now. + EXPECT_EQ(1UL, manager_->listeners(ListenerManager::WARMING).size()); + EXPECT_EQ(1UL, manager_->listeners(ListenerManager::ACTIVE).size()); + checkStats(__LINE__, 1, 1, 0, 1, 1, 0, 0); + + // Update foo back to original active, should cause the warming listener to be removed. + EXPECT_CALL(*listener_foo_update1, onDestroy()); + EXPECT_TRUE(manager_->addOrUpdateListener(parseListenerFromV3Yaml(listener_foo_yaml), "", true)); + + checkStats(__LINE__, 1, 2, 0, 0, 1, 0, 0); + EXPECT_EQ(0UL, manager_->listeners(ListenerManager::WARMING).size()); + EXPECT_EQ(1UL, manager_->listeners(ListenerManager::ACTIVE).size()); + + EXPECT_CALL(*listener_foo, onDestroy()); +} + TEST_F(ListenerManagerImplTest, AddReusableDrainingListener) { InSequence s;