launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/cobbler-exception into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/cobbler-exception/+merge/98148

This branch parses Cobbler exceptions into more meaningful ProvisioningError exceptions.  Both are derived from xmlrpclib.Fault and so contain a numerical faultCode as well as an error text.  But ProvisioningError has more detailed faultCode numbers.  If we need to distinguish more kinds of Cobbler exceptions, we can extend the enum that specifies these error codes (PSERV_FAULT) and extend the parsing code that figures out the Cobbler exception.  It's a bit hackish, but that's what you get for trying to deal with Cobbler.

A note on xmlrpc and type information.  Cobbler likes to raise an exception called CX, which has no faultCode but arrives at pserv (on the other side of the xmlrpc interface) in the form of an xmlrpclib Fault anyway.  The faultCode is simply always 1.

Something similar will happen to ProvisioningError: maasserver won't see it as its own type of exception, and so won't be able to handle it as such.  But ProvisioningError provides a meaningful Fault code, and maasserver can differentiate its handling of the exception based on that code.

I had originally designed this as a context manager, called CobblerCatcher.  According to the PEP for context managers, however, raising an exception from __exit__ is the wrong thing to do.  The current documentation actually only says that it's bad to re-raise exc_val, but better safe than sorry.
-- 
https://code.launchpad.net/~jtv/maas/cobbler-exception/+merge/98148
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/cobbler-exception into lp:maas.

=== added file 'src/provisioningserver/cobblercatcher.py'
--- src/provisioningserver/cobblercatcher.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/cobblercatcher.py	2012-03-19 05:40:29 +0000
@@ -0,0 +1,95 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Helping hands for dealing with Cobbler exceptions."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'convert_cobbler_exception',
+    'ProvisioningError',
+    ]
+
+import re
+from xmlrpclib import Fault
+
+from provisioningserver.enum import PSERV_FAULT
+
+
+class ProvisioningError(Fault):
+    """Fault, as passed from provisioning server to maasserver.
+
+    This acts as a marker class within the provisioning server.  As far as
+    maasserver is concerned, when it sees an exception of this kind, it will
+    just be a Fault with a more telling faultCode.
+
+    The faultCode is one of :class:`PSERV_FAULT`.
+    """
+
+
+def extract_text(fault_string):
+    """Get the actual error message out of a Cobbler fault string.
+
+    This removes exception type information that may have been added on
+    the Cobbler side.
+
+    :param fault_string: A `Fault.faultString` as found on an exception
+        raised while working with Cobbler.
+    :type fault_string: basestring
+    :return: Extracted error message.
+    :rtype: basestring
+    """
+    match = re.match(
+        "<class 'cobbler\.cexceptions\.CX'>:'(.*)'", fault_string)
+    if match is None:
+        return fault_string
+    else:
+        return match.groups(0)[0]
+
+
+def divine_fault_code(err_str):
+    """Parse error string to figure out what kind of error it is.
+
+    :param err_str: An error string, as extracted from a `Fault` by
+        `extract_text`.
+    :type err_str: basestring
+    :return: A fault code from :class:`PSERV_FAULT`, for use as a
+        `Fault.faultCode`.
+    :rtype: int
+    """
+    prefixes = [
+        ("login failed", PSERV_FAULT.COBBLER_AUTH_FAILED),
+        ("invalid token:", PSERV_FAULT.COBBLER_AUTH_ERROR),
+        ("invalid profile name", PSERV_FAULT.NO_SUCH_PROFILE),
+        ("", PSERV_FAULT.GENERIC_COBBLER_ERROR),
+    ]
+    for prefix, code in prefixes:
+        if err_str.startswith(prefix):
+            return code
+    assert False, "No prefix matched fault string '%s'." % err_str
+
+
+def convert_cobbler_exception(fault):
+    """Convert a :class:`Fault` from Cobbler to a :class:`ProvisioningError`.
+
+    :param fault: The original exception, as raised by code that tried to
+        talk to Cobbler.
+    :type fault: Fault
+    :return: A more descriptive exception, for consumption by maasserver.
+    :rtype: :class:`ProvisioningError`
+    """
+    assert isinstance(fault, Fault)
+    assert not isinstance(fault, ProvisioningError), (
+        "Fault went through double conversion.")
+
+    err_str = extract_text(fault.faultString)
+    if fault.faultCode != 1:
+        fault_code = PSERV_FAULT.NO_COBBLER
+    else:
+        fault_code = divine_fault_code(err_str)
+
+    return ProvisioningError(faultCode=fault_code, faultString=err_str)

