updated cloudstack to setup multiple port forwarding rules so that inboundPorts Template Option can operate

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

@@ -21,16 +21,15 @@ package org.jclouds.cloudstack.compute.strategy;
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Predicates.and;
-import static com.google.common.base.Throwables.propagate;
 import static com.google.common.collect.Iterables.filter;
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static org.jclouds.cloudstack.options.DeployVirtualMachineOptions.Builder.displayName;
 import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
 import static org.jclouds.cloudstack.predicates.TemplatePredicates.isReady;
 
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.concurrent.ExecutionException;
 
 import javax.annotation.Resource;
 import javax.inject.Inject;
@@ -40,7 +39,6 @@ import javax.inject.Singleton;
 import org.jclouds.cloudstack.CloudStackClient;
 import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
 import org.jclouds.cloudstack.domain.AsyncCreateResponse;
-import org.jclouds.cloudstack.domain.AsyncJob;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
@@ -48,9 +46,11 @@ import org.jclouds.cloudstack.domain.ServiceOffering;
 import org.jclouds.cloudstack.domain.Template;
 import org.jclouds.cloudstack.domain.VirtualMachine;
 import org.jclouds.cloudstack.domain.Zone;
+import org.jclouds.cloudstack.functions.CreatePortForwardingRulesForIP;
 import org.jclouds.cloudstack.functions.StaticNATVirtualMachineInNetwork;
 import org.jclouds.cloudstack.functions.StaticNATVirtualMachineInNetwork.Factory;
 import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
 import org.jclouds.collect.Memoized;
 import org.jclouds.compute.ComputeService;
 import org.jclouds.compute.ComputeServiceAdapter;
@@ -61,6 +61,11 @@ import org.jclouds.logging.Logger;
 
 import com.google.common.base.Predicate;
 import com.google.common.base.Supplier;
