Permissions of deployed files and keep node.id #270

rschlussel-zz · 2016-09-23T18:55:48Z

fixes #266

rschlussel-zz · 2016-09-23T18:59:27Z

prestoadmin/deploy.py

-    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:


we need to do this because the rpm doesn't set up the config files with correct ownership (see Teradata/presto#358)

rschlussel-zz · 2016-09-23T19:05:01Z

@petroav @ebd2

petroav

Everything looks good except for one question on the default permissions on the connector directory.

petroav · 2016-09-26T15:51:04Z

prestoadmin/deploy.py

+    missing_owner_code = 42
+    command = \
+        "( getent passwd {user} >/dev/null || exit {missing_owner_code} ) &&" \
+        " mkdir -p {filepath} && " \


Can you put the space at the beginning of this line at the end of the line above?

petroav · 2016-09-26T15:52:14Z

prestoadmin/deploy.py

@@ -118,6 +118,26 @@ def secure_create_file(filepath, user_group, mode=600):
            abort("Failed to securely create file %s" % (filepath))


+def secure_create_directory(filepath, user_group, mode=755):


I thought the directory had to have permissions 644. Why is the default 755?

it should have 755 permissions so that it is traversable. I'll fix the commit message

petroav · 2016-09-26T15:53:33Z

prestoadmin/deploy.py

+        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,))


In (user,) what's the comma for? Is it required?

The comma makes this a tuple, but I'll change this to not be a tuple. when only one argument is needed to % operator, you don't need a tuple

petroav · 2016-09-26T15:55:42Z

tests/product/base_product_case.py

@@ -269,6 +269,13 @@ def assert_config_perms(self, host, filepath):
        self.assert_file_perm_owner(
            host, filepath, '-rw-------', 'presto', 'presto')

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))


Same question about the comma here as well.

same answer

ebd2 · 2016-09-26T21:58:44Z

prestoadmin/connector.py

@@ -41,7 +42,7 @@

 def deploy_files(filenames, local_dir, remote_dir, user_group, mode=0600):


Please comment explaining why the mode for connectors is different than for regular config now that they're different.

ebd2 · 2016-09-26T22:01:19Z

tests/product/base_product_case.py

@@ -267,8 +267,19 @@ def get_file_content(self, host, filepath):

    def assert_config_perms(self, host, filepath):
        self.assert_file_perm_owner(
+            host, filepath, '-rw-r--r--', 'presto', 'presto')
+
+    def assert_connnector_perms(self, host, filepath):


typo: connnector

ebd2 · 2016-09-26T22:03:23Z

tests/product/base_product_case.py

            host, filepath, '-rw-------', 'presto', 'presto')

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))


Everything but the ls command is the same in this and assert_file_perm_owner. Please consolidate.

ebd2 · 2016-09-26T22:06:51Z

tests/product/base_product_case.py

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))
+        fields = ls.split()
+        self.assertEqual(fields[0], permissions)


we should probably assert that the expected perms includes the 'd' or 'l' at the beginning or there's no way the perms are going to match. This assertion will fail, but it's misleading because it's the expected value that's totally bogus.

Actually, let's not worry about symlinks for now...

rschlussel-zz

Addressed comments

rschlussel-zz · 2016-09-27T17:28:26Z

prestoadmin/deploy.py

@@ -118,6 +118,26 @@ def secure_create_file(filepath, user_group, mode=600):
            abort("Failed to securely create file %s" % (filepath))


+def secure_create_directory(filepath, user_group, mode=755):


it should have 755 permissions so that it is traversable. I'll fix the commit message

rschlussel-zz · 2016-09-27T17:29:50Z

prestoadmin/deploy.py

+    missing_owner_code = 42
+    command = \
+        "( getent passwd {user} >/dev/null || exit {missing_owner_code} ) &&" \
+        " mkdir -p {filepath} && " \


rschlussel-zz · 2016-09-27T17:35:44Z

prestoadmin/deploy.py

+        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,))


The comma makes this a tuple, but I'll change this to not be a tuple. when only one argument is needed to % operator, you don't need a tuple

rschlussel-zz · 2016-09-27T17:35:47Z

tests/product/base_product_case.py

@@ -269,6 +269,13 @@ def assert_config_perms(self, host, filepath):
        self.assert_file_perm_owner(
            host, filepath, '-rw-------', 'presto', 'presto')

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))


same answer

rschlussel-zz · 2016-09-27T17:36:11Z

