Merge pull request #8263 from markdroth/grpclb_resolver_semantic_fix

Centralized logic for choosing the right LB policy.

src/core/ext/client_config/client_channel.c

@@ -185,10 +185,34 @@ static void on_resolver_result_changed(grpc_exec_ctx *exec_ctx, void *arg,
     lb_policy_args.additional_args =
         grpc_resolver_result_get_lb_policy_args(chand->resolver_result);
     lb_policy_args.client_channel_factory = chand->client_channel_factory;
-    lb_policy = grpc_lb_policy_create(
-        exec_ctx,
-        grpc_resolver_result_get_lb_policy_name(chand->resolver_result),
-        &lb_policy_args);
+
+    // Special case: If all of the addresses are balancer addresses,
+    // assume that we should use the grpclb policy, regardless of what the
+    // resolver actually specified.
+    const char* lb_policy_name =
+        grpc_resolver_result_get_lb_policy_name(chand->resolver_result);
+    bool found_backend_address = false;
+    for (size_t i = 0; i < lb_policy_args.addresses->num_addresses; ++i) {
+      if (!lb_policy_args.addresses->addresses[i].is_balancer) {
+        found_backend_address = true;
+        break;
+      }
+    }
+    if (!found_backend_address) {
+      if (lb_policy_name != NULL && strcmp(lb_policy_name, "grpclb") != 0) {
+        gpr_log(GPR_INFO,
+                "resolver requested LB policy %s but provided only balancer "
+                "addresses, no backend addresses -- forcing use of grpclb LB "
+                "policy", (lb_policy_name == NULL ? "(none)" : lb_policy_name));
+      }
+      lb_policy_name = "grpclb";
+    }
+    // Use pick_first if nothing was specified and we didn't select grpclb
+    // above.
+    if (lb_policy_name == NULL) lb_policy_name = "pick_first";
+
+    lb_policy =
+        grpc_lb_policy_create(exec_ctx, lb_policy_name, &lb_policy_args);
     if (lb_policy != NULL) {
       GRPC_LB_POLICY_REF(lb_policy, "config_change");
       GRPC_ERROR_UNREF(state_error);

src/core/ext/resolver/dns/native/dns_resolver.c

@@ -61,8 +61,6 @@ typedef struct {
   char *name_to_resolve;
   /** default port to use */
   char *default_port;
-  /** load balancing policy name */
-  char *lb_policy_name;
 
   /** mutex guarding the rest of the state */
   gpr_mu mu;
@@ -181,7 +179,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg,
     }
     grpc_resolved_addresses_destroy(r->addresses);
     result = grpc_resolver_result_create(r->target_name, addresses,
-                                         r->lb_policy_name, NULL);
+                                         NULL /* lb_policy_name */, NULL);
   } else {
     gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC);
     gpr_timespec next_try = gpr_backoff_step(&r->backoff_state, now);
@@ -245,13 +243,11 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
   gpr_free(r->target_name);
   gpr_free(r->name_to_resolve);
   gpr_free(r->default_port);
-  gpr_free(r->lb_policy_name);
   gpr_free(r);
 }
 
 static grpc_resolver *dns_create(grpc_resolver_args *args,
-                                 const char *default_port,
-                                 const char *lb_policy_name) {
+                                 const char *default_port) {
   if (0 != strcmp(args->uri->authority, "")) {
     gpr_log(GPR_ERROR, "authority based dns uri's not supported");
     return NULL;
@@ -272,7 +268,6 @@ static grpc_resolver *dns_create(grpc_resolver_args *args,
   r->default_port = gpr_strdup(default_port);
   gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER,
                    BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000);
-  r->lb_policy_name = gpr_strdup(lb_policy_name);
   return &r->base;
 }
 
@@ -286,7 +281,7 @@ static void dns_factory_unref(grpc_resolver_factory *factory) {}
 
 static grpc_resolver *dns_factory_create_resolver(
     grpc_resolver_factory *factory, grpc_resolver_args *args) {
-  return dns_create(args, "https", "pick_first");
+  return dns_create(args, "https");
 }
 
 static char *dns_factory_get_default_host_name(grpc_resolver_factory *factory,

src/core/ext/resolver/sockaddr/sockaddr_resolver.c

@@ -164,7 +164,7 @@ char *unix_get_default_authority(grpc_resolver_factory *factory,
 static void do_nothing(void *ignored) {}
 
 static grpc_resolver *sockaddr_create(
-    grpc_resolver_args *args, const char *default_lb_policy_name,
+    grpc_resolver_args *args,
     int parse(grpc_uri *uri, struct sockaddr_storage *dst, size_t *len)) {
   bool errors_found = false;
   sockaddr_resolver *r;
@@ -198,10 +198,6 @@ static grpc_resolver *sockaddr_create(
     abort();
   }
 
-  if (r->lb_policy_name == NULL) {
-    r->lb_policy_name = gpr_strdup(default_lb_policy_name);
-  }
-
   path_slice =
       gpr_slice_new(args->uri->path, strlen(args->uri->path), do_nothing);
   gpr_slice_buffer_init(&path_parts);
@@ -251,7 +247,7 @@ static void sockaddr_factory_unref(grpc_resolver_factory *factory) {}
 #define DECL_FACTORY(name)                                                  \
   static grpc_resolver *name##_factory_create_resolver(                     \
       grpc_resolver_factory *factory, grpc_resolver_args *args) {           \
-    return sockaddr_create(args, "pick_first", parse_##name);               \
+    return sockaddr_create(args, parse_##name);                             \
   }                                                                         \
   static const grpc_resolver_factory_vtable name##_factory_vtable = {       \
       sockaddr_factory_ref, sockaddr_factory_unref,                         \