Permissions of deployed files and keep node.id

prestoadmin/configure_cmds.py

@@ -93,7 +93,7 @@ def deploy_all(source_directory, should_warn=True):
                      % (env.host, local_config_file))
             continue
         remote_config_file = os.path.join(constants.REMOTE_CONF_DIR, file_name)
-        put_secure(PRESTO_STANDALONE_USER_GROUP, 0600, local_config_file,
+        put_secure(PRESTO_STANDALONE_USER_GROUP, 0644, local_config_file,
                    remote_config_file, use_sudo=True)
 
 

prestoadmin/connector.py

@@ -24,6 +24,7 @@
 from fabric.contrib import files
 from fabric.operations import sudo, os, get
 
+from prestoadmin.deploy import secure_create_directory
 from prestoadmin.standalone.config import StandaloneConfig, \
     PRESTO_STANDALONE_USER_GROUP
 from prestoadmin.util import constants
@@ -40,9 +41,11 @@
 COULD_NOT_REMOVE = 'Could not remove connector'
 
 
+# we deploy connector files with 0600 permissions because they can contain passwords
+# that should not be world readable
 def deploy_files(filenames, local_dir, remote_dir, user_group, mode=0600):
     _LOGGER.info('Deploying configurations for ' + str(filenames))
-    sudo('mkdir -p ' + remote_dir)
+    secure_create_directory(remote_dir, PRESTO_STANDALONE_USER_GROUP)
     for name in filenames:
         put_secure(user_group, mode, os.path.join(local_dir, name), remote_dir,
                    use_sudo=True)

prestoadmin/deploy.py

@@ -22,6 +22,7 @@
 
 from fabric.contrib import files
 from fabric.context_managers import settings
+from fabric.contrib.files import exists
 from fabric.operations import sudo, abort
 from fabric.api import env
 
@@ -95,7 +96,7 @@ def deploy(confs, remote_dir):
     sudo("mkdir -p " + remote_dir)
     for name, content in confs.iteritems():
         write_to_remote_file(content, os.path.join(remote_dir, name),
-                             'presto')
+                             owner=PRESTO_STANDALONE_USER_GROUP, mode=644)
 
 
 def secure_create_file(filepath, user_group, mode=600):
@@ -113,7 +114,27 @@ def secure_create_file(filepath, user_group, mode=600):
         result = sudo(command)
         if result.return_code == missing_owner_code:
             abort("User %s does not exist. Make sure the Presto server RPM "
-                  "is installed and try again" % (user,))
+                  "is installed and try again" % user)
+        elif result.failed:
+            abort("Failed to securely create file %s" % (filepath))
+
+
+def secure_create_directory(filepath, user_group, mode=755):
+    user, group = user_group.split(':')
+    missing_owner_code = 42
+    command = \
+        "( getent passwd {user} >/dev/null || exit {missing_owner_code} ) && " \
+        "mkdir -p {filepath} && " \
+        "chown {user_group} {filepath} && " \
+        "chmod {mode} {filepath} ".format(
+            filepath=filepath, user=user, user_group=user_group, mode=mode,
+            missing_owner_code=missing_owner_code)
+
+    with settings(warn_only=True):
+        result = sudo(command)
+        if result.return_code == missing_owner_code:
+            abort("User %s does not exist. Make sure the Presto server RPM "
+                  "is installed and try again" % user)
         elif result.failed:
             abort("Failed to securely create file %s" % (filepath))
 
@@ -122,7 +143,11 @@ def deploy_node_properties(content, remote_dir):
     _LOGGER.info("Deploying node.properties configuration")
     name = "node.properties"
     node_file_path = (os.path.join(remote_dir, name))
