impl(rest): dedup and merge headers right before writing them to curl (#15100)

Author: scotthart

google/cloud/internal/ci/run_integration_tests_emulator_bazel.sh

@@ -30,10 +30,6 @@ shift
 BAZEL_VERB="$1"
 shift
 
-if bazel::has_no_tests "//google/cloud:all"; then
-  exit 0
-fi
-
 # Separate caller-provided excluded targets (starting with "-//..."), so that
 # we can make sure those appear on the command line after `--`.
 bazel_test_args=()

google/cloud/internal/curl_impl.cc

@@ -237,29 +237,70 @@ CurlImpl::~CurlImpl() {
   factory_->CleanupMultiHandle(std::move(multi_), HandleDisposition::kKeep);
 }
 
-void CurlImpl::SetHeader(std::string const& header) {
+void CurlImpl::SetHeader(HttpHeader header) {
   if (header.empty()) return;
 
   // The API for credentials is complicated, and the authorization
-  // header can be empty. See, for example, AnonymousCredentials.
-  if (header == "authorization: ") return;
+  // header can be empty, but we don't actually want to send it. See, for
+  // example, AnonymousCredentials.
+  if (header.IsSameKey("authorization") && header.EmptyValues()) {
+    return;
+  }
+
+  pending_request_headers_.push_back(std::move(header));
+}
 
+void CurlImpl::WriteHeader(std::string const& header) {
   auto* headers = curl_slist_append(request_headers_.get(), header.c_str());
   (void)request_headers_.release();  // Now owned by list, not us.
   request_headers_.reset(headers);
 }
 
-void CurlImpl::SetHeader(std::pair<std::string, std::string> const& header) {
-  SetHeader(absl::StrCat(header.first, ": ", header.second));
-}
+void CurlImpl::MergeAndWriteHeaders(
+    std::function<void(HttpHeader const&)> const& write_fn) {
+  // There are some headers that we do not want to merge. These headers
+  // could have been added via custom user headers or due to bugs in the SDK.
+  static constexpr std::array<char const*, 2> kDoNotMergeHeaderKeys{
+      "authorization", "content-length"};
+  std::stable_sort(pending_request_headers_.begin(),
+                   pending_request_headers_.end());
+
+  auto current = pending_request_headers_.begin();
+  while (current != pending_request_headers_.end()) {
+    // If this is the last header, write it and stop.
+    if (current + 1 == pending_request_headers_.end()) {
+      write_fn(*current);
+      break;
+    }
 
-void CurlImpl::SetHeaders(RestContext const& context,
-                          RestRequest const& request) {
-  for (auto const& header : context.headers()) {
-    SetHeader(std::make_pair(header.first, absl::StrJoin(header.second, ",")));
+    // Look ahead to see if there is another header with the same key. If so,
+    // merge its values into this header until all headers of the same key are
+    // merged.
+    auto next = current + 1;
+    while (next != pending_request_headers_.end() &&
+           current->IsSameKey(*next)) {
+      if (internal::ContainsIf(
+              kDoNotMergeHeaderKeys,
+              [&current](char const* h) { return current->IsSameKey(h); })) {
+        GCP_LOG(WARNING) << "Ignoring duplicate header: "
+                         << next->DebugString();
+      } else {
+        current->MergeHeader(*next);
+      }
+      ++next;
+    }
+
+    // Write the merged (or not merged) header and proceed to the next header
+    // key.
+    write_fn(*current);
+    current = next;
   }
-  for (auto const& header : request.headers()) {
-    SetHeader(std::make_pair(header.first, absl::StrJoin(header.second, ",")));
+}
+
+void CurlImpl::SetHeaders(
+    std::unordered_map<std::string, std::vector<std::string>> const& headers) {
+  for (auto const& header : headers) {
+    SetHeader(HttpHeader(header.first, header.second));
   }
 }
 
@@ -427,7 +468,12 @@ Status CurlImpl::MakeRequest(HttpMethod method, RestContext& context,
     if (!status.ok()) return OnTransferError(context, std::move(status));
     status = handle_.SetOption(CURLOPT_SEEKDATA, &writev_);
     if (!status.ok()) return OnTransferError(context, std::move(status));
-    SetHeader("Expect:");
+    // Adding an empty Expect header instructs libcurl never to attempt to
+    // insert an "Expect: 100-continue" header per its own logic. When libcurl
+    // adds this header/value pair it introduces another round trip before data
+    // is sent. Additionally, some webservers do not handle the 100-continue
+    // correctly causing the request to fail.
+    SetHeader(HttpHeader("Expect"));
     return MakeRequestImpl(context);
   }
 
@@ -538,6 +584,8 @@ Status CurlImpl::MakeRequestImpl(RestContext& context) {
   Status status;
   status = handle_.SetOption(CURLOPT_URL, url_.c_str());
   if (!status.ok()) return OnTransferError(context, std::move(status));
+  MergeAndWriteHeaders(
+      [this](HttpHeader const& h) { WriteHeader(std::string(h)); });
   status = handle_.SetOption(CURLOPT_HTTPHEADER, request_headers_.get());
   if (!status.ok()) return OnTransferError(context, std::move(status));
   status = handle_.SetOption(CURLOPT_USERAGENT, user_agent_.c_str());

google/cloud/internal/curl_impl.h

@@ -19,6 +19,7 @@
 #include "google/cloud/internal/curl_handle_factory.h"
 #include "google/cloud/internal/curl_wrappers.h"
 #include "google/cloud/internal/curl_writev.h"
+#include "google/cloud/internal/http_header.h"
 #include "google/cloud/internal/rest_context.h"
 #include "google/cloud/internal/rest_request.h"
 #include "google/cloud/internal/rest_response.h"
@@ -82,9 +83,9 @@ class CurlImpl {
   CurlImpl& operator=(CurlImpl const&) = delete;
   CurlImpl& operator=(CurlImpl&&) = default;
 
-  void SetHeader(std::string const& header);
-  void SetHeader(std::pair<std::string, std::string> const& header);
-  void SetHeaders(RestContext const& context, RestRequest const& request);
+  void SetHeader(HttpHeader header);
+  void SetHeaders(
+      std::unordered_map<std::string, std::vector<std::string>> const& headers);
 
   std::string MakeEscapedString(std::string const& s);
 
@@ -108,11 +109,20 @@ class CurlImpl {
   std::size_t WriteCallback(absl::Span<char> response);
   std::size_t HeaderCallback(absl::Span<char> response);
 
+  // Traverses the accrued headers added via SetHeader(s), merges the values of
+  // headers with like keys and writes them via the write_fn.
+  // The write_fn is provided for testing purposes. When used internally, the
+  // write_fn writes to libcurl buffers.
+  void MergeAndWriteHeaders(
+      std::function<void(HttpHeader const&)> const& write_fn);
+
  private:
   class ReadFunctionAbortGuard;
   Status MakeRequestImpl(RestContext& context);
   StatusOr<std::size_t> ReadImpl(RestContext& context, absl::Span<char> output);
 
+  void WriteHeader(std::string const& header);
+
   // Cleanup the CURL handles, leaving them ready for reuse.
   void CleanupHandles();
   // Perform at least part of the request.
@@ -126,6 +136,7 @@ class CurlImpl {
   void OnTransferDone();
 
   std::shared_ptr<CurlHandleFactory> factory_;
+  std::vector<HttpHeader> pending_request_headers_;
   CurlHeaders request_headers_;
   CurlHandle handle_;
   CurlMulti multi_;

google/cloud/internal/curl_impl_test.cc

@@ -15,7 +15,6 @@
 #include "google/cloud/internal/curl_impl.h"
 #include "google/cloud/common_options.h"
 #include "google/cloud/rest_options.h"
-#include "google/cloud/testing_util/status_matchers.h"
 #include <gmock/gmock.h>
 #include <vector>
 
@@ -189,6 +188,210 @@ TEST_F(CurlImplTest, CurlOptInterface) {
             absl::make_optional(std::string("interface")));
 }
 
+TEST_F(CurlImplTest, MergeAndWriteHeadersEmpty) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  impl.MergeAndWriteHeaders(write_fn);
+  EXPECT_THAT(headers_written, ElementsAre());
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersOneEntry) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader h("my-key", "my-value");
+  impl.SetHeader(h);
+  impl.MergeAndWriteHeaders(write_fn);
+  EXPECT_THAT(headers_written, ElementsAre(std::string(h)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersTwoUniqueEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader h("my-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader h2("my-key2", "my-value2");
+  impl.SetHeader(h2);
+  impl.MergeAndWriteHeaders(write_fn);
+  EXPECT_THAT(headers_written, ElementsAre(std::string(h), std::string(h2)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersMoreThanTwoUniqueEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader h("my-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader h2("my-key2", "my-value2");
+  impl.SetHeader(h2);
+  HttpHeader h3("my-key3", "my-value3");
+  impl.SetHeader(h3);
+  impl.MergeAndWriteHeaders(write_fn);
+  EXPECT_THAT(headers_written,
+              ElementsAre(std::string(h), std::string(h2), std::string(h3)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersTwoDuplicateEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader h("my-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader h2("my-key", "my-value2");
+  impl.SetHeader(h2);
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader expected("my-key", "my-value,my-value2");
+  EXPECT_THAT(headers_written, ElementsAre(std::string(expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersMoreThanTwoDuplicateEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader h("my-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader h2("my-key", "my-value2");
+  impl.SetHeader(h2);
+  HttpHeader h3("my-key", "my-value3");
+  impl.SetHeader(h3);
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader expected("my-key", "my-value,my-value2,my-value3");
+  EXPECT_THAT(headers_written, ElementsAre(std::string(expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersBeginningDuplicateEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader i("i-key", "value");
+  impl.SetHeader(i);
+  HttpHeader h("h-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader k("k-key", "my-value");
+  impl.SetHeader(k);
+  HttpHeader h2("h-key", "my-value2");
+  impl.SetHeader(h2);
+  HttpHeader h3("h-key", "my-value3");
+  impl.SetHeader(h3);
+
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader i_expected("i-key", "value");
+  HttpHeader h_expected("h-key", "my-value,my-value2,my-value3");
+  HttpHeader k_expected("k-key", "my-value");
+  EXPECT_THAT(headers_written,
+              ElementsAre(std::string(h_expected), std::string(i_expected),
+                          std::string(k_expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersMiddleDuplicateEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader i("i-key", "value");
+  impl.SetHeader(i);
+  HttpHeader h("h-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader k("k-key", "my-value");
+  impl.SetHeader(k);
+  HttpHeader i2("i-key", "my-value2");
+  impl.SetHeader(i2);
+  HttpHeader i3("i-key", "my-value3");
+  impl.SetHeader(i3);
+
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader i_expected("i-key", "value,my-value2,my-value3");
+  HttpHeader h_expected("h-key", "my-value");
+  HttpHeader k_expected("k-key", "my-value");
+  EXPECT_THAT(headers_written,
+              ElementsAre(std::string(h_expected), std::string(i_expected),
+                          std::string(k_expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersEndingDuplicateEntries) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader i("i-key", "value");
+  impl.SetHeader(i);
+  HttpHeader h("h-key", "my-value");
+  impl.SetHeader(h);
+  HttpHeader k("k-key", "my-value");
+  impl.SetHeader(k);
+  HttpHeader k2("k-key", "my-value2");
+  impl.SetHeader(k2);
+  HttpHeader k3("k-key", "my-value3");
+  impl.SetHeader(k3);
+
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader i_expected("i-key", "value");
+  HttpHeader h_expected("h-key", "my-value");
+  HttpHeader k_expected("k-key", "my-value,my-value2,my-value3");
+  EXPECT_THAT(headers_written,
+              ElementsAre(std::string(h_expected), std::string(i_expected),
+                          std::string(k_expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersDoNotMergeAuthorization) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader auth("Authorization", "Bearer my-token");
+  impl.SetHeader(auth);
+  HttpHeader auth2("Authorization", "Bearer my-token");
+  impl.SetHeader(auth2);
+  HttpHeader auth3("authorization", "Bearer my-token");
+  impl.SetHeader(auth3);
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader expected("Authorization", "Bearer my-token");
+  EXPECT_THAT(headers_written, ElementsAre(std::string(expected)));
+}
+
+TEST_F(CurlImplTest, MergeAndWriteHeadersDoNotMergeContentLength) {
+  std::vector<std::string> headers_written;
+  auto write_fn = [&headers_written](HttpHeader const& header) {
+    headers_written.push_back(std::string(header));
+  };
+
+  auto impl = CurlImpl(std::move(handle_), factory_, {});
+  HttpHeader auth("content-length", "42");
+  impl.SetHeader(auth);
+  HttpHeader auth2("Content-Length", "43");
+  impl.SetHeader(auth2);
+  HttpHeader auth3("content-length", "44");
+  impl.SetHeader(auth3);
+  impl.MergeAndWriteHeaders(write_fn);
+  HttpHeader expected("content-length", "42");
+  EXPECT_THAT(headers_written, ElementsAre(std::string(expected)));
+}
+
 }  // namespace
 GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
 }  // namespace rest_internal