Rework the validation of network/security groups in CloudStackComputeServiceAdapter.createNodeWithGroupEncodedIntoName(). Add a live test to verify that jclouds works with CloudStack's convention of assigning the user's default security group, if one is not specified.

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

@@ -24,6 +24,7 @@ import static com.google.common.base.Predicates.and;
 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.defaultNetworkInZone;
 import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
 import static org.jclouds.cloudstack.predicates.TemplatePredicates.isReady;
 
@@ -36,11 +37,13 @@ import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.collect.Iterables;
 import org.jclouds.cloudstack.CloudStackClient;
 import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
 import org.jclouds.cloudstack.domain.AsyncCreateResponse;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.domain.NetworkType;
 import org.jclouds.cloudstack.domain.PublicIPAddress;
 import org.jclouds.cloudstack.domain.ServiceOffering;
 import org.jclouds.cloudstack.domain.Template;
@@ -118,29 +121,40 @@ public class CloudStackComputeServiceAdapter implements
       Map<Long, Network> networks = networkSupplier.get();
 
       final long zoneId = Long.parseLong(template.getLocation().getId());
+      Zone zone = client.getZoneClient().getZone(zoneId);
 
       CloudStackTemplateOptions templateOptions = template.getOptions().as(CloudStackTemplateOptions.class);
 
       DeployVirtualMachineOptions options = displayName(name).name(name);
