Merge pull request #666 from cderici/improve-constraint-parsing

#666

#### Description

Since the constraints are parsed at the client side, we might as well validate the constraints before sending, instead of relying on the provider's capability of handling invalid constraints. In #661 it is reported that an invalid constraint is not raising an exception on MAAS.

Fixes #661 

#### QA Steps

```sh
tox -e py3 -- tests/unit/test_constraints.py
```

or you should see an error when you try to use an invalid constraint (e.g. `root-disk>=16G`)

```sh
Python 3.9.7 (default, Sep 10 2021, 14:59:43) 
[GCC 11.2.0] on linux
>>> import juju
>>> import juju.constraints
>>> juju.constraints.parse('tags=scalebot-dev-controller arch=amd64 root-disk>=16G')
Traceback (most recent call last):
 File "<stdin>", line 1, in <module>
 File "/home/caner/work/cderici/python-libjuju/juju/constraints.py", line 79, in parse
 normalized_constraints[normalize_key(k)] = normalize_list_value(v) if\n File "/home/caner/work/cderici/python-libjuju/juju/constraints.py", line 95, in normalize_key
 raise Exception("unknown constraint in %s" % orig_key)
Exception: unknown constraint in root-disk>
>>> 
```

#### Notes & Discussion

This is fundamentally not the ideal way of handling the constraints, which is also related to https://bugs.launchpad.net/juju/+bug/1645402, however, as long as the clients are responsible for parsing the constraints, it makes sense to avoid any unnecessary api calls due to invalid constraints.

Author: jujubot

juju/constraints.py

@@ -32,6 +32,25 @@
     "Y": 1024 ** 6
 }
 
+# List of supported constraint keys, see
+# http://github.com/cderici/juju/blob/2.9/core/constraints/constraints.go#L20-L39
+SUPPORTED_KEYS = [
+    "arch",
+    "container",
+    "cpu_cores",
+    "cores",
+    "cpu_power",
+    "mem",
+    "root_disk",
+    "root_disk_source",
+    "tags",
+    "instance_role",
+    "instance_type",
+    "spaces",
+    "virt_type",
+    "zones",
+    "allocate_public_ip"]
+
 LIST_KEYS = {'tags', 'spaces'}
 
 SNAKE1 = re.compile(r'(.)([A-Z][a-z]+)')
@@ -51,24 +70,29 @@ def parse(constraints):
         # Fowards compatibilty: already parsed
         return constraints
 
-    constraints = {
-        normalize_key(k): (
-            normalize_list_value(v) if k in LIST_KEYS else
-            normalize_value(v)
-        ) for k, v in [s.split("=") for s in constraints.split(" ")]}
+    normalized_constraints = {}
+    for s in constraints.split(" "):
+        if "=" not in s:
+            raise Exception("malformed constraint %s" % s)
+
+        k, v = s.split("=")
+        normalized_constraints[normalize_key(k)] = normalize_list_value(v) if\
+            k in LIST_KEYS else normalize_value(v)
 
-    return constraints
+    return normalized_constraints
 
 
-def normalize_key(key):
-    key = key.strip()
+def normalize_key(orig_key):
+    key = orig_key.strip()
 
     key = key.replace("-", "_")  # Our _client lib wants "_" in place of "-"
 
     # Convert camelCase to snake_case
     key = SNAKE1.sub(r'\1_\2', key)
     key = SNAKE2.sub(r'\1_\2', key).lower()
 
+    if key not in SUPPORTED_KEYS:
+        raise Exception("unknown constraint in %s" % orig_key)
     return key
 
 

tests/unit/test_constraints.py

@@ -20,11 +20,13 @@ def test_mem_regex(self):
     def test_normalize_key(self):
         _ = constraints.normalize_key
 
-        self.assertEqual(_("test-key"), "test_key")
-        self.assertEqual(_("test-key  "), "test_key")
-        self.assertEqual(_("  test-key"), "test_key")
-        self.assertEqual(_("TestKey"), "test_key")
-        self.assertEqual(_("testKey"), "test_key")
+        self.assertEqual(_("root-disk"), "root_disk")
+        self.assertEqual(_("root-disk  "), "root_disk")
+        self.assertEqual(_("  root-disk"), "root_disk")
+        self.assertEqual(_("RootDisk"), "root_disk")
+        self.assertEqual(_("rootDisk"), "root_disk")
+
+        self.assertRaises(Exception, lambda: _("not-one-of-the-supported-keys"))
 
     def test_normalize_val(self):
         _ = constraints.normalize_value
@@ -53,13 +55,16 @@ def test_parse_constraints(self):
         )
 
         self.assertEqual(
-            _("mem=10G foo=bar,baz tags=tag1 spaces=space1,space2"),
+            _("mem=10G zones=bar,baz tags=tag1 spaces=space1,space2"),
             {"mem": 10 * 1024,
-             "foo": "bar,baz",
+             "zones": "bar,baz",
              "tags": ["tag1"],
              "spaces": ["space1", "space2"]}
         )
 
+        self.assertRaises(Exception, lambda: _("root-disk>16G"))
+        self.assertRaises(Exception, lambda: _("root-disk>=16G"))
+
     def test_parse_storage_constraint(self):
         _ = constraints.parse_storage_constraint