=== modified file 'src/provisioningserver/cobblerclient.py'
--- src/provisioningserver/cobblerclient.py	2012-03-15 13:58:32 +0000
+++ src/provisioningserver/cobblerclient.py	2012-03-19 05:40:29 +0000
@@ -33,6 +33,8 @@
 from functools import partial
 import xmlrpclib
 
+from provisioningserver.cobblercatcher import convert_cobbler_exception
+from provisioningserver.enum import PSERV_FAULT
 from twisted.internet import reactor as default_reactor
 from twisted.internet.defer import (
     DeferredLock,
@@ -87,15 +89,6 @@
         return d
 
 
-def looks_like_auth_expiry(exception):
-    """Does `exception` look like an authentication token expired?"""
-    if not hasattr(exception, 'faultString'):
-        # An auth failure would come as an xmlrpclib.Fault.
-        return False
-    else:
-        return "invalid token: " in exception.faultString
-
-
 def looks_like_object_not_found(exception):
     """Does `exception` look like an unknown object failure?"""
     if not hasattr(exception, 'faultString'):
@@ -234,6 +227,7 @@
         message = "%s(%s)" % (method, args_repr)
         msg(message, system=__name__)
 
+    @inlineCallbacks
     def _issue_call(self, method, *args):
         """Initiate call to XMLRPC method.
 
@@ -249,8 +243,12 @@
         method = method.encode('ascii')
         self._log_call(method, args)
         args = map(self._substitute_token, args)
-        d = self._with_timeout(self.proxy.callRemote(method, *args))
-        return d
+        try:
+            result = yield self._with_timeout(
+                self.proxy.callRemote(method, *args))
+        except xmlrpclib.Fault as e:
+            raise convert_cobbler_exception(e)
+        returnValue(result)
 
     @inlineCallbacks
     def call(self, method, *args):
@@ -279,7 +277,7 @@
             try:
                 result = yield self._issue_call(method, *args)
             except xmlrpclib.Fault as e:
-                if uses_auth and looks_like_auth_expiry(e):
+                if e.faultCode == PSERV_FAULT.COBBLER_AUTH_ERROR:
                     need_auth = True
                 else:
                     raise

=== modified file 'src/provisioningserver/enum.py'
--- src/provisioningserver/enum.py	2012-03-13 09:34:02 +0000
+++ src/provisioningserver/enum.py	2012-03-19 05:40:29 +0000
@@ -12,9 +12,29 @@
 __all__ = [
     'POWER_TYPE',
     'POWER_TYPE_CHOICES',
+    'PSERV_FAULT',
     ]
 
 
+class PSERV_FAULT:
+    """Fault codes for errors raised by the provisioning server."""
+
+    # Could not communicate with Cobbler.
+    NO_COBBLER = 2
+
+    # Failed to authenticate with Cobbler.
+    COBBLER_AUTH_FAILED = 3
+
+    # Cobbler no longer accepts the provisioning server's login token.
+    COBBLER_AUTH_ERROR = 4
+
+    # Profile does not exist.
+    NO_SUCH_PROFILE = 5
+
+    # Non-specific error inside Cobbler.
+    GENERIC_COBBLER_ERROR = 99
+
+
 class POWER_TYPE:
     """Choice of mechanism to control a node's power."""
 

=== modified file 'src/provisioningserver/testing/fakecobbler.py'
--- src/provisioningserver/testing/fakecobbler.py	2012-03-12 12:26:24 +0000
+++ src/provisioningserver/testing/fakecobbler.py	2012-03-19 05:40:29 +0000
@@ -39,7 +39,7 @@
 
 def fake_auth_failure_string(token):
     """Fake a Cobbler authentication failure fault string for `token`."""
-    return "<class 'cobbler.exceptions.CX'>:'invalid token: %s'" % token
+    return "<class 'cobbler.cexceptions.CX'>:'invalid token: %s'" % token
 
 
 def fake_object_not_found_string(object_type, object_name):
@@ -277,6 +277,15 @@
 
         session_obj[key] = value
 
+    def _validate(self, token, object_type, attributes):
+        """Check object for validity.  Raise :class:`Fault` if it's bad."""
+        # Add more checks here as needed for testing realism.
+        if object_type == 'system':
+            profile_name = attributes.get('profile')
+            profile = self._api_get_object('profile', profile_name)
+            if profile is None:
+                raise Fault(1, "invalid profile name: '%s'" % profile_name)
+
     def _api_save_object(self, token, object_type, handle):
         """Save an object's modifications to the saved store."""
         self._check_token(token)
@@ -289,6 +298,8 @@
             # Object not modified.  Nothing to do.
             return True
 
+        self._validate(token, object_type, session_obj)
+
         name = session_obj['name']
         other_handle = self._get_handle_if_present(token, object_type, name)
         if other_handle is not None and other_handle != handle:
@@ -360,6 +371,7 @@
 
     def xapi_object_edit(self, object_type, name, operation, attrs, token):
         """Swiss-Army-Knife API: create/rename/copy/edit object."""
+        self._check_token(token)
         if operation == 'remove':
             self._api_remove_object(token, object_type, name)
             return True

=== added file 'src/provisioningserver/tests/test_cobblercatcher.py'
--- src/provisioningserver/tests/test_cobblercatcher.py	1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_cobblercatcher.py	2012-03-19 05:40:29 +0000
@@ -0,0 +1,187 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for conversion of Cobbler exceptions to `ProvisioningError`."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from abc import (
+    ABCMeta,
+    abstractmethod,
+    )
+from random import randint
+from unittest import skipIf
+from xmlrpclib import Fault
+
+from maastesting.factory import factory
+from provisioningserver.cobblercatcher import (
+    convert_cobbler_exception,
+    divine_fault_code,
+    extract_text,
+    ProvisioningError,
+    )
+from provisioningserver.cobblerclient import CobblerSystem
+from provisioningserver.enum import PSERV_FAULT
+from provisioningserver.testing.fakecobbler import (
+    fake_auth_failure_string,
+    fake_token,
+    make_fake_cobbler_session,
+    )
+from provisioningserver.testing.realcobbler import RealCobbler
+from provisioningserver.utils import deferred
+from testtools import TestCase
+from testtools.deferredruntest import AsynchronousDeferredRunTest
+from twisted.internet.defer import inlineCallbacks
+
+
+class TestExceptionConversionWithFakes(TestCase):
+    """Tests for handling of Cobbler errors by cobblercatcher.
+
+    Can be targeted to real or fake Cobbler.
+    """
+
+    def test_recognizes_auth_expiry(self):
+        original_fault = Fault(1, fake_auth_failure_string(fake_token()))
+        converted_fault = convert_cobbler_exception(original_fault)
+        self.assertEqual(
+            PSERV_FAULT.COBBLER_AUTH_ERROR, converted_fault.faultCode)
+
+    def test_extract_text_leaves_unrecognized_message_intact(self):
+        text = factory.getRandomString()
+        self.assertEqual(text, extract_text(text))
+
+    def test_extract_text_strips_CX(self):
+        text = factory.getRandomString()
+        self.assertEqual(
+            text,
+            extract_text("<class 'cobbler.cexceptions.CX'>:'%s'" % text))
+
+    def test_divine_fault_code_recognizes_errors(self):
+        errors = {
+            "login failed (%s)": PSERV_FAULT.COBBLER_AUTH_FAILED,
+            "invalid token: %s": PSERV_FAULT.COBBLER_AUTH_ERROR,
+            "invalid profile name: %s": PSERV_FAULT.NO_SUCH_PROFILE,
+            "Huh? %s. Aaaaargh!": PSERV_FAULT.GENERIC_COBBLER_ERROR,
+        }
+        random = factory.getRandomString()
+        self.assertEqual(
+            errors, {
+                text: divine_fault_code(text % random)
+                for text in errors.keys()})
+
+    def test_convert_cobbler_exception_passes_through_other_faults(self):
+        original_fault = Fault(1234, "Error while talking to Cobbler")
+        converted_fault = convert_cobbler_exception(original_fault)
+        self.assertEqual(
+            (PSERV_FAULT.NO_COBBLER, original_fault.faultString),
+            (converted_fault.faultCode, converted_fault.faultString))
+
+    def test_convert_cobbler_exception_converts_to_provisioning_error(self):
+        self.assertIsInstance(
+            convert_cobbler_exception(Fault(1, "Kaboom")), ProvisioningError)
+
+    def test_convert_cobbler_exception_checks_against_double_conversion(self):
+        self.assertRaises(
+            AssertionError,
+            convert_cobbler_exception,
+            ProvisioningError(1, "Ayiieee!"))
+
+
+class FaultFinder:
+    """Context manager: catch and store a :class:`Fault` exception.
+
+    :ivar fault: The Fault this context manager caught.  This attribute will
+        not exist if no Fault occurred.
+    """
+
+    def __enter__(self):
+        pass
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        self.fault = exc_val
+        return isinstance(exc_val, Fault)
+
+
+class ExceptionConversionTests:
+    """Tests for exception handling; run against a Cobbler (real or fake)."""
+
+    __metaclass__ = ABCMeta
+
+    @abstractmethod
+    def get_cobbler_session(self):
+        """Override: provide a (real or fake) :class:`CobblerSession`.
+
+        :return: A :class:`Deferred` which in turn will return a
+            :class:`CobblerSession`.
+        """
+
+    @inlineCallbacks
+    def test_bad_token_means_token_expired(self):
+        session = yield self.get_cobbler_session()
+        session.token = factory.getRandomString()
+        arbitrary_id = randint(10000, 99999)
+
+        faultfinder = FaultFinder()
+        with faultfinder:
+            yield session._issue_call(
+                'xapi_object_edit', 'repo', 'repo-%d' % arbitrary_id,
+                'edit', {'mirror': 'on-the-wall'}, session.token)
+
+        self.assertEqual(
+            PSERV_FAULT.COBBLER_AUTH_ERROR, faultfinder.fault.faultCode)
+
+    @inlineCallbacks
+    def test_bad_profile_name_is_distinct_error(self):
+        session = yield self.get_cobbler_session()
+        arbitrary_id = randint(10000, 99999)
+
+        faultfinder = FaultFinder()
+        with faultfinder:
+            yield CobblerSystem.new(
+                session, 'system-%d' % arbitrary_id,
+                {'profile': 'profile-%d' % arbitrary_id})
+
+        self.assertEqual(
+            PSERV_FAULT.NO_SUCH_PROFILE, faultfinder.fault.faultCode)
+
+
+class TestExceptionConversionWithFakeCobbler(ExceptionConversionTests,
+                                             TestCase):
+    """Run `ExceptionConversionTests` against a fake Cobbler instance.
+
+    All tests from `ExceptionConversionTests` are included here through
+    inheritance.
+    """
+
+    run_tests_with = AsynchronousDeferredRunTest
+
+    @deferred
+    def get_cobbler_session(self):
+        return make_fake_cobbler_session()
+
+
+class TestExceptionConversionWithRealCobbler(ExceptionConversionTests,
+                                             TestCase):
+    """Run `ExceptionConversionTests` against a real Cobbler instance.
+
+    Activate this by setting the PSERV_TEST_COBBLER_URL environment variable
+    to point to a real Cobbler instance.
+
+    All tests from `ExceptionConversionTests` are included here through
+    inheritance.
+    """
+
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
+    real_cobbler = RealCobbler()
+
+    @skipIf(not real_cobbler.is_available(), RealCobbler.help_text)
+    @deferred
+    def get_cobbler_session(self):
+        return self.real_cobbler.get_session()

