Merge lp:~fcorrea/landscape-client/add-otp-field into lp:~landscape/landscape-client/trunk

[Fernando Correa Neto]

This branch adds a optinal OTP field to the REGISTER message so computers can be registered automatically.

[Free Ekanayaka (free.ekanayaka)]

Nice working, marking needs fixing mainly because of [1].

Thanks!

[1]

+ elif self._config.provisioning_otp:
+ return True

I think this will make the client try to register again every time it's started. It should be probably

return (not id.secure_id) and self._message_store.accepts("register-provisioned-machine")

the (not id.secure_id) means that the computer has not a secure ID yet (which afair is something that the client get and stores after a successful registration) and self._message_store.accepts("register-provisioned-machine") ensures that the server actually supports that kind of message (in principle it might be an older server that does not support it yet).

[2]

+ logging.info(u"Queueing message to register with OTP as a"
+ u" provisiong machine.")

I'd suggest s/provisioning/newly provisioned/

[3]

+ {"otp": Any(String(), Constant(None))})

This should be just

    {"otp": String()})

as the client must never send an otp with a None value.

[4]
+ """
+ If the provisioning OTP is specified, there is no need to pass the
+ account name and the computer title.
+ """

This is fine, but I just wanted to remind that in practice what we will do is probably to include the account name and computer title in the configuration file (via debconf), for sake of reference. However I think this code doesn't need to be changed in that case.

review: Needs Fixing

[Alberto Donato (ack)]

Looks good! +1

#1:
+ self.assertMessages([{'otp': 'ohteepee', 'timestamp': 0, 'api': '3.2',
+ 'type': 'register-provisioned-machine'}],

Please use double quotes (in the tests that follow as well).

#2:
         if not config.get("otp") and (not config.get("account_name") or not
- config.get("computer_title")):
+ config.get("computer_title"))\
+ and not config.provisioning_otp:

This is a bit hard to read, I think it could be rewritten as

if not (config.get("otp") or config.provisioning_otp or
       (config.get("account_name") and config.get("computer_title"))):

review: Approve

[lp:~fcorrea/landscape-client/add-otp-field]

399. By Fernando Correa Neto <email address hidden> on 2011-11-11

- addressing review comments

400. By Fernando Correa Neto <email address hidden> on 2011-11-11

- addressing review comments

[Fernando Correa Neto (fcorrea)]

> Nice working, marking needs fixing mainly because of [1].
>
> Thanks!
>
> [1]
>
> + elif self._config.provisioning_otp:
> + return True
>
> I think this will make the client try to register again every time it's
> started. It should be probably
>
> return (not id.secure_id) and self._message_store.accepts("register-
> provisioned-machine")
>
> the (not id.secure_id) means that the computer has not a secure ID yet (which
> afair is something that the client get and stores after a successful
> registration) and self._message_store.accepts("register-provisioned-machine")
> ensures that the server actually supports that kind of message (in principle
> it might be an older server that does not support it yet).
>

Nice. Fixed and thanks for the explanation. It really helps understanding how this works.

> [2]
>
> + logging.info(u"Queueing message to register with OTP as a"
> + u" provisiong machine.")
>
> I'd suggest s/provisioning/newly provisioned/

Done.

>
> [3]
>
> + {"otp": Any(String(), Constant(None))})
>
> This should be just
>
> {"otp": String()})
>
> as the client must never send an otp with a None value.

Right. I was pretty much doing the same thing was being done by the cloud instance opt. Fixed.

>
> [4]
> + """
> + If the provisioning OTP is specified, there is no need to pass the
> + account name and the computer title.
> + """
>
> This is fine, but I just wanted to remind that in practice what we will do is
> probably to include the account name and computer title in the configuration
> file (via debconf), for sake of reference. However I think this code doesn't
> need to be changed in that case.

Sure. I believe we'll have some more interactions until we get everything done so that we can change it appropriately.

Thanks!

[Fernando Correa Neto (fcorrea)]

> Looks good! +1
>
> #1:
> + self.assertMessages([{'otp': 'ohteepee', 'timestamp': 0, 'api':
> '3.2',
> + 'type': 'register-provisioned-machine'}],
>
> Please use double quotes (in the tests that follow as well).

Fixed.

>
> #2:
> if not config.get("otp") and (not config.get("account_name") or not
> - config.get("computer_title")):
> + config.get("computer_title"))\
> + and not config.provisioning_otp:
>
> This is a bit hard to read, I think it could be rewritten as
>
> if not (config.get("otp") or config.provisioning_otp or
> (config.get("account_name") and config.get("computer_title"))):

