Merge lp:~ack/landscape-client/max-pending-computers-message into lp:~landscape/landscape-client/trunk

[Alberto Donato]

This adds support in the client to handle a new error condition when trying to register a client, when there are too many pending computers already.
To do that, the client API version has been bumped, since the server needs to know if the client knows about this new error code.

This adds support in the client to handle a new error condition when trying to register a client, when there are too many pending computers already.
To do that, the client API version has been bumped, since the server needs to know if the client knows about this new error code.
The server side of this change is lp:~ack/landscape/max-pending-computers-message (which needs this branch landed because of the version bump).

Testing instructions:

I tested the changes as follows:

 - deploy an LDS from lp:~ack/landscape/max-pending-computers-message (make stage-landscape-charm ...)
 - create a LXD and install landscape-client, configure it to point to the LDS.
 - register the client 20 times (without accepting it) to create pending computers with: landscape-client --silent
 - try to register again, get the "Invalid account name or registration key." error (current client still works as previously)
 - make package from the client branch, install those packages in the LXD
 - the error message will be "Maximum number of computers pending approval reached."

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 946
Branch: lp:~ack/landscape-client/max-pending-computers-message
Jenkins: https://ci.lscape.net/job/latch-test-precise/867/

review: Approve (test results)

[Chad Smith (chad.smith)]

just a couple nits so far. Going through deploy testing now.

[Free Ekanayaka (free.ekanayaka)]

+1 with a few nits

review: Approve

[lp:~ack/landscape-client/max-pending-computers-message]

947. By Alberto Donato on 2017-02-24

Address review comments.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Fail
Revno: 947
Branch: lp:~ack/landscape-client/max-pending-computers-message
Jenkins: https://ci.lscape.net/job/latch-test-precise/868/

review: Needs Fixing (test results)

[lp:~ack/landscape-client/max-pending-computers-message]

948. By Alberto Donato on 2017-02-24

Fix test.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: TRIAL_ARGS=-j4 make check
Result: Success
Revno: 948
Branch: lp:~ack/landscape-client/max-pending-computers-message
Jenkins: https://ci.lscape.net/job/latch-test-precise/869/

review: Approve (test results)

[Chad Smith (chad.smith)]

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2017-01-25 19:23:36 +0000
+++ debian/changelog	2017-02-24 10:09:35 +0000
@@ -1,3 +1,9 @@
+landscape-client (17.01+bzr946-0ubuntu0) xenial; urgency=medium
+
+  * New local test build
+
+ -- Ubuntu <[e-mail]>  Wed, 22 Feb 2017 12:04:19 +0000
+
 landscape-client (17.01-0ubuntu1) UNRELEASED; urgency=medium
 
   * New trunk build

=== modified file 'landscape/__init__.py'
--- landscape/__init__.py	2017-01-25 19:23:36 +0000
+++ landscape/__init__.py	2017-02-24 10:09:35 +0000
@@ -1,5 +1,5 @@
-DEBIAN_REVISION = ""
-UPSTREAM_VERSION = "17.01"
+DEBIAN_REVISION = "-0ubuntu0"
+UPSTREAM_VERSION = "17.01+bzr946"
 VERSION = "%s%s" % (UPSTREAM_VERSION, DEBIAN_REVISION)
 
 # The minimum server API version that all Landscape servers are known to speak
@@ -17,17 +17,13 @@
 #
 # Changelog:
 #
-# 3.3:
-#  * Add new schema for the "registration" message, providing Juju information
 # 3.2:
 #  * Add new "eucalyptus-info" and "eucalyptus-info-error" messages.
+# 3.3:
+#  * Add new schema for the "registration" message, providing Juju information
 #
 SERVER_API = "3.3"
 
-# XXX This is needed for backward compatibility in the server code importing
-# the API variable. We should eventually replace it in the server code.
-API = SERVER_API
-
 # The "client-api" field of outgoing messages will be set to this value, and
 # used by the server to know which schema do the message types accepted by the
 # client support. Bump it when the schema of an accepted message type changes
@@ -53,4 +49,9 @@
 # 3.7:
 #  * Server returns 402 Payment Required if the computer has no valid license.
 #
-CLIENT_API = "3.7"
+# 3.8:
+#  * Handle the new "max-pending-computers" key returned by the server in the
+#    "info" field of the response payload for a fail registration, in case the
+#    account has too many pending computers.
+#
+CLIENT_API = "3.8"

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2015-11-09 21:10:59 +0000
+++ landscape/broker/registration.py	2017-02-24 10:09:35 +0000
@@ -19,10 +19,11 @@
 from landscape.lib.versioning import is_version_higher
 
 
