SERVER-47133 Refactor HttpClient to allow returning on error

6 files changed, 154 insertions, 88 deletions

diff --git a/jstests/noPassthrough/http_client_keep_alive.js b/jstests/noPassthrough/http_client_keep_alive.js
index 689231dbe03..6af354aa7a9 100644
--- a/jstests/noPassthrough/http_client_keep_alive.js
+++ b/jstests/noPassthrough/http_client_keep_alive.js
@@ -49,6 +49,11 @@ function runTest(mongod, web) {
         admin.runCommand({httpClientRequest: 1, uri: web.getURL() + '/connect_count'});
     const connects = assert.commandWorked(connectsCmd).body;
     assert.eq(connects, 2, "Connection count incorrect.");
+
+    // Check 404 returns failure.
+    const failedCmd = assert.commandWorked(
+        admin.runCommand({httpClientRequest: 1, uri: web.getURL() + '/no_such_path'}));
+    assert.eq(failedCmd.code, 404);
 }
 
 const web = new ConfigExpandRestServer();
diff --git a/src/mongo/db/commands/http_client.cpp b/src/mongo/db/commands/http_client.cpp
index 3448c7a3cb7..ceda5650ee2 100644
--- a/src/mongo/db/commands/http_client.cpp
+++ b/src/mongo/db/commands/http_client.cpp
@@ -38,6 +38,7 @@
 #include "mongo/db/commands/http_client_gen.h"
 #include "mongo/db/commands/test_commands_enabled.h"
 #include "mongo/db/operation_context.h"
+#include "mongo/logv2/log.h"
 #include "mongo/util/net/hostandport.h"
 #include "mongo/util/net/http_client.h"
 
@@ -76,6 +77,14 @@ bool isLocalhostURI(StringData uri) {
     return hp.isLocalHost();
 }
 
+std::string stringFromDataBuilder(DataBuilder& builder) {
+    const auto sz = builder.size();
+    std::string ret;
+    ret.resize(sz);
+    std::copy_n(builder.release().get(), sz, &ret[0]);
+    return ret;
+}
+
 class CmdHttpClient final : public TypedCommand<CmdHttpClient> {
 public:
     using Request = HttpClientRequest;
@@ -104,15 +113,12 @@ public:
                 client->setTimeout(Seconds(timeoutSecs.get()));
             }
 
-            auto ret = client->get(uri);
-            const auto sz = ret.size();
-
-            std::string output;
-            output.resize(sz);
-            std::copy_n(ret.release().get(), sz, &output[0]);
+            auto ret = client->request(HttpClient::HttpMethod::kGET, uri, {nullptr, 0});
 
             Reply reply;
-            reply.setBody(std::move(output));
+            reply.setCode(ret.code);
+            reply.setHeader(stringFromDataBuilder(ret.header));
+            reply.setBody(stringFromDataBuilder(ret.body));
             return reply;
         }
 
diff --git a/src/mongo/db/commands/http_client.idl b/src/mongo/db/commands/http_client.idl
index 139e9f5dc02..9395fa839ba 100644
--- a/src/mongo/db/commands/http_client.idl
+++ b/src/mongo/db/commands/http_client.idl
@@ -36,6 +36,12 @@ structs:
     httpClientReply:
         description: "Result of an HTTP(S) GET request"
         fields:
+            code:
+                description: "Status code returned by server"
+                type: int
+            header:
+                description: "Headers returned by server"
+                type: string
             body:
                 description: "Content returned by server"
                 type: string
