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

5 files changed, 41 insertions, 34 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 {