new constraints middleware

Adding a constraints middleware that allows different constraints for a
Swift-on-File storage policy.

Currently, Swift allows only one set of object naming and metadata rules
per cluster. As new storage policies are implemented, it is possible that
different storage back-ends have different set of rules on how objects should
be created for that specific storage system. Swift-on-File has different rules
as objects name are mapped to a POSIX filesystem directory structure.
For example: no names with double slashes or ending with slashes are allowed.

At first a solution was proposed to include a generic patch in the upstream
swift code, but after discussing with the Swift community it became clear that
it would be better to have a global set of constraints that covers the whole
cluster and if a specific storage policy has specific constraints than it
should have its own middleware.
Link to patch for reference: https://review.openstack.org/#/c/113325/

Change-Id: I323ead5d98bf5c087930ccf446d3e8d83075e584
Signed-off-by: Thiago da Silva <thiago@redhat.com>

setup.py

@@ -47,5 +47,9 @@ setup(
         'paste.app_factory': [
             'object=swiftonfile.swift.obj.server:app_factory',
         ],
+        'paste.filter_factory': [
+            'sof_constraints=swiftonfile.swift.common.middleware.'
+            'check_constraints:filter_factory',
+        ],
     },
 )

swiftonfile/swift/common/constraints.py

@@ -15,8 +15,8 @@
 
 import os
 from swift.common.swob import HTTPBadRequest
-import swift.common.constraints
 
+SOF_MAX_CONTAINER_NAME_LENGTH = 255
 SOF_MAX_OBJECT_NAME_LENGTH = 221
 # Why 221 ?
 # The longest filename supported by XFS in 255.
@@ -38,17 +38,12 @@ def validate_obj_name_component(obj):
         return 'cannot be . or ..'
     return ''
 
-# Store Swift's check_object_creation method to be invoked later
-swift_check_object_creation = swift.common.constraints.check_object_creation
 
-
-# Define our new one which invokes the original
-def sof_check_object_creation(req, object_name):
+def check_object_creation(req, object_name):
     """
     Check to ensure that everything is alright about an object to be created.
-    Monkey patches swift.common.constraints.check_object_creation, invoking
-    the original, and then adding an additional check for individual object
-    name components.
+    Swift-on-File has extra constraints on object names regarding the
+    length of directories and the actual file name created on the Filesystem.
 
     :param req: HTTP request object
     :param object_name: name of object to be created
@@ -58,17 +53,14 @@ def sof_check_object_creation(req, object_name):
     :raises HTTPBadRequest: missing or bad content-type header, or
                             bad metadata
     """
-    # Invoke Swift's method
-    ret = swift_check_object_creation(req, object_name)
-
     # SoF's additional checks
-    if ret is None:
-        for obj in object_name.split(os.path.sep):
-            reason = validate_obj_name_component(obj)
-            if reason:
-                bdy = 'Invalid object name "%s", component "%s" %s' \
-                    % (object_name, obj, reason)
-                ret = HTTPBadRequest(body=bdy,
-                                     request=req,
-                                     content_type='text/plain')
+    ret = None
+    for obj in object_name.split(os.path.sep):
+        reason = validate_obj_name_component(obj)
+        if reason:
+            bdy = 'Invalid object name "%s", component "%s" %s' \
+                % (object_name, obj, reason)
+            ret = HTTPBadRequest(body=bdy,
+                                 request=req,
+                                 content_type='text/plain')
     return ret

swiftonfile/swift/common/middleware/check_constraints.py

