From 79a8336b100ab1393f93c1c8a3ba99643a689ee5 Mon Sep 17 00:00:00 2001 From: Adrian Cole Date: Thu, 22 Sep 2011 22:53:09 -0700 Subject: [PATCH] Issue 696:The security group 'X' does not exist error when creating nodes --- .../org/jclouds/ec2/EC2PropertiesBuilder.java | 2 + .../EC2ComputeServiceDependenciesModule.java | 14 +++ .../CreateSecurityGroupIfNeeded.java | 34 ++++-- .../predicates/SecurityGroupPresent.java | 70 +++++++++++ .../jclouds/ec2/domain/UserIdGroupPair.java | 6 + .../jclouds/ec2/reference/EC2Constants.java | 5 + .../CreateSecurityGroupIfNeededTest.java | 110 ++++++++++++++++++ 7 files changed, 230 insertions(+), 11 deletions(-) create mode 100644 apis/ec2/src/main/java/org/jclouds/ec2/compute/predicates/SecurityGroupPresent.java create mode 100644 apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeededTest.java diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/EC2PropertiesBuilder.java b/apis/ec2/src/main/java/org/jclouds/ec2/EC2PropertiesBuilder.java index 73bbbc873a..6a28aa6463 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/EC2PropertiesBuilder.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/EC2PropertiesBuilder.java @@ -22,6 +22,7 @@ import static org.jclouds.Constants.PROPERTY_API_VERSION; import static org.jclouds.aws.reference.AWSConstants.PROPERTY_AUTH_TAG; import static org.jclouds.aws.reference.AWSConstants.PROPERTY_HEADER_TAG; import static org.jclouds.ec2.reference.EC2Constants.PROPERTY_EC2_AMI_OWNERS; +import static org.jclouds.ec2.reference.EC2Constants.PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT; import java.util.Properties; @@ -40,6 +41,7 @@ public class EC2PropertiesBuilder extends PropertiesBuilder { properties.setProperty(PROPERTY_HEADER_TAG, "amz"); properties.setProperty(PROPERTY_API_VERSION, EC2AsyncClient.VERSION); properties.setProperty(PROPERTY_EC2_AMI_OWNERS, "*"); + properties.setProperty(PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT, "500"); return properties; } diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/compute/config/EC2ComputeServiceDependenciesModule.java b/apis/ec2/src/main/java/org/jclouds/ec2/compute/config/EC2ComputeServiceDependenciesModule.java index 21f9f2f7c7..fd4c246188 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/compute/config/EC2ComputeServiceDependenciesModule.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/compute/config/EC2ComputeServiceDependenciesModule.java @@ -18,8 +18,11 @@ */ package org.jclouds.ec2.compute.config; +import static org.jclouds.ec2.reference.EC2Constants.PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT; + import java.security.SecureRandom; import java.util.Map; +import java.util.concurrent.TimeUnit; import javax.inject.Named; import javax.inject.Singleton; @@ -44,13 +47,16 @@ import org.jclouds.ec2.compute.functions.RegionAndIdToImage; import org.jclouds.ec2.compute.functions.RunningInstanceToNodeMetadata; import org.jclouds.ec2.compute.internal.EC2TemplateBuilderImpl; import org.jclouds.ec2.compute.options.EC2TemplateOptions; +import org.jclouds.ec2.compute.predicates.SecurityGroupPresent; import org.jclouds.ec2.domain.InstanceState; import org.jclouds.ec2.domain.KeyPair; import org.jclouds.ec2.domain.RunningInstance; +import org.jclouds.predicates.RetryablePredicate; import org.jclouds.rest.RestContext; import org.jclouds.rest.internal.RestContextImpl; import com.google.common.base.Function; +import com.google.common.base.Predicate; import com.google.common.base.Supplier; import com.google.common.cache.Cache; import com.google.common.cache.CacheBuilder; @@ -144,6 +150,14 @@ public class EC2ComputeServiceDependenciesModule extends AbstractModule { return CacheBuilder.newBuilder().build(in); } + @Provides + @Singleton + @Named("SECURITY") + protected Predicate securityGroupEventualConsistencyDelay(SecurityGroupPresent in, + @Named(PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) { + return new RetryablePredicate(in, msDelay, 100l, TimeUnit.MILLISECONDS); + } + @Provides @Singleton // keys that we know about. diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeeded.java b/apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeeded.java index 73d182776e..5fa6cc73a1 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeeded.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeeded.java @@ -31,8 +31,10 @@ import org.jclouds.ec2.compute.domain.RegionAndName; import org.jclouds.ec2.compute.domain.RegionNameAndIngressRules; import org.jclouds.ec2.domain.IpProtocol; import org.jclouds.ec2.domain.UserIdGroupPair; +import org.jclouds.ec2.services.SecurityGroupClient; import org.jclouds.logging.Logger; +import com.google.common.base.Predicate; import com.google.common.cache.CacheLoader; import com.google.common.collect.Iterables; @@ -45,16 +47,25 @@ public class CreateSecurityGroupIfNeeded extends CacheLoader securityGroupEventualConsistencyDelay; @Inject - public CreateSecurityGroupIfNeeded(EC2Client ec2Client) { - this.ec2Client = checkNotNull(ec2Client, "ec2Client"); + public CreateSecurityGroupIfNeeded(EC2Client ec2Client, + @Named("SECURITY") Predicate securityGroupEventualConsistencyDelay) { + this(checkNotNull(ec2Client, "ec2Client").getSecurityGroupServices(), securityGroupEventualConsistencyDelay); + } + + public CreateSecurityGroupIfNeeded(SecurityGroupClient securityClient, + @Named("SECURITY") Predicate securityGroupEventualConsistencyDelay) { + this.securityClient = checkNotNull(securityClient, "securityClient"); + this.securityGroupEventualConsistencyDelay = checkNotNull(securityGroupEventualConsistencyDelay, + "securityGroupEventualConsistencyDelay"); } @Override public String load(RegionAndName from) { - RegionNameAndIngressRules realFrom = RegionNameAndIngressRules.class.cast(from); + RegionNameAndIngressRules realFrom = RegionNameAndIngressRules.class.cast(from); createSecurityGroupInRegion(from.getRegion(), from.getName(), realFrom.getPorts()); return from.getName(); } @@ -64,7 +75,11 @@ public class CreateSecurityGroupIfNeeded extends CacheLoader> creating securityGroup region(%s) name(%s)", region, name); try { - ec2Client.getSecurityGroupServices().createSecurityGroupInRegion(region, name, name); + securityClient.createSecurityGroupInRegion(region, name, name); + boolean created = securityGroupEventualConsistencyDelay.apply(new RegionAndName(region, name)); + if (!created) + throw new RuntimeException(String.format("security group %s/%s is not available after creating", region, + name)); logger.debug("<< created securityGroup(%s)", name); for (int port : ports) { createIngressRuleForTCPPort(region, name, port); @@ -79,17 +94,14 @@ public class CreateSecurityGroupIfNeeded extends CacheLoader> authorizing securityGroup region(%s) name(%s) port(%s)", region, name, port); - ec2Client.getSecurityGroupServices().authorizeSecurityGroupIngressInRegion(region, name, IpProtocol.TCP, port, - port, "0.0.0.0/0"); + securityClient.authorizeSecurityGroupIngressInRegion(region, name, IpProtocol.TCP, port, port, "0.0.0.0/0"); logger.debug("<< authorized securityGroup(%s)", name); } private void authorizeGroupToItself(String region, String name) { logger.debug(">> authorizing securityGroup region(%s) name(%s) permission to itself", region, name); - String myOwnerId = Iterables.get( - ec2Client.getSecurityGroupServices().describeSecurityGroupsInRegion(region, name), 0).getOwnerId(); - ec2Client.getSecurityGroupServices().authorizeSecurityGroupIngressInRegion(region, name, - new UserIdGroupPair(myOwnerId, name)); + String myOwnerId = Iterables.get(securityClient.describeSecurityGroupsInRegion(region, name), 0).getOwnerId(); + securityClient.authorizeSecurityGroupIngressInRegion(region, name, new UserIdGroupPair(myOwnerId, name)); logger.debug("<< authorized securityGroup(%s)", name); } diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/compute/predicates/SecurityGroupPresent.java b/apis/ec2/src/main/java/org/jclouds/ec2/compute/predicates/SecurityGroupPresent.java new file mode 100644 index 0000000000..8b38312c5a --- /dev/null +++ b/apis/ec2/src/main/java/org/jclouds/ec2/compute/predicates/SecurityGroupPresent.java @@ -0,0 +1,70 @@ +/** + * Licensed to jclouds, Inc. (jclouds) under one or more + * contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. jclouds 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.jclouds.ec2.compute.predicates; + +import static com.google.common.base.Preconditions.checkNotNull; + +import java.util.NoSuchElementException; + +import javax.annotation.Resource; +import javax.inject.Singleton; + +import org.jclouds.ec2.EC2Client; +import org.jclouds.ec2.compute.domain.RegionAndName; +import org.jclouds.ec2.domain.SecurityGroup; +import org.jclouds.logging.Logger; +import org.jclouds.rest.ResourceNotFoundException; + +import com.google.common.base.Predicate; +import com.google.common.collect.Iterables; +import com.google.inject.Inject; + +/** + * + * @author Adrian Cole + */ +@Singleton +public class SecurityGroupPresent implements Predicate { + + private final EC2Client client; + + @Resource + protected Logger logger = Logger.NULL; + + @Inject + public SecurityGroupPresent(EC2Client client) { + this.client = checkNotNull(client, "client"); + } + + public boolean apply(RegionAndName securityGroup) { + logger.trace("looking for security group %s/%s", securityGroup.getRegion(), securityGroup.getName()); + try { + return refresh(securityGroup) != null; + } catch (ResourceNotFoundException e) { + return false; + } catch (NoSuchElementException e) { + return false; + } + } + + protected SecurityGroup refresh(RegionAndName securityGroup) { + return Iterables.getOnlyElement(client.getSecurityGroupServices().describeSecurityGroupsInRegion( + securityGroup.getRegion(), securityGroup.getName())); + } +} diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/domain/UserIdGroupPair.java b/apis/ec2/src/main/java/org/jclouds/ec2/domain/UserIdGroupPair.java index 9e600a7a16..f055cd43f1 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/domain/UserIdGroupPair.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/domain/UserIdGroupPair.java @@ -37,6 +37,12 @@ public class UserIdGroupPair implements Comparable { } + @Override + public String toString() { + return "[userId=" + userId + ", groupName=" + groupName + "]"; + } + + /** * {@inheritDoc} */ diff --git a/apis/ec2/src/main/java/org/jclouds/ec2/reference/EC2Constants.java b/apis/ec2/src/main/java/org/jclouds/ec2/reference/EC2Constants.java index f42afc1439..44a05464f0 100644 --- a/apis/ec2/src/main/java/org/jclouds/ec2/reference/EC2Constants.java +++ b/apis/ec2/src/main/java/org/jclouds/ec2/reference/EC2Constants.java @@ -31,5 +31,10 @@ public interface EC2Constants { * the ami owners you wish to use in {@link ComputeService} */ public static final String PROPERTY_EC2_AMI_OWNERS = "jclouds.ec2.ami-owners"; + + /** + * Eventual consistency delay for retrieving a security group after it is created (in ms) + */ + public static final String PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT = "jclouds.ec2.timeout.securitygroup-present"; } diff --git a/apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeededTest.java b/apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeededTest.java new file mode 100644 index 0000000000..ceaea5e117 --- /dev/null +++ b/apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/CreateSecurityGroupIfNeededTest.java @@ -0,0 +1,110 @@ +/** + * Licensed to jclouds, Inc. (jclouds) under one or more + * contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. jclouds 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.jclouds.ec2.compute.functions; + +import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; +import static org.easymock.classextension.EasyMock.createMock; +import static org.easymock.classextension.EasyMock.createNiceMock; +import static org.easymock.classextension.EasyMock.replay; +import static org.easymock.classextension.EasyMock.verify; +import static org.testng.Assert.assertEquals; + +import java.util.Set; +import java.util.concurrent.ExecutionException; + +import org.jclouds.ec2.compute.domain.RegionAndName; +import org.jclouds.ec2.compute.domain.RegionNameAndIngressRules; +import org.jclouds.ec2.domain.IpProtocol; +import org.jclouds.ec2.domain.SecurityGroup; +import org.jclouds.ec2.domain.UserIdGroupPair; +import org.jclouds.ec2.services.SecurityGroupClient; +import org.testng.annotations.Test; + +import com.google.common.base.Predicate; +import com.google.common.base.Predicates; +import com.google.common.collect.ImmutableSet; + +/** + * @author Adrian Cole + */ +@Test(groups = "unit", singleThreaded = true, testName = "CreateSecurityGroupIfNeeded") +public class CreateSecurityGroupIfNeededTest { + + @SuppressWarnings("unchecked") + @Test + public void testWhenPort22AndToItselfAuthorizesIngressTwice() throws ExecutionException { + + SecurityGroupClient client = createMock(SecurityGroupClient.class); + Predicate tester = Predicates.alwaysTrue(); + + SecurityGroup group = createNiceMock(SecurityGroup.class); + Set groups = ImmutableSet. of(group); + + client.createSecurityGroupInRegion("region", "group", "group"); + client.authorizeSecurityGroupIngressInRegion("region", "group", IpProtocol.TCP, 22, 22, "0.0.0.0/0"); + expect(client.describeSecurityGroupsInRegion("region", "group")).andReturn(Set.class.cast(groups)); + expect(group.getOwnerId()).andReturn("ownerId"); + client.authorizeSecurityGroupIngressInRegion("region", "group", new UserIdGroupPair("ownerId", "group")); + + replay(client); + replay(group); + + CreateSecurityGroupIfNeeded function = new CreateSecurityGroupIfNeeded(client, tester); + + assertEquals("group", function.load(new RegionNameAndIngressRules("region", "group", new int[] { 22 }, true))); + + verify(client); + verify(group); + + } + + @Test + public void testIllegalStateExceptionCreatingGroupJustReturns() throws ExecutionException { + + SecurityGroupClient client = createMock(SecurityGroupClient.class); + Predicate tester = Predicates.alwaysTrue(); + + client.createSecurityGroupInRegion("region", "group", "group"); + expectLastCall().andThrow(new IllegalStateException()); + + replay(client); + + CreateSecurityGroupIfNeeded function = new CreateSecurityGroupIfNeeded(client, tester); + + assertEquals("group", function.load(new RegionNameAndIngressRules("region", "group", new int[] { 22 }, true))); + + verify(client); + + } + + @Test(expectedExceptions = RuntimeException.class) + public void testWhenEventualConsistencyExpiresIllegalStateException() throws ExecutionException { + + SecurityGroupClient client = createMock(SecurityGroupClient.class); + Predicate tester = Predicates.alwaysFalse(); + + client.createSecurityGroupInRegion("region", "group", "group"); + + replay(client); + + CreateSecurityGroupIfNeeded function = new CreateSecurityGroupIfNeeded(client, tester); + function.load(new RegionNameAndIngressRules("region", "group", new int[] { 22 }, true)); + } +}