Issue 696:The security group 'X' does not exist error when creating nodes

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;
    }
 

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<RegionAndName> securityGroupEventualConsistencyDelay(SecurityGroupPresent in,
+         @Named(PROPERTY_EC2_TIMEOUT_SECURITYGROUP_PRESENT) long msDelay) {
+      return new RetryablePredicate<RegionAndName>(in, msDelay, 100l, TimeUnit.MILLISECONDS);
+   }
+
    @Provides
    @Singleton
    // keys that we know about.

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<RegionAndName, Stri
    @Resource
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
-   protected final EC2Client ec2Client;
+   protected final SecurityGroupClient securityClient;
+   protected final Predicate<RegionAndName> securityGroupEventualConsistencyDelay;
 
    @Inject
-   public CreateSecurityGroupIfNeeded(EC2Client ec2Client) {
-      this.ec2Client = checkNotNull(ec2Client, "ec2Client");
+   public CreateSecurityGroupIfNeeded(EC2Client ec2Client,
+         @Named("SECURITY") Predicate<RegionAndName> securityGroupEventualConsistencyDelay) {
+      this(checkNotNull(ec2Client, "ec2Client").getSecurityGroupServices(), securityGroupEventualConsistencyDelay);
+   }
+
+   public CreateSecurityGroupIfNeeded(SecurityGroupClient securityClient,
+         @Named("SECURITY") Predicate<RegionAndName> 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<RegionAndName, Stri
       checkNotNull(name, "name");
       logger.debug(">> 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<RegionAndName, Stri
 
    private void createIngressRuleForTCPPort(String region, String name, int port) {
       logger.debug(">> 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);
    }
 

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<RegionAndName> {
+
+   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()));
+   }
+}

apis/ec2/src/main/java/org/jclouds/ec2/domain/UserIdGroupPair.java

@@ -37,6 +37,12 @@ public class UserIdGroupPair implements Comparable<UserIdGroupPair> {
    }
 
 
+   @Override
+   public String toString() {
+      return "[userId=" + userId + ", groupName=" + groupName + "]";
+   }
+
+
    /**
     * {@inheritDoc}
     */

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";
 
 }

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<RegionAndName> tester = Predicates.alwaysTrue();
+
+      SecurityGroup group = createNiceMock(SecurityGroup.class);
+      Set<SecurityGroup> groups = ImmutableSet.<SecurityGroup> 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<RegionAndName> 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<RegionAndName> 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));
+   }
+}