ratelimit doesn't filter requests very well

Bug #1669888 reported by John Dickinson
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
Low
Unassigned

Bug Description

The ratelimit middleware attempts to rate limit a request if the request patch has at least 4 elements delineated by a '/'. It attempts to do account HEAD requests to the second element.

If the ratelimit middleware is placed before pipeline elements that mutate the path (eg swift3, domain_remap), then the ratelimit middleware ends up generating a lot of spurious subrequests that will fail.

At a minimum, we should better document what ratelimit is looking for and/or where it should be in the pipeline. We should also probably filter the first element of split_path() in ratelimit to look for valid swift api versions (swift.conf 'valid_api_versions').

tags: added: low-hanging-fruit
Changed in swift:
importance: Undecided → Low
Tim Burke (1-tim-z)
Changed in swift:
status: New → Confirmed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to swift (master)

Reviewed: https://review.openstack.org/540092
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=6994a2e392be4096beb49bd33e1a507dd04d491e
Submitter: Zuul
Branch: master

commit 6994a2e392be4096beb49bd33e1a507dd04d491e
Author: Samuel Merritt <email address hidden>
Date: Wed Jan 31 17:05:28 2018 -0800

    ratelimit: ignore requests with invalid API versions

    If you've got things like domain_remap, swift3, or other such
    middlewares in your pipeline, you wind up with requests that aren't of
    the form "/v1/<account>/<container>/<object>". When encountering such
    an oddball request, it's not useful to call get_account_info() on the
    second path component since it's probably not an account.

    This commit makes the ratelimit middleware skip requests that don't
    start with either "/v1" or "/v1.0". The requests will still be
    handled, but they won't be rate-limited.

    Change-Id: I9980cd0e902610ac99d13a502ae955bca2d99df3
    Closes-Bug: 1669888
    Closes-Bug: 1695273

Changed in swift:
status: Confirmed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.20.0

This issue was fixed in the openstack/swift 2.20.0 release.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (master)

Related fix proposed to branch: master
Review: https://review.opendev.org/697535

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (stable/train)

Related fix proposed to branch: stable/train
Review: https://review.opendev.org/704646

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (stable/stein)

Related fix proposed to branch: stable/stein
Review: https://review.opendev.org/704659

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (master)

Reviewed: https://review.opendev.org/697535
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=f33c061ae92f33ea467c58749d3f15d2b1cc942c
Submitter: Zuul
Branch: master

commit f33c061ae92f33ea467c58749d3f15d2b1cc942c
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 09:42:33 2019 -0800

    s3api: Better handle 498/429 responses

    Currently, they just 500 as an unexpected response status. Much better
    would be S3's '503 Slow Down' response.

    Of course, that's all dependent on where you place ratelimit in your
    pipeline -- and we haven't really given clear guidance on that. I'm not
    actually sure you *want* ratelimit to be after s3api and auth... but if
    you *do*, let's at least handle it gracefully.

    Change-Id: I36f0954fd9949d7d1404a0c381b917d1cfb17ec5
    Related-Bug: 1669888

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (stable/train)

Reviewed: https://review.opendev.org/704646
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=092054da632037cd3632d8a2c85b7d2015998fff
Submitter: Zuul
Branch: stable/train

commit 092054da632037cd3632d8a2c85b7d2015998fff
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 09:42:33 2019 -0800

    s3api: Better handle 498/429 responses

    Currently, they just 500 as an unexpected response status. Much better
    would be S3's '503 Slow Down' response.

    Of course, that's all dependent on where you place ratelimit in your
    pipeline -- and we haven't really given clear guidance on that. I'm not
    actually sure you *want* ratelimit to be after s3api and auth... but if
    you *do*, let's at least handle it gracefully.

    Change-Id: I36f0954fd9949d7d1404a0c381b917d1cfb17ec5
    Related-Bug: 1669888
    (cherry picked from commit f33c061ae92f33ea467c58749d3f15d2b1cc942c)

tags: added: in-stable-train
tags: added: in-stable-stein
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (stable/stein)

Reviewed: https://review.opendev.org/704659
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=778767f324e82dd7a075306cf8468aabbbc363b6
Submitter: Zuul
Branch: stable/stein

