You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2017/01/26 00:12:22 UTC
[jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
https://issues.apache.org/jira/browse/JCLOUDS-1231
This implements the SecurityGroupExtension and adds support for the `TemplateOptions.inboundPorts` feature. There is also some minor refactoring in the cleanup classes, to make it more reusable and to minimize the number of times the code is executed when cleaning up orphaned nodes.
All extension live tests are passing.
/cc @andreaturli
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds-labs/pull/347
-- Commit Summary --
* JCLOUDS-1231: Implement the SecurityGroupExtension in ARM
-- File Changes --
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeService.java (82)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/AzureComputeServiceAdapter.java (20)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/config/AzureComputeServiceContextModule.java (145)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/domain/RegionAndIdAndIngressRules.java (66)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/extensions/AzureComputeSecurityGroupExtension.java (315)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/functions/NetworkSecurityGroupToSecurityGroup.java (72)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/functions/NetworkSecurityRuleToIpPermission.java (76)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/functions/VirtualMachineToNodeMetadata.java (4)
A azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/loaders/CreateSecurityGroupIfNeeded.java (97)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/compute/strategy/CreateResourceGroupThenCreateNodes.java (112)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/NetworkSecurityGroup.java (17)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/domain/RegionAndId.java (2)
M azurecompute-arm/src/main/java/org/jclouds/azurecompute/arm/functions/CleanupResources.java (66)
M azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/extensions/AzureComputeImageExtensionLiveTest.java (1)
A azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/compute/extensions/AzureComputeSecurityGroupExtensionLiveTest.java (154)
M azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/features/NetworkSecurityGroupApiMockTest.java (2)
M azurecompute-arm/src/test/java/org/jclouds/azurecompute/arm/internal/BaseAzureComputeApiLiveTest.java (2)
M azurecompute-arm/src/test/resources/logback-test.xml (2)
-- Patch Links --
https://github.com/jclouds/jclouds-labs/pull/347.patch
https://github.com/jclouds/jclouds-labs/pull/347.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Andrea Turli <no...@github.com>.
andreaturli commented on this pull request.
lgtm, some minor comments
> @@ -114,7 +103,7 @@ public static Properties defaultProperties() {
properties.put(API_VERSION_PREFIX + PublicIPAddressApi.class.getSimpleName(), "2015-06-15");
properties.put(API_VERSION_PREFIX + ResourceGroupApi.class.getSimpleName(), "2015-01-01");
properties.put(API_VERSION_PREFIX + ResourceProviderApi.class.getSimpleName(), "2015-01-01");
- properties.put(API_VERSION_PREFIX + StorageAccountApi.class.getSimpleName(), STORAGE_API_VERSION);
+ properties.put(API_VERSION_PREFIX + StorageAccountApi.class.getSimpleName(), "2015-06-15");
we should decide at some point if we want to use the latest version of all of them or just use them randomly :D
> }
- public void deleteResourceGroupIfEmpty(String group) {
- if (api.getVirtualMachineApi(group).list().isEmpty()
- && api.getStorageAccountApi(group).list().isEmpty()
+ public boolean deleteResourceGroupIfEmpty(String group) {
+ boolean deleted = false;
+ if (api.getVirtualMachineApi(group).list().isEmpty() && api.getStorageAccountApi(group).list().isEmpty()
those api calls are really expensive, I'll try to replace them in a subsequent PR
> RegionAndId regionAndId = RegionAndId.fromSlashEncoded(id);
- String group = locationToResourceGroupName.apply(regionAndId.region());
-
+ ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+ String group = resourceGroup.name();
[minor] could you name it `resourceGroupName`
> final RegionAndId regionAndId = RegionAndId.fromSlashEncoded(cloneTemplate.getSourceNodeId());
- final String group = locationToResourceGroupName.apply(regionAndId.region());
+ ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+ final String group = resourceGroup.name();
[minor] maybe `resourceGroupName`?
> + AzureComputeSecurityGroupExtension(AzureComputeApi api, @Memoized Supplier<Set<? extends Location>> locations,
+ Function<NetworkSecurityGroup, SecurityGroup> groupConverter,
+ SecurityGroupAvailablePredicateFactory securityRuleAvailable,
+ @Named(TIMEOUT_RESOURCE_DELETED) Predicate<URI> resourceDeleted,
+ LoadingCache<String, ResourceGroup> resourceGroupMap) {
+ this.api = api;
+ this.locations = locations;
+ this.securityGroupConverter = groupConverter;
+ this.securityGroupAvailable = securityRuleAvailable;
+ this.resourceDeleted = resourceDeleted;
+ this.resourceGroupMap = resourceGroupMap;
+ }
+
+ @Override
+ public Set<SecurityGroup> listSecurityGroups() {
+ return ImmutableSet.copyOf(concat(transform(locations.get(), new Function<Location, Set<SecurityGroup>>() {
why immutableSet?
> + VirtualMachine vm = api.getVirtualMachineApi(resourceGroup.name()).get(regionAndId.id());
+ List<IdReference> networkInterfacesIdReferences = vm.properties().networkProfile().networkInterfaces();
+ List<NetworkSecurityGroup> networkGroups = new ArrayList<NetworkSecurityGroup>();
+
+ for (IdReference networkInterfaceCardIdReference : networkInterfacesIdReferences) {
+ String nicName = Iterables.getLast(Splitter.on("/").split(networkInterfaceCardIdReference.id()));
+ NetworkInterfaceCard card = api.getNetworkInterfaceCardApi(resourceGroup.name()).get(nicName);
+ if (card != null && card.properties().networkSecurityGroup() != null) {
+ String secGroupName = Iterables.getLast(Splitter.on("/").split(
+ card.properties().networkSecurityGroup().id()));
+ NetworkSecurityGroup group = api.getNetworkSecurityGroupApi(resourceGroup.name()).get(secGroupName);
+ networkGroups.add(group);
+ }
+ }
+
+ return ImmutableSet.copyOf(transform(filter(networkGroups, notNull()), securityGroupConverter));
why immutableSet?
> + final RegionAndId regionAndId = RegionAndId.fromSlashEncoded(group.getId());
+ ResourceGroup resourceGroup = resourceGroupMap.getUnchecked(regionAndId.region());
+
+ NetworkSecurityGroupApi groupApi = api.getNetworkSecurityGroupApi(resourceGroup.name());
+ NetworkSecurityGroup networkSecurityGroup = groupApi.get(regionAndId.id());
+
+ if (networkSecurityGroup == null) {
+ throw new IllegalArgumentException("Security group " + group.getName() + " was not found");
+ }
+
+ NetworkSecurityRuleApi ruleApi = api.getNetworkSecurityRuleApi(resourceGroup.name(), networkSecurityGroup.name());
+ int nextPriority = getRuleStartingPriority(networkSecurityGroup);
+
+ for (String ipRange : ipRanges) {
+ NetworkSecurityRuleProperties properties = NetworkSecurityRuleProperties.builder()
+ .protocol(Protocol.fromValue(protocol.name())) //
please remove `//`
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347#pullrequestreview-18640274
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
fb0c94c Return null if security group does not exist
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347/files/d96b0d6e3b4f3da51148c6db5cc363740c6db00e..fb0c94c81e18e743ad53305663d4cc17e0e4672a
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
Closed #347.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347#event-938492541
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
a0f45c2 Cleanup legacy code and introduce the resource group cache
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347/files/4ad4216899069216d34e072a310cd4a194a7aa96..a0f45c25d613ea54eb5fb6f8d208148e9daa3269
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
@andreaturli I've just added all the mentioned code cleanup and introduced the resource group cache. Please have a look.
All extension live tests are green. I'll run the compute service adapter ones in a while.
```bash
Starting test testCreateSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-1] Test testCreateSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 1215ms
Test suite progress: tests succeeded: 1, failed: 0, skipped: 0.
Starting test testCreateNodeWithInboundPorts(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-2] Test testCreateNodeWithInboundPorts(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 325603ms
Test suite progress: tests succeeded: 2, failed: 0, skipped: 0.
Starting test testCreateNodeWithSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-2] Test testCreateNodeWithSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 475649ms
Test suite progress: tests succeeded: 3, failed: 0, skipped: 0.
Starting test testGetSecurityGroupById(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-2] Test testGetSecurityGroupById(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 27745ms
Test suite progress: tests succeeded: 4, failed: 0, skipped: 0.
Starting test testAddIpPermission(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-3] Test testAddIpPermission(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 3496ms
Test suite progress: tests succeeded: 5, failed: 0, skipped: 0.
Starting test testRemoveIpPermission(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-4] Test testRemoveIpPermission(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 3441ms
Test suite progress: tests succeeded: 6, failed: 0, skipped: 0.
Starting test testAddIpPermissionsFromSpec(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-5] Test testAddIpPermissionsFromSpec(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 1750ms
Test suite progress: tests succeeded: 7, failed: 0, skipped: 0.
Starting test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-5] Test testAddIpPermissionWithCidrExclusionGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) skipped.
Test suite progress: tests succeeded: 7, failed: 0, skipped: 1.
[pool-1-thread-5] Test testRemoveIpPermissionWithCidrExclusionGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) skipped.
Test suite progress: tests succeeded: 7, failed: 0, skipped: 2.
Starting test testDeleteSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest)
[pool-1-thread-5] Test testDeleteSecurityGroup(org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest) succeeded: 705ms
Test suite progress: tests succeeded: 8, failed: 0, skipped: 2.
Tests run: 10, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 970.066 sec - in org.jclouds.azurecompute.arm.compute.extensions.AzureComputeSecurityGroupExtensionLiveTest
Results :
Tests run: 10, Failures: 0, Errors: 0, Skipped: 2
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347#issuecomment-275371290
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
d96b0d6 Reduce the number of API calls in the security group extension
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347/files/a0f45c25d613ea54eb5fb6f8d208148e9daa3269..d96b0d6e3b4f3da51148c6db5cc363740c6db00e
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
c33fa42 Address review comments
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347/files/fb0c94c81e18e743ad53305663d4cc17e0e4672a..c33fa42fa3a1e7f4a9f04ea6fbe4e06a513046b8
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
Pushed to master and 2.0.x!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347#issuecomment-275522943
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.
> + AzureComputeSecurityGroupExtension(AzureComputeApi api, @Memoized Supplier<Set<? extends Location>> locations,
+ Function<NetworkSecurityGroup, SecurityGroup> groupConverter,
+ SecurityGroupAvailablePredicateFactory securityRuleAvailable,
+ @Named(TIMEOUT_RESOURCE_DELETED) Predicate<URI> resourceDeleted,
+ LoadingCache<String, ResourceGroup> resourceGroupMap) {
+ this.api = api;
+ this.locations = locations;
+ this.securityGroupConverter = groupConverter;
+ this.securityGroupAvailable = securityRuleAvailable;
+ this.resourceDeleted = resourceDeleted;
+ this.resourceGroupMap = resourceGroupMap;
+ }
+
+ @Override
+ public Set<SecurityGroup> listSecurityGroups() {
+ return ImmutableSet.copyOf(concat(transform(locations.get(), new Function<Location, Set<SecurityGroup>>() {
I think it is good to enforce immutability. All the ARM model is immutable. Why should these collections be different? There are no methods that receive a list of security groups, so I don't see any value a mutable list could have, so let's try to default to immutable as a best practice.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347
Re: [jclouds/jclouds-labs] JCLOUDS-1231: Implement the
SecurityGroupExtension in ARM (#347)
Posted by Andrea Turli <no...@github.com>.
+1 @nacx, please merge it!
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/347#issuecomment-275449554