-class InvalidCredentialsError(Exception):
+class RegistrationError(Exception):
     """
-    Raised when an invalid account title and/or registration key
-    is used with L{RegistrationManager.register}.
+    Raised when the registration failed because an invalid account title
+    and/or registration key are used with RegistrationManager.register,
+    or the server has too many pending computers already.
     """
 
 
@@ -116,7 +117,7 @@
 
         @return: A L{Deferred} which will either be fired with None if
             registration was successful or will fail with an
-            L{InvalidCredentialsError} if not.
+            RegistrationError if not.
         """
         self._identity.secure_id = None
         self._identity.insecure_id = None
@@ -163,7 +164,7 @@
         account_name = identity.account_name
 
         if not account_name:
-            self._reactor.fire("registration-failed")
+            self._reactor.fire("registration-failed", reason="unknown-account")
             return
 
         tags = identity.tags
@@ -233,8 +234,8 @@
         self._reactor.fire("resynchronize-clients")
 
     def _handle_registration(self, message):
-        if message["info"] == "unknown-account":
-            self._reactor.fire("registration-failed")
+        if message["info"] in ("unknown-account", "max-pending-computers"):
+            self._reactor.fire("registration-failed", reason=message["info"])
 
     def _handle_unknown_id(self, message):
         id = self._identity
@@ -279,6 +280,6 @@
         self.deferred.callback(None)
         self._cancel_calls()
 
-    def _failed(self):
-        self.deferred.errback(InvalidCredentialsError())
+    def _failed(self, reason=None):
+        self.deferred.errback(RegistrationError(reason))
         self._cancel_calls()

=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2016-06-16 16:17:53 +0000
+++ landscape/broker/tests/test_registration.py	2017-02-24 10:09:35 +0000
@@ -3,8 +3,7 @@
 import socket
 import mock
 
-from landscape.broker.registration import (
-    InvalidCredentialsError, Identity)
+from landscape.broker.registration import RegistrationError, Identity
 from landscape.tests.helpers import LandscapeTest
 from landscape.broker.tests.helpers import (
     BrokerConfigurationHelper, RegistrationHelper)
@@ -343,16 +342,29 @@
         self.assertMessages(self.mstore.get_pending_messages(), [])
         handler_mock.assert_called_once_with()
 
-    def test_registration_failed_event(self):
+    def test_registration_failed_event_unknown_account(self):
         """
         The deferred returned by a registration request should fail
-        with L{InvalidCredentialsError} if the server responds with a
-        failure message.
+        if the server responds with a failure message because credentials are
+        wrong.
         """
         reactor_fire_mock = self.reactor.fire = mock.Mock()
         self.exchanger.handle_message(
             {"type": "registration", "info": "unknown-account"})
-        reactor_fire_mock.assert_called_with("registration-failed")
+        reactor_fire_mock.assert_called_with(
+            "registration-failed", reason="unknown-account")
+
+    def test_registration_failed_event_max_pending_computers(self):
+        """
+        The deferred returned by a registration request should fail
+        if the server responds with a failure message because the max number of
+        pending computers have been reached.
+        """
+        reactor_fire_mock = self.reactor.fire = mock.Mock()
+        self.exchanger.handle_message(
+            {"type": "registration", "info": "max-pending-computers"})
+        reactor_fire_mock.assert_called_with(
+            "registration-failed", reason="max-pending-computers")
 
     def test_registration_failed_event_not_fired_when_uncertain(self):
         """
@@ -424,17 +436,22 @@
 
         self.assertEqual(results, [None])
 
-    def test_register_deferred_called_on_failed(self):
+    def test_register_deferred_called_on_failed_unknown_account(self):
+        """
+        The registration errback is called on failures when credentials are
+        invalid.
+        """
         # We don't want informational messages.
         self.logger.setLevel(logging.WARNING)
 
-        calls = [0]
+        calls = []
         d = self.handler.register()
 
         def add_call(failure):
             exception = failure.value
-            self.assertTrue(isinstance(exception, InvalidCredentialsError))
-            calls[0] += 1
+            self.assertTrue(isinstance(exception, RegistrationError))
+            self.assertEqual("unknown-account", str(exception))
+            calls.append(True)
 
         d.addErrback(add_call)
 
@@ -442,13 +459,45 @@
         self.exchanger.handle_message(
             {"type": "registration", "info": "unknown-account"})
 
-        self.assertEqual(calls, [1])
+        self.assertEqual(calls, [True])
 
         # Doing it again to ensure that the deferred isn't called twice.
         self.exchanger.handle_message(
             {"type": "registration", "info": "unknown-account"})
 