commit 778767f324e82dd7a075306cf8468aabbbc363b6
Author: Tim Burke <email address hidden>
Date: Thu Dec 5 09:42:33 2019 -0800

    s3api: Better handle 498/429 responses

    Currently, they just 500 as an unexpected response status. Much better
    would be S3's '503 Slow Down' response.

    Of course, that's all dependent on where you place ratelimit in your
    pipeline -- and we haven't really given clear guidance on that. I'm not
    actually sure you *want* ratelimit to be after s3api and auth... but if
    you *do*, let's at least handle it gracefully.

    Change-Id: I36f0954fd9949d7d1404a0c381b917d1cfb17ec5
    Related-Bug: 1669888
    (cherry picked from commit f33c061ae92f33ea467c58749d3f15d2b1cc942c)

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to swift (feature/losf)

Related fix proposed to branch: feature/losf
Review: https://review.opendev.org/713632

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to swift (feature/losf)
Download full text (40.5 KiB)

Reviewed: https://review.opendev.org/713632
Committed: https://git.openstack.org/cgit/openstack/swift/commit/?id=79bd2e59e5c15ee84814ec1c4f0893176ba79412
Submitter: Zuul
Branch: feature/losf

commit f2ffd900593db2829a39a073f0c033d82985c40f
Author: Clay Gerrard <email address hidden>
Date: Fri Feb 28 11:09:51 2020 -0600

    Apply limit to list versioned containers

    Change-Id: I28e062273d673c4f07cd3c5da088aa790b77a599
    Closes-Bug: #1863841

commit dc40779307095b8d0b2761b77b9cb2904ec721ae
Author: Clay Gerrard <email address hidden>
Date: Fri Feb 28 10:00:25 2020 -0600

    Use float consistently for proxy timeout settings

    Change-Id: I433c97df99193ec31c863038b9b6fd20bb3705b8

commit 55049beda5b9d7038a3604a87f28312d7702ccb2
Author: Tim Burke <email address hidden>
Date: Fri Feb 28 18:59:32 2020 -0800

    tests: Use timedelta to adjust dates, not string manipulations

    Change-Id: I8f65ccd7f2a79d5b877bfbef0274fb7857e21391

commit 3b65a5998cc921d2763cf1a9ec1e40b88491262d
Author: Tim Burke <email address hidden>
Date: Wed Jan 10 06:16:41 2018 +0000

    Fix up some Content-Type handling in account/container listings

    Update content type on 204 (not just 200) to properly handle HEAD
    requests from xml/txt listings.

    Add "Vary: Accept" header to listings, since otherwise, browsers may
    serve the wrong content type from cache (even though we *would have*
    sent the *right* type if it actually sent the request).

    Change-Id: Iaa333aaca36a8dc2df65d38ef2173e3a6e2000ee

commit ecca23eb806e11cf6517f0456483da7a065350a8
Author: Clay Gerrard <email address hidden>
Date: Fri Feb 21 15:33:21 2020 -0600

    Extend eventlet_debug logging to GreenAsyncPile

    Change-Id: Ibd9fe5c9a1e75b86eb7d540594d5cf516758e17e

commit 0fb3371484f1d0f629d0b0e33f6aafbff0e43ee9
Author: Sam Morrison <email address hidden>
Date: Tue Feb 18 10:17:50 2020 +1100

    Delay importing swiftclient until after monkey-patching

    Commit message below partly copied from nova:

    Eventlet monkey patching should be as early as possible

    We were seeing infinite recursion opening an ssl socket when running
    various combinations of python3, eventlet, and urllib3. It is not
    clear exactly what combination of versions are affected, but for
    background there is an example of this issue documented here:

    https://github.com/eventlet/eventlet/issues/371

    The immediate cause in swift's case was that we were calling
    eventlet.monkey_patch() after importing swiftclient (which imported
    requests, which finally imported urllib3).

    We only use the imported function in one place, however; hold off on
    importing until we actually need it to ensure that monkey patching
    happens first. Note that we *don't* want to monkey-patch at import time,
    as we've previously had bugs related to import-time side-effects.

    Change-Id: I24f4bcc3d62dc37fd9559032bfd25f5b15f98745
    Closes-bug: #1863680
    Related-bug: #1380815

commit a5afe767581d2cb97cf3690067e6d626c7682c2c
Author: Tim Burke <email address hidden>
Date: Wed Feb 19 10:09:49 2020 -0800

    Revert "Make roll...

tags: added: in-feature-losf
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.