Check if device name is valid when adding to the ring

Currently device names can be empty or start and/or end with spaces.
This can create unexpected results, for example these three commands
are all valid:

swift-ring-builder account.builder add "r1z1-127.0.0.1:6000/" 1
swift-ring-builder account.builder add "r1z1-127.0.0.1:6000/sda " 1
swift-ring-builder account.builder add "r1z1-127.0.0.1:6000/ meta" 1

This patch validates device names and prevents empty names or names
starting and/or ending with spaces.

Also fixed the test "test_warn_at_risk" - the test passed if the
exception was not raised.

Closes-Bug: 1438579

Change-Id: I811b0eae7db503279e6429d985275bbab8b29c9f

3 files changed, 48 insertions, 2 deletions

diff --git a/swift/cli/ringbuilder.py b/swift/cli/ringbuilder.py
index 16315627d..eac586e26 100755
--- a/swift/cli/ringbuilder.py
+++ b/swift/cli/ringbuilder.py
@@ -34,7 +34,7 @@ from swift.common.ring.utils import validate_args, \
     validate_and_normalize_ip, build_dev_from_opts, \
     parse_builder_ring_filename_args, parse_search_value, \
     parse_search_values_from_opts, parse_change_values_from_opts, \
-    dispersion_report
+    dispersion_report, validate_device_name
 from swift.common.utils import lock_parent_directory
 
 MAJOR_VERSION = 1
@@ -220,6 +220,9 @@ def _parse_add_values(argvish):
             while i < len(rest) and rest[i] != '_':
                 i += 1
             device_name = rest[1:i]
+            if not validate_device_name(device_name):
+                raise ValueError('Invalid device name')
+
             rest = rest[i:]
 
             meta = ''
diff --git a/swift/common/ring/utils.py b/swift/common/ring/utils.py
index 17186dee8..b00ef825d 100644
--- a/swift/common/ring/utils.py
+++ b/swift/common/ring/utils.py
@@ -515,6 +515,9 @@ def build_dev_from_opts(opts):
         (opts.replication_ip or opts.ip))
     replication_port = opts.replication_port or opts.port
 
+    if not validate_device_name(opts.device):
+        raise ValueError('Invalid device name')
+
     return {'region': opts.region, 'zone': opts.zone, 'ip': ip,
             'port': opts.port, 'device': opts.device, 'meta': opts.meta,
             'replication_ip': replication_ip,
@@ -569,3 +572,10 @@ def get_tier_name(tier, builder):
         device = builder.devs[tier[3]] or {}
         return "r%sz%s-%s/%s" % (tier[0], tier[1], tier[2],
                                  device.get('device', 'IDd%s' % tier[3]))
+
+
+def validate_device_name(device_name):
+    return not (
+        device_name.startswith(' ') or
+        device_name.endswith(' ') or
+        len(device_name) == 0)
diff --git a/test/unit/cli/test_ringbuilder.py b/test/unit/cli/test_ringbuilder.py
index 8b96f2516..246f282f3 100644
--- a/test/unit/cli/test_ringbuilder.py
+++ b/test/unit/cli/test_ringbuilder.py
@@ -1711,10 +1711,43 @@ class TestCommands(unittest.TestCase, RunSwiftRingBuilderMixin):
         ring.devs[0]['weight'] = 10
         ring.save(self.tmpfile)
         argv = ["", self.tmpfile, "rebalance"]
+        err = None
         try:
             ringbuilder.main(argv)
         except SystemExit as e:
-            self.assertEquals(e.code, 1)
+            err = e
+        self.assertEquals(err.code, 1)
+
+    def test_invalid_device_name(self):
+        self.create_sample_ring()
+        for device_name in ["", " ", " sda1", "sda1 ", " meta "]:
+            err = 0
+
+            argv = ["",
+                    self.tmpfile,
+                    "add",
+                    "r1z1-127.0.0.1:6000/%s" % device_name,
+                    "1"]
+            try:
+                ringbuilder.main(argv)
+            except SystemExit as exc:
+                err = exc
+            self.assertEquals(err.code, 2)
+
+            argv = ["",
+                    self.tmpfile,
+                    "add",
+                    "--region", "1",
+                    "--zone", "1",
+                    "--ip", "127.0.0.1",
+                    "--port", "6000",
+                    "--device", device_name,
+                    "--weight", "100"]
+            try:
+                ringbuilder.main(argv)
+            except SystemExit as exc:
+                err = exc
+            self.assertEquals(err.code, 2)
 
 
 class TestRebalanceCommand(unittest.TestCase, RunSwiftRingBuilderMixin):