@@ -0,0 +1,104 @@
+# Copyright (c) 2012-2014 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""
+The ``sof_constraints`` middleware should be added to the pipeline in your
+``/etc/swift/proxy-server.conf`` file, and a mapping of storage policies
+using the swiftonfile object server should be listed in the 'policies'
+variable in the filter section.
+
+The swiftonfile constraints contains additional checks to make sure object
+names conform with POSIX filesystems file and directory naming limitations
+
+For example::
+
+    [pipeline:main]
+    pipeline = catch_errors sof_constraints cache proxy-server
+
+    [filter:sof_constraints]
+    use = egg:swift#sof_constraints
+    policies=swiftonfile,gold
+"""
+
+from urllib import unquote
+from swift.common.utils import get_logger
+from swift.common.swob import Request, HTTPBadRequest
+from swift.proxy.controllers.base import get_container_info
+from swift.common.storage_policy import POLICIES
+from swiftonfile.swift.common import constraints
+from swiftonfile.swift.common.constraints import check_object_creation \
+    as sof_check_object_creation
+
+
+class CheckConstraintsMiddleware(object):
+
+    def __init__(self, app, conf):
+        self.app = app
+        self.logger = get_logger(conf, log_route='constraints')
+        self.swift_dir = conf.get('swift_dir', '/etc/swift')
+        self.policies = conf.get('policies', '')
+
+    def __call__(self, env, start_response):
+        request = Request(env)
+
+        if request.method == 'PUT':
+            try:
+                version, account, container, obj = \
+                    request.split_path(1, 4, True)
+            except ValueError:
+                return self.app(env, start_response)
+
+            # check container creation request
+            if account and container and not obj:
+                policy_name = request.headers.get('X-Storage-Policy', '')
+                default_policy = POLICIES.default.name
+                if (policy_name in self.policies) or \
+                   (policy_name == '' and default_policy in self.policies):
+
+                    container = unquote(container)
+                    if len(container) > constraints. \
+                            SOF_MAX_CONTAINER_NAME_LENGTH:
+                        resp = HTTPBadRequest(request=request)
+                        resp.body = \
+                            'Container name length of %d longer than %d' % \
+                            (len(container),
+                                constraints.SOF_MAX_CONTAINER_NAME_LENGTH)
+                        return resp(env, start_response)
+            elif account and container and obj:
+                # check object creation request
+                obj = unquote(obj)
+
+                container_info = get_container_info(
+                    env, self.app)
+                policy = POLICIES.get_by_index(
+                    container_info['storage_policy'])
+
+                if policy.name in self.policies:
+                    error_response = sof_check_object_creation(request, obj)
+                    if error_response:
+                        self.logger.warn("returning error: %s", error_response)
+                        return error_response(env, start_response)
+
+        return self.app(env, start_response)
+
+
+def filter_factory(global_conf, **local_conf):
+    conf = global_conf.copy()
+    conf.update(local_conf)
+
+    def check_constraints_filter(app):
+        return CheckConstraintsMiddleware(app, conf)
+
+    return check_constraints_filter

swiftonfile/swift/obj/diskfile.py

@@ -559,9 +559,9 @@ class DiskFile(object):
     Manage object files on disk.
 
     Object names ending or beginning with a '/' as in /a, a/, /a/b/,
-    etc, or object names with multiple consecutive slahes, like a//b,
-    are not supported.  The proxy server's contraints filter
-    swiftonfile.common.constrains.sof_check_object_creation() should
+    etc, or object names with multiple consecutive slashes, like a//b,
+    are not supported.  The proxy server's constraints filter
+    swiftonfile.common.constrains.check_object_creation() should
     reject such requests.
 
     :param mgr: associated on-disk manager instance

swiftonfile/swift/obj/server.py

@@ -15,9 +15,9 @@
 
 """ Object Server for Gluster for Swift """
 
-import swiftonfile.swift.common.constraints    # noqa
 from swift.common.swob import HTTPConflict
 from swift.common.utils import public, timing_stats
+from swift.common.request_helpers import get_name_and_placement
 from swiftonfile.swift.common.exceptions import AlreadyExistsAsFile, \
     AlreadyExistsAsDir
 from swift.common.request_helpers import split_and_validate_path
@@ -25,6 +25,7 @@ from swift.common.request_helpers import split_and_validate_path
 from swift.obj import server
 
 from swiftonfile.swift.obj.diskfile import DiskFileManager
+from swiftonfile.swift.common.constraints import check_object_creation
 
 
 class ObjectController(server.ObjectController):
@@ -63,8 +64,15 @@ class ObjectController(server.ObjectController):
     @timing_stats()
     def PUT(self, request):
         try:
-            server.check_object_creation = \
-                swiftonfile.swift.common.constraints.sof_check_object_creation
+            device, partition, account, container, obj, policy_idx = \
+                get_name_and_placement(request, 5, 5, True)
+
+            # check swiftonfile constraints first
+            error_response = check_object_creation(request, obj)
+            if error_response:
+                return error_response
+
+            # now call swift's PUT method
             return server.ObjectController.PUT(self, request)
         except (AlreadyExistsAsFile, AlreadyExistsAsDir):
             device = \

test/unit/common/middleware/test_constraints.py

@@ -0,0 +1,186 @@
+# Copyright (c) 2014 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#    http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
+# implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+import unittest
+from swift.common.swob import Request, Response
+from swiftonfile.swift.common.middleware import check_constraints
+from mock import Mock, patch
+from contextlib import nested
+
+
+class FakeApp(object):
+
+    def __call__(self, env, start_response):
+        return Response(body="OK")(env, start_response)
+
+
+def check_object_creation(req, object_name):
+        return
+
+
+class TestConstraintsMiddleware(unittest.TestCase):
+
+    """ Tests for common.middleware.constraints.check_constraints """
+
+    def setUp(self):
+        self.conf = {
+            'policies': 'swiftonfile,cephfs-policy'}
+
+        self.container1_info_mock = Mock()
+        self.container1_info_mock.return_value = {'status': 0,
+            'sync_key': None, 'storage_policy': '0', 'meta': {},
+            'cors': {'allow_origin': None, 'expose_headers': None,
+            'max_age': None}, 'sysmeta': {}, 'read_acl': None,
+            'object_count': None, 'write_acl': None, 'versions': None,
+            'bytes': None}
+
+        self.container2_info_mock = Mock()
+        self.container2_info_mock.return_value = {'status': 0,
+            'sync_key': None, 'storage_policy': '2', 'meta': {},
+            'cors': {'allow_origin': None, 'expose_headers': None,
+            'max_age': None}, 'sysmeta': {}, 'read_acl': None,
+            'object_count': None, 'write_acl': None, 'versions': None,
+            'bytes': None}
+
+        self.policies_mock = Mock()
+        self.sof_policy_mock = Mock()
+        self.sof_policy_mock.name = 'swiftonfile'
+        attrs = {'get_by_index.return_value': self.sof_policy_mock }
+        self.policies_mock.configure_mock(**attrs)
+
+        self.test_check = check_constraints.filter_factory(
+            self.conf)(FakeApp())
+
+    def test_GET(self):
+        path = '/V1.0/a/c/o'
+        resp = Request.blank(path, environ={'REQUEST_METHOD': 'GET'}
+                             ).get_response(self.test_check)
+        self.assertEquals(resp.status_int, 200)
+
+    def test_PUT_container(self):
+        path = '/V1.0/a/c'
+        resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                             ).get_response(self.test_check)
+        self.assertEquals(resp.status_int, 200)
+
+    def test_PUT_object_with_double_slashes(self):
+        path = '/V1.0/a/c2//o'
+
+        with nested(
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "get_container_info", self.container2_info_mock),
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "POLICIES", self.policies_mock)):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 400)
+            self.assertTrue('Invalid object name' in resp.body)
+            self.assertTrue('cannot begin, end, or have' in resp.body)
+
+    def test_PUT_object_end_with_slashes(self):
+        path = '/V1.0/a/c2/o/'
+
+        with nested(
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "get_container_info", self.container2_info_mock),
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "POLICIES", self.policies_mock)):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 400)
+            self.assertTrue('Invalid object name' in resp.body)
+            self.assertTrue('cannot begin, end, or have' in resp.body)
+
+    def test_PUT_object_named_dot(self):
+        path = '/V1.0/a/c2/.'
+
+        with nested(
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "get_container_info", self.container2_info_mock),
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "POLICIES", self.policies_mock)):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 400)
+            self.assertTrue('Invalid object name' in resp.body)
+            self.assertTrue('cannot be . or ..' in resp.body)
+
+    def test_PUT_container_with_long_names(self):
+        longname = 'c' * 256
+        path = '/V1.0/a/' + longname
+        resp = Request.blank(path, method='PUT',
+                             headers={'X-Storage-Policy': 'swiftonfile'}
+                             ).get_response(self.test_check)
+        self.assertEquals(resp.status_int, 400)
+
+        # test case where storage policy is not defined in header and 
+        # container would be created in default policy, which happens to be
+        # a swiftonfile policy
+        default_policies_mock = Mock()
+        sof_policy_mock = Mock()
+        sof_policy_mock.name = 'swiftonfile'
+        attrs = {'default.return_value': self.sof_policy_mock }
+        default_policies_mock.configure_mock(**attrs)
+        with patch("swiftonfile.swift.common.middleware.check_constraints."
+                   "POLICIES", default_policies_mock):
+          resp = Request.blank(path, method='PUT').get_response(self.test_check)
+          self.assertEquals(resp.status_int, 400)
+
+    def test_PUT_object_with_long_names(self):
+        for i in (220,221):
+            longname = 'o' * i
+            path = '/V1.0/a/c2/' + longname
+
+            with nested(
+                    patch("swiftonfile.swift.common.middleware."
+                          "check_constraints.get_container_info",
+                          self.container2_info_mock),
+                    patch("swiftonfile.swift.common.middleware."
+                          "check_constraints.POLICIES", self.policies_mock)):
+                resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                     ).get_response(self.test_check)
+                self.assertEquals(resp.status_int, 200)
+
+        longname = 'o' * 222
+        path = '/V1.0/a/c2/' + longname
+
+        with nested(
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "get_container_info", self.container2_info_mock),
+                patch("swiftonfile.swift.common.middleware.check_constraints."
+                      "POLICIES", self.policies_mock)):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 400)
+            self.assertTrue('too long' in resp.body)
+
+    def test_PUT_object_with_policy0(self):
+        path = '/V1.0/a/c1//o'
+
+        with patch("swiftonfile.swift.common.middleware."
+                   "check_constraints.get_container_info",
+                   self.container1_info_mock):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 200)
+
+        longname = 'o' * 222
+        path = '/V1.0/a/c2/' + longname
+
+        with patch("swiftonfile.swift.common.middleware.check_constraints."
+                   "get_container_info", self.container1_info_mock):
+            resp = Request.blank(path, environ={'REQUEST_METHOD': 'PUT'}
+                                 ).get_response(self.test_check)
+            self.assertEquals(resp.status_int, 200)

test/unit/common/test_constraints.py

@@ -40,9 +40,7 @@ class TestConstraints(unittest.TestCase):
         self.assertTrue(cnt.validate_obj_name_component('..'))
         self.assertTrue(cnt.validate_obj_name_component(''))
 
-    def test_sof_check_object_creation(self):
-        with patch('swiftonfile.swift.common.constraints.swift_check_object_creation',
-                   mock_check_object_creation):
-            req = Mock()
-            req.headers = []
-            self.assertFalse(cnt.sof_check_object_creation(req, 'dir/z'))
+    def test_check_object_creation(self):
+        req = Mock()
+        req.headers = []
+        self.assertFalse(cnt.check_object_creation(req, 'dir/z'))