You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2017/10/27 17:38:42 UTC
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/2390
[STORM-2790] Add nimbus admins groups
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm add-admin-groups
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2390.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 #2390
----
commit e7d9881c1876494b179e06c7c3ee64c606590343
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2017-10-26T21:59:35Z
Add nimbus admins groups
----
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147777772
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/FixedGroupsMapping.java ---
@@ -0,0 +1,69 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.storm.Config;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class FixedGroupsMapping implements IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(FixedGroupsMapping.class);
+ public static final String STORM_FIXED_GROUP_MAPPING = "storm.fixed.group.mapping";
+ public Map<String, Set<String>> cachedGroups = new HashMap<String, Set<String>>();
+
+ /**
+ * Invoked once immediately after construction
+ *
+ * @param storm_conf Storm configuration
+ */
+ @Override
+ public void prepare(Map storm_conf) {
+ Map<?, ?> params = (Map<?, ?>) storm_conf.get(Config.STORM_GROUP_MAPPING_SERVICE_PARAMS);
+ Map<String, Set<String>> mapping = (Map<String, Set<String>>) params.get(STORM_FIXED_GROUP_MAPPING);
--- End diff --
Should we use `stormConf` instead of `storm_conf`?
Do we want to use Map<String, Object> instead of Map<?, ?>?
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147744795
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -159,14 +159,6 @@
public static final String SCHEDULER_DISPLAY_RESOURCE = "scheduler.display.resource";
/**
- * Initialization parameters for the group mapping service plugin.
- * Provides a way for a @link{STORM_GROUP_MAPPING_SERVICE_PROVIDER_PLUGIN}
- * implementation to access optional settings.
- */
- @isType(type = Map.class)
- public static final String STORM_GROUP_MAPPING_SERVICE_PARAMS = "storm.group.mapping.service.params";
--- End diff --
Was this just not used anywhere?
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147744419
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/authorizer/SimpleACLAuthorizer.java ---
@@ -80,6 +80,7 @@
}
protected Set<String> _admins;
+ protected Set<String> _adminsGroups;
--- End diff --
Same here we should not use '_' as a prefix.
---
[GitHub] storm issue #2390: [STORM-2790] Add nimbus admins groups
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on the issue:
https://github.com/apache/storm/pull/2390
Looks like there is some checkstyle violatiom issues here.
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147774596
--- Diff: storm-server/src/main/java/org/apache/storm/DaemonConfig.java ---
@@ -159,14 +159,6 @@
public static final String SCHEDULER_DISPLAY_RESOURCE = "scheduler.display.resource";
/**
- * Initialization parameters for the group mapping service plugin.
- * Provides a way for a @link{STORM_GROUP_MAPPING_SERVICE_PROVIDER_PLUGIN}
- * implementation to access optional settings.
- */
- @isType(type = Map.class)
- public static final String STORM_GROUP_MAPPING_SERVICE_PARAMS = "storm.group.mapping.service.params";
--- End diff --
It was not used anywhere until this PR - where `FixedGroupsMapping` uses it.
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by Ethanlm <gi...@git.apache.org>.
Github user Ethanlm commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147775339
--- Diff: external/storm-hdfs/src/test/java/org/apache/storm/hdfs/blobstore/BlobStoreTest.java ---
@@ -92,6 +93,19 @@ public static void cleanupAfterClass() throws IOException {
// Method which initializes nimbus admin
public static void initializeConfigs() {
conf.put(Config.NIMBUS_ADMINS,"admin");
+ conf.put(Config.NIMBUS_ADMINS_GROUPS,"adminsGroup");
+
--- End diff --
nit: space after comma?
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2390
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147744518
--- Diff: storm-client/src/jvm/org/apache/storm/security/auth/FixedGroupsMapping.java ---
@@ -0,0 +1,68 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.storm.security.auth;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.HashSet;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.storm.Config;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+
+public class FixedGroupsMapping implements IGroupMappingServiceProvider {
+
+ public static Logger LOG = LoggerFactory.getLogger(FixedGroupsMapping.class);
--- End diff --
The new guidelines have us use 4 spaces not 2 for indentation.
---
[GitHub] storm pull request #2390: [STORM-2790] Add nimbus admins groups
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2390#discussion_r147744169
--- Diff: storm-client/src/jvm/org/apache/storm/blobstore/BlobStoreAclHandler.java ---
@@ -45,6 +47,7 @@
public class BlobStoreAclHandler {
public static final Logger LOG = LoggerFactory.getLogger(BlobStoreAclHandler.class);
private final IPrincipalToLocal _ptol;
+ private final IGroupMappingServiceProvider _groupMappingProvider;
--- End diff --
We should move away from using '_' as a prefix to member variables. Please at least fix the new ones to not use it, and if you want to fix all of them that would be great too.
---