Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22745 )
Change subject: Add --location option to EasyCurl ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/22745/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/22745/1//COMMIT_MSG@9 PS1, Line 9: upport leader redirection in multi-master setups Just curious: did you find it works as-is with SPNEGO authentication or there is a need to add some extra functionality (in a follow-up patch?) to deal with the following curl's behavior, as per 'man CURLOPT_FOLLOWLOCATION': By default, libcurl only sends Authentication: or explicitly set Cookie: headers to the initial host given in the original URL, to avoid leaking username + password to other sites. CURLOPT_UNRESTRICTED_AUTH(3) is provided to change that behavior. http://gerrit.cloudera.org:8080/#/c/22745/1//COMMIT_MSG@10 PS1, Line 10: --location flag I'm not sure I understand what flag it's all about. Does this patch missing a few files? http://gerrit.cloudera.org:8080/#/c/22745/1//COMMIT_MSG@10 PS1, Line 10: This ensures that HTTP : requests automatically follow redirects during testing, aligning with : the behavior of curl when --location is specified. BTW, did you find it's enough just to control the CURLOPT_FOLLOWLOCATION option only or there is a need to tweak CURLOPT_POSTREDIR as well as per 'man CURLOPT_FOLLOWLOCATION' (probably, in a follow-up patch)? When following a redirect, the specific 30x response code also dictates which request method libcurl uses in the subsequent request: For 301, 302 and 303 responses libcurl switches method from POST to GET unless CURLOPT_POSTREDIR(3) instructs libcurl otherwise. All other redirect response codes make libcurl use the same method again. http://gerrit.cloudera.org:8080/#/c/22745/1/src/kudu/util/curl_util.h File src/kudu/util/curl_util.h: http://gerrit.cloudera.org:8080/#/c/22745/1/src/kudu/util/curl_util.h@95 PS1, Line 95: void set_follow_location(bool v) { Add a small documentation blurb for this method helping the users of this API understand: that's about HTTP/HTTPS redirects. http://gerrit.cloudera.org:8080/#/c/22745/1/src/kudu/util/curl_util.cc File src/kudu/util/curl_util.cc: http://gerrit.cloudera.org:8080/#/c/22745/1/src/kudu/util/curl_util.cc@229 PS1, Line 229: if (follow_location_) { : CURL_RETURN_NOT_OK(curl_easy_setopt(curl_, CURLOPT_FOLLOWLOCATION, 1L)); : } Consider changing this to unconditional: CURL_RETURN_NOT_OK(curl_easy_setopt(curl_, CURLOPT_FOLLOWLOCATION, follow_location_)); That way it will behave much more predictable in scenarios when re-using the same instance of EasyCurl handle like below: EasyCurl c; // Need to follow HTTP redirects when fetching url1. c.set_follow_location(true); RETURN_NOT_OK(c.FetchURL(url1, ...)); ... // Now, redirects shouldn't be followed when fetching url2. c.set_follow_location(false); RETURN_NOT_OK(c.FetchURL(url2, ...)); If I'm not mistaken, with the current code in PS1 HTTP redirects would be followed for url2 even if the intention was the exact opposite. In this context, consider adding a small unit test as a part of this patch to make sure CURLOPT_FOLLOWLOCATION is set as expected in the underlying curl library handle once EasyCurl::set_follow_location() is called with false/true followed by the EasyCurl::DoRequest() call. -- To view, visit http://gerrit.cloudera.org:8080/22745 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idc39f380b79afc07553f178f81b7a58816d5cc4b Gerrit-Change-Number: 22745 Gerrit-PatchSet: 1 Gerrit-Owner: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gabriella Lotz <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Mon, 07 Apr 2025 20:45:07 +0000 Gerrit-HasComments: Yes
