Merge pull request #289 from richardcloudsoft/cs-secgrp

Validating network/security groups in CloudStackComputeServiceAdapter

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

@@ -29,6 +29,7 @@ import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 import org.jclouds.cloudstack.CloudStackAsyncClient;
 import org.jclouds.cloudstack.CloudStackClient;
@@ -38,9 +39,13 @@ import org.jclouds.cloudstack.compute.functions.TemplateToOperatingSystem;
 import org.jclouds.cloudstack.compute.functions.VirtualMachineToNodeMetadata;
 import org.jclouds.cloudstack.compute.functions.ZoneToLocation;
 import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
+import org.jclouds.cloudstack.compute.strategy.AdvancedNetworkOptionsConverter;
+import org.jclouds.cloudstack.compute.strategy.BasicNetworkOptionsConverter;
 import org.jclouds.cloudstack.compute.strategy.CloudStackComputeServiceAdapter;
+import org.jclouds.cloudstack.compute.strategy.OptionsConverter;
 import org.jclouds.cloudstack.domain.IPForwardingRule;
 import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.domain.NetworkType;
 import org.jclouds.cloudstack.domain.OSType;
 import org.jclouds.cloudstack.domain.ServiceOffering;
 import org.jclouds.cloudstack.domain.Template;
@@ -49,9 +54,11 @@ import org.jclouds.cloudstack.domain.VirtualMachine;
 import org.jclouds.cloudstack.domain.Zone;
 import org.jclouds.cloudstack.features.GuestOSClient;
 import org.jclouds.cloudstack.functions.StaticNATVirtualMachineInNetwork;
+import org.jclouds.cloudstack.functions.ZoneIdToZone;
 import org.jclouds.cloudstack.predicates.JobComplete;
 import org.jclouds.cloudstack.suppliers.GetCurrentUser;
 import org.jclouds.cloudstack.suppliers.NetworksForCurrentUser;
+import org.jclouds.cloudstack.suppliers.ZoneIdToZoneSupplier;
 import org.jclouds.collect.Memoized;
 import org.jclouds.compute.ComputeServiceAdapter;
 import org.jclouds.compute.config.ComputeServiceAdapterContextModule;
@@ -108,6 +115,10 @@ public class CloudStackComputeServiceContextModule
       install(new FactoryModuleBuilder().build(StaticNATVirtualMachineInNetwork.Factory.class));
       bind(new TypeLiteral<CacheLoader<Long, Set<IPForwardingRule>>>() {
       }).to(GetIPForwardingRulesByVirtualMachine.class);
+      bind(new TypeLiteral<CacheLoader<Long, Zone>>() {
+      }).to(ZoneIdToZone.class);
+      bind(new TypeLiteral<Supplier<LoadingCache<Long, Zone>>>() {
+      }).to(ZoneIdToZoneSupplier.class);
    }
 
    @Provides
@@ -198,4 +209,11 @@ public class CloudStackComputeServiceContextModule
       }
    }
 
+   @Provides
+   @Singleton
+   Map<NetworkType, ? extends OptionsConverter> optionsConverters(){
+      return ImmutableMap.of(
+         NetworkType.ADVANCED, new AdvancedNetworkOptionsConverter(),
+         NetworkType.BASIC, new BasicNetworkOptionsConverter());
+   }
 }

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

@@ -0,0 +1,59 @@
+/**
+ * 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.compute.strategy;
+
+import com.google.common.collect.Iterables;
+import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
+import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
+
+import java.util.Map;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Predicates.and;
+import static com.google.common.collect.Iterables.filter;
+import static org.jclouds.cloudstack.predicates.NetworkPredicates.defaultNetworkInZone;
+import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
+
+/**
+ * Convert template options into DeployVirtualMachineOptions, when the target zone has advanced networking.
+ *
+ * @author Richard Downer
+ */
+public class AdvancedNetworkOptionsConverter implements OptionsConverter {
+   @Override
+   public DeployVirtualMachineOptions apply(CloudStackTemplateOptions templateOptions, Map<Long, Network> networks, long zoneId, DeployVirtualMachineOptions options) {
+      // 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.
+      checkArgument(templateOptions.getSecurityGroupIds().isEmpty(), "security groups cannot be specified for locations (zones) that use advanced networking");
+      if (templateOptions.getNetworkIds().size() > 0) {
+         options.networkIds(templateOptions.getNetworkIds());
+      } else {
+         checkArgument(!networks.isEmpty(), "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());
+         }
+      }
+      return options;
+   }
+}

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

