From ee612de152e1f5aab8414a5bd0fef51ff056eaab Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 1 Apr 2020 00:41:36 +0000 Subject: [PATCH 01/20] http, utility: Use GURL Use GURL as HTTP URL parser utility. Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 17 ++++ bazel/external/icuuc.BUILD | 58 ++++++++++++++ bazel/external/icuuc.patch | 63 +++++++++++++++ bazel/repositories.bzl | 15 ++++ bazel/repository_locations.bzl | 5 ++ source/common/http/BUILD | 2 +- source/common/http/http1/codec_impl.cc | 4 +- source/common/http/utility.cc | 52 +++++------- source/common/http/utility.h | 15 ++-- source/common/router/router.cc | 4 +- .../filters/http/csrf/csrf_filter.cc | 2 +- test/common/http/http1/codec_impl_test.cc | 9 +++ test/common/http/utility_test.cc | 79 ++++++++++--------- 13 files changed, 245 insertions(+), 80 deletions(-) create mode 100644 bazel/external/googleurl.patch create mode 100644 bazel/external/icuuc.BUILD create mode 100644 bazel/external/icuuc.patch diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch new file mode 100644 index 0000000000000..681569e016bac --- /dev/null +++ b/bazel/external/googleurl.patch @@ -0,0 +1,17 @@ +diff --git a/url/BUILD b/url/BUILD +index 0126bdc..a11e973 100644 +--- a/url/BUILD ++++ b/url/BUILD +@@ -43,11 +43,11 @@ cc_library( + "url_util.h", + ], + copts = build_config.default_copts, +- linkopts = ["-licuuc"], + visibility = ["//visibility:public"], + deps = [ + "//base", + "//base/strings", + "//polyfills", ++ "@org_unicode_icuuc//:common" + ], + ) diff --git a/bazel/external/icuuc.BUILD b/bazel/external/icuuc.BUILD new file mode 100644 index 0000000000000..f6f2dd1ce489f --- /dev/null +++ b/bazel/external/icuuc.BUILD @@ -0,0 +1,58 @@ +# https://github.com/tensorflow/tensorflow/commit/1154432f88387c81be4ac2e3cb249b787ffafe21 +# https://github.com/tensorflow/tensorflow/blob/068641e7f858d92407343b2c3994d3bee3822093/third_party/icu/BUILD.bazel +licenses(["notice"]) # Apache 2 + +exports_files([ + "icu4c/LICENSE", + "icu4j/main/shared/licenses/LICENSE", +]) + +icuuc_copts = [ + "-DU_COMMON_IMPLEMENTATION", + "-DU_HAVE_STD_ATOMICS", # TODO(gunan): Remove when TF is on ICU 64+. +] + select({ + "@envoy//bazel:apple": [ + "-Wno-shorten-64-to-32", + "-Wno-unused-variable", + ], + "@envoy//bazel:windows_x86_64": [ + "/utf-8", + "/DLOCALE_ALLOW_NEUTRAL_NAMES=0", + ], + # TODO(dio): Add "@envoy//bazel:android" when we have it. + # "@envoy//bazel:android": [ + # "-fdata-sections", + # "-DU_HAVE_NL_LANGINFO_CODESET=0", + # "-Wno-deprecated-declarations", + # ], + "//conditions:default": [], +}) + +cc_library( + name = "headers", + hdrs = glob(["icu4c/source/common/unicode/*.h"]), + includes = ["icu4c/source/common"], + visibility = ["//visibility:public"], +) + +cc_library( + name = "common", + hdrs = glob(["icu4c/source/common/unicode/*.h"]), + includes = ["icu4c/source/common"], + visibility = ["//visibility:public"], + deps = [":icuuc"], +) + +cc_library( + name = "icuuc", + srcs = glob([ + "icu4c/source/common/*.c", + "icu4c/source/common/*.cpp", + "icu4c/source/stubdata/*.cpp", + ]), + hdrs = glob(["icu4c/source/common/*.h"]), + copts = icuuc_copts, + tags = ["requires-rtti"], + visibility = ["//visibility:private"], + deps = [":headers"], +) diff --git a/bazel/external/icuuc.patch b/bazel/external/icuuc.patch new file mode 100644 index 0000000000000..69e00c8fbe155 --- /dev/null +++ b/bazel/external/icuuc.patch @@ -0,0 +1,63 @@ +# Adapted from TensorFlow's third_party/icu/udata.patch. +# https://github.com/tensorflow/tensorflow/blob/bec9e52822c330ae926aa029cd26e924b42166a9/third_party/icu/udata.patch +diff --git a/icu4c/source/common/udata.cpp b/icu4c/source/common/udata.cpp +index 3cb8863f6c..86fc7999a0 100644 +--- a/icu4c/source/common/udata.cpp ++++ b/icu4c/source/common/udata.cpp +@@ -18,11 +18,10 @@ + + #include "unicode/utypes.h" /* U_PLATFORM etc. */ + +-#ifdef __GNUC__ +-/* if gcc +-#define ATTRIBUTE_WEAK __attribute__ ((weak)) +-might have to #include some other header +-*/ ++#if defined(__GNUC__) || defined(__SUNPRO_CC) ++# define ATTRIBUTE_WEAK __attribute__ ((weak)) ++#else ++# define ATTRIBUTE_WEAK + #endif + + #include "unicode/putil.h" +@@ -641,10 +640,9 @@ extern "C" const DataHeader U_DATA_API U_ICUDATA_ENTRY_POINT; + * partial-data-library access functions where each returns a pointer + * to its data package, if it is linked in. + */ +-/* +-extern const void *uprv_getICUData_collation(void) ATTRIBUTE_WEAK; +-extern const void *uprv_getICUData_conversion(void) ATTRIBUTE_WEAK; +-*/ ++U_CDECL_BEGIN ++const void *uprv_getICUData_conversion(void) ATTRIBUTE_WEAK; ++U_CDECL_END + + /*----------------------------------------------------------------------* + * * +@@ -702,10 +700,10 @@ openCommonData(const char *path, /* Path from OpenChoice? */ + if (uprv_getICUData_collation) { + setCommonICUDataPointer(uprv_getICUData_collation(), FALSE, pErrorCode); + } ++ */ + if (uprv_getICUData_conversion) { + setCommonICUDataPointer(uprv_getICUData_conversion(), FALSE, pErrorCode); + } +- */ + #if U_PLATFORM_HAS_WINUWP_API == 0 // Windows UWP Platform does not support dll icu data at this time + setCommonICUDataPointer(&U_ICUDATA_ENTRY_POINT, FALSE, pErrorCode); + { +diff --git a/icu4c/source/common/unicode/uconfig.h b/icu4c/source/common/unicode/uconfig.h +index 7ddf4e6adf..f95d7a2932 100644 +--- a/icu4c/source/common/unicode/uconfig.h ++++ b/icu4c/source/common/unicode/uconfig.h +@@ -55,6 +55,10 @@ + #include "uconfig_local.h" + #endif + ++#ifndef U_STATIC_IMPLEMENTATION ++#define U_STATIC_IMPLEMENTATION ++#endif ++ + /** + * \def U_DEBUG + * Determines whether to include debugging code. diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 415455e5846db..91e2c3113fabe 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -145,6 +145,7 @@ def envoy_dependencies(skip_targets = []): _com_googlesource_quiche() _com_googlesource_googleurl() _com_lightstep_tracer_cpp() + _org_unicode_icuuc() _io_opentracing_cpp() _net_zlib() _upb() @@ -227,6 +228,18 @@ def _com_github_cyan4973_xxhash(): actual = "@com_github_cyan4973_xxhash//:xxhash", ) +def _org_unicode_icuuc(): + _repository_impl( + name = "org_unicode_icuuc", + build_file = "@envoy//bazel/external:icuuc.BUILD", + patches = ["@envoy//bazel/external:icuuc.patch"], + patch_args = ["-p1"], + ) + native.bind( + name = "icuuc", + actual = "@org_unicode_icuuc//:common", + ) + def _com_github_envoyproxy_sqlparser(): _repository_impl( name = "com_github_envoyproxy_sqlparser", @@ -652,6 +665,8 @@ def _com_googlesource_quiche(): def _com_googlesource_googleurl(): _repository_impl( name = "com_googlesource_googleurl", + patches = ["@envoy//bazel/external:googleurl.patch"], + patch_args = ["-p1"], ) native.bind( name = "googleurl", diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index cebb98e124e33..e0f40065c88e1 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -330,4 +330,9 @@ REPOSITORY_LOCATIONS = dict( strip_prefix = "kafka-python-2.0.0", urls = ["https://github.com/dpkp/kafka-python/archive/2.0.0.tar.gz"], ), + org_unicode_icuuc = dict( + strip_prefix = "icu-release-64-2", + sha256 = "524960ac99d086cdb6988d2a92fc163436fd3c6ec0a84c475c6382fbf989be05", + urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"], + ), ) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index b3f76bd75413f..f81afac224744 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -336,7 +336,7 @@ envoy_cc_library( hdrs = ["utility.h"], external_deps = [ "abseil_optional", - "http_parser", + "googleurl", "nghttp2", ], deps = [ diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 7312f322cae3c..d660672b595a0 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -717,9 +717,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // request-target. A proxy that forwards such a request MUST generate a // new Host field-value based on the received request-target rather than // forward the received Host field-value. - headers.setHost(absolute_url.host_and_port()); + headers.setHost(absolute_url.hostAndPort()); - headers.setPath(absolute_url.path_and_query_params()); + headers.setPath(absolute_url.pathAndQueryParams()); active_request.request_url_.clear(); } diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index ea4aa41cd4e80..024c9ae9a0306 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -1,8 +1,7 @@ #include "common/http/utility.h" -#include - #include +#include #include #include @@ -183,41 +182,30 @@ initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions namespace Http { -static const char kDefaultPath[] = "/"; - bool Utility::Url::initialize(absl::string_view absolute_url) { - struct http_parser_url u; - const bool is_connect = false; - http_parser_url_init(&u); - const int result = - http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); - - if (result != 0) { - return false; - } - if ((u.field_set & (1 << UF_HOST)) != (1 << UF_HOST) && - (u.field_set & (1 << UF_SCHEMA)) != (1 << UF_SCHEMA)) { + // TODO(dio): When we have access to base::StringPiece, probably we can convert absolute_url to + // that instead. + GURL parsed(std::string{absolute_url}); + if (!parsed.is_valid() || (!parsed.SchemeIsHTTPOrHTTPS() && !parsed.SchemeIsWSOrWSS())) { return false; } - scheme_ = absl::string_view(absolute_url.data() + u.field_data[UF_SCHEMA].off, - u.field_data[UF_SCHEMA].len); - uint16_t authority_len = u.field_data[UF_HOST].len; - if ((u.field_set & (1 << UF_PORT)) == (1 << UF_PORT)) { - authority_len = authority_len + u.field_data[UF_PORT].len + 1; - } - host_and_port_ = - absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); + scheme_ = parsed.scheme(); - // RFC allows the absolute-uri to not end in /, but the absolute path form - // must start with - uint64_t path_len = - absolute_url.length() - (u.field_data[UF_HOST].off + host_and_port().length()); - if (path_len > 0) { - uint64_t path_beginning = u.field_data[UF_HOST].off + host_and_port().length(); - path_and_query_params_ = absl::string_view(absolute_url.data() + path_beginning, path_len); - } else { - path_and_query_params_ = absl::string_view(kDefaultPath, 1); + // Only non-default ports will be rendered as part of host_and_port_. For example, + // http://www.host.com:80 has port component (i.e. 80). However, since 80 is a default port for + // http scheme, host_and_port_ will be rendered as www.host.com (without port). The same case with + // https scheme (with port 443) as well. + host_and_port_ = absl::StrCat(parsed.host(), parsed.has_port() ? ":" : "", parsed.port()); + + const int port = parsed.EffectiveIntPort(); + ASSERT(port <= std::numeric_limits::max() && port >= 0); + port_ = static_cast(port); + + // RFC allows the absolute-uri to not end in /, but the absolute path form must start with. + path_and_query_params_ = parsed.PathForRequest(); + if (parsed.has_ref()) { + absl::StrAppend(&path_and_query_params_, "#", parsed.ref()); } return true; } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 2daa237894d17..25abd89463d81 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -18,6 +18,7 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "nghttp2/nghttp2.h" +#include "url/gurl.h" namespace Envoy { namespace Http2 { @@ -102,14 +103,16 @@ namespace Utility { class Url { public: bool initialize(absl::string_view absolute_url); - absl::string_view scheme() { return scheme_; } - absl::string_view host_and_port() { return host_and_port_; } - absl::string_view path_and_query_params() { return path_and_query_params_; } + absl::string_view scheme() const { return scheme_; } + absl::string_view hostAndPort() const { return host_and_port_; } + absl::string_view pathAndQueryParams() const { return path_and_query_params_; } + uint64_t port() const { return port_; } private: - absl::string_view scheme_; - absl::string_view host_and_port_; - absl::string_view path_and_query_params_; + std::string scheme_; + std::string host_and_port_; + std::string path_and_query_params_; + uint16_t port_{0}; }; class PercentEncoding { diff --git a/source/common/router/router.cc b/source/common/router/router.cc index b31792ba24283..dfaed36addbec 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -105,8 +105,8 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Replace the original host, scheme and path. downstream_headers.setScheme(absolute_url.scheme()); - downstream_headers.setHost(absolute_url.host_and_port()); - downstream_headers.setPath(absolute_url.path_and_query_params()); + downstream_headers.setHost(absolute_url.hostAndPort()); + downstream_headers.setPath(absolute_url.pathAndQueryParams()); return true; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index d18f95de35df0..bc46742357381 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -36,7 +36,7 @@ absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { if (absolute_url.initialize(header->value().getStringView())) { - return absolute_url.host_and_port(); + return absolute_url.hostAndPort(); } return header->value().getStringView(); } diff --git a/test/common/http/http1/codec_impl_test.cc b/test/common/http/http1/codec_impl_test.cc index 23aaa9c80f0e2..bef27dae266e2 100644 --- a/test/common/http/http1/codec_impl_test.cc +++ b/test/common/http/http1/codec_impl_test.cc @@ -414,6 +414,15 @@ TEST_F(Http1ServerConnectionImplTest, Http10Absolute) { expectHeadersTest(Protocol::Http10, true, buffer, expected_headers); } +TEST_F(Http1ServerConnectionImplTest, Http10AbsoluteWithCustomPort) { + initialize(); + + TestHeaderMapImpl expected_headers{ + {":authority", "www.somewhere.com:8080"}, {":path", "/foobar"}, {":method", "GET"}}; + Buffer::OwnedImpl buffer("GET http://www.somewhere.com:8080/foobar HTTP/1.0\r\n\r\n"); + expectHeadersTest(Protocol::Http10, true, buffer, expected_headers); +} + TEST_F(Http1ServerConnectionImplTest, Http10MultipleResponses) { initialize(); diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 8c5c4d55efb62..09a73889274bc 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1080,65 +1080,72 @@ TEST(Url, ParsingFails) { } void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, - absl::string_view expected_host_port, absl::string_view expected_path) { + absl::string_view expected_host_port, absl::string_view expected_path, + uint64_t expected_port) { Utility::Url url; ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); - EXPECT_EQ(url.host_and_port(), expected_host_port); - EXPECT_EQ(url.path_and_query_params(), expected_path); + EXPECT_EQ(url.hostAndPort(), expected_host_port); + EXPECT_EQ(url.pathAndQueryParams(), expected_path); + EXPECT_EQ(url.port(), expected_port); } TEST(Url, ParsingTest) { - // Test url with no explicit path (with and without port) - ValidateUrl("http://www.host.com", "http", "www.host.com", "/"); - ValidateUrl("http://www.host.com:80", "http", "www.host.com:80", "/"); + // Test url with no explicit path (with and without port). + ValidateUrl("http://www.host.com", "http", "www.host.com", "/", 80); + ValidateUrl("http://www.host.com:80", "http", "www.host.com", "/", 80); // Test url with "/" path. - ValidateUrl("http://www.host.com:80/", "http", "www.host.com:80", "/"); - ValidateUrl("http://www.host.com/", "http", "www.host.com", "/"); + ValidateUrl("http://www.host.com:80/", "http", "www.host.com", "/", 80); + ValidateUrl("http://www.host.com/", "http", "www.host.com", "/", 80); // Test url with "?". - ValidateUrl("http://www.host.com:80/?", "http", "www.host.com:80", "/?"); - ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/?"); + ValidateUrl("http://www.host.com:80/?", "http", "www.host.com", "/?", 80); + ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/?", 80); // Test url with "?" but without slash. - ValidateUrl("http://www.host.com:80?", "http", "www.host.com:80", "?"); - ValidateUrl("http://www.host.com?", "http", "www.host.com", "?"); + ValidateUrl("http://www.host.com:80?", "http", "www.host.com", "/?", 80); + ValidateUrl("http://www.host.com?", "http", "www.host.com", "/?", 80); - // Test url with multi-character path - ValidateUrl("http://www.host.com:80/path", "http", "www.host.com:80", "/path"); - ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path"); + // Test url with multi-character path. + ValidateUrl("http://www.host.com:80/path", "http", "www.host.com", "/path", 80); + ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path", 80); - // Test url with multi-character path and ? at the end - ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com:80", "/path?"); - ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?"); + // Test url with multi-character path and ? at the end. + ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com", "/path?", 80); + ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?", 80); - // Test https scheme - ValidateUrl("https://www.host.com", "https", "www.host.com", "/"); + // Test https scheme. + ValidateUrl("https://www.host.com", "https", "www.host.com", "/", 443); - // Test url with query parameter - ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com:80", "/?query=param"); - ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param"); + // Test url with query parameter. + ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com", "/?query=param", 80); + ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param", 80); - // Test url with query parameter but without slash - ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com:80", "?query=param"); - ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "?query=param"); + // Test url with query parameter but without slash. It will be normalized. + ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com", "/?query=param", 80); + ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "/?query=param", 80); - // Test url with multi-character path and query parameter - ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com:80", - "/path?query=param"); - ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param"); + // Test url with multi-character path and query parameter. + ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com", + "/path?query=param", 80); + ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param", + 80); - // Test url with multi-character path and more than one query parameter - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com:80", - "/path?query=param&query2=param2"); + // Test url with multi-character path and more than one query parameter. + ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com", + "/path?query=param&query2=param2", 80); ValidateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", - "/path?query=param&query2=param2"); + "/path?query=param&query2=param2", 80); // Test url with multi-character path, more than one query parameter and fragment ValidateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", - "www.host.com:80", "/path?query=param&query2=param2#fragment"); + "www.host.com", "/path?query=param&query2=param2#fragment", 80); ValidateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", - "/path?query=param&query2=param2#fragment"); + "/path?query=param&query2=param2#fragment", 80); + + // Test url with non-default ports. + ValidateUrl("https://www.host.com:8443", "https", "www.host.com:8443", "/", 8443); + ValidateUrl("http://www.host.com:8080", "http", "www.host.com:8080", "/", 8080); } void validatePercentEncodingEncodeDecode(absl::string_view source, From 003bc933a0b3b1dd97d6ff5663f2cd144a937af4 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 1 Apr 2020 10:26:33 +0700 Subject: [PATCH 02/20] Fix build vs BUILD Signed-off-by: Dhi Aurrahman --- bazel/repository_locations.bzl | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index e0f40065c88e1..926b992a179cd 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -282,8 +282,9 @@ REPOSITORY_LOCATIONS = dict( ), com_googlesource_googleurl = dict( # Static snapshot of https://quiche.googlesource.com/quiche/+archive/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz - sha256 = "b40cd22cadba577b7281a76db66f6a66dd744edbad8cc2c861c2c976ef721e4d", - urls = ["https://storage.googleapis.com/quiche-envoy-integration/googleurl_dbf5ad147f60afc125e99db7549402af49a5eae8.tar.gz"], + sha256 = "5bdbecce22d522f03ac86c6a694d5ff76780a16d6d682910e87ac156b0a25b21", + strip_prefix = "googleurl-dbf5ad1", + urls = ["https://github.com/dio/googleurl/archive/dbf5ad1.tar.gz"], ), com_google_cel_cpp = dict( sha256 = "326ec397b55e39f48bd5380ccded1af5b04653ee96e769cd4d694f9a3bacef50", From 553d357308c44e04a8506e0af42e09cdcd3c639b Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 1 Apr 2020 10:32:33 +0700 Subject: [PATCH 03/20] Revert name Signed-off-by: Dhi Aurrahman --- source/common/http/http1/codec_impl.cc | 4 ++-- source/common/http/utility.h | 4 ++-- source/common/router/router.cc | 4 ++-- source/extensions/filters/http/csrf/csrf_filter.cc | 10 +++++----- test/common/http/utility_test.cc | 4 ++-- 5 files changed, 13 insertions(+), 13 deletions(-) diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index d660672b595a0..7312f322cae3c 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -717,9 +717,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // request-target. A proxy that forwards such a request MUST generate a // new Host field-value based on the received request-target rather than // forward the received Host field-value. - headers.setHost(absolute_url.hostAndPort()); + headers.setHost(absolute_url.host_and_port()); - headers.setPath(absolute_url.pathAndQueryParams()); + headers.setPath(absolute_url.path_and_query_params()); active_request.request_url_.clear(); } diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 25abd89463d81..c1658af1253ae 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -104,8 +104,8 @@ class Url { public: bool initialize(absl::string_view absolute_url); absl::string_view scheme() const { return scheme_; } - absl::string_view hostAndPort() const { return host_and_port_; } - absl::string_view pathAndQueryParams() const { return path_and_query_params_; } + absl::string_view host_and_port() const { return host_and_port_; } + absl::string_view path_and_query_params() const { return path_and_query_params_; } uint64_t port() const { return port_; } private: diff --git a/source/common/router/router.cc b/source/common/router/router.cc index dfaed36addbec..b31792ba24283 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -105,8 +105,8 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Replace the original host, scheme and path. downstream_headers.setScheme(absolute_url.scheme()); - downstream_headers.setHost(absolute_url.hostAndPort()); - downstream_headers.setPath(absolute_url.pathAndQueryParams()); + downstream_headers.setHost(absolute_url.host_and_port()); + downstream_headers.setPath(absolute_url.path_and_query_params()); return true; } diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index bc46742357381..470c4eff9e3d7 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -32,11 +32,11 @@ bool isModifyMethod(const Http::RequestHeaderMap& headers) { method_type == method_values.Delete || method_type == method_values.Patch); } -absl::string_view hostAndPort(const Http::HeaderEntry* header) { +absl::string_view host_and_port(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { if (absolute_url.initialize(header->value().getStringView())) { - return absolute_url.hostAndPort(); + return absolute_url.host_and_port(); } return header->value().getStringView(); } @@ -44,15 +44,15 @@ absl::string_view hostAndPort(const Http::HeaderEntry* header) { } absl::string_view sourceOriginValue(const Http::RequestHeaderMap& headers) { - const absl::string_view origin = hostAndPort(headers.Origin()); + const absl::string_view origin = host_and_port(headers.Origin()); if (origin != EMPTY_STRING) { return origin; } - return hostAndPort(headers.Referer()); + return host_and_port(headers.Referer()); } absl::string_view targetOriginValue(const Http::RequestHeaderMap& headers) { - return hostAndPort(headers.Host()); + return host_and_port(headers.Host()); } static CsrfStats generateStats(const std::string& prefix, Stats::Scope& scope) { diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 09a73889274bc..eab64b6776e26 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1085,8 +1085,8 @@ void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, Utility::Url url; ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; EXPECT_EQ(url.scheme(), expected_scheme); - EXPECT_EQ(url.hostAndPort(), expected_host_port); - EXPECT_EQ(url.pathAndQueryParams(), expected_path); + EXPECT_EQ(url.host_and_port(), expected_host_port); + EXPECT_EQ(url.path_and_query_params(), expected_path); EXPECT_EQ(url.port(), expected_port); } From 499e330b575e0970273c996ab9c0579c0f3b1557 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Wed, 1 Apr 2020 10:37:10 +0700 Subject: [PATCH 04/20] Fix Signed-off-by: Dhi Aurrahman --- source/extensions/filters/http/csrf/csrf_filter.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index 470c4eff9e3d7..d18f95de35df0 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -32,7 +32,7 @@ bool isModifyMethod(const Http::RequestHeaderMap& headers) { method_type == method_values.Delete || method_type == method_values.Patch); } -absl::string_view host_and_port(const Http::HeaderEntry* header) { +absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { if (absolute_url.initialize(header->value().getStringView())) { @@ -44,15 +44,15 @@ absl::string_view host_and_port(const Http::HeaderEntry* header) { } absl::string_view sourceOriginValue(const Http::RequestHeaderMap& headers) { - const absl::string_view origin = host_and_port(headers.Origin()); + const absl::string_view origin = hostAndPort(headers.Origin()); if (origin != EMPTY_STRING) { return origin; } - return host_and_port(headers.Referer()); + return hostAndPort(headers.Referer()); } absl::string_view targetOriginValue(const Http::RequestHeaderMap& headers) { - return host_and_port(headers.Host()); + return hostAndPort(headers.Host()); } static CsrfStats generateStats(const std::string& prefix, Stats::Scope& scope) { From 2b89e1af87b543fc9c738d5c20a0f2019e99322f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 2 Apr 2020 05:50:38 +0700 Subject: [PATCH 05/20] Make it compiles on osx Signed-off-by: Dhi Aurrahman --- bazel/external/icuuc.BUILD | 3 +- bazel/external/icuuc.patch | 63 -------------------------------------- bazel/repositories.bzl | 3 +- 3 files changed, 2 insertions(+), 67 deletions(-) delete mode 100644 bazel/external/icuuc.patch diff --git a/bazel/external/icuuc.BUILD b/bazel/external/icuuc.BUILD index f6f2dd1ce489f..315f6fea892b1 100644 --- a/bazel/external/icuuc.BUILD +++ b/bazel/external/icuuc.BUILD @@ -1,5 +1,3 @@ -# https://github.com/tensorflow/tensorflow/commit/1154432f88387c81be4ac2e3cb249b787ffafe21 -# https://github.com/tensorflow/tensorflow/blob/068641e7f858d92407343b2c3994d3bee3822093/third_party/icu/BUILD.bazel licenses(["notice"]) # Apache 2 exports_files([ @@ -8,6 +6,7 @@ exports_files([ ]) icuuc_copts = [ + "-DU_STATIC_IMPLEMENTATION", "-DU_COMMON_IMPLEMENTATION", "-DU_HAVE_STD_ATOMICS", # TODO(gunan): Remove when TF is on ICU 64+. ] + select({ diff --git a/bazel/external/icuuc.patch b/bazel/external/icuuc.patch deleted file mode 100644 index 69e00c8fbe155..0000000000000 --- a/bazel/external/icuuc.patch +++ /dev/null @@ -1,63 +0,0 @@ -# Adapted from TensorFlow's third_party/icu/udata.patch. -# https://github.com/tensorflow/tensorflow/blob/bec9e52822c330ae926aa029cd26e924b42166a9/third_party/icu/udata.patch -diff --git a/icu4c/source/common/udata.cpp b/icu4c/source/common/udata.cpp -index 3cb8863f6c..86fc7999a0 100644 ---- a/icu4c/source/common/udata.cpp -+++ b/icu4c/source/common/udata.cpp -@@ -18,11 +18,10 @@ - - #include "unicode/utypes.h" /* U_PLATFORM etc. */ - --#ifdef __GNUC__ --/* if gcc --#define ATTRIBUTE_WEAK __attribute__ ((weak)) --might have to #include some other header --*/ -+#if defined(__GNUC__) || defined(__SUNPRO_CC) -+# define ATTRIBUTE_WEAK __attribute__ ((weak)) -+#else -+# define ATTRIBUTE_WEAK - #endif - - #include "unicode/putil.h" -@@ -641,10 +640,9 @@ extern "C" const DataHeader U_DATA_API U_ICUDATA_ENTRY_POINT; - * partial-data-library access functions where each returns a pointer - * to its data package, if it is linked in. - */ --/* --extern const void *uprv_getICUData_collation(void) ATTRIBUTE_WEAK; --extern const void *uprv_getICUData_conversion(void) ATTRIBUTE_WEAK; --*/ -+U_CDECL_BEGIN -+const void *uprv_getICUData_conversion(void) ATTRIBUTE_WEAK; -+U_CDECL_END - - /*----------------------------------------------------------------------* - * * -@@ -702,10 +700,10 @@ openCommonData(const char *path, /* Path from OpenChoice? */ - if (uprv_getICUData_collation) { - setCommonICUDataPointer(uprv_getICUData_collation(), FALSE, pErrorCode); - } -+ */ - if (uprv_getICUData_conversion) { - setCommonICUDataPointer(uprv_getICUData_conversion(), FALSE, pErrorCode); - } -- */ - #if U_PLATFORM_HAS_WINUWP_API == 0 // Windows UWP Platform does not support dll icu data at this time - setCommonICUDataPointer(&U_ICUDATA_ENTRY_POINT, FALSE, pErrorCode); - { -diff --git a/icu4c/source/common/unicode/uconfig.h b/icu4c/source/common/unicode/uconfig.h -index 7ddf4e6adf..f95d7a2932 100644 ---- a/icu4c/source/common/unicode/uconfig.h -+++ b/icu4c/source/common/unicode/uconfig.h -@@ -55,6 +55,10 @@ - #include "uconfig_local.h" - #endif - -+#ifndef U_STATIC_IMPLEMENTATION -+#define U_STATIC_IMPLEMENTATION -+#endif -+ - /** - * \def U_DEBUG - * Determines whether to include debugging code. diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 91e2c3113fabe..d0076ef120989 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -232,8 +232,7 @@ def _org_unicode_icuuc(): _repository_impl( name = "org_unicode_icuuc", build_file = "@envoy//bazel/external:icuuc.BUILD", - patches = ["@envoy//bazel/external:icuuc.patch"], - patch_args = ["-p1"], + # TODO(dio): Consider patching udata when we need to embed some data. ) native.bind( name = "icuuc", From 2dfff67e05d4951f8d4d52dd7ffb995409568d9b Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 2 Apr 2020 07:10:15 +0700 Subject: [PATCH 06/20] clang-tidy Signed-off-by: Dhi Aurrahman --- test/common/http/utility_test.cc | 52 ++++++++++++++++---------------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index eab64b6776e26..e593c6071256b 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1079,7 +1079,7 @@ TEST(Url, ParsingFails) { EXPECT_FALSE(url.initialize("random_scheme://host.com/path")); } -void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, +void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, absl::string_view expected_host_port, absl::string_view expected_path, uint64_t expected_port) { Utility::Url url; @@ -1092,60 +1092,60 @@ void ValidateUrl(absl::string_view raw_url, absl::string_view expected_scheme, TEST(Url, ParsingTest) { // Test url with no explicit path (with and without port). - ValidateUrl("http://www.host.com", "http", "www.host.com", "/", 80); - ValidateUrl("http://www.host.com:80", "http", "www.host.com", "/", 80); + validateUrl("http://www.host.com", "http", "www.host.com", "/", 80); + validateUrl("http://www.host.com:80", "http", "www.host.com", "/", 80); // Test url with "/" path. - ValidateUrl("http://www.host.com:80/", "http", "www.host.com", "/", 80); - ValidateUrl("http://www.host.com/", "http", "www.host.com", "/", 80); + validateUrl("http://www.host.com:80/", "http", "www.host.com", "/", 80); + validateUrl("http://www.host.com/", "http", "www.host.com", "/", 80); // Test url with "?". - ValidateUrl("http://www.host.com:80/?", "http", "www.host.com", "/?", 80); - ValidateUrl("http://www.host.com/?", "http", "www.host.com", "/?", 80); + validateUrl("http://www.host.com:80/?", "http", "www.host.com", "/?", 80); + validateUrl("http://www.host.com/?", "http", "www.host.com", "/?", 80); // Test url with "?" but without slash. - ValidateUrl("http://www.host.com:80?", "http", "www.host.com", "/?", 80); - ValidateUrl("http://www.host.com?", "http", "www.host.com", "/?", 80); + validateUrl("http://www.host.com:80?", "http", "www.host.com", "/?", 80); + validateUrl("http://www.host.com?", "http", "www.host.com", "/?", 80); // Test url with multi-character path. - ValidateUrl("http://www.host.com:80/path", "http", "www.host.com", "/path", 80); - ValidateUrl("http://www.host.com/path", "http", "www.host.com", "/path", 80); + validateUrl("http://www.host.com:80/path", "http", "www.host.com", "/path", 80); + validateUrl("http://www.host.com/path", "http", "www.host.com", "/path", 80); // Test url with multi-character path and ? at the end. - ValidateUrl("http://www.host.com:80/path?", "http", "www.host.com", "/path?", 80); - ValidateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?", 80); + validateUrl("http://www.host.com:80/path?", "http", "www.host.com", "/path?", 80); + validateUrl("http://www.host.com/path?", "http", "www.host.com", "/path?", 80); // Test https scheme. - ValidateUrl("https://www.host.com", "https", "www.host.com", "/", 443); + validateUrl("https://www.host.com", "https", "www.host.com", "/", 443); // Test url with query parameter. - ValidateUrl("http://www.host.com:80/?query=param", "http", "www.host.com", "/?query=param", 80); - ValidateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param", 80); + validateUrl("http://www.host.com:80/?query=param", "http", "www.host.com", "/?query=param", 80); + validateUrl("http://www.host.com/?query=param", "http", "www.host.com", "/?query=param", 80); // Test url with query parameter but without slash. It will be normalized. - ValidateUrl("http://www.host.com:80?query=param", "http", "www.host.com", "/?query=param", 80); - ValidateUrl("http://www.host.com?query=param", "http", "www.host.com", "/?query=param", 80); + validateUrl("http://www.host.com:80?query=param", "http", "www.host.com", "/?query=param", 80); + validateUrl("http://www.host.com?query=param", "http", "www.host.com", "/?query=param", 80); // Test url with multi-character path and query parameter. - ValidateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com", + validateUrl("http://www.host.com:80/path?query=param", "http", "www.host.com", "/path?query=param", 80); - ValidateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param", + validateUrl("http://www.host.com/path?query=param", "http", "www.host.com", "/path?query=param", 80); // Test url with multi-character path and more than one query parameter. - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com", + validateUrl("http://www.host.com:80/path?query=param&query2=param2", "http", "www.host.com", "/path?query=param&query2=param2", 80); - ValidateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2", "http", "www.host.com", "/path?query=param&query2=param2", 80); // Test url with multi-character path, more than one query parameter and fragment - ValidateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", + validateUrl("http://www.host.com:80/path?query=param&query2=param2#fragment", "http", "www.host.com", "/path?query=param&query2=param2#fragment", 80); - ValidateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", + validateUrl("http://www.host.com/path?query=param&query2=param2#fragment", "http", "www.host.com", "/path?query=param&query2=param2#fragment", 80); // Test url with non-default ports. - ValidateUrl("https://www.host.com:8443", "https", "www.host.com:8443", "/", 8443); - ValidateUrl("http://www.host.com:8080", "http", "www.host.com:8080", "/", 8080); + validateUrl("https://www.host.com:8443", "https", "www.host.com:8443", "/", 8443); + validateUrl("http://www.host.com:8080", "http", "www.host.com:8080", "/", 8080); } void validatePercentEncodingEncodeDecode(absl::string_view source, From f7f35f03f93e8b9ae9cec71ea6bbb2ea8476e3a4 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 2 Apr 2020 15:13:30 +0000 Subject: [PATCH 07/20] Introduce url_utility Signed-off-by: Dhi Aurrahman --- source/common/http/BUILD | 14 +- source/common/http/http1/BUILD | 1 + source/common/http/http1/codec_impl.cc | 5 +- source/common/http/url_utility.cc | 124 ++++++++++++++++++ source/common/http/url_utility.h | 37 ++++++ source/common/http/utility.cc | 28 ---- source/common/http/utility.h | 20 --- source/common/router/BUILD | 1 + source/common/router/router.cc | 9 +- source/extensions/filters/http/csrf/BUILD | 1 + .../filters/http/csrf/csrf_filter.cc | 3 +- test/common/http/BUILD | 1 + test/common/http/utility_test.cc | 10 +- 13 files changed, 194 insertions(+), 60 deletions(-) create mode 100644 source/common/http/url_utility.cc create mode 100644 source/common/http/url_utility.h diff --git a/source/common/http/BUILD b/source/common/http/BUILD index f81afac224744..8ed8c137197e0 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -336,7 +336,6 @@ envoy_cc_library( hdrs = ["utility.h"], external_deps = [ "abseil_optional", - "googleurl", "nghttp2", ], deps = [ @@ -361,6 +360,19 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "url_utility_lib", + srcs = ["url_utility.cc"], + hdrs = ["url_utility.h"], + deps = [ + "//source/common/common:assert_lib", + "//source/common/common:empty_string", + ] + select({ + "//bazel:windows_x86_64": ["@com_github_nodejs_http_parser//:http_parser"], + "//conditions:default": ["@com_googlesource_googleurl//url:url"], + }), +) + envoy_cc_library( name = "header_utility_lib", srcs = ["header_utility.cc"], diff --git a/source/common/http/http1/BUILD b/source/common/http/http1/BUILD index 4bb878b024eca..5864115268946 100644 --- a/source/common/http/http1/BUILD +++ b/source/common/http/http1/BUILD @@ -36,6 +36,7 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/http:header_utility_lib", "//source/common/http:headers_lib", + "//source/common/http:url_utility_lib", "//source/common/http:utility_lib", "//source/common/http/http1:header_formatter_lib", "//source/common/runtime:runtime_lib", diff --git a/source/common/http/http1/codec_impl.cc b/source/common/http/http1/codec_impl.cc index 7312f322cae3c..8ad1d213ff189 100644 --- a/source/common/http/http1/codec_impl.cc +++ b/source/common/http/http1/codec_impl.cc @@ -15,6 +15,7 @@ #include "common/http/header_utility.h" #include "common/http/headers.h" #include "common/http/http1/header_formatter.h" +#include "common/http/url_utility.h" #include "common/http/utility.h" #include "common/runtime/runtime_impl.h" @@ -717,9 +718,9 @@ void ServerConnectionImpl::handlePath(RequestHeaderMap& headers, unsigned int me // request-target. A proxy that forwards such a request MUST generate a // new Host field-value based on the received request-target rather than // forward the received Host field-value. - headers.setHost(absolute_url.host_and_port()); + headers.setHost(absolute_url.getHostAndPort()); - headers.setPath(absolute_url.path_and_query_params()); + headers.setPath(absolute_url.getPathAndQueryParams()); active_request.request_url_.clear(); } diff --git a/source/common/http/url_utility.cc b/source/common/http/url_utility.cc new file mode 100644 index 0000000000000..9ccdb3280e29f --- /dev/null +++ b/source/common/http/url_utility.cc @@ -0,0 +1,124 @@ +#include "common/http/url_utility.h" + +#include "common/common/assert.h" +#include "common/common/empty_string.h" + +#include "absl/strings/str_cat.h" + +#ifndef WIN32 +#include "url/gurl.h" +#else +#include +#endif + +namespace Envoy { +namespace Http { +namespace Utility { + +#ifndef WIN32 +constexpr char COLON[] = ":"; +constexpr char HASH[] = "#"; +#else +constexpr char DEFAULT_PATH[] = "/"; +constexpr char HTTP[] = "http"; +constexpr char HTTPS[] = "https"; +#endif + +bool Url::initialize(absl::string_view absolute_url) { +#ifndef WIN32 + // TODO(dio): When we have access to base::StringPiece, probably we can convert absolute_url to + // that instead. + GURL parsed(std::string{absolute_url}); + if (!parsed.is_valid() || (!parsed.SchemeIsHTTPOrHTTPS())) { + return false; + } + + scheme_ = parsed.scheme(); + + // Only non-default ports will be rendered as part of host_and_port_. For example, + // http://www.host.com:80 has port component (i.e. 80). However, since 80 is a default port for + // http scheme, host_and_port_ will be rendered as www.host.com (without port). The same case with + // https scheme (with port 443) as well. + host_and_port_ = + absl::StrCat(parsed.host(), parsed.has_port() ? COLON : EMPTY_STRING, parsed.port()); + + const int port = parsed.EffectiveIntPort(); + ASSERT(port <= std::numeric_limits::max() && port >= 0); + port_ = static_cast(port); + + // RFC allows the absolute-uri to not end in "/", but the absolute path form must start with "/". + path_and_query_params_ = parsed.PathForRequest(); + if (parsed.has_ref()) { + absl::StrAppend(&path_and_query_params_, HASH, parsed.ref()); + } +#else + struct http_parser_url u; + const bool is_connect = false; + http_parser_url_init(&u); + const int result = + http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); + + if (result != 0) { + return false; + } + if ((u.field_set & (1 << UF_HOST)) != (1 << UF_HOST) && + (u.field_set & (1 << UF_SCHEMA)) != (1 << UF_SCHEMA)) { + return false; + } + scheme_ = absl::string_view(absolute_url.data() + u.field_data[UF_SCHEMA].off, + u.field_data[UF_SCHEMA].len); + + uint16_t authority_len = u.field_data[UF_HOST].len; + uint16_t authority_and_port_len = u.field_data[UF_HOST].len; + + bool has_port = (u.field_set & (1 << UF_PORT)) == (1 << UF_PORT); + if (has_port) { + int port = 0; + const bool converted_to_int = + absl::SimpleAtoi(absl::string_view(absolute_url.data() + u.field_data[UF_PORT].off, + u.field_data[UF_PORT].len), + &port); + ASSERT(converted_to_int); + ASSERT(port <= std::numeric_limits::max() && port >= 0); + port_ = static_cast(port); + authority_and_port_len = authority_and_port_len + u.field_data[UF_PORT].len + 1; + } else { + // When there is no port specified in the input URL, we set the port_ value by default ports of + // the known schemes. + if (scheme_ == HTTP) { + port_ = 80; + } else if (scheme_ == HTTPS) { + port_ = 443; + } else { + port_ = 0; + } + } + + const bool has_http_default_port = port_ == 80 && scheme_ == HTTP; + const bool has_https_default_port = port_ == 443 && scheme_ == HTTPS; + if (!has_http_default_port && !has_https_default_port) { + authority_len = authority_len + u.field_data[UF_PORT].len + 1; + } + host_and_port_ = + absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); + + // RFC allows the absolute-uri to not end in "/", but the absolute path form must start with "/". + const uint64_t path_len = + absolute_url.length() - (u.field_data[UF_HOST].off + authority_and_port_len); + if (path_len > 0) { + uint64_t path_beginning = u.field_data[UF_HOST].off + authority_and_port_len; + const auto slash = absl::string_view(absolute_url.data() + path_beginning, 1); + path_and_query_params_ = + absl::StrCat(slash != DEFAULT_PATH ? DEFAULT_PATH : EMPTY_STRING, + absl::string_view(absolute_url.data() + path_beginning, path_len)); + } else { + path_and_query_params_ = DEFAULT_PATH; + } +#endif + + return true; +} + +} // namespace Utility +} // namespace Http +} // namespace Envoy \ No newline at end of file diff --git a/source/common/http/url_utility.h b/source/common/http/url_utility.h new file mode 100644 index 0000000000000..3a07e26657b0b --- /dev/null +++ b/source/common/http/url_utility.h @@ -0,0 +1,37 @@ +#pragma once + +#include + +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Http { +namespace Utility { +/** + * Given a fully qualified URL, splits the string_view provided into scheme, host, port and path + * with query parameters components (including fragments). + */ +class Url { +public: + bool initialize(absl::string_view absolute_url); + absl::string_view getScheme() const { return scheme_; } + absl::string_view getHostAndPort() const { return host_and_port_; } + absl::string_view getPathAndQueryParams() const { return path_and_query_params_; } + uint64_t getPort() const { return port_; } + +private: +#ifndef WIN32 + std::string scheme_; + std::string host_and_port_; +#else + absl::string_view scheme_; + absl::string_view host_and_port_; +#endif + std::string path_and_query_params_; + uint16_t port_{0}; +}; + +} // namespace Utility + +} // namespace Http +} // namespace Envoy \ No newline at end of file diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 024c9ae9a0306..ae67831d720f8 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -182,34 +182,6 @@ initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions namespace Http { -bool Utility::Url::initialize(absl::string_view absolute_url) { - // TODO(dio): When we have access to base::StringPiece, probably we can convert absolute_url to - // that instead. - GURL parsed(std::string{absolute_url}); - if (!parsed.is_valid() || (!parsed.SchemeIsHTTPOrHTTPS() && !parsed.SchemeIsWSOrWSS())) { - return false; - } - - scheme_ = parsed.scheme(); - - // Only non-default ports will be rendered as part of host_and_port_. For example, - // http://www.host.com:80 has port component (i.e. 80). However, since 80 is a default port for - // http scheme, host_and_port_ will be rendered as www.host.com (without port). The same case with - // https scheme (with port 443) as well. - host_and_port_ = absl::StrCat(parsed.host(), parsed.has_port() ? ":" : "", parsed.port()); - - const int port = parsed.EffectiveIntPort(); - ASSERT(port <= std::numeric_limits::max() && port >= 0); - port_ = static_cast(port); - - // RFC allows the absolute-uri to not end in /, but the absolute path form must start with. - path_and_query_params_ = parsed.PathForRequest(); - if (parsed.has_ref()) { - absl::StrAppend(&path_and_query_params_, "#", parsed.ref()); - } - return true; -} - void Utility::appendXff(RequestHeaderMap& headers, const Network::Address::Instance& remote_address) { if (remote_address.type() != Network::Address::Type::Ip) { diff --git a/source/common/http/utility.h b/source/common/http/utility.h index c1658af1253ae..4fc7459a8e2c7 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -18,7 +18,6 @@ #include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "nghttp2/nghttp2.h" -#include "url/gurl.h" namespace Envoy { namespace Http2 { @@ -96,25 +95,6 @@ initializeAndValidateOptions(const envoy::config::core::v3::Http2ProtocolOptions namespace Http { namespace Utility { -/** - * Given a fully qualified URL, splits the string_view provided into scheme, - * host and path with query parameters components. - */ -class Url { -public: - bool initialize(absl::string_view absolute_url); - absl::string_view scheme() const { return scheme_; } - absl::string_view host_and_port() const { return host_and_port_; } - absl::string_view path_and_query_params() const { return path_and_query_params_; } - uint64_t port() const { return port_; } - -private: - std::string scheme_; - std::string host_and_port_; - std::string path_and_query_params_; - uint16_t port_{0}; -}; - class PercentEncoding { public: /** diff --git a/source/common/router/BUILD b/source/common/router/BUILD index f237a458d157a..e97ec28f29bb8 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -301,6 +301,7 @@ envoy_cc_library( "//source/common/http:header_map_lib", "//source/common/http:headers_lib", "//source/common/http:message_lib", + "//source/common/http:url_utility_lib", "//source/common/http:utility_lib", "//source/common/network:application_protocol_lib", "//source/common/network:transport_socket_options_lib", diff --git a/source/common/router/router.cc b/source/common/router/router.cc index b31792ba24283..8950235b740a4 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -24,6 +24,7 @@ #include "common/http/header_map_impl.h" #include "common/http/headers.h" #include "common/http/message_impl.h" +#include "common/http/url_utility.h" #include "common/http/utility.h" #include "common/network/application_protocol.h" #include "common/network/transport_socket_options_impl.h" @@ -76,7 +77,7 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Don't allow serving TLS responses over plaintext. bool scheme_is_http = schemeIsHttp(downstream_headers, connection); - if (scheme_is_http && absolute_url.scheme() == Http::Headers::get().SchemeValues.Https) { + if (scheme_is_http && absolute_url.getScheme() == Http::Headers::get().SchemeValues.Https) { return false; } @@ -104,9 +105,9 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream downstream_headers.Path()->value().getStringView())); // Replace the original host, scheme and path. - downstream_headers.setScheme(absolute_url.scheme()); - downstream_headers.setHost(absolute_url.host_and_port()); - downstream_headers.setPath(absolute_url.path_and_query_params()); + downstream_headers.setScheme(absolute_url.getScheme()); + downstream_headers.setHost(absolute_url.getHostAndPort()); + downstream_headers.setPath(absolute_url.getPathAndQueryParams()); return true; } diff --git a/source/extensions/filters/http/csrf/BUILD b/source/extensions/filters/http/csrf/BUILD index cd2315773e6e0..ddcecdd9ebc9f 100644 --- a/source/extensions/filters/http/csrf/BUILD +++ b/source/extensions/filters/http/csrf/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( "//source/common/common:matchers_lib", "//source/common/http:header_map_lib", "//source/common/http:headers_lib", + "//source/common/http:url_utility_lib", "//source/common/http:utility_lib", "//source/extensions/filters/http:well_known_names", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", diff --git a/source/extensions/filters/http/csrf/csrf_filter.cc b/source/extensions/filters/http/csrf/csrf_filter.cc index d18f95de35df0..c29efae9e3c09 100644 --- a/source/extensions/filters/http/csrf/csrf_filter.cc +++ b/source/extensions/filters/http/csrf/csrf_filter.cc @@ -6,6 +6,7 @@ #include "common/common/empty_string.h" #include "common/http/header_map_impl.h" #include "common/http/headers.h" +#include "common/http/url_utility.h" #include "common/http/utility.h" #include "extensions/filters/http/well_known_names.h" @@ -36,7 +37,7 @@ absl::string_view hostAndPort(const Http::HeaderEntry* header) { Http::Utility::Url absolute_url; if (header != nullptr && !header->value().empty()) { if (absolute_url.initialize(header->value().getStringView())) { - return absolute_url.host_and_port(); + return absolute_url.getHostAndPort(); } return header->value().getStringView(); } diff --git a/test/common/http/BUILD b/test/common/http/BUILD index dce641d6475c3..918c05adc6f74 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -356,6 +356,7 @@ envoy_cc_test( deps = [ "//source/common/http:exception_lib", "//source/common/http:header_map_lib", + "//source/common/http:url_utility_lib", "//source/common/http:utility_lib", "//source/common/network:address_lib", "//test/mocks/http:http_mocks", diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index e593c6071256b..9ad50cbca0049 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -9,6 +9,7 @@ #include "common/common/fmt.h" #include "common/http/exception.h" #include "common/http/header_map_impl.h" +#include "common/http/url_utility.h" #include "common/http/utility.h" #include "common/network/address_impl.h" @@ -1077,6 +1078,7 @@ TEST(Url, ParsingFails) { EXPECT_FALSE(url.initialize("foo")); EXPECT_FALSE(url.initialize("http://")); EXPECT_FALSE(url.initialize("random_scheme://host.com/path")); + EXPECT_FALSE(url.initialize("http://host.com:65536/path")); } void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, @@ -1084,10 +1086,10 @@ void validateUrl(absl::string_view raw_url, absl::string_view expected_scheme, uint64_t expected_port) { Utility::Url url; ASSERT_TRUE(url.initialize(raw_url)) << "Failed to initialize " << raw_url; - EXPECT_EQ(url.scheme(), expected_scheme); - EXPECT_EQ(url.host_and_port(), expected_host_port); - EXPECT_EQ(url.path_and_query_params(), expected_path); - EXPECT_EQ(url.port(), expected_port); + EXPECT_EQ(url.getScheme(), expected_scheme); + EXPECT_EQ(url.getHostAndPort(), expected_host_port); + EXPECT_EQ(url.getPathAndQueryParams(), expected_path); + EXPECT_EQ(url.getPort(), expected_port); } TEST(Url, ParsingTest) { From 2214afe12d526ded3f9f4adc4d297f7167e230ee Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 2 Apr 2020 15:45:17 +0000 Subject: [PATCH 08/20] Fix quic Signed-off-by: Dhi Aurrahman --- source/extensions/quic_listeners/quiche/platform/BUILD | 2 +- .../quic_listeners/quiche/platform/quic_hostname_utils_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/quic_listeners/quiche/platform/BUILD b/source/extensions/quic_listeners/quiche/platform/BUILD index 3f5cb5fef47f6..3545532250731 100644 --- a/source/extensions/quic_listeners/quiche/platform/BUILD +++ b/source/extensions/quic_listeners/quiche/platform/BUILD @@ -187,7 +187,7 @@ envoy_cc_library( "//source/common/common:assert_lib", "//source/common/filesystem:directory_lib", "//source/common/filesystem:filesystem_lib", - "//source/common/http:utility_lib", + "//source/common/http:url_utility_lib", ], ) diff --git a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc index 4b7b40744977a..159a0448eb8c5 100644 --- a/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc +++ b/source/extensions/quic_listeners/quiche/platform/quic_hostname_utils_impl.cc @@ -8,7 +8,7 @@ #include -#include "common/http/utility.h" +#include "common/http/url_utility.h" #include "absl/strings/ascii.h" #include "absl/strings/str_cat.h" From 76c618fe702fa3acdd847000db49a7e5eee5204d Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Thu, 2 Apr 2020 16:15:57 +0000 Subject: [PATCH 09/20] Fix Signed-off-by: Dhi Aurrahman --- source/extensions/quic_listeners/quiche/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/quic_listeners/quiche/BUILD b/source/extensions/quic_listeners/quiche/BUILD index dde1927d81abc..59e2d45bfbc87 100644 --- a/source/extensions/quic_listeners/quiche/BUILD +++ b/source/extensions/quic_listeners/quiche/BUILD @@ -247,6 +247,7 @@ envoy_cc_library( ":envoy_quic_server_connection_lib", ":envoy_quic_server_session_lib", "//include/envoy/network:listener_interface", + "//source/common/http:utility_lib", "//source/server:connection_handler_lib", "@com_googlesource_quiche//:quic_core_server_lib", "@com_googlesource_quiche//:quic_core_utils_lib", From 380c24edfbfba760a6ecc6ce49a2d2dcabc3d576 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 04:47:35 +0000 Subject: [PATCH 10/20] gurl on windows please Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 40 +++++++++++++++- source/common/http/BUILD | 6 +-- source/common/http/url_utility.cc | 78 +------------------------------ source/common/http/url_utility.h | 7 +-- 4 files changed, 42 insertions(+), 89 deletions(-) diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index 681569e016bac..a3e174b123d22 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -1,5 +1,41 @@ +diff --git a/base/compiler_specific.h b/base/compiler_specific.h +index 2962537..ebdac08 100644 +--- a/base/compiler_specific.h ++++ b/base/compiler_specific.h +@@ -7,10 +7,6 @@ + + #include "build/build_config.h" + +-#if defined(COMPILER_MSVC) && !defined(__clang__) +-#error "Only clang-cl is supported on Windows, see https://crbug.com/988071" +-#endif +- + // Annotate a variable indicating it's ok if the variable is not used. + // (Typically used to silence a compiler warning when the assignment + // is important for some other reason.) +diff --git a/build_config.bzl b/build_config.bzl +index d5fca65..b1bc3a4 100644 +--- a/build_config.bzl ++++ b/build_config.bzl +@@ -1,7 +1,12 @@ +-_default_copts = [ +- "-std=c++14", +- "-fno-strict-aliasing", +-] ++_default_copts = select({ ++ "@envoy//bazel:windows_x86_64": [ ++ "/std:c++14" ++ ], ++ "//conditions:default": [ ++ "-std=c++14", ++ "-fno-strict-aliasing", ++ ] ++}) + + build_config = struct( + default_copts = _default_copts, diff --git a/url/BUILD b/url/BUILD -index 0126bdc..a11e973 100644 +index 0126bdc..3d1aeed 100644 --- a/url/BUILD +++ b/url/BUILD @@ -43,11 +43,11 @@ cc_library( @@ -12,6 +48,6 @@ index 0126bdc..a11e973 100644 "//base", "//base/strings", "//polyfills", -+ "@org_unicode_icuuc//:common" ++ "@org_unicode_icuuc//:common", ], ) diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 8ed8c137197e0..546673aebe094 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -367,10 +367,8 @@ envoy_cc_library( deps = [ "//source/common/common:assert_lib", "//source/common/common:empty_string", - ] + select({ - "//bazel:windows_x86_64": ["@com_github_nodejs_http_parser//:http_parser"], - "//conditions:default": ["@com_googlesource_googleurl//url:url"], - }), + "@com_googlesource_googleurl//url", + ], ) envoy_cc_library( diff --git a/source/common/http/url_utility.cc b/source/common/http/url_utility.cc index 9ccdb3280e29f..daebbf00a272c 100644 --- a/source/common/http/url_utility.cc +++ b/source/common/http/url_utility.cc @@ -4,28 +4,16 @@ #include "common/common/empty_string.h" #include "absl/strings/str_cat.h" - -#ifndef WIN32 #include "url/gurl.h" -#else -#include -#endif namespace Envoy { namespace Http { namespace Utility { -#ifndef WIN32 constexpr char COLON[] = ":"; constexpr char HASH[] = "#"; -#else -constexpr char DEFAULT_PATH[] = "/"; -constexpr char HTTP[] = "http"; -constexpr char HTTPS[] = "https"; -#endif bool Url::initialize(absl::string_view absolute_url) { -#ifndef WIN32 // TODO(dio): When we have access to base::StringPiece, probably we can convert absolute_url to // that instead. GURL parsed(std::string{absolute_url}); @@ -51,74 +39,10 @@ bool Url::initialize(absl::string_view absolute_url) { if (parsed.has_ref()) { absl::StrAppend(&path_and_query_params_, HASH, parsed.ref()); } -#else - struct http_parser_url u; - const bool is_connect = false; - http_parser_url_init(&u); - const int result = - http_parser_parse_url(absolute_url.data(), absolute_url.length(), is_connect, &u); - - if (result != 0) { - return false; - } - if ((u.field_set & (1 << UF_HOST)) != (1 << UF_HOST) && - (u.field_set & (1 << UF_SCHEMA)) != (1 << UF_SCHEMA)) { - return false; - } - scheme_ = absl::string_view(absolute_url.data() + u.field_data[UF_SCHEMA].off, - u.field_data[UF_SCHEMA].len); - - uint16_t authority_len = u.field_data[UF_HOST].len; - uint16_t authority_and_port_len = u.field_data[UF_HOST].len; - - bool has_port = (u.field_set & (1 << UF_PORT)) == (1 << UF_PORT); - if (has_port) { - int port = 0; - const bool converted_to_int = - absl::SimpleAtoi(absl::string_view(absolute_url.data() + u.field_data[UF_PORT].off, - u.field_data[UF_PORT].len), - &port); - ASSERT(converted_to_int); - ASSERT(port <= std::numeric_limits::max() && port >= 0); - port_ = static_cast(port); - authority_and_port_len = authority_and_port_len + u.field_data[UF_PORT].len + 1; - } else { - // When there is no port specified in the input URL, we set the port_ value by default ports of - // the known schemes. - if (scheme_ == HTTP) { - port_ = 80; - } else if (scheme_ == HTTPS) { - port_ = 443; - } else { - port_ = 0; - } - } - - const bool has_http_default_port = port_ == 80 && scheme_ == HTTP; - const bool has_https_default_port = port_ == 443 && scheme_ == HTTPS; - if (!has_http_default_port && !has_https_default_port) { - authority_len = authority_len + u.field_data[UF_PORT].len + 1; - } - host_and_port_ = - absl::string_view(absolute_url.data() + u.field_data[UF_HOST].off, authority_len); - - // RFC allows the absolute-uri to not end in "/", but the absolute path form must start with "/". - const uint64_t path_len = - absolute_url.length() - (u.field_data[UF_HOST].off + authority_and_port_len); - if (path_len > 0) { - uint64_t path_beginning = u.field_data[UF_HOST].off + authority_and_port_len; - const auto slash = absl::string_view(absolute_url.data() + path_beginning, 1); - path_and_query_params_ = - absl::StrCat(slash != DEFAULT_PATH ? DEFAULT_PATH : EMPTY_STRING, - absl::string_view(absolute_url.data() + path_beginning, path_len)); - } else { - path_and_query_params_ = DEFAULT_PATH; - } -#endif return true; } } // namespace Utility } // namespace Http -} // namespace Envoy \ No newline at end of file +} // namespace Envoy diff --git a/source/common/http/url_utility.h b/source/common/http/url_utility.h index 3a07e26657b0b..686731be3e9e9 100644 --- a/source/common/http/url_utility.h +++ b/source/common/http/url_utility.h @@ -20,13 +20,8 @@ class Url { uint64_t getPort() const { return port_; } private: -#ifndef WIN32 std::string scheme_; std::string host_and_port_; -#else - absl::string_view scheme_; - absl::string_view host_and_port_; -#endif std::string path_and_query_params_; uint16_t port_{0}; }; @@ -34,4 +29,4 @@ class Url { } // namespace Utility } // namespace Http -} // namespace Envoy \ No newline at end of file +} // namespace Envoy From 73e399dd8128ba3a5754865ea01d7c88ccdd1109 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 05:46:41 +0000 Subject: [PATCH 11/20] win: Try again Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index a3e174b123d22..a145b09d94b38 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -13,6 +13,18 @@ index 2962537..ebdac08 100644 // Annotate a variable indicating it's ok if the variable is not used. // (Typically used to silence a compiler warning when the assignment // is important for some other reason.) +diff --git a/base/strings/BUILD b/base/strings/BUILD +index 7a06170..5aa84b4 100644 +--- a/base/strings/BUILD ++++ b/base/strings/BUILD +@@ -20,6 +20,7 @@ cc_library( + "string_piece_forward.h", + "string_util.h", + "string_util_posix.h", ++ "string_util_win.h", + "utf_string_conversion_utils.h", + "utf_string_conversions.h", + ], diff --git a/build_config.bzl b/build_config.bzl index d5fca65..b1bc3a4 100644 --- a/build_config.bzl @@ -35,7 +47,7 @@ index d5fca65..b1bc3a4 100644 build_config = struct( default_copts = _default_copts, diff --git a/url/BUILD b/url/BUILD -index 0126bdc..3d1aeed 100644 +index 0126bdc..5d1a171 100644 --- a/url/BUILD +++ b/url/BUILD @@ -43,11 +43,11 @@ cc_library( @@ -48,6 +60,6 @@ index 0126bdc..3d1aeed 100644 "//base", "//base/strings", "//polyfills", -+ "@org_unicode_icuuc//:common", ++ "@org_unicode_icuuc//:common", ], ) From b27f6eae0b0729b0163df978e2b2d495849cfba1 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 07:17:27 +0000 Subject: [PATCH 12/20] string16 Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 49 +++++++++++++++++++++++++++------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index a145b09d94b38..d3a94518dd86a 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -14,38 +14,69 @@ index 2962537..ebdac08 100644 // (Typically used to silence a compiler warning when the assignment // is important for some other reason.) diff --git a/base/strings/BUILD b/base/strings/BUILD -index 7a06170..5aa84b4 100644 +index 7a06170..21e9b73 100644 --- a/base/strings/BUILD +++ b/base/strings/BUILD -@@ -20,6 +20,7 @@ cc_library( +@@ -6,23 +6,20 @@ load("//:build_config.bzl", "build_config") + cc_library( + name = "strings", + srcs = [ +- "string16.cc", + "string_piece.cc", + "string_util.cc", + "string_util_constants.cc", + "utf_string_conversion_utils.cc", + "utf_string_conversions.cc", +- ], ++ ] + build_config.strings_srcs, + hdrs = [ + "char_traits.h", +- "string16.h", + "string_piece.h", "string_piece_forward.h", "string_util.h", - "string_util_posix.h", -+ "string_util_win.h", +- "string_util_posix.h", "utf_string_conversion_utils.h", "utf_string_conversions.h", - ], +- ], ++ ] + build_config.strings_hdrs, + copts = build_config.default_copts, + visibility = ["//visibility:public"], + deps = [ diff --git a/build_config.bzl b/build_config.bzl -index d5fca65..b1bc3a4 100644 +index d5fca65..4e938e4 100644 --- a/build_config.bzl +++ b/build_config.bzl -@@ -1,7 +1,12 @@ +@@ -1,8 +1,25 @@ -_default_copts = [ - "-std=c++14", - "-fno-strict-aliasing", -] +_default_copts = select({ + "@envoy//bazel:windows_x86_64": [ -+ "/std:c++14" ++ "/std:c++14", + ], + "//conditions:default": [ + "-std=c++14", + "-fno-strict-aliasing", -+ ] ++ ], ++}) ++ ++_strings_srcs = select({ ++ "@envoy//bazel:windows_x86_64": [], ++ "//conditions:default": ["string16.cc"], ++}) ++ ++_strings_hdrs = select({ ++ "@envoy//bazel:windows_x86_64": ["string_util_win.h"], ++ "//conditions:default": ["string16.h", "string_util_posix.h"], +}) build_config = struct( default_copts = _default_copts, ++ strings_srcs = _strings_srcs, ++ strings_hdrs = _strings_hdrs, + ) diff --git a/url/BUILD b/url/BUILD index 0126bdc..5d1a171 100644 --- a/url/BUILD From 815063c450cd2a48c599aa91532aab272de95d63 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 08:06:13 +0000 Subject: [PATCH 13/20] Don't include string16.cc on windows Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index d3a94518dd86a..d2bb629008601 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -14,10 +14,10 @@ index 2962537..ebdac08 100644 // (Typically used to silence a compiler warning when the assignment // is important for some other reason.) diff --git a/base/strings/BUILD b/base/strings/BUILD -index 7a06170..21e9b73 100644 +index 7a06170..7c86a5f 100644 --- a/base/strings/BUILD +++ b/base/strings/BUILD -@@ -6,23 +6,20 @@ load("//:build_config.bzl", "build_config") +@@ -6,23 +6,21 @@ load("//:build_config.bzl", "build_config") cc_library( name = "strings", srcs = [ @@ -31,7 +31,7 @@ index 7a06170..21e9b73 100644 + ] + build_config.strings_srcs, hdrs = [ "char_traits.h", -- "string16.h", + "string16.h", "string_piece.h", "string_piece_forward.h", "string_util.h", @@ -44,7 +44,7 @@ index 7a06170..21e9b73 100644 visibility = ["//visibility:public"], deps = [ diff --git a/build_config.bzl b/build_config.bzl -index d5fca65..4e938e4 100644 +index d5fca65..fc0d7e5 100644 --- a/build_config.bzl +++ b/build_config.bzl @@ -1,8 +1,25 @@ @@ -69,7 +69,7 @@ index d5fca65..4e938e4 100644 + +_strings_hdrs = select({ + "@envoy//bazel:windows_x86_64": ["string_util_win.h"], -+ "//conditions:default": ["string16.h", "string_util_posix.h"], ++ "//conditions:default": ["string_util_posix.h"], +}) build_config = struct( From 2c1c2011da07a9e7061cd5b4eaaee8e01bb04fe8 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 09:32:36 +0000 Subject: [PATCH 14/20] Fix MSVC error C4067 Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index d2bb629008601..0c6280c327846 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -1,5 +1,5 @@ diff --git a/base/compiler_specific.h b/base/compiler_specific.h -index 2962537..ebdac08 100644 +index 2962537..6193b56 100644 --- a/base/compiler_specific.h +++ b/base/compiler_specific.h @@ -7,10 +7,6 @@ @@ -13,6 +13,26 @@ index 2962537..ebdac08 100644 // Annotate a variable indicating it's ok if the variable is not used. // (Typically used to silence a compiler warning when the assignment // is important for some other reason.) +@@ -212,7 +208,9 @@ + #endif + #endif + +-#if defined(__clang__) && __has_attribute(uninitialized) ++#if defined(__clang__) ++#if defined(__has_attribute) ++#if __has_attribute(uninitialized) + // Attribute "uninitialized" disables -ftrivial-auto-var-init=pattern for + // the specified variable. + // Library-wide alternative is +@@ -243,6 +241,8 @@ + // E.g. platform, bot, benchmark or test name in patch description or next to + // the attribute. + #define STACK_UNINITIALIZED __attribute__((uninitialized)) ++#endif ++#endif + #else + #define STACK_UNINITIALIZED + #endif diff --git a/base/strings/BUILD b/base/strings/BUILD index 7a06170..7c86a5f 100644 --- a/base/strings/BUILD From c3b01da0a08d9e2eabf3953d07925f694f09ad65 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 15:12:33 +0000 Subject: [PATCH 15/20] Use foreign_cc Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 4 ++-- bazel/foreign_cc/BUILD | 39 ++++++++++++++++++++++++++++++++++ bazel/foreign_cc/icuuc.patch | 17 +++++++++++++++ bazel/repositories.bzl | 10 +++++---- bazel/repository_locations.bzl | 2 +- source/common/http/BUILD | 4 +++- 6 files changed, 68 insertions(+), 8 deletions(-) create mode 100644 bazel/foreign_cc/icuuc.patch diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index 0c6280c327846..36fe86f9a98b5 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -98,7 +98,7 @@ index d5fca65..fc0d7e5 100644 + strings_hdrs = _strings_hdrs, ) diff --git a/url/BUILD b/url/BUILD -index 0126bdc..5d1a171 100644 +index 0126bdc..860f000 100644 --- a/url/BUILD +++ b/url/BUILD @@ -43,11 +43,11 @@ cc_library( @@ -111,6 +111,6 @@ index 0126bdc..5d1a171 100644 "//base", "//base/strings", "//polyfills", -+ "@org_unicode_icuuc//:common", ++ "@envoy//bazel/foreign_cc:icuuc" ], ) diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD index 24910612adf22..1cc7bc6ab313a 100644 --- a/bazel/foreign_cc/BUILD +++ b/bazel/foreign_cc/BUILD @@ -221,3 +221,42 @@ envoy_cmake_external( "//conditions:default": ["libz.a"], }), ) + +configure_make( + name = "icuuc", + configure_command = "source/configure", + configure_env_vars = select({ + "//bazel:windows_x86_64": { + "CXXFLAGS": "/utf-8 /DLOCALE_ALLOW_NEUTRAL_NAMES=0 /std:c++11", + }, + "//bazel:apple": { + "CXXFLAGS": "-Wno-shorten-64-to-32 -Wno-unused-variable -std=c++11", + }, + "//conditions:default": { + "CXXFLAGS": "-std=c++11 -DU_STATIC_IMPLEMENTATION -DU_HAVE_STD_ATOMICS", + }, + }), + configure_options = [ + "--enable-option-checking", + "--enable-static", + "--enable-tools", + "--disable-shared", + "--disable-dyload", + "--disable-extras", + "--disable-plugins", + "--disable-tests", + "--disable-samples", + "--with-data-packaging=static", + ], + lib_source = "@org_unicode_icuuc//:all", + static_libraries = select({ + "//bazel:windows_x86_64": [ + "libicuuc.lib", + "libicudata.lib", + ], + "//conditions:default": [ + "libicuuc.a", + "libicudata.a", + ], + }), +) diff --git a/bazel/foreign_cc/icuuc.patch b/bazel/foreign_cc/icuuc.patch new file mode 100644 index 0000000000000..a23bdcafc158e --- /dev/null +++ b/bazel/foreign_cc/icuuc.patch @@ -0,0 +1,17 @@ +--- source/icudefs.mk.in ++++ source/icudefs.mk.in +@@ -117,7 +117,7 @@ + CC = @CC@ + CXX = @CXX@ + AR = @AR@ +-ARFLAGS = @ARFLAGS@ r ++ARFLAGS = @ARFLAGS@ + RANLIB = @RANLIB@ + COMPILE_LINK_ENVVAR = @COMPILE_LINK_ENVVAR@ + UCLN_NO_AUTO_CLEANUP = @UCLN_NO_AUTO_CLEANUP@ + +--- source/config/mh-darwin ++++ source/config/mh-darwin +@@ -22,1 +22,1 @@ +-ARFLAGS += -c ++ARFLAGS += -crs diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index d0076ef120989..2742a3aa98151 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -229,14 +229,16 @@ def _com_github_cyan4973_xxhash(): ) def _org_unicode_icuuc(): - _repository_impl( + location = REPOSITORY_LOCATIONS["org_unicode_icuuc"] + http_archive( name = "org_unicode_icuuc", - build_file = "@envoy//bazel/external:icuuc.BUILD", - # TODO(dio): Consider patching udata when we need to embed some data. + build_file_content = BUILD_ALL_CONTENT, + patches = ["@envoy//bazel/foreign_cc:icuuc.patch"], + **location ) native.bind( name = "icuuc", - actual = "@org_unicode_icuuc//:common", + actual = "@envoy//bazel/foreign_cc:icuuc", ) def _com_github_envoyproxy_sqlparser(): diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 735ae1900d40a..49334caeac692 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -332,7 +332,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/dpkp/kafka-python/archive/2.0.0.tar.gz"], ), org_unicode_icuuc = dict( - strip_prefix = "icu-release-64-2", + strip_prefix = "icu-release-64-2/icu4c", sha256 = "524960ac99d086cdb6988d2a92fc163436fd3c6ec0a84c475c6382fbf989be05", urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"], ), diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 546673aebe094..026d495e1c70d 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -364,10 +364,12 @@ envoy_cc_library( name = "url_utility_lib", srcs = ["url_utility.cc"], hdrs = ["url_utility.h"], + external_deps = [ + "googleurl", + ], deps = [ "//source/common/common:assert_lib", "//source/common/common:empty_string", - "@com_googlesource_googleurl//url", ], ) From d802953b28ad2f4f726afa5ba8648c3306d9e9f2 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 15:24:39 +0000 Subject: [PATCH 16/20] Fix osx Signed-off-by: Dhi Aurrahman --- bazel/foreign_cc/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD index 1cc7bc6ab313a..8b5d7640a93df 100644 --- a/bazel/foreign_cc/BUILD +++ b/bazel/foreign_cc/BUILD @@ -231,6 +231,7 @@ configure_make( }, "//bazel:apple": { "CXXFLAGS": "-Wno-shorten-64-to-32 -Wno-unused-variable -std=c++11", + "AR": "/usr/bin/ar", }, "//conditions:default": { "CXXFLAGS": "-std=c++11 -DU_STATIC_IMPLEMENTATION -DU_HAVE_STD_ATOMICS", From 0154933806439b18574c3a030333db59f1691feb Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 15:41:14 +0000 Subject: [PATCH 17/20] mingw Signed-off-by: Dhi Aurrahman --- bazel/foreign_cc/BUILD | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD index 8b5d7640a93df..06180dd19e910 100644 --- a/bazel/foreign_cc/BUILD +++ b/bazel/foreign_cc/BUILD @@ -226,15 +226,12 @@ configure_make( name = "icuuc", configure_command = "source/configure", configure_env_vars = select({ - "//bazel:windows_x86_64": { - "CXXFLAGS": "/utf-8 /DLOCALE_ALLOW_NEUTRAL_NAMES=0 /std:c++11", - }, "//bazel:apple": { "CXXFLAGS": "-Wno-shorten-64-to-32 -Wno-unused-variable -std=c++11", "AR": "/usr/bin/ar", }, "//conditions:default": { - "CXXFLAGS": "-std=c++11 -DU_STATIC_IMPLEMENTATION -DU_HAVE_STD_ATOMICS", + "CXXFLAGS": "-std=c++11", }, }), configure_options = [ From 87f112fd9ce9d82eab6370eec2011c230c16313e Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 16:08:03 +0000 Subject: [PATCH 18/20] Faster build Signed-off-by: Dhi Aurrahman --- bazel/external/googleurl.patch | 4 ++-- bazel/foreign_cc/BUILD | 37 ---------------------------------- bazel/foreign_cc/icuuc.patch | 17 ---------------- bazel/repositories.bzl | 10 ++++----- bazel/repository_locations.bzl | 2 +- 5 files changed, 7 insertions(+), 63 deletions(-) delete mode 100644 bazel/foreign_cc/icuuc.patch diff --git a/bazel/external/googleurl.patch b/bazel/external/googleurl.patch index 36fe86f9a98b5..0c6280c327846 100644 --- a/bazel/external/googleurl.patch +++ b/bazel/external/googleurl.patch @@ -98,7 +98,7 @@ index d5fca65..fc0d7e5 100644 + strings_hdrs = _strings_hdrs, ) diff --git a/url/BUILD b/url/BUILD -index 0126bdc..860f000 100644 +index 0126bdc..5d1a171 100644 --- a/url/BUILD +++ b/url/BUILD @@ -43,11 +43,11 @@ cc_library( @@ -111,6 +111,6 @@ index 0126bdc..860f000 100644 "//base", "//base/strings", "//polyfills", -+ "@envoy//bazel/foreign_cc:icuuc" ++ "@org_unicode_icuuc//:common", ], ) diff --git a/bazel/foreign_cc/BUILD b/bazel/foreign_cc/BUILD index 06180dd19e910..24910612adf22 100644 --- a/bazel/foreign_cc/BUILD +++ b/bazel/foreign_cc/BUILD @@ -221,40 +221,3 @@ envoy_cmake_external( "//conditions:default": ["libz.a"], }), ) - -configure_make( - name = "icuuc", - configure_command = "source/configure", - configure_env_vars = select({ - "//bazel:apple": { - "CXXFLAGS": "-Wno-shorten-64-to-32 -Wno-unused-variable -std=c++11", - "AR": "/usr/bin/ar", - }, - "//conditions:default": { - "CXXFLAGS": "-std=c++11", - }, - }), - configure_options = [ - "--enable-option-checking", - "--enable-static", - "--enable-tools", - "--disable-shared", - "--disable-dyload", - "--disable-extras", - "--disable-plugins", - "--disable-tests", - "--disable-samples", - "--with-data-packaging=static", - ], - lib_source = "@org_unicode_icuuc//:all", - static_libraries = select({ - "//bazel:windows_x86_64": [ - "libicuuc.lib", - "libicudata.lib", - ], - "//conditions:default": [ - "libicuuc.a", - "libicudata.a", - ], - }), -) diff --git a/bazel/foreign_cc/icuuc.patch b/bazel/foreign_cc/icuuc.patch deleted file mode 100644 index a23bdcafc158e..0000000000000 --- a/bazel/foreign_cc/icuuc.patch +++ /dev/null @@ -1,17 +0,0 @@ ---- source/icudefs.mk.in -+++ source/icudefs.mk.in -@@ -117,7 +117,7 @@ - CC = @CC@ - CXX = @CXX@ - AR = @AR@ --ARFLAGS = @ARFLAGS@ r -+ARFLAGS = @ARFLAGS@ - RANLIB = @RANLIB@ - COMPILE_LINK_ENVVAR = @COMPILE_LINK_ENVVAR@ - UCLN_NO_AUTO_CLEANUP = @UCLN_NO_AUTO_CLEANUP@ - ---- source/config/mh-darwin -+++ source/config/mh-darwin -@@ -22,1 +22,1 @@ --ARFLAGS += -c -+ARFLAGS += -crs diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 2742a3aa98151..d0076ef120989 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -229,16 +229,14 @@ def _com_github_cyan4973_xxhash(): ) def _org_unicode_icuuc(): - location = REPOSITORY_LOCATIONS["org_unicode_icuuc"] - http_archive( + _repository_impl( name = "org_unicode_icuuc", - build_file_content = BUILD_ALL_CONTENT, - patches = ["@envoy//bazel/foreign_cc:icuuc.patch"], - **location + build_file = "@envoy//bazel/external:icuuc.BUILD", + # TODO(dio): Consider patching udata when we need to embed some data. ) native.bind( name = "icuuc", - actual = "@envoy//bazel/foreign_cc:icuuc", + actual = "@org_unicode_icuuc//:common", ) def _com_github_envoyproxy_sqlparser(): diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 49334caeac692..735ae1900d40a 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -332,7 +332,7 @@ REPOSITORY_LOCATIONS = dict( urls = ["https://github.com/dpkp/kafka-python/archive/2.0.0.tar.gz"], ), org_unicode_icuuc = dict( - strip_prefix = "icu-release-64-2/icu4c", + strip_prefix = "icu-release-64-2", sha256 = "524960ac99d086cdb6988d2a92fc163436fd3c6ec0a84c475c6382fbf989be05", urls = ["https://github.com/unicode-org/icu/archive/release-64-2.tar.gz"], ), From 6e190bc4b1de91394c1e5b6c99f68745abe69342 Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Mon, 6 Apr 2020 16:12:48 +0000 Subject: [PATCH 19/20] CLeanup Signed-off-by: Dhi Aurrahman --- bazel/external/icuuc.BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/external/icuuc.BUILD b/bazel/external/icuuc.BUILD index 315f6fea892b1..d7754da502e54 100644 --- a/bazel/external/icuuc.BUILD +++ b/bazel/external/icuuc.BUILD @@ -8,7 +8,7 @@ exports_files([ icuuc_copts = [ "-DU_STATIC_IMPLEMENTATION", "-DU_COMMON_IMPLEMENTATION", - "-DU_HAVE_STD_ATOMICS", # TODO(gunan): Remove when TF is on ICU 64+. + "-DU_HAVE_STD_ATOMICS", ] + select({ "@envoy//bazel:apple": [ "-Wno-shorten-64-to-32", From 747bcc891c7e59f3c4cc77b04dd4b7d096c4203f Mon Sep 17 00:00:00 2001 From: Dhi Aurrahman Date: Tue, 7 Apr 2020 10:02:47 +0000 Subject: [PATCH 20/20] Update Signed-off-by: Dhi Aurrahman --- test/common/http/utility_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 9ad50cbca0049..17a7c3961733e 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1169,12 +1169,14 @@ TEST(PercentEncoding, EncodeDecode) { " \"message\": \"Unauthorized\"%0A }%0A}%0A "); validatePercentEncodingEncodeDecode("too large", "too large"); validatePercentEncodingEncodeDecode("_-ok-_", "_-ok-_"); + validatePercentEncodingEncodeDecode("~-ok-~", "%7E-ok-%7E"); } TEST(PercentEncoding, Trailing) { EXPECT_EQ(Utility::PercentEncoding::decode("too%20lar%20"), "too lar "); EXPECT_EQ(Utility::PercentEncoding::decode("too%20larg%e"), "too larg%e"); EXPECT_EQ(Utility::PercentEncoding::decode("too%20large%"), "too large%"); + EXPECT_EQ(Utility::PercentEncoding::decode("%ok"), "\x94"); } } // namespace Http