Uploaded image for project: 'Libcloud'
  1. Libcloud
  2. LIBCLOUD-396

Commit c10249d1d733ba5215820c5d935265de5ae6bc84 break SWIFT storage

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 0.13.1
    • Fix Version/s: 0.13.2
    • Component/s: Storage
    • Labels:
      None

      Description

      Commit c10249d1d733ba5215820c5d935265de5ae6bc84 break SWIFT storage (Provider.CLOUDFILES_SWIFT).

      The commit sets the Content-Length to 0 which can overwrite the existing content length (in libcloud/common/base.py).

      The proposed fix: set the length to 0 only if it is not already set.

        Activity

        Hide
        ikusalic Ivan added a comment -

        Patch to fix the issue.

        Show
        ikusalic Ivan added a comment - Patch to fix the issue.
        Hide
        kami Tomaz Muraus added a comment -

        Can you please provide more information and background why would we want to allow that?

        Not sending any data with a request and setting Content-Length header to non-zero is invalid and seems like a bad thing to do.

        Thanks

        Show
        kami Tomaz Muraus added a comment - Can you please provide more information and background why would we want to allow that? Not sending any data with a request and setting Content-Length header to non-zero is invalid and seems like a bad thing to do. Thanks
        Hide
        sfriesel Stefan Friesel added a comment -

        The function storage.common.base._upload_object is doing just that. It sets data=None and Content-Length to a non-zero value when calling request(). The data itself is then sent after the call to request() by calling a upload_func callback. So other drivers using this base function may be afected as well.

        Show
        sfriesel Stefan Friesel added a comment - The function storage.common.base._upload_object is doing just that. It sets data=None and Content-Length to a non-zero value when calling request(). The data itself is then sent after the call to request() by calling a upload_func callback. So other drivers using this base function may be afected as well.
        Hide
        sfriesel Stefan Friesel added a comment -

        As an alternative to Ivan's patch, there could also be a distinction between data == '' and data is None, the former meaning empty/no content and the latter unspecified content, in which case the Content-Length should remain untouched (independent of http method).

        Show
        sfriesel Stefan Friesel added a comment - As an alternative to Ivan's patch, there could also be a distinction between data == '' and data is None, the former meaning empty/no content and the latter unspecified content, in which case the Content-Length should remain untouched (independent of http method).
        Hide
        kami Tomaz Muraus added a comment -

        Stefan Friesel ah, you are correct, thanks. Storage driver does that because it uses "raw" mode where only headers are sent first and data is sent later.

        Thanks and a good catch.

        In this case the patch looks good, but it needs some test cases before it can be merged. We already have some tests for this functionality (https://github.com/apache/libcloud/blob/trunk/libcloud/test/test_connection.py#L37) so it should be fairly easy to add additional ones.

        I'd also rather do "if method.upper() in ['POST', 'PUT'] and 'Content-Length' in headers" since it's more obvious what is going one compared to the set efault method.

        Please also add a comment to the this code which sums up a discussion from this ticket so other people looking at the code will know why we (need to) do this.

        Show
        kami Tomaz Muraus added a comment - Stefan Friesel ah, you are correct, thanks. Storage driver does that because it uses "raw" mode where only headers are sent first and data is sent later. Thanks and a good catch. In this case the patch looks good, but it needs some test cases before it can be merged. We already have some tests for this functionality ( https://github.com/apache/libcloud/blob/trunk/libcloud/test/test_connection.py#L37 ) so it should be fairly easy to add additional ones. I'd also rather do "if method.upper() in ['POST', 'PUT'] and 'Content-Length' in headers" since it's more obvious what is going one compared to the set efault method. Please also add a comment to the this code which sums up a discussion from this ticket so other people looking at the code will know why we (need to) do this.
        Hide
        kami Tomaz Muraus added a comment -

        Stefan Friesel I gave it some thought and I'm not really a big fan of empty string and None having a different meaning in this case. Imo, both should indicate "no data".

        In general I'm a big fan and proponent of "explicit is better than implicit", but in this case I think it would be very easy to make a mistake.

        Show
        kami Tomaz Muraus added a comment - Stefan Friesel I gave it some thought and I'm not really a big fan of empty string and None having a different meaning in this case. Imo, both should indicate "no data". In general I'm a big fan and proponent of "explicit is better than implicit", but in this case I think it would be very easy to make a mistake.
        Hide
        ikusalic Ivan added a comment - - edited

        (Added new patch.) I think this is the best set of conditions, as it explicitly references the raw argument. And raw argument has the description that should explain what's happening. Aside from that, '' and None (for data) are treated the same way.

        Show
        ikusalic Ivan added a comment - - edited (Added new patch.) I think this is the best set of conditions, as it explicitly references the raw argument. And raw argument has the description that should explain what's happening. Aside from that, '' and None (for data) are treated the same way.
        Hide
        kami Tomaz Muraus added a comment -

        Thanks, I've merged your patch into trunk.

        I agree with you and I think this is the most reasonable approach. To be on the safe side, I've also added a comment with extra clarification to the if statement.

        Since this was again a pretty nasty bug I will try to get 0.13.2 release with this bug fix out as soon as possible.

        On a side note - how did you encounter this bug? Did you use a storage driver and if so, which one?

        Show
        kami Tomaz Muraus added a comment - Thanks, I've merged your patch into trunk. I agree with you and I think this is the most reasonable approach. To be on the safe side, I've also added a comment with extra clarification to the if statement. Since this was again a pretty nasty bug I will try to get 0.13.2 release with this bug fix out as soon as possible. On a side note - how did you encounter this bug? Did you use a storage driver and if so, which one?
        Hide
        ikusalic Ivan added a comment -

        We used SWIFT on OpenStack (running on a small test cluster). The provider we use is 'CLOUDFILES_SWIFT'.

        Show
        ikusalic Ivan added a comment - We used SWIFT on OpenStack (running on a small test cluster). The provider we use is 'CLOUDFILES_SWIFT'.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 7179aacecebd3a0be303e59b49c5fc946200bb88 in branch refs/heads/trunk from Ivan
        [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=7179aac ]

        LIBCLOUD-396: Do not set Content-Length if present in raw requests

        Signed-off-by: Tomaz Muraus <tomaz@apache.org>

        Show
        jira-bot ASF subversion and git services added a comment - Commit 7179aacecebd3a0be303e59b49c5fc946200bb88 in branch refs/heads/trunk from Ivan [ https://git-wip-us.apache.org/repos/asf?p=libcloud.git;h=7179aac ] LIBCLOUD-396 : Do not set Content-Length if present in raw requests Signed-off-by: Tomaz Muraus <tomaz@apache.org>

          People

          • Assignee:
            Unassigned
            Reporter:
            ikusalic Ivan
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development