-        self.assertEqual(calls, [1])
+        self.assertEqual(calls, [True])
+
+        self.assertEqual(self.logfile.getvalue(), "")
+
+    def test_register_deferred_called_on_failed_max_pending_computers(self):
+        """
+        The registration errback is called on failures when max number of
+        pending computers has been reached.
+        """
+        # We don't want informational messages.
+        self.logger.setLevel(logging.WARNING)
+
+        calls = []
+        d = self.handler.register()
+
+        def add_call(failure):
+            exception = failure.value
+            self.assertTrue(isinstance(exception, RegistrationError))
+            self.assertEqual("max-pending-computers", str(exception))
+            calls.append(True)
+
+        d.addErrback(add_call)
+
+        self.exchanger.handle_message(
+            {"type": "registration", "info": "max-pending-computers"})
+
+        self.assertEqual(calls, [True])
+
+        # Doing it again to ensure that the deferred isn't called twice.
+        self.exchanger.handle_message(
+            {"type": "registration", "info": "max-pending-computers"})
+
+        self.assertEqual(calls, [True])
 
         self.assertEqual(self.logfile.getvalue(), "")
 

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2015-11-09 21:10:59 +0000
+++ landscape/configuration.py	2017-02-24 10:09:35 +0000
@@ -23,7 +23,7 @@
 from landscape.lib.fetch import fetch, FetchError
 from landscape.lib.bootstrap import BootstrapList, BootstrapDirectory
 from landscape.reactor import LandscapeReactor
-from landscape.broker.registration import InvalidCredentialsError
+from landscape.broker.registration import RegistrationError
 from landscape.broker.config import BrokerConfiguration
 from landscape.broker.amp import RemoteBrokerConnector
 
@@ -104,7 +104,7 @@
                         raise ImportOptionError(
                             "Couldn't read configuration from %s." %
                             self.import_from)
-            except Exception, error:
+            except Exception as error:
                 raise ImportOptionError(str(error))
 
             # But real command line options have precedence.
@@ -124,7 +124,7 @@
         error_message = None
         try:
             content = fetch(url)
