Use http_proxy environment variable instead of URI query param.

src/core/ext/client_config/http_connect_handshaker.c

@@ -40,9 +40,11 @@
 #include <grpc/impl/codegen/slice_buffer.h>
 #include <grpc/support/string_util.h>
 
+#include "src/core/ext/client_config/uri_parser.h"
 #include "src/core/lib/http/format_request.h"
 #include "src/core/lib/http/parser.h"
 #include "src/core/lib/iomgr/timer.h"
+#include "src/core/lib/support/env.h"
 
 typedef struct http_connect_handshaker {
   // Base class.  Must be first.
@@ -247,3 +249,26 @@ grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
   gpr_ref_init(&handshaker->refcount, 1);
   return &handshaker->base;
 }
+
+char* grpc_get_http_proxy_server() {
+  char* uri_str = gpr_getenv("http_proxy");
+  if (uri_str == NULL) return NULL;
+  grpc_uri* uri = grpc_uri_parse(uri_str, false /* suppress_errors */);
+  char* proxy_name = NULL;
+  if (uri == NULL || uri->authority == NULL) {
+    gpr_log(GPR_ERROR, "cannot parse value of 'http_proxy' env var");
+    goto done;
+  }
+  if (strcmp(uri->scheme, "http") != 0) {
+    gpr_log(GPR_ERROR, "'%s' scheme not supported in proxy URI", uri->scheme);
+    goto done;
+  }
+  if (strchr(uri->authority, '@') != NULL) {
+    gpr_log(GPR_ERROR, "userinfo not supported in proxy URI");
+    goto done;
+  }
+  proxy_name = gpr_strdup(uri->authority);
+done:
+  grpc_uri_destroy(uri);
+  return proxy_name;
+}

src/core/ext/client_config/http_connect_handshaker.h

@@ -40,4 +40,8 @@
 grpc_handshaker* grpc_http_connect_handshaker_create(const char* proxy_server,
                                                      const char* server_name);
 
+/// Returns the name of the proxy to use, or NULL if no proxy is configured.
+/// Caller takes ownership of result.
+char* grpc_get_http_proxy_server();
+
 #endif /* GRPC_CORE_EXT_CLIENT_CONFIG_HTTP_CONNECT_HANDSHAKER_H */

src/core/ext/client_config/lb_policy_factory.h

@@ -48,7 +48,7 @@ struct grpc_lb_policy_factory {
 };
 
 typedef struct grpc_lb_policy_args {
-  char *server_name;  // Does not own.
+  char *server_name;
   grpc_resolved_addresses *addresses;
   grpc_client_channel_factory *client_channel_factory;
 } grpc_lb_policy_args;

src/core/ext/client_config/resolver_registry.c

@@ -129,8 +129,7 @@ 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;
@@ -139,8 +138,6 @@ 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;
 }

src/core/ext/client_config/resolver_registry.h