diff --git a/src/mongo/util/net/http_client.h b/src/mongo/util/net/http_client.h
index f6dea4b4ab5..5c0a1916918 100644
--- a/src/mongo/util/net/http_client.h
+++ b/src/mongo/util/net/http_client.h
@@ -49,6 +49,21 @@ constexpr Seconds kTotalRequestTimeout{120};
  */
 class HttpClient {
 public:
+    enum class HttpMethod {
+        kGET,
+        kPOST,
+        kPUT,
+    };
+
+    struct HttpReply {
+        std::uint16_t code;
+        DataBuilder header;
+        DataBuilder body;
+
+        HttpReply(std::uint16_t code_, DataBuilder&& header_, DataBuilder&& body_)
+            : code(code_), header(std::move(header_)), body(std::move(body_)) {}
+    };
+
     virtual ~HttpClient() = default;
 
     /**
@@ -75,19 +90,33 @@ public:
     virtual void setTimeout(Seconds timeout) = 0;
 
     /**
-     * Perform a POST request to specified URL.
+     * Perform a request to specified URL.
+     * Note that some methods (GET, HEAD) prohibit context bodies.
      */
-    virtual DataBuilder post(StringData url, ConstDataRange data) const = 0;
+    virtual HttpReply request(HttpMethod method,
+                              StringData url,
+                              ConstDataRange data = {nullptr, 0}) const = 0;
 
     /**
-     * Perform a PUT request to specified URL.
+     * Convenience wrapper to perform a POST and require a 200 response.
      */
-    virtual DataBuilder put(StringData url, ConstDataRange data) const = 0;
+    DataBuilder post(StringData url, ConstDataRange data) {
+        return requestSuccess(HttpMethod::kPOST, url, data);
+    }
 
     /**
-     * Perform a GET request from the specified URL.
+     * Convenience wrapper to perform a PUT and require a 200 response.
      */
-    virtual DataBuilder get(StringData url) const = 0;
+    DataBuilder put(StringData url, ConstDataRange data) {
+        return requestSuccess(HttpMethod::kPUT, url, data);
+    }
+
+    /**
+     * Convenience wrapper to perform a GET and require a 200 response.
+     */
+    DataBuilder get(StringData url) {
+        return requestSuccess(HttpMethod::kGET, url, {nullptr, 0});
+    }
 
     /**
      * Factory method provided by client implementation.
@@ -98,6 +127,15 @@ public:
      * Content for ServerStatus http_client section.
      */
     static BSONObj getServerStatus();
+
+private:
+    DataBuilder requestSuccess(HttpMethod method, StringData url, ConstDataRange data) const {
+        auto reply = request(method, url, data);
+        uassert(ErrorCodes::OperationFailed,
+                str::stream() << "Unexpected http status code from server: " << reply.code,
+                reply.code == 200);
+        return std::move(reply.body);
+    }
 };
 
 }  // namespace mongo
diff --git a/src/mongo/util/net/http_client_curl.cpp b/src/mongo/util/net/http_client_curl.cpp
index 853b9699552..f0b15bf5bc6 100644
--- a/src/mongo/util/net/http_client_curl.cpp
+++ b/src/mongo/util/net/http_client_curl.cpp
@@ -226,6 +226,7 @@ public:
         curl_easy_setopt(_handle.get(), CURLOPT_SSLVERSION, CURL_SSLVERSION_TLSv1_2);
 #endif
         curl_easy_setopt(_handle.get(), CURLOPT_WRITEFUNCTION, WriteMemoryCallback);
+        curl_easy_setopt(_handle.get(), CURLOPT_HEADERFUNCTION, WriteMemoryCallback);
 
         // TODO: CURLOPT_EXPECT_100_TIMEOUT_MS?
         // TODO: consider making this configurable
@@ -257,88 +258,78 @@ public:
         curl_easy_setopt(_handle.get(), CURLOPT_CONNECTTIMEOUT, longSeconds(timeout));
     }
 
