Use URI query string instead of channel arg to indicate use of proxy.

8 files changed, 70 insertions, 54 deletions

diff --git a/src/core/ext/client_config/http_connect_handshaker.c b/src/core/ext/client_config/http_connect_handshaker.c
index f7fa931788..a5cb6d1047 100644
--- a/src/core/ext/client_config/http_connect_handshaker.c
+++ b/src/core/ext/client_config/http_connect_handshaker.c
@@ -46,7 +46,6 @@ typedef struct http_connect_handshaker {
   // Base class.  Must be first.
   grpc_handshaker base;
 
-  // These pointers are borrowed, we don't own them.
   char* proxy_server;
   char* server_name;
 
@@ -69,8 +68,14 @@ typedef struct http_connect_handshaker {
 static void on_write_done(grpc_exec_ctx* exec_ctx, void* arg,
                           grpc_error* error) {
   http_connect_handshaker* h = arg;
-  grpc_endpoint_read(exec_ctx, h->endpoint, &h->response_buffer,
-                     &h->response_read_closure);
+  if (error != GRPC_ERROR_NONE) {
+    // If the write failed, invoke the callback immediately with the error.
+    h->cb(exec_ctx, h->endpoint, h->args, h->user_data, GRPC_ERROR_REF(error));
+  } else {
+    // Otherwise, read the response.
+    grpc_endpoint_read(exec_ctx, h->endpoint, &h->response_buffer,
+                       &h->response_read_closure);
+  }
 }
 
 // Callback invoked for reading HTTP CONNECT response.
@@ -102,6 +107,14 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg,
                          &h->response_read_closure);
       return;
     }
+    // Make sure we got a 2xx response.
+    if (h->http_response.status < 200 || h->http_response.status >= 300) {
+      char* msg;
+      gpr_asprintf(&msg, "HTTP proxy returned response code %d",
+                   h->http_response.status);
+      error = GRPC_ERROR_CREATE(msg);
+      gpr_free(msg);
+    }
   }
  done:
   // Invoke handshake-done callback.