+      if (zone.getNetworkType() == NetworkType.ADVANCED) {
+         // security groups not allowed.
+         // at least one network must be given to CloudStack,
+         // but jclouds will try to autodetect an appropriate network if none given.
+         if (templateOptions.getSecurityGroupIds().size() > 0) {
+            throw new IllegalArgumentException("security groups cannot be specified for locations (zones) that use advanced networking");
+         }
+         if (templateOptions.getNetworkIds().size() > 0) {
+            options.networkIds(templateOptions.getNetworkIds());
+         } else {
+            if (networks.size() == 0) {
+               throw new IllegalArgumentException("please setup a network for zone: " + zoneId);
+            }
+            Network defaultNetworkInZone = Iterables.getFirst(filter(networks.values(), and(defaultNetworkInZone(zoneId), supportsStaticNAT())), null);
+            if(defaultNetworkInZone == null) {
+               throw new IllegalArgumentException("please choose a specific network in zone " + zoneId + ": " + networks);
+            } else {
+               options.networkId(defaultNetworkInZone.getId());
+            }
+         }
+      } else if(zone.getNetworkType() == NetworkType.BASIC) {
+         // both security groups and networks are optional, and CloudStack will
+         // use the zone/user's default network/security group if none given
          if (templateOptions.getSecurityGroupIds().size() > 0) {
             options.securityGroupIds(templateOptions.getSecurityGroupIds());
-      } else if (templateOptions.getNetworkIds().size() > 0) {
+         }
+         if (templateOptions.getNetworkIds().size() > 0) {
             options.networkIds(templateOptions.getNetworkIds());
-      } else if (networks.size() > 0) {
-         try {
-            options.networkId(getOnlyElement(filter(networks.values(), and(new Predicate<Network>() {
-
-               @Override
-               public boolean apply(Network arg0) {
-                  return arg0.getZoneId() == zoneId && arg0.isDefault();
          }
-
-            }, supportsStaticNAT()))).getId());
-         } catch (IllegalArgumentException e) {
-            throw new IllegalArgumentException("please choose a specific network in zone " + zoneId + ": " + networks);
-         }
-      } else {
-         throw new IllegalArgumentException("please setup a network or security group for zone: " + zoneId);
       }
 
       if (templateOptions.getIpOnDefaultNetwork() != null) {

apis/cloudstack/src/main/java/org/jclouds/cloudstack/predicates/NetworkPredicates.java

@@ -93,6 +93,19 @@ public class NetworkPredicates {
       }
    }
 
+   private static class DefaultNetworkInZone implements Predicate<Network> {
+      private final long zoneId;
+
+      public DefaultNetworkInZone(long zoneId) {
+         this.zoneId = zoneId;
+      }
+
+      @Override
+      public boolean apply(Network network) {
+         return network.getZoneId() == zoneId && network.isDefault();
+      }
+   }
+
    public static class NetworkServiceNamed implements Predicate<NetworkService> {
       private final String name;
 
@@ -189,6 +202,16 @@ public class NetworkPredicates {
       return IsVirtualNetwork.INSTANCE;
    }
 
+   /**
+    * Filters for default networks in a specific zone.
+    *
+    * @param zoneId the ID of the required zone.
+    * @return networks in the zone that have the default flag set.
+    */
+   public static Predicate<Network> defaultNetworkInZone(final long zoneId) {
+      return new DefaultNetworkInZone(zoneId);
+   }
+
    /**
     * 
     * @return always returns true.

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

@@ -31,6 +31,7 @@ import org.jclouds.cloudstack.domain.SecurityGroup;
 import org.jclouds.cloudstack.domain.VirtualMachine;
 import org.jclouds.cloudstack.domain.Zone;
 import org.jclouds.cloudstack.options.AccountInDomainOptions;
+import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
 import org.jclouds.cloudstack.options.ListSecurityGroupsOptions;
 import org.jclouds.net.IPSocket;
 import org.jclouds.util.Strings2;
@@ -195,6 +196,23 @@ public class SecurityGroupClientLiveTest extends BaseCloudStackClientLiveTest {
       assert group.getIngressRules() != null : group;
    }
 
+   @Test
+   public void testCreateVMWithoutSecurityGroupAssignsDefault() throws Exception {
+      if (!securityGroupsSupported)
+         return;
+      Long defaultTemplate = (imageId != null && !"".equals(imageId)) ? new Long(imageId) : null;
+      VirtualMachine newVm = VirtualMachineClientLiveTest.createVirtualMachineWithOptionsInZone(DeployVirtualMachineOptions.NONE,
+            zone.getId(), defaultTemplateOrPreferredInZone(defaultTemplate, client, zone.getId()), client,
+            jobComplete, virtualMachineRunning);
+      try {
+         VirtualMachine runningVm = client.getVirtualMachineClient().getVirtualMachine(newVm.getId());
+         assertTrue(runningVm.getSecurityGroups().size() == 1);
+         assertEquals(Iterables.getOnlyElement(runningVm.getSecurityGroups()).getName(), "default");
+      } finally {
+         assertTrue(jobComplete.apply(client.getVirtualMachineClient().destroyVirtualMachine(newVm.getId())));
+      }
+   }
+
    @AfterGroups(groups = "live")
    protected void tearDown() {
       if (vm != null) {

apis/cloudstack/src/test/java/org/jclouds/cloudstack/predicates/NetworkPredicatesTest.java

@@ -18,10 +18,7 @@
  */
 package org.jclouds.cloudstack.predicates;
 
-import static org.jclouds.cloudstack.predicates.NetworkPredicates.hasLoadBalancerService;
+import com.google.common.base.Predicate;
-import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsPortForwarding;
-import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
-
 import org.jclouds.cloudstack.domain.Network;
 import org.jclouds.cloudstack.domain.NetworkService;
 import org.testng.annotations.Test;
@@ -30,6 +27,10 @@ import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
+import static org.jclouds.cloudstack.predicates.NetworkPredicates.*;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
 /**
  * 
  * @author Adrian Cole
@@ -93,4 +94,17 @@ public class NetworkPredicatesTest {
       assert !hasLoadBalancerService().apply(network);
 
    }
+
+   public void testDefaultNetworkInZone() {
+      Network defaultInZone = Network.builder().isDefault(true).zoneId(42).build();
+      Network defaultNotInZone = Network.builder().isDefault(true).zoneId(200).build();
+      Network notDefaultInZone = Network.builder().isDefault(false).zoneId(42).build();
+      Network notDefaultNotInZone = Network.builder().isDefault(false).zoneId(200).build();
+
+      Predicate<Network> predicate = defaultNetworkInZone(42);
+      assertTrue(predicate.apply(defaultInZone));
+      assertFalse(predicate.apply(defaultNotInZone));
+      assertFalse(predicate.apply(notDefaultInZone));
+      assertFalse(predicate.apply(notDefaultNotInZone));
+   }
 }