Repository: incubator-brooklyn Updated Branches: refs/heads/master 7aa53e0bf -> 42e9aad4e
Improvements to JcloudsLocationSecurityGroupCustomizer * Class is no longer final. Useful methods are exposed to subclasses. * 0.0.0.0/0 used for CIDR for SSH so that Brooklyn failing over doesn't break its SSH access to machines. * Incorporates SecurityGroupDefinitions. * Adds ICMP permission to shared group. * Predicate<Exception> can be provided to indicate that failed requests may be retried. * Includes AwsExceptionRetryPredicate to retry InvalidGroup.InUse, DependencyViolation and RequestLimitExceeded errors. Project: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/commit/64ae942b Tree: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/tree/64ae942b Diff: http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/diff/64ae942b Branch: refs/heads/master Commit: 64ae942b4296119b468fa2522ea53142e6851070 Parents: 78776ca Author: Sam Corbett <[email protected]> Authored: Fri May 1 18:42:59 2015 +0100 Committer: Sam Corbett <[email protected]> Committed: Fri May 1 18:42:59 2015 +0100 ---------------------------------------------------------------------- .../JcloudsLocationSecurityGroupCustomizer.java | 164 ++++++++++++++++--- ...oudsLocationSecurityGroupCustomizerTest.java | 107 +++++++++++- 2 files changed, 241 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/64ae942b/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java ---------------------------------------------------------------------- diff --git a/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java b/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java index 6c18ed4..98edfcf 100644 --- a/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java +++ b/locations/jclouds/src/main/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizer.java @@ -18,12 +18,14 @@ */ package brooklyn.location.jclouds.networking; -import java.util.Collection; +import static com.google.common.base.Preconditions.checkNotNull; + import java.util.Iterator; import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; +import org.jclouds.aws.AWSResponseException; import org.jclouds.compute.ComputeService; import org.jclouds.compute.domain.SecurityGroup; import org.jclouds.compute.domain.Template; @@ -39,6 +41,8 @@ import brooklyn.location.geo.LocalhostExternalIpLoader; import brooklyn.location.jclouds.BasicJcloudsLocationCustomizer; import brooklyn.location.jclouds.JcloudsLocation; import brooklyn.location.jclouds.JcloudsSshMachineLocation; +import brooklyn.management.ManagementContext; +import brooklyn.util.exceptions.Exceptions; import brooklyn.util.net.Cidr; import brooklyn.util.task.Tasks; import brooklyn.util.time.Duration; @@ -47,22 +51,25 @@ import com.google.common.annotations.Beta; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Optional; import com.google.common.base.Predicate; +import com.google.common.base.Predicates; import com.google.common.base.Supplier; +import com.google.common.base.Suppliers; import com.google.common.base.Throwables; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; import com.google.common.cache.CacheLoader; import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; /** * Configures custom security groups on Jclouds locations. - * - * TODO this should allow {@link SecurityGroupDefinition} instances to be applied + * + * @since 0.7.0 */ @Beta -public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLocationCustomizer { +public class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLocationCustomizer { private static final Logger LOG = LoggerFactory.getLogger(JcloudsLocationSecurityGroupCustomizer.class); @@ -77,19 +84,31 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo /** Caches the base security group that should be shared between all instances in the same Jclouds location */ private final Cache<Location, SecurityGroup> sharedGroupCache = CacheBuilder.newBuilder().build(); + /** Caches security groups unique to instances */ private final Cache<String, SecurityGroup> uniqueGroupCache = CacheBuilder.newBuilder().build(); + /** The context for this location customizer. */ private final String applicationId; - private final Supplier<Cidr> brooklynCidrSupplier; - JcloudsLocationSecurityGroupCustomizer(String applicationId) { - this(applicationId, new LocalhostExternalIpCidrSupplier()); + /** The CIDR for addresses that may SSH to machines. */ + private Supplier<Cidr> sshCidrSupplier; + + /** + * A predicate indicating whether the customiser can retry a request to add a security group + * or a rule after an throwable is thrown. + */ + private Predicate<Exception> isExceptionRetryable = Predicates.alwaysFalse(); + + protected JcloudsLocationSecurityGroupCustomizer(String applicationId) { + // Would be better to restrict with something like LocalhostExternalIpCidrSupplier, but + // we risk making machines inaccessible from Brooklyn when HA fails over. + this(applicationId, Suppliers.ofInstance(new Cidr("0.0.0.0/0"))); } - JcloudsLocationSecurityGroupCustomizer(String applicationId, Supplier<Cidr> cidrSupplier) { + protected JcloudsLocationSecurityGroupCustomizer(String applicationId, Supplier<Cidr> sshCidrSupplier) { this.applicationId = applicationId; - this.brooklynCidrSupplier = cidrSupplier; + this.sshCidrSupplier = sshCidrSupplier; } /** @@ -112,34 +131,61 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo return getInstance(entity.getApplicationId()); } - /** @see #addPermissionsToLocation(brooklyn.location.jclouds.JcloudsSshMachineLocation, java.util.Collection) */ - public void addPermissionsToLocation(final JcloudsSshMachineLocation location, IpPermission... permissions) { + /** + * @param predicate + * A predicate whose return value indicates whether a request to add a security group + * or permission may be retried after its input {@link Exception} was thrown. + * @return this + */ + public JcloudsLocationSecurityGroupCustomizer setRetryExceptionPredicate(Predicate<Exception> predicate) { + this.isExceptionRetryable = checkNotNull(predicate, "predicate"); + return this; + } + + /** + * @param cidrSupplier A supplier returning a CIDR for hosts that are allowed to SSH to locations. + */ + public JcloudsLocationSecurityGroupCustomizer setSshCidrSupplier(Supplier<Cidr> cidrSupplier) { + this.sshCidrSupplier = checkNotNull(cidrSupplier, "cidrSupplier"); + return this; + } + + /** @see #addPermissionsToLocation(brooklyn.location.jclouds.JcloudsSshMachineLocation, java.lang.Iterable) */ + public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsSshMachineLocation location, IpPermission... permissions) { addPermissionsToLocation(location, ImmutableList.copyOf(permissions)); + return this; } + /** @see #addPermissionsToLocation(brooklyn.location.jclouds.JcloudsSshMachineLocation, java.lang.Iterable) */ + public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsSshMachineLocation location, SecurityGroupDefinition securityGroupDefinition) { + addPermissionsToLocation(location, securityGroupDefinition.getPermissions()); + return this; + } + /** * Applies the given security group permissions to the given location. - * <p/> + * <p> * Takes no action if the location's compute service does not have a security group extension. * @param permissions The set of permissions to be applied to the location * @param location Location to gain permissions */ - public void addPermissionsToLocation(final JcloudsSshMachineLocation location, final Collection<IpPermission> permissions) { + public JcloudsLocationSecurityGroupCustomizer addPermissionsToLocation(final JcloudsSshMachineLocation location, final Iterable<IpPermission> permissions) { ComputeService computeService = location.getParent().getComputeService(); String nodeId = location.getNode().getId(); addPermissionsToLocation(permissions, nodeId, computeService); + return this; } /** * Applies the given security group permissions to the given node with the given compute service. - * <p/> + * <p> * Takes no action if the compute service does not have a security group extension. * @param permissions The set of permissions to be applied to the node * @param nodeId The id of the node to update * @param computeService The compute service to use to apply the changes */ @VisibleForTesting - void addPermissionsToLocation(Collection<IpPermission> permissions, final String nodeId, ComputeService computeService) { + void addPermissionsToLocation(Iterable<IpPermission> permissions, final String nodeId, ComputeService computeService) { if (!computeService.getSecurityGroupExtension().isPresent()) { LOG.warn("Security group extension for {} absent; cannot update node {} with {}", new Object[] {computeService, nodeId, permissions}); @@ -221,7 +267,7 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo * SSH access on port 22 and allows communication on all ports between machines in * the same group. Security groups are reused when templates have equal * {@link org.jclouds.compute.domain.Template#getLocation locations}. - * <p/> + * <p> * This method is called by Brooklyn when obtaining machines, as part of the * {@link brooklyn.location.jclouds.JcloudsLocationCustomizer} contract. It * should not be called from anywhere else. @@ -279,7 +325,8 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo final String groupName = getNameForSharedSecurityGroup(); // Could sort-and-search if straight search is too expensive Optional<SecurityGroup> shared = Iterables.tryFind(securityApi.listSecurityGroupsInLocation(location), new Predicate<SecurityGroup>() { - @Override public boolean apply(final SecurityGroup input) { + @Override + public boolean apply(final SecurityGroup input) { // endsWith because Jclouds prepends 'jclouds#' to security group names. return input.getName().endsWith(groupName); } @@ -299,7 +346,7 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo * Creates a security group with rules to: * <ul> * <li>Allow SSH access on port 22 from the world</li> - * <li>Allow all communication between machines in the same group</li> + * <li>Allow TCP, UDP and ICMP communication between machines in the same group</li> * </ul> * @param groupName The name of the security group to create * @param location The location in which the security group will be created @@ -318,31 +365,44 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo .toPort(65535); addPermission(allWithinGroup.ipProtocol(IpProtocol.TCP).build(), group, securityApi); addPermission(allWithinGroup.ipProtocol(IpProtocol.UDP).build(), group, securityApi); + addPermission(allWithinGroup.ipProtocol(IpProtocol.ICMP).fromPort(-1).toPort(-1).build(), group, securityApi); IpPermission sshPermission = IpPermission.builder() .fromPort(22) .toPort(22) .ipProtocol(IpProtocol.TCP) - .cidrBlock(brooklynCidrSupplier.get().toString()) + .cidrBlock(getBrooklynCidrBlock()) .build(); addPermission(sshPermission, group, securityApi); return group; } - private SecurityGroup addSecurityGroupInLocation(String groupName, Location location, SecurityGroupExtension securityApi) { + protected SecurityGroup addSecurityGroupInLocation(final String groupName, final Location location, final SecurityGroupExtension securityApi) { LOG.debug("Creating security group {} in {}", groupName, location); - return securityApi.createSecurityGroup(groupName, location); + Callable<SecurityGroup> callable = new Callable<SecurityGroup>() { + @Override + public SecurityGroup call() throws Exception { + return securityApi.createSecurityGroup(groupName, location); + } + }; + return runOperationWithRetry(callable); } - private SecurityGroup addPermission(IpPermission permission, SecurityGroup group, SecurityGroupExtension securityApi) { + protected SecurityGroup addPermission(final IpPermission permission, final SecurityGroup group, final SecurityGroupExtension securityApi) { LOG.debug("Adding permission to security group {}: {}", group.getName(), permission); - return securityApi.addIpPermission(permission, group); + Callable<SecurityGroup> callable = new Callable<SecurityGroup>() { + @Override + public SecurityGroup call() throws Exception { + return securityApi.addIpPermission(permission, group); + } + }; + return runOperationWithRetry(callable); } /** @return the CIDR block used to configure Brooklyn's in security groups */ public String getBrooklynCidrBlock() { - return brooklynCidrSupplier.get().toString(); + return sshCidrSupplier.get().toString(); } /** @@ -366,6 +426,62 @@ public final class JcloudsLocationSecurityGroupCustomizer extends BasicJcloudsLo } /** + * Runs the given callable. Repeats until the operation succeeds or {@link #isExceptionRetryable} indicates + * that the request cannot be retried. + */ + protected <T> T runOperationWithRetry(Callable<T> operation) { + int backoff = 64; + Exception lastException = null; + for (int retries = 0; retries < 100; retries++) { + try { + return operation.call(); + } catch (Exception e) { + lastException = e; + if (isExceptionRetryable.apply(e)) { + LOG.debug("Attempt #{} failed to add security group: {}", retries + 1, e.getMessage()); + try { + Thread.sleep(backoff); + } catch (InterruptedException e1) { + throw Exceptions.propagate(e1); + } + backoff = backoff << 1; + } else { + break; + } + } + } + + throw new RuntimeException("Unable to add security group rule; repeated errors from provider", lastException); + } + + /** + * @return + * A predicate that is true if an exception contains an {@link org.jclouds.aws.AWSResponseException} + * whose error code is either <code>InvalidGroup.InUse</code>, <code>DependencyViolation</code> or + * <code>RequestLimitExceeded</code>. + */ + public static Predicate<Exception> newAwsExceptionRetryPredicate() { + return new AwsExceptionRetryPredicate(); + } + + private static class AwsExceptionRetryPredicate implements Predicate<Exception> { + // Error reference: http://docs.aws.amazon.com/AWSEC2/latest/APIReference/errors-overview.html + private static final Set<String> AWS_ERRORS_TO_RETRY = ImmutableSet.of( + "InvalidGroup.InUse", "DependencyViolation", "RequestLimitExceeded"); + + @Override + public boolean apply(Exception input) { + @SuppressWarnings("ThrowableResultOfMethodCallIgnored") + AWSResponseException exception = Exceptions.getFirstThrowableOfType(input, AWSResponseException.class); + if (exception != null) { + String code = exception.getError().getCode(); + return AWS_ERRORS_TO_RETRY.contains(code); + } + return false; + } + } + + /** * A supplier of CIDRs that loads the external IP address of the localhost machine. */ private static class LocalhostExternalIpCidrSupplier implements Supplier<Cidr> { http://git-wip-us.apache.org/repos/asf/incubator-brooklyn/blob/64ae942b/locations/jclouds/src/test/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizerTest.java ---------------------------------------------------------------------- diff --git a/locations/jclouds/src/test/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizerTest.java b/locations/jclouds/src/test/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizerTest.java index c2fcbb0..643f78f 100644 --- a/locations/jclouds/src/test/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizerTest.java +++ b/locations/jclouds/src/test/java/brooklyn/location/jclouds/networking/JcloudsLocationSecurityGroupCustomizerTest.java @@ -29,10 +29,13 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.testng.Assert.assertEquals; import static org.testng.Assert.assertNotEquals; +import static org.testng.Assert.assertTrue; import java.net.URI; import java.util.Collections; +import org.jclouds.aws.AWSResponseException; +import org.jclouds.aws.domain.AWSError; import org.jclouds.compute.ComputeService; import org.jclouds.compute.domain.SecurityGroup; import org.jclouds.compute.domain.Template; @@ -44,15 +47,16 @@ import org.jclouds.net.domain.IpProtocol; import org.testng.annotations.BeforeMethod; import org.testng.annotations.Test; -import brooklyn.location.jclouds.JcloudsLocation; -import brooklyn.util.collections.MutableMap; -import brooklyn.util.net.Cidr; - import com.google.common.base.Optional; +import com.google.common.base.Predicate; import com.google.common.base.Supplier; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; +import brooklyn.location.jclouds.JcloudsLocation; +import brooklyn.util.collections.MutableMap; +import brooklyn.util.net.Cidr; + public class JcloudsLocationSecurityGroupCustomizerTest { JcloudsLocationSecurityGroupCustomizer customizer; @@ -100,9 +104,9 @@ public class JcloudsLocationSecurityGroupCustomizerTest { customizer.customize(jcloudsLocationB, computeService, template); // One group with three permissions shared by both locations. - // Expect TCP+UDP between members of group and SSH to Brooklyn + // Expect TCP, UDP and ICMP between members of group and SSH to Brooklyn verify(securityApi).createSecurityGroup(anyString(), eq(location)); - verify(securityApi, times(3)).addIpPermission(any(IpPermission.class), eq(group)); + verify(securityApi, times(4)).addIpPermission(any(IpPermission.class), eq(group)); // New groups set on options verify(templateOptions, times(2)).securityGroups(anyString()); } @@ -183,6 +187,86 @@ public class JcloudsLocationSecurityGroupCustomizerTest { verify(securityApi, never()).addIpPermission(any(IpPermission.class), eq(sharedGroup)); } + @Test + public void testAddRuleNotRetriedByDefault() { + IpPermission ssh = newPermission(22); + String nodeId = "node"; + SecurityGroup sharedGroup = newGroup(customizer.getNameForSharedSecurityGroup()); + SecurityGroup uniqueGroup = newGroup("unique"); + when(securityApi.listSecurityGroupsForNode(nodeId)).thenReturn(ImmutableSet.of(sharedGroup, uniqueGroup)); + when(securityApi.addIpPermission(eq(ssh), eq(uniqueGroup))) + .thenThrow(new RuntimeException("exception creating " + ssh)); + + try { + customizer.addPermissionsToLocation(ImmutableList.of(ssh), nodeId, computeService); + } catch (Exception e) { + assertTrue(e.getMessage().contains("repeated errors from provider"), "message=" + e.getMessage()); + } + verify(securityApi, never()).createSecurityGroup(anyString(), any(Location.class)); + verify(securityApi, times(1)).addIpPermission(ssh, uniqueGroup); + } + + @Test + public void testCustomExceptionRetryablePredicate() { + final String message = "testCustomExceptionRetryablePredicate"; + Predicate<Exception> messageChecker = new Predicate<Exception>() { + @Override + public boolean apply(Exception input) { + Throwable t = input; + while (t != null) { + if (t.getMessage().contains(message)) { + return true; + } else { + t = t.getCause(); + } + } + return false; + } + }; + customizer.setRetryExceptionPredicate(messageChecker); + + IpPermission ssh = newPermission(22); + String nodeId = "node"; + SecurityGroup sharedGroup = newGroup(customizer.getNameForSharedSecurityGroup()); + SecurityGroup uniqueGroup = newGroup("unique"); + when(securityApi.listSecurityGroupsForNode(nodeId)).thenReturn(ImmutableSet.of(sharedGroup, uniqueGroup)); + when(securityApi.addIpPermission(eq(ssh), eq(uniqueGroup))) + .thenThrow(new RuntimeException(new Exception(message))) + .thenThrow(new RuntimeException(new Exception(message))) + .thenReturn(sharedGroup); + + customizer.addPermissionsToLocation(ImmutableList.of(ssh), nodeId, computeService); + + verify(securityApi, never()).createSecurityGroup(anyString(), any(Location.class)); + verify(securityApi, times(3)).addIpPermission(ssh, uniqueGroup); + } + + @Test + public void testAddRuleRetriedOnAwsFailure() { + IpPermission ssh = newPermission(22); + String nodeId = "nodeId"; + SecurityGroup sharedGroup = newGroup(customizer.getNameForSharedSecurityGroup()); + SecurityGroup uniqueGroup = newGroup("unique"); + customizer.setRetryExceptionPredicate(JcloudsLocationSecurityGroupCustomizer.newAwsExceptionRetryPredicate()); + when(securityApi.listSecurityGroupsForNode(nodeId)).thenReturn(ImmutableSet.of(sharedGroup, uniqueGroup)); + when(securityApi.addIpPermission(any(IpPermission.class), eq(uniqueGroup))) + .thenThrow(newAwsResponseExceptionWithCode("InvalidGroup.InUse")) + .thenThrow(newAwsResponseExceptionWithCode("DependencyViolation")) + .thenThrow(newAwsResponseExceptionWithCode("RequestLimitExceeded")) + .thenThrow(newAwsResponseExceptionWithCode("Blocked")) + .thenReturn(sharedGroup); + + try { + customizer.addPermissionsToLocation(ImmutableList.of(ssh), nodeId, computeService); + } catch (Exception e) { + String expected = "repeated errors from provider"; + assertTrue(e.getMessage().contains(expected), "expected exception message to contain " + expected + ", was: " + e.getMessage()); + } + + verify(securityApi, never()).createSecurityGroup(anyString(), any(Location.class)); + verify(securityApi, times(4)).addIpPermission(ssh, uniqueGroup); + } + private SecurityGroup newGroup(String id) { URI uri = null; String ownerId = null; @@ -206,4 +290,15 @@ public class JcloudsLocationSecurityGroupCustomizerTest { .cidrBlock("0.0.0.0/0") .build(); } + + private AWSError newAwsErrorWithCode(String code) { + AWSError e = new AWSError(); + e.setCode(code); + return e; + } + + private Exception newAwsResponseExceptionWithCode(String code) { + AWSResponseException e = new AWSResponseException("irrelevant message", null, null, newAwsErrorWithCode(code)); + return new RuntimeException(e); + } }
