xds: remove warming listeners in xDS SOTW updates

[samflattery]

Signed-off-by: Sam Flattery samflattery@google.com

Commit Message: Remove warming listeners in xDS SOTW updates
Additional Description:

• my xDS fuzzer timed out on OSS fuzz here because Envoy does not remove warming listeners when they are left out of a SOTW update like it removes active ones
• changes LdsApiImpl::onConfigUpdate to use a new function in ListenerManagerImpl, allListeners() instead of the old listeners() which just returns active listeners to compute the diff between the update and the current SOTW
• changed lds_api_test.cc to use this new function

Risk Level: Medium
Testing: passes regression corpus entry, passes lds_api_test.cc
Docs Changes: N/A
Release Notes: N/A
Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24600

[samflattery]

/cc @asraa

[samflattery]

/retest

[repokitteh-read-only]

🔨 rebuilding ci/circleci: docs (failed build)

🐱

Caused by: a #12461 (comment) was created by @samflattery.

see: more, trace.

[samflattery]

Pipelines seems to be down https://status.dev.azure.com/

[sqkerner]

When changing the API of a class, think about how the methods look in code that uses the class. Do the names of functions make it clear what is happening?

In this case, the class has two public methods:

listeners() : Returns some of the listeners.
allListeners(): Returns all of the listeners.

If I saw a call to listeners() in some code, I would not expect that it returns some listeners, or know which ones.

Ideas to fix this:

1. Rename listeners() to loadedListners(), activeListners(), etc.

2. Make the type of listeners to return an enum:

  enum ListenerState {
     ACTIVE = 1,
     WARMING = 2,
     ALL = 3
  }

Function listeners(ListenerState state) would return the listeners in the given state.

[samflattery]

Thanks for the feedback. I was thinking of changing listeners() to activeListeners() but I didn't want to be too disruptive to the code base since listeners() is used in a lot of different places (mostly in tests). But if you're happy enough with this I can go ahead and do it.

[sqkerner]

@asraa knows this code better than I do, and I would defer to her judgement.

[asraa]

(2) Along with draining listeners might be good. It can default to active listeners as currently used. @lambdai What do you think?

[lambdai]

I am fine to add draining listener in the query.
Though to fix the fuzzer issue we don't need the names in drain list.

[samflattery]

Sure, I'll do this and add draining listeners too

[mattklein123]

cc @lambdai PTAL

[lambdai]

Nice diagnose!!

Could you add a new test case in ListenerManagerImplTest beside
TEST_F(ListenerManagerImplTest, RemoveListener)?

You may want to

1. start the workers
2. provide a lds response with an unique name
3. don't call ready() of the new listener
4. provide a lds response w/o that unique name
   And see if the listeners are erased from the new allListeners().

[lambdai]

I am fine to add draining listener in the query.
Though to fix the fuzzer issue we don't need the names in drain list.

[samflattery]

@lambdai

2. provide a lds response with an unique name
3. don't call ready() of the new listener
4. provide a lds response w/o that unique name
   And see if the listeners are erased from the new allListeners().

I looked into this and I'm not sure if this is possible in ListenerManagerImplTest since it doesn't have access to an LdsApiImpl, just an LdsApi, which only has a versionInfo method and not an onConfigUpdate which is what we need.

envoy/test/server/listener_manager_impl_test.cc

Lines 151 to 154 in c2b0d37

	class MockLdsApi : public LdsApi {
	public:
	MOCK_METHOD(std::string, versionInfo, (), (const));
	};

I could do it in lds_api_test.cc but I think it would end up being identical to this test

envoy/test/server/lds_api_test.cc

Lines 271 to 278 in c2b0d37

	makeListenersAndExpectCall({"listener1", "listener2"});
	EXPECT_CALL(listener_manager_, removeListener("listener2")).WillOnce(Return(true));
	expectAdd("listener1", "1", false);
	expectAdd("listener3", "1", true);
	EXPECT_CALL(listener_manager_, endListenerUpdate(_));
	const auto decoded_resources_2 =
	TestUtility::decodeResources<envoy::config::listener::v3::Listener>(response2);
	lds_callbacks_->onConfigUpdate(decoded_resources_2.refvec_, response2.version_info());

since there's no real way to add in warming/active listeners beyond making expectations of the return of listeners() which is what that test does.

[samflattery]

Failing //test/integration:protocol_integration_test, might be a flake? Passes locally on --runs_per_test=10 and doesn't seem to touch any LDS code

edit: also failing //test/extensions/transport_sockets/tls/integration:ssl_integration_test which is also passing locally with asan enabled

[asraa]

since there's no real way to add in warming/active listeners beyond making expectations of the return of listeners() which is what that test does.

Is it possible to reproduce this in ads_integration_test? Just looking for unit test coverage (since this is a slight functional change)

[samflattery]

@asraa that's definitely possible. I'll do that now

[samflattery]

@asraa added

[asraa]

@lambdai If the test looks good to you, this has my LGTM.

[lambdai]

The comment should start with capital letter and end with period.

The code LGTM. Thanks!

[samflattery]

@lambdai @asraa fixed the comments

[asraa]

Thanks!