Merge ~ltrager/maas:lp1835289 into maas:master

[Lee Trager]

LP: #1835289 - Fix error handling on the commissioning API.

The commissioning API was returning an exception instead or raising it. This
caused a 500 internal error with no readable error message. Fixing this
revealed the commissioning API cannot be used with embedded script YAML as
the commissioning API sets the script_type to commissioning. The form now
allows fields to be set as long as they match what is in the script YAML.

[Newell Jensen (newell-jensen)]

Since this endpoint is deprecated, why are we not raising a message that says this when an admin tries using it?

review: Needs Information

[Lee Trager (ltrager)]

When I added the node-scripts API end point we decided that the commissioning-scripts end point had to be kept around for API compatibility. If we ever bump the API version we'd remove it.

[Newell Jensen (newell-jensen)]

I think simply raising a message letting the user know they are using a deprecated endpoint and pointing them to its successor is a better overall user experience and gets them using all the bells and whistles of supported endpoint. Having a message doesn't break API compatibility as those endpoints are still reachable.

Not going to block you on this though but would be good to get another team members opinion on this. As we will be bumping up the API version at some point in the future and cleaning all of this up, seems like that would have been the easier coarse of action.

review: Approve

c939af6... by Lee Trager on 2019-07-10

Merge branch 'master' into lp1835289

324b243... by Lee Trager on 2019-07-10

Add deprecation warning

[Lee Trager (ltrager)]

As discussed in the SU I added a warning that the commissioning-scripts endpoint is deprecated. Unfortunatly I can only do this with the output when a commissioning script is created. reading or updating a script does not allow me to add the message without breaking API compatibility.

diff --git a/src/maasserver/api/commissioning_scripts.py b/src/maasserver/api/commissioning_scripts.py
index 16791fc..906e3ee 100644
--- a/src/maasserver/api/commissioning_scripts.py
+++ b/src/maasserver/api/commissioning_scripts.py
@@ -1,4 +1,4 @@
-# Copyright 2014-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """API handlers: `CommissioningScript`."""
@@ -92,11 +92,14 @@ class CommissioningScriptsHandler(OperationsHandler):
             return {
                 'name': script.name,
                 'content': b64encode(script.script.data.encode()),
+                'deprecated': (
+                    'The commissioning-scripts endpoint is deprecated. '
+                    'Please use the node-scripts endpoint.'),
                 'resource_uri': reverse(
                     'commissioning_script_handler', args=[script.name]),
             }
         else:
-            return MAASAPIValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls):
@@ -142,7 +145,7 @@ class CommissioningScriptHandler(OperationsHandler):
             form.save(request)
             return rc.ALL_OK
         else:
-            return MAASAPIValidationError(form.errors)
+            raise MAASAPIValidationError(form.errors)
 
     @classmethod
     def resource_uri(cls, script=None):
diff --git a/src/maasserver/api/tests/test_commissioning.py b/src/maasserver/api/tests/test_commissioning.py
index 1cfa07b..49dcce2 100644
--- a/src/maasserver/api/tests/test_commissioning.py
+++ b/src/maasserver/api/tests/test_commissioning.py
@@ -1,4 +1,4 @@
-# Copyright 2013-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2013-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the commissioning-related portions of the MAAS API."""
@@ -74,6 +74,22 @@ class AdminCommissioningScriptsAPITest(APITestCase.ForAdmin):
         self.assertEqual(content, stored_script.script.data)
         self.assertEqual(SCRIPT_TYPE.COMMISSIONING, stored_script.script_type)
 
+    def test_POST_creates_errors(self):
+        content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        response = self.client.post(
+            self.get_url(),
+            {
+                'name': factory.make_name('name'),
+                'content': factory.make_file_upload(content=content.encode()),
+            })
+        self.assertThat(response, HasStatusCode(http.client.BAD_REQUEST))
+
+        ret = json_load_bytes(response.content)
+        self.assertDictEqual(
+            {'name': ['May not override values defined in embedded YAML.']},
+            ret)
+
 
 class CommissioningScriptsAPITest(APITestCase.ForUser):
 
@@ -118,6 +134,28 @@ class AdminCommissioningScriptAPITest(APITestCase.ForAdmin):
         self.assertEqual(
             new_content.decode('utf-8'), reload_object(script).script.data)
 
+    def test_PUT_errors(self):
+        old_content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        old_content = old_content.encode('ascii')
+        script = factory.make_Script(
+            script_type=SCRIPT_TYPE.COMMISSIONING, script=old_content)
+        new_content = factory.make_script_content(
+            yaml_content={'name': factory.make_name('name')})
+        new_content = new_content.encode('ascii')
+
+        response = self.client.put(
+            self.get_url(script.name),
+            {
+                'name': factory.make_name('name'),
+                'content': factory.make_file_upload(content=new_content)})
+        self.assertThat(response, HasStatusCode(http.client.BAD_REQUEST))
+
+        ret = json_load_bytes(response.content)
+        self.assertDictEqual(
+            {'name': ['May not override values defined in embedded YAML.']},
+            ret)
+
     def test_DELETE_deletes_script(self):
         script = factory.make_Script(script_type=SCRIPT_TYPE.COMMISSIONING)
         self.client.delete(self.get_url(script.name))
diff --git a/src/maasserver/forms/script.py b/src/maasserver/forms/script.py
index da95d3c..e32e6fc 100644
--- a/src/maasserver/forms/script.py
+++ b/src/maasserver/forms/script.py
@@ -173,6 +173,73 @@ class ScriptForm(ModelForm):
             valid = False
         return valid
 