-        except FetchError, error:
+        except FetchError as error:
             error_message = str(error)
         if error_message is not None:
             raise ImportOptionError(
@@ -324,9 +324,9 @@
             proxies now.  If you don't use a proxy, leave these fields empty.
             """)
 
-        if not "http_proxy" in options:
+        if "http_proxy" not in options:
             self.prompt("http_proxy", "HTTP proxy URL")
-        if not "https_proxy" in options:
+        if "https_proxy" not in options:
             self.prompt("https_proxy", "HTTPS proxy URL")
 
     def query_script_plugin(self):
@@ -413,7 +413,7 @@
             self.prompt("tags", "Tags", False)
             if self._get_invalid_tags(self.config.tags):
                 self.show_help("Tag names may only contain alphanumeric "
-                              "characters.")
+                               "characters.")
                 self.config.tags = None  # Reset for the next prompt
             else:
                 break
@@ -588,9 +588,10 @@
     return key_filename
 
 
-def failure(add_result):
+def failure(add_result, reason=None):
     """Handle a failed communication by recording the kind of failure."""
-    add_result("failure")
+    if reason:
+        add_result(reason)
 
 
 def exchange_failure(add_result, ssl_error=False):
@@ -601,17 +602,19 @@
         add_result("non-ssl-error")
 
 
-def handle_registration_errors(failure, connector):
-    """Handle invalid credentials.
+def handle_registration_errors(add_result, failure, connector):
+    """Handle registration errors.
 
     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.
+    failed, because of invalid credentials or excessive pending computers.
+    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)
+    error = failure.trap(RegistrationError, MethodCallError)
+    if error is RegistrationError:
+        add_result(str(failure.value))
     connector.disconnect()
 
 
@@ -633,7 +636,8 @@
                 "exchange-failed": partial(exchange_failure, add_result)}
     deferreds = [
         remote.call_on_event(handlers),
-        remote.register().addErrback(handle_registration_errors, connector)]
+        remote.register().addErrback(
+            partial(handle_registration_errors, add_result), connector)]
     results = gather_results(deferreds)
     results.addCallback(done, connector, reactor)
     return results
@@ -646,8 +650,8 @@
 
 
 def register(config, reactor=None, connector_factory=RemoteBrokerConnector,
-        got_connection=got_connection, max_retries=14, on_error=None,
-        results=None):
+             got_connection=got_connection, max_retries=14, on_error=None,
+             results=None):
     """Instruct the Landscape Broker to register the client.
 
     The broker will be instructed to reload its configuration and then to
@@ -694,24 +698,32 @@
 
 
 def report_registration_outcome(what_happened, print=print):
-    """Report the registrtion interaction outcome to the user in human-readable
+    """Report the registration interaction outcome to the user in human-readable
     form.
     """
-    if what_happened == "success":
-        print("System successfully registered.")
-    elif what_happened == "failure":
-        print("Invalid account name or registration key.", file=sys.stderr)
-    elif what_happened == "ssl-error":
-        print("\nThe server's SSL information is incorrect, or fails "
-              "signature verification!\n"
-              "If the server is using a self-signed certificate, "
-              "please ensure you supply it with the --ssl-public-key "
-              "parameter.", file=sys.stderr)
-    elif what_happened == "non-ssl-error":
-        print("\nWe were unable to contact the server.\n"
-              "Your internet connection may be down. "
-              "The landscape client will continue to try and contact "
-              "the server periodically.", file=sys.stderr)
+    messages = {
+        "success": "System successfully registered.",
+        "unknown-account": "Invalid account name or registration key.",
+        "max-pending-computers": (
+            "Maximum number of computers pending approval reached. ",
+            "Login to your Landscape server account page to manage "
+            "pending computer approvals."),
+        "ssl-error": (
+            "\nThe server's SSL information is incorrect, or fails "
+            "signature verification!\n"
+            "If the server is using a self-signed certificate, "
+            "please ensure you supply it with the --ssl-public-key "
+            "parameter."),
+        "non-ssl-error": (
+            "\nWe were unable to contact the server.\n"
+            "Your internet connection may be down. "
+            "The landscape client will continue to try and contact "
+            "the server periodically.")
+    }
+    message = messages.get(what_happened)
+    if message:
+        fd = sys.stdout if what_happened == "success" else sys.stderr
+        print(message, file=fd)
 
 
 def determine_exit_code(what_happened):

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2016-12-08 12:48:46 +0000
+++ landscape/tests/test_configuration.py	2017-02-24 10:09:35 +0000
@@ -1,5 +1,6 @@
 from __future__ import print_function
 
+from functools import partial
 from ConfigParser import ConfigParser
 from cStringIO import StringIO
 import os
@@ -9,7 +10,7 @@
 import mock
 from twisted.internet.defer import succeed, fail, Deferred
 
-from landscape.broker.registration import InvalidCredentialsError
+from landscape.broker.registration import RegistrationError
 from landscape.broker.tests.helpers import RemoteBrokerHelper
 from landscape.configuration import (
     print_text, LandscapeSetupScript, LandscapeSetupConfiguration,
@@ -44,6 +45,7 @@
 
 
 class SuccessTests(unittest.TestCase):
+
     def test_success(self):
         """The success handler records the success."""
         results = []
@@ -52,11 +54,12 @@
 
 
 class FailureTests(unittest.TestCase):
+
     def test_failure(self):
         """The failure handler records the failure and returns non-zero."""
         results = []
-        self.assertNotEqual(0, failure(results.append))
-        self.assertEqual(["failure"], results)
+        self.assertNotEqual(0, failure(results.append, "an-error"))
+        self.assertEqual(["an-error"], results)
 
 
 class ExchangeFailureTests(unittest.TestCase):
@@ -84,7 +87,7 @@
 
     def test_handle_registration_errors_traps(self):
         """
-        The handle_registration_errors() function traps InvalidCredentialsError
+        The handle_registration_errors() function traps RegistrationError
         and MethodCallError errors.
         """
         class FauxFailure(object):
@@ -94,10 +97,15 @@
         faux_connector = FauxConnector()
         faux_failure = FauxFailure()
 
+        results = []
+        add_result = results.append
+
         self.assertNotEqual(
-            0, handle_registration_errors(faux_failure, faux_connector))
+            0,
+            handle_registration_errors(
+                add_result, faux_failure, faux_connector))
         self.assertTrue(
-            [InvalidCredentialsError, MethodCallError],
+            [RegistrationError, MethodCallError],
             faux_failure.trapped_exceptions)
 
     def test_handle_registration_errors_disconnects_cleanly(self):
@@ -112,8 +120,13 @@
         faux_connector = FauxConnector()
         faux_failure = FauxFailure()
 
+        results = []
+        add_result = results.append
+
         self.assertNotEqual(
-            0, handle_registration_errors(faux_failure, faux_connector))
+            0,
+            handle_registration_errors(
+                add_result, faux_failure, faux_connector))
         self.assertTrue(faux_connector.was_disconnected)
 
     def test_handle_registration_errors_as_errback(self):
@@ -128,11 +141,15 @@
 
         def i_raise(result):
             calls.append(True)
-            return InvalidCredentialsError("Bad mojo")
+            return RegistrationError("Bad mojo")
+
+        results = []
+        add_result = results.append
 
         deferred = Deferred()
         deferred.addCallback(i_raise)
-        deferred.addErrback(handle_registration_errors, faux_connector)
+        deferred.addErrback(
+            partial(handle_registration_errors, add_result), faux_connector)
         deferred.callback("")  # This kicks off the callback chain.
 
         self.assertEqual([True], calls)
@@ -1126,7 +1143,8 @@
             printed)
 
     @mock.patch("__builtin__.raw_input", return_value="y")
-    @mock.patch("landscape.configuration.register", return_value="failure")
+    @mock.patch(
+        "landscape.configuration.register", return_value="unknown-account")
     @mock.patch("landscape.configuration.setup")
     def test_main_user_interaction_failure(
             self, mock_setup, mock_register, mock_raw_input):
@@ -1177,11 +1195,13 @@
             printed)
 
     @mock.patch("__builtin__.raw_input")
-    @mock.patch("landscape.configuration.register", return_value="failure")
+    @mock.patch(
+        "landscape.configuration.register", return_value="unknown-account")
     @mock.patch("landscape.configuration.setup")
     def test_main_user_interaction_failure_silent(
             self, mock_setup, mock_register, mock_raw_input):
-        """A failure result is communicated to the user even with --silent.
+        """
+        A failure result is communicated to the user even with --silent.
         """
         printed = []
 
@@ -1797,13 +1817,13 @@
 
     def test_register_registration_error(self):
         """
-        If we get a registration error, the register() function returns
-        "failure".
+        If we get a registration error, the register() function returns the
+        error from the registration response message.
         """
         self.reactor.call_later(0, self.reactor.fire, "registration-failed")
 
         def fail_register():
-            return fail(InvalidCredentialsError("Nope."))
+            return fail(RegistrationError("max-pending-computers"))
 
         self.remote.register = fail_register
 
@@ -1811,7 +1831,7 @@
         result = register(
             config=self.config, reactor=self.reactor,
             connector_factory=connector_factory, max_retries=99)
-        self.assertEqual("failure", result)
+        self.assertEqual("max-pending-computers", result)
 
 
 class FauxConnection(object):
@@ -1981,9 +2001,10 @@
                 for handler in faux_remote.handlers.values()])
         # We include a single error handler to react to exchange errors.
         self.assertTrue(1, len(faux_remote.register_deferred.errbacks))
