Issue 757: Remove old public IPs when associating elastic IP addresses

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

@@ -53,7 +53,11 @@ public class AddElasticIpsToNodemetadata implements Function<NodeMetadata, NodeM
       this.cache = checkNotNull(cache, "cache");
    }
 
-   //TODO: can there be multiple elastic ips on one instance?
+   // Note: Instances only have one Internet routable IP address. When an Elastic IP is associated to an
+   // instance, the instance's existing Public IP address mapping is removed and is no longer valid for this instance
+   // http://aws.amazon.com/articles/1346
+
+   // TODO can there be multiple elastic ips on one instance?
    @Override
    public NodeMetadata apply(NodeMetadata arg0) {
       String[] parts = AWSUtils.parseHandle(arg0.getId());
@@ -61,8 +65,9 @@ public class AddElasticIpsToNodemetadata implements Function<NodeMetadata, NodeM
       String instanceId = parts[1];
       try {
          String publicIp = cache.get(new RegionAndName(region, instanceId));
-         return NodeMetadataBuilder.fromNodeMetadata(arg0).publicAddresses(
+         // Replace existing public addresses with elastic IP (see note above)
-                  ImmutableSet.<String> builder().addAll(arg0.getPublicAddresses()).add(publicIp).build()).build();
+         return NodeMetadataBuilder.fromNodeMetadata(arg0)
+                 .publicAddresses(ImmutableSet.<String> builder().add(publicIp).build()).build();
       } catch (CacheLoader.InvalidCacheLoadException e) {
          // no ip was found
          return arg0;

apis/ec2/src/test/java/org/jclouds/ec2/compute/EC2ComputeServiceLiveTest.java

@@ -18,10 +18,6 @@
  */
 package org.jclouds.ec2.compute;
 
-import static org.testng.Assert.assertEquals;
-import static org.testng.Assert.assertFalse;
-import static org.testng.Assert.assertTrue;
-
 import java.util.Map;
 import java.util.Properties;
 import java.util.Set;
@@ -69,6 +65,8 @@ import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.inject.Module;
 
+import static org.testng.Assert.*;
+
 /**
  * 
  * @author Adrian Cole
@@ -189,30 +187,31 @@ public class EC2ComputeServiceLiveTest extends BaseComputeServiceLiveTest {
          // create a node
          Set<? extends NodeMetadata> nodes =
                context.getComputeService().createNodesInGroup(group, 1);
-         assertTrue(nodes.size() == 1);
+         assertEquals(nodes.size(), 1, "One node should have been created");
 
-         // Get public IPs (on EC2 we should get 2, on Nova only 1)
+         // Get public IPs (We should get 1)
          NodeMetadata node = Iterables.get(nodes, 0);
          String region = node.getLocation().getParent().getId();
          Set<String> publicIps = node.getPublicAddresses();
+         assertFalse(Iterables.isEmpty(publicIps), String.format("no public addresses attached to node %s", node));
+         assertEquals(Iterables.size(publicIps), 1);
 
-         // Check that all ips are public and port 22 is accessible
+         // Check that the address is public and port 22 is accessible
-         for (String ip: publicIps) {
+         String ip = Iterables.getOnlyElement(publicIps);
-            assertFalse(InetAddresses2.isPrivateIPAddress(ip));
+         assertFalse(InetAddresses2.isPrivateIPAddress(ip));
-            IPSocket socket = new IPSocket(ip, 22);
+         IPSocket socket = new IPSocket(ip, 22);
-            assert socketTester.apply(socket) : String.format("failed to open socket %s on node %s", socket,
+         assertTrue(socketTester.apply(socket), String.format("failed to open socket %s on node %s", socket, node));
-                  node);
-         }
 
          // check that there is an elastic ip correlating to it
          EC2Client ec2 = EC2Client.class.cast(context.getProviderSpecificContext().getApi());
          Set<PublicIpInstanceIdPair> ipidpairs =
                ec2.getElasticIPAddressServices().describeAddressesInRegion(region, publicIps.toArray(new String[0]));
-         assert (ipidpairs.size() == 1) : ipidpairs;
+         assertEquals(ipidpairs.size(), 1, String.format("there should only be one address pair (%s)",
+               Iterables.toString(ipidpairs)));
 
          // check that the elastic ip is in node.publicAddresses
          PublicIpInstanceIdPair ipidpair = Iterables.get(ipidpairs, 0);
-         assertTrue(ipidpair.getInstanceId() == node.getId());
+         assertEquals(ipidpair.getInstanceId(), node.getId());
          
          // delete the node
          context.getComputeService().destroyNodesMatching(NodePredicates.inGroup(group));
@@ -220,11 +219,10 @@ public class EC2ComputeServiceLiveTest extends BaseComputeServiceLiveTest {
          // check that the ip is deallocated
          try {
             ec2.getElasticIPAddressServices().describeAddressesInRegion(region, ipidpair.getPublicIp());
-            assert false;
+            fail("address has not been deallocated");
          } catch (HttpResponseException e) {
-            // do nothing. .. it is expected to fail.
+            // do nothing ... this is expected to fail
          }
-
       } finally {
          context.getComputeService().destroyNodesMatching(NodePredicates.inGroup(group));
          if (context != null)

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

@@ -45,14 +45,14 @@ public class AddElasticIpsToNodemetadataTest {
             .build();
 
    @Test
-   public void testReturnsNodeWithExtraIpWhenFoundInCacheAndNodeHadAPublicIp() throws Exception {
+   public void testReturnsNodeWithElasticIpWhenFoundInCacheAndNodeHadAPublicIp() throws Exception {
       RegionAndName key = new RegionAndName("us-east-1", node.getProviderId());
       String val = "1.1.1.1";
       LoadingCache<RegionAndName, String> cache = cacheOf(key, val);
 
       AddElasticIpsToNodemetadata fn = new AddElasticIpsToNodemetadata(cache);
 
-      assertEquals(fn.apply(node).getPublicAddresses(), ImmutableSet.of("174.129.173.155", "1.1.1.1"));
+      assertEquals(fn.apply(node).getPublicAddresses(), ImmutableSet.of("1.1.1.1"));
    }
 
    @Test