Block re-use of master/node as load balancer in attended install.

Code was present to catch this in unattended installs but was looking for a
host record with both master/node and master_lb set to true, but in the
attended installs we were adding a separate host record with the same
connect_to.

Attended tests can now optionally specify multiple "attempted" strings for the
master_lb specification, we'll try to input each if multiple are specified.

Cleanup some empty defaults and error messages as well.

3 files changed, 59 insertions, 18 deletions

diff --git a/utils/src/ooinstall/cli_installer.py b/utils/src/ooinstall/cli_installer.py
index d7c06745e..a016a5597 100644
--- a/utils/src/ooinstall/cli_installer.py
+++ b/utils/src/ooinstall/cli_installer.py
@@ -106,8 +106,7 @@ http://docs.openshift.com/enterprise/latest/architecture/infrastructure_componen
     num_masters = 0
     while more_hosts:
         host_props = {}
-        host_props['connect_to'] = click.prompt('Enter hostname or IP address:',
-                                                default='',
+        host_props['connect_to'] = click.prompt('Enter hostname or IP address',
                                                 value_proc=validate_prompt_hostname)
 
         if not masters_set:
@@ -144,13 +143,17 @@ http://docs.openshift.com/enterprise/latest/architecture/infrastructure_componen
             more_hosts = click.confirm('Do you want to add additional hosts?')
 
     if num_masters > 1:
-        hosts.append(collect_master_lb())
+        collect_master_lb(hosts)
 
     return hosts
 
-def collect_master_lb():
+def collect_master_lb(hosts):
     """
-    Get an HA proxy from the user
+    Get a valid load balancer from the user and append it to the list of
+    hosts.
+
+    Ensure user does not specify a system already used as a master/node as
+    this is an invalid configuration.
     """
     message = """
 Setting up High Availability Masters requires a load balancing solution.
@@ -166,17 +169,28 @@ hostname.
 """
     click.echo(message)
     host_props = {}
-    host_props['connect_to'] = click.prompt('Enter hostname or IP address:',
-                                            default='',
-                                            value_proc=validate_prompt_hostname)
+
+    # Using an embedded function here so we have access to the hosts list:
+    def validate_prompt_lb(hostname):
+        # Run the standard hostname check first:
+        hostname = validate_prompt_hostname(hostname)
+
+        # Make sure this host wasn't already specified:
+        for host in hosts:
+            if host.connect_to == hostname and (host.master or host.node):
+                raise click.BadParameter('Cannot re-use "%s" as a load balancer, '
+                                         'please specify a separate host' % hostname)
+        return hostname
+
+    host_props['connect_to'] = click.prompt('Enter hostname or IP address',
+                                            value_proc=validate_prompt_lb)
     install_haproxy = click.confirm('Should the reference haproxy load balancer be installed on this host?')
     host_props['preconfigured'] = not install_haproxy
     host_props['master'] = False
     host_props['node'] = False
     host_props['master_lb'] = True
     master_lb = Host(**host_props)
-
-    return master_lb
+    hosts.append(master_lb)
 
 def confirm_hosts_facts(oo_cfg, callback_facts):
     hosts = oo_cfg.hosts
@@ -261,6 +275,7 @@ def check_hosts_config(oo_cfg):
             if master_lb[0].master or master_lb[0].node:
                 click.echo('The Master load balancer is configured as a master or node. Please correct this.')
                 sys.exit(0)
+            # Check for another host with same connect_to?
         else:
             message = """
 No HAProxy given in config. Either specify one or provide a load balancing solution
diff --git a/utils/src/ooinstall/oo_config.py b/utils/src/ooinstall/oo_config.py
index b6f0cdce3..37aaf9197 100644
--- a/utils/src/ooinstall/oo_config.py
+++ b/utils/src/ooinstall/oo_config.py
@@ -50,8 +50,8 @@ class Host(object):
         self.containerized = kwargs.get('containerized', False)
 
         if self.connect_to is None:
-            raise OOConfigInvalidHostError("You must specify either and 'ip' " \
-                                           "or 'hostname' to connect to.")
+            raise OOConfigInvalidHostError("You must specify either an ip " \
+                "or hostname as 'connect_to'")
 
         if self.master is False and self.node is False and self.master_lb is False:
             raise OOConfigInvalidHostError(
diff --git a/utils/test/cli_installer_tests.py b/utils/test/cli_installer_tests.py
index 8e9c2d698..2891cefcc 100644
--- a/utils/test/cli_installer_tests.py
+++ b/utils/test/cli_installer_tests.py
@@ -568,8 +568,9 @@ class UnattendedCliTests(OOCliFixture):
         self.cli_args.extend(["-c", config_file, "install"])
         result = self.runner.invoke(cli.cli, self.cli_args)
 
-        assert result.exit_code == 1
-        assert result.output == "You must specify either and 'ip' or 'hostname' to connect to.\n"
+        self.assertEquals(1, result.exit_code)
+        self.assertTrue("You must specify either an ip or hostname"
+            in result.output)
 
     #unattended with two masters, one node, and haproxy
     @patch('ooinstall.openshift_ansible.run_main_playbook')
@@ -651,8 +652,12 @@ class AttendedCliTests(OOCliFixture):
                     inputs.append('n')  # Done adding hosts
                 i += 1
 
+        # You can pass a single master_lb or a list if you intend for one to get rejected:
         if master_lb:
-            inputs.append(master_lb[0])
+            if type(master_lb[0]) is list or type(master_lb[0]) is tuple:
+                inputs.extend(master_lb[0])
+            else:
+                inputs.append(master_lb[0])
             inputs.append('y' if master_lb[1] else 'n')
 
         # TODO: support option 2, fresh install
@@ -801,7 +806,7 @@ class AttendedCliTests(OOCliFixture):
     #interactive multimaster: one more node than master
     @patch('ooinstall.openshift_ansible.run_main_playbook')
     @patch('ooinstall.openshift_ansible.load_system_facts')
-    def test_quick_ha1(self, load_facts_mock, run_playbook_mock):
+    def test_ha_dedicated_node(self, load_facts_mock, run_playbook_mock):
         load_facts_mock.return_value = (MOCK_FACTS_QUICKHA, 0)
         run_playbook_mock.return_value = 0
 
@@ -836,10 +841,10 @@ class AttendedCliTests(OOCliFixture):
         self.assertEquals('False',
             inventory.get('nodes', '10.0.0.4  openshift_schedulable'))
 
-    #interactive multimaster: equal number masters and nodes
+    #interactive multimaster: identical masters and nodes
     @patch('ooinstall.openshift_ansible.run_main_playbook')
     @patch('ooinstall.openshift_ansible.load_system_facts')
-    def test_quick_ha2(self, load_facts_mock, run_playbook_mock):
+    def test_ha_no_dedicated_nodes(self, load_facts_mock, run_playbook_mock):
         load_facts_mock.return_value = (MOCK_FACTS_QUICKHA, 0)
         run_playbook_mock.return_value = 0
 
@@ -871,6 +876,27 @@ class AttendedCliTests(OOCliFixture):
         self.assertEquals('True',
             inventory.get('nodes', '10.0.0.3  openshift_schedulable'))
 
+    #interactive multimaster: attempting to use a master as the load balancer should fail:
+    @patch('ooinstall.openshift_ansible.run_main_playbook')
+    @patch('ooinstall.openshift_ansible.load_system_facts')
+    def test_ha_reuse_master_as_lb(self, load_facts_mock, run_playbook_mock):
+        load_facts_mock.return_value = (MOCK_FACTS_QUICKHA, 0)
+        run_playbook_mock.return_value = 0
+
+        cli_input = self._build_input(hosts=[
+                                      ('10.0.0.1', True),
+                                      ('10.0.0.2', True),
+                                      ('10.0.0.3', False),
+                                      ('10.0.0.4', True)],
+                                      ssh_user='root',
+                                      variant_num=1,
+                                      confirm_facts='y',
+                                      master_lb=(['10.0.0.2', '10.0.0.5'], False))
+        self.cli_args.append("install")
+        result = self.runner.invoke(cli.cli, self.cli_args,
+            input=cli_input)
+        self.assert_result(result, 0)
+
     #interactive all-in-one
     @patch('ooinstall.openshift_ansible.run_main_playbook')
     @patch('ooinstall.openshift_ansible.load_system_facts')