You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by knusbaum <gi...@git.apache.org> on 2016/10/05 22:21:03 UTC
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
GitHub user knusbaum opened a pull request:
https://github.com/apache/storm/pull/1724
STORM-2131: Add blob command to worker-launcher, make stormdist directory not writeable by topo owner
Re-opening this, as I reverted the last one to fix a typo (#1723)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/knusbaum/incubator-storm STORM-2131
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1724.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1724
----
commit 49514b6e9265d0510ce61ba998a53000e0b2e57e
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-10-03T19:26:24Z
Updating worker launcher for blobs.
commit 5825e14a11c3d91c10036971e7757c8e4f82283d
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-09-26T20:42:18Z
Fixing permissions for stormdist directory.
commit f930b9a7e5c6d86c5782a44f5154c81bff82d9d8
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-10-03T20:08:33Z
Fixing workers artifacts permissions.
commit 1ce0f639c53a8fd1d22a74ea78cfbdb15a81b90b
Author: Kyle Nusbaum <ky...@gmail.com>
Date: 2016-10-05T22:19:19Z
fixing typo.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1724: STORM-2131: Add blob command to worker-launcher, make sto...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/1724
+1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82815297
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -485,14 +482,14 @@ int setup_stormdist_dir(const char* local_dir) {
case FTS_DP: // A directory being visited in post-order
case FTS_DOT: // A dot directory
+ case FTS_SL: // A symbolic link
+ case FTS_SLNONE: // A broken symbolic link
--- End diff --
I talked with @tgravescs and ran some tests. It works just fine, and it should not be a big deal. I was a bit worried about the default permissions on the symlinks but they are open, and because they are all under a directory that should be locked down I am not concerned abut it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1724: STORM-2131: Add blob command to worker-launcher, make sto...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1724
Leaving a note: #1697 needs to pull this to apply the change for 1.x.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82672822
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -414,33 +414,30 @@ static int copy_file(int input, const char* in_filename,
return 0;
}
-int setup_stormdist(FTSENT* entry, uid_t euser) {
+static int setup_permissions(FTSENT* entry, uid_t euser, int user_write) {
if (lchown(entry->fts_path, euser, launcher_gid) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
mode_t mode = entry->fts_statp->st_mode;
- mode_t new_mode = (mode & (S_IRWXU)) | S_IRGRP | S_IWGRP;
- if ((mode & S_IXUSR) == S_IXUSR) {
- new_mode = new_mode | S_IXGRP;
+ mode_t new_mode = (mode & (S_IRUSR | S_IXUSR)) | S_IRGRP | S_IWGRP;
+ if (user_write) {
+ new_mode = new_mode | S_IWUSR;
--- End diff --
\U0001f44d
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1724: STORM-2131: Add blob command to worker-launcher, make sto...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1724
@kishorvpatil @revans2 @jerrypeng
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82608722
--- Diff: storm-core/src/native/worker-launcher/impl/main.c ---
@@ -155,7 +155,23 @@ int main(int argc, char **argv) {
fflush(ERRORFILE);
return INVALID_ARGUMENT_NUMBER;
}
- exit_code = setup_stormdist_dir(argv[optind]);
+ exit_code = setup_dir_permissions(argv[optind], 0);
+ } else if (strcasecmp("artifacts-dir", command) == 0) {
+ if (argc != 4) {
--- End diff --
indentation appears off
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82634841
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SupervisorUtils.java ---
@@ -104,6 +104,16 @@ public static void setupStormCodeDir(Map<String, Object> conf, Map<String, Objec
}
}
+ public static void setupWorkerArtifactsDir(Map<String, Object> conf, Map<String, Object> stormConf, String dir) throws IOException {
+ if (Utils.getBoolean(conf.get(Config.SUPERVISOR_RUN_WORKER_AS_USER), false)) {
+ String logPrefix = "setup conf for " + dir;
--- End diff --
No idea. I copied this over from setupStormCodeDir so they would look similar in the logs.
Can change both to actually make sense.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82634237
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -485,14 +482,14 @@ int setup_stormdist_dir(const char* local_dir) {
case FTS_DP: // A directory being visited in post-order
case FTS_DOT: // A dot directory
+ case FTS_SL: // A symbolic link
+ case FTS_SLNONE: // A broken symbolic link
--- End diff --
This is ported from a change @tgravescs made. We can ask him why.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82611549
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -414,33 +414,30 @@ static int copy_file(int input, const char* in_filename,
return 0;
}
-int setup_stormdist(FTSENT* entry, uid_t euser) {
+static int setup_permissions(FTSENT* entry, uid_t euser, int user_write) {
if (lchown(entry->fts_path, euser, launcher_gid) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
mode_t mode = entry->fts_statp->st_mode;
- mode_t new_mode = (mode & (S_IRWXU)) | S_IRGRP | S_IWGRP;
- if ((mode & S_IXUSR) == S_IXUSR) {
- new_mode = new_mode | S_IXGRP;
+ mode_t new_mode = (mode & (S_IRUSR | S_IXUSR)) | S_IRGRP | S_IWGRP;
+ if (user_write) {
+ new_mode = new_mode | S_IWUSR;
}
if ((mode & S_IFDIR) == S_IFDIR) {
- new_mode = new_mode | S_ISGID;
+ new_mode = new_mode | S_IXGRP | S_ISGID;
}
if (chmod(entry->fts_path, new_mode) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
return 0;
}
-int setup_stormdist_dir(const char* local_dir) {
- //This is the same as
- //> chmod g+rwX -R $local_dir
- //> chown -no-dereference -R $user:$supervisor-group $local_dir
--- End diff --
Again a comment like this explaining what the program does would be very valuable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1724
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82608542
--- Diff: storm-core/src/jvm/org/apache/storm/daemon/supervisor/SupervisorUtils.java ---
@@ -104,6 +104,16 @@ public static void setupStormCodeDir(Map<String, Object> conf, Map<String, Objec
}
}
+ public static void setupWorkerArtifactsDir(Map<String, Object> conf, Map<String, Object> stormConf, String dir) throws IOException {
+ if (Utils.getBoolean(conf.get(Config.SUPERVISOR_RUN_WORKER_AS_USER), false)) {
+ String logPrefix = "setup conf for " + dir;
--- End diff --
conf???
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82612830
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -485,14 +482,14 @@ int setup_stormdist_dir(const char* local_dir) {
case FTS_DP: // A directory being visited in post-order
case FTS_DOT: // A dot directory
+ case FTS_SL: // A symbolic link
+ case FTS_SLNONE: // A broken symbolic link
--- End diff --
I'm also not sure why we want the symlink/a broken symlink to be a noop? We use lchown to not follow the symlink, so we will be sure to change ownership of the symlink itself (otherwise it will still be owned by the supervisor user). Was chmod causing issues? If so we probably want to split the chown and chmod portions and only call chmod on things that are not symlinks.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82672834
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -414,33 +414,30 @@ static int copy_file(int input, const char* in_filename,
return 0;
}
-int setup_stormdist(FTSENT* entry, uid_t euser) {
+static int setup_permissions(FTSENT* entry, uid_t euser, int user_write) {
if (lchown(entry->fts_path, euser, launcher_gid) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
mode_t mode = entry->fts_statp->st_mode;
- mode_t new_mode = (mode & (S_IRWXU)) | S_IRGRP | S_IWGRP;
- if ((mode & S_IXUSR) == S_IXUSR) {
- new_mode = new_mode | S_IXGRP;
+ mode_t new_mode = (mode & (S_IRUSR | S_IXUSR)) | S_IRGRP | S_IWGRP;
+ if (user_write) {
+ new_mode = new_mode | S_IWUSR;
}
if ((mode & S_IFDIR) == S_IFDIR) {
- new_mode = new_mode | S_ISGID;
+ new_mode = new_mode | S_IXGRP | S_ISGID;
}
if (chmod(entry->fts_path, new_mode) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
return 0;
}
-int setup_stormdist_dir(const char* local_dir) {
- //This is the same as
- //> chmod g+rwX -R $local_dir
- //> chown -no-dereference -R $user:$supervisor-group $local_dir
--- End diff --
\U0001f44d
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #1724: STORM-2131: Add blob command to worker-launcher, m...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1724#discussion_r82611232
--- Diff: storm-core/src/native/worker-launcher/impl/worker-launcher.c ---
@@ -414,33 +414,30 @@ static int copy_file(int input, const char* in_filename,
return 0;
}
-int setup_stormdist(FTSENT* entry, uid_t euser) {
+static int setup_permissions(FTSENT* entry, uid_t euser, int user_write) {
if (lchown(entry->fts_path, euser, launcher_gid) != 0) {
- fprintf(ERRORFILE, "Failure to exec app initialization process - %s\n",
- strerror(errno));
+ fprintf(ERRORFILE, "Failure to exec app initialization process - %s, fts_path=%s\n",
+ strerror(errno), entry->fts_path);
return -1;
}
mode_t mode = entry->fts_statp->st_mode;
- mode_t new_mode = (mode & (S_IRWXU)) | S_IRGRP | S_IWGRP;
- if ((mode & S_IXUSR) == S_IXUSR) {
- new_mode = new_mode | S_IXGRP;
+ mode_t new_mode = (mode & (S_IRUSR | S_IXUSR)) | S_IRGRP | S_IWGRP;
+ if (user_write) {
+ new_mode = new_mode | S_IWUSR;
--- End diff --
It was not completely obvious from reading the code what was happening here. Could we put in some comments at a high level how the permissions are setup and why they are setup that way.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1724: STORM-2131: Add blob command to worker-launcher, make sto...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/1724
+1 looks good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---