core: add missing extension registration

[Augustyniak]

Commit Message: A follow up fixes to #24495. Without these fixes application crashes after addDirectResponse call on TestEngineBuilder (iOS only api) as the handler_ in

envoy/source/server/worker_impl.cc

Line 61 in 93ea91f

handler_->addListener(overridden_listener, listener, runtime);

is nullptr.
Additional Description: The question is whether without the added registration calls application may crash even without the addDirectResponse call. cc @alyssawilk. Asking since Lyft has pushed #24495 to prod pipeline already and the question is whether we need to cherry-pick the fix from this PR now.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Rafal Augustyniak raugustyniak@lyft.com

[repokitteh-read-only]

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24750 was opened by Augustyniak.

see: more, trace.

[repokitteh-read-only]

CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @alyssawilk

🐱

Caused by: #24750 was opened by Augustyniak.

see: more, trace.

[Augustyniak]

@alyssawilk the crash I observed was happening when we were trying to add a listener

envoy/source/server/worker_impl.cc

Line 57 in 2e8413c

void WorkerImpl::addListener(absl::optional<uint64_t> overridden_listener,

. That's something that we do only in our tests so it should not impact "real" apps but I wonder whether you can see any other cases for when the EM would crash (without the fix from this PR) if the application does not add/remove listeners?

[alyssawilk]

Legacy add direct response doesn't serve a direct response but instead proxies to the loopback listener, which wouldn't play well when missing a connection handler. That said I thought I fixed the proxying for add direct response in #24402 so I'm not clear why adding a direct response would trigger this path any more :-/

[Augustyniak]

looking at the EnvoyConfiguration.m:

envoy/mobile/library/objective-c/EnvoyConfiguration.m

Lines 128 to 137 in 2e8413c

	if (hasDirectResponses) {
	templateYAML = [templateYAML stringByReplacingOccurrencesOfString:@"#{fake_remote_responses}"
	withString:self.directResponses];
	[customClusters appendString:[[NSString alloc] initWithUTF8String:fake_remote_cluster_insert]];
	[customListeners
	appendString:[[NSString alloc] initWithUTF8String:fake_remote_listener_insert]];
	[customRoutes appendString:self.directResponseMatchers];
	[customFilters
	appendString:[[NSString alloc] initWithUTF8String:route_cache_reset_filter_insert]];
	}

I think that we are still injecting the fake listener for cases when direct responses are defined. We replace #{custom_listeners} substring

envoy/mobile/library/common/config/config.cc

Line 375 in 2e8413c

#{custom_listeners}

with the list of listeners (that includes the fake listener) in here

envoy/mobile/library/objective-c/EnvoyConfiguration.m

Lines 141 to 142 in 2e8413c

	templateYAML = [templateYAML stringByReplacingOccurrencesOfString:@"#{custom_listeners}"
	withString:customListeners];

[alyssawilk]

ahhh, that'd do it. How would you folks feel about removing that listener overall, if I can sort out all the tests? I'd planned on doing it as a follow up but didn't want to rush things in case y'all had other deps on it

[Augustyniak]

Removing the listener sounds fine 👍

@alyssawilk Do you think that having #24495 without having whatever is in this PR fine for cases when EM does not add direct responses? I am asking since we pushed exactly that (#24495 without the registration calls) to our prod release pipeline yesterday and I am trying to determine whether I need to patch that pushed version with the fix from this PR to avoid crashing our apps? I cannot repro a crash without addDirectResponse call but maybe you can think of any other case when such crash would happen?

[alyssawilk]

So this is all code which shouldn't be accessed by E-M (which is why I'm trying to remove it) but as you say the direct response path is inadvertently using it when I thought it was fixed, so I'd lean towards erring on the side of caution and cherry picking. I'm sorry I missed yet another force-register :-(