You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Gabriella Lotz (Code Review)" <ge...@cloudera.org> on 2025/04/07 12:21:09 UTC
[kudu-CR] Add --location option to EasyCurl
Gabriella Lotz has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22745
Change subject: Add --location option to EasyCurl
......................................................................
Add --location option to EasyCurl
To support leader redirection in multi-master setups for the REST API,
EasyCurl now includes the --location flag. This ensures that HTTP
requests automatically follow redirects during testing, aligning with
the behavior of curl when --location is specified.
Change-Id: Idc39f380b79afc07553f178f81b7a58816d5cc4b
---
M src/kudu/util/curl_util.cc
M src/kudu/util/curl_util.h
2 files changed, 10 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/45/22745/1
--
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: newchange
Gerrit-Change-Id: Idc39f380b79afc07553f178f81b7a58816d5cc4b
Gerrit-Change-Number: 22745
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
[kudu-CR] Add --location option to EasyCurl
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22745 )
Change subject: Add --location option to EasyCurl
......................................................................
Patch Set 1: Verified-1
Build Failed
http://jenkins.kudu.apache.org/job/pre_commit/1254/ : FAILURE
--
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 <lo...@gmail.com>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Apr 2025 13:26:40 +0000
Gerrit-HasComments: No
[kudu-CR] Add --location option to EasyCurl
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 07 Apr 2025 20:45:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] Add --location option to EasyCurl
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22745 )
Change subject: Add --location option to EasyCurl
......................................................................
Patch Set 1:
Build Started http://jenkins.kudu.apache.org/job/pre_commit/1254/
--
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 <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 07 Apr 2025 12:21:16 +0000
Gerrit-HasComments: No
[kudu-CR] Add --location option to EasyCurl
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has posted comments on this change. ( http://gerrit.cloudera.org:8080/22745 )
Change subject: Add --location option to EasyCurl
......................................................................
Patch Set 1:
--location (follow redirects) does not work with SPNEGO authentication, which is required for secure clusters. While there is a --location-trusted option that allows curl to follow redirects and send credentials to other hosts, using it may introduce a security breach.
Given these constraints, instead of relying on redirect-based leader discovery, I will implement a dedicated mechanism to explicitly expose the leader address.
--
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 <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>
Gerrit-Comment-Date: Mon, 14 Apr 2025 09:02:17 +0000
Gerrit-HasComments: No
[kudu-CR] Add --location option to EasyCurl
Posted by "Gabriella Lotz (Code Review)" <ge...@cloudera.org>.
Gabriella Lotz has abandoned this change. ( http://gerrit.cloudera.org:8080/22745 )
Change subject: Add --location option to EasyCurl
......................................................................
Abandoned
--location (follow redirects) does not work with SPNEGO authentication, which is required for secure clusters. While there is a --location-trusted option that allows curl to follow redirects and send credentials to other hosts, using it may introduce a security breach—especially when the redirect changes the hostname, protocol, or port.
Given these constraints, instead of relying on redirect-based leader discovery, I will implement a dedicated mechanism to explicitly expose the leader address.
--
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: abandon
Gerrit-Change-Id: Idc39f380b79afc07553f178f81b7a58816d5cc4b
Gerrit-Change-Number: 22745
Gerrit-PatchSet: 1
Gerrit-Owner: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Gabriella Lotz <lo...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Marton Greber <gr...@gmail.com>
Gerrit-Reviewer: Zoltan Chovan <zc...@cloudera.com>