+import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
+import com.google.common.collect.Sets;
+import com.google.common.primitives.Ints;
 
 /**
  * defines the connection between the {@link CloudStackClient} implementation
@@ -78,17 +83,27 @@ public class CloudStackComputeServiceAdapter implements
    private final CloudStackClient client;
    private final Predicate<Long> jobComplete;
    private final Supplier<Map<Long, Network>> networkSupplier;
+   private final BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult;
    private final Factory staticNATVMInNetwork;
+   private final CreatePortForwardingRulesForIP setupPortForwardingRulesForIP;
+   private final Cache<Long, Set<IPForwardingRule>> vmToRules;
    private final Map<String, Credentials> credentialStore;
 
    @Inject
    public CloudStackComputeServiceAdapter(CloudStackClient client, Predicate<Long> jobComplete,
          @Memoized Supplier<Map<Long, Network>> networkSupplier,
-         StaticNATVirtualMachineInNetwork.Factory staticNATVMInNetwork, Map<String, Credentials> credentialStore) {
+         BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult,
+         StaticNATVirtualMachineInNetwork.Factory staticNATVMInNetwork,
+         CreatePortForwardingRulesForIP setupPortForwardingRulesForIP, Cache<Long, Set<IPForwardingRule>> vmToRules,
+         Map<String, Credentials> credentialStore) {
       this.client = checkNotNull(client, "client");
       this.jobComplete = checkNotNull(jobComplete, "jobComplete");
       this.networkSupplier = checkNotNull(networkSupplier, "networkSupplier");
+      this.blockUntilJobCompletesAndReturnResult = checkNotNull(blockUntilJobCompletesAndReturnResult,
+            "blockUntilJobCompletesAndReturnResult");
       this.staticNATVMInNetwork = checkNotNull(staticNATVMInNetwork, "staticNATVMInNetwork");
+      this.setupPortForwardingRulesForIP = checkNotNull(setupPortForwardingRulesForIP, "setupPortForwardingRulesForIP");
+      this.vmToRules = checkNotNull(vmToRules, "vmToRules");
       this.credentialStore = checkNotNull(credentialStore, "credentialStore");
    }
 
@@ -154,15 +169,7 @@ public class CloudStackComputeServiceAdapter implements
             zoneId, options);
       AsyncCreateResponse job = client.getVirtualMachineClient().deployVirtualMachineInZone(zoneId, serviceOfferingId,
             templateId, options);
-      boolean completed = jobComplete.apply(job.getJobId());
+      VirtualMachine vm = blockUntilJobCompletesAndReturnResult.<VirtualMachine> apply(job);
-      AsyncJob<VirtualMachine> jobWithResult = client.getAsyncJobClient().<VirtualMachine> getAsyncJob(job.getJobId());
-      assert completed : jobWithResult;
-      if (jobWithResult.getError() != null)
-         propagate(new ExecutionException(String.format("job %s failed with exception %s", job.getId(), jobWithResult
-               .getError().toString())) {
-            private static final long serialVersionUID = 4371112085613620239L;
-         });
-      VirtualMachine vm = jobWithResult.getResult();
       LoginCredentials credentials = null;
       if (vm.isPasswordEnabled()) {
          assert vm.getPassword() != null : vm;
@@ -173,8 +180,13 @@ public class CloudStackComputeServiceAdapter implements
       if (templateOptions.shouldSetupStaticNat()) {
          // TODO: possibly not all network ids, do we want to do this
          for (long networkId : options.getNetworkIds()) {
-            // TODO: log this
+            logger.debug(">> creating static NAT for virtualMachine(%s) in network(%s)", vm.getId(), networkId);
             PublicIPAddress ip = staticNATVMInNetwork.create(networks.get(networkId)).apply(vm);
+            logger.trace("<< static NATed IPAddress(%s) to virtualMachine(%s)", ip.getId(), vm.getId());
+            List<Integer> ports = Ints.asList(templateOptions.getInboundPorts());
+            logger.debug(">> setting up IP forwarding for IPAddress(%s) rules(%s)", ip.getId(), ports);
+            Set<IPForwardingRule> rules = setupPortForwardingRulesForIP.apply(ip, ports);
+            logger.trace("<< setup %d IP forwarding rules on IPAddress(%s)", rules.size(), ip.getId());
          }
       }
       return new NodeAndInitialCredentials<VirtualMachine>(vm, vm.getId() + "", credentials);
@@ -206,62 +218,99 @@ public class CloudStackComputeServiceAdapter implements
 
    @Override
    public VirtualMachine getNode(String id) {
-      long guestId = Long.parseLong(id);
+      long virtualMachineId = Long.parseLong(id);
-      return client.getVirtualMachineClient().getVirtualMachine(guestId);
+      return client.getVirtualMachineClient().getVirtualMachine(virtualMachineId);
    }
 
    @Override
    public void destroyNode(String id) {
-      long guestId = Long.parseLong(id);
+      long virtualMachineId = Long.parseLong(id);
-      Long job = client.getVirtualMachineClient().destroyVirtualMachine(guestId);
+      Builder<Long> jobsToTrack = ImmutableSet.<Long> builder();
-      if (job != null) {
+      
-         logger.debug(">> destroying virtualMachine(%s)", guestId);
+      Set<Long> ipAddresses = deleteIPForwardingRulesForVMAndReturnDistinctIPs(virtualMachineId, jobsToTrack);
-         boolean completed = jobComplete.apply(job);
+      disableStaticNATOnIPAddresses(jobsToTrack, ipAddresses);
-         logger.trace("<< virtualMachine(%s) destroyed(%s)", guestId, completed);
+      destroyVirtualMachine(virtualMachineId, jobsToTrack);
-         Set<IPForwardingRule> forwardingRules = client.getNATClient().getIPForwardingRulesForVirtualMachine(guestId);
+      
-         for (IPForwardingRule rule : forwardingRules) {
+      ImmutableSet<Long> jobs = jobsToTrack.build();
-            deleteIPForwardingRuleAndDisableStaticNAT(rule);
+      logger.debug(">> awaiting completion of jobs(%s)", jobs);
+      for (long job : jobsToTrack.build())
+         awaitCompletion(job);
+      logger.trace("<< completed jobs(%s)", jobs);
+      vmToRules.invalidate(virtualMachineId);
    }
+
+   public void destroyVirtualMachine(long virtualMachineId, Builder<Long> jobsToTrack) {
+      Long destroyVirtualMachine = client.getVirtualMachineClient().destroyVirtualMachine(virtualMachineId);
+      if (destroyVirtualMachine != null) {
+         logger.debug(">> destroying virtualMachine(%s) job(%s)", virtualMachineId, destroyVirtualMachine);
+         jobsToTrack.add(destroyVirtualMachine);
       } else {
-         logger.trace("<< virtualMachine(%s) not found", guestId);
+         logger.trace("<< virtualMachine(%s) not found", virtualMachineId);
       }
    }
 
-   public void deleteIPForwardingRuleAndDisableStaticNAT(IPForwardingRule rule) {
+   public void disableStaticNATOnIPAddresses(Builder<Long> jobsToTrack, Set<Long> ipAddresses) {
-      Long job = client.getNATClient().deleteIPForwardingRule(rule.getId());
+      for (Long ipAddress : ipAddresses) {
-      if (job != null) {
+         Long disableStaticNAT = client.getNATClient().disableStaticNATOnPublicIP(ipAddress);
-         logger.debug(">> deleting IPForwardingRule(%s)", rule.getId());
+         if (disableStaticNAT != null) {
-         boolean completed = jobComplete.apply(job);
+            logger.debug(">> disabling static NAT IPAddress(%s) job(%s)", ipAddress, disableStaticNAT);
-         logger.trace("<< IPForwardingRule(%s) deleted(%s)", rule.getId(), completed);
+            jobsToTrack.add(disableStaticNAT);
-         disableStaticNATOnPublicIP(rule.getIPAddressId());
+         }
       }
    }
 
-   public void disableStaticNATOnPublicIP(Long IPAddressId) {
+   public Set<Long> deleteIPForwardingRulesForVMAndReturnDistinctIPs(long virtualMachineId, Builder<Long> jobsToTrack) {
-      Long job = client.getNATClient().disableStaticNATOnPublicIP(IPAddressId);
+      // immutable doesn't permit duplicates
-      if (job != null) {
+      Set<Long> ipAddresses = Sets.newLinkedHashSet();
-         logger.debug(">> disabling static nat IPAddress(%s)", IPAddressId);
+
-         boolean completed = jobComplete.apply(job);
+      Set<IPForwardingRule> forwardingRules = client.getNATClient().getIPForwardingRulesForVirtualMachine(
-         logger.trace("<< IPAddress(%s) static nat disabled(%s)", IPAddressId, completed);
+            virtualMachineId);
+      for (IPForwardingRule rule : forwardingRules) {
+         if (!"Deleting".equals(rule.getState())) {
+            ipAddresses.add(rule.getIPAddressId());
+            Long deleteForwardingRule = client.getNATClient().deleteIPForwardingRule(rule.getId());
+            if (deleteForwardingRule != null) {
+               logger.debug(">> deleting IPForwardingRule(%s) job(%s)", rule.getId(), deleteForwardingRule);
+               jobsToTrack.add(deleteForwardingRule);
             }
          }
+      }
+      return ipAddresses;
+   }
+
+   public void awaitCompletion(long job) {
+      boolean completed = jobComplete.apply(job);
+      logger.trace("<< job(%s) complete(%s)", job, completed);
+   }
 
    @Override
    public void rebootNode(String id) {
-      Long job = client.getVirtualMachineClient().rebootVirtualMachine(Long.parseLong(id));
+      long virtualMachineId = Long.parseLong(id);
-      boolean completed = jobComplete.apply(job);
+      Long job = client.getVirtualMachineClient().rebootVirtualMachine(virtualMachineId);
+      if (job != null) {
+         logger.debug(">> rebooting virtualMachine(%s) job(%s)", virtualMachineId, job);
+         awaitCompletion(job);
+      }
    }
 
    @Override
    public void resumeNode(String id) {
+      long virtualMachineId = Long.parseLong(id);
       Long job = client.getVirtualMachineClient().startVirtualMachine(Long.parseLong(id));
-      boolean completed = jobComplete.apply(job);
+      if (job != null) {
+         logger.debug(">> starting virtualMachine(%s) job(%s)", virtualMachineId, job);
+         awaitCompletion(job);
+      }
    }
 
    @Override
    public void suspendNode(String id) {
+      long virtualMachineId = Long.parseLong(id);
       Long job = client.getVirtualMachineClient().stopVirtualMachine(Long.parseLong(id));
-      boolean completed = jobComplete.apply(job);
+      if (job != null) {
+         logger.debug(">> stopping virtualMachine(%s) job(%s)", virtualMachineId, job);
+         awaitCompletion(job);
+      }
    }
 
 }

apis/cloudstack/src/main/java/org/jclouds/cloudstack/domain/AsyncCreateResponse.java

@@ -25,9 +25,11 @@ import com.google.gson.annotations.SerializedName;
  * @author Adrian Cole
  */
 public class AsyncCreateResponse {
-   private long id;
+   public static final AsyncCreateResponse UNINITIALIZED = new AsyncCreateResponse();
+   
+   private long id = -1;
    @SerializedName("jobid")
-   private long jobId;
+   private long jobId = -1;
 
    /**
     * present only for serializer

apis/cloudstack/src/main/java/org/jclouds/cloudstack/domain/PublicIPAddress.java

@@ -49,7 +49,7 @@ public class PublicIPAddress implements Comparable<PublicIPAddress> {
       private long networkId;
       private State state;
       private String virtualMachineDisplayName;
-      private long virtualMachineId;
+      private long virtualMachineId = -1;
       private String virtualMachineName;
       private long VLANId;
       private String VLANName;
@@ -180,7 +180,7 @@ public class PublicIPAddress implements Comparable<PublicIPAddress> {
    @SerializedName("virtualmachinedisplayname")
    private String virtualMachineDisplayName;
    @SerializedName("virtualmachineid")
-   private long virtualMachineId;
+   private long virtualMachineId = -1;
    @SerializedName("virtualmachinename")
    private String virtualMachineName;
    @SerializedName("VLANid")

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

@@ -0,0 +1,93 @@
+/**
+ * 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.cloudstack.functions;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
+import java.util.Set;
+
+import javax.annotation.Resource;
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.jclouds.cloudstack.CloudStackClient;
+import org.jclouds.cloudstack.domain.AsyncCreateResponse;
+import org.jclouds.cloudstack.domain.IPForwardingRule;
+import org.jclouds.cloudstack.domain.PublicIPAddress;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
+import org.jclouds.compute.reference.ComputeServiceConstants;
+import org.jclouds.logging.Logger;
+
+import com.google.common.cache.Cache;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSet.Builder;
+import com.google.common.collect.Iterables;
+
+/**
+ * 
+ * @author Adrian Cole
+ */
+@Singleton
+public class CreatePortForwardingRulesForIP {
+
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
+   private final CloudStackClient client;
+   private final BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult;
+   private final Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine;
+
+   @Inject
+   public CreatePortForwardingRulesForIP(CloudStackClient client,
+         BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult,
+         Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine) {
+      this.client = checkNotNull(client, "client");
+      this.blockUntilJobCompletesAndReturnResult = checkNotNull(blockUntilJobCompletesAndReturnResult,
+            "blockUntilJobCompletesAndReturnResult");
+      this.getIPForwardingRulesByVirtualMachine = checkNotNull(getIPForwardingRulesByVirtualMachine,
+            "getIPForwardingRulesByVirtualMachine");
+   }
+
+   public Set<IPForwardingRule> apply(PublicIPAddress ip, Iterable<Integer> ports) {
+      return apply(ip, "tcp", ports);
+   }
+
+   public Set<IPForwardingRule> apply(PublicIPAddress ip, String protocol, Iterable<Integer> ports) {
+      checkState(ip.getVirtualMachineId() != -1, "ip should be static NATed to a virtual machine");
+      if (Iterables.size(ports) == 0)
+         return ImmutableSet.<IPForwardingRule> of();
+      Builder<AsyncCreateResponse> responses = ImmutableSet.<AsyncCreateResponse> builder();
+      for (int port : ports) {
+         AsyncCreateResponse response = client.getNATClient().createIPForwardingRule(ip.getId(), protocol, port);
+         logger.debug(">> creating IP forwarding rule IPAddress(%s) for protocol(%s), port(%s); response(%s)",
+               ip.getId(), protocol, port, response);
+         responses.add(response);
+      }
+      Builder<IPForwardingRule> rules = ImmutableSet.<IPForwardingRule> builder();
+      for (AsyncCreateResponse response : responses.build()) {
+         IPForwardingRule rule = blockUntilJobCompletesAndReturnResult.<IPForwardingRule> apply(response);
+         rules.add(rule);
+         getIPForwardingRulesByVirtualMachine.asMap().put(ip.getVirtualMachineId(), ImmutableSet.of(rule));
+      }
+      return rules.build();
+   }
+}

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

@@ -19,7 +19,6 @@
 package org.jclouds.cloudstack.functions;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 import static com.google.common.base.Predicates.and;
 import static com.google.common.collect.Iterables.find;
 import static org.jclouds.cloudstack.options.AssociateIPAddressOptions.Builder.networkId;
@@ -40,10 +39,10 @@ import org.jclouds.cloudstack.domain.AsyncCreateResponse;
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
 import org.jclouds.cloudstack.features.AddressClient;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
 import org.jclouds.logging.Logger;
 
 import com.google.common.base.Function;
-import com.google.common.base.Predicate;
 
 /**
  * 
@@ -52,15 +51,17 @@ import com.google.common.base.Predicate;
 @Singleton
 public class ReuseOrAssociateNewPublicIPAddress implements Function<Network, PublicIPAddress> {
    private final CloudStackClient client;
-   private final Predicate<Long> jobComplete;
+   private final BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult;
    @Resource
    @Named(COMPUTE_LOGGER)
    protected Logger logger = Logger.NULL;
 
    @Inject
-   public ReuseOrAssociateNewPublicIPAddress(CloudStackClient client, Predicate<Long> jobComplete) {
+   public ReuseOrAssociateNewPublicIPAddress(CloudStackClient client,
+         BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult) {
       this.client = checkNotNull(client, "client");
-      this.jobComplete = checkNotNull(jobComplete, "jobComplete");
+      this.blockUntilJobCompletesAndReturnResult = checkNotNull(blockUntilJobCompletesAndReturnResult,
+            "blockUntilJobCompletesAndReturnResult");
    }
 
    /**
@@ -81,11 +82,10 @@ public class ReuseOrAssociateNewPublicIPAddress implements Function<Network, Pub
    }
 
    public static PublicIPAddress associateIPAddressInNetwork(Network network, CloudStackClient client,
-         Predicate<Long> jobComplete) {
+         BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult) {
       AsyncCreateResponse job = client.getAddressClient().associateIPAddressInZone(network.getZoneId(),
             networkId(network.getId()));
-      checkState(jobComplete.apply(job.getJobId()), "job %d failed to complete", job.getJobId());
+      PublicIPAddress ip = blockUntilJobCompletesAndReturnResult.<PublicIPAddress> apply(job);
-      PublicIPAddress ip = client.getAsyncJobClient().<PublicIPAddress> getAsyncJob(job.getJobId()).getResult();
       assert ip.getZoneId() == network.getZoneId();
       return ip;
    }
@@ -93,14 +93,14 @@ public class ReuseOrAssociateNewPublicIPAddress implements Function<Network, Pub
    @Override
    public PublicIPAddress apply(Network input) {
       try {
-         logger.debug(">> looking for existing address in network %d", input.getId());
+         logger.debug(">> looking for existing address in network(%d)", input.getId());
          PublicIPAddress returnVal = findAvailableAndAssociatedWithNetwork(input.getId(), client.getAddressClient());
-         logger.debug("<< address(%d)", returnVal.getId());
+         logger.debug("<< reused address(%d)", returnVal.getId());
          return returnVal;
       } catch (NoSuchElementException e) {
-         logger.debug(">> associating new address in network %d", input.getId());
+         logger.debug(">> associating new address in network(%d)", input.getId());
-         PublicIPAddress returnVal = associateIPAddressInNetwork(input, client, jobComplete);
+         PublicIPAddress returnVal = associateIPAddressInNetwork(input, client, blockUntilJobCompletesAndReturnResult);
-         logger.debug("<< address(%d)", returnVal.getId());
+         logger.debug("<< associated address(%d)", returnVal.getId());
          return returnVal;
       }
    }

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

@@ -19,27 +19,24 @@
 package org.jclouds.cloudstack.functions;
 
 import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.common.base.Preconditions.checkState;
 
+import javax.annotation.Resource;
 import javax.inject.Inject;
+import javax.inject.Named;
 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;
-import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
 import org.jclouds.cloudstack.domain.VirtualMachine;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
+import org.jclouds.compute.reference.ComputeServiceConstants;
+import org.jclouds.logging.Logger;
 
 import com.google.common.base.Function;
-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
@@ -50,21 +47,23 @@ public class StaticNATVirtualMachineInNetwork implements Function<VirtualMachine
       StaticNATVirtualMachineInNetwork create(Network in);
    }
 
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+
    private final CloudStackClient client;
+   private final BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult;
    private final ReuseOrAssociateNewPublicIPAddress reuseOrAssociate;
    private final Network network;
-   private final Predicate<Long> jobComplete;
-   private final Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine;
 
    @Inject
    public StaticNATVirtualMachineInNetwork(CloudStackClient client,
-         ReuseOrAssociateNewPublicIPAddress reuseOrAssociate, Predicate<Long> jobComplete,
+         BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult,
-         Cache<Long, Set<IPForwardingRule>> getIPForwardingRulesByVirtualMachine, @Assisted Network network) {
+         ReuseOrAssociateNewPublicIPAddress reuseOrAssociate, @Assisted Network network) {
       this.client = checkNotNull(client, "client");
+      this.blockUntilJobCompletesAndReturnResult = checkNotNull(blockUntilJobCompletesAndReturnResult,
+            "blockUntilJobCompletesAndReturnResult");
       this.reuseOrAssociate = checkNotNull(reuseOrAssociate, "reuseOrAssociate");
-      this.jobComplete = checkNotNull(jobComplete, "jobComplete");
-      this.getIPForwardingRulesByVirtualMachine = checkNotNull(getIPForwardingRulesByVirtualMachine,
-            "getIPForwardingRulesByVirtualMachine");
       this.network = checkNotNull(network, "network");
    }
 
@@ -76,8 +75,16 @@ public class StaticNATVirtualMachineInNetwork implements Function<VirtualMachine
          if (ip.getVirtualMachineId() > 0 && ip.getVirtualMachineId() != vm.getId())
             continue;
          try {
-            client.getNATClient().enableStaticNATForVirtualMachine(vm.getId(), ip.getId());
+            AsyncCreateResponse response = client.getNATClient().enableStaticNATForVirtualMachine(vm.getId(),
+                  ip.getId());
+            logger.debug(">> static NATing IPAddress(%s) to virtualMachine(%s); response(%s)", ip.getId(), vm.getId(),
+                  response);
+            // cloudstack 2.2.8 doesn't return an async job. replace this with
+            // an assertion when we stop supporting 2.2.8
+            if (AsyncCreateResponse.UNINITIALIZED.equals(response))
                ip = client.getAddressClient().getPublicIPAddress(ip.getId());
+            else
+               ip = blockUntilJobCompletesAndReturnResult.<PublicIPAddress> apply(response);
             if (ip.isStaticNAT() && ip.getVirtualMachineId() == vm.getId())
                break;
          } catch (IllegalStateException e) {
@@ -85,11 +92,6 @@ public class StaticNATVirtualMachineInNetwork implements Function<VirtualMachine
          }
          return ip;
       }
-      AsyncCreateResponse job = client.getNATClient().createIPForwardingRule(ip.getId(), "tcp", 22);
-      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);
-      getIPForwardingRulesByVirtualMachine.asMap().put(vm.getId(), ImmutableSet.of(response.getResult()));
       return ip;
    }
 }

apis/cloudstack/src/main/java/org/jclouds/cloudstack/strategy/BlockUntilJobCompletesAndReturnResult.java

@@ -0,0 +1,77 @@
+/**
+ * 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.cloudstack.strategy;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Preconditions.checkState;
+
+import java.util.concurrent.ExecutionException;
+
+import javax.annotation.Resource;
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import org.jclouds.cloudstack.CloudStackClient;
+import org.jclouds.cloudstack.domain.AsyncCreateResponse;
+import org.jclouds.cloudstack.domain.AsyncJob;
+import org.jclouds.compute.reference.ComputeServiceConstants;
+import org.jclouds.logging.Logger;
+
+import com.google.common.base.Predicate;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+
+/**
+ * @author Adrian Cole
+ */
+@Singleton
+public class BlockUntilJobCompletesAndReturnResult {
+   @Resource
+   @Named(ComputeServiceConstants.COMPUTE_LOGGER)
+   protected Logger logger = Logger.NULL;
+   
+   private final CloudStackClient client;
+   private final Predicate<Long> jobComplete;
+
+   @Inject
+   public BlockUntilJobCompletesAndReturnResult(CloudStackClient client, Predicate<Long> jobComplete) {
+      this.client = checkNotNull(client, "client");
+      this.jobComplete = checkNotNull(jobComplete, "jobComplete");
+   }
+
+   /**
+    * 
+    * @param job
+    * @return result of the job's execution
+    * @throws ExecutionException
+    *            if the job contained an error
+    */
+   public <T> T apply(AsyncCreateResponse job) {
+      boolean completed = jobComplete.apply(job.getJobId());
+      logger.trace("<< job(%s) complete(%s)", job, completed);
+      AsyncJob<T> jobWithResult = client.getAsyncJobClient().<T> getAsyncJob(job.getJobId());
+      checkState(completed, "job %s failed to complete in time %s", job.getJobId(), jobWithResult);
+      if (jobWithResult.getError() != null)
+         throw new UncheckedExecutionException(String.format("job %s failed with exception %s", job.getJobId(),
+               jobWithResult.getError().toString())) {
+            private static final long serialVersionUID = 4371112085613620239L;
+         };
+      return jobWithResult.getResult();
+   }
+}

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

@@ -47,6 +47,7 @@ import org.jclouds.cloudstack.predicates.TemplatePredicates;
 import org.jclouds.cloudstack.predicates.UserPredicates;
 import org.jclouds.cloudstack.predicates.VirtualMachineDestroyed;
 import org.jclouds.cloudstack.predicates.VirtualMachineRunning;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
 import org.jclouds.compute.BaseVersionedServiceLiveTest;
 import org.jclouds.compute.ComputeServiceContextFactory;
 import org.jclouds.compute.domain.ExecResponse;
@@ -210,7 +211,8 @@ public class BaseCloudStackClientLiveTest extends BaseVersionedServiceLiveTest {
       adminVirtualMachineDestroyed = new RetryablePredicate<VirtualMachine>(new VirtualMachineDestroyed(adminClient),
             600, 5, 5, TimeUnit.SECONDS);
       injector.injectMembers(adminVirtualMachineDestroyed);
-      reuseOrAssociate = new ReuseOrAssociateNewPublicIPAddress(client, jobComplete);
+      reuseOrAssociate = new ReuseOrAssociateNewPublicIPAddress(client, new BlockUntilJobCompletesAndReturnResult(
+            client, jobComplete));
       injector.injectMembers(reuseOrAssociate);
    }
 

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

@@ -0,0 +1,117 @@
+/**
+ * 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.cloudstack.functions;
+
+import static org.easymock.EasyMock.expect;
+import static org.easymock.classextension.EasyMock.createMock;
+import static org.easymock.classextension.EasyMock.replay;
+import static org.easymock.classextension.EasyMock.verify;
+import static org.testng.Assert.assertEquals;
+
+import org.jclouds.cloudstack.CloudStackClient;
+import org.jclouds.cloudstack.domain.AsyncCreateResponse;
+import org.jclouds.cloudstack.domain.AsyncJob;
+import org.jclouds.cloudstack.domain.AsyncJobError;
+import org.jclouds.cloudstack.features.AsyncJobClient;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
+import org.testng.annotations.Test;
+
+import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
+import com.google.common.util.concurrent.UncheckedExecutionException;
+
+/**
+ * @author Adrian Cole
+ */
+@Test(groups = "unit", testName = "BlockUntilJobCompletesAndReturnResultTest")
+public class BlockUntilJobCompletesAndReturnResultTest {
+
+   public void testApply() {
+      long id = 1;
+      long jobId = 2;
+
+      CloudStackClient client = createMock(CloudStackClient.class);
+      Predicate<Long> jobComplete = Predicates.alwaysTrue();
+      AsyncJobClient jobClient = createMock(AsyncJobClient.class);
+
+      expect(client.getAsyncJobClient()).andReturn(jobClient).atLeastOnce();
+      expect(jobClient.getAsyncJob(jobId)).andReturn(AsyncJob.builder().id(jobId).result("foo").build()).atLeastOnce();
+
+      replay(client);
+      replay(jobClient);
+
+      assertEquals(
+            new BlockUntilJobCompletesAndReturnResult(client, jobComplete).<String> apply(new AsyncCreateResponse(id,
+                  jobId)), "foo");
+
+      verify(client);
+      verify(jobClient);
+
+   }
+
+   @Test(expectedExceptions = IllegalStateException.class)
+   public void testJobDoesntCompleteThrowsIllegalStateException() {
+      long id = 1;
+      long jobId = 2;
+
+      CloudStackClient client = createMock(CloudStackClient.class);
+      // the alwaysfalse predicate should blow up with IllegalStateException
+      Predicate<Long> jobComplete = Predicates.alwaysFalse();
+      AsyncJobClient jobClient = createMock(AsyncJobClient.class);
+
+      expect(client.getAsyncJobClient()).andReturn(jobClient).atLeastOnce();
+      expect(jobClient.getAsyncJob(jobId)).andReturn(AsyncJob.builder().id(jobId).result("foo").build()).atLeastOnce();
+
+      replay(client);
+      replay(jobClient);
+
+      assertEquals(
+            new BlockUntilJobCompletesAndReturnResult(client, jobComplete).<String> apply(new AsyncCreateResponse(id,
+                  jobId)), "foo");
+
+      verify(client);
+      verify(jobClient);
+
+   }
+
+   @Test(expectedExceptions = UncheckedExecutionException.class)
+   public void testJobWithErrorThrowsUncheckedExecutionException() {
+      long id = 1;
+      long jobId = 2;
+
+      CloudStackClient client = createMock(CloudStackClient.class);
+      Predicate<Long> jobComplete = Predicates.alwaysTrue();
+      AsyncJobClient jobClient = createMock(AsyncJobClient.class);
+
+      expect(client.getAsyncJobClient()).andReturn(jobClient).atLeastOnce();
+      expect(jobClient.getAsyncJob(jobId)).andReturn(
+            AsyncJob.builder().id(jobId).error(new AsyncJobError(1, "ERRROR")).result("foo").build()).atLeastOnce();
+
+      replay(client);
+      replay(jobClient);
+
+      assertEquals(
+            new BlockUntilJobCompletesAndReturnResult(client, jobComplete).<String> apply(new AsyncCreateResponse(id,
+                  jobId)), "foo");
+
+      verify(client);
+      verify(jobClient);
+
+   }
+}

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

@@ -28,15 +28,12 @@ import static org.testng.Assert.assertEquals;
 
 import org.jclouds.cloudstack.CloudStackClient;
 import org.jclouds.cloudstack.domain.AsyncCreateResponse;
-import org.jclouds.cloudstack.domain.AsyncJob;
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
 import org.jclouds.cloudstack.features.AddressClient;
-import org.jclouds.cloudstack.features.AsyncJobClient;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
 import org.testng.annotations.Test;
 
-import com.google.common.base.Predicate;
-import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableSet;
 
 /**
@@ -54,7 +51,7 @@ public class ReuseOrAssociateNewPublicIPAddressTest {
 
       // create mocks
       CloudStackClient client = createMock(CloudStackClient.class);
-      Predicate<Long> jobComplete = Predicates.alwaysTrue();
+      BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult = createMock(BlockUntilJobCompletesAndReturnResult.class);
       AddressClient addressClient = createMock(AddressClient.class);
       expect(client.getAddressClient()).andReturn(addressClient).atLeastOnce();
 
@@ -62,28 +59,25 @@ public class ReuseOrAssociateNewPublicIPAddressTest {
       expect(addressClient.listPublicIPAddresses(allocatedOnly(true).networkId(networkId))).andReturn(
             ImmutableSet.<PublicIPAddress> of(address));
 
-      // replay mocks
       replay(client);
+      replay(blockUntilJobCompletesAndReturnResult);
       replay(addressClient);
 
-      // run
       assertEquals(
-            new ReuseOrAssociateNewPublicIPAddress(client, jobComplete).apply(Network.builder().id(networkId)
+            new ReuseOrAssociateNewPublicIPAddress(client, blockUntilJobCompletesAndReturnResult).apply(Network
-                  .zoneId(zoneId).build()), address);
+                  .builder().id(networkId).zoneId(zoneId).build()), address);
 
-      // verify mocks
       verify(client);
+      verify(blockUntilJobCompletesAndReturnResult);
       verify(addressClient);
 
    }
 
    public void testAssociateWorks() throws SecurityException, NoSuchMethodException {
-      long networkId = 99l;
-      long zoneId = 100l;
 
       // create mocks
       CloudStackClient client = createMock(CloudStackClient.class);
-      Predicate<Long> jobComplete = Predicates.alwaysTrue();
+      BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult = createMock(BlockUntilJobCompletesAndReturnResult.class);
       AddressClient addressClient = createMock(AddressClient.class);
       expect(client.getAddressClient()).andReturn(addressClient).atLeastOnce();
 
@@ -95,61 +89,19 @@ public class ReuseOrAssociateNewPublicIPAddressTest {
       // make sure we created the job relating to a new ip
       expect(addressClient.associateIPAddressInZone(zoneId, networkId(networkId))).andReturn(job);
 
-      AsyncJobClient jobClient = createMock(AsyncJobClient.class);
+      expect(blockUntilJobCompletesAndReturnResult.apply(job)).andReturn(address);
-      expect(client.getAsyncJobClient()).andReturn(jobClient).atLeastOnce();
 
-      expect(jobClient.getAsyncJob(2)).andReturn(AsyncJob.builder().result(address).build());
-
-      // replay mocks
       replay(client);
       replay(addressClient);
-      replay(jobClient);
+      replay(blockUntilJobCompletesAndReturnResult);
 
-      // run
       assertEquals(
-            new ReuseOrAssociateNewPublicIPAddress(client, jobComplete).apply(Network.builder().id(networkId)
+            new ReuseOrAssociateNewPublicIPAddress(client, blockUntilJobCompletesAndReturnResult).apply(Network
-                  .zoneId(zoneId).build()), address);
+                  .builder().id(networkId).zoneId(zoneId).build()), address);
 
-      // verify mocks
-      verify(client);
-      verify(addressClient);
-      verify(jobClient);
-
-   }
-
-   @Test(expectedExceptions = IllegalStateException.class)
-   public void testJobDoesntCompleteThrowsIllegalStateException() throws SecurityException, NoSuchMethodException {
-      long networkId = 99l;
-      long zoneId = 100l;
-
-      // create mocks
-      CloudStackClient client = createMock(CloudStackClient.class);
-      Predicate<Long> jobComplete = Predicates.alwaysFalse();
-      AddressClient addressClient = createMock(AddressClient.class);
-      expect(client.getAddressClient()).andReturn(addressClient).atLeastOnce();
-
-      // no ip addresses available
-      expect(addressClient.listPublicIPAddresses(allocatedOnly(true).networkId(networkId))).andReturn(
-            ImmutableSet.<PublicIPAddress> of());
-
-      AsyncCreateResponse job = new AsyncCreateResponse(1, 2);
-      // make sure we created the job relating to a new ip
-      expect(addressClient.associateIPAddressInZone(zoneId, networkId(networkId))).andReturn(job);
-
-      // the alwaysfalse predicate above should blow up with
-      // IllegalStateException
-
-      // replay mocks
-      replay(client);
-      replay(addressClient);
-
-      // run
-      new ReuseOrAssociateNewPublicIPAddress(client, jobComplete).apply(Network.builder().id(networkId).zoneId(zoneId)
-            .build());
-
-      // verify mocks
       verify(client);
       verify(addressClient);
+      verify(blockUntilJobCompletesAndReturnResult);
 
    }
 

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

@@ -26,7 +26,8 @@ import static org.testng.Assert.assertEquals;
 import java.util.NoSuchElementException;
 import java.util.Set;
 
-import com.google.common.base.Predicate;
+import javax.annotation.Nullable;
+
 import org.jclouds.cloudstack.compute.config.CloudStackComputeServiceContextModule.GetIPForwardingRulesByVirtualMachine;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
@@ -35,6 +36,7 @@ import org.jclouds.cloudstack.domain.VirtualMachine;
 import org.jclouds.cloudstack.features.NATClientLiveTest;
 import org.jclouds.cloudstack.features.VirtualMachineClientLiveTest;
 import org.jclouds.cloudstack.predicates.NetworkPredicates;
+import org.jclouds.cloudstack.strategy.BlockUntilJobCompletesAndReturnResult;
 import org.jclouds.compute.domain.ExecResponse;
 import org.jclouds.domain.Credentials;
 import org.jclouds.net.IPSocket;
@@ -43,9 +45,9 @@ import org.testng.annotations.AfterGroups;
 import org.testng.annotations.BeforeGroups;
 import org.testng.annotations.Test;
 
+import com.google.common.base.Predicate;
 import com.google.common.cache.CacheBuilder;
-
+import com.google.common.collect.ImmutableSet;
-import javax.annotation.Nullable;
 
 /**
  * Tests behavior of {@code StaticNATVirtualMachineInNetwork}
@@ -80,8 +82,21 @@ public class StaticNATVirtualMachineInNetworkLiveTest extends NATClientLiveTest
    public void testCreateIPForwardingRule() throws Exception {
       if (networksDisabled)
          return;
-      ip = new StaticNATVirtualMachineInNetwork(client, reuseOrAssociate, jobComplete, CacheBuilder.newBuilder()
+      BlockUntilJobCompletesAndReturnResult blocker = new BlockUntilJobCompletesAndReturnResult(client, jobComplete);
-            .<Long, Set<IPForwardingRule>>build(new GetIPForwardingRulesByVirtualMachine(client)), network).apply(vm);
+      StaticNATVirtualMachineInNetwork fn = new StaticNATVirtualMachineInNetwork(client, blocker, reuseOrAssociate,
+            network);
+      CreatePortForwardingRulesForIP createPortForwardingRulesForIP = new CreatePortForwardingRulesForIP(client,
+            blocker, CacheBuilder.newBuilder().<Long, Set<IPForwardingRule>> build(
+                  new GetIPForwardingRulesByVirtualMachine(client)));
+
+      // logger
+      injector.injectMembers(blocker);
+      injector.injectMembers(fn);
+      injector.injectMembers(createPortForwardingRulesForIP);
+
+      ip = fn.apply(vm);
+
+      createPortForwardingRulesForIP.apply(ip, ImmutableSet.of(22));
       
       rule = getOnlyElement(filter(client.getNATClient().getIPForwardingRulesForIPAddress(ip.getId()),
             new Predicate<IPForwardingRule>() {

apis/cloudstack/src/test/java/org/jclouds/cloudstack/parse/EnableStaticNATResponseWhereResponseDoesntHaveJobTest.java

@@ -0,0 +1,48 @@
+/**
+ * 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.cloudstack.parse;
+
+import javax.ws.rs.Consumes;
+import javax.ws.rs.core.MediaType;
+
+import org.jclouds.cloudstack.domain.AsyncCreateResponse;
+import org.jclouds.json.BaseItemParserTest;
+import org.jclouds.rest.annotations.Unwrap;
+import org.testng.annotations.Test;
+
+/**
+ * 
+ * @author Adrian Cole
+ */
+@Test(groups = "unit", testName = "EnableStaticNATResponseWhereResponseDoesntHaveJobTest")
+public class EnableStaticNATResponseWhereResponseDoesntHaveJobTest extends BaseItemParserTest<AsyncCreateResponse> {
+
+   @Override
+   public String resource() {
+      return "/enablestaticnatresponse-withoutjob.json";
+   }
+
+   @Override
+   @Unwrap
+   @Consumes(MediaType.APPLICATION_JSON)
+   public AsyncCreateResponse expected() {
+      return AsyncCreateResponse.UNINITIALIZED;
+   }
+
+}

apis/cloudstack/src/test/resources/enablestaticnatresponse-withoutjob.json

@@ -0,0 +1 @@
+{ "enablestaticnatresponse" : { "success" : "true"}  }