Add a provision to allow specification of separate set of channel args for the channel to grpclb server (#30755)

* Add a provision to allow specification of separate set of channel args for the grpclb channel

* fix asan issue

* review comments

* review comments

* add missing file

* remove unused hdr

* fix sanity

* fix comments

* remove unused hdr

include/grpc/impl/codegen/grpc_types.h

@@ -368,6 +368,12 @@ typedef struct {
    balancer before using fallback backend addresses from the resolver.
    If 0, enter fallback mode immediately. Default value is 10000. */
 #define GRPC_ARG_GRPCLB_FALLBACK_TIMEOUT_MS "grpc.grpclb_fallback_timeout_ms"
+/* Experimental Arg. Channel args to be used for the control-plane channel
+ * created to the grpclb load balancers. This is a pointer arg whose value is a
+ * grpc_channel_args object. If unset, most channel args from the parent channel
+ * will be propagated to the grpclb channel. */
+#define GRPC_ARG_EXPERIMENTAL_GRPCLB_CHANNEL_ARGS \
+  "grpc.experimental.grpclb_channel_args"
 /* Timeout in milliseconds to wait for the child of a specific priority to
    complete its initial connection attempt before the priority LB policy fails
    over to the next priority. Default value is 10 seconds. */

src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc

@@ -1358,6 +1358,47 @@ ServerAddressList ExtractBalancerAddresses(const ChannelArgs& args) {
 ChannelArgs BuildBalancerChannelArgs(
     FakeResolverResponseGenerator* response_generator,
     const ChannelArgs& args) {
+  ChannelArgs grpclb_channel_args;
+  const grpc_channel_args* lb_channel_specific_args =
+      args.GetPointer<grpc_channel_args>(
+          GRPC_ARG_EXPERIMENTAL_GRPCLB_CHANNEL_ARGS);
+  if (lb_channel_specific_args != nullptr) {
+    grpclb_channel_args = ChannelArgs::FromC(lb_channel_specific_args);
+  } else {
+    // Set grpclb_channel_args based on the parent channel's channel args.
+    grpclb_channel_args =
+        args
+            // LB policy name, since we want to use the default (pick_first) in
+            // the LB channel.
+            .Remove(GRPC_ARG_LB_POLICY_NAME)
+            // Strip out the service config, since we don't want the LB policy
+            // config specified for the parent channel to affect the LB channel.
+            .Remove(GRPC_ARG_SERVICE_CONFIG)
+            // The channel arg for the server URI, since that will be different
+            // for the LB channel than for the parent channel.  The client
+            // channel factory will re-add this arg with the right value.
+            .Remove(GRPC_ARG_SERVER_URI)
+            // The fake resolver response generator, because we are replacing it
+            // with the one from the grpclb policy, used to propagate updates to
+            // the LB channel.
+            .Remove(GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR)
+            // The LB channel should use the authority indicated by the target
+            // authority table (see \a ModifyGrpclbBalancerChannelArgs),
+            // as opposed to the authority from the parent channel.
+            .Remove(GRPC_ARG_DEFAULT_AUTHORITY)
+            // Just as for \a GRPC_ARG_DEFAULT_AUTHORITY, the LB channel should
+            // be treated as a stand-alone channel and not inherit this argument
+            // from the args of the parent channel.
+            .Remove(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)
+            // Don't want to pass down channelz node from parent; the balancer
+            // channel will get its own.
+            .Remove(GRPC_ARG_CHANNELZ_CHANNEL_NODE)
+            // Remove the channel args for channel credentials and replace it
+            // with a version that does not contain call credentials. The
+            // loadbalancer is not necessarily trusted to handle bearer token
+            // credentials.
+            .Remove(GRPC_ARG_CHANNEL_CREDENTIALS);
+  }
   // Create channel args for channel credentials that does not contain bearer
   // token credentials.
   auto* channel_credentials = args.GetObject<grpc_channel_credentials>();
@@ -1365,36 +1406,7 @@ ChannelArgs BuildBalancerChannelArgs(
   RefCountedPtr<grpc_channel_credentials> creds_sans_call_creds =
       channel_credentials->duplicate_without_call_credentials();
   GPR_ASSERT(creds_sans_call_creds != nullptr);
-  return args
+  return grpclb_channel_args
-      // LB policy name, since we want to use the default (pick_first) in
-      // the LB channel.
-      .Remove(GRPC_ARG_LB_POLICY_NAME)
-      // Strip out the service config, since we don't want the LB policy
-      // config specified for the parent channel to affect the LB channel.
-      .Remove(GRPC_ARG_SERVICE_CONFIG)
-      // The channel arg for the server URI, since that will be different for
-      // the LB channel than for the parent channel.  The client channel
-      // factory will re-add this arg with the right value.
-      .Remove(GRPC_ARG_SERVER_URI)
-      // The fake resolver response generator, because we are replacing it
-      // with the one from the grpclb policy, used to propagate updates to
-      // the LB channel.
-      .Remove(GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR)
-      // The LB channel should use the authority indicated by the target
-      // authority table (see \a ModifyGrpclbBalancerChannelArgs),
-      // as opposed to the authority from the parent channel.
-      .Remove(GRPC_ARG_DEFAULT_AUTHORITY)
-      // Just as for \a GRPC_ARG_DEFAULT_AUTHORITY, the LB channel should be
-      // treated as a stand-alone channel and not inherit this argument from the
-      // args of the parent channel.
-      .Remove(GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)
-      // Don't want to pass down channelz node from parent; the balancer
-      // channel will get its own.
-      .Remove(GRPC_ARG_CHANNELZ_CHANNEL_NODE)
-      // Remove the channel args for channel credentials and replace it
-      // with a version that does not contain call credentials. The loadbalancer
-      // is not necessarily trusted to handle bearer token credentials.
-      .Remove(GRPC_ARG_CHANNEL_CREDENTIALS)
       // A channel arg indicating the target is a grpclb load balancer.
       .Set(GRPC_ARG_ADDRESS_IS_GRPCLB_LOAD_BALANCER, 1)
       // Tells channelz that this is an internal channel.

test/cpp/end2end/grpclb_end2end_test.cc

@@ -45,6 +45,7 @@
 #include "src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb_balancer_addresses.h"
 #include "src/core/ext/filters/client_channel/resolver/fake/fake_resolver.h"
 #include "src/core/lib/address_utils/parse_address.h"
+#include "src/core/lib/channel/channel_args.h"
 #include "src/core/lib/gpr/env.h"
 #include "src/core/lib/gprpp/ref_counted_ptr.h"
 #include "src/core/lib/iomgr/sockaddr.h"
@@ -102,12 +103,22 @@ using BalancerService = CountedService<LoadBalancer::Service>;
 const char g_kCallCredsMdKey[] = "Balancer should not ...";
 const char g_kCallCredsMdValue[] = "... receive me";
 
+// A test user agent string sent by the client only to the grpclb loadbalancer.
+// The backend should not see this user-agent string.
+constexpr char kGrpclbSpecificUserAgentString[] = "grpc-grpclb-test-user-agent";
+
 class BackendServiceImpl : public BackendService {
  public:
   BackendServiceImpl() {}
 
   Status Echo(ServerContext* context, const EchoRequest* request,
               EchoResponse* response) override {
+    // The backend should not see a test user agent configured at the client
+    // using GRPC_ARG_GRPCLB_CHANNEL_ARGS.
+    auto it = context->client_metadata().find("user-agent");
+    if (it != context->client_metadata().end()) {
+      EXPECT_FALSE(it->second.starts_with(kGrpclbSpecificUserAgentString));
+    }
     // Backend should receive the call credentials metadata.
     auto call_credentials_entry =
         context->client_metadata().find(g_kCallCredsMdKey);
@@ -198,6 +209,15 @@ class BalancerServiceImpl : public BalancerService {
       if (serverlist_done_) goto done;
     }
     {
+      // The loadbalancer should see a test user agent because it was
+      // specifically configured at the client using
+      // GRPC_ARG_GRPCLB_CHANNEL_ARGS
+      auto it = context->client_metadata().find("user-agent");
+      EXPECT_TRUE(it != context->client_metadata().end());
+      if (it != context->client_metadata().end()) {
+        EXPECT_THAT(std::string(it->second.data(), it->second.length()),
+                    ::testing::StartsWith(kGrpclbSpecificUserAgentString));
+      }
       // Balancer shouldn't receive the call credentials metadata.
       EXPECT_EQ(context->client_metadata().find(g_kCallCredsMdKey),
                 context->client_metadata().end());
@@ -406,17 +426,45 @@ class GrpclbEnd2endTest : public ::testing::Test {
   void ResetStub(int fallback_timeout = 0,
                  const std::string& expected_targets = "",
                  int subchannel_cache_delay_ms = 0) {
+    // Send a separate user agent string for the grpclb load balancer alone.
+    grpc_core::ChannelArgs grpclb_channel_args;
+    // Set a special user agent string for the grpclb load balancer. It
+    // will be verified at the load balancer.
+    grpclb_channel_args = grpclb_channel_args.Set(
+        GRPC_ARG_PRIMARY_USER_AGENT_STRING, kGrpclbSpecificUserAgentString);
     ChannelArguments args;
     if (fallback_timeout > 0) args.SetGrpclbFallbackTimeout(fallback_timeout);
     args.SetPointer(GRPC_ARG_FAKE_RESOLVER_RESPONSE_GENERATOR,
                     response_generator_.get());
     if (!expected_targets.empty()) {
       args.SetString(GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS, expected_targets);
+      grpclb_channel_args = grpclb_channel_args.Set(
+          GRPC_ARG_FAKE_SECURITY_EXPECTED_TARGETS, expected_targets);
     }
     if (subchannel_cache_delay_ms > 0) {
       args.SetInt(GRPC_ARG_GRPCLB_SUBCHANNEL_CACHE_INTERVAL_MS,
                   subchannel_cache_delay_ms * grpc_test_slowdown_factor());
     }
+    static const grpc_arg_pointer_vtable channel_args_vtable = {
+        // copy
+        [](void* p) -> void* {
+          return grpc_channel_args_copy(static_cast<grpc_channel_args*>(p));
+        },
+        // destroy
+        [](void* p) {
+          grpc_channel_args_destroy(static_cast<grpc_channel_args*>(p));
+        },
+        // compare
+        [](void* p1, void* p2) {
+          return grpc_channel_args_compare(static_cast<grpc_channel_args*>(p1),
+                                           static_cast<grpc_channel_args*>(p2));
+        },
+    };
+    // Specify channel args for the channel to the load balancer.
+    args.SetPointerWithVtable(
+        GRPC_ARG_EXPERIMENTAL_GRPCLB_CHANNEL_ARGS,
+        const_cast<grpc_channel_args*>(grpclb_channel_args.ToC().get()),
+        &channel_args_vtable);
     std::ostringstream uri;
     uri << "fake:///" << kApplicationTargetName_;
     // TODO(dgq): templatize tests to run everything using both secure and