+        # the handle_registration_errors is wrapped in a partial()
         self.assertEqual(
             'handle_registration_errors',
-            faux_remote.register_deferred.errbacks[0].__name__)
+            faux_remote.register_deferred.errbacks[0].func.__name__)
 
     def test_register_with_on_error_and_an_error(self):
         """A caller-provided on_error callable will be called if errors occur.
@@ -2070,11 +2091,30 @@
         self.assertIn("System successfully registered.", self.result)
         self.assertIn(sys.stdout.name, self.output)
 
-    def test_failure_case(self):
-        report_registration_outcome("failure", print=self.record_result)
+    def test_unknown_account_case(self):
+        """
+        If the unknown-account error is found, an appropriate message is
+        returned.
+        """
+        report_registration_outcome(
+            "unknown-account", print=self.record_result)
         self.assertIn("Invalid account name or registration key.", self.result)
         self.assertIn(sys.stderr.name, self.output)
 
+    def test_max_pending_computers_case(self):
+        """
+        If the max-pending-computers error is found, an appropriate message is
+        returned.
+        """
+        report_registration_outcome(
+            "max-pending-computers", print=self.record_result)
+        messages = (
+            "Maximum number of computers pending approval reached. ",
+            "Login to your Landscape server account page to manage pending "
+            "computer approvals.")
+        self.assertIn(messages, self.result)
+        self.assertIn(sys.stderr.name, self.output)
+
     def test_ssl_error_case(self):
         report_registration_outcome("ssl-error", print=self.record_result)
         self.assertIn("\nThe server's SSL information is incorrect, or fails "
@@ -2107,7 +2147,9 @@
         When passed a failure result, the determine_exit_code function returns
         2.
         """
-        failure_codes = ["failure", "ssl-error", "non-ssl-error"]
+        failure_codes = [
+            "unknown-account", "max-computers-count", "ssl-error",
+            "non-ssl-error"]
         for code in failure_codes:
             result = determine_exit_code(code)
             self.assertEqual(2, result)