@@ -0,0 +1,45 @@
+/**
+ * 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.compute.strategy;
+
+import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
+import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
+
+import java.util.Map;
+
+/**
+ * Convert template options into DeployVirtualMachineOptions, when the target zone has basic networking.
+ *
+ * @author Richard Downer
+ */
+public class BasicNetworkOptionsConverter implements OptionsConverter {
+   @Override
+   public DeployVirtualMachineOptions apply(CloudStackTemplateOptions templateOptions, Map<Long, Network> networks, long zoneId, DeployVirtualMachineOptions options) {
+      // 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());
+      }
+      if (templateOptions.getNetworkIds().size() > 0) {
+         options.networkIds(templateOptions.getNetworkIds());
+      }
+      return options;
+   }
+}

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

@@ -20,27 +20,33 @@ 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.Preconditions.checkState;
 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;
 
 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;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.base.Throwables;
+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;
@@ -88,6 +94,8 @@ public class CloudStackComputeServiceAdapter implements
    private final CreatePortForwardingRulesForIP setupPortForwardingRulesForIP;
    private final LoadingCache<Long, Set<IPForwardingRule>> vmToRules;
    private final Map<String, Credentials> credentialStore;
+   private final Map<NetworkType, ? extends OptionsConverter> optionsConverters;
+   private final Supplier<LoadingCache<Long, Zone>> zoneIdToZone;
 
    @Inject
    public CloudStackComputeServiceAdapter(CloudStackClient client, Predicate<Long> jobComplete,
@@ -95,7 +103,8 @@ public class CloudStackComputeServiceAdapter implements
          BlockUntilJobCompletesAndReturnResult blockUntilJobCompletesAndReturnResult,
          StaticNATVirtualMachineInNetwork.Factory staticNATVMInNetwork,
          CreatePortForwardingRulesForIP setupPortForwardingRulesForIP, LoadingCache<Long, Set<IPForwardingRule>> vmToRules,
-         Map<String, Credentials> credentialStore) {
+         Map<String, Credentials> credentialStore, Map<NetworkType, ? extends OptionsConverter> optionsConverters,
+         Supplier<LoadingCache<Long, Zone>> zoneIdToZone) {
       this.client = checkNotNull(client, "client");
       this.jobComplete = checkNotNull(jobComplete, "jobComplete");
       this.networkSupplier = checkNotNull(networkSupplier, "networkSupplier");
@@ -105,6 +114,8 @@ public class CloudStackComputeServiceAdapter implements
       this.setupPortForwardingRulesForIP = checkNotNull(setupPortForwardingRulesForIP, "setupPortForwardingRulesForIP");
       this.vmToRules = checkNotNull(vmToRules, "vmToRules");
       this.credentialStore = checkNotNull(credentialStore, "credentialStore");
+      this.optionsConverters = optionsConverters;
+      this.zoneIdToZone = zoneIdToZone;
    }
 
    @Override
@@ -118,30 +129,19 @@ public class CloudStackComputeServiceAdapter implements
       Map<Long, Network> networks = networkSupplier.get();
 
       final long zoneId = Long.parseLong(template.getLocation().getId());
+      Zone zone = null;
+      try {
+         zone = zoneIdToZone.get().get(zoneId);
+      } catch (ExecutionException e) {
+         Throwables.propagate(e);
+      }
 
       CloudStackTemplateOptions templateOptions = template.getOptions().as(CloudStackTemplateOptions.class);
 
