Issue 888: use GroupNamingConvention for keyName/securityGroup; don't include region in name

apis/ec2/src/main/java/org/jclouds/ec2/compute/EC2ComputeService.java

@@ -45,6 +45,8 @@ import org.jclouds.compute.domain.Hardware;
 import org.jclouds.compute.domain.Image;
 import org.jclouds.compute.domain.NodeMetadata;
 import org.jclouds.compute.domain.TemplateBuilder;
+import org.jclouds.compute.functions.GroupNamingConvention;
+import org.jclouds.compute.functions.GroupNamingConvention.Factory;
 import org.jclouds.compute.internal.BaseComputeService;
 import org.jclouds.compute.internal.PersistNodeCredentials;
 import org.jclouds.compute.options.TemplateOptions;
@@ -87,6 +89,7 @@ public class EC2ComputeService extends BaseComputeService {
    private final EC2Client ec2Client;
    private final ConcurrentMap<RegionAndName, KeyPair> credentialsMap;
    private final LoadingCache<RegionAndName, String> securityGroupMap;
+   private final Factory namingConvention;
 
    @Inject
    protected EC2ComputeService(ComputeServiceContext context, Map<String, Credentials> credentialStore,
@@ -104,7 +107,7 @@ public class EC2ComputeService extends BaseComputeService {
          PersistNodeCredentials persistNodeCredentials, Timeouts timeouts,
          @Named(Constants.PROPERTY_USER_THREADS) ExecutorService executor, EC2Client ec2Client,
          ConcurrentMap<RegionAndName, KeyPair> credentialsMap, @Named("SECURITY") LoadingCache<RegionAndName, String> securityGroupMap,
-         Optional<ImageExtension> imageExtension) {
+         Optional<ImageExtension> imageExtension, GroupNamingConvention.Factory namingConvention) {
       super(context, credentialStore, images, sizes, locations, listNodesStrategy, getNodeMetadataStrategy,
             runNodesAndAddToSetStrategy, rebootNodeStrategy, destroyNodeStrategy, startNodeStrategy, stopNodeStrategy,
             templateBuilderProvider, templateOptionsProvider, nodeRunning, nodeTerminated, nodeSuspended,
@@ -113,6 +116,7 @@ public class EC2ComputeService extends BaseComputeService {
       this.ec2Client = ec2Client;
       this.credentialsMap = credentialsMap;
       this.securityGroupMap = securityGroupMap;
+      this.namingConvention = namingConvention;
    }
 
    @Inject(optional = true)
@@ -126,7 +130,8 @@ public class EC2ComputeService extends BaseComputeService {
    void deleteSecurityGroup(String region, String group) {
       checkNotEmpty(region, "region");
       checkNotEmpty(group, "group");
-      String groupName = String.format("jclouds#%s#%s", group, region).replace('#', delimiter);
+      String groupName = namingConvention.create().sharedNameForGroup(group);
+      
       if (ec2Client.getSecurityGroupServices().describeSecurityGroupsInRegion(region, groupName).size() > 0) {
          logger.debug(">> deleting securityGroup(%s)", groupName);
          ec2Client.getSecurityGroupServices().deleteSecurityGroupInRegion(region, groupName);
@@ -139,13 +144,12 @@ public class EC2ComputeService extends BaseComputeService {
    @VisibleForTesting
    void deleteKeyPair(String region, String group) {
       for (KeyPair keyPair : ec2Client.getKeyPairServices().describeKeyPairsInRegion(region)) {
-         if (
+         String keyName = keyPair.getKeyName();
-         // when the keypair is unique per group
+         Predicate<String> keyNameMatcher = namingConvention.create().containsGroup(group);
-         keyPair.getKeyName().equals("jclouds"+ delimiter + group)
+         String oldKeyNameRegex = String.format("jclouds#%s#%s#%s", group, region, "[0-9a-f]+").replace('#', delimiter);
-               || keyPair.getKeyName().matches(String.format("jclouds#%s#%s", group, "[0-9a-f]+").replace('#', delimiter))
+         // old keypair pattern too verbose as it has an unnecessary region qualifier
-               // old keypair pattern too verbose as it has an unnecessary
+         
-               // region qualifier
+         if (keyNameMatcher.apply(keyName) || keyName.matches(oldKeyNameRegex)) {
-               || keyPair.getKeyName().matches(String.format("jclouds#%s#%s#%s", group, region, "[0-9a-f]+").replace('#', delimiter))) {
             Set<String> instancesUsingKeyPair = extractIdsFromInstances(filter(concat(ec2Client.getInstanceServices()
                   .describeInstancesInRegion(region)), usingKeyPairAndNotDead(keyPair)));
             if (instancesUsingKeyPair.size() > 0) {

apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/CreateUniqueKeyPair.java

@@ -19,12 +19,12 @@
 package org.jclouds.ec2.compute.functions;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static org.jclouds.compute.config.ComputeServiceProperties.RESOURCENAME_DELIMITER;
 
 import javax.annotation.Resource;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.compute.reference.ComputeServiceConstants;
 import org.jclouds.ec2.EC2Client;
 import org.jclouds.ec2.compute.domain.RegionAndName;
@@ -33,7 +33,6 @@ import org.jclouds.logging.Logger;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
-import com.google.common.base.Supplier;
 import com.google.inject.Inject;
 
 /**
@@ -46,12 +45,12 @@ public class CreateUniqueKeyPair implements Function<RegionAndName, KeyPair> {
    @Named(ComputeServiceConstants.COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
    protected final EC2Client ec2Client;
-   protected final Supplier<String> randomSuffix;
+   protected final GroupNamingConvention.Factory namingConvention;
 
    @Inject
-   public CreateUniqueKeyPair(EC2Client ec2Client, Supplier<String> randomSuffix) {
+   public CreateUniqueKeyPair(EC2Client ec2Client, GroupNamingConvention.Factory namingConvention) {
       this.ec2Client = ec2Client;
-      this.randomSuffix = randomSuffix;
+      this.namingConvention = checkNotNull(namingConvention, "namingConvention");
    }
 
    @Override
@@ -65,22 +64,18 @@ public class CreateUniqueKeyPair implements Function<RegionAndName, KeyPair> {
       checkNotNull(group, "group");
       logger.debug(">> creating keyPair region(%s) group(%s)", region, group);
       KeyPair keyPair = null;
+      String prefix = group;
+      
       while (keyPair == null) {
+         String keyName = namingConvention.create().uniqueNameForGroup(prefix);
          try {
-            keyPair = ec2Client.getKeyPairServices().createKeyPairInRegion(region, getNextName(region, group));
+            keyPair = ec2Client.getKeyPairServices().createKeyPairInRegion(region, keyName);
-            logger.debug("<< created keyPair(%s)", keyPair);
          } catch (IllegalStateException e) {
-
+            logger.trace("   invalid keyname (%s in %s); retrying", keyName, region);
          }
       }
+      
+      logger.debug("<< created keyPair(%s)", keyPair);
       return keyPair;
    }
-   
-   @Inject(optional=true)
-   @Named(RESOURCENAME_DELIMITER) 
-   char delimiter = '#';
-   
-   private String getNextName(String region, String group) {
-      return String.format("jclouds#%s#%s#%s", group, region, randomSuffix.get()).replace('#', delimiter);
-   }
 }

apis/ec2/src/main/java/org/jclouds/ec2/compute/functions/RunningInstanceToNodeMetadata.java

@@ -21,16 +21,14 @@ package org.jclouds.ec2.compute.functions;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Predicates.not;
 import static com.google.common.collect.Iterables.filter;
-import static org.jclouds.compute.config.ComputeServiceProperties.RESOURCENAME_DELIMITER;
 
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Set;
-import java.util.Map.Entry;
 
 import javax.annotation.Resource;
-import javax.inject.Named;
 import javax.inject.Singleton;
 
 import org.jclouds.collect.Memoized;
@@ -42,6 +40,7 @@ import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.NodeState;
 import org.jclouds.compute.domain.Volume;
 import org.jclouds.compute.domain.internal.VolumeImpl;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.domain.Credentials;
 import org.jclouds.domain.Location;
 import org.jclouds.domain.LoginCredentials;
@@ -61,10 +60,10 @@ import com.google.common.base.Supplier;
 import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
-import com.google.common.collect.ImmutableSet.Builder;
 import com.google.common.util.concurrent.UncheckedExecutionException;
 import com.google.inject.Inject;
 
@@ -82,16 +81,19 @@ public class RunningInstanceToNodeMetadata implements Function<RunningInstance,
    protected final Supplier<LoadingCache<RegionAndName, ? extends Image>> imageMap;
    protected final Map<String, Credentials> credentialStore;
    protected final Map<InstanceState, NodeState> instanceToNodeState;
+   protected final GroupNamingConvention.Factory namingConvention;
 
    @Inject
    protected RunningInstanceToNodeMetadata(Map<InstanceState, NodeState> instanceToNodeState,
             Map<String, Credentials> credentialStore, Supplier<LoadingCache<RegionAndName, ? extends Image>> imageMap,
-            @Memoized Supplier<Set<? extends Location>> locations, @Memoized Supplier<Set<? extends Hardware>> hardware) {
+            @Memoized Supplier<Set<? extends Location>> locations, @Memoized Supplier<Set<? extends Hardware>> hardware,
+            GroupNamingConvention.Factory namingConvention) {
       this.locations = checkNotNull(locations, "locations");
       this.hardware = checkNotNull(hardware, "hardware");
       this.imageMap = checkNotNull(imageMap, "imageMap");
       this.instanceToNodeState = checkNotNull(instanceToNodeState, "instanceToNodeState");
       this.credentialStore = checkNotNull(credentialStore, "credentialStore");
+      this.namingConvention = checkNotNull(namingConvention, "namingConvention");
    }
 
    @Override
@@ -197,24 +199,16 @@ public class RunningInstanceToNodeMetadata implements Function<RunningInstance,
       return group;
    }
 
-   @Inject(optional = true)
-   @Named(RESOURCENAME_DELIMITER)
-   char delimiter = '#';
-
    private String parseGroupFrom(final RunningInstance instance, final Set<String> data) {
       String group = null;
       try {
-         group = Iterables.getOnlyElement(Iterables.filter(data, new Predicate<String>() {
+         Predicate<String> containsAnyGroup = namingConvention.create().containsAnyGroup();
-
+         String encodedGroup = Iterables.getOnlyElement(Iterables.filter(data, containsAnyGroup));
-            @Override
+         group = namingConvention.create().extractGroup(encodedGroup);
-            public boolean apply(String input) {
-               return input.startsWith("jclouds" + delimiter) && input.contains(delimiter + instance.getRegion());
-            }
-         })).split(delimiter + "")[1];
       } catch (NoSuchElementException e) {
          logger.debug("no group parsed from %s's data: %s", instance.getId(), data);
       } catch (IllegalArgumentException e) {
-         logger.debug("too many groups match %s%s; %s's data: %s", "jclouds", delimiter, instance.getId(), data);
+         logger.debug("too many groups match naming convention; %s's data: %s", instance.getId(), data);
       }
       return group;
    }

apis/ec2/src/main/java/org/jclouds/ec2/compute/strategy/CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions.java

@@ -21,7 +21,6 @@ package org.jclouds.ec2.compute.strategy;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
-import static org.jclouds.compute.config.ComputeServiceProperties.RESOURCENAME_DELIMITER;
 import static org.jclouds.crypto.SshKeys.fingerprintPrivateKey;
 import static org.jclouds.crypto.SshKeys.sha1PrivateKey;
 
@@ -33,6 +32,8 @@ import javax.inject.Provider;
 import javax.inject.Singleton;
 
 import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.functions.GroupNamingConvention;
+import org.jclouds.compute.functions.GroupNamingConvention.Factory;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.ec2.compute.domain.RegionAndName;
 import org.jclouds.ec2.compute.domain.RegionNameAndIngressRules;
@@ -63,16 +64,19 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions {
    public final LoadingCache<RegionAndName, String> securityGroupMap;
    @VisibleForTesting
    public final Provider<RunInstancesOptions> optionsProvider;
+   private final Factory namingConvention;
 
    @Inject
    public CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions(Function<RegionAndName, KeyPair> makeKeyPair,
             ConcurrentMap<RegionAndName, KeyPair> credentialsMap,
             @Named("SECURITY") LoadingCache<RegionAndName, String> securityGroupMap,
-            Provider<RunInstancesOptions> optionsProvider) {
+            Provider<RunInstancesOptions> optionsProvider,
+            GroupNamingConvention.Factory namingConvention) {
       this.makeKeyPair = checkNotNull(makeKeyPair, "makeKeyPair");
       this.credentialsMap = checkNotNull(credentialsMap, "credentialsMap");
       this.securityGroupMap = checkNotNull(securityGroupMap, "securityGroupMap");
       this.optionsProvider = checkNotNull(optionsProvider, "optionsProvider");
+      this.namingConvention = checkNotNull(namingConvention, "namingConvention");
    }
 
    public RunInstancesOptions execute(String region, String group, Template template) {
@@ -160,16 +164,13 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions {
       return keyPair.getKeyName();
    }
 
-   @Inject(optional = true)
-   @Named(RESOURCENAME_DELIMITER)
-   char delimiter = '#';
-
    @VisibleForTesting
    public Set<String> getSecurityGroupsForTagAndOptions(String region, @Nullable String group, TemplateOptions options) {
       Builder<String> groups = ImmutableSet.builder();
 
       if (group != null) {
-         String markerGroup = String.format("jclouds#%s#%s", group, region).replace('#', delimiter);
+         String markerGroup = namingConvention.create().sharedNameForGroup(group);
+
          groups.add(markerGroup);
 
          RegionNameAndIngressRules regionNameAndIngessRulesForMarkerGroup;

apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/CreateUniqueKeyPairTest.java

@@ -26,73 +26,92 @@ import static org.testng.Assert.assertEquals;
 
 import java.net.UnknownHostException;
 
+import org.jclouds.ec2.EC2ApiMetadata;
 import org.jclouds.ec2.EC2Client;
 import org.jclouds.ec2.domain.KeyPair;
 import org.jclouds.ec2.services.KeyPairClient;
 import org.testng.annotations.Test;
 
 import com.google.common.base.Supplier;
+import com.google.common.base.Suppliers;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.TypeLiteral;
+import com.google.inject.name.Names;
 
 /**
  * @author Adrian Cole
  */
 @Test(groups = "unit", testName = "CreateUniqueKeyPairTest")
 public class CreateUniqueKeyPairTest {
-   @SuppressWarnings( { "unchecked" })
+   
    @Test
    public void testApply() throws UnknownHostException {
-      EC2Client client = createMock(EC2Client.class);
+      final EC2Client client = createMock(EC2Client.class);
       KeyPairClient keyClient = createMock(KeyPairClient.class);
-      Supplier<String> uniqueIdSupplier = createMock(Supplier.class);
-
       KeyPair pair = createMock(KeyPair.class);
 
       expect(client.getKeyPairServices()).andReturn(keyClient).atLeastOnce();
 
-      expect(uniqueIdSupplier.get()).andReturn("1");
+      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#1")).andReturn(pair);
-      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#region#1")).andReturn(pair);
 
       replay(client);
       replay(keyClient);
-      replay(uniqueIdSupplier);
 
-      CreateUniqueKeyPair parser = new CreateUniqueKeyPair(client, uniqueIdSupplier);
+      CreateUniqueKeyPair parser = Guice.createInjector(new AbstractModule() {
+
+         @Override
+         protected void configure() {
+            Names.bindProperties(binder(),new EC2ApiMetadata().getDefaultProperties());
+            bind(new TypeLiteral<Supplier<String>>() {
+            }).toInstance(Suppliers.ofInstance("1"));
+            bind(EC2Client.class).toInstance(client);
+         }
+
+      }).getInstance(CreateUniqueKeyPair.class);
 
       assertEquals(parser.createNewKeyPairInRegion("region", "group"), pair);
 
       verify(client);
       verify(keyClient);
-      verify(uniqueIdSupplier);
    }
 
    @SuppressWarnings( { "unchecked" })
    @Test
    public void testApplyWithIllegalStateException() throws UnknownHostException {
-      EC2Client client = createMock(EC2Client.class);
+      final EC2Client client = createMock(EC2Client.class);
       KeyPairClient keyClient = createMock(KeyPairClient.class);
-      Supplier<String> uniqueIdSupplier = createMock(Supplier.class);
+      final Supplier<String> uniqueIdSupplier = createMock(Supplier.class);
 
       KeyPair pair = createMock(KeyPair.class);
 
       expect(client.getKeyPairServices()).andReturn(keyClient).atLeastOnce();
 
       expect(uniqueIdSupplier.get()).andReturn("1");
-      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#region#1")).andThrow(new IllegalStateException());
+      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#1")).andThrow(new IllegalStateException());
       expect(uniqueIdSupplier.get()).andReturn("2");
-      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#region#2")).andReturn(pair);
+      expect(keyClient.createKeyPairInRegion("region", "jclouds#group#2")).andReturn(pair);
 
       replay(client);
       replay(keyClient);
       replay(uniqueIdSupplier);
 
-      CreateUniqueKeyPair parser = new CreateUniqueKeyPair(client, uniqueIdSupplier);
+      CreateUniqueKeyPair parser = Guice.createInjector(new AbstractModule() {
+
+         @Override
+         protected void configure() {
+            Names.bindProperties(binder(),new EC2ApiMetadata().getDefaultProperties());
+            bind(new TypeLiteral<Supplier<String>>() {
+            }).toInstance(uniqueIdSupplier);
+            bind(EC2Client.class).toInstance(client);
+         }
+
+      }).getInstance(CreateUniqueKeyPair.class);
 
       assertEquals(parser.createNewKeyPairInRegion("region", "group"), pair);
 
       verify(client);
       verify(keyClient);
       verify(uniqueIdSupplier);
-
    }
-
 }

apis/ec2/src/test/java/org/jclouds/ec2/compute/functions/RunningInstanceToNodeMetadataTest.java

@@ -32,11 +32,13 @@ import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.NodeState;
 import org.jclouds.compute.domain.OperatingSystem;
 import org.jclouds.compute.domain.OsFamily;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.domain.Credentials;
 import org.jclouds.domain.Location;
 import org.jclouds.domain.LocationBuilder;
 import org.jclouds.domain.LocationScope;
 import org.jclouds.domain.LoginCredentials;
+import org.jclouds.ec2.EC2ApiMetadata;
 import org.jclouds.ec2.compute.config.EC2ComputeServiceDependenciesModule;
 import org.jclouds.ec2.compute.domain.RegionAndName;
 import org.jclouds.ec2.domain.InstanceState;
@@ -53,6 +55,9 @@ import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.name.Names;
 
 /**
  * @author Adrian Cole
@@ -221,14 +226,14 @@ public class RunningInstanceToNodeMetadataTest {
    public void testGroupNameIsSetWhenCustomKeyNameIsSetAndSecurityGroupIsGenerated() {
       checkGroupName(RunningInstance.builder().instanceId("id").imageId("image").instanceType("m1.small")
               .instanceState(InstanceState.RUNNING).region("us-east-1").keyName("custom-key")
-              .groupId("jclouds#groupname#us-east-1").build());
+              .groupId("jclouds#groupname").build());
    }
 
    @Test
    public void testGroupNameIsSetWhenCustomSecurityGroupIsSetAndKeyNameIsGenerated() {
       checkGroupName(RunningInstance.builder().instanceId("id").imageId("image").instanceType("m1.small")
               .instanceState(InstanceState.RUNNING).region("us-east-1").groupId("custom-sec")
-              .keyName("jclouds#groupname#us-east-1#23").build());
+              .keyName("jclouds#groupname#23").build());
    }
 
    protected RunningInstance firstInstanceFromResource(String resource) {
@@ -278,9 +283,19 @@ public class RunningInstanceToNodeMetadataTest {
          }
 
       };
+
+      GroupNamingConvention.Factory namingConvention = Guice.createInjector(new AbstractModule() {
+
+         @Override
+         protected void configure() {
+            Names.bindProperties(binder(),new EC2ApiMetadata().getDefaultProperties());
+         }
+
+      }).getInstance(GroupNamingConvention.Factory.class);
+
       RunningInstanceToNodeMetadata parser = new RunningInstanceToNodeMetadata(instanceToNodeState, credentialStore,
             Suppliers.<LoadingCache<RegionAndName, ? extends Image>> ofInstance(instanceToImage), locationSupplier,
-            hardwareSupplier);
+            hardwareSupplier, namingConvention);
       return parser;
    }
 

apis/ec2/src/test/java/org/jclouds/ec2/compute/strategy/CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest.java

@@ -34,6 +34,7 @@ import javax.inject.Provider;
 import org.jclouds.aws.domain.Region;
 import org.jclouds.compute.domain.Hardware;
 import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.domain.LoginCredentials;
 import org.jclouds.ec2.compute.domain.EC2HardwareBuilder;
@@ -416,7 +417,7 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest {
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupIds = ImmutableSet.<String> of();
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -450,7 +451,7 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest {
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupIds = ImmutableSet.<String> of();
       int[] ports = new int[] { 22, 80 };
       boolean shouldAuthorizeSelf = true;
@@ -484,7 +485,7 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest {
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupIds = ImmutableSet.<String> of();
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -517,7 +518,7 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest {
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupIds = ImmutableSet.<String> of("group1", "group2");
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -559,8 +560,15 @@ public class CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptionsTest {
       Function<RegionAndName, KeyPair> makeKeyPair = createMock(Function.class);
       ConcurrentMap<RegionAndName, KeyPair> credentialsMap = createMock(ConcurrentMap.class);
       LoadingCache<RegionAndName, String> securityGroupMap = createMock(LoadingCache.class);
+      GroupNamingConvention.Factory namingConventionFactory = createMock(GroupNamingConvention.Factory.class);
+      GroupNamingConvention namingConvention = createMock(GroupNamingConvention.class);
+      expect(namingConventionFactory.create()).andReturn(namingConvention).anyTimes();
+      expect(namingConvention.sharedNameForGroup("group")).andReturn("jclouds#group").anyTimes();
+      replay(namingConventionFactory);
+      replay(namingConvention);
+      
       return new CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions(makeKeyPair, credentialsMap,
-            securityGroupMap, OPTIONS_PROVIDER);
+            securityGroupMap, OPTIONS_PROVIDER, namingConventionFactory);
    }
 
    private void replayStrategy(CreateKeyPairAndSecurityGroupsAsNeededAndReturnRunOptions strategy) {

compute/src/main/java/org/jclouds/compute/functions/GroupNamingConvention.java

@@ -156,8 +156,17 @@ public interface GroupNamingConvention {
    String groupInSharedNameOrNull(String encoded);
 
    /**
-    * identifies if this name has a group encoded in it.
+    * A predicate that identifies if an input has the given group encoded in it.
     */
    Predicate<String> containsGroup(String group);
 
+   /**
+    * A predicate that identifies if an input has any group encoded in it.
+    */
+   Predicate<String> containsAnyGroup();
+   
+   /**
+    * Extracts the group from a shared/unique name. Or returns null if not in correct format to contain a group.
+    */
+   String extractGroup(String encoded);
 }

compute/src/main/java/org/jclouds/compute/internal/FormatSharedNamesAndAppendUniqueStringToThoseWhichRepeat.java

@@ -111,9 +111,8 @@ public class FormatSharedNamesAndAppendUniqueStringToThoseWhichRepeat implements
       this.delimiter = delimiter;
       this.suffixSupplier = checkNotNull(suffixSupplier, "suffixSupplier");
       this.groupValidator = checkNotNull(groupValidator, "groupValidator");
-      this.sharedFormat = "".equals(prefix) ? "%s" : new StringBuilder().append(prefix).append(delimiter).append("%s")
+      this.sharedFormat = "".equals(prefix) ? "%s" : prefix + delimiter + "%s";
-            .toString();
+      this.uniqueFormat = sharedFormat + delimiter + "%s";
-      this.uniqueFormat = new StringBuilder(sharedFormat).append(delimiter).append("%s").toString();
       this.uniqueGroupPattern = Pattern.compile("^" + ("".equals(prefix) ? "" : (prefix + delimiter)) + "(.+)"
             + delimiter + "[^" + delimiter + "]+");
       this.sharedGroupPattern = Pattern.compile("^" + ("".equals(prefix) ? "" : (prefix + delimiter)) + "(.+)$");
@@ -172,4 +171,32 @@ public class FormatSharedNamesAndAppendUniqueStringToThoseWhichRepeat implements
       };
    }
 
+   @Override
+   public Predicate<String> containsAnyGroup() {
+      return new Predicate<String>() {
+
+         @Override
+         public boolean apply(String input) {
+            try {
+               return groupInUniqueNameOrNull(input) != null || groupInSharedNameOrNull(input) != null;
+            } catch (NoSuchElementException e) {
+               return false;
+            }
+         }
+
+         @Override
+         public String toString() {
+            return "containsAnyGroup()";
+         }
+      };
+   }
+
+   @Override
+   public String extractGroup(String encoded) {
+      String result = groupInUniqueNameOrNull(encoded);
+      if (result != null) return result;
+      
+      result = groupInSharedNameOrNull(encoded);
+      return result;
+   }
 }

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeService.java

@@ -51,6 +51,7 @@ import org.jclouds.compute.domain.NodeMetadata;
 import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.Template;
 import org.jclouds.compute.domain.TemplateBuilder;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.compute.internal.PersistNodeCredentials;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.compute.reference.ComputeServiceConstants.Timeouts;
@@ -114,12 +115,12 @@ public class AWSEC2ComputeService extends EC2ComputeService {
             @Named("PLACEMENT") LoadingCache<RegionAndName, String> placementGroupMap,
             @Named("DELETED") Predicate<PlacementGroup> placementGroupDeleted,
             @Named(PROPERTY_EC2_GENERATE_INSTANCE_NAMES) boolean generateInstanceNames, AWSEC2AsyncClient aclient,
-            Optional<ImageExtension> imageExtension) {
+            Optional<ImageExtension> imageExtension, GroupNamingConvention.Factory namingConvention) {
       super(context, credentialStore, images, sizes, locations, listNodesStrategy, getNodeMetadataStrategy,
                runNodesAndAddToSetStrategy, rebootNodeStrategy, destroyNodeStrategy, startNodeStrategy,
                stopNodeStrategy, templateBuilderProvider, templateOptionsProvider, nodeRunning, nodeTerminated,
                nodeSuspended, initScriptRunnerFactory, runScriptOnNodeFactory, initAdminAccess, persistNodeCredentials,
-               timeouts, executor, ec2Client, credentialsMap, securityGroupMap, imageExtension);
+               timeouts, executor, ec2Client, credentialsMap, securityGroupMap, imageExtension, namingConvention);
       this.ec2Client = ec2Client;
       this.placementGroupMap = placementGroupMap;
       this.placementGroupDeleted = placementGroupDeleted;

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/functions/AWSRunningInstanceToNodeMetadata.java

@@ -35,6 +35,7 @@ import org.jclouds.compute.domain.HardwareBuilder;
 import org.jclouds.compute.domain.Image;
 import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.NodeState;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.domain.Credentials;
 import org.jclouds.domain.Location;
 import org.jclouds.domain.LoginCredentials;
@@ -55,8 +56,9 @@ public class AWSRunningInstanceToNodeMetadata extends RunningInstanceToNodeMetad
    @Inject
    protected AWSRunningInstanceToNodeMetadata(Map<InstanceState, NodeState> instanceToNodeState,
          Map<String, Credentials> credentialStore, Supplier<LoadingCache<RegionAndName, ? extends Image>> imageMap,
-         @Memoized Supplier<Set<? extends Location>> locations, @Memoized Supplier<Set<? extends Hardware>> hardware) {
+         @Memoized Supplier<Set<? extends Location>> locations, @Memoized Supplier<Set<? extends Hardware>> hardware,
-      super(instanceToNodeState, credentialStore, imageMap, locations, hardware);
+         GroupNamingConvention.Factory namingConvention) {
+      super(instanceToNodeState, credentialStore, imageMap, locations, hardware, namingConvention);
    }
 
    @Override

providers/aws-ec2/src/main/java/org/jclouds/aws/ec2/compute/strategy/CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions.java

@@ -34,6 +34,7 @@ import org.jclouds.aws.ec2.domain.RegionNameAndPublicKeyMaterial;
 import org.jclouds.aws.ec2.functions.CreatePlacementGroupIfNeeded;
 import org.jclouds.aws.ec2.options.AWSRunInstancesOptions;
 import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.compute.reference.ComputeServiceConstants;
 import org.jclouds.ec2.compute.domain.RegionAndName;
@@ -72,8 +73,9 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions
          Provider<RunInstancesOptions> optionsProvider,
          @Named("PLACEMENT") LoadingCache<RegionAndName, String> placementGroupMap,
          CreatePlacementGroupIfNeeded createPlacementGroupIfNeeded,
-         Function<RegionNameAndPublicKeyMaterial, KeyPair> importExistingKeyPair) {
+         Function<RegionNameAndPublicKeyMaterial, KeyPair> importExistingKeyPair, 
-      super(makeKeyPair, credentialsMap, securityGroupMap, optionsProvider);
+         GroupNamingConvention.Factory namingConvention) {
+      super(makeKeyPair, credentialsMap, securityGroupMap, optionsProvider, namingConvention);
       this.placementGroupMap = placementGroupMap;
       this.createPlacementGroupIfNeeded = createPlacementGroupIfNeeded;
       this.importExistingKeyPair = importExistingKeyPair;

providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/AWSEC2ComputeServiceLiveTest.java

@@ -180,12 +180,11 @@ public class AWSEC2ComputeServiceLiveTest extends EC2ComputeServiceLiveTest {
          }
 
          // make sure we made our dummy group and also let in the user's group
-         assertEquals(newTreeSet(instance.getGroupIds()), ImmutableSortedSet.<String> of("jclouds#" + group + "#"
+         assertEquals(newTreeSet(instance.getGroupIds()), ImmutableSortedSet.<String> of("jclouds#" + group, group));
-                  + instance.getRegion(), group));
 
          // make sure our dummy group has no rules
          SecurityGroup secgroup = getOnlyElement(securityGroupClient.describeSecurityGroupsInRegion(instance
-                  .getRegion(), "jclouds#" + group + "#" + instance.getRegion()));
+                  .getRegion(), "jclouds#" + group));
 
          assert secgroup.getIpPermissions().size() == 0 : secgroup;
 
@@ -258,7 +257,7 @@ public class AWSEC2ComputeServiceLiveTest extends EC2ComputeServiceLiveTest {
 
          // Assert the two instances are in the same groups
          region = instance1.getRegion();
-         String expectedSecurityGroupName = "jclouds#" + group + "#" + region;
+         String expectedSecurityGroupName = "jclouds#" + group;
          
          assertEquals(instance1.getRegion(), region);
          assertNotNull(instance1.getKeyName());

providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/functions/AWSRunningInstanceToNodeMetadataTest.java

@@ -23,12 +23,14 @@ import static org.testng.Assert.assertEquals;
 import java.util.Map;
 import java.util.Set;
 
+import org.jclouds.aws.ec2.AWSEC2ApiMetadata;
 import org.jclouds.aws.ec2.domain.AWSRunningInstance;
 import org.jclouds.aws.ec2.domain.MonitoringState;
 import org.jclouds.compute.domain.Hardware;
 import org.jclouds.compute.domain.Image;
 import org.jclouds.compute.domain.NodeMetadataBuilder;
 import org.jclouds.compute.domain.NodeState;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.date.DateService;
 import org.jclouds.domain.Credentials;
 import org.jclouds.domain.Location;
@@ -53,7 +55,9 @@ import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.inject.AbstractModule;
 import com.google.inject.Guice;
+import com.google.inject.name.Names;
 
 /**
  * @author Adrian Cole
@@ -86,7 +90,7 @@ public class AWSRunningInstanceToNodeMetadataTest {
                .instanceState(InstanceState.RUNNING)
                .privateDnsName("ip-10-212-81-7.ec2.internal")
                .dnsName("ec2-174-129-173-155.compute-1.amazonaws.com")
-               .keyName("jclouds#zkclustertest#us-east-1#23")
+               .keyName("jclouds#zkclustertest#23")
                .amiLaunchIndex("0")
                .instanceType("t1.micro")
                .launchTime(dateService.iso8601DateParse("2011-08-16T13:40:50.000Z"))
@@ -95,7 +99,7 @@ public class AWSRunningInstanceToNodeMetadataTest {
                .monitoringState(MonitoringState.DISABLED)
                .privateIpAddress("10.212.81.7")
                .ipAddress("174.129.173.155")
-               .securityGroupIdToNames(ImmutableMap.<String, String> of("sg-ef052b86", "jclouds#zkclustertest#us-east-1"))
+               .securityGroupIdToNames(ImmutableMap.<String, String> of("sg-ef052b86", "jclouds#zkclustertest"))
                .rootDeviceType(RootDeviceType.EBS)
                .rootDeviceName("/dev/sda1")
                .device("/dev/sda1", new BlockDevice("vol-5829fc32", Attachment.Status.ATTACHED, dateService.iso8601DateParse("2011-08-16T13:41:19.000Z"), true))
@@ -111,7 +115,7 @@ public class AWSRunningInstanceToNodeMetadataTest {
                         .instanceState(InstanceState.RUNNING)
                         .privateDnsName("ip-10-212-185-8.ec2.internal")
                         .dnsName("ec2-50-19-207-248.compute-1.amazonaws.com")
-                        .keyName("jclouds#zkclustertest#us-east-1#23")
+                        .keyName("jclouds#zkclustertest#23")
                         .amiLaunchIndex("0")
                         .instanceType("t1.micro")
                         .launchTime(dateService.iso8601DateParse("2011-08-16T13:40:50.000Z"))
@@ -120,7 +124,7 @@ public class AWSRunningInstanceToNodeMetadataTest {
                         .monitoringState(MonitoringState.DISABLED)
                         .privateIpAddress("10.212.185.8")
                         .ipAddress("50.19.207.248")
-                        .securityGroupIdToNames(ImmutableMap.<String, String>of("sg-ef052b86", "jclouds#zkclustertest#us-east-1"))
+                        .securityGroupIdToNames(ImmutableMap.<String, String>of("sg-ef052b86", "jclouds#zkclustertest"))
                         .rootDeviceType(RootDeviceType.EBS)
                         .rootDeviceName("/dev/sda1")
                         .device("/dev/sda1", new BlockDevice("vol-5029fc3a", Attachment.Status.ATTACHED, dateService.iso8601DateParse("2011-08-16T13:41:19.000Z"), true))
@@ -195,9 +199,19 @@ public class AWSRunningInstanceToNodeMetadataTest {
          }
 
       };
+      
+      GroupNamingConvention.Factory namingConvention = Guice.createInjector(new AbstractModule() {
+
+         @Override
+         protected void configure() {
+            Names.bindProperties(binder(),new AWSEC2ApiMetadata().getDefaultProperties());
+         }
+
+      }).getInstance(GroupNamingConvention.Factory.class);
+
       AWSRunningInstanceToNodeMetadata parser = new AWSRunningInstanceToNodeMetadata(instanceToNodeState,
             credentialStore, Suppliers.<LoadingCache<RegionAndName, ? extends Image>> ofInstance(instanceToImage),
-            locationSupplier, hardwareSupplier);
+            locationSupplier, hardwareSupplier, namingConvention);
       return parser;
    }
 

providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/compute/strategy/CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsTest.java

@@ -41,6 +41,7 @@ import org.jclouds.aws.ec2.functions.CreatePlacementGroupIfNeeded;
 import org.jclouds.aws.ec2.options.AWSRunInstancesOptions;
 import org.jclouds.compute.domain.Hardware;
 import org.jclouds.compute.domain.Template;
+import org.jclouds.compute.functions.GroupNamingConvention;
 import org.jclouds.compute.options.TemplateOptions;
 import org.jclouds.domain.LoginCredentials;
 import org.jclouds.ec2.compute.EC2TemplateBuilderTest;
@@ -659,7 +660,7 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupNames = ImmutableSet.<String> of();
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -693,7 +694,7 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupNames = ImmutableSet.<String> of();
       int[] ports = new int[] { 22, 80 };
       boolean shouldAuthorizeSelf = true;
@@ -727,7 +728,7 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupNames = ImmutableSet.<String> of();
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -761,7 +762,7 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupNames = ImmutableSet.<String> of("group1", "group2");
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -797,7 +798,7 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       // setup constants
       String region = Region.AP_SOUTHEAST_1;
       String group = "group";
-      String generatedMarkerGroup = "jclouds#group#" + Region.AP_SOUTHEAST_1;
+      String generatedMarkerGroup = "jclouds#group";
       Set<String> groupNames = ImmutableSet.<String> of();
       int[] ports = new int[] {};
       boolean shouldAuthorizeSelf = true;
@@ -938,9 +939,16 @@ public class CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptionsT
       LoadingCache<RegionAndName, String> placementGroupMap = createMock(LoadingCache.class);
       Function<RegionNameAndPublicKeyMaterial, KeyPair> importExistingKeyPair = createMock(Function.class);
       CreatePlacementGroupIfNeeded createPlacementGroupIfNeeded = createMock(CreatePlacementGroupIfNeeded.class);
+      GroupNamingConvention.Factory namingConventionFactory = createMock(GroupNamingConvention.Factory.class);
+      GroupNamingConvention namingConvention = createMock(GroupNamingConvention.class);
+      expect(namingConventionFactory.create()).andReturn(namingConvention).anyTimes();
+      expect(namingConvention.sharedNameForGroup("group")).andReturn("jclouds#group").anyTimes();
+      replay(namingConventionFactory);
+      replay(namingConvention);
 
       return new CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions(makeKeyPair, credentialsMap,
-            securityGroupMap, OPTIONS_PROVIDER, placementGroupMap, createPlacementGroupIfNeeded, importExistingKeyPair);
+            securityGroupMap, OPTIONS_PROVIDER, placementGroupMap, createPlacementGroupIfNeeded, importExistingKeyPair,
+            namingConventionFactory);
    }
 
    private void replayStrategy(CreateKeyPairPlacementAndSecurityGroupsAsNeededAndReturnRunOptions strategy) {