launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14510
[Merge] lp:~jtv/maas/commissioning-script-api into lp:maas
Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/commissioning-script-api into lp:maas with lp:~jtv/maas/more-make-file-upload as a prerequisite.
Commit message:
API for managing custom commissioning scripts.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/maas/commissioning-script-api/+merge/135927
As pre-imped with Gavin. I split various supporting changes out into separate branches, one of which is currently still awaiting review. I made that one a prerequisite so it won't pollute your diff, dear reviewer.
This API does some things differently than we're used to. It's simple enough that basic POST/GET/PUT/DELETE requests cover all functionality, and it uses file uploads to ensure that binary data comes across unchanged. Gavin and I decided that, since it shouldn't be too hard to adapt maascli to this usage, we should implement it this way first. If it turns out that we can't change maascli, we can start worrying about workarounds such as passing script contents as text.
I made the commissioning-scripts API admin-only. The user/admin distinction we've been using was between a "consumer" of a MAAS who gets nodes allocated to them, and an "owner" who manages the MAAS and its systems. I assumed that the owner may want to include credentials of some sort in the scripts, and for that reason I didn't even provide read-only visibility to non-admin users. Since this is a security boundary, there are specific tests.
Jeroen
--
https://code.launchpad.net/~jtv/maas/commissioning-script-api/+merge/135927
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/commissioning-script-api into lp:maas.
=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py 2012-11-23 12:08:48 +0000
+++ src/maasserver/api.py 2012-11-23 15:18:23 +0000
@@ -177,6 +177,8 @@
strip_domain,
)
from maasserver.utils.orm import get_one
+from metadataserver.fields import Bin
+from metadataserver.models import CommissioningScript
from piston.utils import rc
from provisioningserver.enum import POWER_TYPE
from provisioningserver.kernel_opts import KernelParameters
@@ -1737,6 +1739,88 @@
return HttpResponse("OK")
+def get_content_parameter(request):
+ """Get the "content" parameter from a CommissioningScript POST or PUT."""
+ content_file = get_mandatory_param(request.FILES, 'content')
+ return content_file.read()
+
+
+class CommissioningScriptsHandler(OperationsHandler):
+ """Management of custom commissioning scripts.
+
+ This functionality is only available to administrators.
+ """
+
+ update = delete = None
+
+ def read(self, request):
+ """List commissioning scripts."""
+ return [
+ script.name
+ for script in CommissioningScript.objects.all().order_by('name')]
+
+ def create(self, request):
+ """Create a new commissioning script.
+
+ Each commissioning script is identified by a unique name.
+
+ By convention the name should consist of a two-digit number, a dash,
+ and a brief descriptive identifier consisting only of ASCII
+ characters. You don't need to follow this convention, but not doing
+ so opens you up to risks w.r.t. encoding and ordering.
+
+ A commissioning node will run each of the scripts in lexicographical
+ order. There are no promises about how non-ASCII characters are
+ sorted, or even how upper-case letters are sorted relative to
+ lower-case letters. So where ordering matters, use unique numbers.
+
+ Scripts built into MAAS will have names starting with "00-maas" or
+ "99-maas" to ensure that they run first or last, respectively.
+
+ Usually a commissioning script will be just that, a script. Ideally a
+ script should be ASCII text to avoid any confusion over encoding. But
+ in some cases a commissioning script might consist of a binary tool
+ provided by a hardware vendor. Either way, the script gets passed to
+ the commissioning node in the exact form in which it was uploaded.
+
+ :param name: Unique identifying name for the script. Names should
+ follow the pattern of "25-burn-in-hard-disk" (all ASCII, and with
+ numbers greater than zero).
+ :param content: A script file, to be uploaded in binary form. Note:
+ this is not a normal parameter, but a file upload. Its filename
+ is ignored; MAAS will know it by the name you pass to the request.
+ """
+ name = get_mandatory_param(request.data, 'name')
+ content = Bin(get_content_parameter(request))
+ return CommissioningScript.objects.create(name=name, content=content)
+
+
+class CommissioningScriptHandler(OperationsHandler):
+ """A CommissioningScript.
+
+ This functionality is only available to administrators.
+ """
+ # Relies heavily on Piston's built-in CRUD implementations: GET,
+ # PUT, and DELETE are provided implicitly.
+
+ model = CommissioningScript
+ fields = ('name', 'content')
+
+ create = None
+
+ def read(self, request, name):
+ """Read a commissioning script."""
+ script = get_object_or_404(CommissioningScript, name=name)
+ return HttpResponse(script.content, content_type='application/binary')
+
+ def update(self, request, name):
+ """Update a commissioning script."""
+ content = Bin(get_content_parameter(request))
+ script = get_object_or_404(CommissioningScript, name=name)
+ script.content = content
+ script.save()
+
+
def describe(request):
"""Return a description of the whole MAAS API.
=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py 2012-11-23 15:18:22 +0000
+++ src/maasserver/tests/test_api.py 2012-11-23 15:18:23 +0000
@@ -109,7 +109,9 @@
from maastesting.djangotestcase import TransactionTestCase
from maastesting.fakemethod import FakeMethod
from maastesting.matchers import ContainsAll
+from maastesting.utils import sample_binary_data
from metadataserver.models import (
+ CommissioningScript,
NodeKey,
NodeUserData,
)
@@ -4402,6 +4404,130 @@
images=ANY, nodegroup=ANY)
+class AdminCommissioningScriptsAPITest(APIv10TestMixin, AdminLoggedInTestCase):
+ """Tests for `CommissioningScriptsHandler`."""
+
+ def get_url(self):
+ return reverse('commissioning_scripts_handler')
+
+ def test_GET_lists_commissioning_scripts(self):
+ # Use lower-case names. The database and the test may use
+ # different collation orders with different ideas about case
+ # sensitivity.
+ names = {factory.make_name('script').lower() for counter in range(5)}
+ for name in names:
+ factory.make_commissioning_script(name=name)
+
+ response = self.client.get(self.get_url())
+
+ self.assertEqual(
+ (httplib.OK, sorted(names)),
+ (response.status_code, json.loads(response.content)))
+
+ def test_POST_creates_commissioning_script(self):
+ # This uses Piston's built-in POST code, so there are no tests for
+ # corner cases (like "script already exists") here.
+ name = factory.make_name('script')
+ content = factory.getRandomString().encode('ascii')
+
+ # Every uploaded file also has a name. But this is completely
+ # unrelated to the name we give to the commissioning script.
+ response = self.client.post(
+ self.get_url(),
+ {
+ 'name': name,
+ 'content': factory.make_file_upload(content=content),
+ })
+ self.assertEqual(httplib.OK, response.status_code)
+
+ returned_script = json.loads(response.content)
+ self.assertEqual(
+ (name, content),
+ (returned_script['name'], returned_script['content']))
+
+ stored_script = CommissioningScript.objects.get(name=name)
+ self.assertEqual(content, stored_script.content)
+
+
+class CommissioningScriptsAPITest(APITestCase):
+
+ def get_url(self):
+ return reverse('commissioning_scripts_handler')
+
+ def test_GET_is_forbidden(self):
+ response = self.client.get(self.get_url())
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+ def test_POST_is_forbidden(self):
+ response = self.client.post(
+ self.get_url(),
+ {'name': factory.make_name('script')})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+
+class AdminCommissioningScriptAPITest(APIv10TestMixin, AdminLoggedInTestCase):
+ """Tests for `CommissioningScriptHandler`."""
+
+ def get_url(self, script_name):
+ return reverse('commissioning_script_handler', args=[script_name])
+
+ def test_GET_returns_script_contents(self):
+ script = factory.make_commissioning_script()
+ response = self.client.get(self.get_url(script.name))
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(script.content, response.content)
+
+ def test_GET_preserves_binary_data(self):
+ script = factory.make_commissioning_script(content=sample_binary_data)
+ response = self.client.get(self.get_url(script.name))
+ self.assertEqual(httplib.OK, response.status_code)
+ self.assertEqual(sample_binary_data, response.content)
+
+ def test_PUT_updates_contents(self):
+ old_content = b'old:%s' % factory.getRandomString().encode('ascii')
+ script = factory.make_commissioning_script(content=old_content)
+ new_content = b'new:%s' % factory.getRandomString().encode('ascii')
+
+ response = self.client.put(
+ self.get_url(script.name),
+ {'content': factory.make_file_upload(content=new_content)})
+ self.assertEqual(httplib.OK, response.status_code)
+
+ self.assertEqual(new_content, reload_object(script).content)
+
+ def test_DELETE_deletes_script(self):
+ script = factory.make_commissioning_script()
+ self.client.delete(self.get_url(script.name))
+ self.assertItemsEqual(
+ [],
+ CommissioningScript.objects.filter(name=script.name))
+
+
+class CommissioningScriptAPITest(APITestCase):
+
+ def get_url(self, script_name):
+ return reverse('commissioning_script_handler', args=[script_name])
+
+ def test_GET_is_forbidden(self):
+ # It's not inconceivable that commissioning scripts contain
+ # credentials of some sort. There is no need for regular users
+ # (consumers of the MAAS) to see these.
+ script = factory.make_commissioning_script()
+ response = self.client.get(self.get_url(script.name))
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+ def test_PUT_is_forbidden(self):
+ script = factory.make_commissioning_script()
+ response = self.client.put(
+ self.get_url(script.name), {'content': factory.getRandomString()})
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+ def test_DELETE_is_forbidden(self):
+ script = factory.make_commissioning_script()
+ response = self.client.put(self.get_url(script.name))
+ self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+
class TestDescribe(AnonAPITestCase):
"""Tests for the `describe` view."""
=== modified file 'src/maasserver/urls_api.py'
--- src/maasserver/urls_api.py 2012-11-22 13:27:50 +0000
+++ src/maasserver/urls_api.py 2012-11-23 15:18:23 +0000
@@ -20,6 +20,8 @@
AccountHandler,
api_doc,
BootImagesHandler,
+ CommissioningScriptHandler,
+ CommissioningScriptsHandler,
describe,
FilesHandler,
MAASHandler,
@@ -57,6 +59,10 @@
BootImagesHandler, authentication=api_auth)
tag_handler = RestrictedResource(TagHandler, authentication=api_auth)
tags_handler = RestrictedResource(TagsHandler, authentication=api_auth)
+commissioning_script_handler = AdminRestrictedResource(
+ CommissioningScriptHandler, authentication=api_auth)
+commissioning_scripts_handler = AdminRestrictedResource(
+ CommissioningScriptsHandler, authentication=api_auth)
# Admin handlers.
@@ -106,4 +112,10 @@
# API URLs for admin users.
urlpatterns += patterns('',
url(r'maas/$', maas_handler, name='maas_handler'),
+ url(
+ r'commissioning-scripts/$', commissioning_scripts_handler,
+ name='commissioning_scripts_handler'),
+ url(
+ r'commissioning-scripts/(?P<name>[^/]+)$',
+ commissioning_script_handler, name='commissioning_script_handler'),
)