tests/product/base_product_case.py

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))
+        fields = ls.split()
+        self.assertEqual(fields[0], permissions)


rschlussel-zz · 2016-09-27T18:03:34Z

tests/product/base_product_case.py

@@ -267,8 +267,19 @@ def get_file_content(self, host, filepath):

    def assert_config_perms(self, host, filepath):
        self.assert_file_perm_owner(
+            host, filepath, '-rw-r--r--', 'presto', 'presto')
+
+    def assert_connnector_perms(self, host, filepath):


rschlussel-zz · 2016-09-27T18:04:07Z

prestoadmin/connector.py

@@ -41,7 +42,7 @@

 def deploy_files(filenames, local_dir, remote_dir, user_group, mode=0600):


rschlussel-zz · 2016-09-27T18:12:25Z

tests/product/base_product_case.py

            host, filepath, '-rw-------', 'presto', 'presto')

+    def assert_directory_perm_owner(self, host, filepath, permissions, owner, group):
+        ls = self.cluster.exec_cmd_on_host(host, "ls -l -d %s" % (filepath,))


The catalog directory should be owned by presto:presto and have 755 permissions.

Keep the node.id consistent across multiple runs of configuration deploy. Instead of overwriting the file if it exists, append to it, and just make sure it has the right permissions and ownership.

rschlussel-zz commented Sep 23, 2016

View reviewed changes

petroav approved these changes Sep 26, 2016

View reviewed changes

ebd2 reviewed Sep 26, 2016

View reviewed changes

rschlussel-zz commented Sep 27, 2016

View reviewed changes

rschlussel-zz force-pushed the deploy branch 2 times, most recently from c553b77 to ae6d73d Compare September 27, 2016 18:42

rschlussel-zz added 4 commits September 29, 2016 16:13

Update pip version to 8.1.2

9eda9d3

Give remote catalog directory correct permissions

4bff7aa

The catalog directory should be owned by presto:presto and have 755 permissions.

Change permissions on config files to 644

a4e091d

Keep node.id across configuration updates

c38b7dc

Keep the node.id consistent across multiple runs of configuration deploy. Instead of overwriting the file if it exists, append to it, and just make sure it has the right permissions and ownership.

rschlussel-zz force-pushed the deploy branch from ae6d73d to c38b7dc Compare September 29, 2016 20:29

rschlussel-zz merged commit c38b7dc into prestodb:master Sep 29, 2016

rschlussel-zz deleted the deploy branch November 14, 2016 20:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Permissions of deployed files and keep node.id #270

Permissions of deployed files and keep node.id #270

rschlussel-zz commented Sep 23, 2016

rschlussel-zz Sep 23, 2016

rschlussel-zz commented Sep 23, 2016

petroav left a comment

petroav Sep 26, 2016

rschlussel-zz Sep 27, 2016

petroav Sep 26, 2016

rschlussel-zz Sep 27, 2016

petroav Sep 26, 2016

rschlussel-zz Sep 27, 2016

petroav Sep 26, 2016

rschlussel-zz Sep 27, 2016 •

edited

Loading

ebd2 Sep 26, 2016 •

edited

Loading

rschlussel-zz Sep 27, 2016

ebd2 Sep 26, 2016

rschlussel-zz Sep 27, 2016

ebd2 Sep 26, 2016

rschlussel-zz Sep 27, 2016

ebd2 Sep 26, 2016

ebd2 Sep 26, 2016

rschlussel-zz Sep 27, 2016

rschlussel-zz left a comment

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016 •

edited

Loading

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016

rschlussel-zz Sep 27, 2016

		@@ -118,6 +118,26 @@ def secure_create_file(filepath, user_group, mode=600):
		abort("Failed to securely create file %s" % (filepath))


		def secure_create_directory(filepath, user_group, mode=755):

		@@ -41,7 +42,7 @@

		def deploy_files(filenames, local_dir, remote_dir, user_group, mode=0600):

Permissions of deployed files and keep node.id #270

Permissions of deployed files and keep node.id #270

Conversation

rschlussel-zz commented Sep 23, 2016

Choose a reason for hiding this comment

rschlussel-zz commented Sep 23, 2016

petroav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

ebd2 Sep 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschlussel-zz Sep 27, 2016 •

edited

Loading

ebd2 Sep 26, 2016 •

edited

Loading

rschlussel-zz Sep 27, 2016 •

edited

Loading