@@ -54,12 +54,9 @@ 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 */

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 / or : */
+        /* maybe it's ? or / */
-        if (uri_text[*i] == '?' || uri_text[*i] == '/' || uri_text[*i] == ':') {
+        if (uri_text[*i] == '?' || uri_text[*i] == '/') {
           (*i)++;
           break;
         } else {
@@ -282,7 +282,6 @@ grpc_uri *grpc_uri_parse(const char *uri_text, int suppress_errors) {
 }
 
 const char *grpc_uri_get_query_arg(const grpc_uri *uri, const char *key) {
-  if (uri == NULL) return NULL;
   GPR_ASSERT(key != NULL);
   if (key[0] == '\0') return NULL;
 

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

@@ -37,6 +37,7 @@
 #include <grpc/support/host_port.h>
 #include <grpc/support/string_util.h>
 
+#include "src/core/ext/client_config/http_connect_handshaker.h"
 #include "src/core/ext/client_config/lb_policy_registry.h"
 #include "src/core/ext/client_config/resolver_registry.h"
 #include "src/core/lib/iomgr/resolve_address.h"
@@ -262,10 +263,11 @@ static grpc_resolver *dns_create(grpc_resolver_args *args,
     gpr_log(GPR_ERROR, "authority based dns uri's not supported");
     return NULL;
   }
-  // Get name and (optionally) proxy address from args.
+  // Get name from args.
   const char *path = args->uri->path;
   if (path[0] == '/') ++path;
-  const char *proxy_name = grpc_uri_get_query_arg(args->uri, "http_proxy");
+  // Get proxy name, if any.
+  char *proxy_name = grpc_get_http_proxy_server();
   // Create resolver.
   dns_resolver *r = gpr_malloc(sizeof(dns_resolver));
   memset(r, 0, sizeof(*r));
@@ -273,7 +275,7 @@ static grpc_resolver *dns_create(grpc_resolver_args *args,
   gpr_mu_init(&r->mu);
   grpc_resolver_init(&r->base, &dns_resolver_vtable);
   r->target_name = gpr_strdup(path);
-  r->name_to_resolve = gpr_strdup(proxy_name == NULL ? path : proxy_name);
+  r->name_to_resolve = proxy_name == NULL ? gpr_strdup(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,

src/core/ext/transport/chttp2/client/insecure/channel_create.c

@@ -162,7 +162,6 @@ 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(
@@ -180,7 +179,6 @@ 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);
   }
 }
@@ -197,10 +195,12 @@ 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();
-  if (f->http_proxy != NULL) {
+  char *proxy_name = grpc_get_http_proxy_server();
+  if (proxy_name != NULL) {
     grpc_handshake_manager_add(
         c->handshake_mgr,
-        grpc_http_connect_handshaker_create(f->http_proxy, args->server_name));
+        grpc_http_connect_handshaker_create(proxy_name, args->server_name));
+    gpr_free(proxy_name);
   }
   args->args = final_args;
   s = grpc_subchannel_create(exec_ctx, &c->base, args);
@@ -218,8 +218,7 @@ 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 *resolver = grpc_resolver_create(target, &f->base);
-      grpc_resolver_create(target, &f->base, &f->http_proxy);
   if (!resolver) {
     GRPC_CHANNEL_INTERNAL_UNREF(exec_ctx, channel,
                                 "client_channel_factory_create_channel");

src/core/ext/transport/chttp2/client/secure/secure_channel_create.c

@@ -222,7 +222,6 @@ 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(
@@ -242,7 +241,6 @@ 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);
   }
 }
@@ -259,10 +257,12 @@ 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();
-  if (f->http_proxy != NULL) {
+  char *proxy_name = grpc_get_http_proxy_server();
+  if (proxy_name != NULL) {
     grpc_handshake_manager_add(
         c->handshake_mgr,
-        grpc_http_connect_handshaker_create(f->http_proxy, args->server_name));
+        grpc_http_connect_handshaker_create(proxy_name, args->server_name));
+    gpr_free(proxy_name);
   }
   gpr_mu_init(&c->mu);
   gpr_ref_init(&c->refs, 1);
@@ -284,8 +284,7 @@ static grpc_channel *client_channel_factory_create_channel(
                                               GRPC_CLIENT_CHANNEL, NULL);
   grpc_channel_args_destroy(final_args);
 
-  grpc_resolver *resolver =
+  grpc_resolver *resolver = grpc_resolver_create(target, &f->base);
-      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);

test/core/client_config/uri_parser_test.c

@@ -142,8 +142,6 @@ int main(int argc, char **argv) {
   test_succeeds("http:?legit#twice", "http", "", "", "legit", "twice");
   test_succeeds("http://foo?bar#lol?", "http", "foo", "", "bar", "lol?");
   test_succeeds("http://foo?bar#lol?/", "http", "foo", "", "bar", "lol?/");
-  test_succeeds("dns:///server:123?http_proxy=proxy:456", "dns", "",
-                "/server:123", "http_proxy=proxy:456", "");
 
   test_fails("xyz");
   test_fails("http:?dangling-pct-%0");

test/core/end2end/fixtures/h2_http_proxy.c

@@ -46,6 +46,7 @@
 #include "src/core/ext/transport/chttp2/transport/chttp2_transport.h"
 #include "src/core/lib/channel/connected_channel.h"
 #include "src/core/lib/channel/http_server_filter.h"
+#include "src/core/lib/support/env.h"
 #include "src/core/lib/surface/channel.h"
 #include "src/core/lib/surface/server.h"
 #include "test/core/end2end/fixtures/http_proxy.h"
@@ -76,12 +77,12 @@ static grpc_end2end_test_fixture chttp2_create_fixture_fullstack(
 void chttp2_init_client_fullstack(grpc_end2end_test_fixture *f,
                                   grpc_channel_args *client_args) {
   fullstack_fixture_data *ffd = f->fixture_data;
-  char *target_uri;
+  char *proxy_uri;
-  gpr_asprintf(&target_uri, "%s?http_proxy=%s", ffd->server_addr,
+  gpr_asprintf(&proxy_uri, "http://%s",
                grpc_end2end_http_proxy_get_proxy_name(ffd->proxy));
-  gpr_log(GPR_INFO, "target_uri: %s", target_uri);
+  gpr_setenv("http_proxy", proxy_uri);
-  f->client = grpc_insecure_channel_create(target_uri, client_args, NULL);
+  gpr_free(proxy_uri);
-  gpr_free(target_uri);
+  f->client = grpc_insecure_channel_create(ffd->server_addr, client_args, NULL);
   GPR_ASSERT(f->client);
 }
 

test/core/end2end/fixtures/http_proxy.c

@@ -122,7 +122,7 @@ static void proxy_connection_failed(grpc_exec_ctx* exec_ctx,
                                     proxy_connection* conn, bool is_client,
                                     const char* prefix, grpc_error* error) {
   const char* msg = grpc_error_string(error);
-  gpr_log(GPR_ERROR, "%s: %s", prefix, msg);
+  gpr_log(GPR_INFO, "%s: %s", prefix, msg);
   grpc_error_free_string(msg);
   grpc_endpoint_shutdown(exec_ctx, conn->client_endpoint);
   if (conn->server_endpoint != NULL)