-    DataBuilder get(StringData url) const final {
-        // Make a local copy of the base handle for this request.
+    HttpReply request(HttpMethod method,
+                      StringData url,
+                      ConstDataRange cdr = {nullptr, 0}) const final {
         CurlHandle myHandle(curl_easy_duphandle(_handle.get()));
         uassert(ErrorCodes::InternalError, "Curl initialization failed", myHandle);
 
-        return doRequest(myHandle.get(), url);
-    }
-
-    DataBuilder post(StringData url, ConstDataRange cdr) const final {
-        // Make a local copy of the base handle for this request.
-        CurlHandle myHandle(curl_easy_duphandle(_handle.get()));
-        uassert(ErrorCodes::InternalError, "Curl initialization failed", myHandle);
-
-        curl_easy_setopt(myHandle.get(), CURLOPT_POST, 1);
-
-        ConstDataRangeCursor cdrc(cdr);
-        curl_easy_setopt(myHandle.get(), CURLOPT_READFUNCTION, ReadMemoryCallback);
-        curl_easy_setopt(myHandle.get(), CURLOPT_READDATA, &cdrc);
-        curl_easy_setopt(myHandle.get(), CURLOPT_POSTFIELDSIZE, (long)cdrc.length());
-
-        return doRequest(myHandle.get(), url);
-    }
-
-    DataBuilder put(StringData url, ConstDataRange cdr) const final {
-        // Make a local copy of the base handle for this request.
-        CurlHandle myHandle(curl_easy_duphandle(_handle.get()));
-        uassert(ErrorCodes::InternalError, "Curl initialization failed", myHandle);
-
-        curl_easy_setopt(myHandle.get(), CURLOPT_PUT, 1);
-
         ConstDataRangeCursor cdrc(cdr);
-        curl_easy_setopt(myHandle.get(), CURLOPT_READFUNCTION, ReadMemoryCallback);
-        curl_easy_setopt(myHandle.get(), CURLOPT_READDATA, &cdrc);
-        curl_easy_setopt(myHandle.get(), CURLOPT_INFILESIZE_LARGE, (long)cdrc.length());
-
-        return doRequest(myHandle.get(), url);
-    }
-
-private:
-    /**
-     * Helper for use with curl_easy_setopt which takes a vararg list,
-     * and expects a long, not the long long durationCount() returns.
-     */
-    long longSeconds(Seconds tm) {
-        return static_cast<long>(durationCount<Seconds>(tm));
-    }
+        switch (method) {
+            case HttpMethod::kGET:
+                uassert(ErrorCodes::BadValue,
+                        "Request body not permitted with GET requests",
+                        cdr.length() == 0);
+                break;
+            case HttpMethod::kPOST:
+                curl_easy_setopt(myHandle.get(), CURLOPT_POST, 1);
+
+                curl_easy_setopt(myHandle.get(), CURLOPT_READFUNCTION, ReadMemoryCallback);
+                curl_easy_setopt(myHandle.get(), CURLOPT_READDATA, &cdrc);
+                curl_easy_setopt(myHandle.get(), CURLOPT_POSTFIELDSIZE, (long)cdrc.length());
+                break;
+            case HttpMethod::kPUT:
+                curl_easy_setopt(myHandle.get(), CURLOPT_PUT, 1);
+
+                curl_easy_setopt(myHandle.get(), CURLOPT_READFUNCTION, ReadMemoryCallback);
+                curl_easy_setopt(myHandle.get(), CURLOPT_READDATA, &cdrc);
+                curl_easy_setopt(myHandle.get(), CURLOPT_INFILESIZE_LARGE, (long)cdrc.length());
+                break;
+            default:
+                MONGO_UNREACHABLE;
+        }
 
-    DataBuilder doRequest(CURL* handle, StringData url) const {
         const auto urlString = url.toString();
-        curl_easy_setopt(handle, CURLOPT_URL, urlString.c_str());
-        curl_easy_setopt(handle, CURLOPT_SHARE, curlLibraryManager.getShareHandle());
+        curl_easy_setopt(myHandle.get(), CURLOPT_URL, urlString.c_str());
+        curl_easy_setopt(myHandle.get(), CURLOPT_SHARE, curlLibraryManager.getShareHandle());
 
-        DataBuilder dataBuilder(4096);
-        curl_easy_setopt(handle, CURLOPT_WRITEDATA, &dataBuilder);
+        DataBuilder dataBuilder(4096), headerBuilder(4096);
+        curl_easy_setopt(myHandle.get(), CURLOPT_WRITEDATA, &dataBuilder);
+        curl_easy_setopt(myHandle.get(), CURLOPT_HEADERDATA, &headerBuilder);
 
         curl_slist* chunk = curl_slist_append(nullptr, "Connection: keep-alive");
         for (const auto& header : _headers) {
             chunk = curl_slist_append(chunk, header.c_str());
         }
-        curl_easy_setopt(handle, CURLOPT_HTTPHEADER, chunk);
+        curl_easy_setopt(myHandle.get(), CURLOPT_HTTPHEADER, chunk);
         CurlSlist _headers(chunk);
 
-        CURLcode result = curl_easy_perform(handle);
+        CURLcode result = curl_easy_perform(myHandle.get());
         uassert(ErrorCodes::OperationFailed,
                 str::stream() << "Bad HTTP response from API server: "
                               << curl_easy_strerror(result),
                 result == CURLE_OK);
 
         long statusCode;
-        result = curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &statusCode);
+        result = curl_easy_getinfo(myHandle.get(), CURLINFO_RESPONSE_CODE, &statusCode);
         uassert(ErrorCodes::OperationFailed,
                 str::stream() << "Unexpected error retrieving response: "
                               << curl_easy_strerror(result),
                 result == CURLE_OK);
 
-        uassert(ErrorCodes::OperationFailed,
-                str::stream() << "Unexpected http status code from server: " << statusCode,
-                statusCode == 200);
+        return HttpReply(statusCode, std::move(headerBuilder), std::move(dataBuilder));
+    }
 