-    secure_create_file(node_file_path, PRESTO_STANDALONE_USER_GROUP)
+    if not exists(node_file_path, use_sudo=True):
+        secure_create_file(node_file_path, PRESTO_STANDALONE_USER_GROUP, mode=644)
+    else:
+        sudo('chown %(owner)s %(filepath)s && chmod %(mode)s %(filepath)s'
+             % {'owner': PRESTO_STANDALONE_USER_GROUP, 'mode': 644, 'filepath': node_file_path})
     node_id_command = (
         "if ! ( grep -q -s 'node.id' " + node_file_path + " ); then "
         "uuid=$(uuidgen); "
@@ -135,7 +160,7 @@ def deploy_node_properties(content, remote_dir):
 
 
 def write_to_remote_file(text, filepath, owner, mode=600):
-    secure_create_file(filepath, PRESTO_STANDALONE_USER_GROUP)
+    secure_create_file(filepath, owner, mode)
     command = "echo '{text}' > {filepath}".format(
         text=escape_single_quotes(text), filepath=filepath)
     sudo(command)

requirements.txt

@@ -17,5 +17,5 @@ fudge==1.1.0
 PyYAML==3.11
 overrides==0.5
 setuptools==20.1.1
-pip==7.1.2
+pip==8.1.2
 retrying==1.3.3

setup.py

@@ -50,7 +50,7 @@
     'fabric==1.10.1',
     'requests==2.7.0',
     'overrides==0.5',
-    'pip==7.1.2',
+    'pip==8.1.2',
     'setuptools==20.1.1',
     'wheel==0.23.0',
     'flake8==2.5.4',

tests/product/base_product_case.py

@@ -280,13 +280,24 @@ def get_file_content(self, host, filepath):
         return self.cluster.exec_cmd_on_host(host, 'cat %s' % (filepath))
 
     def assert_config_perms(self, host, filepath):
+        self.assert_file_perm_owner(
+            host, filepath, '-rw-r--r--', 'presto', 'presto')
+
+    def assert_connector_perms(self, host, filepath):
         self.assert_file_perm_owner(
             host, filepath, '-rw-------', 'presto', 'presto')
 
-    def assert_file_perm_owner(
-            self, host, filepath, permissions, owner, group):
-        ls = self.cluster.exec_cmd_on_host(host, "ls -l %s" % (filepath,))
-        fields = ls.split()
+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        self.assertEqual(permissions[0], 'd', 'expected permissions should begin with a d')
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % filepath)
+        self.assert_perm_owner(permissions, owner, group, ls)
+
+    def assert_file_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l %s" % filepath)
+        self.assert_perm_owner(permissions, owner, group, ls)
+
+    def assert_perm_owner(self, permissions, owner, group, actual):
+        fields = actual.split()
         self.assertEqual(fields[0], permissions)
         self.assertEqual(fields[2], owner)
         self.assertEqual(fields[3], group)
@@ -340,11 +351,13 @@ def assert_file_content_regex(self, host, filepath, expected):
         config = self.get_file_content(host, filepath)
         self.assertRegexpMatches(config, expected)
 
-    def assert_has_default_connector(self, container):
-        filepath = '/etc/presto/catalog/tpch.properties'
+    def assert_has_default_connector(self, host):
+        catalog_dir = constants.REMOTE_CATALOG_DIR
+        self.assert_directory_perm_owner(host, catalog_dir, 'drwxr-xr-x', 'presto', 'presto')
 
