Can't shard containers with special characters in their names

Bug #1856894 reported by Tim Burke
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Object Storage (swift)
Fix Released
Undecided
Unassigned

Bug Description

...in particular, at least newlines.

When we set the root container information in shards [0], we don't quote the name, we just send it raw. A slightly modified probe test demonstrates the bad:

diff --git a/test/probe/test_sharder.py b/test/probe/test_sharder.py
index 2d5c5417f..5a829e376 100644
--- a/test/probe/test_sharder.py
+++ b/test/probe/test_sharder.py
@@ -104,7 +104,7 @@ class BaseTestContainerSharding(ReplProbeTest):
         return ['obj-%04d' % x for x in range(number)]

     def _setup_container_name(self):
- self.container_name = 'container-%s' % uuid.uuid4()
+ self.container_name = 'container\n%s' % uuid.uuid4()

     def setUp(self):
         client.logger.setLevel(client.logging.WARNING)

Tests fail like:

FAIL: test_async_pendings (test.probe.test_sharder.TestContainerSharding)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/vagrant/swift/test/probe/test_sharder.py", line 1069, in test_async_pendings
    [sr.state for sr in expected_shard_ranges])
AssertionError: Lists differ: [30, 30, 20] != [10, 10, 10]

While logs show errors like:

Dec 18 18:53:00 saio container-sharder-6041: Failed to put shard ranges to 127.0.0.1:6041/sdb4: Invalid header value b'AUTH_test/container\nd05ffbed-1576-4e7a-bea0-d1faaba3cbe5':
Traceback (most recent call last):
  File "/vagrant/swift/swift/container/sharder.py", line 595, in _put_container
    headers=headers, contents=body)
  File "/vagrant/swift/swift/common/direct_client.py", line 351, in direct_put_container
    content_length=content_length, chunk_size=chunk_size)
  File "/vagrant/swift/swift/common/direct_client.py", line 106, in _make_req
    method, path, headers=headers)
  File "/vagrant/swift/swift/common/bufferedhttp.py", line 269, in http_connect
    ipaddr, port, method, path, headers, query_string, ssl)
  File "/vagrant/swift/swift/common/bufferedhttp.py", line 309, in http_connect_raw
    conn.putheader(header, str(value))
  File "/vagrant/swift/swift/common/bufferedhttp.py", line 221, in putheader
    HTTPConnection.putheader(self, header, value)
  File "/usr/local/lib/python3.6/dist-packages/eventlet/green/http/client.py", line 1278, in putheader
    raise ValueError('Invalid header value %r' % (values[i],))
ValueError: Invalid header value b'AUTH_test/container\nd05ffbed-1576-4e7a-bea0-d1faaba3cbe5'

Revision history for this message
Tim Burke (1-tim-z) wrote :

Bah, forgot my link for [0] -- https://github.com/openstack/swift/blob/2.23.0/swift/container/sharder.py#L1129

Also, there's a similar (but subtly different) problem when sharding a reserved-name container: http.client is willing to send the header, but then it fails validation on the server: https://github.com/openstack/swift/blob/2.23.0/swift/common/db.py#L884-L885

I think the way to get around this is our usual approach of quote()ing the header value -- but we'll likely need to use a new header name so we don't mess up existing containers whose (unquoted) name actually includes percent-signs.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on swift (master)

Change abandoned by Clay Gerrard (<email address hidden>) on branch: master
Review: https://review.opendev.org/700789
Reason: https://review.opendev.org/#/c/699892/ is the way to go!

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

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

commit 3f8890701259e24ef81c93947faf2d4ccf223c5f
Author: Tim Burke <email address hidden>
Date: Wed Dec 18 15:14:00 2019 -0800

    sharding: Better-handle newlines in container names

    Previously, if you were on Python 2.7.10+ [0], such a newline would cause the
    sharder to fail, complaining about invalid header values when trying to create
    the shard containers. On older versions of Python, it would most likely cause a
    parsing error in the container-server that was trying to handle the PUT.

    Now, quote all places that we pass around container paths. This includes:

      * The X-Container-Sysmeta-Shard-(Quoted-)Root sent when creating the (empty)
        remote shards
      * The X-Container-Sysmeta-Shard-(Quoted-)Root included when initializing the
        local handoff for cleaving
      * The X-Backend-(Quoted-)Container-Path the proxy sends to the object-server
        for container updates
      * The Location header the container-server sends to the object-updater

    Note that a new header was required in requests so that servers would
    know whether the value should be unquoted or not. We can get away with
    reusing Location in responses by having clients opt-in to quoting with
    a new X-Backend-Accept-Quoted-Location header.

    During a rolling upgrade,

      * old object-servers servicing requests from new proxy-servers will
        not know about the container path override and so will try to update
        the root container,
      * in general, object updates are more likely to land in the root
        container; the sharder will deal with them as misplaced objects, and
      * shard containers created by new code on servers running old code
        will think they are root containers until the server is running new
        code, too; during this time they'll fail the sharder audit and report
        stats to their account, but both of these should get cleared up upon
        upgrade.

    Drive-by: fix a "conainer_name" typo that prevented us from testing that
    we can shard a container with unicode in its name. Also, add more UTF8
    probe tests.

    [0] See https://bugs.python.org/issue22928

    Change-Id: Ie08f36e31a448a547468dd85911c3a3bc30e89f1
    Closes-Bug: 1856894

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

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

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

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

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : 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
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/swift 2.23.3

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

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.