Refactoring ClusterManagerImpl to support dynamic config for composite filter when its in upstream filter chain

[yanjunxiang-google]

Title: Refactoring ClusterManagerImpl c'tor and to support dynamic config for composite filter when its in upstream filter chain

This issue is related to the PR: #33013

Description:
Currently, the cluster manager is initialized with the below sequence:

envoy/source/server/configuration_impl.cc

Line 131 in 8ff28fe

cluster_manager_ = cluster_manager_factory.clusterManagerFromProto(bootstrap);

2)

envoy/source/common/upstream/cluster_manager_impl.cc

Line 2227 in 8ff28fe

THROW_IF_NOT_OK(cluster_manager_impl->init(bootstrap));

i.e, it calls the cluster_manager_impl->init(bootstrap) before return unique_ptr, and assign to the cluster_manager_.

This is causing problems when functions called inside cluster_manager_impl->init(bootstrap) needs to access cluster_manger_, which is the case for composite filter in the upstream filter chain.

So, for composite filter case, if it is put in the upstream filter chain, when the dynamic_config is configured:

envoy/api/envoy/extensions/filters/http/composite/v3/composite.proto

Line 59 in 8ff28fe

DynamicConfig dynamic_config = 2

the above cluster_manager_impl->init(bootstrap) will eventually call this function:

envoy/source/extensions/filters/http/composite/action.cc

Line 43 in 8ff28fe

factory_context, server_factory_context.clusterManager(), false, "http", nullptr);

which access above cluster_manger_ through server_factory_context.clusterManager(). As cluster_manager_ is nullptr, this crashed.

So, if we want to support composite filter with dynamic_config in the upstream filter chain, one of the solution is moving the
cluster_manager_impl->init(bootstrap)
out from
ProdClusterManagerFactory::clusterManagerFromProto

And call it after, like this:

cluster_manager_ = clusterManagerFromProto();
cluster_manager_->init(bootstrap);

This way, when the above init() function access clusterManager(), cluster_manager_ is already setup and no issues.
We need to add good comments to make sure no other code wrote in between these two lines though.

[optional Relevant Links:]

Any extra documentation required to understand the issue.

[soulxu]

cc @wbpcode @RyanTheOptimist

It will lead to a crash, so it should be a bug I think

[wbpcode]

This is duplicate with #26653

I am glad to review/see a fix to close these two issue. :)

[yanjunxiang-google]

Thanks for sharing the info! I have a draft PR for this:
#33221

[yanjunxiang-google]

@soulxu As we currently does not support composite filter in upstream filter chain, there is no crash at this moment.

[yanjunxiang-google]

BTW, we need to make same change for nighthawk here: https://github.com/envoyproxy/nighthawk/blob/ac98827203dafabf447ff68e1403e0c2834ee726/source/client/process_impl.cc#L897

[qqustc]

nighthawk CI crashed due to #33221
https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/168085/logs/28

fixed in envoyproxy/nighthawk#1133