=== modified file 'src/provisioningserver/tests/test_cobblersession.py'
--- src/provisioningserver/tests/test_cobblersession.py	2012-03-12 14:58:32 +0000
+++ src/provisioningserver/tests/test_cobblersession.py	2012-03-19 05:40:29 +0000
@@ -16,6 +16,7 @@
 
 import fixtures
 from provisioningserver import cobblerclient
+from provisioningserver.cobblercatcher import ProvisioningError
 from provisioningserver.testing.fakecobbler import (
     fake_auth_failure_string,
     fake_object_not_found_string,
@@ -57,11 +58,9 @@
             1, fake_auth_failure_string(token))
 
 
-def make_auth_failure(broken_token=None):
+def make_auth_failure():
     """Mimick a Cobbler authentication failure."""
-    if broken_token is None:
-        broken_token = fake_token()
-    return FakeAuthFailure(broken_token)
+    return FakeAuthFailure(fake_token())
 
 
 class RecordingFakeProxy:
@@ -242,19 +241,6 @@
         self.assertEqual(return_value, actual_return_value)
         self.assertEqual([(method, arg)], session.proxy.calls)
 
-    def test_looks_like_auth_expiry_returns_False_for_regular_exception(self):
-        self.assertFalse(
-            cobblerclient.looks_like_auth_expiry(RuntimeError("Error")))
-
-    def test_looks_like_auth_expiry_returns_False_for_other_Fault(self):
-        self.assertFalse(
-            cobblerclient.looks_like_auth_expiry(
-                Fault(1, "Missing sprocket")))
-
-    def test_looks_like_auth_expiry_recognizes_auth_expiry(self):
-        self.assertTrue(
-            cobblerclient.looks_like_auth_expiry(make_auth_failure()))
-
     def test_looks_like_object_not_found_for_regular_exception(self):
         self.assertFalse(
             cobblerclient.looks_like_object_not_found(RuntimeError("Error")))
