Issue 671: NPE on aws-ec2 w/vpc security groups

apis/ec2/src/main/java/org/jclouds/ec2/domain/IpPermission.java

@@ -18,9 +18,17 @@
  */
 package org.jclouds.ec2.domain;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import java.util.Set;
 
+import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableMultimap;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
+import com.google.common.collect.LinkedHashMultimap;
 import com.google.common.collect.Multimap;
+import com.google.common.collect.Sets;
 
 /**
  *
@@ -29,38 +37,155 @@ import com.google.common.collect.Multimap;
  *      />
  * @author Adrian Cole
  */
-public interface IpPermission extends Comparable<IpPermission> {
+public class IpPermission {
+   public static Builder builder() {
+      return new Builder();
+   }
+
+   public static class Builder {
+      private int fromPort;
+      private int toPort;
+      private IpProtocol ipProtocol;
+      private Multimap<String, String> userIdGroupPairs = LinkedHashMultimap.create();
+      private Set<String> groupIds = Sets.newLinkedHashSet();
+      private Set<String> ipRanges = Sets.newLinkedHashSet();
+
+      public Builder fromPort(int fromPort) {
+         this.fromPort = fromPort;
+         return this;
+      }
+
+      public Builder toPort(int toPort) {
+         this.toPort = toPort;
+         return this;
+      }
+
+      public Builder ipProtocol(IpProtocol ipProtocol) {
+         this.ipProtocol = checkNotNull(ipProtocol, "ipProtocol");
+         return this;
+      }
+
+      public Builder userIdGroupPair(String userId, String groupNameOrId) {
+         this.userIdGroupPairs.put(checkNotNull(userId, "userId"), checkNotNull(groupNameOrId, "groupNameOrId of %s", userId));
+         return this;
+      }
+
+      public Builder userIdGroupPairs(Multimap<String, String> userIdGroupPairs) {
+         this.userIdGroupPairs.putAll(checkNotNull(userIdGroupPairs, "userIdGroupPairs"));
+         return this;
+      }
+
+      public Builder ipRange(String ipRange) {
+         this.ipRanges.add(ipRange);
+         return this;
+      }
+
+      public Builder ipRanges(Iterable<String> ipRanges) {
+         Iterables.addAll(this.ipRanges, checkNotNull(ipRanges, "ipRanges"));
+         return this;
+      }
+
+      public Builder groupId(String groupId) {
+         this.groupIds.add(checkNotNull(groupId, "groupId"));
+         return this;
+      }
+
+      public Builder groupIds(Iterable<String> groupIds) {
+         Iterables.addAll(this.groupIds, checkNotNull(groupIds, "groupIds"));
+         return this;
+      }
+
+      public IpPermission build() {
+         return new IpPermission(ipProtocol, fromPort, toPort, userIdGroupPairs, groupIds, ipRanges);
+      }
+   }
+
+   private final int fromPort;
+   private final int toPort;
+   private final Multimap<String, String> userIdGroupPairs;
+   private final Set<String> groupIds;
+   private final IpProtocol ipProtocol;
+   private final Set<String> ipRanges;
+
+   public IpPermission(IpProtocol ipProtocol, int fromPort, int toPort, Multimap<String, String> userIdGroupPairs,
+         Iterable<String> groupIds, Iterable<String> ipRanges) {
+      this.fromPort = fromPort;
+      this.toPort = toPort;
+      this.userIdGroupPairs = ImmutableMultimap.copyOf(checkNotNull(userIdGroupPairs, "userIdGroupPairs"));
+      this.ipProtocol = checkNotNull(ipProtocol, "ipProtocol");
+      this.groupIds = ImmutableSet.copyOf(checkNotNull(groupIds, "groupIds"));
+      this.ipRanges = ImmutableSet.copyOf(checkNotNull(ipRanges, "ipRanges"));
+   }
 
    /**
     * Start of port range for the TCP and UDP protocols, or an ICMP type number.
     * An ICMP type number of -1 indicates a wildcard (i.e., any ICMP type
     * number).
     */
-   int getFromPort();
+   public int getFromPort() {
+      return fromPort;
+   }
 
    /**
     * End of port range for the TCP and UDP protocols, or an ICMP code. An ICMP
     * code of -1 indicates a wildcard (i.e., any ICMP code).
     */
-   int getToPort();
+   public int getToPort() {
+      return toPort;
+   }
 
    /**
     * List of security group and user ID pairs.
     */
-   Multimap<String, String> getUserIdGroupPairs();
+   public Multimap<String, String> getUserIdGroupPairs() {
+      return userIdGroupPairs;
+   }
 
    /**
     * List of security group Ids
     */
-   Set<String> getGroupIds();
+   public Set<String> getGroupIds() {
+      return groupIds;
+   }
 
    /**
     * IP protocol
     */
-   IpProtocol getIpProtocol();
+   public IpProtocol getIpProtocol() {
+      return ipProtocol;
+   }
 
    /**
     * IP ranges.
     */
-   Set<String> getIpRanges();
+   public Set<String> getIpRanges() {
+      return ipRanges;
+   }
+
+   @Override
+   public int hashCode() {
+      return Objects.hashCode(fromPort, toPort, groupIds, ipProtocol, ipRanges, userIdGroupPairs);
+   }
+
+   @Override
+   public boolean equals(Object obj) {
+      if (this == obj)
+         return true;
+      if (obj == null || getClass() != obj.getClass())
+         return false;
+      IpPermission that = IpPermission.class.cast(obj);
+      return Objects.equal(this.fromPort, that.fromPort) && Objects.equal(this.toPort, that.toPort)
+            && Objects.equal(this.groupIds, that.groupIds) && Objects.equal(this.ipProtocol, that.ipProtocol)
+            && Objects.equal(this.ipRanges, that.ipRanges)
+            && Objects.equal(this.userIdGroupPairs, that.userIdGroupPairs);
+   }
+
+   @Override
+   public String toString() {
+      return Objects.toStringHelper(this).add("fromPort", fromPort == -1 ? null : fromPort)
+            .add("toPort", toPort == -1 ? null : toPort).add("groupIds", groupIds.size() == 0 ? null : groupIds)
+            .add("ipProtocol", ipProtocol).add("ipRanges", ipRanges.size() == 0 ? null : ipRanges)
+            .add("userIdGroupPairs", userIdGroupPairs.size() == 0 ? null : userIdGroupPairs).toString();
+   }
+
 }

apis/ec2/src/main/java/org/jclouds/ec2/domain/IpPermissionImpl.java

@@ -1,226 +0,0 @@
-/**
- * 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.ec2.domain;
-
-import static com.google.common.base.Preconditions.checkNotNull;
-
-import java.util.Set;
-
-import com.google.common.collect.ImmutableMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.LinkedHashMultimap;
-import com.google.common.collect.Multimap;
-import com.google.common.collect.Sets;
-
-/**
- * 
- * @see <a href=
- *      "http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/ApiReference-ItemType-IpPermissionType.html"
- *      />
- * @author Adrian Cole
- */
-public class IpPermissionImpl implements IpPermission {
-   public static Builder builder() {
-      return new Builder();
-   }
-
-   public static class Builder {
-      private int fromPort;
-      private int toPort;
-      private IpProtocol ipProtocol;
-      private Multimap<String, String> userIdGroupPairs = LinkedHashMultimap.create();
-      private Set<String> groupIds = Sets.newLinkedHashSet();
-      private Set<String> ipRanges = Sets.newLinkedHashSet();
-
-      public Builder fromPort(int fromPort) {
-         this.fromPort = fromPort;
-         return this;
-      }
-
-      public Builder toPort(int toPort) {
-         this.fromPort = toPort;
-         return this;
-      }
-
-      public Builder ipProtocol(IpProtocol ipProtocol) {
-         this.ipProtocol = ipProtocol;
-         return this;
-      }
-
-      public Builder userIdGroupPair(String userId, String groupNameOrId) {
-         this.userIdGroupPairs.put(userId, groupNameOrId);
-         return this;
-      }
-
-      public Builder userIdGroupPairs(Multimap<String, String> userIdGroupPairs) {
-         this.userIdGroupPairs.putAll(userIdGroupPairs);
-         return this;
-      }
-
-      public Builder ipRange(String ipRange) {
-         this.ipRanges.add(ipRange);
-         return this;
-      }
-
-      public Builder ipRanges(Iterable<String> ipRanges) {
-         Iterables.addAll(this.ipRanges, ipRanges);
-         return this;
-      }
-
-      public Builder groupId(String groupId) {
-         this.groupIds.add(groupId);
-         return this;
-      }
-
-      public Builder groupIds(Iterable<String> groupIds) {
-         Iterables.addAll(this.groupIds, groupIds);
-         return this;
-      }
-
-      public IpPermission build() {
-         return new IpPermissionImpl(ipProtocol, fromPort, toPort, userIdGroupPairs, groupIds, ipRanges);
-      }
-   }
-
-   private final int fromPort;
-   private final int toPort;
-   private final Multimap<String, String> userIdGroupPairs;
-   private final Set<String> groupIds;
-   private final IpProtocol ipProtocol;
-   private final Set<String> ipRanges;
-
-   public IpPermissionImpl(IpProtocol ipProtocol, int fromPort, int toPort,
-         Multimap<String, String> userIdGroupPairs, Iterable<String> groupIds, Iterable<String> ipRanges) {
-      this.fromPort = fromPort;
-      this.toPort = toPort;
-      this.userIdGroupPairs = ImmutableMultimap.copyOf(checkNotNull(userIdGroupPairs, "userIdGroupPairs"));
-      this.ipProtocol = checkNotNull(ipProtocol, "ipProtocol");
-      this.groupIds = ImmutableSet.copyOf(checkNotNull(groupIds, "groupIds"));
-      this.ipRanges = ImmutableSet.copyOf(checkNotNull(ipRanges, "ipRanges"));
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   public int compareTo(IpPermission o) {
-      return (this == o) ? 0 : getIpProtocol().compareTo(o.getIpProtocol());
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public int getFromPort() {
-      return fromPort;
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public int getToPort() {
-      return toPort;
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public Multimap<String, String> getUserIdGroupPairs() {
-      return userIdGroupPairs;
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public Set<String> getGroupIds() {
-      return groupIds;
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public IpProtocol getIpProtocol() {
-      return ipProtocol;
-   }
-
-   /**
-    * {@inheritDoc}
-    */
-   @Override
-   public Set<String> getIpRanges() {
-      return ipRanges;
-   }
-
-   @Override
-   public int hashCode() {
-      final int prime = 31;
-      int result = 1;
-      result = prime * result + fromPort;
-      result = prime * result + ((groupIds == null) ? 0 : groupIds.hashCode());
-      result = prime * result + ((ipProtocol == null) ? 0 : ipProtocol.hashCode());
-      result = prime * result + ((ipRanges == null) ? 0 : ipRanges.hashCode());
-      result = prime * result + toPort;
-      result = prime * result + ((userIdGroupPairs == null) ? 0 : userIdGroupPairs.hashCode());
-      return result;
-   }
-
-   @Override
-   public boolean equals(Object obj) {
-      if (this == obj)
-         return true;
-      if (obj == null)
-         return false;
-      if (getClass() != obj.getClass())
-         return false;
-      IpPermissionImpl other = (IpPermissionImpl) obj;
-      if (fromPort != other.fromPort)
-         return false;
-      if (groupIds == null) {
-         if (other.groupIds != null)
-            return false;
-      } else if (!groupIds.equals(other.groupIds))
-         return false;
-      if (ipProtocol != other.ipProtocol)
-         return false;
-      if (ipRanges == null) {
-         if (other.ipRanges != null)
-            return false;
-      } else if (!ipRanges.equals(other.ipRanges))
-         return false;
-      if (toPort != other.toPort)
-         return false;
-      if (userIdGroupPairs == null) {
-         if (other.userIdGroupPairs != null)
-            return false;
-      } else if (!userIdGroupPairs.equals(other.userIdGroupPairs))
-         return false;
-      return true;
-   }
-
-   @Override
-   public String toString() {
-      return "[fromPort=" + fromPort + ", toPort=" + toPort + ", userIdGroupPairs=" + userIdGroupPairs + ", groupIds="
-            + groupIds + ", ipProtocol=" + ipProtocol + ", ipRanges=" + ipRanges + "]";
-   }
-
-}

apis/ec2/src/main/java/org/jclouds/ec2/domain/SecurityGroup.java

@@ -18,11 +18,16 @@
  */
 package org.jclouds.ec2.domain;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import java.util.Set;
 
 import org.jclouds.javax.annotation.Nullable;
 
-import static com.google.common.base.Preconditions.checkNotNull;
+import com.google.common.base.Objects;
+import com.google.common.base.Objects.ToStringHelper;
+import com.google.common.collect.ForwardingSet;
+import com.google.common.collect.ImmutableSet;
 
 /**
  *
@@ -31,42 +36,135 @@ import static com.google.common.base.Preconditions.checkNotNull;
  *      />
  * @author Adrian Cole
  */
-public class SecurityGroup implements Comparable<SecurityGroup> {
+public class SecurityGroup extends ForwardingSet<IpPermission> {
+
+   public static Builder<?> builder() {
+      return new ConcreteBuilder();
+   }
+
+   public Builder<?> toBuilder() {
+      return new ConcreteBuilder().fromSecurityGroup(this);
+   }
+
+   public static abstract class Builder<T extends Builder<T>> {
+      protected abstract T self();
+
+      protected String region;
+      protected String id;
+      protected String name;
+      protected String ownerId;
+      protected String description;
+      protected ImmutableSet.Builder<IpPermission> ipPermissions = ImmutableSet.<IpPermission> builder();
+
+      /**
+       * @see SecurityGroup#getRegion()
+       */
+      public T region(String region) {
+         this.region = region;
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#getId()
+       */
+      public T id(String id) {
+         this.id = id;
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#getName()
+       */
+      public T name(String name) {
+         this.name = name;
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#getOwnerId()
+       */
+      public T ownerId(String ownerId) {
+         this.ownerId = ownerId;
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#getDescription()
+       */
+      public T description(String description) {
+         this.description = description;
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#delegate()
+       */
+      public T role(IpPermission role) {
+         this.ipPermissions.add(role);
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#delegate()
+       */
+      public T ipPermissions(Iterable<IpPermission> ipPermissions) {
+         this.ipPermissions.addAll(checkNotNull(ipPermissions, "ipPermissions"));
+         return self();
+      }
+
+      /**
+       * @see SecurityGroup#delegate()
+       */
+      public T ipPermission(IpPermission ipPermission) {
+         this.ipPermissions.add(checkNotNull(ipPermission, "ipPermission"));
+         return self();
+      }
+
+      public SecurityGroup build() {
+         return new SecurityGroup(region, id, name, ownerId, description, ipPermissions.build());
+      }
+
+      public T fromSecurityGroup(SecurityGroup in) {
+         return region(in.region).id(in.id).name(in.name).ownerId(in.ownerId).description(in.description)
+               .ipPermissions(in);
+      }
+   }
+
+   private static class ConcreteBuilder extends Builder<ConcreteBuilder> {
+      @Override
+      protected ConcreteBuilder self() {
+         return this;
+      }
+   }
 
    private final String region;
    private final String id;
    private final String name;
    private final String ownerId;
    private final String description;
-   private final Set<IpPermissionImpl> ipPermissions;
+   private final Set<IpPermission> ipPermissions;
 
    public SecurityGroup(String region, String id, String name, String ownerId, String description,
-         Set<IpPermissionImpl> ipPermissions) {
+         Iterable<IpPermission> ipPermissions) {
       this.region = checkNotNull(region, "region");
       this.id = id;
       this.name = name;
       this.ownerId = ownerId;
       this.description = description;
-      this.ipPermissions = ipPermissions;
+      this.ipPermissions = ImmutableSet.copyOf(checkNotNull(ipPermissions, "ipPermissions"));
    }
 
    /**
-    * Security groups are not copied across Regions. Instances within the Region
+    * To be removed in jclouds 1.6 <h4>Warning</h4>
-    * cannot communicate with instances outside the Region using group-based
+    *
-    * firewall rules. Traffic from instances in another Region is seen as WAN
+    * Especially on EC2 clones that may not support regions, this value is
-    * bandwidth.
+    * fragile. Consider alternate means to determine context.
     */
+   @Deprecated
    public String getRegion() {
       return region;
    }
 
-   /**
-    * {@inheritDoc}
-    */
-   public int compareTo(SecurityGroup o) {
-      return (this == o) ? 0 : getName().compareTo(o.getName());
-   }
-
    /**
     * id of the security group. Not in all EC2 impls
     */
@@ -97,70 +195,46 @@ public class SecurityGroup implements Comparable<SecurityGroup> {
    }
 
    /**
-    * Set of IP permissions associated with the security group.
+    * Please use this class as a collection
     */
-   public Set<IpPermissionImpl> getIpPermissions() {
+   @Deprecated
+   public Set<IpPermission> getIpPermissions() {
       return ipPermissions;
    }
 
    @Override
    public int hashCode() {
-      final int prime = 31;
+      return Objects.hashCode(region, id, name, ownerId, description, ipPermissions);
-      int result = 1;
-      result = prime * result + ((description == null) ? 0 : description.hashCode());
-      result = prime * result + ((id == null) ? 0 : id.hashCode());
-      result = prime * result + ((ipPermissions == null) ? 0 : ipPermissions.hashCode());
-      result = prime * result + ((name == null) ? 0 : name.hashCode());
-      result = prime * result + ((ownerId == null) ? 0 : ownerId.hashCode());
-      result = prime * result + ((region == null) ? 0 : region.hashCode());
-      return result;
    }
 
    @Override
    public boolean equals(Object obj) {
       if (this == obj)
          return true;
-      if (obj == null)
+      if (obj == null || getClass() != obj.getClass())
          return false;
-      if (getClass() != obj.getClass())
+      SecurityGroup that = SecurityGroup.class.cast(obj);
-         return false;
+      return Objects.equal(this.region, that.region)
-      SecurityGroup other = (SecurityGroup) obj;
+            && Objects.equal(this.id, that.id)
-      if (description == null) {
+            && Objects.equal(this.name, that.name)
-         if (other.description != null)
+            && Objects.equal(this.ownerId, that.ownerId)
-            return false;
+            && Objects.equal(this.description, that.description)
-      } else if (!description.equals(other.description))
+            && Objects.equal(this.ipPermissions, that.ipPermissions);
-         return false;
+   }
-      if (id == null) {
+
-         if (other.id != null)
+   protected ToStringHelper string() {
-            return false;
+      return Objects.toStringHelper(this).add("region", region).add("id", id).add("name", name)
-      } else if (!id.equals(other.id))
+            .add("ownerId", ownerId).add("description", description)
-         return false;
+            .add("ipPermissions", ipPermissions.size() == 0 ? null : ipPermissions);
-      if (ipPermissions == null) {
-         if (other.ipPermissions != null)
-            return false;
-      } else if (!ipPermissions.equals(other.ipPermissions))
-         return false;
-      if (name == null) {
-         if (other.name != null)
-            return false;
-      } else if (!name.equals(other.name))
-         return false;
-      if (ownerId == null) {
-         if (other.ownerId != null)
-            return false;
-      } else if (!ownerId.equals(other.ownerId))
-         return false;
-      if (region == null) {
-         if (other.region != null)
-            return false;
-      } else if (!region.equals(other.region))
-         return false;
-      return true;
    }
 
    @Override
    public String toString() {
-      return "[region=" + region + ", id=" + id + ", name=" + name + ", ownerId=" + ownerId + ", description="
+      return string().toString();
-            + description + ", ipPermissions=" + ipPermissions + "]";
+   }
+
+   @Override
+   protected Set<IpPermission> delegate() {
+      return ipPermissions;
    }
 }

apis/ec2/src/main/java/org/jclouds/ec2/util/IpPermissions.java

@@ -24,7 +24,6 @@ import java.util.Map;
 import java.util.Map.Entry;
 
 import org.jclouds.ec2.domain.IpPermission;
-import org.jclouds.ec2.domain.IpPermissionImpl;
 import org.jclouds.ec2.domain.IpProtocol;
 import org.jclouds.util.Maps2;
 
@@ -42,7 +41,7 @@ import com.google.common.collect.Multimaps;
  *
  * @author Adrian Cole
  */
-public class IpPermissions extends IpPermissionImpl {
+public class IpPermissions extends IpPermission {
 
    protected IpPermissions(IpProtocol ipProtocol, int fromPort, int toPort,
          Multimap<String, String> userIdGroupPairs, Iterable<String> groupIds, Iterable<String> ipRanges) {

apis/ec2/src/main/java/org/jclouds/ec2/xml/DescribeSecurityGroupsResponseHandler.java

@@ -18,138 +18,111 @@
  */
 package org.jclouds.ec2.xml;
 
-import static org.jclouds.util.SaxUtils.currentOrNegative;
-import static org.jclouds.util.SaxUtils.currentOrNull;
 import static org.jclouds.util.SaxUtils.equalsOrSuffix;
 
 import java.util.Set;
 
 import javax.inject.Inject;
 
-import org.jclouds.aws.util.AWSUtils;
-import org.jclouds.ec2.domain.IpPermissionImpl;
-import org.jclouds.ec2.domain.IpProtocol;
 import org.jclouds.ec2.domain.SecurityGroup;
+import org.jclouds.http.HttpRequest;
 import org.jclouds.http.functions.ParseSax;
-import org.jclouds.location.Region;
+import org.jclouds.http.functions.ParseSax.HandlerForGeneratedRequestWithResult;
 import org.xml.sax.Attributes;
+import org.xml.sax.SAXException;
 
-import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.LinkedHashMultimap;
+import com.google.common.collect.ImmutableSet.Builder;
-import com.google.common.collect.Multimap;
-import com.google.common.collect.Sets;
 
 /**
  * Parses: DescribeSecurityGroupsResponse
  * xmlns="http://ec2.amazonaws.com/doc/2010-06-15/"
  *
  * @see <a href=
- *      "http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/index.html?ApiReference-query-DescribeSecurityGroups.html"
+ *      "http://docs.amazonwebservices.com/AWSEC2/latest/APIReference/index.html?ApiReference-query-DescribesecurityGroupInfo.html"
  *      />
  * @author Adrian Cole
  */
 public class DescribeSecurityGroupsResponseHandler extends
       ParseSax.HandlerForGeneratedRequestWithResult<Set<SecurityGroup>> {
-   @Inject
+
-   @Region
+   private final SecurityGroupHandler securityGroupHandler;
-   Supplier<String> defaultRegion;
 
    private StringBuilder currentText = new StringBuilder();
-   private Set<SecurityGroup> securtyGroups = Sets.newLinkedHashSet();
+   private Builder<SecurityGroup> securityGroups = ImmutableSet.<SecurityGroup> builder();
-   private String groupId;
+   private boolean inSecurityGroupInfo;
-   private String groupName;
-   private String ownerId;
-   private String groupDescription;
-   private Set<IpPermissionImpl> ipPermissions = Sets.newLinkedHashSet();
-   private int fromPort;
-   private int toPort;
-   private Multimap<String, String> groups = LinkedHashMultimap.create();
-   private String userId;
-   private String userIdGroupName;
-   private IpProtocol ipProtocol;
-   private Set<String> ipRanges = Sets.newLinkedHashSet();
 
-   private boolean inIpPermissions;
+   protected int itemDepth;
-   private boolean inIpRanges;
-   private boolean inGroups;
 
+   @Inject
+   public DescribeSecurityGroupsResponseHandler(SecurityGroupHandler securityGroupHandler) {
+      this.securityGroupHandler = securityGroupHandler;
+   }
+
+   @Override
+   public HandlerForGeneratedRequestWithResult<Set<SecurityGroup>> setContext(HttpRequest request) {
+      securityGroupHandler.setContext(request);
+      return super.setContext(request);
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
    public Set<SecurityGroup> getResult() {
-      return securtyGroups;
+      return securityGroups.build();
    }
 
-   public void startElement(String uri, String name, String qName, Attributes attrs) {
+   /**
-      if (equalsOrSuffix(qName, "ipPermissions")) {
+    * {@inheritDoc}
-         inIpPermissions = true;
+    */
-      } else if (equalsOrSuffix(qName, "ipRanges")) {
+   @Override
-         inIpRanges = true;
+   public void startElement(String url, String name, String qName, Attributes attributes) throws SAXException {
-      } else if (equalsOrSuffix(qName, "groups")) {
+      if (equalsOrSuffix(qName, "item")) {
-         inGroups = true;
+         itemDepth++;
+      } else if (equalsOrSuffix(qName, "securityGroupInfo")) {
+         inSecurityGroupInfo = true;
+      }
+      if (inSecurityGroupInfo) {
+         securityGroupHandler.startElement(url, name, qName, attributes);
       }
    }
 
-   public void endElement(String uri, String name, String qName) {
+   /**
-      if (equalsOrSuffix(qName, "groupName")) {
+    * {@inheritDoc}
-         if (!inGroups)
+    */
-            this.groupName = currentOrNull(currentText);
+   @Override
-         else
+   public void endElement(String uri, String name, String qName) throws SAXException {
-            this.userIdGroupName = currentOrNull(currentText);
+      if (equalsOrSuffix(qName, "item")) {
-      } else if (equalsOrSuffix(qName, "groupId")) {
+         endItem(uri, name, qName);
-         this.groupId = currentOrNull(currentText);
+         itemDepth--;
-      } else if (equalsOrSuffix(qName, "ownerId")) {
+      } else if (equalsOrSuffix(qName, "securityGroupInfo")) {
-         this.ownerId = currentOrNull(currentText);
+         inSecurityGroupInfo = false;
-      } else if (equalsOrSuffix(qName, "userId")) {
+      } else if (inSecurityGroupInfo) {
-         this.userId = currentOrNull(currentText);
+         securityGroupHandler.endElement(uri, name, qName);
-      } else if (equalsOrSuffix(qName, "groupDescription")) {
-         this.groupDescription = currentOrNull(currentText);
-      } else if (equalsOrSuffix(qName, "ipProtocol")) {
-         // Algorete: ipProtocol can be an empty tag on EC2 clone (e.g. OpenStack EC2)
-         this.ipProtocol = IpProtocol.fromValue(currentOrNegative(currentText));
-      } else if (equalsOrSuffix(qName, "fromPort")) {
-         // Algorete: fromPort can be an empty tag on EC2 clone (e.g. OpenStack EC2)
-         this.fromPort = Integer.parseInt(currentOrNegative(currentText));
-      } else if (equalsOrSuffix(qName, "toPort")) {
-         // Algorete: toPort can be an empty tag on EC2 clone (e.g. OpenStack EC2)
-         this.toPort = Integer.parseInt(currentOrNegative(currentText));
-      } else if (equalsOrSuffix(qName, "cidrIp")) {
-         this.ipRanges.add(currentOrNull(currentText));
-      } else if (equalsOrSuffix(qName, "ipPermissions")) {
-         inIpPermissions = false;
-      } else if (equalsOrSuffix(qName, "ipRanges")) {
-         inIpRanges = false;
-      } else if (equalsOrSuffix(qName, "groups")) {
-         inGroups = false;
-      } else if (equalsOrSuffix(qName, "item")) {
-         if (inIpPermissions && !inIpRanges && !inGroups) {
-            // TODO groups? we need an example of VPC stuff
-            ipPermissions.add(new IpPermissionImpl(ipProtocol, fromPort, toPort, groups, ImmutableSet.<String> of(),
-                  ipRanges));
-            this.fromPort = -1;
-            this.toPort = -1;
-            this.groups = LinkedHashMultimap.create();
-            this.ipProtocol = null;
-            this.ipRanges = Sets.newLinkedHashSet();
-         } else if (inIpPermissions && !inIpRanges && inGroups) {
-            this.groups.put(userId, userIdGroupName);
-            this.userId = null;
-            this.userIdGroupName = null;
-         } else if (!inIpPermissions && !inIpRanges && !inGroups) {
-            String region = AWSUtils.findRegionInArgsOrNull(getRequest());
-            if (region == null)
-               region = defaultRegion.get();
-            securtyGroups.add(new SecurityGroup(region, groupId, groupName, ownerId, groupDescription, ipPermissions));
-            this.groupName = null;
-            this.groupId = null;
-            this.ownerId = null;
-            this.groupDescription = null;
-            this.ipPermissions = Sets.newLinkedHashSet();
       }
-      }
-
       currentText = new StringBuilder();
    }
 
+   protected void endItem(String uri, String name, String qName) throws SAXException {
+      if (inSecurityGroupInfo) {
+         if (itemDepth == 1)
+            securityGroups.add(securityGroupHandler.getResult());
+         else
+            securityGroupHandler.endElement(uri, name, qName);
+      }
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
    public void characters(char ch[], int start, int length) {
+      if (inSecurityGroupInfo) {
+         securityGroupHandler.characters(ch, start, length);
+      } else {
          currentText.append(ch, start, length);
       }
    }
+
+}

apis/ec2/src/main/java/org/jclouds/ec2/xml/IpPermissionHandler.java

@@ -0,0 +1,75 @@
+package org.jclouds.ec2.xml;
+
+import static org.jclouds.util.SaxUtils.currentOrNegative;
+import static org.jclouds.util.SaxUtils.currentOrNull;
+import static org.jclouds.util.SaxUtils.equalsOrSuffix;
+
+import org.jclouds.ec2.domain.IpPermission;
+import org.jclouds.ec2.domain.IpProtocol;
+import org.jclouds.http.functions.ParseSax;
+import org.xml.sax.SAXException;
+
+/**
+ *
+ * @author Adrian Cole
+ */
+public class IpPermissionHandler extends ParseSax.HandlerForGeneratedRequestWithResult<IpPermission> {
+
+   private StringBuilder currentText = new StringBuilder();
+   private IpPermission.Builder builder = IpPermission.builder();
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public IpPermission getResult() {
+      try {
+         return builder.build();
+      } finally {
+         builder = IpPermission.builder();
+      }
+   }
+
+   private String userId;
+   private String groupId;
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public void endElement(String uri, String name, String qName) throws SAXException {
+      if (equalsOrSuffix(qName, "ipProtocol")) {
+         // Algorete: ipProtocol can be an empty tag on EC2 clone (e.g.
+         // OpenStack EC2)
+         builder.ipProtocol(IpProtocol.fromValue(currentOrNegative(currentText)));
+      } else if (equalsOrSuffix(qName, "fromPort")) {
+         // Algorete: fromPort can be an empty tag on EC2 clone (e.g. OpenStack
+         // EC2)
+         builder.fromPort(Integer.parseInt(currentOrNegative(currentText)));
+      } else if (equalsOrSuffix(qName, "toPort")) {
+         // Algorete: toPort can be an empty tag on EC2 clone (e.g. OpenStack
+         // EC2)
+         builder.toPort(Integer.parseInt(currentOrNegative(currentText)));
+      } else if (equalsOrSuffix(qName, "cidrIp")) {
+         builder.ipRange(currentOrNull(currentText));
+      } else if (equalsOrSuffix(qName, "userId")) {
+         this.userId = currentOrNull(currentText);
+      } else if (equalsOrSuffix(qName, "groupName") || equalsOrSuffix(qName, "groupId")) {
+         this.groupId = currentOrNull(currentText);
+      } else if (equalsOrSuffix(qName, "item")) {
+         if (userId != null && groupId != null)
+            builder.userIdGroupPair(userId, groupId);
+         userId = groupId = null;
+      }
+      currentText = new StringBuilder();
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public void characters(char ch[], int start, int length) {
+      currentText.append(ch, start, length);
+   }
+
+}

apis/ec2/src/main/java/org/jclouds/ec2/xml/SecurityGroupHandler.java

@@ -0,0 +1,145 @@
+/**
+ * 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.ec2.xml;
+
+import static org.jclouds.util.SaxUtils.currentOrNull;
+import static org.jclouds.util.SaxUtils.equalsOrSuffix;
+
+import org.jclouds.aws.util.AWSUtils;
+import org.jclouds.ec2.domain.SecurityGroup;
+import org.jclouds.http.HttpRequest;
+import org.jclouds.http.functions.ParseSax;
+import org.jclouds.http.functions.ParseSax.HandlerForGeneratedRequestWithResult;
+import org.jclouds.location.Region;
+import org.jclouds.rest.internal.GeneratedHttpRequest;
+import org.xml.sax.Attributes;
+import org.xml.sax.SAXException;
+
+import com.google.common.base.Supplier;
+import com.google.inject.Inject;
+
+/**
+ * @author Adrian Cole
+ */
+public class SecurityGroupHandler extends ParseSax.HandlerForGeneratedRequestWithResult<SecurityGroup> {
+
+   protected final IpPermissionHandler ipPermissionHandler;
+   protected final Supplier<String> defaultRegion;
+
+   protected StringBuilder currentText = new StringBuilder();
+   protected SecurityGroup.Builder<?> builder;
+   protected boolean inIpPermissions;
+
+   protected int itemDepth;
+
+   protected String region;
+
+   @Inject
+   public SecurityGroupHandler(IpPermissionHandler ipPermissionHandler, @Region Supplier<String> defaultRegion) {
+      this.ipPermissionHandler = ipPermissionHandler;
+      this.defaultRegion = defaultRegion;
+   }
+
+   protected SecurityGroup.Builder<?> builder() {
+      return SecurityGroup.builder().region(region);
+   }
+
+   @Override
+   public HandlerForGeneratedRequestWithResult<SecurityGroup> setContext(HttpRequest request) {
+      region = AWSUtils.findRegionInArgsOrNull(GeneratedHttpRequest.class.cast(request));
+      if (region == null)
+         region = defaultRegion.get();
+      builder = builder();
+      return super.setContext(request);
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public SecurityGroup getResult() {
+      try {
+         return builder.build();
+      } finally {
+         builder = builder();
+      }
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public void startElement(String url, String name, String qName, Attributes attributes) throws SAXException {
+      if (equalsOrSuffix(qName, "item")) {
+         itemDepth++;
+      } else if (equalsOrSuffix(qName, "ipPermissions")) {
+         inIpPermissions = true;
+      }
+      if (inIpPermissions) {
+         ipPermissionHandler.startElement(url, name, qName, attributes);
+      }
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public void endElement(String uri, String name, String qName) throws SAXException {
+      if (equalsOrSuffix(qName, "item")) {
+         endItem(uri, name, qName);
+         itemDepth--;
+      } else if (equalsOrSuffix(qName, "ipPermissions")) {
+         inIpPermissions = false;
+         itemDepth = 0;
+      } else if (inIpPermissions) {
+         ipPermissionHandler.endElement(uri, name, qName);
+      } else if (equalsOrSuffix(qName, "groupName")) {
+         builder.name(currentOrNull(currentText));
+      } else if (equalsOrSuffix(qName, "groupId")) {
+         builder.id(currentOrNull(currentText));
+      } else if (equalsOrSuffix(qName, "ownerId")) {
+         builder.ownerId(currentOrNull(currentText));
+      } else if (equalsOrSuffix(qName, "groupDescription")) {
+         builder.description(currentOrNull(currentText));
+      }
+      currentText = new StringBuilder();
+   }
+
+   protected void endItem(String uri, String name, String qName) throws SAXException {
+      if (inIpPermissions) {
+         if (itemDepth == 2)
+            builder.ipPermission(ipPermissionHandler.getResult());
+         else
+            ipPermissionHandler.endElement(uri, name, qName);
+      }
+   }
+
+   /**
+    * {@inheritDoc}
+    */
+   @Override
+   public void characters(char ch[], int start, int length) {
+      if (inIpPermissions) {
+         ipPermissionHandler.characters(ch, start, length);
+      } else {
+         currentText.append(ch, start, length);
+      }
+   }
+
+}

apis/ec2/src/test/java/org/jclouds/ec2/xml/DescribeSecurityGroupsResponseHandlerTest.java

@@ -26,7 +26,7 @@ import static org.testng.Assert.assertEquals;
 import java.io.InputStream;
 import java.util.Set;
 
-import org.jclouds.ec2.domain.IpPermissionImpl;
+import org.jclouds.ec2.domain.IpPermission;
 import org.jclouds.ec2.domain.IpProtocol;
 import org.jclouds.ec2.domain.SecurityGroup;
 import org.jclouds.http.functions.ParseSax;
@@ -54,17 +54,17 @@ public class DescribeSecurityGroupsResponseHandlerTest extends BaseEC2HandlerTes
 
       Set<SecurityGroup> expected = ImmutableSet.of(
             new SecurityGroup(defaultRegion, null, "WebServers", "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM", "Web Servers",
-                  ImmutableSet.of(new IpPermissionImpl(IpProtocol.TCP, 80, 80, ImmutableMultimap.<String, String> of(),
+                  ImmutableSet.of(new IpPermission(IpProtocol.TCP, 80, 80, ImmutableMultimap.<String, String> of(),
                         ImmutableSet.<String> of(), ImmutableSet.of("0.0.0.0/0")))),
             new SecurityGroup(defaultRegion, null, "RangedPortsBySource", "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM", "Group A",
-                  ImmutableSet.of(new IpPermissionImpl(IpProtocol.TCP, 6000, 7000, ImmutableMultimap
+                  ImmutableSet.of(new IpPermission(IpProtocol.TCP, 6000, 7000, ImmutableMultimap
                         .<String, String> of(), ImmutableSet.<String> of(), ImmutableSet.<String> of()))));
 
       DescribeSecurityGroupsResponseHandler handler = injector.getInstance(DescribeSecurityGroupsResponseHandler.class);
       addDefaultRegionToHandler(handler);
       Set<SecurityGroup> result = factory.create(handler).parse(is);
 
-      assertEquals(result, expected);
+      assertEquals(result.toString(), expected.toString());
    }
 
    // Response from OpenStack 1.1 EC2 API
@@ -78,16 +78,16 @@ public class DescribeSecurityGroupsResponseHandlerTest extends BaseEC2HandlerTes
       Set<SecurityGroup> expected = ImmutableSet.of(
             new SecurityGroup(defaultRegion, null, "jclouds#cluster#world", "UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM", "Cluster",
                   ImmutableSet.of(
-                        new IpPermissionImpl(IpProtocol.TCP, 22, 22, ImmutableMultimap.<String, String> of(),
+                        new IpPermission(IpProtocol.TCP, 22, 22, ImmutableMultimap.<String, String> of(),
                               ImmutableSet.<String> of(), ImmutableSet.of("0.0.0.0/0")),
-                        new IpPermissionImpl(IpProtocol.ALL, -1, -1, userIdGroupPairs,
+                        new IpPermission(IpProtocol.ALL, -1, -1, userIdGroupPairs,
                               ImmutableSet.<String> of(), ImmutableSet.<String> of()))));
 
       DescribeSecurityGroupsResponseHandler handler = injector.getInstance(DescribeSecurityGroupsResponseHandler.class);
       addDefaultRegionToHandler(handler);
       Set<SecurityGroup> result = factory.create(handler).parse(is);
 
-      assertEquals(result, expected);
+      assertEquals(result.toString(), expected.toString());
    }
 
    private void addDefaultRegionToHandler(ParseSax.HandlerWithResult<?> handler) {

apis/ec2/src/test/resources/describe_securitygroups.xml

@@ -1,4 +1,5 @@
-<DescribeSecurityGroupsResponse xmlns="http://ec2.amazonaws.com/doc/2009-11-30/">
+<DescribeSecurityGroupsResponse
+	xmlns="http://ec2.amazonaws.com/doc/2009-11-30/">
 	<securityGroupInfo>
 		<item>
 			<ownerId>UYY3TLBUXIEON5NQVUUX6OMPWBZIQNFM</ownerId>

apis/ec2/src/test/resources/describe_securitygroups_empty.xml

@@ -1,8 +1,8 @@
 <?xml version="1.0" ?>
-<DescribeSecurityGroupsResponse xmlns="http://ec2.amazonaws.com/doc/2010-06-15/">
+<DescribeSecurityGroupsResponse
+	xmlns="http://ec2.amazonaws.com/doc/2010-06-15/">
 	<requestId>L6EFIZVPJS76T3K5-0UV</requestId>
 	<securityGroupInfo>
-
 		<item>
 			<ipPermissions>
 				<item>

providers/aws-ec2/src/test/java/org/jclouds/aws/ec2/parse/DescribeSecurityGroupsResponseTest.java

@@ -0,0 +1,87 @@
+/**
+ * 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.aws.ec2.parse;
+
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.testng.Assert.assertEquals;
+
+import java.io.InputStream;
+import java.util.Set;
+
+import org.jclouds.ec2.domain.IpPermission;
+import org.jclouds.ec2.domain.IpProtocol;
+import org.jclouds.ec2.domain.SecurityGroup;
+import org.jclouds.ec2.xml.BaseEC2HandlerTest;
+import org.jclouds.ec2.xml.DescribeSecurityGroupsResponseHandler;
+import org.jclouds.http.functions.ParseSax;
+import org.jclouds.rest.internal.GeneratedHttpRequest;
+import org.testng.annotations.Test;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+
+/**
+ *
+ * @author Adrian Cole
+ */
+// NOTE:without testName, this will not call @Before* and fail w/NPE during
+// surefire
+@Test(groups = "unit", testName = "DescribeSecurityGroupsResponseTest")
+public class DescribeSecurityGroupsResponseTest extends BaseEC2HandlerTest {
+
+   public void test() {
+      InputStream is = getClass().getResourceAsStream("/describe_security_groups_vpc.xml");
+
+      Set<SecurityGroup> expected = expected();
+
+      DescribeSecurityGroupsResponseHandler handler = injector.getInstance(DescribeSecurityGroupsResponseHandler.class);
+      addDefaultRegionToHandler(handler);
+      Set<SecurityGroup> result = factory.create(handler).parse(is);
+
+      assertEquals(result.toString(), expected.toString());
+   }
+
+   public Set<SecurityGroup> expected() {
+      return ImmutableSet.of(SecurityGroup.builder()
+                                          .region(defaultRegion)
+                                          .ownerId("123123123123")
+                                          .id("sg-11111111")
+                                          .name("default")
+                                          .description("default VPC security group")
+//                                          .vpcId("vpc-99999999")
+                                          .ipPermission(IpPermission.builder()
+                                                                    .ipProtocol(IpProtocol.ALL)
+                                                                    .userIdGroupPair("123123123123","sg-11111111").build())
+//                                          .ipPermissionEgress(IpPermission.builder()
+//                                                                    .ipProtocol(IpProtocol.ALL)
+//                                                                    .ipRange("0.0.0.0/0").build())
+                                          .build());
+
+   }
+
+   private void addDefaultRegionToHandler(ParseSax.HandlerWithResult<?> handler) {
+      GeneratedHttpRequest request = createMock(GeneratedHttpRequest.class);
+      expect(request.getArgs()).andReturn(ImmutableList.<Object> of()).atLeastOnce();
+      replay(request);
+      handler.setContext(request);
+   }
+
+}

providers/aws-ec2/src/test/resources/describe_security_groups_vpc.xml

@@ -0,0 +1,35 @@
+<DescribeSecurityGroupsResponse xmlns="http://ec2.amazonaws.com/doc/2011-05-15/">
+        <requestId>xxxxxxxxxxxxxxxx</requestId>
+        <securityGroupInfo>
+            <item>
+                <ownerId>123123123123</ownerId>
+                <groupId>sg-11111111</groupId>
+                <groupName>default</groupName>
+                <groupDescription>default VPC security group</groupDescription>
+                <vpcId>vpc-99999999</vpcId>
+                <ipPermissions>
+                    <item>
+                        <ipProtocol>-1</ipProtocol>
+                        <groups>
+                            <item>
+                                <userId>123123123123</userId>
+                                <groupId>sg-11111111</groupId>
+                            </item>
+                        </groups>
+                        <ipRanges/>
+                    </item>
+                </ipPermissions>
+                <ipPermissionsEgress>
+                    <item>
+                        <ipProtocol>-1</ipProtocol>
+                        <groups/>
+                        <ipRanges>
+                            <item>
+                                <cidrIp>0.0.0.0/0</cidrIp>
+                            </item>
+                        </ipRanges>
+                    </item>
+                </ipPermissionsEgress>
+            </item>
+    </securityGroupInfo>
+</DescribeSecurityGroupsResponse>