@@ -115,6 +128,8 @@ static void on_read_done(grpc_exec_ctx* exec_ctx, void* arg,
 static void http_connect_handshaker_destroy(grpc_exec_ctx* exec_ctx,
                                             grpc_handshaker* handshaker) {
   http_connect_handshaker* h = (http_connect_handshaker*)handshaker;
+  gpr_free(h->proxy_server);
+  gpr_free(h->server_name);
   gpr_slice_buffer_destroy(&h->request_buffer);
   gpr_slice_buffer_destroy(&h->response_buffer);
   grpc_http_parser_destroy(&h->http_parser);
@@ -145,6 +160,8 @@ static void http_connect_handshaker_do_handshake(
   grpc_http_parser_init(&h->http_parser, GRPC_HTTP_RESPONSE,
                         &h->http_response);
   // Send HTTP CONNECT request.
+  gpr_log(GPR_INFO, "Connecting to server %s via HTTP proxy %s",
+          h->server_name, h->proxy_server);
   grpc_httpcli_request request;
   memset(&request, 0, sizeof(request));
   request.host = gpr_strdup(h->proxy_server);
@@ -161,27 +178,15 @@ static const struct grpc_handshaker_vtable http_connect_handshaker_vtable = {
     http_connect_handshaker_destroy, http_connect_handshaker_shutdown,
     http_connect_handshaker_do_handshake};
 
-char* grpc_get_http_connect_proxy_server_from_args(grpc_channel_args* args) {
-  for (size_t i = 0; i < args->num_args; ++i) {
-    if (strcmp(args->args[i].key, GRPC_ARG_HTTP_CONNECT_PROXY_SERVER) == 0) {
-      if (args->args[i].type != GRPC_ARG_STRING) {
-        gpr_log(GPR_ERROR, "%s: must be a string",
-                GRPC_ARG_HTTP_CONNECT_PROXY_SERVER);
-        break;
-      }
-      return args->args[i].value.string;
-    }
-  }
-  return NULL;
-}
-
-grpc_handshaker* grpc_http_connect_handshaker_create(char* proxy_server,
-                                                     char* server_name) {
+grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
+                                                     const char* server_name) {
+  GPR_ASSERT(proxy_server != NULL);
+  GPR_ASSERT(server_name != NULL);
   http_connect_handshaker* handshaker =
       gpr_malloc(sizeof(http_connect_handshaker));
   memset(handshaker, 0, sizeof(*handshaker));
   grpc_handshaker_init(&http_connect_handshaker_vtable, &handshaker->base);
-  handshaker->proxy_server = proxy_server;
-  handshaker->server_name = server_name;
+  handshaker->proxy_server = gpr_strdup(proxy_server);
+  handshaker->server_name = gpr_strdup(server_name);
   return (grpc_handshaker*)handshaker;
 }
diff --git a/src/core/ext/client_config/http_connect_handshaker.h b/src/core/ext/client_config/http_connect_handshaker.h
index feb2ec13f0..146ef9369a 100644
--- a/src/core/ext/client_config/http_connect_handshaker.h
+++ b/src/core/ext/client_config/http_connect_handshaker.h
@@ -36,13 +36,8 @@
 
 #include "src/core/lib/channel/handshaker.h"
 
-/// Caller does NOT take ownership of returned string.
-char* grpc_get_http_connect_proxy_server_from_args(grpc_channel_args *args);
-
-/// Borrows references to \a proxy_server or \a server_name.
-/// The caller must ensure that they remain alive until handshaking is
-/// complete.
-grpc_handshaker* grpc_http_connect_handshaker_create(char* proxy_server,
-                                                     char* server_name);
+/// Does NOT take ownership of \a proxy_server or \a server_name.
+grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
+                                                     const char* server_name);
 
 #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_HTTP_CONNECT_HANDSHAKER_H */
diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c
index e7a4abd568..13f08e9fe6 100644
--- a/src/core/ext/client_config/resolver_registry.c
+++ b/src/core/ext/client_config/resolver_registry.c
@@ -129,7 +129,8 @@ static grpc_resolver_factory *resolve_factory(const char *target,
 }
 
 grpc_resolver *grpc_resolver_create(
-    const char *target, grpc_client_channel_factory *client_channel_factory) {
+    const char *target, grpc_client_channel_factory *client_channel_factory,
+    char **http_proxy) {
   grpc_uri *uri = NULL;
   grpc_resolver_factory *factory = resolve_factory(target, &uri);
   grpc_resolver *resolver;
@@ -138,6 +139,9 @@ grpc_resolver *grpc_resolver_create(
   args.uri = uri;
   args.client_channel_factory = client_channel_factory;
   resolver = grpc_resolver_factory_create_resolver(factory, &args);
+  const char *proxy = grpc_uri_get_query_arg(uri, "http_proxy");
+  if (proxy != NULL)
+    *http_proxy = gpr_strdup(proxy);
   grpc_uri_destroy(uri);
   return resolver;
 }
diff --git a/src/core/ext/client_config/resolver_registry.h b/src/core/ext/client_config/resolver_registry.h
index 5ef1383cd3..28843001ea 100644
--- a/src/core/ext/client_config/resolver_registry.h
+++ b/src/core/ext/client_config/resolver_registry.h
@@ -54,9 +54,12 @@ void grpc_register_resolver_type(grpc_resolver_factory *factory);
     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. */
+    If a resolver factory was not found, return NULL.
+    If \a target specifies an http_proxy as a query arg, sets \a http_proxy
+    to the value (which the caller takes ownership of). */
 grpc_resolver *grpc_resolver_create(
-    const char *target, grpc_client_channel_factory *client_channel_factory);
+    const char *target, grpc_client_channel_factory *client_channel_factory,
+    char **http_proxy);
 
 /** Find a resolver factory given a name and return an (owned-by-the-caller)
  *  reference to it */
diff --git a/src/core/ext/client_config/uri_parser.c b/src/core/ext/client_config/uri_parser.c
index 3ca1a58e69..bc80432336 100644
--- a/src/core/ext/client_config/uri_parser.c
+++ b/src/core/ext/client_config/uri_parser.c
@@ -118,8 +118,8 @@ static int parse_fragment_or_query(const char *uri_text, size_t *i) {
     const size_t advance = parse_pchar(uri_text, *i); /* pchar */
     switch (advance) {
       case 0: /* uri_text[i] isn't in pchar */
-        /* maybe it's ? or / */
-        if (uri_text[*i] == '?' || uri_text[*i] == '/') {
+        /* maybe it's ? or / or : */
+        if (uri_text[*i] == '?' || uri_text[*i] == '/' || uri_text[*i] == ':') {
           (*i)++;
           break;
         } else {
diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c
index 255448f427..c8402da0fd 100644
--- a/src/core/ext/resolver/dns/native/dns_resolver.c
+++ b/src/core/ext/resolver/dns/native/dns_resolver.c
@@ -54,8 +54,10 @@ typedef struct {
   grpc_resolver base;
   /** refcount */
   gpr_refcount refs;
-  /** name to resolve */
-  char *name;
+  /** target name */
+  char *target_name;
+  /** name to resolve (usually the same as target_name) */
+  char *name_to_resolve;
   /** default port to use */
   char *default_port;
   /** subchannel factory */
@@ -174,7 +176,7 @@ static void dns_on_resolved(grpc_exec_ctx *exec_ctx, void *arg,
   if (addresses != NULL) {
     grpc_lb_policy_args lb_policy_args;
     memset(&lb_policy_args, 0, sizeof(lb_policy_args));
-    lb_policy_args.server_name = r->name;
+    lb_policy_args.server_name = r->target_name;
     lb_policy_args.addresses = addresses;
     lb_policy_args.client_channel_factory = r->client_channel_factory;
     lb_policy =
@@ -221,7 +223,7 @@ static void dns_start_resolving_locked(grpc_exec_ctx *exec_ctx,
   GPR_ASSERT(!r->resolving);
   r->resolving = true;
   r->addresses = NULL;
-  grpc_resolve_address(exec_ctx, r->name, r->default_port,
+  grpc_resolve_address(exec_ctx, r->name_to_resolve, r->default_port,
                        grpc_closure_create(dns_on_resolved, r), &r->addresses);
 }
 
@@ -246,7 +248,8 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
     grpc_client_config_unref(exec_ctx, r->resolved_config);
   }
   grpc_client_channel_factory_unref(exec_ctx, r->client_channel_factory);
-  gpr_free(r->name);
+  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);
@@ -255,22 +258,22 @@ static void dns_destroy(grpc_exec_ctx *exec_ctx, grpc_resolver *gr) {
 static grpc_resolver *dns_create(grpc_resolver_args *args,
                                  const char *default_port,
                                  const char *lb_policy_name) {
-  dns_resolver *r;
-  const char *path = args->uri->path;
-
   if (0 != strcmp(args->uri->authority, "")) {
     gpr_log(GPR_ERROR, "authority based dns uri's not supported");
     return NULL;
   }
-
+  // Get name and (optionally) proxy address from args.
+  const char *path = args->uri->path;
   if (path[0] == '/') ++path;
-
-  r = gpr_malloc(sizeof(dns_resolver));
+  const char *proxy_name = grpc_uri_get_query_arg(args->uri, "http_proxy");
+  // Create resolver.
+  dns_resolver *r = gpr_malloc(sizeof(dns_resolver));
   memset(r, 0, sizeof(*r));
   gpr_ref_init(&r->refs, 1);
   gpr_mu_init(&r->mu);
   grpc_resolver_init(&r->base, &dns_resolver_vtable);
-  r->name = gpr_strdup(path);
+  r->target_name = gpr_strdup(path);
+  r->name_to_resolve = gpr_strdup(proxy_name == NULL ? path : proxy_name);
   r->default_port = gpr_strdup(default_port);
   r->client_channel_factory = args->client_channel_factory;
   gpr_backoff_init(&r->backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER,
diff --git a/src/core/ext/transport/chttp2/client/insecure/channel_create.c b/src/core/ext/transport/chttp2/client/insecure/channel_create.c
index 2df4ce1dda..8f8da7f5ea 100644
--- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c
+++ b/src/core/ext/transport/chttp2/client/insecure/channel_create.c
@@ -155,6 +155,7 @@ typedef struct {
   gpr_refcount refs;
   grpc_channel_args *merge_args;
   grpc_channel *master;
+  char *http_proxy;
 } client_channel_factory;
 
 static void client_channel_factory_ref(
@@ -172,6 +173,8 @@ static void client_channel_factory_unref(
                                   "client_channel_factory");
     }
     grpc_channel_args_destroy(f->merge_args);
+    if (f->http_proxy != NULL)
+      gpr_free(f->http_proxy);
     gpr_free(f);
   }
 }
@@ -188,10 +191,9 @@ static grpc_subchannel *client_channel_factory_create_subchannel(
   c->base.vtable = &connector_vtable;
   gpr_ref_init(&c->refs, 1);
   c->handshake_mgr = grpc_handshake_manager_create();
-  char *proxy_server = grpc_get_http_connect_proxy_server_from_args(final_args);
-  if (proxy_server != NULL) {
+  if (f->http_proxy != NULL) {
     grpc_handshake_manager_add(
-        grpc_http_connect_handshaker_create(proxy_server, args->server_name),
+        grpc_http_connect_handshaker_create(f->http_proxy, args->server_name),
         c->handshake_mgr);
   }
   args->args = final_args;
@@ -210,7 +212,8 @@ static grpc_channel *client_channel_factory_create_channel(
   grpc_channel *channel = grpc_channel_create(exec_ctx, target, final_args,
                                               GRPC_CLIENT_CHANNEL, NULL);
   grpc_channel_args_destroy(final_args);
-  grpc_resolver *resolver = grpc_resolver_create(target, &f->base);
+  grpc_resolver *resolver = grpc_resolver_create(target, &f->base,
+                                                 &f->http_proxy);
   if (!resolver) {
     GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel,
                                 "client_channel_factory_create_channel");
diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c
index ee8f39434b..863051fea4 100644
--- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c
+++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c
@@ -222,6 +222,7 @@ typedef struct {
   grpc_channel_args *merge_args;
   grpc_channel_security_connector *security_connector;
   grpc_channel *master;
+  char *http_proxy;
 } client_channel_factory;
 
 static void client_channel_factory_ref(
@@ -241,6 +242,8 @@ static void client_channel_factory_unref(
                                   "client_channel_factory");
     }
     grpc_channel_args_destroy(f->merge_args);
+    if (f->http_proxy != NULL)
+      gpr_free(f->http_proxy);
     gpr_free(f);
   }
 }
@@ -257,10 +260,9 @@ static grpc_subchannel *client_channel_factory_create_subchannel(
   c->base.vtable = &connector_vtable;
   c->security_connector = f->security_connector;
   c->handshake_mgr = grpc_handshake_manager_create();
-  char *proxy_server = grpc_get_http_connect_proxy_server_from_args(final_args);
-  if (proxy_server != NULL) {
+  if (f->http_proxy != NULL) {
     grpc_handshake_manager_add(
-        grpc_http_connect_handshaker_create(proxy_server, args->server_name),
+        grpc_http_connect_handshaker_create(f->http_proxy, args->server_name),
         c->handshake_mgr);
   }
   gpr_mu_init(&c->mu);
@@ -283,7 +285,8 @@ static grpc_channel *client_channel_factory_create_channel(
                                               GRPC_CLIENT_CHANNEL, NULL);
   grpc_channel_args_destroy(final_args);
 
-  grpc_resolver *resolver = grpc_resolver_create(target, &f->base);
+  grpc_resolver *resolver = grpc_resolver_create(target, &f->base,
+                                                 &f->http_proxy);
   if (resolver != NULL) {
     grpc_client_channel_set_resolver(
         exec_ctx, grpc_channel_get_channel_stack(channel), resolver);