Merge lp:~tribaal/landscape-client/fix-1429888-wrong-credentials-stacktrace into lp:~landscape/landscape-client/trunk

[Chris Glass]

This branch fixes the related bug, where inappropriate account/password combinations made client registration badly.

This branch fixes the related bug, where inappropriate account/password combinations made client registration badly.

Test with:
- Spin up an LXC
- Install landscape-client
- symlink /usr/lib/python2.7/dist-packages/landscape to a checkout of this branch
- run landscape-config, enter a wrong registration password
- re-run landscape-config, enter a correct registration password.
- The client registers.

[Andreas Hasenack (ahasenack)]

Once this lands, please remove the skip tag from the robot test in landscape/trunk (server):

=== modified file 'robot/system/test/client/upgrade.txt'
--- robot/system/test/client/upgrade.txt 2015-03-10 18:45:51 +0000
+++ robot/system/test/client/upgrade.txt 2015-03-12 16:07:01 +0000
@@ -5,8 +5,7 @@
 Suite Setup Client Upgrade Suite Setup
 Suite Teardown Client Upgrade Suite Teardown
 Test Setup Default Setup
-# XXX remove skip tag after #1429888 is fixed
-Force Tags client-upgrade skip
+Force Tags client-upgrade

 *** Variables ***

[Adam Collard (adam-collard)]

Question inline

review: Needs Information

[lp:~tribaal/landscape-client/fix-1429888-wrong-credentials-stacktrace]

817. By Chris Glass on 2015-03-17

Made tests more explicit (hopefully)

[Adam Collard (adam-collard)]

Spellings

[Adam Collard (adam-collard)]

Two inline comments

[lp:~tribaal/landscape-client/fix-1429888-wrong-credentials-stacktrace]

818. By Chris Glass on 2015-03-17

PEP-257

819. By Chris Glass on 2015-03-17

Fixed comments.

[Chris Glass (tribaal)]

All comments should be addressed.

[Adam Collard (adam-collard)]

Looks good, +1.

FWIW I tested by making packages and then installing those :)

review: Approve

[Björn Tillenius (bjornt)]

I think we need to try to test this better. The style of testing you use is the same style that didn't caught this error, so I don't have much confidence in those tests. IMO, they don't test that things actually work, they test that the code is written in a certain way.

I think you should try to add a test in RegisterRealFunctionTest suite, so that we can see that register() actually returns a sane error. I would trust that more than I trust the comment you added.

Note that you will have to change FakeRemoteBroker, so that you can tell it to make register() to fail. But it shouldn't be too hard. Ping me if you have any problems.

review: Needs Fixing

[lp:~tribaal/landscape-client/fix-1429888-wrong-credentials-stacktrace]

820. By Chris Glass on 2015-03-19

Added test. No idea why this one fails.

821. By Chris Glass on 2015-03-19

The test passes now. Thanks for the insight Bjorn!

[Chris Glass (tribaal)]

All comments should be addressed.

[Björn Tillenius (bjornt)]

Thanks for the added test. +1.

review: Approve

[Chris Glass (tribaal)]

For the record I removed the skip tag in the server's robot test now that this landed, see: https://bazaar.launchpad.net/~landscape/landscape/trunk/revision/8616

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2015-03-10 16:53:24 +0000
+++ landscape/configuration.py	2015-03-19 13:44:35 +0000
@@ -601,11 +601,17 @@
         add_result("non-ssl-error")
 
 
-def handle_registration_errors(add_result, connector, failure):
-    """HAndle a failed registration by recording the kind of failure."""
-    # We'll get invalid credentials through the signal.
+def handle_registration_errors(failure, connector):
+    """Handle invalid credentials.
+
+    The connection to the broker succeeded but the registration itself
+    failed, because of invalid credentials. We need to trap the exceptions
+    so they don't stacktrace (we know what is going on), and try to cleanly
+    disconnect from the broker.
+
+    Note: "results" contains a failure indication already (or will shortly)
+    since the registration-failed signal will fire."""
     failure.trap(InvalidCredentialsError, MethodCallError)