-        return dataBuilder;
+private:
+    /**
+     * Helper for use with curl_easy_setopt which takes a vararg list,
+     * and expects a long, not the long long durationCount() returns.
+     */
+    long longSeconds(Seconds tm) {
+        return static_cast<long>(durationCount<Seconds>(tm));
     }
 
+
 private:
     CurlHandle _handle;
     std::vector<std::string> _headers;
diff --git a/src/mongo/util/net/http_client_winhttp.cpp b/src/mongo/util/net/http_client_winhttp.cpp
index 4401059d23e..db80bcf2150 100644
--- a/src/mongo/util/net/http_client_winhttp.cpp
+++ b/src/mongo/util/net/http_client_winhttp.cpp
@@ -157,22 +157,26 @@ public:
         _timeout = timeout;
     }
 
-    DataBuilder post(StringData url, ConstDataRange cdr) const final {
-        return doRequest(
-            L"POST", url, const_cast<void*>(static_cast<const void*>(cdr.data())), cdr.length());
-    }
-
-    DataBuilder put(StringData url, ConstDataRange cdr) const final {
-        return doRequest(
-            L"PUT", url, const_cast<void*>(static_cast<const void*>(cdr.data())), cdr.length());
-    }
-
-    DataBuilder get(StringData url) const final {
-        return doRequest(L"GET", url, nullptr, 0);
-    }
+    HttpReply request(HttpMethod methodType, StringData urlSD, ConstDataRange cdrData) const final {
+        LPCWSTR method = L"GET";
+        LPVOID data = const_cast<void*>(static_cast<const void*>(cdrData.data()));
+        DWORD data_len = cdrData.length();
+        switch (methodType) {
+            case HttpMethod::kGET:
+                uassert(ErrorCodes::BadValue,
+                        "GET requests do not support content",
+                        cdrData.length() == 0);
+                break;
+            case HttpMethod::kPOST:
+                method = L"POST";
+                break;
+            case HttpMethod::kPUT:
+                method = L"PUT";
+                break;
+            default:
+                MONGO_UNREACHABLE;
+        }
 
-private:
-    DataBuilder doRequest(LPCWSTR method, StringData urlSD, LPVOID data, DWORD data_len) const {
         const auto uassertWithErrno = [](StringData reason, bool ok) {
             const auto msg = errnoWithDescription(GetLastError());
             uassert(ErrorCodes::OperationFailed, str::stream() << reason << ": " << msg, ok);
@@ -272,14 +276,11 @@ private:
                                              &statusCodeLength,
                                              WINHTTP_NO_HEADER_INDEX));
 
-        uassert(ErrorCodes::OperationFailed,
-                str::stream() << "Unexpected http status code from server: " << statusCode,
-                statusCode == 200);
-
+        DWORD len = 0;
         std::vector<char> buffer;
         DataBuilder ret(4096);
         for (;;) {
-            DWORD len = 0;
+            len = 0;
             uassertWithErrno("Failed receiving response data",
                              WinHttpQueryDataAvailable(request, &len));
             if (!len) {
@@ -294,7 +295,26 @@ private:
             ret.writeAndAdvance(cdr);
         }
 
-        return ret;
+        DataBuilder headers(4096);
+        if (!WinHttpQueryHeaders(request,
+                                 WINHTTP_QUERY_RAW_HEADERS_CRLF,
+                                 WINHTTP_HEADER_NAME_BY_INDEX,
+                                 WINHTTP_NO_OUTPUT_BUFFER,
+                                 &len,
+                                 WINHTTP_NO_HEADER_INDEX) &&
+            (GetLastError() == ERROR_INSUFFICIENT_BUFFER)) {
+            buffer.resize(len);
+            uassertWithErrno("Error querying headers from server",
+                             WinHttpQueryHeaders(request,
+                                                 WINHTTP_QUERY_RAW_HEADERS_CRLF,
+                                                 WINHTTP_HEADER_NAME_BY_INDEX,
+                                                 &buffer[0],
+                                                 &len,
+                                                 WINHTTP_NO_HEADER_INDEX));
+            headers.writeAndAdvance(ConstDataRange(buffer.data(), len));
+        }
+
+        return HttpReply(statusCode, std::move(headers), std::move(ret));
     }
 
 private: