SecurityGroupRulesCmd code cleanup review comments handled

Author: DaanHoogland

core/src/com/cloud/agent/api/SecurityGroupRulesCmd.java

@@ -33,6 +33,7 @@
 import com.cloud.utils.net.NetUtils;
 
 public class SecurityGroupRulesCmd extends Command {
+    private static final String CIDR_LENGTH_SEPARATOR = "/";
     private static final char RULE_TARGET_SEPARATOR = ',';
     private static final char RULE_COMMAND_SEPARATOR = ':';
     protected static final String EGRESS_RULE = "E:";
@@ -155,11 +156,10 @@ public String getVmName() {
         return vmName;
     }
 
-    //convert cidrs in the form "a.b.c.d/e" to "hexvalue of 32bit ip/e"
-    private String compressCidr(final String cidr) {
-        final String[] toks = cidr.split("/");
+    private String compressCidrToHexRepresentation(final String cidr) {
+        final String[] toks = cidr.split(CIDR_LENGTH_SEPARATOR);
         final long ipnum = NetUtils.ip2Long(toks[0]);
-        return Long.toHexString(ipnum) + "/" + toks[1];
+        return Long.toHexString(ipnum) + CIDR_LENGTH_SEPARATOR + toks[1];
     }
 
     public String getSecIpsString() {
@@ -189,42 +189,41 @@ public String stringifyCompressedRules() {
         return ruleBuilder.toString();
     }
 
-    /**
-     * @param ipPandPs
-     * @param gression
-     * @param compress
-     * @param ruleBuilder
-     */
-    private void stringifyRulesFor(
-            final List<IpPortAndProto> ipPandPs,
-            final String gression,
-            final boolean compress,
-            final StringBuilder ruleBuilder) {
-        for (final IpPortAndProto ipPandP : ipPandPs) {
-            ruleBuilder.append(gression).append(ipPandP.getProto()).append(RULE_COMMAND_SEPARATOR).append(ipPandP.getStartPort()).append(RULE_COMMAND_SEPARATOR).append(ipPandP.getEndPort()).append(RULE_COMMAND_SEPARATOR);
+    private void stringifyRulesFor(final List<IpPortAndProto> ipPortAndProtocols, final String inOrEgress, final boolean compressed, final StringBuilder ruleBuilder) {
+        for (final IpPortAndProto ipPandP : ipPortAndProtocols) {
+            ruleBuilder.append(inOrEgress).append(ipPandP.getProto()).append(RULE_COMMAND_SEPARATOR).append(ipPandP.getStartPort()).append(RULE_COMMAND_SEPARATOR)
+                    .append(ipPandP.getEndPort()).append(RULE_COMMAND_SEPARATOR);
             for (final String cidr : ipPandP.getAllowedCidrs()) {
-                ruleBuilder.append(compress?compressCidr(cidr):cidr).append(RULE_TARGET_SEPARATOR);
+                ruleBuilder.append(represent(cidr, compressed)).append(RULE_TARGET_SEPARATOR);
             }
             ruleBuilder.append("NEXT ");
         }
     }
 
-    /*
+    private String represent(final String cidr, final boolean compressed) {
+        if (compressed) {
+            return compressCidrToHexRepresentation(cidr);
+        } else {
+            return cidr;
+        }
+    }
+
+    /**
      * Compress the security group rules using zlib compression to allow the call to the hypervisor
      * to scale beyond 8k cidrs.
+     * Note : not using {@see GZipOutputStream} since that is for files, using {@see DeflaterOutputStream} instead.
+     * {@see GZipOutputStream} gives a different header, although the compression is the same
      */
     public String compressStringifiedRules() {
         final String stringified = stringifyRules();
         final ByteArrayOutputStream out = new ByteArrayOutputStream();
         String encodedResult = null;
         try {
-            //Note : not using GZipOutputStream since that is for files
-            //GZipOutputStream gives a different header, although the compression is the same
             final DeflaterOutputStream dzip = new DeflaterOutputStream(out);
             dzip.write(stringified.getBytes());
             dzip.close();
             encodedResult = Base64.encodeBase64String(out.toByteArray());
-        } catch (IOException e) {
+        } catch (final IOException e) {
             LOGGER.warn("Exception while compressing security group rules");
         }
         return encodedResult;
@@ -246,8 +245,11 @@ public Long getVmId() {
         return vmId;
     }
 
+    /**
+     * used for logging
+     * @return the number of Cidrs in the in and egress rule sets for this security group rules command.
+     */
     public int getTotalNumCidrs() {
-        //useful for logging
         int count = 0;
         for (final IpPortAndProto i : ingressRuleSet) {
             count += i.allowedCidrs.size();

core/test/com/cloud/agent/api/SecurityGroupRulesCmdTest.java

@@ -25,7 +25,6 @@
 import java.util.Vector;
 
 import org.junit.Before;
-import org.junit.BeforeClass;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.runners.MockitoJUnitRunner;
@@ -40,31 +39,24 @@
 public class SecurityGroupRulesCmdTest {
     private SecurityGroupRulesCmd securityGroupRulesCmd;
 
-    /**
-     * @throws java.lang.Exception
-     */
-    @BeforeClass
-    public static void setUpBeforeClass() throws Exception {
-    }
-
     /**
      * @throws java.lang.Exception
      */
     @Before
     public void setUp() throws Exception {
-        String guestIp = "10.10.10.10";
-        String guestMac = "aa:aa:aa:aa:aa:aa";
-        String vmName = "vm";
-        Long vmId = 1L;
-        String signature = "sig";
-        Long seqNum = 0L;
-        String proto = "abc";
-        int startPort = 1;
-        int endPort = 2;
-        String[] allowedCidrs = new String[] {"1.2.3.4/5","6.7.8.9/0"};
-        IpPortAndProto[] ingressRuleSet = new IpPortAndProto[]{new IpPortAndProto(proto, startPort, endPort, allowedCidrs)};
-        IpPortAndProto[] egressRuleSet = new IpPortAndProto[]{new IpPortAndProto(proto, startPort, endPort, allowedCidrs)};
-        List<String> secIps = new Vector<String>();
+        final String guestIp = "10.10.10.10";
+        final String guestMac = "aa:aa:aa:aa:aa:aa";
+        final String vmName = "vm";
+        final Long vmId = 1L;
+        final String signature = "sig";
+        final Long seqNum = 0L;
+        final String proto = "abc";
+        final int startPort = 1;
+        final int endPort = 2;
+        final String[] allowedCidrs = new String[] {"1.2.3.4/5","6.7.8.9/0"};
+        final IpPortAndProto[] ingressRuleSet = new IpPortAndProto[]{new IpPortAndProto(proto, startPort, endPort, allowedCidrs)};
+        final IpPortAndProto[] egressRuleSet = new IpPortAndProto[]{new IpPortAndProto(proto, startPort, endPort, allowedCidrs)};
+        final List<String> secIps = new Vector<String>();
         securityGroupRulesCmd = new SecurityGroupRulesCmd(guestIp, guestMac, vmName, vmId, signature, seqNum, ingressRuleSet, egressRuleSet, secIps);
     }
 
@@ -73,7 +65,7 @@ public void setUp() throws Exception {
      */
     @Test
     public void testStringifyRules() throws Exception {
-        String a = securityGroupRulesCmd.stringifyRules();
+        final String a = securityGroupRulesCmd.stringifyRules();
 // do verification on a
         assertTrue(a.contains(SecurityGroupRulesCmd.EGRESS_RULE));
     }
@@ -83,7 +75,7 @@ public void testStringifyRules() throws Exception {
      */
     @Test
     public void testStringifyCompressedRules() throws Exception {
-        String a = securityGroupRulesCmd.stringifyCompressedRules();
+        final String a = securityGroupRulesCmd.stringifyCompressedRules();
 // do verification on a
         assertTrue(a.contains(SecurityGroupRulesCmd.EGRESS_RULE));
     }
@@ -93,8 +85,8 @@ public void testStringifyCompressedRules() throws Exception {
      */
     @Test
     public void testCompressStringifiedRules() throws Exception {
-        String compressed = "eJzztEpMSrYytDKyMtQz0jPWM9E31THTM9ez0LPUN9Dxc40IUXAlrAQAPdoP3Q==";
-        String a = securityGroupRulesCmd.compressStringifiedRules();
+        final String compressed = "eJzztEpMSrYytDKyMtQz0jPWM9E31THTM9ez0LPUN9Dxc40IUXAlrAQAPdoP3Q==";
+        final String a = securityGroupRulesCmd.compressStringifiedRules();
         assertTrue(compressed.equals(a));
     }