Merge pull request #185 from andreisavu/multiple-ip-forwarding

A VM and an IP address can have multiple forwarding rules attached

apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/config/CloudStackComputeServiceContextModule.java

@@ -22,12 +22,14 @@ import static com.google.common.base.Preconditions.checkNotNull;
 import static org.jclouds.Constants.PROPERTY_SESSION_INTERVAL;
 
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.collect.ImmutableSet;
 import org.jclouds.cloudstack.CloudStackAsyncClient;
 import org.jclouds.cloudstack.CloudStackClient;
 import org.jclouds.cloudstack.compute.functions.ServiceOfferingToHardware;
@@ -104,8 +106,8 @@ public class CloudStackComputeServiceContextModule
       bind(new TypeLiteral<Function<Template, OperatingSystem>>() {
       }).to(TemplateToOperatingSystem.class);
       install(new FactoryModuleBuilder().build(StaticNATVirtualMachineInNetwork.Factory.class));
-      bind(new TypeLiteral<CacheLoader<Long, IPForwardingRule>>() {
+      bind(new TypeLiteral<CacheLoader<Long, Set<IPForwardingRule>>>() {
-      }).to(GetIPForwardingRuleByVirtualMachine.class);
+      }).to(GetIPForwardingRulesByVirtualMachine.class);
    }
 
    @Provides
@@ -171,17 +173,17 @@ public class CloudStackComputeServiceContextModule
 
    @Provides
    @Singleton
