Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rgw: dont set CURLOPT_UPLOAD for GET requests #12105

Merged
merged 1 commit into from Nov 30, 2016

Conversation

@cbodley
Copy link
Contributor

commented Nov 21, 2016

when set on GET requests, curl sends a 'Transfer-encoding: chunked'
header, but doesn't do the actual encoding to terminate the message

Fixes: http://tracker.ceph.com/issues/17822

@cbodley

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2016

with this fix applied, test_multi.py can now run to completion

@@ -260,7 +260,9 @@ int RGWHTTPClient::process(const char *method, const char *url)
}
curl_easy_setopt(curl_handle, CURLOPT_READFUNCTION, simple_send_http_data);
curl_easy_setopt(curl_handle, CURLOPT_READDATA, (void *)this);
curl_easy_setopt(curl_handle, CURLOPT_UPLOAD, 1L);
if (method == nullptr || strcmp(method, "GET") != 0) {

This comment has been minimized.

Copy link
@yehudasa

yehudasa Nov 21, 2016

Member

@cbodley should also consider "HEAD"

@cbodley cbodley force-pushed the cbodley:wip-rgw-curl-upload branch from 1e7fc38 to bed28ee Nov 21, 2016

@cbodley

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2016

@yehudasa thanks for the review - i changed it to set CURLOPT_UPLOAD for PUT and POST

@@ -226,6 +226,13 @@ static curl_slist *headers_to_slist(param_vec_t& headers)
return h;
}

static bool is_upload_request(const char *method)
{
return method == nullptr

This comment has been minimized.

Copy link
@yehudasa

yehudasa Nov 21, 2016

Member

@cbodley should we treat method == nullptr as upload? what does it even mean if it's null?

This comment has been minimized.

Copy link
@cbodley

cbodley Nov 22, 2016

Author Contributor

I wasn't able to tell - our wrappers don't appear to ever pass null here, so it felt like a coin flip.

@cbodley

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2016

Some more background..

We pass the method string to CURLOPT_CUSTOMREQUEST, which curl uses directly in the HTTP request line. The libcurl tutorial states:

If just changing the actual HTTP request keyword is what you want, like when GET, HEAD or POST is not good enough for you, CURLOPT_CUSTOMREQUEST is there for you.

The CURLOPT_UPLOAD option flags the request as a PUT (data->set.httpreq = HTTPREQ_PUT), but CURLOPT_CUSTOMREQUEST only overrides the string sent in the request line to GET. Curl thought it was processing a PUT based on this httpreq flag, so it defaulted to chunked transfer encoding.

I have a feeling that we should avoid using CURLOPT_CUSTOMREQUEST, and use CURLOPT_UPLOAD and other the method-specific options instead. It looks like CURLOPT_CUSTOMREQUEST is only needed for DELETE requests.

@cbodley

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2016

@yehudasa I spent some time experimenting with a switch statement like this:

  switch (op) {
  case OP_GET:
    curl_easy_setopt(handle, CURLOPT_UPLOAD, 0L);
    break;
  case OP_HEAD:
    curl_easy_setopt(handle, CURLOPT_UPLOAD, 0L);
    curl_easy_setopt(handle, CURLOPT_NOBODY, 1L);
    break;
  case OP_PUT:
    curl_easy_setopt(handle, CURLOPT_UPLOAD, 1L);
    if (has_send_len) {
      curl_easy_setopt(handle, CURLOPT_INFILESIZE, (void *)send_len);
    }
    break;
  case OP_POST:
    // pass nullptr to get data from read callback
    curl_easy_setopt(handle, CURLOPT_POSTFIELDS, nullptr);
    if (has_send_len) {
      curl_easy_setopt(handle, CURLOPT_POSTFIELDSIZE, (void *)send_len);
    }
    break;
  case OP_DELETE:
  case OP_COPY:
  case OP_OPTIONS:
  default:
    curl_easy_setopt(handle, CURLOPT_CUSTOMREQUEST, method);
    break;
  }

but I ran into problems because libcurl will add a Content-Type: application/x-www-form-urlencoded header to these POST requests - and because Content-Type is part of the s3 signature, this breaks the authentication

we only generate POST requests for period commit and RGWPeriodPusher, so I tried passing this header in explicitly - but it got pretty messy, and I'm not sure the refactoring is worth it at this point when the fix in this PR is so much simpler. what do you think?

@yehudasa

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

@cbodley I think that we should keep it simple and the change minimal

@theanalyst

This comment has been minimized.

Copy link
Member

commented Nov 28, 2016

Also ran into this with civetweb when trying out multisite on master, will apply the patch and see if it works

@theanalyst

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

After applying the patch can run multisite on master again, before this patch saw a few sync issues

rgw: only set CURLOPT_UPLOAD for PUT/POST requests
when set on GET requests, curl sends a 'Transfer-encoding: chunked'
header, but doesn't do the actual encoding to terminate the message

Fixes: http://tracker.ceph.com/issues/17822

Signed-off-by: Casey Bodley <cbodley@redhat.com>

@cbodley cbodley force-pushed the cbodley:wip-rgw-curl-upload branch from bed28ee to f3a0c40 Nov 29, 2016

@cbodley

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2016

@yehudasa updated to treat method == nullptr as a GET request - still passing test_multi.py

@yehudasa
Copy link
Member

left a comment

lgtm

@mattbenjamin mattbenjamin merged commit 426c910 into ceph:master Nov 30, 2016

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details
tchaikov added a commit to tchaikov/ceph that referenced this pull request Dec 1, 2016
Merge pull request ceph#12105 from cbodley/wip-rgw-curl-upload
rgw: dont set CURLOPT_UPLOAD for GET requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.