[GitHub] cloudstack issue #1773: CLOUDSTACK-9607: Preventing template deletion when t...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1773 @priyankparihar I agree with @ustcweizhou regarding the default value of `forced` in terms of backwards compatibility. Also, why we permit deletion of a template when it is associated with one or more active volumes? It seems like we are giving the user the means to corrupt their system. --- 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 issue #1888: CLOUDSTACK-9710: Switch to JRE1.8
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1888 @rhtyd most Java-based systems do not specific a particular JDK/JRE as a dependency in their packages in order to allow administrators to pick the implementation they prefer (e.g. OpenJDK, Sun JDK, IBM JDK, etc). Should we consider removing those dependencies from ours packages for the same reason? Also, Travis supports testing [multiple Java versions](https://blog.travis-ci.com/support_for_multiple_jdks/). If we are not discontinuing support for 1.7, we should have Travis both 1.7 and 1.8. --- 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 #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95035212
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java
---
@@ -790,6 +791,18 @@ private String validateConfigurationValue(final String
name, String value, final
return null;
}
+if (type.equals(Integer.class) &&
NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
+try {
+final int val = Integer.parseInt(value);
+if(val <0 || val >255){
+throw new InvalidParameterValueException(name+" value
should be between 0 and 255. 0 value will disable this feature");
+}
--- End diff --
Why are we using a magic value in the parameter to determine whether or not
a feature is enabled? Why not add an explicit flag to make it clearer for the
end user?
---
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 #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95034972
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long
networkId, final long accountId, f
}
@Override
-public String getNextAvailableMacAddress(final long networkConfigId) {
+public String getNextAvailableMacAddress(final long networkConfigId,
Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress,
networkConfigId);
-seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 &
0xl;
+if(zoneMacIdentifier!=0){
--- End diff --
What is `zoneMacIdentifier` is `null`? Should there be a
`Preconditions.checkArgument` check added or a different behavior?
---
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 #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95035623
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long
networkId, final long accountId, f
}
@Override
-public String getNextAvailableMacAddress(final long networkConfigId) {
+public String getNextAvailableMacAddress(final long networkConfigId,
Integer zoneMacIdentifier) {
--- End diff --
Please add unit test case(s) to verify the behavior of this method.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request #1883: CLOUDSTACK-9723: Enable unique mac address ac...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1883#discussion_r95034912
--- Diff: engine/schema/src/com/cloud/network/dao/NetworkDaoImpl.java ---
@@ -377,11 +377,16 @@ protected void addAccountToNetwork(final long
networkId, final long accountId, f
}
@Override
-public String getNextAvailableMacAddress(final long networkConfigId) {
+public String getNextAvailableMacAddress(final long networkConfigId,
Integer zoneMacIdentifier) {
final SequenceFetcher fetch = SequenceFetcher.getInstance();
-
long seq = fetch.getNextSequence(Long.class, _tgMacAddress,
networkConfigId);
-seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) << 16 &
0xl;
+if(zoneMacIdentifier!=0){
+seq = seq | _prefix << 40 | (long)zoneMacIdentifier << 32 |
networkConfigId << 16 & 0xl;
+}
+else {
+seq = seq | _prefix << 40 | _rand.nextInt(Short.MAX_VALUE) <<
16 & 0xl;
+
+}
--- End diff --
Please format per coding standards.
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95033850
--- Diff: ui/scripts/storage.js ---
@@ -54,6 +54,12 @@
label: 'label.vm.display.name'
}
},
+actionPreFilter: function(args){
+if (g_enablemetricsui)
+return ['add', 'viewMetrics', 'uploadVolume',
'uploadVolumefromLocal'];
+else
+return ['add', 'uploadVolume',
'uploadVolumefromLocal'];
+},
--- End diff --
Please remove the duplication in this function by refactoring as follows:
```
actionPreFilter: function(args){
actions = ['add', 'uploadVolume',
'uploadVolumefromLocal'];
if (g_enablemetricsui) {
actions.splice(1, 0, 'viewMetrics');
}
return actions;
},
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034584
--- Diff: ui/scripts/ui/widgets/listView.js ---
@@ -1922,7 +1922,14 @@
// List view header actions
if (listViewData.actions) {
+// If a preFilter is set, then get the values. Else set this
to null
+var filteredActions = listViewData.actionPreFilter ==
undefined ?
+null : listViewData.actionPreFilter();
--- End diff --
Why not set to an empty list rather than ``null`` to avoid a potential null
error?
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034385
--- Diff: ui/scripts/system.js ---
@@ -17309,7 +17325,12 @@
}
});
},
-
+actionPreFilter: function(args){
+if (g_enablemetricsui)
+return ['add', 'viewMetrics'];
+else
+return ['add'];
+},
--- End diff --
Please code duplication comments 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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1884#discussion_r95033188 --- Diff: test/integration/smoke/test_global_settings.py --- @@ -63,6 +63,33 @@ def test_UpdateConfigParamWithScope(self): self.assertEqual(configParam.value, updateConfigurationResponse.value, "Check if the update API returned \ is the same as the one we got in the list API") +@attr(tags=["devcloud", "basic", "advanced"], required_hardware="false") +def test_list_capabilities(self): +""" +@summary: Test List Capabilities +@Steps +Step1: Listing all the Capabilities for a user +Step2: Verifying the listcapabilities object is not null +Step3: Verifying enable.metrics.ui is not null +""" +# Listing all the Capabilities for a user + +listCapabilities = Configurations.listCapabilities(self.apiClient) +# Verifying the listcapabilities object is not null +self.assertIsNotNone( +listCapabilities, +"Failed to list Capabilities" +) + +# Verifying enable.metrics.ui is not null +self.assertIsNotNone( +listCapabilities.enablemetricsui, +"Failed to fetch enable.metrics.ui" +) --- End diff -- Please add tests to verify that the value is set as expected. --- 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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95032496
--- Diff:
api/test/org/apache/cloudstack/api/command/test/ListCapabilitiesCmdTest.java ---
@@ -0,0 +1,80 @@
+// 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.test;
+
+
+import com.cloud.server.ManagementService;
+import com.cloud.utils.Pair;
+import junit.framework.Assert;
+import junit.framework.TestCase;
+import org.apache.cloudstack.api.ResponseGenerator;
+import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd;
+import org.apache.cloudstack.api.command.user.config.ListCapabilitiesCmd;
+import org.apache.cloudstack.api.response.CapabilitiesResponse;
+import org.apache.cloudstack.api.response.ConfigurationResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.config.Configuration;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class ListCapabilitiesCmdTest extends TestCase {
+
+private ListCapabilitiesCmd listCapabilitiesCmd;
+private ManagementService mgr;
+private ResponseGenerator responseGenerator;
+
+@Override
+@Before
+public void setUp() {
+responseGenerator = Mockito.mock(ResponseGenerator.class);
+mgr = Mockito.mock(ManagementService.class);
+listCapabilitiesCmd = new ListCapabilitiesCmd();
+}
+
+@Test
+public void testCreateSuccess() {
+
+listCapabilitiesCmd._mgr = mgr;
+listCapabilitiesCmd._responseGenerator = responseGenerator;
+
+Map<String, Object> result = new HashMap<String, Object>();
+
+try {
+
Mockito.when(mgr.listCapabilities(listCapabilitiesCmd)).thenReturn(result);
+} catch (Exception e) {
+Assert.fail("Received exception when success expected " +
e.getMessage());
+}
--- End diff --
Why not perform lines 58-67 in in `setUp`? Not only would this approach
focus this method on the test operations, but remove the need for unnecessary
class-level attributes.
Also, catching exceptions is not required because JUnit will automatically
fail when exceptions are thrown. If there are checked exceptions, add them to
the `throws` of the test method to keep test methods as succinct as possible.
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034302
--- Diff: ui/scripts/system.js ---
@@ -14088,6 +14093,12 @@
}
});
},
+actionPreFilter: function(args){
+if (g_enablemetricsui)
+return ['add', 'viewMetrics'];
+else
+return ['add'];
+},
--- End diff --
Lines 14096-14101 appear to be a duplication of lines 7857-7860. Can this
code duplication be removed?
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034357
--- Diff: ui/scripts/system.js ---
@@ -15581,7 +15592,12 @@
}
});
},
-
+actionPreFilter: function(args){
+if (g_enablemetricsui)
+return ['add', 'viewMetrics'];
+else
+return ['add'];
+},
--- End diff --
Please code duplication comments 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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95033034
--- Diff:
api/test/org/apache/cloudstack/api/command/test/ListCapabilitiesCmdTest.java ---
@@ -0,0 +1,80 @@
+// 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.test;
+
+
+import com.cloud.server.ManagementService;
+import com.cloud.utils.Pair;
+import junit.framework.Assert;
+import junit.framework.TestCase;
+import org.apache.cloudstack.api.ResponseGenerator;
+import org.apache.cloudstack.api.command.admin.config.ListCfgsByCmd;
+import org.apache.cloudstack.api.command.user.config.ListCapabilitiesCmd;
+import org.apache.cloudstack.api.response.CapabilitiesResponse;
+import org.apache.cloudstack.api.response.ConfigurationResponse;
+import org.apache.cloudstack.api.response.ListResponse;
+import org.apache.cloudstack.config.Configuration;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+public class ListCapabilitiesCmdTest extends TestCase {
+
+private ListCapabilitiesCmd listCapabilitiesCmd;
+private ManagementService mgr;
+private ResponseGenerator responseGenerator;
+
+@Override
+@Before
+public void setUp() {
+responseGenerator = Mockito.mock(ResponseGenerator.class);
+mgr = Mockito.mock(ManagementService.class);
+listCapabilitiesCmd = new ListCapabilitiesCmd();
+}
+
+@Test
+public void testCreateSuccess() {
+
+listCapabilitiesCmd._mgr = mgr;
+listCapabilitiesCmd._responseGenerator = responseGenerator;
+
+Map<String, Object> result = new HashMap<String, Object>();
--- End diff --
Please consider using ``Collections.emptyMap()`` instead of allocating a
new ``Map`` to improve clarity of intention.
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034175
--- Diff: ui/scripts/system.js ---
@@ -7851,9 +7851,14 @@
data: zoneObjs
});
}
-});
+ });
+},
+actionPreFilter: function(args){
+if (g_enablemetricsui)
+return ['add', 'viewMetrics'];
+else
+return ['add'];
--- End diff --
Please remove the duplication in this function by refactoring as follows:
```
actions = ['add'];
if (g_enablemetricsui) {
actions.append('viewMetrics');
}
return actions;
```
---
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 #1884: CLOUDSTACK-9699: Add global setting for enabl...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1884#discussion_r95034635
--- Diff: ui/scripts/ui/widgets/listView.js ---
@@ -1922,7 +1922,14 @@
// List view header actions
if (listViewData.actions) {
+// If a preFilter is set, then get the values. Else set this
to null
+var filteredActions = listViewData.actionPreFilter ==
undefined ?
+null : listViewData.actionPreFilter();
$.each(listViewData.actions, function(actionName, action) {
+// filter out those actions that shouldn't be shown
+if (filteredActions != null
+&& (filteredActions.indexOf(actionName) < 0))
+return false;
--- End diff --
Per coding standards, please surround all `if` blocks with curly braces.
---
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 issue #1763: CLOUDSTACK-9594: API "list templates templatefilter=...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1763 @sudhansu7 could you please create a Marvin test case with the tests outlined in your [comment](https://github.com/apache/cloudstack/pull/1763#issuecomment-265221113)? When it is available, it can be run along with the regression test suite on blueorangutan to complete testing. --- 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 issue #1804: CLOUDSTACK-9639: Unable to create shared network wit...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1804 @rhtyd looks like the Travis build is failing due to a timeout. Do you have any ideas what could be causing this timeout? @nitin-maharana are their existing Marvin tests that verify this fix? --- 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 issue #1579: CLOUDSTACK-9403 : Support for shared networks in Nua...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1579 @murali-reddy @rhtyd is there an ETA on the `test_create_volume_under_domain` fix? The current Travis build is failing on the following test cases: * `test_volumes` * `test_vpc_network` * `test_vpc_offerings` I would like to understand the root cause of these failures before merging. --- 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 issue #1797: CLOUDSTACK-9630: Cannot use listNics API as advertis...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1797 @sudhansu7 could you please either add or update an existing a Marvin test case to verify this change? Also, this change seems like it would be useful for LTS users. Could you please change the base branch to 4.9? --- 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 #873: CLOUDSTACK-8896: allocated percentage of stora...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/873#discussion_r90589284
--- Diff: server/src/com/cloud/storage/StorageManagerImpl.java ---
@@ -1719,6 +1719,7 @@ public boolean storagePoolHasEnoughSpace(List
volumes, StoragePool pool,
}
// allocated space includes templates
+s_logger.debug("Destination pool id: " + pool.getId());
--- End diff --
Please wrap this `DEBUG` log in an `if (s_logger.isDebugEnabled)` check to
prevent unnecessary/expensive string concatenation when `DEBUG` logging is not
enabled. Also, please add context to message to explain what operation is
being performed for the destination pool.
---
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 #873: CLOUDSTACK-8896: allocated percentage of stora...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/873#discussion_r90589240
--- Diff: server/src/com/cloud/storage/StorageManagerImpl.java ---
@@ -1746,10 +1747,10 @@ public boolean
storagePoolHasEnoughSpace(List volumes, StoragePool pool,
allocatedSizeWithTemplate =
_capacityMgr.getAllocatedPoolCapacity(poolVO, tmpl);
}
}
-
-if (volumeVO.getState() != Volume.State.Ready) {
-totalAskingSize +=
getDataObjectSizeIncludingHypervisorSnapshotReserve(volumeVO, pool);
-
+// A ready state volume is already allocated in a pool. so the
asking size is zero for it.
+// In case the volume is moving across pools or is not ready
yet, the asking size has to be computed
+s_logger.debug("pool id for the volume with id: " +
volumeVO.getId() + " is: " + volumeVO.getPoolId());
--- End diff --
Please wrap this `DEBUG` log in an `if (s_logger.isDebugEnabled)` check to
prevent unnecessary/expensive string concatenation when `DEBUG` logging is not
enabled.
Minor nit: grammatically, the `:` character after `is` is unnecessary.
---
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 issue #1802: CLOUDSTACK-9635: fix test_privategw_acl.py
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1802 Coupled with @rhtyd's explanation, we can merge this PR if the current blueorganutan run comes up clean. --- 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 issue #1765: Cloudstack 9586: When using local storage with Xense...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1765 @abhinandanprateek could you please rebase this PR to pick up the fixes to the broken tests? I want to make sure that this PR doesn't introduce any side-effects that break the test runs. --- 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 issue #1802: CLOUDSTACK-9635: fix test_privategw_acl.py
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1802 @blueorangutan test matrix --- 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 issue #1802: CLOUDSTACK-9635: fix test_privategw_acl.py
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1802 @murali-reddy @rhtyd can you investigate the Travis failures? --- 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 issue #1763: CLOUDSTACK-9594: API "list templates templatefilter=...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1763 @rhtyd can you investigate why the fix for this issue in 4.5 was not pulled forward? For traceability purposes, it would be preferable to forward merge the fix than commit a new version of it. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1786: CLOUDSTACK-9618: Load Balancer configuration ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1786#discussion_r90487461
--- Diff:
plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java
---
@@ -260,7 +264,7 @@ public boolean applyLBRules(Network config,
List rules) throw
Map<Capability, String> lbCapabilities = new HashMap<Capability,
String>();
// Specifies that the RoundRobin and Leastconn algorithms are
supported for load balancing rules
-lbCapabilities.put(Capability.SupportedLBAlgorithms,
"roundrobin,leastconn");
+lbCapabilities.put(Capability.SupportedLBAlgorithms,
"roundrobin,leastconn,source");
--- End diff --
The list of strings declared on this line must be sync with the list used
to validate the algorithm on line 237. Therefore, please consider extracting
this list to a constant `ImmutableSet`, `SUPPORTED_ALGORITHMS` in order
to ensure cohesion between declaration and validation. With this change, the
validateLBRule can be re-implemented as follows:
```java
public boolean validateLBRule(Network network, LoadBalancingRule rule) {
Preconditions.checkArgument(network != null, "validateLBRule requires a
non-null network");
Preconditions.checkArgument(rule != null, "validateLBRule requires a
non-null rule");
return (canHandle(network, Service.Lb)) :
SUPPORTED_ALGORITHMS.contains(rule.getAlgorithm()) : true;
}
```
This line can be re-implemented as follows:
```java
lbCapabilities.put(Capability.SupportedLBAlgorithms,
Joiner.on(",").join(SUPPORTED_ALGORITHMS));
```
---
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 issue #1776: CLOUDSTACK-9603: concurrent.snapshots.threshold.perh...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1776 @priyankparihar could you please provide further explanation as to how this fix addresses the issue of `concurrent.snapshots.threshold.perhost` not being validated? Also, is there an existing Marvin test case and/or unit test that verifies this fix? If not, could you please add one? --- 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 #1773: CLOUDSTACK-9607: Preventing template deletion...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1773#discussion_r90480678
--- Diff: server/src/com/cloud/template/TemplateManagerImpl.java ---
@@ -1176,6 +1176,23 @@ public boolean deleteTemplate(DeleteTemplateCmd cmd)
{
throw new InvalidParameterValueException("unable to find
template with id " + templateId);
}
+List vmInstanceVOList;
+if(cmd.getZoneId() != null) {
+vmInstanceVOList =
_vmInstanceDao.listNonExpungedByZoneAndTemplate(cmd.getZoneId(), templateId);
+}
+else {
+vmInstanceVOList =
_vmInstanceDao.listNonExpungedByTemplate(templateId);
+}
+if(!cmd.isForced() &&
CollectionUtils.isNotEmpty(vmInstanceVOList)) {
+StringBuilder s = new StringBuilder("Unable to delete
template with id: " + templateId + " because some VM instances are using it. ");
+for (VMInstanceVO elm : vmInstanceVOList) {
+s.append(elm.getInstanceName() + ", ");
+}
+
+s_logger.warn(s.substring(0,s.length()-2));
+throw new
InvalidParameterValueException(s.substring(0,s.length()-2));
--- End diff --
Lines 1187-1193 should replaced with the following to be DRY and improve
clarity:
```java
final String message = String.format("Unable to delete template with id:
%1$s because VM instances: [%2$s] are using it.", templateId,
Joiner.on(",").join(vmInstanceVOList));
s_logger.warn(message);
throw new InvalidParameterValueException(message);
```
---
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 #1773: CLOUDSTACK-9607: Preventing template deletion...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1773#discussion_r90481980 --- Diff: api/src/org/apache/cloudstack/api/command/user/template/DeleteTemplateCmd.java --- @@ -52,6 +52,9 @@ @Parameter(name = ApiConstants.ZONE_ID, type = CommandType.UUID, entityType = ZoneResponse.class, description = "the ID of zone of the template") private Long zoneId; +@Parameter(name = ApiConstants.FORCED, type = CommandType.BOOLEAN, required = false, description = "Force delete a template.") --- End diff -- Please add `since` to this annotation to indicate that this parameter is available in 4.9+. --- 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 issue #1802: CLOUDSTACK-9635: fix test_privategw_acl.py
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1802 @murali-reddy agreed that it is extremely unlikely that the code change impacts that test case. However, we know that a failure to cleanup between tests can cause failures when they are run together. Therefore, I would like to see this fix passing with #1801 to ensure we don't have one of those transitive 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 issue #1799: CLOUDSTACK-9632: Upgrade bouncy castle to version 1....
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1799 @rhtyd the Travis build failed due a timeout on one of the workers. Could you please do a force push to trigger a new build? --- 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 #1804: CLOUDSTACK-9639: Unable to create shared netw...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1804#discussion_r90476049
--- Diff: server/src/com/cloud/configuration/ConfigurationManagerImpl.java
---
@@ -3092,8 +3092,12 @@ public Vlan createVlanAndPublicIpRange(final long
zoneId, final long networkId,
final String guestNetworkCidr = zone.getGuestNetworkCidr();
if (guestNetworkCidr != null) {
if (NetUtils.isNetworksOverlap(newCidr, guestNetworkCidr))
{
-throw new InvalidParameterValueException("The new IP
range you have specified has overlapped with the guest network in zone: " +
zone.getName()
-+ ". Please specify a different
gateway/netmask.");
+// when adding shared network with same cidr of zone
guest cidr,
+// if the specified vlan is not present in zone,
physical network, allow to create the network as the isolation is based on VLAN.
+if (_zoneDao.findVnet(zoneId, physicalNetworkId,
vlanId).size() > 0) {
--- End diff --
Please consider using `isEmpty` rather than a size check to determine
whether or not a list contains elements. It is more idiomatic/clear, but less
error prone. Also, why isn't this `if` condition combined with the previous
`if` block and an `&&` operator.
---
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 issue #1435: Dockerfile4.9
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1435 @pdion891 this [blog](https://ariejan.net/2011/07/05/git-squash-your-latests-commits-into-one/) is good description of squash process. --- 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 issue #1802: CLOUDSTACK-9635: fix test_privategw_acl.py
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1802 @murali-reddy I see failures in the `test_router_dhcp_opts` test case. Was this PR rebased to get the fix from #1801? If not, I would like to rebase and re-run the tests. Otherwise, we may have a regression to investigate. --- 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 issue #1659: CLOUDSTACK-9339 Virtual Routers don't handle Multipl...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1659 @blueorangutan test centos7 xenserver-65sp1 --- 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 issue #1659: CLOUDSTACK-9339 Virtual Routers don't handle Multipl...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1659 Tests look good on KVM. However, it seems like a Good Thing(tm) to test on KVM and XenServer as well. @blueorangutan test centos7 vmware-55u3 @blueorangutan test centos7 xenserver-65sp1 --- 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 issue #1675: CLOUDSTACK-9453: WIP
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1675 @abhinandanprateek ping re: closing this PR in favor of #1639 --- 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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90457477
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90454905
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspRequestWrapper.java
---
@@ -0,0 +1,78 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.Hashtable;
+import java.util.Set;
+
+import org.reflections.Reflections;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.Command;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.RequestWrapper;
+import com.cloud.resource.ServerResource;
+
+public class NuageVspRequestWrapper extends RequestWrapper {
+
+private static NuageVspRequestWrapper instance;
+
+static {
+instance = new NuageVspRequestWrapper();
+}
+
+Reflections baseWrappers = new
Reflections("com.cloud.network.vsp.resource.wrapper");
--- End diff --
Why isn't this attribute declared `private static final`? My understanding
of reflections, it only needs to instantiated once to scan a package path. It
seems unnecessary/expensive to spin it up every time `NuageVspRequestWrapper`
is instantiated.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90455598
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90452488
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
---
@@ -255,7 +284,30 @@ public void reserve(NicProfile nic, Network network,
VirtualMachineProfile vm, D
}
HostVO nuageVspHost =
_nuageVspManager.getNuageVspHost(network.getPhysicalNetworkId());
-VspNetwork vspNetwork =
_nuageVspEntityBuilder.buildVspNetwork(network, false);
+VspNetwork vspNetwork =
_nuageVspEntityBuilder.buildVspNetwork(vm.getVirtualMachine().getDomainId(),
network);
+
+if (vspNetwork.isShared()) {
+vspNetwork =
_nuageVspEntityBuilder.updateVspNetworkByPublicIp(vspNetwork, network,
nic.getIPv4Address());
+
+if (VirtualMachine.Type.DomainRouter.equals(vm.getType())
&& !nic.getIPv4Address().equals(vspNetwork.getVirtualRouterIp())) {
+s_logger.debug("VR got spawned with a different IP,
releasing the previously allocated public IP " + nic.getIPv4Address());
--- End diff --
Please wrap this `DEBUG` log statement in an `if
(s_logger.isDebugEnabled())` block to avoid unnecessary string concatenation
when `DEBUG` logging is not enabled.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90456898
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90451456 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/UpdateNuageVspDeviceCommand.java --- @@ -0,0 +1,43 @@ +// +// 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 com.cloud.agent.api.manager; --- End diff -- This class appears to be Nuage specific. Why is is being added to core CloudStack and not the Nuage plugin? --- 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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90452834
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
---
@@ -317,20 +368,28 @@ private void updateDhcpOptionsForExistingVms(Network
network, HostVO nuageVspHos
if(s_logger.isDebugEnabled()) {
s_logger.debug(String.format("DomainRouter is added to an
existing network: %s in state: %s", network.getName(), network.getState()));
}
-List dhcpOptions = Lists.newLinkedList();
-for (NicVO userNic :_nicDao.listByNetworkId(network.getId())) {
-if (userNic.getVmType() != VirtualMachine.Type.DomainRouter) {
-boolean defaultHasDns =
getDefaultHasDns(networkHasDnsCache, userNic);
-
dhcpOptions.add(_nuageVspEntityBuilder.buildVmDhcpOption(userNic,
defaultHasDns, networkHasDns));
-}
+
+List userNics = _nicDao.listByNetworkId(network.getId());
+LinkedListMultimap<Long, VspDhcpVMOption> dhcpOptionsPerDomain =
LinkedListMultimap.create();
+
+for (NicVO userNic : userNics) {
+if (userNic.getVmType() == VirtualMachine.Type.DomainRouter)
continue;
--- End diff --
Per our coding standards, all `if` blocks must be wrapped in curly braces.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90451924
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java
---
@@ -387,41 +400,39 @@ public boolean canEnableIndividualServices() {
@Override
public boolean destroy(Network network, ReservationContext context)
throws ConcurrentOperationException, ResourceUnavailableException {
-if (!canHandle(network, Service.Connectivity)) {
-return false;
-}
-
-return true;
+return canHandle(network, Service.Connectivity);
}
@Override
public boolean verifyServicesCombination(Set services) {
--- End diff --
The method assumes that `services` is not `null`. Therefore, please
consider adding a `Preconditions.checkArgument` check to verify that `services`
is not `null`
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90450632 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java --- @@ -128,6 +135,26 @@ private static final Map<Service, Map<Capability, String>> capabilities = setCapabilities(); +private static final Set REQUIRED_SERVICES = Sets.newHashSet( +Service.Connectivity, +Service.Dhcp +); +private static final Set NUAGE_ONLY_SERVICES = Sets.newHashSet( +Service.SourceNat, +Service.StaticNat, +Service.Gateway +); +private static final Set UNSUPPORTED_SERVICES = Sets.newHashSet( --- End diff -- Since this value is constant, please use ``ImmutableSet`` rather than ``HashSet``. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90455612
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90450815
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/CleanUpDomainCommand.java
---
@@ -0,0 +1,63 @@
+//
+// 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 com.cloud.agent.api.manager;
+
+import com.cloud.agent.api.Command;
+import net.nuage.vsp.acs.client.api.model.VspDomainCleanUp;
+
+public class CleanUpDomainCommand extends Command {
+
+private final VspDomainCleanUp _domainCleanUp;
+
+public CleanUpDomainCommand(VspDomainCleanUp domainCleanUp) {
+super();
+this._domainCleanUp = domainCleanUp;
+}
+
+public VspDomainCleanUp getDomainCleanUp() {
+return _domainCleanUp;
+}
+
+@Override
+public boolean executeInSequence() {
+return false;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) return true;
+if (!(o instanceof CleanUpDomainCommand)) return false;
+if (!super.equals(o)) return false;
+
+CleanUpDomainCommand that = (CleanUpDomainCommand) o;
+
+if (_domainCleanUp != null ?
!_domainCleanUp.equals(that._domainCleanUp) : that._domainCleanUp != null)
--- End diff --
Per our coding standards, all `if` blocks must wrapped in curly braces.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90455401 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java --- @@ -0,0 +1,310 @@ +// +// 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 com.cloud.network.resource; --- End diff -- Why is package namespaced to a core package when it is part of the Nuage package? From a reporting/logging perspective, it makes finding the code less intuitive. --- 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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90455021
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspRequestWrapper.java
---
@@ -0,0 +1,78 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.Hashtable;
+import java.util.Set;
+
+import org.reflections.Reflections;
+
+import com.cloud.agent.api.Answer;
+import com.cloud.agent.api.Command;
+import com.cloud.resource.CommandWrapper;
+import com.cloud.resource.RequestWrapper;
+import com.cloud.resource.ServerResource;
+
+public class NuageVspRequestWrapper extends RequestWrapper {
+
+private static NuageVspRequestWrapper instance;
+
+static {
+instance = new NuageVspRequestWrapper();
+}
+
+Reflections baseWrappers = new
Reflections("com.cloud.network.vsp.resource.wrapper");
+
+@SuppressWarnings("rawtypes")
+Set<Class> baseSet =
baseWrappers.getSubTypesOf(CommandWrapper.class);
--- End diff --
Per previous comment, this attribute seems like it could be `private static
final`.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90450669 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java --- @@ -128,6 +135,26 @@ private static final Map<Service, Map<Capability, String>> capabilities = setCapabilities(); +private static final Set REQUIRED_SERVICES = Sets.newHashSet( +Service.Connectivity, +Service.Dhcp +); +private static final Set NUAGE_ONLY_SERVICES = Sets.newHashSet( +Service.SourceNat, +Service.StaticNat, +Service.Gateway +); +private static final Set UNSUPPORTED_SERVICES = Sets.newHashSet( +Service.Vpn, +Service.Dns, +Service.PortForwarding, +Service.SecurityGroup +); +private static final Set<Pair<Service, Service>> ANY_REQUIRED_SERVICES = Sets.newHashSet( --- End diff -- Since this value is constant, please use ``ImmutableSet`` rather than ``HashSet``. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90450606 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java --- @@ -128,6 +135,26 @@ private static final Map<Service, Map<Capability, String>> capabilities = setCapabilities(); +private static final Set REQUIRED_SERVICES = Sets.newHashSet( +Service.Connectivity, +Service.Dhcp +); +private static final Set NUAGE_ONLY_SERVICES = Sets.newHashSet( --- End diff -- Since this value is constant, please use ``ImmutableSet`` rather than ``HashSet``. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90451522
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/UpdateNuageVspDeviceCommand.java
---
@@ -0,0 +1,43 @@
+//
+// 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 com.cloud.agent.api.manager;
+
+import com.cloud.agent.api.Command;
+import com.cloud.network.resource.NuageVspResourceConfiguration;
+
+public class UpdateNuageVspDeviceCommand extends Command {
--- End diff --
Please add `equals`, `hashCode`, and `toString` implementations.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90453536
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
---
@@ -339,34 +398,63 @@ private void updateDhcpOptionsForExistingVms(Network
network, HostVO nuageVspHos
}
}
+private void checkMultipleSubnetsCombinedWithUseData(Network network){
+if
(_ntwkOfferingSrvcDao.listServicesForNetworkOffering(network.getNetworkOfferingId()).contains(Network.Service.UserData.getName())
&& _vlanDao.listVlansByNetworkId(network.getId()).size() > 1) {
+List vlanVOs =
_vlanDao.listVlansByNetworkId(network.getId());
+for(VlanVO vlanVoItem : vlanVOs){
+for(VlanVO VlanVoItem2 : vlanVOs){
+if(!vlanVoItem.equals(VlanVoItem2) &&
!VlanVoItem2.getVlanGateway().equals(vlanVoItem.getVlanGateway())){
+s_logger.error("NuageVsp provider does not support
multiple subnets in combination with user data.");
--- End diff --
Please add identifying information of the duplicated vlan/subnet and
network to the error message to assist with forensic debugging.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90458374
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
--- End diff --
What is the purpose of this class? It seems that a Map<String, String>
with a static validation function on `VspHostBuilder` would accomplish the same
thing with less code and higher cohesion.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90453232
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java
---
@@ -339,34 +398,63 @@ private void updateDhcpOptionsForExistingVms(Network
network, HostVO nuageVspHos
}
}
+private void checkMultipleSubnetsCombinedWithUseData(Network network){
+if
(_ntwkOfferingSrvcDao.listServicesForNetworkOffering(network.getNetworkOfferingId()).contains(Network.Service.UserData.getName())
&& _vlanDao.listVlansByNetworkId(network.getId()).size() > 1) {
--- End diff --
Please consider using `isEmpty` rather than a size check to determine
whether or not a list has elements. It is more idiomatic/expressive and less
error-prone.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90451818
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java
---
@@ -387,41 +400,39 @@ public boolean canEnableIndividualServices() {
@Override
public boolean destroy(Network network, ReservationContext context)
throws ConcurrentOperationException, ResourceUnavailableException {
-if (!canHandle(network, Service.Connectivity)) {
-return false;
-}
-
-return true;
+return canHandle(network, Service.Connectivity);
}
@Override
public boolean verifyServicesCombination(Set services) {
-// This element can only function in a NuageVsp based
-// SDN network, so Connectivity needs to be present here
-if (!services.contains(Service.Connectivity)) {
-s_logger.warn("Unable to support services combination without
Connectivity service provided by Nuage VSP.");
-return false;
+final Sets.SetView missingServices =
Sets.difference(REQUIRED_SERVICES, services);
+final Sets.SetView unsupportedServices =
Sets.intersection(UNSUPPORTED_SERVICES, services);
+final Sets.SetView wantedServices =
Sets.intersection(NUAGE_ONLY_SERVICES, new HashSet<>());
+
+if (!missingServices.isEmpty()) {
+throw new UnsupportedServiceException("Provider " +
Provider.NuageVsp + " requires services: " + missingServices);
}
-if (!services.contains(Service.SourceNat)) {
-s_logger.warn("Unable to support services combination without
SourceNat service provided by Nuage VSP.");
+if (!unsupportedServices.isEmpty()) {
+// NuageVsp doesn't implement any of these services.
+// So if these services are requested, we can't handle it.
+s_logger.debug("Unable to support services combination. The
services " + unsupportedServices + " are not supported by Nuage VSP.");
--- End diff --
Should this message logged as at `WARN` instead of `DEBUG`?
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90451387
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/UpdateNuageVspDeviceCommand.java
---
@@ -0,0 +1,43 @@
+//
+// 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 com.cloud.agent.api.manager;
+
+import com.cloud.agent.api.Command;
+import com.cloud.network.resource.NuageVspResourceConfiguration;
+
+public class UpdateNuageVspDeviceCommand extends Command {
+
+NuageVspResourceConfiguration configuration;
--- End diff --
Why isn't this attribute declared `private`? Also, could it be declared
`final`?
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90457155
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90457877
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/network/resource/NuageVspResourceConfiguration.java
---
@@ -0,0 +1,310 @@
+//
+// 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 com.cloud.network.resource;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.naming.ConfigurationException;
+
+import net.nuage.vsp.acs.client.api.model.NuageVspUser;
+import net.nuage.vsp.acs.client.api.model.VspHost;
+import net.nuage.vsp.acs.client.common.NuageVspApiVersion;
+
+import org.apache.commons.lang.builder.ToStringBuilder;
+
+import com.cloud.util.NuageVspUtil;
+
+public class NuageVspResourceConfiguration {
+private static final String NAME = "name";
+private static final String GUID = "guid";
+private static final String ZONE_ID = "zoneid";
+private static final String HOST_NAME = "hostname";
+private static final String CMS_USER = "cmsuser";
+private static final String CMS_USER_PASSWORD = "cmsuserpass";
+private static final String PORT = "port";
+private static final String API_VERSION = "apiversion";
+private static final String API_RELATIVE_PATH = "apirelativepath";
+private static final String RETRY_COUNT = "retrycount";
+private static final String RETRY_INTERVAL = "retryinterval";
+private static final String NUAGE_VSP_CMS_ID = "nuagevspcmsid";
+
+private static final String CMS_USER_ENTEPRISE_NAME = "CSP";
+
+private String _name;
+private String _guid;
+private String _zoneId;
+private String _hostName;
+private String _cmsUser;
+private String _cmsUserPassword;
+private String _port;
+private String _apiVersion;
+private String _apiRelativePath;
+private String _retryCount;
+private String _retryInterval;
+private String _nuageVspCmsId;
+
+public String name() {
+return _name;
+}
+
+public String guid() {
+return this._guid;
+}
+
+public NuageVspResourceConfiguration guid(String guid) {
+this._guid = guid;
+return this;
+}
+
+public String zoneId() {
+return this._zoneId;
+}
+
+public NuageVspResourceConfiguration zoneId(String zoneId) {
+this._zoneId = zoneId;
+return this;
+}
+
+public String hostName() {
+return this._hostName;
+}
+
+public NuageVspResourceConfiguration hostName(String hostName) {
+this._hostName = hostName;
+this._name = "Nuage VSD - " + _hostName;
+return this;
+}
+
+public String cmsUser() {
+return this._cmsUser;
+}
+
+public NuageVspResourceConfiguration cmsUser(String cmsUser) {
+this._cmsUser = cmsUser;
+return this;
+}
+
+public String cmsUserPassword() {
+return this._cmsUserPassword;
+}
+
+public NuageVspResourceConfiguration cmsUserPassword(String
cmsUserPassword) {
+this._cmsUserPassword = cmsUserPassword;
+return this;
+}
+
+public String port() {
+return this._port;
+}
+
+public NuageVspResourceConfiguration port(String port) {
+this._port = port;
+return this;
+}
+
+public String apiVersion() {
+return this._apiVersion;
+}
+
+public NuageVspResourceConfiguration apiVersion(String apiVersion) {
+this._apiVersion = apiVersion;
+return this;
+}
+
+pu
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90451135
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/CleanUpDomainCommand.java
---
@@ -0,0 +1,63 @@
+//
+// 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 com.cloud.agent.api.manager;
+
+import com.cloud.agent.api.Command;
+import net.nuage.vsp.acs.client.api.model.VspDomainCleanUp;
+
+public class CleanUpDomainCommand extends Command {
+
+private final VspDomainCleanUp _domainCleanUp;
+
+public CleanUpDomainCommand(VspDomainCleanUp domainCleanUp) {
--- End diff --
Is it acceptable for `domainCleanup` to be `null`? If not, please consider
adding a `Preconditions.checkArgument` check to verify that the `domainCleanup`
is not `null`. You may also be to remove `null` checks from the `equals` and
`hashCode` methods if this precondition is added.
---
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 #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1579#discussion_r90450578 --- Diff: plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java --- @@ -128,6 +135,26 @@ private static final Map<Service, Map<Capability, String>> capabilities = setCapabilities(); +private static final Set REQUIRED_SERVICES = Sets.newHashSet( --- End diff -- Since this value is constant, please use ``ImmutableSet`` rather than ``HashSet``. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1579: CLOUDSTACK-9403 : Support for shared networks...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1579#discussion_r90450802
--- Diff:
plugins/network-elements/nuage-vsp/src/com/cloud/agent/api/manager/CleanUpDomainCommand.java
---
@@ -0,0 +1,63 @@
+//
+// 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 com.cloud.agent.api.manager;
+
+import com.cloud.agent.api.Command;
+import net.nuage.vsp.acs.client.api.model.VspDomainCleanUp;
+
+public class CleanUpDomainCommand extends Command {
+
+private final VspDomainCleanUp _domainCleanUp;
+
+public CleanUpDomainCommand(VspDomainCleanUp domainCleanUp) {
+super();
+this._domainCleanUp = domainCleanUp;
+}
+
+public VspDomainCleanUp getDomainCleanUp() {
+return _domainCleanUp;
+}
+
+@Override
+public boolean executeInSequence() {
+return false;
+}
+
+@Override
+public boolean equals(Object o) {
+if (this == o) return true;
+if (!(o instanceof CleanUpDomainCommand)) return false;
+if (!super.equals(o)) return false;
--- End diff --
Per our coding standards, all `if` blocks must wrapped in curly braces.
---
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 issue #1435: Dockerfile4.9
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1435 @rhtyd agreed. I apologize for being unclear -- I would like to get this PR into 4.9.1.0, but it is not a release blocker. --- 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 issue #1579: CLOUDSTACK-9403 : Support for shared networks in Nua...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1579 @prashanthvarma agreed regarding the most common causes of failures. @murali-reddy @borisstoyanov and @abhinandanprateek have been working to address these issues as they are encountered. To avoid timeouts, I suggest using the `utils.wait_until` when you have to wait for an operation to complete. Please feel free to submit fixes to test cases that you see failing. --- 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 issue #1435: Dockerfile4.9
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1435 @PaulAngus @rhtyd what are your thoughts on automating the testing of the Docker container? --- 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 issue #1435: Dockerfile4.9
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1435 @pdion891 can you please investigate the Jenkins failure? --- 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 issue #1435: Dockerfile4.9
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1435 @pdion891 if possible, I would like to get this merged for 4.9.1.0. How does this PR relate to #1789? Also, could you please investigate the Jenkins failure and create JIRA ticket for this 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 issue #1784: CS-505: Marvin test to check VR internal DNS Service
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1784 @murali-reddy is internal DNS service impacted by having multiple NICs? --- 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 #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380293
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+cls.services[&qu
[GitHub] cloudstack pull request #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380262
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+
[GitHub] cloudstack pull request #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380120
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+
[GitHub] cloudstack pull request #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380926
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+
[GitHub] cloudstack pull request #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380579
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+
[GitHub] cloudstack pull request #1784: CS-505: Marvin test to check VR internal DNS ...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1784#discussion_r90380041
--- Diff: test/integration/smoke/test_router_dnsservice.py ---
@@ -0,0 +1,268 @@
+# 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 logging
+import dns.resolver
+
+from nose.plugins.attrib import attr
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.lib.utils import cleanup_resources
+from marvin.lib.base import (ServiceOffering,
+ VirtualMachine,
+ Account,
+ NATRule,
+ FireWallRule,
+ NetworkOffering,
+ Network)
+from marvin.lib.common import (get_zone,
+ get_template,
+ get_domain,
+ list_routers,
+ list_nat_rules,
+ list_publicIP)
+
+
+class TestRouterDnsService(cloudstackTestCase):
+
+@classmethod
+def setUpClass(cls):
+cls.logger = logging.getLogger('TestRouterDnsService')
+cls.stream_handler = logging.StreamHandler()
+cls.logger.setLevel(logging.DEBUG)
+cls.logger.addHandler(cls.stream_handler)
+
+cls.testClient = super(TestRouterDnsService,
cls).getClsTestClient()
+cls.api_client = cls.testClient.getApiClient()
+cls.services = cls.testClient.getParsedTestDataConfig()
+
+cls.domain = get_domain(cls.api_client)
+cls.zone = get_zone(cls.api_client,
cls.testClient.getZoneForTests())
+cls.services['mode'] = cls.zone.networktype
+cls.template = get_template(
+cls.api_client,
+cls.zone.id,
+cls.services["ostype"]
+)
+cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+cls.logger.debug("Creating Admin Account for domain %s on zone %s"
% (cls.domain.id, cls.zone.id))
+cls.account = Account.create(
+cls.api_client,
+cls.services["account"],
+admin=True,
+domainid=cls.domain.id
+)
+
+cls.logger.debug("Creating Service Offering on zone %s" %
(cls.zone.id))
+cls.service_offering = ServiceOffering.create(
+cls.api_client,
+cls.services["service_offering"]
+)
+
+cls.logger.debug("Creating Network Offering on zone %s" %
(cls.zone.id))
+cls.services["isolated_network_offering"]["egress_policy"] = "true"
+cls.network_offering = NetworkOffering.create(cls.api_client,
+
cls.services["isolated_network_offering"],
+ conservemode=True)
+cls.network_offering.update(cls.api_client, state='Enabled')
+
+cls.logger.debug("Creating Network for Account %s using offering
%s" % (cls.account.name, cls.network_offering.id))
+cls.network = Network.create(cls.api_client,
+ cls.services["network"],
+ accountid=cls.account.name,
+ domainid=cls.account.domainid,
+
networkofferingid=cls.network_offering.id,
+ zoneid=cls.zone.id)
+
+cls.logger.debug("Creating guest VM for Account %s using offering
%s" % (cls.account.name, cls.service_offering.id))
+cls.services["virtual_machine"]["displayname"] = 'drump';
+
[GitHub] cloudstack issue #1798: CLOUDSTACK-9631: API: affinitygroupids or affinitygr...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1798 @marcaurele there appear to be Travis failures. Could you please check into them? --- 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 #1798: CLOUDSTACK-9631: API: affinitygroupids or aff...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1798#discussion_r90372709
--- Diff:
api/src/org/apache/cloudstack/api/command/user/affinitygroup/UpdateVMAffinityGroupCmd.java
---
@@ -96,6 +96,10 @@ public Long getId() {
throw new InvalidParameterValueException("affinitygroupids
parameter is mutually exclusive with affinitygroupnames parameter");
}
+if (affinityGroupNameList == null && affinityGroupIdList == null) {
+throw new InvalidParameterValueException("affinitygroupids
parameter or affinitygroupnames parameter must be given");
+}
+
--- End diff --
Lines 95-102 should be at the beginning of the `execute` method rather than
in an accessor.
---
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 issue #1711: XenServer 7 Support
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1711 @syed yes, you will add the changes to `schema-4910to4920` for the `4.9` port. This will cover anyone upgrading from a version >= 4.9.1.0 to 4.9.2.0. I do not plan to automatically forward merge these changes. Instead, you will be submitting another PR for the 4.10.1.0 and master merges which will provide the opportunity to re-test change in each branch. Therefore, to cover 4.10.0.0 users who upgrade, the most straightforward approach seems to be duplicating the changes in `schema-4910to4920` into `schema-41000-41100`. We need to ensure that the script is idempotent to cover users who upgrade from 4.9.2.0->4.11.0.0+. Does this approach make sense? --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90342302
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90342459
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack issue #1803: CLOUDSTACK-9636: The host alerts box should be named...
Github user jburwell commented on the issue: https://github.com/apache/cloudstack/pull/1803 @nitin-maharana could you please add screen shots of the change to the 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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90341780
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90340642
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
--- End diff --
What is the purpose of these two lines? Information is being retrieved,
but not used.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90341157
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
--- End diff --
Please consider consolidating lines 434-439 into a multi-catch. Also,
`IllegalArgumentException` seems inappropriate as it indicates that the
preconditions of the method were violated. `IllegalStateException` seems more
appropriate.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90341463
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90341553
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90340513
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
--- End diff --
Please add a check for the return of the `add` method to ensure that the
element was added to `anchors`.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90341649
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO
cert, List chain, Certificate cert) {
+private void validateChain(final List chain, final
Certificate cert) {
-List certs = new ArrayList();
-Set anchors = new HashSet();
+final List certs = new ArrayList();
+final Set anchors = new HashSet();
certs.add(cert); // adding for self signed certs
certs.addAll(chain);
-for (Certificate c : certs) {
-if (!(c instanceof X509Certificate))
+for (final Certificate c : certs) {
+if (!(c instanceof X509Certificate)) {
throw new IllegalArgumentException("Invalid chain format.
Expected X509 certificate");
+}
-X509Certificate xCert = (X509Certificate)c;
+final X509Certificate xCert = (X509Certificate)c;
-Principal subject = xCert.getSubjectDN();
-Principal issuer = xCert.getIssuerDN();
+xCert.getSubjectDN();
+xCert.getIssuerDN();
- anchors.add(new TrustAnchor(xCert, null));
+anchors.add(new TrustAnchor(xCert, null));
}
-X509CertSelector target = new X509CertSelector();
+final X509CertSelector target = new X509CertSelector();
target.setCertificate((X509Certificate)cert);
PKIXBuilderParameters params = null;
try {
params = new PKIXBuilderParameters(anchors, target);
params.setRevocationEnabled(false);
params.addCertStore(CertStore.getInstance("Collection", new
CollectionCertStoreParameters(certs)));
-CertPathBuilder builder = CertPathBuilder.getInstance("PKIX",
"BC");
+final CertPathBuilder builder =
CertPathBuilder.getInstance("PKIX", "BC");
builder.build(params);
-} catch (InvalidAlgorithmParameterException e) {
+} catch (final InvalidAlgorithmParameterException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (CertPathBuilderException e) {
+} catch (final CertPathBuilderException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchAlgorithmException e) {
+} catch (final NoSuchAlgorithmException e) {
throw new IllegalArgumentException("Invalid certificate
chain", e);
-} catch (NoSuchProviderException e) {
+} catch (final NoSuchProviderException e) {
throw new CloudRuntimeException("No provider for certificate
validation", e);
}
}
-public PrivateKey parsePrivateKey(String key, String password) throws
IOException {
-
-PasswordFinder pGet = null;
-
-if (password != null)
-pGet = new KeyPassword(password.toCharArray());
-
-PEMReader privateKey = new PEMReader(new StringReader(key), pGet);
-Object obj = null;
-try {
-obj = privateKey.readObject();
-} finally {
-IOUtils.closeQuietly(privateKey);
-}
-
-try {
-
-if (obj instanceof KeyPair)
-return ((KeyPair)obj).getPrivate();
-
-return (PrivateKey)obj;
-
-} catch (Exception e) {
-throw new IOException("Invalid Key format or invalid
password.", e);
+public PrivateKey parsePrivateKey(final String key) throws IOException
{
+try (final PemReader pemReader = new PemReader(new
StringReader(key));) {
+final PemObject pemObject = pemReader.readPemObject();
+final byte[] content = pemObject.getContent();
+final PKCS8EncodedKeySpec privKeySpec = new
PKCS8EncodedKeySpec(content);
+final KeyFactory factory = KeyFactory.getInstance("RSA", "BC");
+return factory.generatePrivate(privKeySpec);
+} catch (NoSuchAlgorithmException | NoSuchProviderException e) {
+throw new IOException("No encryption provider available.", e);
+} catch (final InvalidKeySpecException e) {
+throw new IOException("Invalid Key format.", e);
}
}
-public Certificate parseCertificate
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1799#discussion_r90339611 --- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java --- @@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO cert, List
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90315044
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -111,37 +116,37 @@ public CertServiceImpl() {
@DB
@Override
@ActionEvent(eventType = EventTypes.EVENT_LB_CERT_UPLOAD,
eventDescription = "Uploading a certificate to cloudstack", async = false)
-public SslCertResponse uploadSslCert(UploadSslCertCmd certCmd) {
+public SslCertResponse uploadSslCert(final UploadSslCertCmd certCmd) {
--- End diff --
Please consider adding a `Preconditions.checkArgument` to check that
`certCmd` is not null.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90310924
--- Diff:
utils/src/main/java/com/cloud/utils/security/CertificateHelper.java ---
@@ -40,123 +46,122 @@
import java.util.ArrayList;
import java.util.List;
-import com.cloud.utils.exception.CloudRuntimeException;
-import org.apache.commons.codec.binary.Base64;
-
-import com.cloud.utils.Ternary;
-import org.bouncycastle.openssl.PEMReader;
-
public class CertificateHelper {
-public static byte[] buildAndSaveKeystore(String alias, String cert,
String privateKey, String storePassword) throws KeyStoreException,
CertificateException,
-NoSuchAlgorithmException, InvalidKeySpecException, IOException {
-KeyStore ks = buildKeystore(alias, cert, privateKey,
storePassword);
-
-ByteArrayOutputStream os = new ByteArrayOutputStream();
-ks.store(os, storePassword != null ? storePassword.toCharArray() :
null);
-os.close();
-return os.toByteArray();
+public static byte[] buildAndSaveKeystore(final String alias, final
String cert, final String privateKey, final String storePassword) throws
KeyStoreException, CertificateException,
+NoSuchAlgorithmException, InvalidKeySpecException, IOException {
+final KeyStore ks = buildKeystore(alias, cert, privateKey,
storePassword);
+
+try (final ByteArrayOutputStream os = new ByteArrayOutputStream())
{
+ks.store(os, storePassword != null ?
storePassword.toCharArray() : null);
+return os.toByteArray();
+}
}
-public static byte[] buildAndSaveKeystore(List<Ternary<String, String,
String>> certs, String storePassword) throws KeyStoreException,
NoSuchAlgorithmException,
-CertificateException, IOException, InvalidKeySpecException {
-KeyStore ks = KeyStore.getInstance("JKS");
+public static byte[] buildAndSaveKeystore(final List<Ternary<String,
String, String>> certs, final String storePassword) throws KeyStoreException,
NoSuchAlgorithmException,
+CertificateException, IOException, InvalidKeySpecException {
+final KeyStore ks = KeyStore.getInstance("JKS");
ks.load(null, storePassword != null ? storePassword.toCharArray()
: null);
//name,cert,key
-for (Ternary<String, String, String> cert : certs) {
+for (final Ternary<String, String, String> cert : certs) {
if (cert.third() == null) {
-Certificate c = buildCertificate(cert.second());
+final Certificate c = buildCertificate(cert.second());
ks.setCertificateEntry(cert.first(), c);
} else {
-Certificate[] c = new Certificate[certs.size()];
+final Certificate[] c = new Certificate[certs.size()];
int i = certs.size();
-for (Ternary<String, String, String> ct : certs) {
+for (final Ternary<String, String, String> ct : certs) {
c[i - 1] = buildCertificate(ct.second());
i--;
}
ks.setKeyEntry(cert.first(),
buildPrivateKey(cert.third()), storePassword != null ?
storePassword.toCharArray() : null, c);
}
}
-ByteArrayOutputStream os = new ByteArrayOutputStream();
-ks.store(os, storePassword != null ? storePassword.toCharArray() :
null);
-os.close();
-return os.toByteArray();
+try (final ByteArrayOutputStream os = new ByteArrayOutputStream())
{
+ks.store(os, storePassword != null ?
storePassword.toCharArray() : null);
+return os.toByteArray();
+}
}
-public static KeyStore loadKeystore(byte[] ksData, String
storePassword) throws KeyStoreException, CertificateException,
NoSuchAlgorithmException, IOException {
-assert (ksData != null);
-KeyStore ks = KeyStore.getInstance("JKS");
-ks.load(new ByteArrayInputStream(ksData), storePassword != null ?
storePassword.toCharArray() : null);
+public static KeyStore loadKeystore(final byte[] ksData, final String
storePassword) throws KeyStoreException, CertificateException,
NoSuchAlgorithmException, IOException {
+assert ksData != null;
+final KeyStore ks = KeyStore.getInstance("JKS");
+try (final ByteArrayInputStream is = new
ByteArrayInputStream(ksData)) {
+ks.load(is, storePassword != null ?
storePassword.toCharArray() : null);
+}
return ks;
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90315713
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -180,16 +185,16 @@ public void deleteSslCert(DeleteSslCertCmd
deleteSslCertCmd) {
}
@Override
-public List listSslCerts(ListSslCertsCmd
listSslCertCmd) {
-CallContext ctx = CallContext.current();
-Account caller = ctx.getCallingAccount();
+public List listSslCerts(final ListSslCertsCmd
listSslCertCmd) {
--- End diff --
Please consider adding a `Preconditions.checkArgument` check to verify that
`listSslCertCmd` is not `null`.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90338425
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -240,68 +245,71 @@ public void deleteSslCert(DeleteSslCertCmd
deleteSslCertCmd) {
}
if (projectId != null) {
-Project project = _projectMgr.getProject(projectId);
+final Project project = _projectMgr.getProject(projectId);
if (project == null) {
throw new InvalidParameterValueException("Found no project
with id: " + projectId);
}
-List projectCertVOList =
_sslCertDao.listByAccountId(project.getProjectAccountId());
-if (projectCertVOList == null || projectCertVOList.isEmpty())
+final List projectCertVOList =
_sslCertDao.listByAccountId(project.getProjectAccountId());
+if (projectCertVOList == null || projectCertVOList.isEmpty()) {
return certResponseList;
+}
_accountMgr.checkAccess(caller,
SecurityChecker.AccessType.UseEntry, true, projectCertVOList.get(0));
-for (SslCertVO cert : projectCertVOList) {
+for (final SslCertVO cert : projectCertVOList) {
certLbMap = _lbCertDao.listByCertId(cert.getId());
certResponseList.add(createCertResponse(cert, certLbMap));
}
return certResponseList;
}
//reached here look by accountId
-List certVOList =
_sslCertDao.listByAccountId(accountId);
-if (certVOList == null || certVOList.isEmpty())
+final List certVOList =
_sslCertDao.listByAccountId(accountId);
+if (certVOList == null || certVOList.isEmpty()) {
return certResponseList;
+}
_accountMgr.checkAccess(caller,
SecurityChecker.AccessType.UseEntry, true, certVOList.get(0));
-for (SslCertVO cert : certVOList) {
+for (final SslCertVO cert : certVOList) {
certLbMap = _lbCertDao.listByCertId(cert.getId());
certResponseList.add(createCertResponse(cert, certLbMap));
}
return certResponseList;
}
-private void validate(String certInput, String keyInput, String
password, String chainInput) {
+private void validate(final String certInput, final String keyInput,
final String password, final String chainInput) {
Certificate cert;
PrivateKey key;
List chain = null;
try {
cert = parseCertificate(certInput);
-key = parsePrivateKey(keyInput, password);
+key = parsePrivateKey(keyInput);
--- End diff --
Please consider consolidating the declaration of the `cert` and `key`
variables into the `try` block, as well as, pulling lines 298-303 into the
`try` block.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90313790
--- Diff: server/test/org/apache/cloudstack/network/lb/CertServiceTest.java
---
@@ -245,48 +245,48 @@ public void runUploadSslCertSelfSignedNoPassword()
throws Exception {
public void runUploadSslCertBadChain() throws IOException,
IllegalAccessException, NoSuchFieldException {
Assume.assumeTrue(isOpenJdk() || isJCEInstalled());
-String certFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.crt").getFile(),Charset.defaultCharset().name());
-String keyFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.key").getFile(),Charset.defaultCharset().name());
-String chainFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_self_signed.crt").getFile(),Charset.defaultCharset().name());
+final String certFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.crt").getFile(),Charset.defaultCharset().name());
+final String keyFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_ca_signed.key").getFile(),Charset.defaultCharset().name());
+final String chainFile =
URLDecoder.decode(getClass().getResource("/certs/rsa_self_signed.crt").getFile(),Charset.defaultCharset().name());
-String cert = readFileToString(new File(certFile));
-String key = readFileToString(new File(keyFile));
-String chain = readFileToString(new File(chainFile));
+final String cert = readFileToString(new File(certFile));
+final String key = readFileToString(new File(keyFile));
+final String chain = readFileToString(new File(chainFile));
-CertServiceImpl certService = new CertServiceImpl();
+final CertServiceImpl certService = new CertServiceImpl();
//setting mock objects
certService._accountMgr = Mockito.mock(AccountManager.class);
-Account account = new AccountVO("testaccount", 1, "networkdomain",
(short)0, UUID.randomUUID().toString());
+final Account account = new AccountVO("testaccount", 1,
"networkdomain", (short)0, UUID.randomUUID().toString());
when(certService._accountMgr.getAccount(anyLong())).thenReturn(account);
certService._domainDao = Mockito.mock(DomainDao.class);
-DomainVO domain = new DomainVO("networkdomain", 1L, 1L,
"networkdomain");
+final DomainVO domain = new DomainVO("networkdomain", 1L, 1L,
"networkdomain");
when(certService._domainDao.findByIdIncludingRemoved(anyLong())).thenReturn(domain);
certService._sslCertDao = Mockito.mock(SslCertDao.class);
when(certService._sslCertDao.persist(any(SslCertVO.class))).thenReturn(new
SslCertVO());
//creating the command
-UploadSslCertCmd uploadCmd = new UploadSslCertCmdExtn();
-Class _class = uploadCmd.getClass().getSuperclass();
+final UploadSslCertCmd uploadCmd = new UploadSslCertCmdExtn();
+final Class klazz = uploadCmd.getClass().getSuperclass();
-Field certField = _class.getDeclaredField("cert");
+final Field certField = klazz.getDeclaredField("cert");
certField.setAccessible(true);
certField.set(uploadCmd, cert);
-Field keyField = _class.getDeclaredField("key");
+final Field keyField = klazz.getDeclaredField("key");
keyField.setAccessible(true);
keyField.set(uploadCmd, key);
-Field chainField = _class.getDeclaredField("chain");
+final Field chainField = klazz.getDeclaredField("chain");
chainField.setAccessible(true);
chainField.set(uploadCmd, chain);
try {
certService.uploadSslCert(uploadCmd);
fail("The chain given is not the correct chain for the
certificate");
-} catch (Exception e) {
+} catch (final Exception e) {
--- End diff --
Please consider refactoring this method to use `@Test(expected)` to
eliminate this `try-catch` block.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90315176
--- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java
---
@@ -111,37 +116,37 @@ public CertServiceImpl() {
@DB
@Override
@ActionEvent(eventType = EventTypes.EVENT_LB_CERT_UPLOAD,
eventDescription = "Uploading a certificate to cloudstack", async = false)
-public SslCertResponse uploadSslCert(UploadSslCertCmd certCmd) {
+public SslCertResponse uploadSslCert(final UploadSslCertCmd certCmd) {
try {
-String cert = certCmd.getCert();
-String key = certCmd.getKey();
-String password = certCmd.getPassword();
-String chain = certCmd.getChain();
+final String cert = certCmd.getCert();
+final String key = certCmd.getKey();
+final String password = certCmd.getPassword();
+final String chain = certCmd.getChain();
validate(cert, key, password, chain);
s_logger.debug("Certificate Validation succeeded");
-String fingerPrint =
generateFingerPrint(parseCertificate(cert));
+final String fingerPrint =
generateFingerPrint(parseCertificate(cert));
-CallContext ctx = CallContext.current();
-Account caller = ctx.getCallingAccount();
+final CallContext ctx = CallContext.current();
+final Account caller = ctx.getCallingAccount();
Account owner = null;
-if ((certCmd.getAccountName() != null && certCmd.getDomainId()
!= null) || certCmd.getProjectId() != null) {
+if (certCmd.getAccountName() != null && certCmd.getDomainId()
!= null || certCmd.getProjectId() != null) {
--- End diff --
Should `certCmd.getAccountName()` be checked that is not blank?
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90309869
--- Diff:
utils/src/main/java/com/cloud/utils/security/CertificateHelper.java ---
@@ -40,123 +46,122 @@
import java.util.ArrayList;
import java.util.List;
-import com.cloud.utils.exception.CloudRuntimeException;
-import org.apache.commons.codec.binary.Base64;
-
-import com.cloud.utils.Ternary;
-import org.bouncycastle.openssl.PEMReader;
-
public class CertificateHelper {
-public static byte[] buildAndSaveKeystore(String alias, String cert,
String privateKey, String storePassword) throws KeyStoreException,
CertificateException,
-NoSuchAlgorithmException, InvalidKeySpecException, IOException {
-KeyStore ks = buildKeystore(alias, cert, privateKey,
storePassword);
-
-ByteArrayOutputStream os = new ByteArrayOutputStream();
-ks.store(os, storePassword != null ? storePassword.toCharArray() :
null);
-os.close();
-return os.toByteArray();
+public static byte[] buildAndSaveKeystore(final String alias, final
String cert, final String privateKey, final String storePassword) throws
KeyStoreException, CertificateException,
+NoSuchAlgorithmException, InvalidKeySpecException, IOException {
+final KeyStore ks = buildKeystore(alias, cert, privateKey,
storePassword);
+
+try (final ByteArrayOutputStream os = new ByteArrayOutputStream())
{
+ks.store(os, storePassword != null ?
storePassword.toCharArray() : null);
+return os.toByteArray();
+}
}
-public static byte[] buildAndSaveKeystore(List<Ternary<String, String,
String>> certs, String storePassword) throws KeyStoreException,
NoSuchAlgorithmException,
-CertificateException, IOException, InvalidKeySpecException {
-KeyStore ks = KeyStore.getInstance("JKS");
+public static byte[] buildAndSaveKeystore(final List<Ternary<String,
String, String>> certs, final String storePassword) throws KeyStoreException,
NoSuchAlgorithmException,
+CertificateException, IOException, InvalidKeySpecException {
+final KeyStore ks = KeyStore.getInstance("JKS");
ks.load(null, storePassword != null ? storePassword.toCharArray()
: null);
//name,cert,key
-for (Ternary<String, String, String> cert : certs) {
+for (final Ternary<String, String, String> cert : certs) {
if (cert.third() == null) {
-Certificate c = buildCertificate(cert.second());
+final Certificate c = buildCertificate(cert.second());
ks.setCertificateEntry(cert.first(), c);
} else {
-Certificate[] c = new Certificate[certs.size()];
+final Certificate[] c = new Certificate[certs.size()];
int i = certs.size();
-for (Ternary<String, String, String> ct : certs) {
+for (final Ternary<String, String, String> ct : certs) {
c[i - 1] = buildCertificate(ct.second());
i--;
}
ks.setKeyEntry(cert.first(),
buildPrivateKey(cert.third()), storePassword != null ?
storePassword.toCharArray() : null, c);
}
}
-ByteArrayOutputStream os = new ByteArrayOutputStream();
-ks.store(os, storePassword != null ? storePassword.toCharArray() :
null);
-os.close();
-return os.toByteArray();
+try (final ByteArrayOutputStream os = new ByteArrayOutputStream())
{
+ks.store(os, storePassword != null ?
storePassword.toCharArray() : null);
+return os.toByteArray();
+}
}
-public static KeyStore loadKeystore(byte[] ksData, String
storePassword) throws KeyStoreException, CertificateException,
NoSuchAlgorithmException, IOException {
-assert (ksData != null);
-KeyStore ks = KeyStore.getInstance("JKS");
-ks.load(new ByteArrayInputStream(ksData), storePassword != null ?
storePassword.toCharArray() : null);
+public static KeyStore loadKeystore(final byte[] ksData, final String
storePassword) throws KeyStoreException, CertificateException,
NoSuchAlgorithmException, IOException {
+assert ksData != null;
--- End diff --
Since we don't usually run with assertions enabled, please consider
converting `assert` to `Preconditions.checkArgument`.
---
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 featu
[GitHub] cloudstack pull request #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request:
https://github.com/apache/cloudstack/pull/1799#discussion_r90314823
--- Diff: server/test/org/apache/cloudstack/network/lb/CertServiceTest.java
---
@@ -125,48 +125,48 @@ public void runUploadSslCertWithCAChain() throws
Exception {
when(certService._accountDao.findByIdIncludingRemoved(anyLong())).thenReturn((AccountVO)account);
//creating the command
-UploadSslCertCmd uploadCmd = new UploadSslCertCmdExtn();
-Class _class = uploadCmd.getClass().getSuperclass();
+final UploadSslCertCmd uploadCmd = new UploadSslCertCmdExtn();
+final Class klazz = uploadCmd.getClass().getSuperclass();
-Field certField = _class.getDeclaredField("cert");
+final Field certField = klazz.getDeclaredField("cert");
certField.setAccessible(true);
certField.set(uploadCmd, cert);
-Field keyField = _class.getDeclaredField("key");
+final Field keyField = klazz.getDeclaredField("key");
keyField.setAccessible(true);
keyField.set(uploadCmd, key);
-Field chainField = _class.getDeclaredField("chain");
+final Field chainField = klazz.getDeclaredField("chain");
chainField.setAccessible(true);
chainField.set(uploadCmd, chain);
--- End diff --
This method has not assertions to verify the correctness of
`certService.uploadSslCert`. Please consider adding such assertions.
---
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 #1799: CLOUDSTACK-9632: Upgrade bouncy castle to ver...
Github user jburwell commented on a diff in the pull request: https://github.com/apache/cloudstack/pull/1799#discussion_r90339460 --- Diff: server/src/org/apache/cloudstack/network/lb/CertServiceImpl.java --- @@ -339,190 +348,182 @@ public SslCertResponse createCertResponse(SslCertVO cert, List