Agreed. Before pushing that one, I tried two other possible ways that didn't end up as nice as yours :-)

Thanks!

[Free Ekanayaka (free.ekanayaka)]

+1!

[5]

+ parser.add_option("--provisioning-otp", default="",

I think you can just omit the default="" parameter and you will get None if nothing is passed. I see it's there for the cloud otp option, but I'm not sure if there's a good reason for that.

review: Approve

[lp:~fcorrea/landscape-client/add-otp-field]

401. By Fernando Correa Neto <email address hidden> on 2011-11-11

- removing defaults for provisioning otp cli option

=== modified file 'landscape/broker/config.py'
--- landscape/broker/config.py	2011-10-20 17:59:29 +0000
+++ landscape/broker/config.py	2011-11-11 16:21:24 +0000
@@ -85,7 +85,9 @@
                           help="Comma separated list of tag names to be sent "
                                "to the server.")
         parser.add_option("--record", action="store_true",
-                          help="Record data sent to the server on filesystem ")
+                          help="Record data sent to the server on filesystem.")
+        parser.add_option("--provisioning-otp",
+                          help="The OTP to use for a provisioned machine.")
         return parser
 
     @property

=== modified file 'landscape/broker/registration.py'
--- landscape/broker/registration.py	2011-07-21 23:55:47 +0000
+++ landscape/broker/registration.py	2011-11-11 16:21:24 +0000
@@ -104,6 +104,9 @@
         if self._config.cloud:
             return bool(not id.secure_id
                         and self._message_store.accepts("register-cloud-vm"))
+        elif self._config.provisioning_otp:
+            return (not id.secure_id) and\
+                self._message_store.accepts("register-provisioned-machine")
         return bool(not id.secure_id and id.computer_title and id.account_name
                     and self._message_store.accepts("register"))
 
@@ -274,8 +277,8 @@
                 with_word = ["without", "with"][bool(id.registration_password)]
                 with_tags = ["", u"and tags %s " % tags][bool(tags)]
                 logging.info(u"Queueing message to register with account %r %s"
-                              "%s a password." % (id.account_name, with_tags,
-                              with_word))
+                             "%s a password." % (id.account_name, with_tags,
+                                                 with_word))
                 message = {"type": "register",
                            "computer_title": id.computer_title,
                            "account_name": id.account_name,
@@ -284,6 +287,12 @@
                            "tags": tags,
                            "vm-info": get_vm_info()}
                 self._exchange.send(message)
+            elif self._config.provisioning_otp:
+                logging.info(u"Queueing message to register with OTP as a"
+                             u" newly provisioned machine.")
+                message = {"type": "register-provisioned-machine",
+                           "otp": self._config.provisioning_otp}
+                self._exchange.send(message)
             else:
                 self._reactor.fire("registration-failed")
 

=== modified file 'landscape/broker/tests/test_registration.py'
--- landscape/broker/tests/test_registration.py	2011-06-15 10:08:15 +0000
+++ landscape/broker/tests/test_registration.py	2011-11-11 16:21:24 +0000
@@ -1078,3 +1078,41 @@
             self.assertFalse(is_cloud_managed(fake_fetch))
         finally:
             self.mocker.reset()
