Move the inner classes of OptionsConverter into own classes. Add JavaDoc and unit test.

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

@@ -39,6 +39,8 @@ 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;
@@ -211,7 +213,7 @@ public class CloudStackComputeServiceContextModule
    @Singleton
    Map<NetworkType, ? extends OptionsConverter> optionsConverters(){
       return ImmutableMap.of(
-         NetworkType.ADVANCED, new OptionsConverter.AdvancedNetworkOptionsConverter(),
+         NetworkType.ADVANCED, new AdvancedNetworkOptionsConverter(),
-         NetworkType.BASIC, new OptionsConverter.BasicNetworkOptionsConverter());
+         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/OptionsConverter.java

@@ -31,43 +31,24 @@ 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);
 
-   public static 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;
-      }
-   }
-
-   public static 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/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);
+   }
+}