-    add_result("registration-error")
     connector.disconnect()
 
 
@@ -627,8 +633,7 @@
                 "exchange-failed": partial(exchange_failure, add_result)}
     deferreds = [
         remote.call_on_event(handlers),
-        remote.register().addErrback(
-            handle_registration_errors, add_result, connector)]
+        remote.register().addErrback(handle_registration_errors, connector)]
     results = gather_results(deferreds)
     results.addCallback(done, connector, reactor)
     return results

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2015-03-10 16:53:24 +0000
+++ landscape/tests/test_configuration.py	2015-03-19 13:44:35 +0000
@@ -7,7 +7,7 @@
 import sys
 import unittest
 
-from twisted.internet.defer import succeed
+from twisted.internet.defer import succeed, fail, Deferred
 
 from landscape.broker.registration import InvalidCredentialsError
 from landscape.broker.tests.helpers import RemoteBrokerHelper
@@ -82,11 +82,10 @@
 
 class HandleRegistrationErrorsTests(unittest.TestCase):
 
-    def test_handle_registration_errors(self):
+    def test_handle_registration_errors_traps(self):
         """
-        The handle_registration_errors() function handles
-        InvalidCredentialsError and MethodCallError errors and records the
-        type of failure and disconnects.
+        The handle_registration_errors() function traps InvalidCredentialsError
+        and MethodCallError errors.
         """
         class FauxFailure(object):
             def trap(self, *trapped):
@@ -95,16 +94,49 @@
         faux_connector = FauxConnector()
         faux_failure = FauxFailure()
 
-        results = []
-        self.assertNotEqual(0,
-            handle_registration_errors(
-                results.append, faux_connector, faux_failure))
-        self.assertEqual(["registration-error"], results)
-        self.assertTrue(faux_connector.was_disconnected)
+        self.assertNotEqual(
+            0, handle_registration_errors(faux_failure, faux_connector))
         self.assertTrue(
             [InvalidCredentialsError, MethodCallError],
             faux_failure.trapped_exceptions)
 
+    def test_handle_registration_errors_disconnects_cleanly(self):
+        """
+        The handle_registration_errors function disconnects the broker
+        connector cleanly.
+        """
+        class FauxFailure(object):
+            def trap(self, *trapped):
+                pass
+
+        faux_connector = FauxConnector()
+        faux_failure = FauxFailure()
+
+        self.assertNotEqual(
+            0, handle_registration_errors(faux_failure, faux_connector))
+        self.assertTrue(faux_connector.was_disconnected)
+
+    def test_handle_registration_errors_as_errback(self):
+        """
+        The handle_registration_errors functions works as an errback.
+
+        This test was put in place to assert the parameters passed to the
+        function when used as an errback are in the correct order.
+        """
+        faux_connector = FauxConnector()
+        calls = []
+
+        def i_raise(result):
+            calls.append(True)
+            return InvalidCredentialsError("Bad mojo")
+
+        deferred = Deferred()
+        deferred.addCallback(i_raise)
+        deferred.addErrback(handle_registration_errors, faux_connector)
+        deferred.callback("")  # This kicks off the callback chain.
+
+        self.assertEqual([True], calls)
+
 
 class DoneTests(unittest.TestCase):
 
@@ -1933,6 +1965,24 @@
             self.config, self.reactor, connector_factory, max_retries=99)
         self.assertEqual("success", result)
 
+    def test_register_registration_error(self):
+        """
+        If we get a registration error, the register() function returns
+        "failure".
+        """
+        self.reactor.call_later(0, self.reactor.fire, "registration-failed")
+
+        def fail_register():
+            return fail(InvalidCredentialsError("Nope."))
+
+        self.remote.register = fail_register
+
+        connector_factory = FakeConnectorFactory(self.remote)
+        result = register(
+            config=self.config, reactor=self.reactor,
+            connector_factory=connector_factory, max_retries=99)
+        self.assertEqual("failure", result)
+
 
 class FauxConnection(object):
     def __init__(self):