Merge pull request grpc#3035 from ctiller/naming-crisis

Refactor default host name resolution
crast · Aug 26, 2015 · 52730d3 · 52730d3
2 parents fd70b71 + 26fd1a3
commit 52730d3
Show file tree

Hide file tree

Showing 14 changed files with 190 additions and 129 deletions.
diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h
@@ -134,6 +134,14 @@ typedef struct {
 /** Secondary user agent: goes at the end of the user-agent metadata
     sent on each request */
 #define GRPC_ARG_SECONDARY_USER_AGENT_STRING "grpc.secondary_user_agent"
+/* The caller of the secure_channel_create functions may override the target
+   name used for SSL host name checking using this channel argument which is of
+   type GRPC_ARG_STRING. This *should* be used for testing only.
+   If this argument is not specified, the name used for SSL host name checking
+   will be the target parameter (assuming that the secure channel is an SSL
+   channel). If this parameter is specified and the underlying is not an SSL
+   channel, it will just be ignored. */
+#define GRPC_SSL_TARGET_NAME_OVERRIDE_ARG "grpc.ssl_target_name_override"
 
 /** Connectivity state of a channel. */
 typedef enum {

diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h
@@ -142,15 +142,6 @@ grpc_credentials *grpc_iam_credentials_create(const char *authorization_token,
 
 /* --- Secure channel creation. --- */
 
-/* The caller of the secure_channel_create functions may override the target
-   name used for SSL host name checking using this channel argument which is of
-   type GRPC_ARG_STRING. This *should* be used for testing only.
-   If this argument is not specified, the name used for SSL host name checking
-   will be the target parameter (assuming that the secure channel is an SSL
-   channel). If this parameter is specified and the underlying is not an SSL
-   channel, it will just be ignored. */
-#define GRPC_SSL_TARGET_NAME_OVERRIDE_ARG "grpc.ssl_target_name_override"
-
 /* Creates a secure channel using the passed-in credentials. */
 grpc_channel *grpc_secure_channel_create(grpc_credentials *creds,
                                          const char *target,

diff --git a/src/core/channel/http_client_filter.c b/src/core/channel/http_client_filter.c
@@ -45,7 +45,6 @@ typedef struct call_data {
   grpc_linked_mdelem content_type;
   grpc_linked_mdelem user_agent;
   int sent_initial_metadata;
-  int sent_authority;
 
   int got_initial_metadata;
   grpc_stream_op_buffer *recv_ops;
@@ -64,7 +63,6 @@ typedef struct channel_data {
   grpc_mdelem *scheme;
   grpc_mdelem *content_type;
   grpc_mdelem *status;
-  grpc_mdelem *default_authority;
   /** complete user agent mdelem */
   grpc_mdelem *user_agent;
 } channel_data;
@@ -103,18 +101,13 @@ static void hc_on_recv(void *user_data, int success) {
 
 static grpc_mdelem *client_strip_filter(void *user_data, grpc_mdelem *md) {
   grpc_call_element *elem = user_data;
-  call_data *calld = elem->call_data;
   channel_data *channeld = elem->channel_data;
   /* eat the things we'd like to set ourselves */
   if (md->key == channeld->method->key) return NULL;
   if (md->key == channeld->scheme->key) return NULL;
   if (md->key == channeld->te_trailers->key) return NULL;
   if (md->key == channeld->content_type->key) return NULL;
   if (md->key == channeld->user_agent->key) return NULL;
-  if (channeld->default_authority &&
-      channeld->default_authority->key == md->key) {
-    calld->sent_authority = 1;
-  }
   return md;
 }
 
@@ -138,11 +131,6 @@ static void hc_mutate_op(grpc_call_element *elem,
                                    GRPC_MDELEM_REF(channeld->method));
       grpc_metadata_batch_add_head(&op->data.metadata, &calld->scheme,
                                    GRPC_MDELEM_REF(channeld->scheme));
-      if (channeld->default_authority && !calld->sent_authority) {
-        grpc_metadata_batch_add_head(
-            &op->data.metadata, &calld->authority,
-            GRPC_MDELEM_REF(channeld->default_authority));
-      }
       grpc_metadata_batch_add_tail(&op->data.metadata, &calld->te_trailers,
                                    GRPC_MDELEM_REF(channeld->te_trailers));
       grpc_metadata_batch_add_tail(&op->data.metadata, &calld->content_type,
@@ -175,7 +163,6 @@ static void init_call_elem(grpc_call_element *elem,
   call_data *calld = elem->call_data;
   calld->sent_initial_metadata = 0;
   calld->got_initial_metadata = 0;
-  calld->sent_authority = 0;
   calld->on_done_recv = NULL;
   grpc_iomgr_closure_init(&calld->hc_on_recv, hc_on_recv, elem);
   if (initial_op) hc_mutate_op(elem, initial_op);
@@ -257,8 +244,6 @@ static grpc_mdstr *user_agent_from_args(grpc_mdctx *mdctx,
 static void init_channel_elem(grpc_channel_element *elem, grpc_channel *master,
                               const grpc_channel_args *channel_args,
                               grpc_mdctx *mdctx, int is_first, int is_last) {
-  size_t i;
-
   /* grab pointers to our data from the channel element */
   channel_data *channeld = elem->channel_data;
 
@@ -267,21 +252,6 @@ static void init_channel_elem(grpc_channel_element *elem, grpc_channel *master,
      path */
   GPR_ASSERT(!is_last);
 
-  channeld->default_authority = NULL;
-  if (channel_args) {
-    for (i = 0; i < channel_args->num_args; i++) {
-      if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) {
-        if (channel_args->args[i].type != GRPC_ARG_STRING) {
-          gpr_log(GPR_ERROR, "%s: must be an string",
-                  GRPC_ARG_DEFAULT_AUTHORITY);
-        } else {
-          channeld->default_authority = grpc_mdelem_from_strings(
-              mdctx, ":authority", channel_args->args[i].value.string);
-        }
-      }
-    }
-  }
-
   /* initialize members */
   channeld->te_trailers = grpc_mdelem_from_strings(mdctx, "te", "trailers");
   channeld->method = grpc_mdelem_from_strings(mdctx, ":method", "POST");
@@ -306,9 +276,6 @@ static void destroy_channel_elem(grpc_channel_element *elem) {
   GRPC_MDELEM_UNREF(channeld->content_type);
   GRPC_MDELEM_UNREF(channeld->status);
   GRPC_MDELEM_UNREF(channeld->user_agent);
-  if (channeld->default_authority) {
-    GRPC_MDELEM_UNREF(channeld->default_authority);
-  }
 }
 
 const grpc_channel_filter grpc_http_client_filter = {

diff --git a/src/core/client_config/resolver_factory.c b/src/core/client_config/resolver_factory.c
@@ -45,6 +45,12 @@ void grpc_resolver_factory_unref(grpc_resolver_factory *factory) {
 grpc_resolver *grpc_resolver_factory_create_resolver(
     grpc_resolver_factory *factory, grpc_uri *uri,
     grpc_subchannel_factory *subchannel_factory) {
-  if (!factory) return NULL;
+  if (factory == NULL) return NULL;
   return factory->vtable->create_resolver(factory, uri, subchannel_factory);
 }
+
+char *grpc_resolver_factory_get_default_authority(
+    grpc_resolver_factory *factory, grpc_uri *uri) {
+  if (factory == NULL) return NULL;
+  return factory->vtable->get_default_authority(factory, uri);
+}
diff --git a/src/core/client_config/resolver_factory.h b/src/core/client_config/resolver_factory.h
@@ -51,9 +51,16 @@ struct grpc_resolver_factory_vtable {
   void (*ref)(grpc_resolver_factory *factory);
   void (*unref)(grpc_resolver_factory *factory);
 
+  /** Implementation of grpc_resolver_factory_create_resolver */
   grpc_resolver *(*create_resolver)(
       grpc_resolver_factory *factory, grpc_uri *uri,
       grpc_subchannel_factory *subchannel_factory);
+
+  /** Implementation of grpc_resolver_factory_get_default_authority */
+  char *(*get_default_authority)(grpc_resolver_factory *factory, grpc_uri *uri);
+
+  /** URI scheme that this factory implements */
+  const char *scheme;
 };
 
 void grpc_resolver_factory_ref(grpc_resolver_factory *resolver);
@@ -64,4 +71,9 @@ grpc_resolver *grpc_resolver_factory_create_resolver(
     grpc_resolver_factory *factory, grpc_uri *uri,
     grpc_subchannel_factory *subchannel_factory);
 
+/** Return a (freshly allocated with gpr_malloc) string representing
+    the default authority to use for this scheme. */
+char *grpc_resolver_factory_get_default_authority(
+    grpc_resolver_factory *factory, grpc_uri *uri);
+
 #endif /* GRPC_INTERNAL_CORE_CONFIG_RESOLVER_FACTORY_H */
diff --git a/src/core/client_config/resolver_registry.c b/src/core/client_config/resolver_registry.c
@@ -41,41 +41,33 @@
 
 #define MAX_RESOLVERS 10
 
-typedef struct {
-  char *scheme;
-  grpc_resolver_factory *factory;
-} registered_resolver;
-
-static registered_resolver g_all_of_the_resolvers[MAX_RESOLVERS];
+static grpc_resolver_factory *g_all_of_the_resolvers[MAX_RESOLVERS];
 static int g_number_of_resolvers = 0;
 
-static char *g_default_resolver_scheme;
+static char *g_default_resolver_prefix;
 
-void grpc_resolver_registry_init(const char *default_resolver_scheme) {
+void grpc_resolver_registry_init(const char *default_resolver_prefix) {
   g_number_of_resolvers = 0;
-  g_default_resolver_scheme = gpr_strdup(default_resolver_scheme);
+  g_default_resolver_prefix = gpr_strdup(default_resolver_prefix);
 }
 
 void grpc_resolver_registry_shutdown(void) {
   int i;
   for (i = 0; i < g_number_of_resolvers; i++) {
-    gpr_free(g_all_of_the_resolvers[i].scheme);
-    grpc_resolver_factory_unref(g_all_of_the_resolvers[i].factory);
+    grpc_resolver_factory_unref(g_all_of_the_resolvers[i]);
   }
-  gpr_free(g_default_resolver_scheme);
+  gpr_free(g_default_resolver_prefix);
 }
 
-void grpc_register_resolver_type(const char *scheme,
-                                 grpc_resolver_factory *factory) {
+void grpc_register_resolver_type(grpc_resolver_factory *factory) {
   int i;
   for (i = 0; i < g_number_of_resolvers; i++) {
-    GPR_ASSERT(0 != strcmp(scheme, g_all_of_the_resolvers[i].scheme));
+    GPR_ASSERT(0 != strcmp(factory->vtable->scheme,
+                           g_all_of_the_resolvers[i]->vtable->scheme));
   }
   GPR_ASSERT(g_number_of_resolvers != MAX_RESOLVERS);
-  g_all_of_the_resolvers[g_number_of_resolvers].scheme = gpr_strdup(scheme);
   grpc_resolver_factory_ref(factory);
-  g_all_of_the_resolvers[g_number_of_resolvers].factory = factory;
-  g_number_of_resolvers++;
+  g_all_of_the_resolvers[g_number_of_resolvers++] = factory;
 }
 
 static grpc_resolver_factory *lookup_factory(grpc_uri *uri) {
@@ -85,40 +77,57 @@ static grpc_resolver_factory *lookup_factory(grpc_uri *uri) {
   if (!uri) return NULL;
 
   for (i = 0; i < g_number_of_resolvers; i++) {
-    if (0 == strcmp(uri->scheme, g_all_of_the_resolvers[i].scheme)) {
-      return g_all_of_the_resolvers[i].factory;
+    if (0 == strcmp(uri->scheme, g_all_of_the_resolvers[i]->vtable->scheme)) {
+      return g_all_of_the_resolvers[i];
     }
   }
 
   return NULL;
 }
 
-grpc_resolver *grpc_resolver_create(
-    const char *name, grpc_subchannel_factory *subchannel_factory) {
-  grpc_uri *uri;
+static grpc_resolver_factory *resolve_factory(const char *target,
+                                              grpc_uri **uri) {
   char *tmp;
   grpc_resolver_factory *factory = NULL;
-  grpc_resolver *resolver;
-
-  uri = grpc_uri_parse(name, 1);
-  factory = lookup_factory(uri);
-  if (factory == NULL && g_default_resolver_scheme != NULL) {
-    grpc_uri_destroy(uri);
-    gpr_asprintf(&tmp, "%s%s", g_default_resolver_scheme, name);
-    uri = grpc_uri_parse(tmp, 1);
-    factory = lookup_factory(uri);
-    if (factory == NULL) {
-      grpc_uri_destroy(grpc_uri_parse(name, 0));
-      grpc_uri_destroy(grpc_uri_parse(tmp, 0));
-      gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", name, tmp);
+
+  GPR_ASSERT(uri != NULL);
+  *uri = grpc_uri_parse(target, 1);
+  factory = lookup_factory(*uri);
+  if (factory == NULL) {
+    if (g_default_resolver_prefix != NULL) {
+      grpc_uri_destroy(*uri);
+      gpr_asprintf(&tmp, "%s%s", g_default_resolver_prefix, target);
+      *uri = grpc_uri_parse(tmp, 1);
+      factory = lookup_factory(*uri);
+      if (factory == NULL) {
+        grpc_uri_destroy(grpc_uri_parse(target, 0));
+        grpc_uri_destroy(grpc_uri_parse(tmp, 0));
+        gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target,
+                tmp);
+      }
+      gpr_free(tmp);
+    } else {
+      grpc_uri_destroy(grpc_uri_parse(target, 0));
+      gpr_log(GPR_ERROR, "don't know how to resolve '%s'", target);
     }
-    gpr_free(tmp);
-  } else if (factory == NULL) {
-    grpc_uri_destroy(grpc_uri_parse(name, 0));
-    gpr_log(GPR_ERROR, "don't know how to resolve '%s'", name);
   }
-  resolver =
+  return factory;
+}
+
+grpc_resolver *grpc_resolver_create(
+    const char *target, grpc_subchannel_factory *subchannel_factory) {
+  grpc_uri *uri = NULL;
+  grpc_resolver_factory *factory = resolve_factory(target, &uri);
+  grpc_resolver *resolver =
       grpc_resolver_factory_create_resolver(factory, uri, subchannel_factory);
   grpc_uri_destroy(uri);
   return resolver;
 }
+
+char *grpc_get_default_authority(const char *target) {
+  grpc_uri *uri = NULL;
+  grpc_resolver_factory *factory = resolve_factory(target, &uri);
+  char *authority = grpc_resolver_factory_get_default_authority(factory, uri);
+  grpc_uri_destroy(uri);
+  return authority;
+}
diff --git a/src/core/client_config/resolver_registry.h b/src/core/client_config/resolver_registry.h
@@ -44,19 +44,22 @@ void grpc_resolver_registry_shutdown(void);
     If \a priority is greater than zero, then the resolver will be eligible
     to resolve names that are passed in with no scheme. Higher priority
     resolvers will be tried before lower priority schemes. */
-void grpc_register_resolver_type(const char *scheme,
-                                 grpc_resolver_factory *factory);
+void grpc_register_resolver_type(grpc_resolver_factory *factory);
 
-/** Create a resolver given \a name.
-    First tries to parse \a name as a URI. If this succeeds, tries
+/** Create a resolver given \a target.
+    First tries to parse \a target as a URI. If this succeeds, tries
     to locate a registered resolver factory based on the URI scheme.
     If parsing or location fails, prefixes default_prefix from
-    grpc_resolver_registry_init to name, and tries again (if default_prefix
+    grpc_resolver_registry_init to target, and tries again (if default_prefix
     was not NULL).
     If a resolver factory was found, use it to instantiate a resolver and
     return it.
     If a resolver factory was not found, return NULL. */
 grpc_resolver *grpc_resolver_create(
-    const char *name, grpc_subchannel_factory *subchannel_factory);
+    const char *target, grpc_subchannel_factory *subchannel_factory);
+
+/** Given a target, return a (freshly allocated with gpr_malloc) string
+    representing the default authority to pass from a client. */
+char *grpc_get_default_authority(const char *target);
 
 #endif /* GRPC_INTERNAL_CORE_CLIENT_CONFIG_RESOLVER_REGISTRY_H */
diff --git a/src/core/client_config/resolvers/dns_resolver.c b/src/core/client_config/resolvers/dns_resolver.c
@@ -203,9 +203,6 @@ static grpc_resolver *dns_create(
     grpc_subchannel_factory *subchannel_factory) {
   dns_resolver *r;
   const char *path = uri->path;
-  grpc_arg default_host_arg;
-  char *host;
-  char *port;
 
   if (0 != strcmp(uri->authority, "")) {
     gpr_log(GPR_ERROR, "authority based uri's not supported");
@@ -214,17 +211,6 @@ static grpc_resolver *dns_create(
 
   if (path[0] == '/') ++path;
 
-  gpr_split_host_port(path, &host, &port);
-
-  default_host_arg.type = GRPC_ARG_STRING;
-  default_host_arg.key = GRPC_ARG_DEFAULT_AUTHORITY;
-  default_host_arg.value.string = host;
-  subchannel_factory = grpc_subchannel_factory_add_channel_arg(
-      subchannel_factory, &default_host_arg);
-
-  gpr_free(host);
-  gpr_free(port);
-
   r = gpr_malloc(sizeof(dns_resolver));
   memset(r, 0, sizeof(*r));
   gpr_ref_init(&r->refs, 1);
@@ -233,6 +219,7 @@ static grpc_resolver *dns_create(
   r->name = gpr_strdup(path);
   r->default_port = gpr_strdup(default_port);
   r->subchannel_factory = subchannel_factory;
+  grpc_subchannel_factory_ref(subchannel_factory);
   r->lb_policy_factory = lb_policy_factory;
   return &r->base;
 }
@@ -252,8 +239,16 @@ static grpc_resolver *dns_factory_create_resolver(
                     subchannel_factory);
 }
 
+char *dns_factory_get_default_host_name(grpc_resolver_factory *factory,
+                                        grpc_uri *uri) {
+  const char *path = uri->path;
+  if (path[0] == '/') ++path;
+  return gpr_strdup(path);
+}
+
 static const grpc_resolver_factory_vtable dns_factory_vtable = {
-    dns_factory_ref, dns_factory_unref, dns_factory_create_resolver};
+    dns_factory_ref, dns_factory_unref, dns_factory_create_resolver,
+    dns_factory_get_default_host_name, "dns"};
 static grpc_resolver_factory dns_resolver_factory = {&dns_factory_vtable};
 
 grpc_resolver_factory *grpc_dns_resolver_factory_create() {