Fix web-server memory overrun when downloading objects from Swift

To prevent memory overrun when downloading large objects from Swift
* `resp_chunk_size` keyword should be passed to swiftclient
* `obj.data` iterator returned from swiftclient is passed to HttpResponse
  (or StreamingHttpResponse for Django>=1.5) as usual since both response
  classes work with iterators/files/byte strings (yet StreamingHttpResponse
  does it better).

The commit introduces new setting SWIFT_FILE_TRANSFER_CHUNK_SIZE that
defines the size of chunk in bytes for Swift objects downloading.

DocImpact

Change-Id: I18e5809b86bfa24948dc642da2a55dffaa1a4ce1
Closes-Bug: #1427819

doc/source/topics/settings.rst

@@ -335,6 +335,19 @@ Valid values are  ``"AUTO"``(default), ``"VNC"``, ``"SPICE"``, ``"RDP"``,
 ``"SERIAL"`` is available since 2005.1(Kilo).
 
 
+``SWIFT_FILE_TRANSFER_CHUNK_SIZE``
+----------------------------------
+
+.. versionadded:: 2015.1(Kilo)
+
+Default: ``512 * 1024``
+
+This setting specifies the size of the chunk (in bytes) for downloading objects
+from Swift. Do not make it very large (higher than several dozens of Megabytes,
+exact number depends on your connection speed), otherwise you may encounter
+socket timeout. The default value is 524288 bytes (or 512 Kilobytes).
+
+
 ``INSTANCE_LOG_LENGTH``
 -----------------------
 

horizon/test/helpers.py

@@ -31,6 +31,7 @@ from django.core.handlers import wsgi
 from django import http
 from django import test as django_test
 from django.test.client import RequestFactory  # noqa
+from django.test import testcases
 from django.utils.encoding import force_text
 from django.utils import unittest
 
@@ -208,6 +209,48 @@ class TestCase(django_test.TestCase):
                 "%s messages not as expected: %s" % (msg_type.title(),
                                                      ", ".join(msgs))
 
+    def assertNotContains(self, response, text, status_code=200,
+                          msg_prefix='', html=False):
+        """Asserts that a response indicates that some content was retrieved
+        successfully, (i.e., the HTTP status code was as expected), and that
+        ``text`` doesn't occurs in the content of the response.
+
+        This is an override of django_test.TestCase.assertNotContains method,
+        which is able to work with StreamingHttpResponse.
+        """
+        # If the response supports deferred rendering and hasn't been rendered
+        # yet, then ensure that it does get rendered before proceeding further.
+        if (hasattr(response, 'render') and callable(response.render) and
+                not response.is_rendered):
+            response.render()
+
+        if msg_prefix:
+            msg_prefix += ": "
+
+        self.assertEqual(
+            response.status_code, status_code,
+            msg_prefix + "Couldn't retrieve content: Response code was %d"
+            " (expected %d)" % (response.status_code, status_code))
+
+        if getattr(response, 'streaming', False):
+            content = b''.join(response.streaming_content)
+        else:
+            content = response.content
+        if not isinstance(text, bytes) or html:
+            text = force_text(text, encoding=response._charset)
+            content = content.decode(response._charset)
+            text_repr = "'%s'" % text
+        else:
+            text_repr = repr(text)
+        if html:
+            content = testcases.assert_and_parse_html(
+                self, content, None, 'Response\'s content is not valid HTML:')
+            text = testcases.assert_and_parse_html(
+                self, text, None, 'Second argument is not valid HTML:')
+        self.assertEqual(
+            content.count(text), 0,
+            msg_prefix + "Response should not contain %s" % text_repr)
+
 
 @unittest.skipUnless(os.environ.get('WITH_SELENIUM', False),
                      "The WITH_SELENIUM env variable is not set.")

openstack_dashboard/api/swift.py

@@ -33,6 +33,7 @@ from openstack_dashboard.api import base
 
 LOG = logging.getLogger(__name__)
 FOLDER_DELIMITER = "/"
+CHUNK_SIZE = getattr(settings, 'SWIFT_FILE_TRANSFER_CHUNK_SIZE', 512 * 1024)
 # Swift ACL
 GLOBAL_READ_ACL = ".r:*"
 LIST_CONTENTS_ACL = ".rlistings"
@@ -326,10 +327,11 @@ def swift_delete_object(request, container_name, object_name):
     return True
 
 
-def swift_get_object(request, container_name, object_name, with_data=True):
+def swift_get_object(request, container_name, object_name, with_data=True,
+                     resp_chunk_size=CHUNK_SIZE):
     if with_data:
