[GitHub] cloudstack pull request: 4.9 mvn version safeupgradeonly
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1510#issuecomment-213521104 @swill please include this PR in your list, 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1502#issuecomment-213520037 Thanks @swill @kiwiflyer --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60772858
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,138 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.warn("Unable to insert mapping for role id:" + roleId
+ " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
--- End diff --
Thanks, will fix this.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60772677
--- Diff: utils/src/main/java/com/cloud/utils/PropertiesUtil.java ---
@@ -34,6 +34,10 @@
public class PropertiesUtil {
private static final Logger s_logger =
Logger.getLogger(PropertiesUtil.class);
+public static String getDefaultApiCommandsFileName() {
+return "commands.properties";
+}
--- End diff --
PropertiesUtil deals with commands.properties file related processing, so
I've put it here. Also, utils is a common packages used by several other
modules, putting this in an authenticator (server or plugin package) won't make
it generally consumable across several modules if needed.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60772392
--- Diff: server/src/com/cloud/api/dispatch/ParamProcessWorker.java ---
@@ -92,6 +94,55 @@ public void handle(final DispatchTask task) {
processParameters(task.getCmd(), task.getParams());
}
+private void validateNonEmptyString(final Object param, final String
argName) {
+if (param == null || Strings.isNullOrEmpty(param.toString())) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Empty or null value provided for API arg: %s", argName));
+}
+}
+
+private void validateNaturalNumber(final Object param, final String
argName) {
+Long value = null;
+if (param != null && param instanceof Long) {
+value = (Long) param;
+} else if (param != null) {
+value = Long.valueOf(param.toString());
+}
+if (value == null || value < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
String.format("Invalid value provided for API arg: %s", argName));
+}
+}
+
+private void validateField(final Object paramObj, final Parameter
annotation) throws ServerApiException {
+if (annotation == null) {
+return;
+}
+final String argName = annotation.name();
+for (final ApiArgValidator validator : annotation.validations()) {
+if (validator == null) {
+continue;
+}
+switch (validator) {
+case NotNullOrEmpty:
+switch (annotation.type()) {
+case UUID:
+case STRING:
+validateNonEmptyString(paramObj, argName);
+break;
+}
+break;
+case PositiveNumber:
+switch (annotation.type()) {
+case SHORT:
+case INTEGER:
+case LONG:
+validateNaturalNumber(paramObj, argName);
+break;
+}
--- End diff --
annotation fields need an exact value, so it could either have a list of
enum or classes; using a list of classes would make implementation complex and
creation of several classes for each new validator. We also needed to know what
the annotation type is to only execute operation based on type, for example
PositiveNumber validator on a string or boolean field/arg type won't make
sense. It was easiest in terms of maintainability and implementation to pass in
enum, and handle them here like this. I'm not sure the best way of doing this,
advise if there are better ways of doing validation?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60771928
--- Diff: scripts/util/migrate-dynamicroles.py ---
@@ -0,0 +1,134 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# 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
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# 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.
+
+import os
+import sys
+import uuid
+
+from contextlib import closing
+from optparse import OptionParser
+
+try:
+import MySQLdb
+except ImportError:
+print("MySQLdb cannot be imported, please install python-mysqldb(apt)
or mysql-python(yum)")
+sys.exit(1)
+
+dryrun = False
+
+
+def runSql(conn, query):
+if dryrun:
+print("Running SQL query: " + query)
+return
+with closing(conn.cursor()) as cursor:
+cursor.execute(query)
+
+
+def migrateApiRolePermissions(apis, conn):
+# All allow for root admin role Admin(id:1)
+runSql(conn, "INSERT INTO `cloud`.`role_permissions` (`uuid`,
`role_id`, `rule`, `permission`) values (UUID(), 1, '*', 'Allow');")
+# Migrate rules based on commands.properties rule for
ResourceAdmin(id:2), DomainAdmin(id:3), User(id:4)
+octetKey = {2:2, 3:4, 4:8}
+for role in [2, 3, 4]:
+for api in sorted(apis.keys()):
+# Ignore auth commands
+if api in ['login', 'logout', 'samlSso', 'samlSlo',
'listIdps', 'listAndSwitchSamlAccount', 'getSPMetadata']:
+continue
+if (octetKey[role] & int(apis[api])) > 0:
+runSql(conn, "INSERT INTO `cloud`.`role_permissions`
(`uuid`, `role_id`, `rule`, `permission`) values (UUID(), %d, '%s', 'Allow');"
% (role, api))
+
+
+def main():
+parser = OptionParser()
+parser.add_option("-b", "--db", action="store", type="string",
dest="db", default="cloud",
+help="The name of the database, default: cloud")
+parser.add_option("-u", "--user", action="store", type="string",
dest="user", default="cloud",
+help="User name a MySQL user with privileges on
cloud database")
+parser.add_option("-p", "--password", action="store", type="string",
dest="password", default="cloud",
+help="Password of a MySQL user with privileges on
cloud database")
+parser.add_option("-H", "--host", action="store", type="string",
dest="host", default="127.0.0.1",
+help="Host or IP of the MySQL server")
+parser.add_option("-P", "--port", action="store", type="int",
dest="port", default=3306,
+help="Host or IP of the MySQL server")
+parser.add_option("-f", "--properties-file", action="store",
type="string", dest="commandsfile",
default="/etc/cloudstack/management/commands.properties",
+help="The commands.properties file")
+parser.add_option("-d", "--dryrun", action="store_true",
dest="dryrun", default=False,
+help="Dry run and debug operations this tool will
perform")
+(options, args) = parser.parse_args()
+
+print("Apache CloudStack Role Permission Migration Tool")
+print("(c) Apache CloudStack Authors and the ASF, under the Apache
License, Version 2.0\n")
+
+global dryrun
+if options.dryrun:
+dryrun = True
+
+conn = MySQLdb.connect(
+host=options.host,
+user=options.user,
+
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60771659
--- Diff: scripts/util/migrate-dynamicroles.py ---
@@ -0,0 +1,134 @@
+#!/usr/bin/python
+# -*- coding: utf-8 -*-
+# 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
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# 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.
+
+import os
+import sys
+import uuid
+
+from contextlib import closing
+from optparse import OptionParser
+
+try:
+import MySQLdb
+except ImportError:
+print("MySQLdb cannot be imported, please install python-mysqldb(apt)
or mysql-python(yum)")
+sys.exit(1)
+
+dryrun = False
+
+
+def runSql(conn, query):
+if dryrun:
+print("Running SQL query: " + query)
+return
+with closing(conn.cursor()) as cursor:
+cursor.execute(query)
+
+
+def migrateApiRolePermissions(apis, conn):
+# All allow for root admin role Admin(id:1)
+runSql(conn, "INSERT INTO `cloud`.`role_permissions` (`uuid`,
`role_id`, `rule`, `permission`) values (UUID(), 1, '*', 'Allow');")
+# Migrate rules based on commands.properties rule for
ResourceAdmin(id:2), DomainAdmin(id:3), User(id:4)
+octetKey = {2:2, 3:4, 4:8}
+for role in [2, 3, 4]:
+for api in sorted(apis.keys()):
+# Ignore auth commands
+if api in ['login', 'logout', 'samlSso', 'samlSlo',
'listIdps', 'listAndSwitchSamlAccount', 'getSPMetadata']:
+continue
+if (octetKey[role] & int(apis[api])) > 0:
+runSql(conn, "INSERT INTO `cloud`.`role_permissions`
(`uuid`, `role_id`, `rule`, `permission`) values (UUID(), %d, '%s', 'Allow');"
% (role, api))
+
+
+def main():
+parser = OptionParser()
+parser.add_option("-b", "--db", action="store", type="string",
dest="db", default="cloud",
+help="The name of the database, default: cloud")
+parser.add_option("-u", "--user", action="store", type="string",
dest="user", default="cloud",
+help="User name a MySQL user with privileges on
cloud database")
+parser.add_option("-p", "--password", action="store", type="string",
dest="password", default="cloud",
+help="Password of a MySQL user with privileges on
cloud database")
+parser.add_option("-H", "--host", action="store", type="string",
dest="host", default="127.0.0.1",
+help="Host or IP of the MySQL server")
+parser.add_option("-P", "--port", action="store", type="int",
dest="port", default=3306,
+help="Host or IP of the MySQL server")
+parser.add_option("-f", "--properties-file", action="store",
type="string", dest="commandsfile",
default="/etc/cloudstack/management/commands.properties",
+help="The commands.properties file")
+parser.add_option("-d", "--dryrun", action="store_true",
dest="dryrun", default=False,
+help="Dry run and debug operations this tool will
perform")
+(options, args) = parser.parse_args()
+
+print("Apache CloudStack Role Permission Migration Tool")
+print("(c) Apache CloudStack Authors and the ASF, under the Apache
License, Version 2.0\n")
+
+global dryrun
+if options.dryrun:
+dryrun = True
+
+conn = MySQLdb.connect(
+host=options.host,
+user=options.user,
+
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60771393
--- Diff:
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapImportUsersCmd.java
---
@@ -131,7 +135,9 @@ private void createCloudstackUserAccount(LdapUser user,
String accountName, Doma
@Override
public void execute() throws ResourceUnavailableException,
InsufficientCapacityException, ServerApiException, ConcurrentOperationException,
ResourceAllocationException, NetworkRuleConflictException {
-
+if (getAccountType() == null && getRoleId() == null) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Both
account type and role ID are not provided");
--- End diff --
same reasons from above
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60771374
--- Diff:
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccountCmd.java
---
@@ -119,6 +131,9 @@ private Long getDomainId() {
@Override
public void execute() throws ServerApiException {
+if (getAccountType() == null && getRoleId() == null) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Both
account type and role ID are not provided");
--- End diff --
@jburwell because of backward compatibility we cannot make roleid param as
required, while accounttype used to be required but is now made non-required;
this checks ensures that at least one of them is passed. When only account type
is passed, we translate that to one of the four default roles (id).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: 4.9 mvn version safeupgradeonly
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1510#issuecomment-213398409 With this change we would be able to use JDK8 for building cloudstack once we fix the build issues of F5 plugin (that's the only component that fails to build with javac 1.8 now). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: 4.9 bountycastle daan
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/1511 4.9 bountycastle daan This PR adds bountycastle related version and code fixes from @DaanHoogland on top of https://github.com/apache/cloudstack/pull/1510 I could not fix java compilation issues when using version 1.54 with openjdk 1.7.0_99, @DaanHoogland can you take over? The branch in on asf remote so you can push/pull as necessary. Thanks. cc @swill You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/cloudstack 4.9-bountycastle-daan Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1511.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 #1511 commit bb29b1d06389e41c779d94f2d433f3b7c48009b3 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-01-27T00:41:02Z maven: Upgrade dependency versions Updated most dependencies to latest minor releases, EXCEPT: - Gson 2.x - Major spring framework version - Servlet version - Embedded jetty version - Mockito version (beta) - Mysql lib minor version upgrade (breaks mysql-ha plugin) Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 770aa0133ee3011239033e2dfe3f6ed41b76761a Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-02-02T18:55:06Z framework/quota: fix checkstyle issue Fixes enum name to supress checkstyle failure due to the latest checkstyle version Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 101668994dd27e3ca67c09a8118dbcea255805fd Author: Daan Hoogland <d...@onecht.net> Date: 2016-04-22T10:55:27Z further maven dependency updates from Daan Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 8af677a0f0b2a74db74223fc3d0e2d3e9549e960 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-01-28T12:06:08Z maven: fix dependency version support by JDK7 - Fix jetty dependency that is compatible with Java7 - Upgrade minor revisions of dependencies - Upgrade vmware mvn sdk dependency to 6.0 - Downgrade bounty castle version to 1.46 (same as before) Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 2ac0837765f1bbf407706fef5002a6fbfa0dba8b Author: Daan Hoogland <d...@onecht.net> Date: 2016-04-22T10:58:33Z maven: upgrade bounty castle version to 1.54 Upgrades bountycastle dependency version to bcprov-15on v1.54 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: maven: Upgrade dependency versions
Github user bhaisaab closed the pull request at: https://github.com/apache/cloudstack/pull/1397 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: maven: Upgrade dependency versions
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1397#issuecomment-213397292 @swill closing this PR, opened two new PRs that split changes from this PR --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: 4.9 mvn version safeupgradeonly
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/1510 4.9 mvn version safeupgradeonly Upgrades maven dependencies versions that can be safely upgraded without breaking console-proxy/crypto usage. Bisected changes from: https://github.com/apache/cloudstack/pull/1397 cc @swill @DaanHoogland You can merge this pull request into a Git repository by running: $ git pull https://github.com/shapeblue/cloudstack 4.9-mvn-version-safeupgradeonly Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1510.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 #1510 commit bb29b1d06389e41c779d94f2d433f3b7c48009b3 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-01-27T00:41:02Z maven: Upgrade dependency versions Updated most dependencies to latest minor releases, EXCEPT: - Gson 2.x - Major spring framework version - Servlet version - Embedded jetty version - Mockito version (beta) - Mysql lib minor version upgrade (breaks mysql-ha plugin) Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 770aa0133ee3011239033e2dfe3f6ed41b76761a Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-02-02T18:55:06Z framework/quota: fix checkstyle issue Fixes enum name to supress checkstyle failure due to the latest checkstyle version Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 101668994dd27e3ca67c09a8118dbcea255805fd Author: Daan Hoogland <d...@onecht.net> Date: 2016-04-22T10:55:27Z further maven dependency updates from Daan Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> commit 8af677a0f0b2a74db74223fc3d0e2d3e9549e960 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-01-28T12:06:08Z maven: fix dependency version support by JDK7 - Fix jetty dependency that is compatible with Java7 - Upgrade minor revisions of dependencies - Upgrade vmware mvn sdk dependency to 6.0 - Downgrade bounty castle version to 1.46 (same as before) Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1502#issuecomment-213395335 @DaanHoogland thanks, @borisstoyanov will share QA results next week as well --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1502#discussion_r60727764
--- Diff: api/src/org/apache/cloudstack/api/response/ZoneResponse.java ---
@@ -233,6 +234,9 @@ public void addTag(ResourceTagResponse tag) {
}
public void setResourceDetails(Map<String, String> details) {
-this.resourceDetails = details;
+if (details == null) {
--- End diff --
yes, if its null no point in setting it as nothing will be marshaled by
gson/xml processor, also we don't want to consume the map but create a new copy
out of it to avoid polluting user's datastructures
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1502#issuecomment-213375165 Pinging for review @jburwell @swill @wido @pyr @koushik-das @agneya2001 @DaanHoogland @rafaelweingartner @GabrielBrascher @kishankavala --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1502#discussion_r60716885 --- Diff: client/tomcatconf/commands.properties.in --- @@ -799,3 +799,14 @@ quotaCredits=1 quotaEmailTemplateList=1 quotaEmailTemplateUpdate=1 quotaIsEnabled=15 + +### Out-of-band Management --- End diff -- If rbac gets merged, we won't need changes in this file as well --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9299: Out-of-band Management f...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1502#discussion_r60716681
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/outofbandmanagement/ChangeOutOfBandManagementPasswordCmd.java
---
@@ -0,0 +1,116 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.api.command.admin.outofbandmanagement;
+
+import com.cloud.event.EventTypes;
+import com.cloud.exception.ConcurrentOperationException;
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.NetworkRuleConflictException;
+import com.cloud.exception.ResourceAllocationException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.host.Host;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseAsyncCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.HostResponse;
+import org.apache.cloudstack.api.response.OutOfBandManagementResponse;
+import org.apache.cloudstack.context.CallContext;
+import
org.apache.cloudstack.outofbandmanagement.OutOfBandManagementService;
+
+import javax.inject.Inject;
+
+@APICommand(name = "changeOutOfBandManagementPassword", description =
"Changes out-of-band management interface password on the host and updates the
interface configuration in CloudStack if the operation succeeds, else reverts
the old password",
+responseObject = OutOfBandManagementResponse.class,
requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, authorized =
{RoleType.Admin})
+public class ChangeOutOfBandManagementPasswordCmd extends BaseAsyncCmd {
+@Inject
+private OutOfBandManagementService outOfBandManagementService;
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.HOST_ID, type = CommandType.UUID,
entityType = HostResponse.class, required = true, description = "the ID of the
host")
+private Long hostId;
+
+@Parameter(name = ApiConstants.PASSWORD, type = CommandType.STRING,
description = "the new host management interface password of maximum length 16,
if none is provided a random password would be used")
+private String password;
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return "changeoutofbandmanagementpasswordresponse";
+}
+
+private void validateParams() {
+if (getHostId() == null || getHostId() < 1L) {
--- End diff --
I'll remove validateParams() as soon as rbac gets merged to master, I'll
rebase this PR and use the new validations annotations field.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8818: Use MySQL native connect...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1054#issuecomment-213331800 @wido we'll need to fix codebase wide mysqldb usage in python scripts and build both rpms/deb to show installation, setting up of db and setting up mgmt server works out of the box. Other than this, I'll also need to fix a python based migration script used in dynamic roles PR that also uses the current mysqldb. We'll also need to fix travis to install the new dependency and fix marvin's dbclient to use it instead of mysqldb. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1365#issuecomment-213329597 @GabrielBrascher @swill @jburwell @rafaelweingartner please share your LGTM or comment what needs fixing, 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-213328879 @jburwell @swill @DaanHoogland @pdube @koushik-das @pdube please share your LGTM if you're satisfied with the changes, or please comment what else needs fixing. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-213327217 @jburwell @GabrielBrascher @rafaelweingartner @swill if you're done with review, LGTM please or share what else should be fixed. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60585031
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,170 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+protected static final Logger LOGGER =
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null || permissions.isEmpty() ||
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+final Rule rule = new Rule(permission.getRule());
+if (rule.matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" +
permission.getId() + " rule=" + permission.getRule());
+continue;
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1302#discussion_r60565861
--- Diff:
plugins/hypervisors/vmware/src/com/cloud/hypervisor/vmware/resource/VmwareResource.java
---
@@ -2030,12 +2030,29 @@ int getReservedCpuMHZ(VirtualMachineTO vmSpec) {
return new String[] {datastoreDiskPath};
}
-// Pair
-private Pair<String, String> composeVmNames(VirtualMachineTO vmSpec) {
-String vmInternalCSName = vmSpec.getName();
-String vmNameOnVcenter = vmSpec.getName();
-if (_instanceNameFlag && vmSpec.getHostName() != null) {
-vmNameOnVcenter = vmSpec.getHostName();
+
+/**
+ * This method generates VM name for Vcenter and Cloudstack( when
Hypervisor is VMware).
+ * It generates VM name according to _instanceNameFlag setting.
+ *
+ * @param VirtualMachineTO
+ *vmSpec
+ * @return Pair. A pair which
contain 'internal CS name' and
+ * 'vCenter display name'(vCenter knows VM by this name).
+ **/
+private Pair<String, String> composeVmNatmes(VirtualMachineTO vmSpec) {
+
+String vmInternalCSName = null;
+String vmNameOnVcenter = null;
+if(vmSpec != null)
+{
+vmInternalCSName = vmNameOnVcenter = vmSpec.getName();
+if (_instanceNameFlag == true &&
vmSpec.getType()==VirtualMachine.Type.User) {
+String[] tokens = vmInternalCSName.split("-");
+assert (tokens.length >= 3); // vmInternalCSName has
format i-x-y-
+vmNameOnVcenter = String.format("%s-%s-%s-%s", tokens[0],
tokens[1], tokens[2], vmSpec.getHostName());
--- End diff --
what if one of the tokens are null/empty? why not simply add the hostname
(if only it's not null/empty) at the end of vmInternalCSName; simply do a
`vmInternalCSName + "-" + vmSpace.getHostName()` (iff
!Strings.isNullOrEmpty(getHostName()))
Using the same internal CS name on vmware should be alright and ideal,
solve the issue of inconsistent and unique naming.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9104: VM naming convention in ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1302#issuecomment-212873785 @priyankparihar we'll need a unit test that shows that this change is backward compatible. I like that we're considering both default instance name with host name here (i.e. the names should be globally unique) but we'll need few more units tests to make sure the contract is not broken again in future. We've changed how internal/external vmware vm names are composed 3-4 times since 4.0, can you explain why we want to do this again. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-212852849 @jburwell I've fixed outstanding issues, including a framework level API arg validation (and removed such validations across cmd implementations) @koushik-das let me know if you've more questions Thanks all for the review, I believe all of the outstanding issues have been solved and fixed. /cc @swill I've executed another test round locally and it looks good to me Marvin Init Started === Marvin Parse Config Successful === === Marvin Setting TestData Successful=== Log Folder Path: /tmp//MarvinLogs//Apr_21_2016_16_16_18_FM3SK5. All logs will be available here === Marvin Init Logging Successful=== Marvin Init Successful === TestName: test_default_role_deletion | Status : SUCCESS === === TestName: test_role_account_acls | Status : SUCCESS === === TestName: test_role_account_acls_multiple_mgmt_servers | Status : SUCCESS === === TestName: test_role_inuse_deletion | Status : SUCCESS === === TestName: test_role_lifecycle_create | Status : SUCCESS === === TestName: test_role_lifecycle_delete | Status : SUCCESS === === TestName: test_role_lifecycle_list | Status : SUCCESS === === TestName: test_role_lifecycle_update | Status : SUCCESS === === TestName: test_role_lifecycle_update_role_inuse | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_create | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_delete | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_list | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_update | Status : SUCCESS === === TestName: test_rolepermission_lifecycle_update_invalid_rule | Status : SUCCESS === --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: fake don't merge
Github user bhaisaab closed the pull request at: https://github.com/apache/cloudstack/pull/1507 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: fake don't merge
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/1507 fake don't merge You can merge this pull request into a Git repository by running: $ git pull https://github.com/shapeblue/cloudstack fake-marvin-faliing-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1507.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 #1507 commit 6c11bab955e7f16c3ed90bc9c8c4ff4527ed93df Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-04-21T07:11:07Z fake don't merge Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: fake don't merge
Github user bhaisaab closed the pull request at: https://github.com/apache/cloudstack/pull/1506 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: Fixing an issue in Marvin around creating...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1501#issuecomment-212797997 LGTM, cc @swill --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: fake don't merge
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/1506 fake don't merge Travis is failing marvin tests repeatedly, this is a fake test to verify if that's happening due to one of my PRs of due to recent PR merged on master. You can merge this pull request into a Git repository by running: $ git pull https://github.com/shapeblue/cloudstack fake-marvin-faliing-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1506.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 #1506 commit 6c11bab955e7f16c3ed90bc9c8c4ff4527ed93df Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-04-21T07:11:07Z fake don't merge Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60440106
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,170 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+protected static final Logger LOGGER =
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null || permissions.isEmpty() ||
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+final Rule rule = new Rule(permission.getRule());
+if (rule.matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" +
permission.getId() + " rule=" + permission.getRule());
+continue;
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60439905
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,170 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+protected static final Logger LOGGER =
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null || permissions.isEmpty() ||
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+final Rule rule = new Rule(permission.getRule());
+if (rule.matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" +
permission.getId() + " rule=" + permission.getRule());
+continue;
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60439732
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,170 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import org.apache.log4j.Logger;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+protected static final Logger LOGGER =
Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class);
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null || permissions.isEmpty() ||
Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+try {
+final Rule rule = new Rule(permission.getRule());
+if (rule.matches(commandName)) {
+return true;
+}
+} catch (InvalidParameterValueException e) {
+LOGGER.warn("Invalid rule permission, please fix id=" +
permission.getId() + " rule=" + permission.getRule());
+continue;
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60439393
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+final String rule = permission.getRule();
+if (rule.contains("*")) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equals(rule)) {
+return true;
+}
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
+if (accountRole.getRoleType() == RoleType.Admin &&
accountRole.getId() == RoleType.Admin.getId()) {
+return true;
+}
+
+final List rolePermissions =
roleService.findAllPermissionsBy(accountRole.getId());
+
+// Check for allow rules
+if (checkPermission(rolePer
[GitHub] cloudstack pull request: CLOUDSTACK-8901: PrepareTemplate job thre...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/880#issuecomment-212474909 Thanks @SudharmaJain LGTM (just code review), can you do a push -f (travis job failed for some reason) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8970 Centos 6.{1,2,3,4,5} gues...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/956#issuecomment-212474106 @SudharmaJain The provided version have already released so these change need to go into 481to490 related sql /cc @swill any vmware/guest-os-mapping guru want to comment on the changes -- @anshulgangwar @devdeep @agneya2001 @koushik-das @sureshanaparti --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8906: /var/log/cloud/ doesn't ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/883#issuecomment-212471834 @SudharmaJain Will @swill is the RM for master/4.9, I'm not following these changes perhaps any Xen guru can comment, @agneya2001 ? @remibergsma do you have any outstanding issues with this PR --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60390739
--- Diff: engine/schema/src/org/apache/cloudstack/acl/RolePermissionVO.java
---
@@ -0,0 +1,109 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import java.util.UUID;
+
+@Entity
+@Table(name = "role_permissions")
+public class RolePermissionVO implements RolePermission {
--- End diff --
if you re-read my reply and see the static-checker and dynamic checker code
-- backward compatibility is to ensure that we deny for all when no rule
matches; for backward compatibility the dynamic checker also needs to check the
annotation map. The dynamic checker allows for wildcard rules, and if you want
to override annotations we'll need a set of deny rules before that. Therefore
we need deny rules explicitly. This way of implementation makes dynamic-checker
a drop-in replacement at the same time allows for wider use-cases and acl
management.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60388741
--- Diff: api/src/org/apache/cloudstack/acl/Rule.java ---
@@ -0,0 +1,65 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import com.google.common.base.Strings;
+
+import java.util.regex.Pattern;
+
+public final class Rule {
+private final String rule;
+private final static Pattern ALLOWED_PATTERN =
Pattern.compile("^[a-zA-Z0-9*]+$");
+
+public Rule(final String rule) {
+validate(rule);
+this.rule = rule;
+}
+
+public boolean matches(final String commandName) {
+if (Strings.isNullOrEmpty(commandName)) {
+return false;
+}
+if (isWildcard()) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equalsIgnoreCase(rule)) {
+return true;
+}
+}
+return false;
+}
+
+public boolean isWildcard() {
+return rule.contains("*");
+}
+
+@Override
+public String toString() {
+return rule;
+}
+
+private static boolean validate(final String rule) throws
InvalidParameterValueException {
+if (Strings.isNullOrEmpty(rule) ||
!ALLOWED_PATTERN.matcher(rule).matches()) {
+throw new InvalidParameterValueException("Invalid rule
provided. Only API names and wildcards are allowed.");
+}
+return true;
--- End diff --
@jburwell I've refactored and move all rule related methods in this class
and added more unit tests. The rule itself is immutable only compo-sable by the
constructor.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60386634
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+final String rule = permission.getRule();
+if (rule.contains("*")) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equals(rule)) {
+return true;
+}
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
+if (accountRole.getRoleType() == RoleType.Admin &&
accountRole.getId() == RoleType.Admin.getId()) {
+return true;
+}
+
+final List rolePermissions =
roleService.findAllPermissionsBy(accountRole.getId());
+
+// Check for allow rules
+if (checkPermission(rolePermi
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60384791
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+final String rule = permission.getRule();
+if (rule.contains("*")) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equals(rule)) {
--- End diff --
@koushik-das I spent some time thinking and I think it should be okay to do
a ignore-case equality check. Fixed, it does equalsIgnoreCase now.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Don't start review yet -- CLOUDSTAC...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1502#issuecomment-212340052 UI Screenshots;       --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [WIP] Don't start review yet -- CLOUDSTAC...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1502#issuecomment-212338195 Local test results (the feature is hypervisor agnostic, integration test also run with Travis as well): Marvin Init Successful === TestName: test_01_configure_oobm_invalid | Status : SUCCESS === === TestName: test_02_configure_oobm_valid | Status : SUCCESS === === TestName: test_03_enabledisable_oobm_invalid | Status : SUCCESS === === TestName: test_04_enabledisable_oobm_valid | Status : SUCCESS === === TestName: test_05_enabledisable_across_clusterzones_oobm_valid | Status : SUCCESS === === TestName: test_06_oobm_issue_power_action | Status : SUCCESS === === TestName: test_07_oobm_background_powerstate_sync | Status : SUCCESS === === TestName: test_08_multiple_mgmt_server_ownership | Status : SUCCESS === === TestName: test_09_oobm_change_password | Status : SUCCESS === --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60369930
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
--- End diff --
This is just a helper method, doing in-memory comparison/looping is faster
than making two DB requests and provided there won't be more than 500 items
returned so I just used a single method in the checkAccess() to get all rules
and check for allow first, then deny, then annotations and finally deny/fail.
Suggest if we still want two db calls?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60369577
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+final String rule = permission.getRule();
+if (rule.contains("*")) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equals(rule)) {
--- End diff --
@koushik-das just like commands.properties declarations the API name much
match provided rule. Though, I see your point here -- let me think if this can
have any side-effect or some use-case where this can fail; otherwise I'll fix
this.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60369373
--- Diff:
plugins/acl/dynamic-role-based/src/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java
---
@@ -0,0 +1,166 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.exception.PermissionDeniedException;
+import com.cloud.user.Account;
+import com.cloud.user.AccountService;
+import com.cloud.user.User;
+import com.cloud.utils.component.AdapterBase;
+import com.cloud.utils.component.PluggableService;
+import com.google.common.base.Strings;
+import org.apache.cloudstack.api.APICommand;
+
+import javax.ejb.Local;
+import javax.inject.Inject;
+import javax.naming.ConfigurationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+@Local(value = APIChecker.class)
+public class DynamicRoleBasedAPIAccessChecker extends AdapterBase
implements APIChecker {
+
+@Inject
+private AccountService accountService;
+@Inject
+private RoleService roleService;
+
+private List services;
+private Map<RoleType, Set> annotationRoleBasedApisMap = new
HashMap<>();
+
+protected DynamicRoleBasedAPIAccessChecker() {
+super();
+for (RoleType roleType : RoleType.values()) {
+annotationRoleBasedApisMap.put(roleType, new
HashSet());
+}
+}
+
+private void denyApiAccess(final String commandName) throws
PermissionDeniedException {
+throw new PermissionDeniedException("The API does not exist or is
blacklisted for the account's role. " +
+"The account with is not allowed to request the api: " +
commandName);
+}
+
+private boolean checkPermission(final List
permissions, final RolePermission.Permission permissionToCheck, final String
commandName) {
+if (permissions == null) {
+return false;
+}
+for (final RolePermission permission : permissions) {
+if (permission.getPermission() != permissionToCheck) {
+continue;
+}
+final String rule = permission.getRule();
+if (rule.contains("*")) {
+if (commandName.matches(rule.replace("*", "\\w*"))) {
+return true;
+}
+} else {
+if (commandName.equals(rule)) {
+return true;
+}
+}
+}
+return false;
+}
+
+public boolean isDisabled() {
+return !roleService.isEnabled();
+}
+
+@Override
+public boolean checkAccess(User user, String commandName) throws
PermissionDeniedException {
+if (isDisabled()) {
+return true;
+}
+Account account = accountService.getAccount(user.getAccountId());
+if (account == null) {
+throw new PermissionDeniedException("The account id=" +
user.getAccountId() + "for user id=" + user.getId() + "is null");
+}
+
+final Role accountRole = roleService.findRole(account.getRoleId());
+if (accountRole == null || accountRole.getId() < 1L) {
+denyApiAccess(commandName);
+}
+
+// Allow all APIs for root admins
+if (accountRole.getRoleType() == RoleType.Admin &&
accountRole.getId() == RoleType.Admin.getId()) {
+return true;
+}
+
+final List rolePermissions =
roleService.findAllPermissionsBy(accountRole.getId());
+
+// Check for allow rules
+if (checkPermissi
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60368615
--- Diff: engine/schema/src/org/apache/cloudstack/acl/RolePermissionVO.java
---
@@ -0,0 +1,109 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import java.util.UUID;
+
+@Entity
+@Table(name = "role_permissions")
+public class RolePermissionVO implements RolePermission {
--- End diff --
@koushik-das okay I got what you're saying, but it's not possible. Also see
we've to be backward compatible with the static checker, so (1) the order of
processing of rules need to be similar to the static-checker which is first
allow rules then deny checks, then finally annotation and lastly deny all, (2)
rules can be both apiname or wildcards such as list\* therefore we cannot have
the references reversed or what you're suggesting, (3) the model promotes
explicit rules than implicit declarations for security and various use-case (so
we don't assume anything), (4) if we want an API like deployVM to be available
for all or some role types we can use the authorized field in the API
annotation (for example using authorized={RoleType.Admin, RoleType.User ...
etc} will enable this API for all when no explicit rules are set by the admin).
I've thought this through but this is the best model we can have trading
feature use, functionality and security. Provided this, do you have suggestion
if we can improve this.?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [WIP] Don't start review yet -- CLOUDSTAC...
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack/pull/1502 [WIP] Don't start review yet -- CLOUDSTACK-9299: Out-of-band Management for CloudStack Support access to a hostâs out-of-band management interface (e.g. IPMI, iLO, DRAC, etc.) to manage host power operations (on/off etc.) and querying current power state in CloudStack. Given the wide range of out-of-band management interfaces such as iLO and iDRA, the service implementation allows for development of separate drivers as plugins. This feature comes with a ipmitool based driver that uses the ipmitool (http://linux.die.net/man/1/ipmitool) to communicate with any out-of-band management interface that support IPMI 2.0. This feature allows following common use-cases: - Restarting stalled/failed hosts - Powering off under-utilised hosts - Powering on hosts for provisioning or to increase capacity - Allowing system administrators to see the current power state of the host For testing this feature `ipmisim` can be used: https://pypi.python.org/pypi/ipmisim FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Out-of-band+Management+for+CloudStack You can merge this pull request into a Git repository by running: $ git pull https://github.com/shapeblue/cloudstack outofband-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack/pull/1502.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 #1502 commit 84157cebaa6bf3986efedb2887999d58653e8dd0 Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2015-12-28T11:07:03Z CLOUDSTACK-9299: Out-of-band Management for CloudStack Support access to a hostâs out-of-band management interface (e.g. IPMI, iLO, DRAC, etc.) to manage host power operations (on/off etc.) and querying current power state in CloudStack. Given the wide range of out-of-band management interfaces such as iLO and iDRA, the service implementation allows for development of separate drivers as plugins. This feature comes with a ipmitool based driver that uses the ipmitool (http://linux.die.net/man/1/ipmitool) to communicate with any out-of-band management interface that support IPMI 2.0. This feature allows following common use-cases: - Restarting stalled/failed hosts - Powering off under-utilised hosts - Powering on hosts for provisioning or to increase capacity - Allowing system administrators to see the current power state of the host For testing this feature `ipmisim` can be used: https://pypi.python.org/pypi/ipmisim FS: https://cwiki.apache.org/confluence/display/CLOUDSTACK/Out-of-band+Management+for+CloudStack Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r60357906
--- Diff: engine/schema/src/org/apache/cloudstack/acl/RolePermissionVO.java
---
@@ -0,0 +1,109 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import javax.persistence.Column;
+import javax.persistence.Entity;
+import javax.persistence.EnumType;
+import javax.persistence.Enumerated;
+import javax.persistence.GeneratedValue;
+import javax.persistence.GenerationType;
+import javax.persistence.Id;
+import javax.persistence.Table;
+import java.util.UUID;
+
+@Entity
+@Table(name = "role_permissions")
+public class RolePermissionVO implements RolePermission {
--- End diff --
@koushik-das with the current static commands.properties based approach,
new APIs are enabled for role by developer by making changes in
commands.properties file. During installation it is seen that any pre-existing
commands.properties file does not get overwritten, so admin need to
enable/change API for roles manually in commands.properties file.
With this feature, the API developer when adding new API will enable the
API for the default role type using the authorized field in \@APICommand
annotation. This is strictly to enforce default behaviour when there are no
allow or deny rule for that API. The admin when upgrading to a new version to
get the new APIs etc won't have to manually allow/deny them if authorized
annotation is present. In general, the release notes should have list of new
APIs that admins read and decide if they want to update/add/modify role
permissions.
I'm not sure what you mean by `Role refers RolePermissions`, as of now each
role has a list of role permissions linked to it if that's what you're asking.
This has been documented in the FS under the section E:
https://cwiki.apache.org/confluence/display/CLOUDSTACK/Dynamic+Role+Based+API+Access+Checker+for+CloudStack
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-212284556 @jburwell fixed use of test timeout within \@Test annotation --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-212065979 @borisstoyanov would like to share our QA results, 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-212065155 @swill just completed local build here: (also pushed -f, let's see if Travis reports any failure) [INFO] [INFO] Building Apache CloudStack apidocs 4.9.0-SNAPSHOT [INFO] [INFO] [INFO] --- maven-clean-plugin:2.5:clean (default-clean) @ cloud-apidoc --- [INFO] Deleting /home/bhaisaab/Lab/apache/cloudstack/tools/apidoc/target (includes = [**/*], excludes = []) [INFO] Deleting /home/bhaisaab/Lab/apache/cloudstack/tools/apidoc (includes = [target, dist], excludes = []) [INFO] [INFO] --- maven-checkstyle-plugin:2.11:check (cloudstack-checkstyle) @ cloud-apidoc --- [INFO] Starting audit... Audit done. [INFO] [INFO] --- maven-remote-resources-plugin:1.3:process (default) @ cloud-apidoc --- [INFO] [INFO] --- exec-maven-plugin:1.2.1:exec (compile) @ cloud-apidoc --- log4j:WARN No appenders could be found for logger (org.reflections.Reflections). log4j:WARN Please initialize the log4j system properly. log4j:WARN See http://logging.apache.org/log4j/1.2/faq.html#noconfig for more info. Warning, API Cmd class com.cloud.api.commands.SimulatorAddSecondaryAgent has no APICommand annotation Scanned and found 557 APIs [INFO] [INFO] --- maven-site-plugin:3.1:attach-descriptor (attach-descriptor) @ cloud-apidoc --- [INFO] [INFO] --- exec-maven-plugin:1.2.1:exec (package) @ cloud-apidoc --- [INFO] [INFO] --- maven-install-plugin:2.3.1:install (default-install) @ cloud-apidoc --- [INFO] Installing /home/bhaisaab/Lab/apache/cloudstack/tools/apidoc/pom.xml to /home/bhaisaab/.m2/repository/org/apache/cloudstack/cloud-apidoc/4.9.0-SNAPSHOT/cloud-apidoc-4.9.0-SNAPSHOT.pom --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: maven: Upgrade dependency versions
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1397#issuecomment-212063724 @swill no problem, I'll deal with this during weekends and see how much we can safely get into master. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-212063057 @swill I looked at it, looks like an environment issue? I was able to build it without issue (also did not find 'root_admin' key in the code. I've rebased master and doing a push -f. Travis builds apidocs as well, we'll know if it fails/passes. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9296] Start ipsec for client ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1423#issuecomment-212062046 @syed looks like marvin plugin is missing; if you've both python 2.6/2.7 (looks like nose is using python 2.6) make sure to install it for the one you use (the default python). Alternatively, try to install using pip with --upgrade; for example, after building cloudstack run: sudo pip install --upgrade tools/marvin/dist/.tar.gz See if this helps. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1420#issuecomment-212060526 @swill I've explained my intent above, and it was not intended to be hard on @rafaelweingartner either While he's indeed putting good effort on PRs, I'm seeing some pattern of review types (not specific to him but in general) and I've shared my suggestions for him that's all. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1420#discussion_r60284224 --- Diff: packaging/centos7/cloud-management.service --- @@ -24,6 +24,7 @@ Description=CloudStack Management Server After=syslog.target network.target [Service] +UMask=0022 --- End diff -- @rafaelweingartner I'm sorry, I don't intend to be rude to you. Bear in mind, I'm doing this after hours in my free time as you are reviewing in your free time; I'm tired and exhausted and I just want to see things move. I'm embarrassed hence the facepalm. I'm trying to help you here, I've shared a list of things (in a comment below) that you can find useful and improve on -- kind of reviewing the reviewer, and it's fine with me if you disagree on things I suggested. This is my experience and view (and the view in not specific to you but reviewers in general) -- In an opensource community, we need to know the kind of expectation we can have from PR authors. For this specific case, it's alright that someone did not understand something, but PR author is not responsible or required to explain how xyz works, in this case how systemd scripts work especially when you can read the documentation. In ACS specific community we take reviews seriously and PR generally get blocked unless outstanding issues get resolved, and in a way that holds the PR author like a hostage where they either fix it the reviewer's way, or reach a compromise or have his contribution rot. This general ends up wasting three people's time -- the author, the release manager (wherever applicable) and the reviewer; and time wasted when something (like this) may become a bigger issue than what was intended in the first place. Lastly, when we're working in globally distributed timez one it adds a drag, momentum is lost and we lose contribution pace or something interest. To give you a background -- last month, we saw sort of a CloudStack "ice-age" where not a single commit was committed/merged for more than 30 days. I received emails from users asking if the project is dying. I figured what I can do to fix this, I fixed Travis so we've a faster way to check PRs. We also want to be pragmatic to PRs and avoid wasting time, or dragging over issues. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1420#issuecomment-212034630 @swill we may ignore them, those are non-issues unless @rafaelweingartner has strong opinions @rafaelweingartner things you can do to improve your reviews: - Google things when you don't understand them, try to read documentation for tool/domain specific things - Try to read CloudStack's code (and not just the diffs) and try to see the bigger picture, understand the why behind the PR and ask questions on dev@ when you hit dead-end. Everyone wants to help each other but we should avoid wasting time as much as we can, esr has documented this nicely here: http://www.catb.org/esr/faqs/smart-questions.html - Stackoverflow helps as well (see this for example, http://stackoverflow.com/questions/13268796/umask-difference-between-0022-and-022) - Understand that adding non-issue comment adds frictions on PRs, while pragmatic reviews (especially those around design, security, real world usage, upgrades and bigger picture) helps a ton - The PR author may not be responsible or required to explain to each individual how each part of the system (while they normally would, but we should not expect that) especially loosely related technologies including dependencies work (for example for this PR, I'm not required to explain you how Linux file permissions or systemd works -- if you don't know that you may invest time in learning them on your own) - Avoid leading a PR and their author to change for a system-wide refactoring: while in general the codebase is inconsistent due to lack of coding guidelines and lack of historic enforcement, your specific taste of how variables are named, what utilities to use, how to write comments, how to place files and structure packages may differ from others and other such cosmetic preferences. All of such things if necessary to fix should have a separate PR/effort and not to be coupled with an given PR. - Other than checkstyle maven plugin we don't have any coding enforcement, if you've strong opinions and want to see change -- you may also consider taking a bigger effort to define a coding guideline and have contributors adopt that over time. - In the community, we value people over code -- therefore, avoid idealism and favour pragmatism else you risk adding unnecessary friction because we'll in general block PRs until an outstanding issue is resolved --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1420#discussion_r60272259 --- Diff: packaging/centos7/cloud-management.service --- @@ -24,6 +24,7 @@ Description=CloudStack Management Server After=syslog.target network.target [Service] +UMask=0022 --- End diff -- @rafaelweingartner (facepalm) RTFM https://www.freedesktop.org/software/systemd/man/systemd.exec.html --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-211923768 @swill okay I'll check this out and see if I can reproduce the failure. I'll keep you posted. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: maven: Upgrade dependency versions
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1397#issuecomment-211786322 @swill thanks Will, I'll try to split this PR into two to isolate what changes are causing this in your env and see if we can get it fixed/improved separately --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1365#issuecomment-211785845 @swill we're good with this PR and it is not dependent on any other PR. For building noredist rpms/deb, one can use this repo with the dep-jars -- https://github.com/bhaisaab/cloudstack-nonoss --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1365#discussion_r60188424 --- Diff: pom.xml --- @@ -92,7 +92,7 @@ 2.5 1.2 1.0-20081010.060147 -5.5 +6.0 --- End diff -- @rafaelweingartner @swill I'll moved version change from the other PR here ^^ --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1420#issuecomment-211785290 @swill thanks, the change builds into systemvm.iso and no new template is necessary. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9296] Start ipsec for client ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1423#issuecomment-211524917 @swill we can ask around on ML, afaik in addition to strongswan I known of the ospf stuff by @abhinandanprateek which needs zebra and related packages for ospf. Abhi is on leave this week, but we can discuss this next week. In general, we should evaluate and build a new systemvm template based on what new features end up in master until the freeze date in May (as you had shared on dev@). --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [CLOUDSTACK-9296] Start ipsec for client ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1423#issuecomment-211481666 @swill we'll need to publish a new systemvm template (i.e. we can no longer use the 4.6 systemvm template) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60096454
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long
vmId, long volumeId) {
}
}
+public void updateMissingRootDiskController(final VMInstanceVO vm,
final String rootVolChainInfo) {
+if (vm == null || !vm.getType().equals(VirtualMachine.Type.User)
|| Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
the order of evalution will be left to right -- the first case is vm ==
null; if this qualifies the if statement will branch into executing code;
otherwise vm.getType() code will be executed (i.e. vm is not null); now here I
think you're suggesting that perhaps vm.getType() may return null in which case
we may have a NPE (unlikely based on db contraints (the type column should not
be null), but I'll modify as suggested.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1365#issuecomment-211479011 @rafaelweingartner both your comments are opinionated and not technical; we've to agree to disagree here ask you to be practical and pragmatic here. I've left comments above and I disagree on both mentioned places. Finally, what you would consider may be a huge change but for me a +190/-42 lines is not huge. If you go on this path, you might even find issues with quarks and neutrinos, and there is no end to over-engineering. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1365#discussion_r60095074
--- Diff: server/src/com/cloud/storage/VolumeApiServiceImpl.java ---
@@ -1835,6 +1847,26 @@ private Volume orchestrateDetachVolumeFromVM(long
vmId, long volumeId) {
}
}
+public void updateMissingRootDiskController(final VMInstanceVO vm,
final String rootVolChainInfo) {
+if (vm == null || !vm.getType().equals(VirtualMachine.Type.User)
|| Strings.isNullOrEmpty(rootVolChainInfo)) {
--- End diff --
@rafaelweingartner we'll get NPE anyway, we'll have to check vm != null as
in what you've suggested we're doing equals against vm.getType() (therefore
vm!=null check needed here).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack-docs-admin pull request: CLOUDSTACK-8562: add informati...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack-docs-admin/pull/37#issuecomment-211475206 Thanks @pdion891 sure after the freeze date, we can add to the release notes list of new APIs, list of new features along with any major changes etc. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-211320093 Doc PR - https://github.com/apache/cloudstack-docs-admin/pull/37 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack-docs-admin pull request: CLOUDSTACK-8562: add informati...
GitHub user bhaisaab opened a pull request: https://github.com/apache/cloudstack-docs-admin/pull/37 CLOUDSTACK-8562: add information on dynamic roles Adds documentation on dynamic roles feature /cc @swill @pdion891 You can merge this pull request into a Git repository by running: $ git pull https://github.com/shapeblue/cloudstack-docs-admin dynamic-roles-master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cloudstack-docs-admin/pull/37.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 #37 commit 300d0640308c36c3701eac50e8252b158a2d49ca Author: Rohit Yadav <rohit.ya...@shapeblue.com> Date: 2016-04-18T10:26:39Z CLOUDSTACK-8562: add information on dynamic roles Adds documentation on dynamic roles feature Signed-off-by: Rohit Yadav <rohit.ya...@shapeblue.com> --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack-docs-admin pull request: Be explicit in regards to Open...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack-docs-admin/pull/35#issuecomment-211291830 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: [4.7] vmware: improve support for disks
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1365#issuecomment-211264892 @swill can we get this merged, while ShapeBlue has tested this internally if you test with your non-vmware CI that should confirm it does not break anything else; ideally we would need a vmware-based CI to confirm this but if you can consider our internal testing results. 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: maven: Upgrade dependency versions
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1397#issuecomment-211263699 @swill advise how to proceed on this one? are you still getting the build issues @DaanHoogland ping --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: systemvm: preserve file permissions, set ...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1420#issuecomment-211263537 @swill yes though we should run CI, the fix is simple and ensure that umask setting is used when creating new files. the umask 022 is default on most distros resulting in 644 or rw-r--r-- file mods (for example ubuntu: http://askubuntu.com/questions/44542/what-is-umask-and-how-does-it-work) --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-211262271 @swill I've fixed the outstanding issues, can you run your CI on this and help merge? 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-211261793 @swill I've fixed the outstanding issues, can you run your CI on this and help merge? 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1493#issuecomment-210576373 Thanks all for the review, I've update the commits; please re-review and advise other outstanding issue. Thanks again. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59917981
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", 0, 1, new NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59915385
--- Diff: utils/src/main/java/com/cloud/utils/nio/Link.java ---
@@ -453,115 +449,192 @@ public static SSLContext initSSLContext(boolean
isClient) throws GeneralSecurity
return sslContext;
}
-public static void doHandshake(SocketChannel ch, SSLEngine sslEngine,
boolean isClient) throws IOException {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: begin Handshake, isClient: " + isClient);
+public static ByteBuffer enlargeBuffer(ByteBuffer buffer, final int
sessionProposedCapacity) {
+if (buffer == null || sessionProposedCapacity < 0) {
+return buffer;
}
-
-SSLEngineResult engResult;
-SSLSession sslSession = sslEngine.getSession();
-HandshakeStatus hsStatus;
-ByteBuffer in_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer in_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-ByteBuffer out_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer out_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-int count;
-ch.socket().setSoTimeout(60 * 1000);
-InputStream inStream = ch.socket().getInputStream();
-// Use readCh to make sure the timeout on reading is working
-ReadableByteChannel readCh = Channels.newChannel(inStream);
-
-if (isClient) {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_WRAP;
+if (sessionProposedCapacity > buffer.capacity()) {
+buffer = ByteBuffer.allocate(sessionProposedCapacity);
} else {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_UNWRAP;
+buffer = ByteBuffer.allocate(buffer.capacity() * 2);
}
+return buffer;
+}
-while (hsStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: Handshake status " + hsStatus);
+public static ByteBuffer handleBufferUnderflow(final SSLEngine engine,
ByteBuffer buffer) {
+if (engine == null || buffer == null) {
+return buffer;
+}
+if (buffer.position() < buffer.limit()) {
+return buffer;
+}
+ByteBuffer replaceBuffer = enlargeBuffer(buffer,
engine.getSession().getPacketBufferSize());
+buffer.flip();
+replaceBuffer.put(buffer);
+return replaceBuffer;
+}
+
+private static boolean doHandshakeUnwrap(final SocketChannel
socketChannel, final SSLEngine sslEngine,
+ ByteBuffer peerAppData,
ByteBuffer peerNetData, final int appBufferSize) throws IOException {
+if (socketChannel == null || sslEngine == null || peerAppData ==
null || peerNetData == null || appBufferSize < 0) {
+return false;
+}
+if (socketChannel.read(peerNetData) < 0) {
+if (sslEngine.isInboundDone() && sslEngine.isOutboundDone()) {
+return false;
}
-engResult = null;
-if (hsStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP) {
-out_pkgBuf.clear();
-out_appBuf.clear();
-out_appBuf.put("Hello".getBytes());
-engResult = sslEngine.wrap(out_appBuf, out_pkgBuf);
-out_pkgBuf.flip();
-int remain = out_pkgBuf.limit();
-while (remain != 0) {
-remain -= ch.write(out_pkgBuf);
-if (remain < 0) {
-throw new IOException("Too much bytes sent?");
-}
-}
-} else if (hsStatus ==
SSLEngineResult.HandshakeStatus.NEED_UNWRAP) {
-in_appBuf.clear();
-// One packet may contained multiply operation
-if (in_pkgBuf.position() == 0 ||
!in_pkgBuf.hasRemaining()) {
-in_pkgBuf.clear();
-count = 0;
-try {
-count = readCh.read(in_pkgBuf);
-} catch (SocketTimeoutException ex) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("Handshake reading time out!
Cut the connection");
-}
-cou
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59915248
--- Diff: utils/src/main/java/com/cloud/utils/nio/Link.java ---
@@ -453,115 +449,192 @@ public static SSLContext initSSLContext(boolean
isClient) throws GeneralSecurity
return sslContext;
}
-public static void doHandshake(SocketChannel ch, SSLEngine sslEngine,
boolean isClient) throws IOException {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: begin Handshake, isClient: " + isClient);
+public static ByteBuffer enlargeBuffer(ByteBuffer buffer, final int
sessionProposedCapacity) {
+if (buffer == null || sessionProposedCapacity < 0) {
+return buffer;
}
-
-SSLEngineResult engResult;
-SSLSession sslSession = sslEngine.getSession();
-HandshakeStatus hsStatus;
-ByteBuffer in_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer in_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-ByteBuffer out_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer out_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-int count;
-ch.socket().setSoTimeout(60 * 1000);
-InputStream inStream = ch.socket().getInputStream();
-// Use readCh to make sure the timeout on reading is working
-ReadableByteChannel readCh = Channels.newChannel(inStream);
-
-if (isClient) {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_WRAP;
+if (sessionProposedCapacity > buffer.capacity()) {
+buffer = ByteBuffer.allocate(sessionProposedCapacity);
} else {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_UNWRAP;
+buffer = ByteBuffer.allocate(buffer.capacity() * 2);
}
+return buffer;
+}
-while (hsStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: Handshake status " + hsStatus);
+public static ByteBuffer handleBufferUnderflow(final SSLEngine engine,
ByteBuffer buffer) {
+if (engine == null || buffer == null) {
+return buffer;
+}
+if (buffer.position() < buffer.limit()) {
+return buffer;
+}
+ByteBuffer replaceBuffer = enlargeBuffer(buffer,
engine.getSession().getPacketBufferSize());
+buffer.flip();
+replaceBuffer.put(buffer);
+return replaceBuffer;
+}
+
+private static boolean doHandshakeUnwrap(final SocketChannel
socketChannel, final SSLEngine sslEngine,
+ ByteBuffer peerAppData,
ByteBuffer peerNetData, final int appBufferSize) throws IOException {
+if (socketChannel == null || sslEngine == null || peerAppData ==
null || peerNetData == null || appBufferSize < 0) {
+return false;
+}
+if (socketChannel.read(peerNetData) < 0) {
+if (sslEngine.isInboundDone() && sslEngine.isOutboundDone()) {
+return false;
}
-engResult = null;
-if (hsStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP) {
-out_pkgBuf.clear();
-out_appBuf.clear();
-out_appBuf.put("Hello".getBytes());
-engResult = sslEngine.wrap(out_appBuf, out_pkgBuf);
-out_pkgBuf.flip();
-int remain = out_pkgBuf.limit();
-while (remain != 0) {
-remain -= ch.write(out_pkgBuf);
-if (remain < 0) {
-throw new IOException("Too much bytes sent?");
-}
-}
-} else if (hsStatus ==
SSLEngineResult.HandshakeStatus.NEED_UNWRAP) {
-in_appBuf.clear();
-// One packet may contained multiply operation
-if (in_pkgBuf.position() == 0 ||
!in_pkgBuf.hasRemaining()) {
-in_pkgBuf.clear();
-count = 0;
-try {
-count = readCh.read(in_pkgBuf);
-} catch (SocketTimeoutException ex) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("Handshake reading time out!
Cut the connection");
-}
-cou
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59915144
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", 0, 1, new NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59914101
--- Diff: utils/src/main/java/com/cloud/utils/nio/Link.java ---
@@ -453,115 +449,192 @@ public static SSLContext initSSLContext(boolean
isClient) throws GeneralSecurity
return sslContext;
}
-public static void doHandshake(SocketChannel ch, SSLEngine sslEngine,
boolean isClient) throws IOException {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: begin Handshake, isClient: " + isClient);
+public static ByteBuffer enlargeBuffer(ByteBuffer buffer, final int
sessionProposedCapacity) {
+if (buffer == null || sessionProposedCapacity < 0) {
+return buffer;
}
-
-SSLEngineResult engResult;
-SSLSession sslSession = sslEngine.getSession();
-HandshakeStatus hsStatus;
-ByteBuffer in_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer in_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-ByteBuffer out_pkgBuf =
ByteBuffer.allocate(sslSession.getPacketBufferSize() + 40);
-ByteBuffer out_appBuf =
ByteBuffer.allocate(sslSession.getApplicationBufferSize() + 40);
-int count;
-ch.socket().setSoTimeout(60 * 1000);
-InputStream inStream = ch.socket().getInputStream();
-// Use readCh to make sure the timeout on reading is working
-ReadableByteChannel readCh = Channels.newChannel(inStream);
-
-if (isClient) {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_WRAP;
+if (sessionProposedCapacity > buffer.capacity()) {
+buffer = ByteBuffer.allocate(sessionProposedCapacity);
} else {
-hsStatus = SSLEngineResult.HandshakeStatus.NEED_UNWRAP;
+buffer = ByteBuffer.allocate(buffer.capacity() * 2);
}
+return buffer;
+}
-while (hsStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("SSL: Handshake status " + hsStatus);
+public static ByteBuffer handleBufferUnderflow(final SSLEngine engine,
ByteBuffer buffer) {
+if (engine == null || buffer == null) {
+return buffer;
+}
+if (buffer.position() < buffer.limit()) {
+return buffer;
+}
+ByteBuffer replaceBuffer = enlargeBuffer(buffer,
engine.getSession().getPacketBufferSize());
+buffer.flip();
+replaceBuffer.put(buffer);
+return replaceBuffer;
+}
+
+private static boolean doHandshakeUnwrap(final SocketChannel
socketChannel, final SSLEngine sslEngine,
+ ByteBuffer peerAppData,
ByteBuffer peerNetData, final int appBufferSize) throws IOException {
+if (socketChannel == null || sslEngine == null || peerAppData ==
null || peerNetData == null || appBufferSize < 0) {
+return false;
+}
+if (socketChannel.read(peerNetData) < 0) {
+if (sslEngine.isInboundDone() && sslEngine.isOutboundDone()) {
+return false;
}
-engResult = null;
-if (hsStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP) {
-out_pkgBuf.clear();
-out_appBuf.clear();
-out_appBuf.put("Hello".getBytes());
-engResult = sslEngine.wrap(out_appBuf, out_pkgBuf);
-out_pkgBuf.flip();
-int remain = out_pkgBuf.limit();
-while (remain != 0) {
-remain -= ch.write(out_pkgBuf);
-if (remain < 0) {
-throw new IOException("Too much bytes sent?");
-}
-}
-} else if (hsStatus ==
SSLEngineResult.HandshakeStatus.NEED_UNWRAP) {
-in_appBuf.clear();
-// One packet may contained multiply operation
-if (in_pkgBuf.position() == 0 ||
!in_pkgBuf.hasRemaining()) {
-in_pkgBuf.clear();
-count = 0;
-try {
-count = readCh.read(in_pkgBuf);
-} catch (SocketTimeoutException ex) {
-if (s_logger.isTraceEnabled()) {
-s_logger.trace("Handshake reading time out!
Cut the connection");
-}
-cou
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59913436
--- Diff:
utils/src/test/java/com/cloud/utils/backoff/impl/ConstantTimeBackoffTest.java
---
@@ -94,7 +94,7 @@ public void wakeupNotExisting() {
@Test
public void wakeupExisting() throws InterruptedException {
final ConstantTimeBackoff backoff = new ConstantTimeBackoff();
-backoff.setTimeToWait(10);
+backoff.setTimeToWait(1000);
--- End diff --
I was trying to diagnose why this test was failing on Travis, so added a
large value. Removed now.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-9348: Use non-blocking SSL han...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1493#discussion_r59913672
--- Diff: utils/src/test/java/com/cloud/utils/testcase/NioTest.java ---
@@ -19,146 +19,198 @@
package com.cloud.utils.testcase;
-import java.nio.channels.ClosedChannelException;
-import java.util.Random;
-
-import junit.framework.TestCase;
-
-import org.apache.log4j.Logger;
-import org.junit.Assert;
-
+import com.cloud.utils.concurrency.NamedThreadFactory;
import com.cloud.utils.exception.NioConnectionException;
import com.cloud.utils.nio.HandlerFactory;
import com.cloud.utils.nio.Link;
import com.cloud.utils.nio.NioClient;
import com.cloud.utils.nio.NioServer;
import com.cloud.utils.nio.Task;
import com.cloud.utils.nio.Task.Type;
+import org.apache.log4j.Logger;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
-/**
- *
- *
- *
- *
- */
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.nio.channels.ClosedChannelException;
+import java.nio.channels.Selector;
+import java.nio.channels.SocketChannel;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+public class NioTest {
-public class NioTest extends TestCase {
+private static final Logger LOGGER = Logger.getLogger(NioTest.class);
-private static final Logger s_logger = Logger.getLogger(NioTest.class);
+final private int totalTestCount = 10;
+private int completedTestCount = 0;
-private NioServer _server;
-private NioClient _client;
+private NioServer server;
+private List clients = new ArrayList<>();
+private List maliciousClients = new ArrayList<>();
-private Link _clientLink;
+private ExecutorService clientExecutor =
Executors.newFixedThreadPool(totalTestCount, new
NamedThreadFactory("NioClientHandler"));;
+private ExecutorService maliciousExecutor =
Executors.newFixedThreadPool(5*totalTestCount, new
NamedThreadFactory("MaliciousNioClientHandler"));;
-private int _testCount;
-private int _completedCount;
+private Random randomGenerator = new Random();
+private byte[] testBytes;
private boolean isTestsDone() {
boolean result;
synchronized (this) {
-result = _testCount == _completedCount;
+result = totalTestCount == completedTestCount;
}
return result;
}
-private void getOneMoreTest() {
-synchronized (this) {
-_testCount++;
-}
-}
-
private void oneMoreTestDone() {
synchronized (this) {
-_completedCount++;
+completedTestCount++;
}
}
-@Override
+@Before
public void setUp() {
-s_logger.info("Test");
+LOGGER.info("Setting up Benchmark Test");
-_testCount = 0;
-_completedCount = 0;
-
-_server = new NioServer("NioTestServer", , 5, new
NioTestServer());
-try {
-_server.start();
-} catch (final NioConnectionException e) {
-fail(e.getMessage());
-}
+completedTestCount = 0;
+testBytes = new byte[100];
+randomGenerator.nextBytes(testBytes);
-_client = new NioClient("NioTestServer", "127.0.0.1", , 5, new
NioTestClient());
+// Server configured with one worker
+server = new NioServer("NioTestServer", 0, 1, new NioTestServer());
try {
-_client.start();
+server.start();
} catch (final NioConnectionException e) {
-fail(e.getMessage());
+Assert.fail(e.getMessage());
}
-while (_clientLink == null) {
-try {
-s_logger.debug("Link is not up! Waiting ...");
-Thread.sleep(1000);
-} catch (final InterruptedException e) {
-// TODO Auto-generated catch block
-e.printStackTrace();
+// 5 malicious clients per valid client
+for (int i = 0; i < totalTestCount; i++) {
+for (int j = 0; j < 5; j++) {
+final NioClient maliciousClient = new
NioMaliciousClient("NioMaliciousTestClient-" + i, "
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on the pull request: https://github.com/apache/cloudstack/pull/1489#issuecomment-210423416 Thanks all, I've fixed most issues. Please see any outstanding issue and let me know if you find any other issue. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59858602
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
+} catch (SQLException e) {
+throw new CloudRuntimeException("Excepti
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59858459
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
+} catch (SQLException e) {
+throw new CloudRuntimeException("Excepti
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59858432
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
+while (selectResultSet.next()) {
+Long accountId = selectResultSet.getLong(1);
+Short accountType = selectResultSet.getShort(2);
+Long roleId = null;
+for (RoleType roleType : roleTypes) {
+if (roleType.getAccountType() == accountType) {
+roleId = roleType.getId();
+break;
+}
+}
+if (roleId == null) {
+continue;
+}
+try (PreparedStatement updateStatement =
conn.prepareStatement("UPDATE `cloud`.`account` SET role_id = ? WHERE id =
?;")) {
+updateStatement.setLong(1, roleId);
+updateStatement.setLong(2, accountId);
+updateStatement.executeUpdate();
+updateStatement.close();
+
+} catch (SQLException e) {
+s_logger.error("Failed to update cloud.account role_id
for account id:" + accountId + " with exception: " + e.getMessage());
+throw new CloudRuntimeException("Exception while
updating cloud.account role_id", e);
+}
+}
--- End diff --
at the time of upgrade there will be only a single thread running this,
pe
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59858001
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
+}
+}
+
+private void addRoleColumnAndMigrateAccountTable(final Connection
conn, final RoleType[] roleTypes) {
+final String alterTableSql = "ALTER TABLE `cloud`.`account` ADD
COLUMN `role_id` bigint(20) unsigned COMMENT 'role id for this account' AFTER
`type`, " +
+"ADD KEY `fk_account__role_id` (`role_id`), " +
+"ADD CONSTRAINT `fk_account__role_id` FOREIGN KEY
(`role_id`) REFERENCES `roles` (`id`);";
+try (PreparedStatement pstmt =
conn.prepareStatement(alterTableSql)) {
+pstmt.executeUpdate();
+s_logger.info("Altered cloud.account table and added column
role_id");
+} catch (SQLException e) {
+if (e.getMessage().contains("role_id")) {
+s_logger.warn("cloud.account table already has the role_id
column, skipping altering table and migration of accounts");
+return;
+} else {
+throw new CloudRuntimeException("Unable to create column
quota_calculated in table cloud_usage.cloud_usage", e);
+}
+}
+migrateAccountsToDefaultRoles(conn, roleTypes);
+}
+
+private void migrateAccountsToDefaultRoles(final Connection conn,
final RoleType[] roleTypes) {
+try (PreparedStatement selectStatement =
conn.prepareStatement("SELECT `id`, `type` FROM `cloud`.`account`;");
+ ResultSet selectResultSet = selectStatement.executeQuery()) {
--- End diff --
try with resource can have multiple resources, no need for explicit close
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59857962
--- Diff: engine/schema/src/com/cloud/upgrade/dao/Upgrade481to490.java ---
@@ -53,6 +62,139 @@ public boolean supportsRollingUpgrade() {
@Override
public void performDataMigration(Connection conn) {
+setupRolesAndPermissionsForDynamicRBAC(conn);
+}
+
+private void createDefaultRole(final Connection conn, final Long id,
final String name, final RoleType roleType) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`roles` (`id`, `uuid`, `name`, `role_type`, `description`) values (%d,
UUID(), '%s', '%s', 'Default %s role');",
+id, name, roleType.name(), roleType.name().toLowerCase());
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql) ) {
+updatePstmt.executeUpdate();
+} catch (SQLException e) {
+throw new CloudRuntimeException("Unable to create default role
with id: " + id + " name: " + name, e);
+}
+}
+
+private void createRoleMapping(final Connection conn, final Long
roleId, final String apiName) {
+final String insertSql = String.format("INSERT INTO
`cloud`.`role_permissions` (`uuid`, `role_id`, `rule`, `permission`) values
(UUID(), %d, '%s', 'ALLOW') ON DUPLICATE KEY UPDATE rule=rule;",
+roleId, apiName);
+try ( PreparedStatement updatePstmt =
conn.prepareStatement(insertSql)) {
+updatePstmt.executeUpdate();
+} catch (SQLException ignored) {
+s_logger.debug("Unable to insert mapping for role id:" +
roleId + " apiName: " + apiName);
--- End diff --
fixed
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59857842
--- Diff: api/test/org/apache/cloudstack/acl/RuleTest.java ---
@@ -0,0 +1,39 @@
+package org.apache.cloudstack.acl;
+
+import com.cloud.exception.InvalidParameterValueException;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.util.Arrays;
+
+public class RuleTest {
+
+@Test
+public void testToString() throws Exception {
+Rule rule = new Rule("someString");
+Assert.assertEquals(rule.toString(), "someString");
+}
+
+@Test
+public void testValidateRuleWithValidData() throws Exception {
+for (String rule : Arrays.asList("a", "1", "someApi",
"someApi321", "123SomeApi",
+"prefix*", "*middle*", "*Suffix",
+"*", "**", "f***", "m0nk3yMa**g1c*")) {
+Assert.assertTrue(Rule.validate(rule));
+Assert.assertEquals(new Rule(rule).toString(), rule);
+}
+}
+
+@Test
+public void testValidateRuleWithInvalidData() throws Exception {
+for (String rule : Arrays.asList(null, "", " ", " ", "\n", "\t",
"\r", "\"", "\'",
+"^someApi$", "^someApi", "some$", "some-Api;", "some,Api",
+"^", "$", "^$", ".*", "\\w+", "r**l3rd0@Kr3",
"j@s1n|+|0È·",
+"[a-z0-9-]+",
"^([a-z0-9_\\.-]+)@([\\da-z\\.-]+)\\.([a-z\\.]{2,6})$")) {
+try {
+new Rule(rule);
+Assert.fail("Invalid rule, exception was expected");
+} catch (InvalidParameterValueException ignored) {}
+}
--- End diff --
It's in a for loop, so won't do
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59857696
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
+for (RoleType roleType : RoleType.values()) {
+Assert.assertEquals(RoleType.fromString(roleType.name()),
roleType);
+}
+}
+
+@Test
+public void testDefaultRoleMaskByValue() {
+Assert.assertEquals(RoleType.fromMask(1), RoleType.Admin);
+Assert.assertEquals(RoleType.fromMask(2), RoleType.ResourceAdmin);
+Assert.assertEquals(RoleType.fromMask(4), RoleType.DomainAdmin);
+Assert.assertEquals(RoleType.fromMask(8), RoleType.User);
+Assert.assertEquals(RoleType.fromMask(0), RoleType.Unknown);
+}
+
+@Test
+public void testGetByAccountType() {
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_NORMAL),
RoleType.User);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_ADMIN),
RoleType.Admin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_DOMAIN_ADMIN),
RoleType.DomainAdmin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_RESOURCE_DOMAIN_ADMIN),
RoleType.ResourceAdmin);
+
Assert.assertEquals(RoleType.getByAccountType(Account.ACCOUNT_TYPE_PROJECT),
RoleType.Unknown);
+}
+
+@Test
+public void testGetRoleByAccountTypeWhenRoleIdIsProvided() {
+Assert.assertEquals(RoleType.getRoleByAccountType(123L,
Account.ACCOUNT_TYPE_ADMIN), Long.valueOf(123L));
+Assert.assertEquals(RoleType.getRoleByAccountType(1234L, null),
Long.valueOf(1234L));
+}
+
+@Test
+public void testGetRoleByAccountTypeForDefaultAccountTypes() {
+Assert.assertEquals(RoleType.getRoleByAccountType(null,
Account.ACCOUNT_TYPE_ADMIN), (Long) RoleType.Admin.getId());
--- End diff --
There is another test for testing when non-null values are passed
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59856830
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
--- End diff --
This method is used by create/update role apicmds, fixed it to throw
exception.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59856707
--- Diff: api/test/org/apache/cloudstack/acl/RoleTypeTest.java ---
@@ -0,0 +1,92 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.acl;
+
+import com.cloud.user.Account;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Arrays;
+
+public class RoleTypeTest {
+
+@Test
+public void testRoleTypeFromString() {
+Assert.assertEquals(RoleType.fromString(null), null);
+Assert.assertEquals(RoleType.fromString(""), null);
+Assert.assertEquals(RoleType.fromString("admin"), null);
+Assert.assertEquals(RoleType.fromString("12345%^&*"), null);
+for (RoleType roleType : RoleType.values()) {
+Assert.assertEquals(RoleType.fromString(roleType.name()),
roleType);
+}
+}
+
+@Test
+public void testDefaultRoleMaskByValue() {
+Assert.assertEquals(RoleType.fromMask(1), RoleType.Admin);
+Assert.assertEquals(RoleType.fromMask(2), RoleType.ResourceAdmin);
+Assert.assertEquals(RoleType.fromMask(4), RoleType.DomainAdmin);
+Assert.assertEquals(RoleType.fromMask(8), RoleType.User);
+Assert.assertEquals(RoleType.fromMask(0), RoleType.Unknown);
--- End diff --
We get `Unknown`, for example 0 is an invalid mask value here.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59856583
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/config/UpdateCfgCmd.java ---
@@ -117,6 +123,12 @@ public long getEntityOwnerId() {
@Override
public void execute() {
+if (Strings.isNullOrEmpty(getCfgName())) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Empty
configuration name provided");
+}
+if
(getCfgName().equalsIgnoreCase(RoleService.EnableDynamicApiChecker.key())) {
+throw new ServerApiException(ApiErrorCode.METHOD_NOT_ALLOWED,
"Restricted configuration update not allowed");
--- End diff --
fixed
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: CLOUDSTACK-8562: Dynamic Role-Based API C...
Github user bhaisaab commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1489#discussion_r59856523
--- Diff:
api/src/org/apache/cloudstack/api/command/admin/acl/ListRolePermissionsCmd.java
---
@@ -0,0 +1,104 @@
+// 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
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// 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.cloudstack.api.command.admin.acl;
+
+import com.cloud.exception.InsufficientCapacityException;
+import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.user.Account;
+import org.apache.cloudstack.acl.Role;
+import org.apache.cloudstack.acl.RolePermission;
+import org.apache.cloudstack.acl.RoleType;
+import org.apache.cloudstack.api.APICommand;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.ApiErrorCode;
+import org.apache.cloudstack.api.BaseCmd;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.api.response.RolePermissionResponse;
+import org.apache.cloudstack.api.response.RoleResponse;
+
+import java.util.ArrayList;
+import java.util.List;
+
+
+@APICommand(name = ListRolePermissionsCmd.APINAME, description = "Lists
role permissions", responseObject = RolePermissionResponse.class,
+requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
+since = "4.9.0",
+authorized = {RoleType.Admin})
+public class ListRolePermissionsCmd extends BaseCmd {
+public static final String APINAME = "listRolePermissions";
+
+/
+ API parameters /
+/
+
+@Parameter(name = ApiConstants.ROLE_ID, type = CommandType.UUID,
entityType = RoleResponse.class, description = "ID of the role")
+private Long roleId;
+
+/
+/// Accessors ///
+/
+
+public Long getRoleId() {
+return roleId;
+}
+
+/
+/// API Implementation///
+/
+
+@Override
+public String getCommandName() {
+return APINAME.toLowerCase() + BaseCmd.RESPONSE_SUFFIX;
+}
+
+@Override
+public long getEntityOwnerId() {
+return Account.ACCOUNT_ID_SYSTEM;
+}
+
+@Override
+public void execute() throws ResourceUnavailableException,
InsufficientCapacityException, ServerApiException {
+if (getRoleId() != null && getRoleId() < 1L) {
+throw new ServerApiException(ApiErrorCode.PARAM_ERROR,
"Invalid role id provided");
+}
+List rolePermissions =
roleService.findAllPermissionsBy(getRoleId());
+
+ListResponse response = new
ListResponse<>();
+List rolePermissionResponses = new
ArrayList<>();
+for (RolePermission rolePermission : rolePermissions) {
+Role role = roleService.findRole(rolePermission.getRoleId());
--- End diff --
I was trying to avoid a table join and Join Daos/VOs, in most cases you
won't be running this API without providing a role Id. I've fixed it for
average case.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