-        self.assert_config_perms(container, filepath)
-        self.assert_file_content(container, filepath, 'connector.name=tpch')
+        filepath = os.path.join(catalog_dir, 'tpch.properties')
+        self.assert_connector_perms(host, filepath)
+        self.assert_file_content(host, filepath, 'connector.name=tpch')
 
     def assert_has_jmx_connector(self, container):
         self.assert_file_content(container,
@@ -375,22 +388,23 @@ def assert_has_default_config(self, host):
             self.assert_file_content(host, config_properties_path,
                                      self.default_coordinator_test_config_)
 
-    def assert_node_config(self, host, expected):
+    def assert_node_config(self, host, expected, expected_node_id=None):
         node_properties_path = '/etc/presto/node.properties'
         self.assert_config_perms(host, node_properties_path)
         node_properties = self.cluster.exec_cmd_on_host(
             host, 'cat %s' % (node_properties_path,))
         split_properties = node_properties.split('\n', 1)
-        self.assertRegexpMatches(split_properties[0], 'node.id=.*')
+        if expected_node_id:
+            self.assertEqual(expected_node_id, split_properties[0])
+        else:
+            self.assertRegexpMatches(split_properties[0], 'node.id=.*')
         actual = split_properties[1]
         if host in self.cluster.slaves:
             conf_dir = get_workers_directory()
         else:
             conf_dir = get_coordinator_directory()
         self.assertLazyMessage(
-            lambda: self.file_content_message(actual, expected,
-                                              os.path.join(conf_dir,
-                                                           'node.properties')),
+            lambda: self.file_content_message(actual, expected, os.path.join(conf_dir, 'node.properties')),
             self.assertEqual,
             actual,
             expected)

tests/product/test_configuration.py

@@ -65,11 +65,17 @@ def __write_dummy_config_file(self):
         self.write_test_configs(self.cluster, extra_configs)
         return dummy_prop1, dummy_prop2
 
+    def _get_node_id(self, host):
+        return self.cluster.exec_cmd_on_host(host, 'grep node.id= /etc/presto/node.properties').strip()
+
     @attr('smoketest')
     def test_configuration_deploy_show(self):
         self.upload_topology()
 
         self.deploy_and_assert_default_config()
+        node_ids = {}
+        for host in self.cluster.all_hosts():
+            node_ids[host] = self._get_node_id(host)
 
         # deploy coordinator configuration only.  Has a non-default file
         dummy_prop1, dummy_prop2 = self.__write_dummy_config_file()
@@ -78,8 +84,8 @@ def test_configuration_deploy_show(self):
         deploy_template = 'Deploying configuration on: %s\n'
         self.assertEqual(output,
                          deploy_template % self.cluster.internal_master)
-        for container in self.cluster.slaves:
-            self.assert_has_default_config(container)
+        for host in self.cluster.slaves:
+            self.assert_has_default_config(host)
 
         config_properties_path = os.path.join(
             constants.REMOTE_CONF_DIR, 'config.properties')
@@ -105,18 +111,19 @@ def test_configuration_deploy_show(self):
             expected += deploy_template % host
         self.assertEqualIgnoringOrder(output, expected)
 
-        for container in self.cluster.slaves:
-            self.assert_config_perms(container, config_properties_path)
-            self.assert_file_content(container,
+        for host in self.cluster.slaves:
+            self.assert_config_perms(host, config_properties_path)
+            self.assert_file_content(host,
                                      config_properties_path,
                                      dummy_prop1 + '\n' +
                                      dummy_prop2 + '\n' +
                                      self.default_workers_test_config_)
             expected = 'node.environment=test\n'
-            self.assert_node_config(container, expected)
+            self.assert_node_config(host, expected, node_ids[host])
 
         self.assert_node_config(self.cluster.master,
-                                self.default_node_properties_)
+                                self.default_node_properties_,
+                                node_ids[self.cluster.master])
 
     def test_configuration_deploy_using_dash_h_coord_worker(self):
         self.upload_topology()

tests/product/test_connectors.py

@@ -66,8 +66,7 @@ def test_connector_add_remove(self):
         for host in self.cluster.all_hosts():
             filepath = '/etc/presto/catalog/jmx.properties'
             self.assert_has_default_connector(host)
-            self.assert_file_perm_owner(host, filepath,
-                                        '-rw-------', 'presto', 'presto')
+            self.assert_connector_perms(host, filepath)
             self.assert_file_content(host, filepath, 'connector.name=jmx')
         self._assert_connectors_loaded([['system'], ['jmx'], ['tpch']])
 

tests/unit/test_connector.py

@@ -149,14 +149,14 @@ def test_remove_os_error(self, remove_file_mock, remove_mock):
         self.assertRaisesRegexp(OSError, 'Permission denied',
                                 connector.remove, 'tpch')
 
-    @patch('prestoadmin.connector.sudo')
+    @patch('prestoadmin.connector.secure_create_directory')
     @patch('prestoadmin.util.fabricapi.put')
-    def test_deploy_files(self, put_mock, sudo_mock):
+    def test_deploy_files(self, put_mock, create_dir_mock):
         local_dir = '/my/local/dir'
         remote_dir = '/my/remote/dir'
         connector.deploy_files(['a', 'b'], local_dir, remote_dir,
                                PRESTO_STANDALONE_USER_GROUP)
-        sudo_mock.assert_called_with('mkdir -p %s' % remote_dir)
+        create_dir_mock.assert_called_with(remote_dir, PRESTO_STANDALONE_USER_GROUP)
         put_mock.assert_any_call('/my/local/dir/a', remote_dir, use_sudo=True,
                                  mode=0600)
         put_mock.assert_any_call('/my/local/dir/b', remote_dir, use_sudo=True,

tests/unit/test_deploy.py

@@ -76,10 +76,12 @@ def test_deploy(self, sudo_mock):
         sudo_mock.assert_any_call("echo 'a=b' > /my/remote/dir/jvm.config")
 
     @patch('__builtin__.open')
+    @patch('prestoadmin.deploy.exists')
     @patch('prestoadmin.deploy.files.append')
     @patch('prestoadmin.deploy.sudo')
-    def test_deploy_node_properties(self, sudo_mock, append_mock, open_mock):
+    def test_deploy_node_properties(self, sudo_mock, append_mock, exists_mock, open_mock):
         sudo_mock.return_value = SudoResult()
+        exists_mock.return_value = True
         file_manager = open_mock.return_value.__enter__.return_value
         file_manager.read.return_value = ("key=value")
         command = (
@@ -94,6 +96,13 @@ def test_deploy_node_properties(self, sudo_mock, append_mock, open_mock):
         append_mock.assert_called_with("/my/remote/dir/node.properties",
                                        "key=value", True, shell=True)
 
+    @patch('prestoadmin.deploy.sudo')
+    @patch('prestoadmin.deploy.secure_create_file')
+    def test_deploys_as_presto_user(self, secure_create_file_mock, sudo_mock):
+        deploy.deploy({'my_file': 'hello!'}, '/remote/path')
+        secure_create_file_mock.assert_called_with('/remote/path/my_file', 'presto:presto', 644)
+        sudo_mock.assert_called_with("echo 'hello!' > /remote/path/my_file")
+
     @patch('prestoadmin.deploy.deploy')
     @patch('prestoadmin.deploy.deploy_node_properties')
     def test_configure_presto(self, deploy_node_mock, deploy_mock):