From 39b5871d7b1da79945c87f98acc4cbbd499ecfba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 6 Sep 2016 12:50:42 -0700 Subject: [PATCH] Use http_proxy environment variable instead of URI query param. --- .../client_config/http_connect_handshaker.c | 25 +++++++++++++++++++ .../client_config/http_connect_handshaker.h | 4 +++ .../ext/client_config/lb_policy_factory.h | 2 +- .../ext/client_config/resolver_registry.c | 5 +--- .../ext/client_config/resolver_registry.h | 7 ++---- src/core/ext/client_config/uri_parser.c | 5 ++-- .../ext/resolver/dns/native/dns_resolver.c | 8 +++--- .../chttp2/client/insecure/channel_create.c | 11 ++++---- .../client/secure/secure_channel_create.c | 11 ++++---- test/core/client_config/uri_parser_test.c | 2 -- test/core/end2end/fixtures/h2_http_proxy.c | 11 ++++---- test/core/end2end/fixtures/http_proxy.c | 2 +- 12 files changed, 57 insertions(+), 36 deletions(-) diff --git a/src/core/ext/client_config/http_connect_handshaker.c b/src/core/ext/client_config/http_connect_handshaker.c index 55b01bd46e3..097465469e6 100644 --- a/src/core/ext/client_config/http_connect_handshaker.c +++ b/src/core/ext/client_config/http_connect_handshaker.c @@ -40,9 +40,11 @@ #include #include +#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; +} diff --git a/src/core/ext/client_config/http_connect_handshaker.h b/src/core/ext/client_config/http_connect_handshaker.h index 146ef9369a9..1fc39482678 100644 --- a/src/core/ext/client_config/http_connect_handshaker.h +++ b/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 */ diff --git a/src/core/ext/client_config/lb_policy_factory.h b/src/core/ext/client_config/lb_policy_factory.h index 5806deef9b3..a9d35887679 100644 --- a/src/core/ext/client_config/lb_policy_factory.h +++ b/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; diff --git a/src/core/ext/client_config/resolver_registry.c b/src/core/ext/client_config/resolver_registry.c index 5a8f1371035..e7a4abd568c 100644 --- a/src/core/ext/client_config/resolver_registry.c +++ b/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, - char **http_proxy) { + const char *target, grpc_client_channel_factory *client_channel_factory) { 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; } diff --git a/src/core/ext/client_config/resolver_registry.h b/src/core/ext/client_config/resolver_registry.h index 28843001ead..5ef1383cd35 100644 --- a/src/core/ext/client_config/resolver_registry.h +++ b/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 target specifies an http_proxy as a query arg, sets \a http_proxy - to the value (which the caller takes ownership of). */ + If a resolver factory was not found, return NULL. */ grpc_resolver *grpc_resolver_create( - const char *target, grpc_client_channel_factory *client_channel_factory, - char **http_proxy); + const char *target, grpc_client_channel_factory *client_channel_factory); /** 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 5e8432c6c8d..3ca1a58e692 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 / or : */ - if (uri_text[*i] == '?' || uri_text[*i] == '/' || uri_text[*i] == ':') { + /* maybe it's ? or / */ + 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; diff --git a/src/core/ext/resolver/dns/native/dns_resolver.c b/src/core/ext/resolver/dns/native/dns_resolver.c index 5f41fdcc2f0..5886f6dcbf5 100644 --- a/src/core/ext/resolver/dns/native/dns_resolver.c +++ b/src/core/ext/resolver/dns/native/dns_resolver.c @@ -37,6 +37,7 @@ #include #include +#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, 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 475224effd0..14dc7f142f6 100644 --- a/src/core/ext/transport/chttp2/client/insecure/channel_create.c +++ b/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_create(target, &f->base, &f->http_proxy); + grpc_resolver *resolver = grpc_resolver_create(target, &f->base); 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 e06ae9e02ca..a9616e92d02 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,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_create(target, &f->base, &f->http_proxy); + grpc_resolver *resolver = grpc_resolver_create(target, &f->base); if (resolver != NULL) { grpc_client_channel_set_resolver( exec_ctx, grpc_channel_get_channel_stack(channel), resolver); diff --git a/test/core/client_config/uri_parser_test.c b/test/core/client_config/uri_parser_test.c index 4bc3d1e39f7..323e8b6f70b 100644 --- a/test/core/client_config/uri_parser_test.c +++ b/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"); diff --git a/test/core/end2end/fixtures/h2_http_proxy.c b/test/core/end2end/fixtures/h2_http_proxy.c index 612a3dbb830..a675a11f665 100644 --- a/test/core/end2end/fixtures/h2_http_proxy.c +++ b/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; - gpr_asprintf(&target_uri, "%s?http_proxy=%s", ffd->server_addr, + char *proxy_uri; + gpr_asprintf(&proxy_uri, "http://%s", grpc_end2end_http_proxy_get_proxy_name(ffd->proxy)); - gpr_log(GPR_INFO, "target_uri: %s", target_uri); - f->client = grpc_insecure_channel_create(target_uri, client_args, NULL); - gpr_free(target_uri); + gpr_setenv("http_proxy", proxy_uri); + gpr_free(proxy_uri); + f->client = grpc_insecure_channel_create(ffd->server_addr, client_args, NULL); GPR_ASSERT(f->client); } diff --git a/test/core/end2end/fixtures/http_proxy.c b/test/core/end2end/fixtures/http_proxy.c index b4c0dfba614..c92f869be16 100644 --- a/test/core/end2end/fixtures/http_proxy.c +++ b/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)