-        headers, data = swift_api(request).get_object(container_name,
+        headers, data = swift_api(request).get_object(
-                                                      object_name)
+            container_name, object_name, resp_chunk_size=resp_chunk_size)
     else:
         data = None
         headers = swift_api(request).head_object(container_name,

openstack_dashboard/dashboards/project/containers/tests.py

@@ -16,8 +16,10 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import copy
 import tempfile
 
+import django
 from django.core.files.uploadedfile import InMemoryUploadedFile  # noqa
 from django import http
 from django.utils import http as utils_http
@@ -337,19 +339,34 @@ class SwiftTests(test.TestCase):
         for container in self.containers.list():
             for obj in self.objects.list():
                 self.mox.ResetAll()  # mandatory in a for loop
-                api.swift.swift_get_object(IsA(http.HttpRequest),
+                obj = copy.copy(obj)
-                                           container.name,
+                _data = obj.data
-                                           obj.name).AndReturn(obj)
+
+                def make_iter():
+                    yield _data
+
+                obj.data = make_iter()
+                api.swift.swift_get_object(
+                    IsA(http.HttpRequest),
+                    container.name,
+                    obj.name,
+                    resp_chunk_size=api.swift.CHUNK_SIZE).AndReturn(obj)
                 self.mox.ReplayAll()
 
                 download_url = reverse(
                     'horizon:project:containers:object_download',
                     args=[container.name, obj.name])
                 res = self.client.get(download_url)
-                self.assertEqual(res.content, obj.data)
+
                 self.assertTrue(res.has_header('Content-Disposition'))
-                self.assertNotContains(res, INVALID_CONTAINER_NAME_1)
+                if django.VERSION >= (1, 5):
-                self.assertNotContains(res, INVALID_CONTAINER_NAME_2)
+                    self.assertEqual(b''.join(res.streaming_content), _data)
+                    self.assertNotContains(res, INVALID_CONTAINER_NAME_1)
+                    self.assertNotContains(res, INVALID_CONTAINER_NAME_2)
+                else:
+                    self.assertEqual(res.content, _data)
+                    self.assertNotContains(res, INVALID_CONTAINER_NAME_1)
+                    self.assertNotContains(res, INVALID_CONTAINER_NAME_2)
 
                 # Check that the returned Content-Disposition filename is well
                 # surrounded by double quotes and with commas removed

openstack_dashboard/dashboards/project/containers/views.py

@@ -22,6 +22,7 @@ Views for managing Swift containers.
 
 import os
 
+import django
 from django import http
 from django.utils.functional import cached_property  # noqa
 from django.utils.translation import ugettext_lazy as _
@@ -193,7 +194,8 @@ class UploadView(forms.ModalFormView):
 
 def object_download(request, container_name, object_path):
     try:
-        obj = api.swift.swift_get_object(request, container_name, object_path)
+        obj = api.swift.swift_get_object(request, container_name, object_path,
+                                         resp_chunk_size=swift.CHUNK_SIZE)
     except Exception:
         redirect = reverse("horizon:project:containers:index")
         exceptions.handle(request,
@@ -205,11 +207,18 @@ def object_download(request, container_name, object_path):
     if not os.path.splitext(obj.name)[1] and obj.orig_name:
         name, ext = os.path.splitext(obj.orig_name)
         filename = "%s%s" % (filename, ext)
-    response = http.HttpResponse()
+    # NOTE(tsufiev): StreamingHttpResponse class had been introduced in
+    # Django 1.5 specifically for the purpose streaming and/or transferring
+    # large files, it's less fragile than standard HttpResponse and should be
+    # used when available.
+    if django.VERSION >= (1, 5):
+        response = http.StreamingHttpResponse(obj.data)
+    else:
+        response = http.HttpResponse(obj.data)
     safe_name = filename.replace(",", "").encode('utf-8')
     response['Content-Disposition'] = 'attachment; filename="%s"' % safe_name
     response['Content-Type'] = 'application/octet-stream'
-    response.write(obj.data)
+    response['Content-Length'] = obj.bytes
     return response
 
 

openstack_dashboard/local/local_settings.py.example

@@ -256,6 +256,9 @@ IMAGE_RESERVED_CUSTOM_PROPERTIES = []
 API_RESULT_LIMIT = 1000
 API_RESULT_PAGE_SIZE = 20
 
+# The size of chunk in bytes for downloading objects from Swift
+SWIFT_FILE_TRANSFER_CHUNK_SIZE = 512 * 1024
+
 # Specify a maximum number of items to display in a dropdown.
 DROPDOWN_MAX_ITEMS = 30
 

openstack_dashboard/test/api_tests/swift_tests.py

@@ -123,19 +123,34 @@ class SwiftApiTests(test.APITestCase):
         self.assertEqual(len(objects), len(objs))
         self.assertFalse(more)
 
-    def test_swift_get_object_with_data(self):
+    def test_swift_get_object_with_data_non_chunked(self):
         container = self.containers.first()
         object = self.objects.first()
 
         swift_api = self.stub_swiftclient()
-        swift_api.get_object(container.name, object.name) \
+        swift_api.get_object(
-            .AndReturn([object, object.data])
+            container.name, object.name, resp_chunk_size=None
+        ).AndReturn([object, object.data])
 
         self.mox.ReplayAll()
 
-        obj = api.swift.swift_get_object(self.request,
+        obj = api.swift.swift_get_object(self.request, container.name,
-                                         container.name,
+                                         object.name, resp_chunk_size=None)
-                                         object.name)
+        self.assertEqual(object.name, obj.name)
+
+    def test_swift_get_object_with_data_chunked(self):
+        container = self.containers.first()
+        object = self.objects.first()
+
+        swift_api = self.stub_swiftclient()
+        swift_api.get_object(
+            container.name, object.name, resp_chunk_size=api.swift.CHUNK_SIZE
+        ).AndReturn([object, object.data])
+
+        self.mox.ReplayAll()
+
+        obj = api.swift.swift_get_object(
+            self.request, container.name, object.name)
         self.assertEqual(object.name, obj.name)
 
     def test_swift_get_object_without_data(self):