-   protected Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine(
+   protected Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine(
-         CacheLoader<Long, IPForwardingRule> getIPForwardingRule) {
+      CacheLoader<Long, Set<IPForwardingRule>> getIPForwardingRules) {
-      return CacheBuilder.newBuilder().build(getIPForwardingRule);
+      return CacheBuilder.newBuilder().build(getIPForwardingRules);
    }
 
    @Singleton
-   public static class GetIPForwardingRuleByVirtualMachine extends CacheLoader<Long, IPForwardingRule> {
+   public static class GetIPForwardingRulesByVirtualMachine extends CacheLoader<Long, Set<IPForwardingRule>> {
       private final CloudStackClient client;
 
       @Inject
-      public GetIPForwardingRuleByVirtualMachine(CloudStackClient client) {
+      public GetIPForwardingRulesByVirtualMachine(CloudStackClient client) {
          this.client = checkNotNull(client, "client");
       }
 
@@ -190,11 +192,9 @@ public class CloudStackComputeServiceContextModule
        *            when there is no ip forwarding rule available for the VM
        */
       @Override
-      public IPForwardingRule load(Long input) {
+      public Set<IPForwardingRule> load(Long input) {
-         IPForwardingRule rule = client.getNATClient().getIPForwardingRuleForVirtualMachine(input);
+         Set<IPForwardingRule> rules = client.getNATClient().getIPForwardingRulesForVirtualMachine(input);
-         if (rule == null)
+         return rules != null ? rules : ImmutableSet.<IPForwardingRule>of();
-            throw new ResourceNotFoundException("no ip forwarding rule for: " + input);
-         return rule;
       }
    }
 

apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/functions/VirtualMachineToNodeMetadata.java

@@ -19,14 +19,19 @@
 package org.jclouds.cloudstack.compute.functions;
 
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.collect.Iterables.filter;
+import static com.google.common.collect.Iterables.transform;
 import static org.jclouds.compute.util.ComputeServiceUtils.parseGroupFromName;
 
 import java.util.Map;
 import java.util.Set;
 
+import javax.annotation.Nullable;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
+import com.google.common.base.Predicate;
+import com.google.common.collect.Iterables;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.VirtualMachine;
 import org.jclouds.collect.FindResourceInSet;
@@ -56,7 +61,7 @@ import com.google.common.util.concurrent.UncheckedExecutionException;
 public class VirtualMachineToNodeMetadata implements Function<VirtualMachine, NodeMetadata> {
 
    public static final Map<VirtualMachine.State, NodeState> vmStateToNodeState = ImmutableMap
-         .<VirtualMachine.State, NodeState> builder().put(VirtualMachine.State.STARTING, NodeState.PENDING)
+      .<VirtualMachine.State, NodeState>builder().put(VirtualMachine.State.STARTING, NodeState.PENDING)
       .put(VirtualMachine.State.RUNNING, NodeState.RUNNING).put(VirtualMachine.State.STOPPING, NodeState.SUSPENDED)
       .put(VirtualMachine.State.STOPPED, NodeState.PENDING)
       .put(VirtualMachine.State.DESTROYED, NodeState.TERMINATED)
@@ -70,18 +75,18 @@ public class VirtualMachineToNodeMetadata implements Function<VirtualMachine, No
    private final FindLocationForVirtualMachine findLocationForVirtualMachine;
    private final FindHardwareForVirtualMachine findHardwareForVirtualMachine;
    private final FindImageForVirtualMachine findImageForVirtualMachine;
-   private final Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine;
+   private final Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine;
 
    @Inject
    VirtualMachineToNodeMetadata(FindLocationForVirtualMachine findLocationForVirtualMachine,
                                 FindHardwareForVirtualMachine findHardwareForVirtualMachine,
                                 FindImageForVirtualMachine findImageForVirtualMachine,
-         Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine) {
+                                Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine) {
       this.findLocationForVirtualMachine = checkNotNull(findLocationForVirtualMachine, "findLocationForVirtualMachine");
       this.findHardwareForVirtualMachine = checkNotNull(findHardwareForVirtualMachine, "findHardwareForVirtualMachine");
       this.findImageForVirtualMachine = checkNotNull(findImageForVirtualMachine, "findImageForVirtualMachine");
-      this.getIPForwardingRuleByVirtualMachine = checkNotNull(getIPForwardingRuleByVirtualMachine,
+      this.getIPForwardingRulesByVirtualMachine = checkNotNull(getIPForwardingRulesByVirtualMachine,
-            "getIPForwardingRuleByVirtualMachine");
+         "getIPForwardingRulesByVirtualMachine");
    }
 
    @Override
@@ -108,15 +113,27 @@ public class VirtualMachineToNodeMetadata implements Function<VirtualMachine, No
       // TODO: check to see public or private
       if (from.getIPAddress() != null) {
          boolean isPrivate = InetAddresses2.isPrivateIPAddress(from.getIPAddress());
-         Set<String> addresses = ImmutableSet.<String> of(from.getIPAddress());
+         Set<String> addresses = ImmutableSet.<String>of(from.getIPAddress());
          if (isPrivate)
             builder.privateAddresses(addresses);
          else
             builder.publicAddresses(addresses);
       }
       try {
-         IPForwardingRule rule = getIPForwardingRuleByVirtualMachine.getUnchecked(from.getId());
+
-         builder.publicAddresses(ImmutableSet.<String> of(rule.getIPAddress()));
+         builder.publicAddresses(transform(filter(getIPForwardingRulesByVirtualMachine.getUnchecked(from.getId()),
+            new Predicate<IPForwardingRule>() {
+               @Override
+               public boolean apply(@Nullable IPForwardingRule rule) {
+                  return !"Deleting".equals(rule.getState());
+               }
+            }),
+            new Function<IPForwardingRule, String>() {
+               @Override
+               public String apply(@Nullable IPForwardingRule rule) {
+                  return rule.getIPAddress();
+               }
+            }));
       } catch (UncheckedExecutionException e) {
          if (Throwables2.getFirstThrowableOfType(e, ResourceNotFoundException.class) == null) {
             Throwables.propagateIfPossible(e.getCause());

apis/cloudstack/src/main/java/org/jclouds/cloudstack/compute/strategy/CloudStackComputeServiceAdapter.java

@@ -29,6 +29,7 @@ import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStatic
 import static org.jclouds.cloudstack.predicates.TemplatePredicates.isReady;
 
 import java.util.Map;
+import java.util.Set;
 import java.util.concurrent.ExecutionException;
 
 import javax.annotation.Resource;
@@ -214,9 +215,11 @@ public class CloudStackComputeServiceAdapter implements
       long guestId = Long.parseLong(id);
       Long job = client.getVirtualMachineClient().destroyVirtualMachine(guestId);
       boolean completed = jobComplete.apply(job);
-      IPForwardingRule forwardingRule = client.getNATClient().getIPForwardingRuleForVirtualMachine(guestId);
+      Set<IPForwardingRule> forwardingRules = client.getNATClient().getIPForwardingRulesForVirtualMachine(guestId);
-      if (forwardingRule != null)
+      for(IPForwardingRule rule : forwardingRules) {
-         client.getNATClient().disableStaticNat(forwardingRule.getIPAddressId());
+         job = client.getNATClient().deleteIPForwardingRule(rule.getId());
+         jobComplete.apply(job);
+      }
    }
 
    @Override

apis/cloudstack/src/main/java/org/jclouds/cloudstack/features/NATAsyncClient.java

@@ -78,26 +78,24 @@ public interface NATAsyncClient {
    ListenableFuture<IPForwardingRule> getIPForwardingRule(@QueryParam("id") long id);
 
    /**
-    * @see NATClient#getIPForwardingRuleForIPAddress
+    * @see NATClient#getIPForwardingRulesForIPAddress
     */
    @GET
    @QueryParams(keys = "command", values = "listIpForwardingRules")
    @SelectJson("ipforwardingrule")
-   @OnlyElement
    @Consumes(MediaType.APPLICATION_JSON)
    @ExceptionParser(ReturnNullOnNotFoundOr404.class)
-   ListenableFuture<IPForwardingRule> getIPForwardingRuleForIPAddress(@QueryParam("ipaddressid") long id);
+   ListenableFuture<Set<IPForwardingRule>> getIPForwardingRulesForIPAddress(@QueryParam("ipaddressid") long id);
 
    /**
-    * @see NATClient#getIPForwardingRuleForVirtualMachine
+    * @see NATClient#getIPForwardingRulesForVirtualMachine
     */
    @GET
    @QueryParams(keys = "command", values = "listIpForwardingRules")
    @SelectJson("ipforwardingrule")
-   @OnlyElement
    @Consumes(MediaType.APPLICATION_JSON)
    @ExceptionParser(ReturnNullOnNotFoundOr404.class)
-   ListenableFuture<IPForwardingRule> getIPForwardingRuleForVirtualMachine(@QueryParam("virtualmachineid") long id);
+   ListenableFuture<Set<IPForwardingRule>> getIPForwardingRulesForVirtualMachine(@QueryParam("virtualmachineid") long id);
 
    /**
     * @see NATClient#createIPForwardingRule

apis/cloudstack/src/main/java/org/jclouds/cloudstack/features/NATClient.java

@@ -59,22 +59,22 @@ public interface NATClient {
    IPForwardingRule getIPForwardingRule(long id);
 
    /**
-    * get a specific IPForwardingRule by ipaddress id
+    * get a set of IPForwardingRules by ipaddress id
     * 
     * @param id
     *           IPAddress of rule to get
-    * @return IPForwardingRule or null if not found
+    * @return IPForwardingRule matching query or empty if not found
     */
-   IPForwardingRule getIPForwardingRuleForIPAddress(long id);
+   Set<IPForwardingRule> getIPForwardingRulesForIPAddress(long id);
 
    /**
-    * get a specific IPForwardingRule by virtual machine id
+    * get a set of IPForwardingRules by virtual machine id
     * 
     * @param id
     *           virtual machine of rule to get
-    * @return IPForwardingRule or null if not found
+    * @return IPForwardingRule matching query or empty set if not found
     */
-   IPForwardingRule getIPForwardingRuleForVirtualMachine(long id);
+   Set<IPForwardingRule> getIPForwardingRulesForVirtualMachine(long id);
 
    /**
     * Creates an ip forwarding rule

apis/cloudstack/src/main/java/org/jclouds/cloudstack/functions/StaticNATVirtualMachineInNetwork.java

@@ -24,6 +24,7 @@ import static com.google.common.base.Preconditions.checkState;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 
+import com.google.common.collect.ImmutableSet;
 import org.jclouds.cloudstack.CloudStackClient;
 import org.jclouds.cloudstack.domain.AsyncCreateResponse;
 import org.jclouds.cloudstack.domain.AsyncJob;
@@ -37,6 +38,8 @@ import com.google.common.base.Predicate;
 import com.google.common.cache.Cache;
 import com.google.inject.assistedinject.Assisted;
 
+import java.util.Set;
+
 /**
  * 
  * @author Adrian Cole
@@ -51,17 +54,17 @@ public class StaticNATVirtualMachineInNetwork implements Function<VirtualMachine
    private final ReuseOrAssociateNewPublicIPAddress reuseOrAssociate;
    private final Network network;
    private final Predicate<Long> jobComplete;
-   private final Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine;
+   private final Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine;
 
    @Inject
    public StaticNATVirtualMachineInNetwork(CloudStackClient client,
          ReuseOrAssociateNewPublicIPAddress reuseOrAssociate, Predicate<Long> jobComplete,
-         Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine, @Assisted Network network) {
+         Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine, @Assisted Network network) {
       this.client = checkNotNull(client, "client");
       this.reuseOrAssociate = checkNotNull(reuseOrAssociate, "reuseOrAssociate");
       this.jobComplete = checkNotNull(jobComplete, "jobComplete");
-      this.getIPForwardingRuleByVirtualMachine = checkNotNull(getIPForwardingRuleByVirtualMachine,
+      this.getIPForwardingRulesByVirtualMachine = checkNotNull(getIPForwardingRulesByVirtualMachine,
-            "getIPForwardingRuleByVirtualMachine");
+            "getIPForwardingRulesByVirtualMachine");
       this.network = checkNotNull(network, "network");
    }
 
@@ -86,7 +89,7 @@ public class StaticNATVirtualMachineInNetwork implements Function<VirtualMachine
       checkState(jobComplete.apply(job.getJobId()), "Timeout creating IP forwarding rule: ", job);
       AsyncJob<IPForwardingRule> response = client.getAsyncJobClient().getAsyncJob(job.getJobId());
       checkState(response.getResult() != null, "No result after creating IP forwarding rule: ", response);
-      getIPForwardingRuleByVirtualMachine.asMap().put(vm.getId(), response.getResult());
+      getIPForwardingRulesByVirtualMachine.asMap().put(vm.getId(), ImmutableSet.of(response.getResult()));
       return ip;
    }
 }

apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/CloudStackComputeServiceAdapterLiveTest.java

@@ -18,6 +18,7 @@
  */
 package org.jclouds.cloudstack.compute;
 
+import static com.google.common.collect.Iterables.getFirst;
 import static com.google.inject.name.Names.bindProperties;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertFalse;
@@ -26,12 +27,13 @@ import static org.testng.Assert.fail;
 import java.io.IOException;
 import java.util.Map;
 import java.util.Random;
+import java.util.Set;
 import java.util.concurrent.TimeUnit;
 
 import javax.inject.Singleton;
 
 import org.jclouds.cloudstack.CloudStackClient;
-import org.jclouds.cloudstack.compute.config.CloudStackComputeServiceContextModule.GetIPForwardingRuleByVirtualMachine;
+import org.jclouds.cloudstack.compute.config.CloudStackComputeServiceContextModule.GetIPForwardingRulesByVirtualMachine;
 import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
 import org.jclouds.cloudstack.compute.strategy.CloudStackComputeServiceAdapter;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
@@ -117,8 +119,8 @@ public class CloudStackComputeServiceAdapterLiveTest extends BaseCloudStackClien
          @SuppressWarnings("unused")
          @Provides
          @Singleton
-         protected Cache<Long, IPForwardingRule> getIPForwardingRuleByVirtualMachine(
+         protected Cache<Long, Set<IPForwardingRule>> getIPForwardingRuleByVirtualMachine(
-               GetIPForwardingRuleByVirtualMachine getIPForwardingRule) {
+               GetIPForwardingRulesByVirtualMachine getIPForwardingRule) {
             return CacheBuilder.newBuilder().build(getIPForwardingRule);
          }
       };
@@ -160,7 +162,8 @@ public class CloudStackComputeServiceAdapterLiveTest extends BaseCloudStackClien
       assertEquals(vm.getNode().getDisplayName(), name);
       // check to see if we setup a NAT rule (conceding we could check this from
       // cache)
-      IPForwardingRule rule = client.getNATClient().getIPForwardingRuleForVirtualMachine(vm.getNode().getId());
+      IPForwardingRule rule = getFirst(
+         client.getNATClient().getIPForwardingRulesForVirtualMachine(vm.getNode().getId()), null);
 
       String address = rule != null ? rule.getIPAddress() : vm.getNode().getIPAddress();
 

apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/functions/VirtualMachineToNodeMetadataTest.java

@@ -64,12 +64,12 @@ public class VirtualMachineToNodeMetadataTest {
             .<Image> of(TemplateToImageTest.one, TemplateToImageTest.two));
       VirtualMachineToNodeMetadata parser = new VirtualMachineToNodeMetadata(new FindLocationForVirtualMachine(
             locationSupplier), new FindHardwareForVirtualMachine(hardwareSupplier), new FindImageForVirtualMachine(
-            imageSupplier), CacheBuilder.newBuilder().<Long, IPForwardingRule> build(
+            imageSupplier), CacheBuilder.newBuilder().<Long, Set<IPForwardingRule>> build(
-            new CacheLoader<Long, IPForwardingRule>() {
+            new CacheLoader<Long, Set<IPForwardingRule>>() {
 
                @Override
-               public IPForwardingRule load(Long arg0) throws Exception {
+               public Set<IPForwardingRule> load(Long arg0) throws Exception {
-                  return IPForwardingRule.builder().id(1234l).IPAddress("1.1.1.1").build();
+                  return ImmutableSet.of(IPForwardingRule.builder().id(1234l).IPAddress("1.1.1.1").build());
                }
 
             }));
@@ -102,11 +102,11 @@ public class VirtualMachineToNodeMetadataTest {
             .<Image> of(TemplateToImageTest.one, TemplateToImageTest.two));
       VirtualMachineToNodeMetadata parser = new VirtualMachineToNodeMetadata(new FindLocationForVirtualMachine(
             locationSupplier), new FindHardwareForVirtualMachine(hardwareSupplier), new FindImageForVirtualMachine(
-            imageSupplier), CacheBuilder.newBuilder().<Long, IPForwardingRule> build(
+            imageSupplier), CacheBuilder.newBuilder().<Long, Set<IPForwardingRule>> build(
-            new CacheLoader<Long, IPForwardingRule>() {
+            new CacheLoader<Long, Set<IPForwardingRule>>() {
 
                @Override
-               public IPForwardingRule load(Long arg0) throws Exception {
+               public Set<IPForwardingRule> load(Long arg0) throws Exception {
                   throw new ResourceNotFoundException("no ip forwarding rule for: " + arg0);
                }
 

apis/cloudstack/src/test/java/org/jclouds/cloudstack/features/NetworkClientLiveTest.java

@@ -115,7 +115,7 @@ public class NetworkClientLiveTest extends BaseCloudStackClientLiveTest {
                .getNetworkClient()
                // startIP/endIP/netmask/gateway must be specified together
                .createNetworkInZone(zone.getId(), offering.getId(), name, name,
-                     vlan("2").startIP("192.168.1.2").netmask("255.255.255.0").gateway("192.168.1.1"));
+                     vlan("65").startIP("192.168.1.2").netmask("255.255.255.0").gateway("192.168.1.1"));
          checkNetwork(network);
       } catch (IllegalStateException e) {
          Logger.getAnonymousLogger().log(Level.SEVERE, "couldn't create a network, skipping test", e);
@@ -173,7 +173,7 @@ public class NetworkClientLiveTest extends BaseCloudStackClientLiveTest {
          break;
       case DIRECT:
          // TODO: I've found a network that doesn't have a netmask associated
-         // assert network.getNetmask() != null : network;
+         assert network.getNetmask() != null : network;
          assert network.getGateway() != null : network;
          assert network.getVLAN() != null : network;
          assertEquals(network.getBroadcastURI(), URI.create("vlan://" + network.getVLAN()));

apis/cloudstack/src/test/java/org/jclouds/cloudstack/functions/StaticNATVirtualMachineInNetworkLiveTest.java

@@ -18,12 +18,16 @@
  */
 package org.jclouds.cloudstack.functions;
 
+import static com.google.common.collect.Iterables.filter;
 import static com.google.common.collect.Iterables.find;
+import static com.google.common.collect.Iterables.getOnlyElement;
 import static org.testng.Assert.assertEquals;
 
 import java.util.NoSuchElementException;
+import java.util.Set;
 
-import org.jclouds.cloudstack.compute.config.CloudStackComputeServiceContextModule.GetIPForwardingRuleByVirtualMachine;
+import com.google.common.base.Predicate;
+import org.jclouds.cloudstack.compute.config.CloudStackComputeServiceContextModule.GetIPForwardingRulesByVirtualMachine;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
@@ -41,6 +45,8 @@ import org.testng.annotations.Test;
 
 import com.google.common.cache.CacheBuilder;
 
+import javax.annotation.Nullable;
+
 /**
  * Tests behavior of {@code StaticNATVirtualMachineInNetwork}
  * 
@@ -75,9 +81,15 @@ public class StaticNATVirtualMachineInNetworkLiveTest extends NATClientLiveTest
       if (networksDisabled)
          return;
       ip = new StaticNATVirtualMachineInNetwork(client, reuseOrAssociate, jobComplete, CacheBuilder.newBuilder()
-            .<Long, IPForwardingRule> build(new GetIPForwardingRuleByVirtualMachine(client)), network).apply(vm);
+            .<Long, Set<IPForwardingRule>>build(new GetIPForwardingRulesByVirtualMachine(client)), network).apply(vm);
 
-      rule = client.getNATClient().getIPForwardingRuleForIPAddress(ip.getId());
+      rule = getOnlyElement(filter(client.getNATClient().getIPForwardingRulesForIPAddress(ip.getId()),
+         new Predicate<IPForwardingRule>() {
+            @Override
+            public boolean apply(@Nullable IPForwardingRule rule) {
+               return rule != null && rule.getStartPort() == 22;
+            }
+         }));
       assertEquals(rule.getIPAddressId(), ip.getId());
       assertEquals(rule.getVirtualMachineId(), vm.getId());
       assertEquals(rule.getStartPort(), 22);