diff --git a/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java b/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java index 88ab63cb21..4f992b953d 100644 --- a/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java +++ b/apis/openstack-nova/src/main/java/org/jclouds/openstack/nova/v2_0/compute/extensions/NovaSecurityGroupExtension.java @@ -191,14 +191,20 @@ public class NovaSecurityGroupExtension implements SecurityGroupExtension { return false; } - if (sgApi.get().get(groupId) == null) { - return false; + // Would be nice to delete the group and invalidate the cache atomically - i.e. use a mutex. + // Will make sure that a create operation in parallel won't see inconsistent state. + + boolean deleted = sgApi.get().delete(groupId); + + for (SecurityGroupInRegion cachedSg : groupCreator.asMap().values()) { + if (groupId.equals(cachedSg.getSecurityGroup().getId())) { + String groupName = cachedSg.getName(); + groupCreator.invalidate(new RegionSecurityGroupNameAndPorts(region, groupName, ImmutableSet.of())); + break; + } } - sgApi.get().delete(groupId); - // TODO: test this clear happens - groupCreator.invalidate(new RegionSecurityGroupNameAndPorts(region, groupId, ImmutableSet. of())); - return true; + return deleted; } @Override diff --git a/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java b/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java index 08047993f5..2aa7e66c87 100644 --- a/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java +++ b/compute/src/test/java/org/jclouds/compute/extensions/internal/BaseSecurityGroupExtensionLiveTest.java @@ -27,6 +27,7 @@ import javax.annotation.Resource; import javax.inject.Named; import org.jclouds.compute.ComputeService; +import org.jclouds.compute.ComputeServiceContext; import org.jclouds.compute.RunNodesException; import org.jclouds.compute.domain.SecurityGroup; import org.jclouds.compute.domain.Template; @@ -60,6 +61,7 @@ public abstract class BaseSecurityGroupExtensionLiveTest extends BaseComputeServ protected Logger logger = Logger.NULL; protected final String secGroupName = "test-create-security-group"; + protected final String secGroupNameToDelete = "test-create-security-group-to-delete"; protected final String nodeGroup = "test-create-node-with-group"; protected String groupId; @@ -379,6 +381,70 @@ public abstract class BaseSecurityGroupExtensionLiveTest extends BaseComputeServ assertTrue(securityGroupExtension.get().removeSecurityGroup(group.getId())); } + @Test(groups = {"integration", "live"}, singleThreaded = true) + public void testSecurityGroupCacheInvalidated() throws Exception { + ComputeService computeService = view.getComputeService(); + Optional securityGroupExtension = computeService.getSecurityGroupExtension(); + assertTrue(securityGroupExtension.isPresent(), "security extension was not present"); + final SecurityGroupExtension security = securityGroupExtension.get(); + final SecurityGroup seedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation()); + boolean deleted = security.removeSecurityGroup(seedGroup.getId()); + assertTrue(deleted, "just created security group failed deletion"); + final SecurityGroup recreatedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation()); + + // Makes sure the security group exists and is re-created and is not just returned from cache + security.addIpPermission(IpPermission.builder() + .fromPort(1000) + .toPort(1000) + .cidrBlock("1.1.1.1/32") + .ipProtocol(IpProtocol.TCP) + .build(), + recreatedGroup); + boolean deleted2 = security.removeSecurityGroup(recreatedGroup.getId()); + assertTrue(deleted2, "just created security group failed deletion"); + } + + @Test(groups = {"integration", "live"}, singleThreaded = true) + public void testSecurityGroupCacheInvalidatedWhenDeletedExternally() throws Exception { + ComputeService computeService = view.getComputeService(); + Optional securityGroupExtension = computeService.getSecurityGroupExtension(); + assertTrue(securityGroupExtension.isPresent(), "security extension was not present"); + final SecurityGroupExtension security = securityGroupExtension.get(); + final SecurityGroup seedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation()); + + deleteSecurityGroupFromAnotherView(seedGroup); + + boolean deleted = security.removeSecurityGroup(seedGroup.getId()); + assertFalse(deleted, "SG deleted externally so should've failed deletion"); + final SecurityGroup recreatedGroup = security.createSecurityGroup(secGroupNameToDelete, getNodeTemplate().getLocation()); + + // Makes sure the security group exists and is re-created and is not just returned from cache + security.addIpPermission(IpPermission.builder() + .fromPort(1000) + .toPort(1000) + .cidrBlock("1.1.1.1/32") + .ipProtocol(IpProtocol.TCP) + .build(), + recreatedGroup); + boolean deleted2 = security.removeSecurityGroup(recreatedGroup.getId()); + assertTrue(deleted2, "just created security group failed deletion"); + } + + private void deleteSecurityGroupFromAnotherView(SecurityGroup seedGroup) { + ComputeServiceContext localView = createView(setupProperties(), setupModules()); + try { + ComputeService localComputeService = localView.getComputeService(); + Optional securityGroupExtension = localComputeService.getSecurityGroupExtension(); + assertTrue(securityGroupExtension.isPresent(), "security extension was not present"); + final SecurityGroupExtension security = securityGroupExtension.get(); + + boolean deleted = security.removeSecurityGroup(seedGroup.getId()); + assertTrue(deleted, "just created security group failed deletion"); + } finally { + localView.close(); + } + } + private Multimap emptyMultimap() { return LinkedHashMultimap.create(); }