-      DeployVirtualMachineOptions options = displayName(name).name(name);
-      if (templateOptions.getSecurityGroupIds().size() > 0) {
-         options.securityGroupIds(templateOptions.getSecurityGroupIds());
-      } else 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);
-      }
+      checkState(optionsConverters.containsKey(zone.getNetworkType()), "no options converter configured for network type %s",zone.getNetworkType());
+      DeployVirtualMachineOptions options = DeployVirtualMachineOptions.Builder.displayName(name).name(name);
+      OptionsConverter optionsConverter = optionsConverters.get(zone.getNetworkType());
+      options = optionsConverter.apply(templateOptions, networks, zoneId, options);
 
       if (templateOptions.getIpOnDefaultNetwork() != null) {
          options.ipOnDefaultNetwork(templateOptions.getIpOnDefaultNetwork());

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

@@ -0,0 +1,54 @@
+/**
+ * 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.compute.strategy;
+
+import com.google.common.collect.Iterables;
+import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
+import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
+
+import java.util.Map;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Predicates.and;
+import static com.google.common.collect.Iterables.filter;
+import static org.jclouds.cloudstack.predicates.NetworkPredicates.defaultNetworkInZone;
+import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
+
+/**
+ * Convert template options into DeployVirtualMachineOptions. Expressed as an interface, because in
+ * CloudStack different zone network types have different requirements when it comes to networks and
+ * security groups.
+ *
+ * @author Richard Downer
+ */
+public interface OptionsConverter {
+
+   /**
+    * Convert a CloudStackTemplateOptions and apply to a DeployVirtualMachineOptions instance.
+    *
+    * @param templateOptions the input set of options
+    * @param networks the networks available
+    * @param zoneId the zone of the new virtual machine
+    * @param options where the resulting set of options will be applied
+    * @return same as "options" parameter
+    */
+   DeployVirtualMachineOptions apply(CloudStackTemplateOptions templateOptions, Map<Long, Network> networks, long zoneId, DeployVirtualMachineOptions options);
+
+}

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

@@ -0,0 +1,50 @@
+/**
+ * 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 com.google.common.cache.CacheLoader;
+import com.google.inject.Inject;
+import org.jclouds.cloudstack.CloudStackClient;
+import org.jclouds.cloudstack.domain.Zone;
+import org.jclouds.cloudstack.features.ZoneClient;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+/**
+ * Defines a cache that allows a zone to be looked up by its ID.
+ *
+ * @author Richard Downer
+ */
+public class ZoneIdToZone extends CacheLoader<Long, Zone> {
+
+   private final ZoneClient zoneClient;
+
+   @Inject
+   public ZoneIdToZone(CloudStackClient client) {
+      checkNotNull(client, "client");
+      this.zoneClient = client.getZoneClient();
+   }
+
+   @Override
+   public Zone load(Long zoneId) throws Exception {
+      checkNotNull(zoneId, "zoneId");
+      return zoneClient.getZone(zoneId);
+   }
+
+}

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/main/java/org/jclouds/cloudstack/suppliers/ZoneIdToZoneSupplier.java

@@ -0,0 +1,50 @@
+/**
+ * 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.suppliers;
+
+import com.google.common.base.Supplier;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+import com.google.inject.Inject;
+import org.jclouds.cloudstack.domain.Zone;
+
+import javax.inject.Named;
+import java.util.concurrent.TimeUnit;
+
+import static org.jclouds.Constants.PROPERTY_SESSION_INTERVAL;
+
+/**
+ * Supplies a cache that maps from zone IDs to zones.
+ *
+ * @author Richard Downer
+ */
+public class ZoneIdToZoneSupplier implements Supplier<LoadingCache<Long, Zone>> {
+   private final LoadingCache<Long, Zone> cache;
+
+   @Inject
+   public ZoneIdToZoneSupplier(CacheLoader<Long, Zone> zoneIdToZone, @Named(PROPERTY_SESSION_INTERVAL) long expirationSecs) {
+      cache = CacheBuilder.newBuilder().expireAfterWrite(expirationSecs, TimeUnit.SECONDS).build(zoneIdToZone);
+   }
+
+   @Override
+   public LoadingCache<Long, Zone> get() {
+      return cache;
+   }
+}

apis/cloudstack/src/test/java/org/jclouds/cloudstack/compute/strategy/OptionsConverterTest.java

@@ -0,0 +1,121 @@
+/**
+ * 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.compute.strategy;
+
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
+import org.jclouds.cloudstack.compute.options.CloudStackTemplateOptions;
+import org.jclouds.cloudstack.domain.Network;
+import org.jclouds.cloudstack.domain.NetworkService;
+import org.jclouds.cloudstack.options.DeployVirtualMachineOptions;
+import org.testng.annotations.Test;
+
+import java.util.Collections;
+import java.util.Map;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertTrue;
+
+@Test
+public class OptionsConverterTest {
+
+   private static final Map<Long,Network> EMPTY_NETWORKS_MAP = Collections.<Long, Network>emptyMap();
+   private static final int ZONE_ID = 2;
+   private final NetworkService firewallServiceWithStaticNat
+      = new NetworkService("Firewall", ImmutableMap.of("StaticNat", "true"));
+
+   @Test
+   public void testBasicNetworkOptionsConverter() {
+      BasicNetworkOptionsConverter converter = new BasicNetworkOptionsConverter();
+
+      CloudStackTemplateOptions optionsIn = CloudStackTemplateOptions.Builder.securityGroupId(42).networkId(46);
+      DeployVirtualMachineOptions optionsOut = DeployVirtualMachineOptions.NONE;
+
+      DeployVirtualMachineOptions optionsOut2 = converter.apply(optionsIn, EMPTY_NETWORKS_MAP, ZONE_ID, optionsOut);
+      assertTrue(optionsOut == optionsOut2);
+
+      DeployVirtualMachineOptions optionsExpected = DeployVirtualMachineOptions.Builder.securityGroupId(42).networkId(46);
+      assertEquals(optionsOut, optionsExpected);
+   }
+
+   @Test
+   public void testAdvancedSecurityGroupsNotAllowed() {
+      boolean exceptionThrown = false;
+      AdvancedNetworkOptionsConverter converter = new AdvancedNetworkOptionsConverter();
+      CloudStackTemplateOptions optionsIn = CloudStackTemplateOptions.Builder.securityGroupId(42);
+
+      try {
+         converter.apply(optionsIn, EMPTY_NETWORKS_MAP, ZONE_ID, DeployVirtualMachineOptions.NONE);
+      } catch(IllegalArgumentException e) {
+         exceptionThrown = true;
+      }
+
+      assertTrue(exceptionThrown, "IllegalArgumentException should have been thrown");
+   }
+
+   @Test
+   public void testAdvancedExplicitNetworkSelection() {
+      AdvancedNetworkOptionsConverter converter = new AdvancedNetworkOptionsConverter();
+      DeployVirtualMachineOptions optionsActual = converter.apply(CloudStackTemplateOptions.Builder.networkId(42),
+         EMPTY_NETWORKS_MAP, ZONE_ID, DeployVirtualMachineOptions.NONE);
+      DeployVirtualMachineOptions optionsExpected = DeployVirtualMachineOptions.Builder.networkId(42);
+      assertEquals(optionsActual, optionsExpected);
+   }
+
+   @Test
+   public void testAdvancedAutoDetectNetwork() {
+      AdvancedNetworkOptionsConverter converter = new AdvancedNetworkOptionsConverter();
+
+      Network eligibleNetwork = Network.builder()
+         .id(25).zoneId(ZONE_ID).isDefault(true).services(ImmutableSet.of(firewallServiceWithStaticNat))
+         .build();
+      DeployVirtualMachineOptions optionsActual = converter.apply(CloudStackTemplateOptions.NONE,
+         ImmutableMap.of(eligibleNetwork.getId(), eligibleNetwork), ZONE_ID, DeployVirtualMachineOptions.NONE);
+      DeployVirtualMachineOptions optionsExpected = DeployVirtualMachineOptions.Builder.networkId(25);
+      assertEquals(optionsActual, optionsExpected);
+   }
+
+   @Test
+   public void testAdvancedWhenNoNetworkGiven() {
+      AdvancedNetworkOptionsConverter converter = new AdvancedNetworkOptionsConverter();
+      boolean exceptionThrown = false;
+      try {
+         converter.apply(CloudStackTemplateOptions.NONE, EMPTY_NETWORKS_MAP, ZONE_ID, DeployVirtualMachineOptions.NONE);
+      } catch(IllegalArgumentException e) {
+         exceptionThrown = true;
+      }
+      assertTrue(exceptionThrown);
+   }
+
+   @Test
+   public void testAdvancedWhenNoNetworkEligible() {
+      AdvancedNetworkOptionsConverter converter = new AdvancedNetworkOptionsConverter();
+      Network unsuitableNetwork = Network.builder()
+         .id(25).zoneId(ZONE_ID)
+         .build();
+
+      boolean exceptionThrown = false;
+      try {
+         converter.apply(CloudStackTemplateOptions.NONE, ImmutableMap.of(unsuitableNetwork.getId(), unsuitableNetwork), ZONE_ID, DeployVirtualMachineOptions.NONE);
+      } catch(IllegalArgumentException e) {
+         exceptionThrown = true;
+      }
+      assertTrue(exceptionThrown);
+   }
+}

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 static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsPortForwarding;
-import static org.jclouds.cloudstack.predicates.NetworkPredicates.supportsStaticNAT;
-
+import com.google.common.base.Predicate;
 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));
+   }
 }