You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by Ethanlm <gi...@git.apache.org> on 2017/07/06 21:52:11 UTC
[GitHub] storm pull request #2189: add topology readonly user configuration
GitHub user Ethanlm opened a pull request:
https://github.com/apache/storm/pull/2189
add topology readonly user configuration
Problem: Listing a user in topology.users means that user can see the topology's storm UI, view logs, but also affect the topology, kill it, restart a worker, do profiling or heap dumps. We want to give some users access to UI and logs but not let them impact the topology.
Solution: Add in some new configs for TOPOLOGY_UI_USERS and TOPOLOGY_UI_GROUPS, and then split the get operations off from the others in SimpleACLAuthorizer
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/Ethanlm/storm STORM-2615
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2189.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 #2189
----
commit c9a7f91f903faa1430b2a0e36e38e226063fbe55
Author: Ethan Li <et...@gmail.com>
Date: 2017-07-06T21:21:51Z
add topology readonly user configuration
----
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2189
Merged via 2f2e9e11d0bb7c8d246517320a47337ff8b5bd19
---
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 #2189: add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
I included some unit tests. I am working on some integration tests.
---
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 #2189: [STORM-2615] Add topology readonly user configurat...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2189#discussion_r126314485
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -157,6 +162,25 @@ public boolean permit(ReqContext context, String operation, Map<String, Object>
}
if (checkUserGroupAllowed(userGroups, topoGroups)) return true;
+
+ if (_topoReadOnlyCommands.contains(operation)){
+ Set topoReadOnlyUsers = new HashSet<String>();
--- End diff --
Thanks. I made the change and pushed the new code
---
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 #2189: [STORM-2615] Add topology readonly user configurat...
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2189#discussion_r126321937
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -157,6 +162,25 @@ public boolean permit(ReqContext context, String operation, Map<String, Object>
}
if (checkUserGroupAllowed(userGroups, topoGroups)) return true;
+
+ if (_topoReadOnlyCommands.contains(operation)){
--- End diff --
Will do. Thanks
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
Squashed two commits.
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the issue:
https://github.com/apache/storm/pull/2189
LGTM
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2189
+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 issue #2189: [STORM-2615] Add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
I tested it manually. It works.
---
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 #2189: [STORM-2615] Add topology readonly user configurat...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2189#discussion_r126318865
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -157,6 +162,25 @@ public boolean permit(ReqContext context, String operation, Map<String, Object>
}
if (checkUserGroupAllowed(userGroups, topoGroups)) return true;
+
+ if (_topoReadOnlyCommands.contains(operation)){
--- End diff --
The two code blocks L150-L164 and L167-L182 are duplicated except some parameters. Could we extract this to a new method?
---
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 #2189: [STORM-2615] Add topology readonly user configurat...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2189
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
Instead of using TOPOLOGY_UI_USERS and TOPOLOGY_UI_GROUPS, I am using TOPOLOGY_READONLY_USERS and TOPOLOGY_READONLY_GROUPS for clarity.
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
I have been thinking about the names of the method, parameters and variables. I am not sure if the submitted code is good. I hope to learn from you. Thanks.
---
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 #2189: [STORM-2615] Add topology readonly user configuration
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2189
I don't know why the 'INTERATION-TEST' failed. I tested it in my computer using "mvn -P integration-tests-only verify" command and the build succeeded.
---
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 #2189: [STORM-2615] Add topology readonly user configurat...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on a diff in the pull request:
https://github.com/apache/storm/pull/2189#discussion_r126295391
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -157,6 +162,25 @@ public boolean permit(ReqContext context, String operation, Map<String, Object>
}
if (checkUserGroupAllowed(userGroups, topoGroups)) return true;
+
+ if (_topoReadOnlyCommands.contains(operation)){
+ Set topoReadOnlyUsers = new HashSet<String>();
--- End diff --
nit: -> `Set<String> topoReadOnlyUsers = new HashSet<>();`
---
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.
---