+    def _clean_script(self, parsed_yaml):
+        """Clean script data and validate input."""
+        # Tags and timeout may not be updated from new embedded YAML. This
+        # allows users to receive updated scripts from an upstream maintainer,
+        # such as Canonical, while maintaining user defined tags and timeout.
+
+        # Tags must be a comma seperated string for the form.
+        tags = parsed_yaml.pop('tags', None)
+        if (tags is not None and self.instance.id is None and
+                'tags' not in self.data):
+            tags_valid = True
+            if isinstance(tags, str):
+                self.data['tags'] = tags
+            elif isinstance(tags, list):
+                for tag in tags:
+                    if not isinstance(tag, str):
+                        tags_valid = False
+                        continue
+                if tags_valid:
+                    self.data['tags'] = ','.join(tags)
+            else:
+                tags_valid = False
+            if not tags_valid:
+                set_form_error(
+                    self, 'tags',
+                    'Embedded tags must be a string of comma seperated '
+                    'values, or a list of strings.')
+
+        # Timeout must be a string for the form.
+        timeout = parsed_yaml.pop('timeout', None)
+        if (timeout is not None and self.instance.id is None and
+                'timeout' not in self.data):
+            self.data['timeout'] = str(timeout)
+
+        # Packages and for_hardware must be a JSON string for the form.
+        for key in ['packages', 'for_hardware']:
+            value = parsed_yaml.pop(key, None)
+            if value is not None and key not in self.data:
+                self.data[key] = json.dumps(value)
+
+        for key, value in parsed_yaml.items():
+            if key in self.fields:
+                error = False
+                if key not in self.data:
+                    self.data[key] = value
+                elif key == 'script_type':
+                    # The deprecated Commissioning API always sets the
+                    # script_type to commissioning as it has always only
+                    # accepted commissioning scripts while the form sets
+                    # the default type to testing. If the YAML matches the
+                    # type allow it.
+                    try:
+                        if (translate_script_type(value) !=
+                                translate_script_type(self.data[key])):
+                            error = True
+                    except ValidationError:
+                        error = True
+                elif value != self.data[key]:
+                    # Only allow form data for fields defined in the YAML if
+                    # the data matches.
+                    error = True
+
+                if error:
+                    set_form_error(
+                        self, key,
+                        'May not override values defined in embedded YAML.')
+
     def _read_script(self):
         """Read embedded YAML configuration in a script.
 
@@ -228,52 +295,7 @@ class ScriptForm(ModelForm):
         self.instance.results = parsed_yaml.pop('results', {})
         self.instance.parameters = parsed_yaml.pop('parameters', {})
 
-        # Tags and timeout may not be updated from new embedded YAML. This
-        # allows users to receive updated scripts from an upstream maintainer,
-        # such as Canonical, while maintaining user defined tags and timeout.
-
-        # Tags must be a comma seperated string for the form.
-        tags = parsed_yaml.pop('tags', None)
-        if (tags is not None and self.instance.id is None and
-                'tags' not in self.data):
-            tags_valid = True
-            if isinstance(tags, str):
-                self.data['tags'] = tags
-            elif isinstance(tags, list):
-                for tag in tags:
-                    if not isinstance(tag, str):
-                        tags_valid = False
-                        continue
-                if tags_valid:
-                    self.data['tags'] = ','.join(tags)
-            else:
-                tags_valid = False
-            if not tags_valid:
-                set_form_error(
-                    self, 'tags',
-                    'Embedded tags must be a string of comma seperated '
-                    'values, or a list of strings.')
-
-        # Timeout must be a string for the form.
-        timeout = parsed_yaml.pop('timeout', None)
-        if (timeout is not None and self.instance.id is None and
-                'timeout' not in self.data):
-            self.data['timeout'] = str(timeout)
-
-        # Packages and for_hardware must be a JSON string for the form.
-        for key in ['packages', 'for_hardware']:
-            value = parsed_yaml.pop(key, None)
-            if value is not None and key not in self.data:
-                self.data[key] = json.dumps(value)
-
-        for key, value in parsed_yaml.items():
-            if key in self.fields:
-                if key not in self.data:
-                    self.data[key] = value
-                else:
-                    set_form_error(
-                        self, key,
-                        'May not override values defined in embedded YAML.')
+        self._clean_script(parsed_yaml)
 
     def clean_packages(self):
         if self.cleaned_data['packages'] == '':
diff --git a/src/maasserver/forms/tests/test_script.py b/src/maasserver/forms/tests/test_script.py
index b67a568..859d802 100644
--- a/src/maasserver/forms/tests/test_script.py
+++ b/src/maasserver/forms/tests/test_script.py
@@ -622,6 +622,19 @@ class TestScriptForm(MAASServerTestCase):
         })
         self.assertFalse(form.is_valid())
 
+    def test__user_option_can_match_yaml_value(self):
+        name = factory.make_name('name')
+        script_type = factory.pick_choice(SCRIPT_TYPE_CHOICES)
+        form = ScriptForm(data={
+            'name': name,
+            'script_type': script_type,
+            'script': factory.make_script_content({
+                'name': name,
+                'script_type': script_type,
+            })
+        })
+        self.assertTrue(form.is_valid())
+
     def test__errors_on_bad_yaml(self):
         form = ScriptForm(data={
             'name': factory.make_name('name'),