From bc85be10ef420f35d660bc12afb47580acec3143 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 24 Aug 2015 10:36:39 -0700 Subject: [PATCH 1/8] Refactor default host name resolution Rephrase API's such that resolvers are constrained to be able to provide a default host given just the text of the URI channel target. This avoids needing to rewrite such details in the core library during retries, and generally makes things much saner to debug. --- src/core/channel/http_client_filter.c | 33 -------- src/core/client_config/resolver_factory.c | 6 ++ src/core/client_config/resolver_factory.h | 9 +++ src/core/client_config/resolver_registry.c | 79 ++++++++++--------- src/core/client_config/resolver_registry.h | 15 ++-- .../client_config/resolvers/dns_resolver.c | 25 +++--- .../resolvers/sockaddr_resolver.c | 49 ++++++++---- .../resolvers/zookeeper_resolver.c | 11 ++- src/core/surface/channel.c | 24 ++++++ src/core/surface/init.c | 8 +- test/core/end2end/tests/default_host.c | 2 +- 11 files changed, 148 insertions(+), 113 deletions(-) diff --git a/src/core/channel/http_client_filter.c b/src/core/channel/http_client_filter.c index 48c623d3591..2b61d33c29d 100644 --- 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,7 +101,6 @@ 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; @@ -111,10 +108,6 @@ static grpc_mdelem *client_strip_filter(void *user_data, grpc_mdelem *md) { 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 index 6721977e21a..bf631cefd13 100644 --- a/src/core/client_config/resolver_factory.c +++ b/src/core/client_config/resolver_factory.c @@ -48,3 +48,9 @@ grpc_resolver *grpc_resolver_factory_create_resolver( if (!factory) 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) 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 index c5d85499c6e..73a05642303 100644 --- a/src/core/client_config/resolver_factory.h +++ b/src/core/client_config/resolver_factory.h @@ -54,6 +54,10 @@ struct grpc_resolver_factory_vtable { grpc_resolver *(*create_resolver)( grpc_resolver_factory *factory, grpc_uri *uri, grpc_subchannel_factory *subchannel_factory); + + char *(*get_default_authority)(grpc_resolver_factory *factory, grpc_uri *uri); + + const char *scheme; }; void grpc_resolver_factory_ref(grpc_resolver_factory *resolver); @@ -64,4 +68,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 index 16be2da9940..3aff4630fba 100644 --- 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,53 @@ 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); + + *uri = grpc_uri_parse(target, 1); + factory = lookup_factory(*uri); + if (factory == NULL && 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(name, 0)); + 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'", name, tmp); + gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, tmp); } 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); + grpc_uri_destroy(grpc_uri_parse(target, 0)); + gpr_log(GPR_ERROR, "don't know how to resolve '%s'", target); } - 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 index 31aa47620ab..5a7193b7ae2 100644 --- 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 index 7b35b7902f7..84643c464a9 100644 --- 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() { diff --git a/src/core/client_config/resolvers/sockaddr_resolver.c b/src/core/client_config/resolvers/sockaddr_resolver.c index 74584e7e2c6..eeafef4174e 100644 --- a/src/core/client_config/resolvers/sockaddr_resolver.c +++ b/src/core/client_config/resolvers/sockaddr_resolver.c @@ -156,8 +156,29 @@ static int parse_unix(grpc_uri *uri, struct sockaddr_storage *addr, int *len) { return 1; } + +static char *unix_get_default_authority(grpc_resolver_factory *factory, + grpc_uri *uri) { + return gpr_strdup("localhost"); +} #endif +static char *ip_get_default_authority(grpc_uri *uri) { + const char *path = uri->path; + if (path[0] == '/') ++path; + return gpr_strdup(path); +} + +static char *ipv4_get_default_authority(grpc_resolver_factory *factory, + grpc_uri *uri) { + return ip_get_default_authority(uri); +} + +static char *ipv6_get_default_authority(grpc_resolver_factory *factory, + grpc_uri *uri) { + return ip_get_default_authority(uri); +} + static int parse_ipv4(grpc_uri *uri, struct sockaddr_storage *addr, int *len) { const char *host_port = uri->path; char *host; @@ -276,20 +297,20 @@ static void sockaddr_factory_ref(grpc_resolver_factory *factory) {} 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_uri *uri, \ - grpc_subchannel_factory *subchannel_factory) { \ - return sockaddr_create(uri, grpc_create_pick_first_lb_policy, \ - subchannel_factory, parse_##name); \ - } \ - static const grpc_resolver_factory_vtable name##_factory_vtable = { \ - sockaddr_factory_ref, sockaddr_factory_unref, \ - name##_factory_create_resolver}; \ - static grpc_resolver_factory name##_resolver_factory = { \ - &name##_factory_vtable}; \ - grpc_resolver_factory *grpc_##name##_resolver_factory_create() { \ - return &name##_resolver_factory; \ +#define DECL_FACTORY(name) \ + static grpc_resolver *name##_factory_create_resolver( \ + grpc_resolver_factory *factory, grpc_uri *uri, \ + grpc_subchannel_factory *subchannel_factory) { \ + return sockaddr_create(uri, grpc_create_pick_first_lb_policy, \ + subchannel_factory, parse_##name); \ + } \ + static const grpc_resolver_factory_vtable name##_factory_vtable = { \ + sockaddr_factory_ref, sockaddr_factory_unref, \ + name##_factory_create_resolver, name##_get_default_authority, #name}; \ + static grpc_resolver_factory name##_resolver_factory = { \ + &name##_factory_vtable}; \ + grpc_resolver_factory *grpc_##name##_resolver_factory_create() { \ + return &name##_resolver_factory; \ } #ifdef GPR_POSIX_SOCKET diff --git a/src/core/client_config/resolvers/zookeeper_resolver.c b/src/core/client_config/resolvers/zookeeper_resolver.c index acb2ba136e9..da399f99548 100644 --- a/src/core/client_config/resolvers/zookeeper_resolver.c +++ b/src/core/client_config/resolvers/zookeeper_resolver.c @@ -467,8 +467,7 @@ static grpc_resolver *zookeeper_create( } static void zookeeper_plugin_init() { - grpc_register_resolver_type("zookeeper", - grpc_zookeeper_resolver_factory_create()); + grpc_register_resolver_type(grpc_zookeeper_resolver_factory_create()); } void grpc_zookeeper_register() { @@ -483,6 +482,11 @@ static void zookeeper_factory_ref(grpc_resolver_factory *factory) {} static void zookeeper_factory_unref(grpc_resolver_factory *factory) {} +static char *zookeeper_factory_get_default_hostname( + grpc_resolver_factory *factory, grpc_uri *uri) { + return NULL; +} + static grpc_resolver *zookeeper_factory_create_resolver( grpc_resolver_factory *factory, grpc_uri *uri, grpc_subchannel_factory *subchannel_factory) { @@ -492,7 +496,8 @@ static grpc_resolver *zookeeper_factory_create_resolver( static const grpc_resolver_factory_vtable zookeeper_factory_vtable = { zookeeper_factory_ref, zookeeper_factory_unref, - zookeeper_factory_create_resolver}; + zookeeper_factory_create_resolver, zookeeper_factory_get_default_hostname, + "zookeeper"}; static grpc_resolver_factory zookeeper_resolver_factory = { &zookeeper_factory_vtable}; diff --git a/src/core/surface/channel.c b/src/core/surface/channel.c index e50251566d5..7378929ca4d 100644 --- a/src/core/surface/channel.c +++ b/src/core/surface/channel.c @@ -40,6 +40,7 @@ #include #include +#include "src/core/client_config/resolver_registry.h" #include "src/core/iomgr/iomgr.h" #include "src/core/support/string.h" #include "src/core/surface/call.h" @@ -70,6 +71,7 @@ struct grpc_channel { grpc_mdstr *grpc_message_string; grpc_mdstr *path_string; grpc_mdstr *authority_string; + grpc_mdelem *default_authority; /** mdelem for grpc-status: 0 thru grpc-status: 2 */ grpc_mdelem *grpc_status_elem[NUM_CACHED_STATUS_ELEMS]; @@ -134,10 +136,27 @@ grpc_channel *grpc_channel_create_from_filters( } else { channel->max_message_length = args->args[i].value.integer; } + } else if (0 == strcmp(args->args[i].key, GRPC_ARG_DEFAULT_AUTHORITY)) { + if (args->args[i].type != GRPC_ARG_STRING) { + gpr_log(GPR_ERROR, "%s: must be an string", + GRPC_ARG_DEFAULT_AUTHORITY); + } else { + channel->default_authority = grpc_mdelem_from_strings( + mdctx, ":authority", args->args[i].value.string); + } } } } + if (channel->is_client && channel->default_authority == NULL) { + char *default_authority = grpc_get_default_authority(target); + if (default_authority) { + channel->default_authority = grpc_mdelem_from_strings( + channel->metadata_context, ":authority", default_authority); + } + gpr_free(default_authority); + } + grpc_channel_stack_init(filters, num_filters, channel, args, channel->metadata_context, CHANNEL_STACK_FROM_CHANNEL(channel)); @@ -161,6 +180,8 @@ static grpc_call *grpc_channel_create_call_internal( send_metadata[num_metadata++] = path_mdelem; if (authority_mdelem != NULL) { send_metadata[num_metadata++] = authority_mdelem; + } else if (channel->default_authority != NULL) { + send_metadata[num_metadata++] = GRPC_MDELEM_REF(channel->default_authority); } return grpc_call_create(channel, parent_call, propagation_mask, cq, NULL, @@ -251,6 +272,9 @@ static void destroy_channel(void *p, int ok) { } gpr_free(rc); } + if (channel->default_authority != NULL) { + GRPC_MDELEM_UNREF(channel->default_authority); + } grpc_mdctx_unref(channel->metadata_context); gpr_mu_destroy(&channel->registered_call_mu); gpr_free(channel->target); diff --git a/src/core/surface/init.c b/src/core/surface/init.c index d9044549f21..0d48cd42d72 100644 --- a/src/core/surface/init.c +++ b/src/core/surface/init.c @@ -86,11 +86,11 @@ void grpc_init(void) { if (++g_initializations == 1) { gpr_time_init(); grpc_resolver_registry_init("dns:///"); - grpc_register_resolver_type("dns", grpc_dns_resolver_factory_create()); - grpc_register_resolver_type("ipv4", grpc_ipv4_resolver_factory_create()); - grpc_register_resolver_type("ipv6", grpc_ipv6_resolver_factory_create()); + grpc_register_resolver_type(grpc_dns_resolver_factory_create()); + grpc_register_resolver_type(grpc_ipv4_resolver_factory_create()); + grpc_register_resolver_type(grpc_ipv6_resolver_factory_create()); #ifdef GPR_POSIX_SOCKET - grpc_register_resolver_type("unix", grpc_unix_resolver_factory_create()); + grpc_register_resolver_type(grpc_unix_resolver_factory_create()); #endif grpc_register_tracer("channel", &grpc_trace_channel); grpc_register_tracer("surface", &grpc_surface_trace); diff --git a/test/core/end2end/tests/default_host.c b/test/core/end2end/tests/default_host.c index 97c19db331d..57f65b834b0 100644 --- a/test/core/end2end/tests/default_host.c +++ b/test/core/end2end/tests/default_host.c @@ -201,7 +201,7 @@ static void simple_request_body(grpc_end2end_test_fixture f) { GPR_ASSERT(status == GRPC_STATUS_UNIMPLEMENTED); GPR_ASSERT(0 == strcmp(details, "xyz")); GPR_ASSERT(0 == strcmp(call_details.method, "/foo")); - GPR_ASSERT(0 == strcmp(call_details.host, "localhost")); + GPR_ASSERT(0 == strncmp(call_details.host, "localhost", 9)); GPR_ASSERT(was_cancelled == 1); gpr_free(details); From 25bf313c5f1cf620879232d864c69b25c39e3f4c Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Mon, 24 Aug 2015 12:35:28 -0700 Subject: [PATCH 2/8] Fix for lame clients --- src/core/surface/channel.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/surface/channel.c b/src/core/surface/channel.c index 7378929ca4d..78eeed1f598 100644 --- a/src/core/surface/channel.c +++ b/src/core/surface/channel.c @@ -148,7 +148,8 @@ grpc_channel *grpc_channel_create_from_filters( } } - if (channel->is_client && channel->default_authority == NULL) { + if (channel->is_client && channel->default_authority == NULL && + target != NULL) { char *default_authority = grpc_get_default_authority(target); if (default_authority) { channel->default_authority = grpc_mdelem_from_strings( From dc31ef38d2530ed814d18150755710545293a8ed Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 07:11:35 -0700 Subject: [PATCH 3/8] Loosen test requirements to better fit spec --- test/cpp/end2end/generic_end2end_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cpp/end2end/generic_end2end_test.cc b/test/cpp/end2end/generic_end2end_test.cc index de7eab8dc29..809eef058cf 100644 --- a/test/cpp/end2end/generic_end2end_test.cc +++ b/test/cpp/end2end/generic_end2end_test.cc @@ -160,7 +160,7 @@ class GenericEnd2endTest : public ::testing::Test { srv_cq_.get(), tag(4)); verify_ok(srv_cq_.get(), 4, true); - EXPECT_EQ(server_host_, srv_ctx.host()); + EXPECT_EQ(server_host_, srv_ctx.host().substr(0, server_host_.length())); EXPECT_EQ(kMethodName, srv_ctx.method()); ByteBuffer recv_buffer; stream.Read(&recv_buffer, tag(5)); @@ -233,7 +233,7 @@ TEST_F(GenericEnd2endTest, SimpleBidiStreaming) { srv_cq_.get(), tag(2)); verify_ok(srv_cq_.get(), 2, true); - EXPECT_EQ(server_host_, srv_ctx.host()); + EXPECT_EQ(server_host_, srv_ctx.host().substr(0, server_host_.length())); EXPECT_EQ(kMethodName, srv_ctx.method()); std::unique_ptr send_buffer = From 775ec1decd597d9ecdf916e3d220b44283075ed9 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 11:03:53 -0700 Subject: [PATCH 4/8] Use SSL override as a default host name if none is specified --- include/grpc/grpc.h | 8 ++++++++ include/grpc/grpc_security.h | 9 --------- src/core/surface/channel.c | 19 +++++++++++++++++++ 3 files changed, 27 insertions(+), 9 deletions(-) diff --git a/include/grpc/grpc.h b/include/grpc/grpc.h index 101fc88d8f6..145052b6d3c 100644 --- 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 index 7f8f4d4a053..de565b2d2fc 100644 --- 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/surface/channel.c b/src/core/surface/channel.c index 78eeed1f598..586402e21c3 100644 --- a/src/core/surface/channel.c +++ b/src/core/surface/channel.c @@ -141,9 +141,28 @@ grpc_channel *grpc_channel_create_from_filters( gpr_log(GPR_ERROR, "%s: must be an string", GRPC_ARG_DEFAULT_AUTHORITY); } else { + if (channel->default_authority) { + /* setting this takes precedence over anything else */ + GRPC_MDELEM_UNREF(channel->default_authority); + } channel->default_authority = grpc_mdelem_from_strings( mdctx, ":authority", args->args[i].value.string); } + } else if (0 == + strcmp(args->args[i].key, GRPC_SSL_TARGET_NAME_OVERRIDE_ARG)) { + if (args->args[i].type != GRPC_ARG_STRING) { + gpr_log(GPR_ERROR, "%s: must be an string", + GRPC_SSL_TARGET_NAME_OVERRIDE_ARG); + } else { + if (channel->default_authority) { + /* other ways of setting this (notably ssl) take precedence */ + gpr_log(GPR_ERROR, "%s: default host already set some other way", + GRPC_ARG_DEFAULT_AUTHORITY); + } else { + channel->default_authority = grpc_mdelem_from_strings( + mdctx, ":authority", args->args[i].value.string); + } + } } } } From c9a3b0d55fc9bf29522699f4be8267b2eb2fdc24 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 11:20:02 -0700 Subject: [PATCH 5/8] Respond to review --- src/core/client_config/resolver_factory.c | 4 ++-- src/core/client_config/resolver_registry.c | 27 ++++++++++++---------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/src/core/client_config/resolver_factory.c b/src/core/client_config/resolver_factory.c index bf631cefd13..5b859a8d10b 100644 --- a/src/core/client_config/resolver_factory.c +++ b/src/core/client_config/resolver_factory.c @@ -45,12 +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) return NULL; + if (factory == NULL) return NULL; return factory->vtable->get_default_authority(factory, uri); } diff --git a/src/core/client_config/resolver_registry.c b/src/core/client_config/resolver_registry.c index 3aff4630fba..af263df26a9 100644 --- a/src/core/client_config/resolver_registry.c +++ b/src/core/client_config/resolver_registry.c @@ -92,20 +92,23 @@ static grpc_resolver_factory *resolve_factory(const char *target, *uri = grpc_uri_parse(target, 1); factory = lookup_factory(*uri); - if (factory == NULL && 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) { + 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)); - grpc_uri_destroy(grpc_uri_parse(tmp, 0)); - gpr_log(GPR_ERROR, "don't know how to resolve '%s' or '%s'", target, tmp); + 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(target, 0)); - gpr_log(GPR_ERROR, "don't know how to resolve '%s'", target); } return factory; } From c0136b9a270e406f6eee4962ce01722c7e72dc6f Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 11:21:43 -0700 Subject: [PATCH 6/8] Add comments --- src/core/client_config/resolver_factory.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/core/client_config/resolver_factory.h b/src/core/client_config/resolver_factory.h index 73a05642303..e243b23988a 100644 --- a/src/core/client_config/resolver_factory.h +++ b/src/core/client_config/resolver_factory.h @@ -51,12 +51,15 @@ 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; }; From f5cf6fa478f2dde957b052739a11d45dcdd0e111 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 13:31:00 -0700 Subject: [PATCH 7/8] Assert uri not null --- src/core/client_config/resolver_registry.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/core/client_config/resolver_registry.c b/src/core/client_config/resolver_registry.c index af263df26a9..886ca5d06d9 100644 --- a/src/core/client_config/resolver_registry.c +++ b/src/core/client_config/resolver_registry.c @@ -90,6 +90,7 @@ static grpc_resolver_factory *resolve_factory(const char *target, char *tmp; grpc_resolver_factory *factory = NULL; + GPR_ASSERT(uri); *uri = grpc_uri_parse(target, 1); factory = lookup_factory(*uri); if (factory == NULL) { From 25de92c997117da87cfde68f97400aa32d27d814 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 25 Aug 2015 13:48:49 -0700 Subject: [PATCH 8/8] Assert uri not null better --- src/core/client_config/resolver_registry.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/client_config/resolver_registry.c b/src/core/client_config/resolver_registry.c index 886ca5d06d9..37979b3b86c 100644 --- a/src/core/client_config/resolver_registry.c +++ b/src/core/client_config/resolver_registry.c @@ -90,7 +90,7 @@ static grpc_resolver_factory *resolve_factory(const char *target, char *tmp; grpc_resolver_factory *factory = NULL; - GPR_ASSERT(uri); + GPR_ASSERT(uri != NULL); *uri = grpc_uri_parse(target, 1); factory = lookup_factory(*uri); if (factory == NULL) {