@@ -326,7 +312,7 @@
             make_auth_failure(),
             ]
         session.proxy.set_return_values(failures)
-        with ExpectedException(failures[-1].__class__, failures[-1].message):
+        with ExpectedException(ProvisioningError, failures[-1].message):
             return_value = yield session.call(
                 'double_fail', cobblerclient.CobblerSession.token_placeholder)
             self.addDetail('return_value', text_content(repr(return_value)))

=== modified file 'src/provisioningserver/tests/test_fakecobbler.py'
--- src/provisioningserver/tests/test_fakecobbler.py	2012-03-15 13:58:32 +0000
+++ src/provisioningserver/tests/test_fakecobbler.py	2012-03-19 05:40:29 +0000
@@ -14,8 +14,8 @@
 from itertools import count
 from random import randint
 from tempfile import NamedTemporaryFile
-import xmlrpclib
 
+from provisioningserver.cobblercatcher import ProvisioningError
 from provisioningserver.cobblerclient import (
     CobblerDistro,
     CobblerImage,
@@ -155,7 +155,7 @@
     @inlineCallbacks
     def test_login_failure_raises_failure(self):
         cobbler = FakeCobbler(passwords={'moi': 'potahto'})
-        with ExpectedException(xmlrpclib.Fault):
+        with ExpectedException(ProvisioningError):
             return_value = yield fake_cobbler_session(
                 user='moi', password='potayto', fake_cobbler=cobbler)
             self.addDetail('return_value', text_content(repr(return_value)))