config: fix CDS gRPC not specifying StreamClusters gRPC method

[fredlas]

Signed-off-by: Fred Douglas fredlas@google.com

Description: Choose gRPC method string to pass to SubscriptionFactory based on whether GRPC or DELTA_GRPC was specified. When the work-in-progress incremental CDS work had its incremental_cds_api_impl merged into cds_api_impl, the StreamClusters was changed (unconditionally) to DeltaClusters.

I wanted to add tests to prevent this particular breaking change from being made again, but found it surprisingly hard, and figured it's better to get it fixed right away.

Fixes #6237

[fredlas]

Specifically, I was trying to do

TEST_F(CdsApiImplTest, GrpcMethodStateOfTheWorld) {
  envoy::api::v2::core::ConfigSource cds_config;
  auto* api = cds_config.mutable_api_config_source();
  api->set_api_type(
      envoy::api::v2::core::ApiConfigSource::GRPC);
  api->add_grpc_services()->mutable_envoy_grpc()->set_cluster_name("some_cds_cluster");
  cluster_map_.emplace("foo_cluster", mock_cluster_);
  EXPECT_CALL(cm_, clusters()).WillRepeatedly(Return(cluster_map_));
  EXPECT_CALL(mock_cluster_, info()).Times(AnyNumber());
  EXPECT_CALL(mock_cluster_, info()).Times(AnyNumber());
  cds_ = CdsApiImpl::create(cds_config, cm_, dispatcher_, random_, local_info_, store_, *api_);
  // somehow verify that the StreamClusters gRPC method is being used
}

but 1) it was failing due to the "some_cds_cluster" xDS upstream not existing, which appears hard to fix short of bringing in the entire integration test framework, and 2) I still need to figure out how to verify the gRPC method.

[htuch]

Thanks, appreciate the quick fix!

[htuch]

Can you make SubscriptionFactory an injectable factory (rather than use a wrapper for a static method)? The main difficulty there might be the use of templates, but it could be possible to at least make the Grpc::AsyncClientFactoryPtr injectable..

[fredlas]

It would entail changes to everything that uses SubscriptionFactory, including all the different xDS _api_impls - I would prefer to keep it in a followup PR.

But, while we're discussing it: having looked into it, I agree that SubscriptionFactory is currently not as clean as it could be. But, rather than going further into factory-ness, I would actually argue for the opposite: get rid of SubscriptionFactory, and just have subscriptionFromConfigSource() as a standalone function. SubscriptionFactory is not currently a real factory, and rather is just a Java-style wrapper around what is more naturally a standalone function. As a result, nothing ever deals with a SubscriptionFactory object, and I don't see any way it would be beneficial.

[htuch]

It would be beneficial in the same way any dependency injection is beneficial; you can create and inject mocks. Anyway, let's agree to do this later as we need to unblock end users, but please keep this on your agenda, as we should have tests.

[fredlas]

So, kind of centralizing what ClusterManagerFactory::createCds, ListenerComponentFactory::createLdsApi, etc currently do?

[fredlas]

I take that back, that wouldn't make sense. CdsApi / LdsApi are not sibling subclasses; they are different interfaces from include/. That's why tests are getting e.g. their MockCdsApis specifically from a mocked ClusterManagerFactory. Maybe there is some improvement to be made, but it seems like it would be a serious overhaul of the structure of the whole [xds]Api setup.

[kyessenov]

Can you explain how we can make the control plane work with both old and new envoy?
There is no more StreamClusters method so the old envoy would be broken with the new control plane.

[fredlas]

There still is a StreamClusters, or rather, these changes are what add it back. (Or maybe "reactivate" or "reenable" would be more appropriate).