+
+
+class ProvisioningRegistrationTest(RegistrationHandlerTestBase):
+
+    def test_provisioned_machine_registration_with_otp(self):
+        """
+        Register provisioned machines using an OTP.
+        """
+        self.mstore.set_accepted_types(["register-provisioned-machine"])
+        self.config.account_name = ""
+        self.config.provisioning_otp = "ohteepee"
+        self.reactor.fire("pre-exchange")
+
+        self.assertMessages([{"otp": "ohteepee", "timestamp": 0, "api": "3.2",
+                              "type": "register-provisioned-machine"}],
+                            self.mstore.get_pending_messages())
+        self.assertEqual(u"INFO: Queueing message to register with OTP as a"
+                         u" newly provisioned machine.",
+                         self.logfile.getvalue().strip())
+
+        self.exchanger.exchange()
+        self.assertMessages([{"otp": "ohteepee", "timestamp": 0, "api": "3.2",
+                              "type": "register-provisioned-machine"}],
+                            self.transport.payloads[0]["messages"])
+
+    def test_provisioned_machine_registration_with_empty_otp(self):
+        """
+        No message should be sent when an empty OTP is passed.
+        """
+        self.mstore.set_accepted_types(["register-provisioned-machine"])
+        self.config.account_name = ""
+        self.config.provisioning_otp = ""
+        self.reactor.fire("pre-exchange")
+
+        self.assertMessages([],
+                            self.mstore.get_pending_messages())
+        self.assertEqual(u"",
+                         self.logfile.getvalue().strip())

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2011-10-20 18:17:59 +0000
+++ landscape/configuration.py	2011-11-11 16:21:24 +0000
@@ -440,8 +440,8 @@
         config.https_proxy = os.environ["https_proxy"]
 
     if config.silent and not config.no_start:
-        if not config.get("otp") and (not config.get("account_name") or not
-                                          config.get("computer_title")):
+        if not (config.get("otp") or config.provisioning_otp or
+                (config.get("account_name") and config.get("computer_title"))):
             raise ConfigurationError("An account name and computer title are "
                                      "required.")
     if config.silent:

=== modified file 'landscape/message_schemas.py'
--- landscape/message_schemas.py	2011-08-10 14:52:05 +0000
+++ landscape/message_schemas.py	2011-11-11 16:21:24 +0000
@@ -143,6 +143,12 @@
     # hostname wasn't around in old versions
     optional=["registration_password", "hostname", "tags", "vm-info"])
 
+
+REGISTER_PROVISIONED_MACHINE = Message(
+    "register-provisioned-machine",
+    {"otp": String()})
+
+
 REGISTER_CLOUD_VM = Message(
     "register-cloud-vm",
     {"hostname": utf8,
@@ -388,8 +394,8 @@
                OPERATION_RESULT, COMPUTER_INFO, DISTRIBUTION_INFO,
                HARDWARE_INVENTORY, LOAD_AVERAGE, MEMORY_INFO,
                RESYNCHRONIZE, MOUNT_ACTIVITY, MOUNT_INFO, FREE_SPACE,
-               REGISTER, REGISTER_CLOUD_VM, TEMPERATURE, PROCESSOR_INFO,
-               USERS, PACKAGES, PACKAGE_LOCKS,
+               REGISTER, REGISTER_CLOUD_VM, REGISTER_PROVISIONED_MACHINE,
+               TEMPERATURE, PROCESSOR_INFO, USERS, PACKAGES, PACKAGE_LOCKS,
                CHANGE_PACKAGES_RESULT, UNKNOWN_PACKAGE_HASHES,
                ADD_PACKAGES, PACKAGE_REPORTER_RESULT, TEXT_MESSAGE, TEST,
                CUSTOM_GRAPH, REBOOT_REQUIRED, APT_PREFERENCES, EUCALYPTUS_INFO,

=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2011-11-02 13:24:35 +0000
+++ landscape/tests/test_configuration.py	2011-11-11 16:21:24 +0000
@@ -778,7 +778,8 @@
                 "--http-proxy", "",
                 "--https-proxy", "",
                 "--otp", "",
-                "--tags", ""]
+                "--tags", "",
+                "--provisioning-otp", ""]
         config = self.get_config(args)
         setup(config)
         self.assertConfigEqual(self.get_content(config),
@@ -826,6 +827,20 @@
 
         self.assertEqual("otp1", config.otp)
 
+    def test_silent_setup_with_provisioning_otp(self):
+        """
+        If the provisioning OTP is specified, there is no need to pass the
+        account name and the computer title.
+        """
+        sysvconfig_mock = self.mocker.patch(SysVConfig)
+        sysvconfig_mock.set_start_on_boot(True)
+        self.mocker.replay()
+
+        config = self.get_config(["--silent", "--provisioning-otp", "otp1"])
+        setup(config)
+
+        self.assertEqual("otp1", config.provisioning_otp)
+
     def test_silent_script_users_imply_script_execution_plugin(self):
         """
         If C{--script-users} is specified, without C{ScriptExecution} in the