You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/03 07:08:13 UTC

[GitHub] [pulsar] BewareMyPower commented on a change in pull request #13112: [C++]Fix libcurl miss auth header when broker return 307

BewareMyPower commented on a change in pull request #13112:
URL: https://github.com/apache/pulsar/pull/13112#discussion_r761691866



##########
File path: pulsar-client-cpp/lib/HTTPLookupService.cc
##########
@@ -148,132 +148,148 @@ void HTTPLookupService::handleNamespaceTopicsHTTPRequest(NamespaceTopicsPromise
     }
 }
 
-Result HTTPLookupService::sendHTTPRequest(const std::string completeUrl, std::string &responseData) {
-    CURL *handle;
-    CURLcode res;
-    std::string version = std::string("Pulsar-CPP-v") + _PULSAR_VERSION_INTERNAL_;
-    handle = curl_easy_init();
-
-    if (!handle) {
-        LOG_ERROR("Unable to curl_easy_init for url " << completeUrl);
-        // No curl_easy_cleanup required since handle not initialized
-        return ResultLookupError;
-    }
-    // set URL
-    curl_easy_setopt(handle, CURLOPT_URL, completeUrl.c_str());
-
-    // Write callback
-    curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, curlWriteCallback);
-    curl_easy_setopt(handle, CURLOPT_WRITEDATA, &responseData);
+Result HTTPLookupService::sendHTTPRequest(std::string completeUrl, std::string &responseData) {
+    uint16_t reqCount = 0;
+    Result retResult = ResultOk;
+    while(++reqCount <= MAX_HTTP_REDIRECTS) {
+        CURL *handle;
+        CURLcode res;
+        std::string version = std::string("Pulsar-CPP-v") + _PULSAR_VERSION_INTERNAL_;
+        handle = curl_easy_init();
+
+        if (!handle) {
+            LOG_ERROR("Unable to curl_easy_init for url " << completeUrl);
+            // No curl_easy_cleanup required since handle not initialized
+            return ResultLookupError;
+        }
+        // set URL
+        curl_easy_setopt(handle, CURLOPT_URL, completeUrl.c_str());
 
-    // New connection is made for each call
-    curl_easy_setopt(handle, CURLOPT_FRESH_CONNECT, 1L);
-    curl_easy_setopt(handle, CURLOPT_FORBID_REUSE, 1L);
+        // Write callback
+        curl_easy_setopt(handle, CURLOPT_WRITEFUNCTION, curlWriteCallback);
+        curl_easy_setopt(handle, CURLOPT_WRITEDATA, &responseData);
 
-    // Skipping signal handling - results in timeouts not honored during the DNS lookup
-    curl_easy_setopt(handle, CURLOPT_NOSIGNAL, 1L);
+        // New connection is made for each call
+        curl_easy_setopt(handle, CURLOPT_FRESH_CONNECT, 1L);
+        curl_easy_setopt(handle, CURLOPT_FORBID_REUSE, 1L);
 
-    // Timer
-    curl_easy_setopt(handle, CURLOPT_TIMEOUT, lookupTimeoutInSeconds_);
+        // Skipping signal handling - results in timeouts not honored during the DNS lookup
+        curl_easy_setopt(handle, CURLOPT_NOSIGNAL, 1L);
 
-    // Set User Agent
-    curl_easy_setopt(handle, CURLOPT_USERAGENT, version.c_str());
+        // Timer
+        curl_easy_setopt(handle, CURLOPT_TIMEOUT, lookupTimeoutInSeconds_);
 
-    // Redirects
-    curl_easy_setopt(handle, CURLOPT_FOLLOWLOCATION, 1L);
-    curl_easy_setopt(handle, CURLOPT_MAXREDIRS, MAX_HTTP_REDIRECTS);
+        // Set User Agent
+        curl_easy_setopt(handle, CURLOPT_USERAGENT, version.c_str());
 
-    // Fail if HTTP return code >=400
-    curl_easy_setopt(handle, CURLOPT_FAILONERROR, 1L);
+        // Redirects
+        // curl_easy_setopt(handle, CURLOPT_FOLLOWLOCATION, 1L);
+        // curl_easy_setopt(handle, CURLOPT_MAXREDIRS, MAX_HTTP_REDIRECTS);
 
-    // Authorization data
-    AuthenticationDataPtr authDataContent;
-    Result authResult = authenticationPtr_->getAuthData(authDataContent);
-    if (authResult != ResultOk) {
-        LOG_ERROR("Failed to getAuthData: " << authResult);
-        curl_easy_cleanup(handle);
-        return authResult;
-    }
-    struct curl_slist *list = NULL;
-    if (authDataContent->hasDataForHttp()) {
-        list = curl_slist_append(list, authDataContent->getHttpHeaders().c_str());
-    }
-    curl_easy_setopt(handle, CURLOPT_HTTPHEADER, list);
+        // Fail if HTTP return code >=400
+        curl_easy_setopt(handle, CURLOPT_FAILONERROR, 1L);
 
-    // TLS
-    if (isUseTls_) {
-        if (curl_easy_setopt(handle, CURLOPT_SSLENGINE, NULL) != CURLE_OK) {
-            LOG_ERROR("Unable to load SSL engine for url " << completeUrl);
+        // Authorization data
+        AuthenticationDataPtr authDataContent;
+        Result authResult = authenticationPtr_->getAuthData(authDataContent);
+        if (authResult != ResultOk) {
+            LOG_ERROR("Failed to getAuthData: " << authResult);
             curl_easy_cleanup(handle);
-            return ResultConnectError;
+            return authResult;
         }
-        if (curl_easy_setopt(handle, CURLOPT_SSLENGINE_DEFAULT, 1L) != CURLE_OK) {
-            LOG_ERROR("Unable to load SSL engine as default, for url " << completeUrl);
-            curl_easy_cleanup(handle);
-            return ResultConnectError;
+        struct curl_slist *list = NULL;
+        if (authDataContent->hasDataForHttp()) {
+            list = curl_slist_append(list, authDataContent->getHttpHeaders().c_str());
         }
-        curl_easy_setopt(handle, CURLOPT_SSLCERTTYPE, "PEM");
+        curl_easy_setopt(handle, CURLOPT_HTTPHEADER, list);
+
+        // TLS
+        if (isUseTls_) {
+            if (curl_easy_setopt(handle, CURLOPT_SSLENGINE, NULL) != CURLE_OK) {
+                LOG_ERROR("Unable to load SSL engine for url " << completeUrl);
+                curl_easy_cleanup(handle);
+                return ResultConnectError;
+            }
+            if (curl_easy_setopt(handle, CURLOPT_SSLENGINE_DEFAULT, 1L) != CURLE_OK) {
+                LOG_ERROR("Unable to load SSL engine as default, for url " << completeUrl);
+                curl_easy_cleanup(handle);
+                return ResultConnectError;
+            }
+            curl_easy_setopt(handle, CURLOPT_SSLCERTTYPE, "PEM");
 
-        if (tlsAllowInsecure_) {
-            curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 0L);
-        } else {
-            curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 1L);
-        }
+            if (tlsAllowInsecure_) {
+                curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 0L);
+            } else {
+                curl_easy_setopt(handle, CURLOPT_SSL_VERIFYPEER, 1L);
+            }
 
-        if (!tlsTrustCertsFilePath_.empty()) {
-            curl_easy_setopt(handle, CURLOPT_CAINFO, tlsTrustCertsFilePath_.c_str());
-        }
+            if (!tlsTrustCertsFilePath_.empty()) {
+                curl_easy_setopt(handle, CURLOPT_CAINFO, tlsTrustCertsFilePath_.c_str());
+            }
 
-        curl_easy_setopt(handle, CURLOPT_SSL_VERIFYHOST, tlsValidateHostname_ ? 1L : 0L);
+            curl_easy_setopt(handle, CURLOPT_SSL_VERIFYHOST, tlsValidateHostname_ ? 1L : 0L);
 
-        if (authDataContent->hasDataForTls()) {
-            curl_easy_setopt(handle, CURLOPT_SSLCERT, authDataContent->getTlsCertificates().c_str());
-            curl_easy_setopt(handle, CURLOPT_SSLKEY, authDataContent->getTlsPrivateKey().c_str());
+            if (authDataContent->hasDataForTls()) {
+                curl_easy_setopt(handle, CURLOPT_SSLCERT, authDataContent->getTlsCertificates().c_str());
+                curl_easy_setopt(handle, CURLOPT_SSLKEY, authDataContent->getTlsPrivateKey().c_str());
+            }
         }
-    }
-
-    LOG_INFO("Curl Lookup Request sent for " << completeUrl);
-
-    // Make get call to server
-    res = curl_easy_perform(handle);
-
-    // Free header list
-    curl_slist_free_all(list);
 
-    Result retResult = ResultOk;
-
-    switch (res) {
-        case CURLE_OK:
-            long response_code;
-            curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &response_code);
-            LOG_INFO("Response received for url " << completeUrl << " code " << response_code);
-            if (response_code == 200) {
-                retResult = ResultOk;
-            } else {
+        LOG_INFO("Curl [" << reqCount <<  "] Lookup Request sent for " << completeUrl);
+
+        // Make get call to server
+        res = curl_easy_perform(handle);
+        
+        long response_code;
+        curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &response_code);
+        LOG_INFO("Response received for url " << completeUrl << " response_code " << response_code << " curl res " << res);
+
+        // Free header list
+        curl_slist_free_all(list);
+
+        switch (res) {
+            case CURLE_OK:
+                long response_code;
+                curl_easy_getinfo(handle, CURLINFO_RESPONSE_CODE, &response_code);
+                LOG_INFO("Response received for url " << completeUrl << " code " << response_code);
+                if (response_code == 200) {
+                    retResult = ResultOk;
+                } else if (response_code == 307) {
+                    char *url = NULL;
+                    curl_easy_getinfo(handle, CURLINFO_REDIRECT_URL, &url);
+                    LOG_INFO("Response from url " << completeUrl << " to new url " << url);
+                    completeUrl = url;
+                    retResult = ResultLookupError;
+                } else {
+                    retResult = ResultLookupError;
+                }
+                break;
+            case CURLE_COULDNT_CONNECT:
+            case CURLE_COULDNT_RESOLVE_PROXY:
+            case CURLE_COULDNT_RESOLVE_HOST:
+            case CURLE_HTTP_RETURNED_ERROR:
+                LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
+                retResult = ResultConnectError;
+                break;
+            case CURLE_READ_ERROR:
+                LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
+                retResult = ResultReadError;
+                break;
+            case CURLE_OPERATION_TIMEDOUT:
+                LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
+                retResult = ResultTimeout;
+                break;
+            default:
+                LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
                 retResult = ResultLookupError;
-            }
-            break;
-        case CURLE_COULDNT_CONNECT:
-        case CURLE_COULDNT_RESOLVE_PROXY:
-        case CURLE_COULDNT_RESOLVE_HOST:
-        case CURLE_HTTP_RETURNED_ERROR:
-            LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
-            retResult = ResultConnectError;
-            break;
-        case CURLE_READ_ERROR:
-            LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
-            retResult = ResultReadError;
-            break;
-        case CURLE_OPERATION_TIMEDOUT:
-            LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
-            retResult = ResultTimeout;
-            break;
-        default:
-            LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res);
-            retResult = ResultLookupError;
+                break;
+        }
+        curl_easy_cleanup(handle);
+        if (response_code != 307) {
             break;
+        }

Review comment:
       What if the response code is other 30x?
   
   See https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#3xx_redirection




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org