Bug 8408 - Deserialization exception in logs when NAT flows are added.

The code is modified to fix the issues with multiple CT actions.

The logic skip padded bit is added during Deserialization

Change-Id: Ic5574f0a877228d966ca92eacfa22c751fa14b3c
Signed-off-by: Aswin Suryanarayanan <asuryana@redhat.com>

diff --git a/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodec.java b/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodec.java
index ea41785e72224ae834e3f30e46e72f3b7032c1ef..dbaf1c75188548f5972d9ea48d90d9ba8601f6a1 100644 (file)
--- a/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodec.java
+++ b/extension/openflowjava-extension-nicira/src/main/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodec.java
@@ -67,7 +67,7 @@ public class ConntrackCodec extends AbstractActionCodec {
         outBuffer.writeShort(action.getNxActionConntrack().getConntrackZone().shortValue());
         outBuffer.writeByte(action.getNxActionConntrack().getRecircTable().byteValue());
         outBuffer.writeZero(5);
-        serializeCtAction(outBuffer,action, length);
+        serializeCtAction(outBuffer,action);
     }
 
     private int getActionLength(final ActionConntrack action) {
@@ -78,37 +78,46 @@ public class ConntrackCodec extends AbstractActionCodec {
         }
         for (CtActions ctActions : ctActionsList) {
             if (ctActions.getOfpactActions() instanceof NxActionNatCase) {
-                length += NX_NAT_LENGTH;
                 NxActionNatCase nxActionNatCase = (NxActionNatCase)ctActions.getOfpactActions();
                 NxActionNat natAction = nxActionNatCase.getNxActionNat();
-                short rangePresent = natAction.getRangePresent().shortValue();
-                if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MIN.getIntValue())) {
-                    length += INT_LENGTH;
+                int natLength = getNatActionLength(natAction);
+                int pad = 8 - (natLength % 8);
+                length += natLength + pad;
                 }
-                if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MAX.getIntValue())) {
-                    length += INT_LENGTH;
-                }
-                if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMIN.getIntValue())) {
-                    length += SHORT_LENGTH;
-                }
-                if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMAX.getIntValue())) {
-                    length += SHORT_LENGTH;
-                }
-            }
         }
         LOG.trace("ActionLength :conntrack: length {}",length);
         return length;
     }
 
-    private void serializeCtAction(final ByteBuf outBuffer, final ActionConntrack action, final int length) {
+    private int getNatActionLength(final NxActionNat natAction) {
+        int natLength = NX_NAT_LENGTH;
+        short rangePresent = natAction.getRangePresent().shortValue();
+        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MIN.getIntValue())) {
+            natLength += INT_LENGTH;
+        }
+        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MAX.getIntValue())) {
+            natLength += INT_LENGTH;
+        }
+        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMIN.getIntValue())) {
+            natLength += SHORT_LENGTH;
+        }
+        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMAX.getIntValue())) {
+            natLength += SHORT_LENGTH;
+        }
+        return natLength;
+
+    }
+
+    private void serializeCtAction(final ByteBuf outBuffer, final ActionConntrack action) {
         List<CtActions> ctActionsList = action.getNxActionConntrack().getCtActions();
         if (ctActionsList != null) {
             for (CtActions ctActions : ctActionsList) {
                 if (ctActions.getOfpactActions() instanceof NxActionNatCase){
                     NxActionNatCase nxActionNatCase = (NxActionNatCase)ctActions.getOfpactActions();
                     NxActionNat natAction = nxActionNatCase.getNxActionNat();
-                    int pad = length % 8;
-                    serializeHeader(length + pad, NXAST_NAT_SUBTYPE, outBuffer);
+                    int natLength = getNatActionLength(natAction);
+                    int pad = 8 - (natLength % 8);
+                    serializeHeader(natLength + pad, NXAST_NAT_SUBTYPE, outBuffer);
                     outBuffer.writeZero(2);
                     outBuffer.writeShort(natAction.getFlags().shortValue());
                     short rangePresent = natAction.getRangePresent().shortValue();
@@ -147,7 +156,7 @@ public class ConntrackCodec extends AbstractActionCodec {
         nxActionConntrackBuilder.setRecircTable(message.readUnsignedByte());
         message.skipBytes(5);
         if  (length > CT_LENGTH) {
-            dserializeCtAction(message,nxActionConntrackBuilder);
+            dserializeCtAction(message,nxActionConntrackBuilder, length - CT_LENGTH);
         }
         ActionBuilder actionBuilder = new ActionBuilder();
         actionBuilder.setExperimenterId(getExperimenterId());
@@ -157,35 +166,41 @@ public class ConntrackCodec extends AbstractActionCodec {
         return actionBuilder.build();
     }
 
-    private void dserializeCtAction(final ByteBuf message, final NxActionConntrackBuilder nxActionConntrackBuilder) {
-        deserializeCtHeader(message);
-
-        NxActionNatBuilder nxActionNatBuilder = new NxActionNatBuilder();
-        message.skipBytes(2);
-        nxActionNatBuilder.setFlags(message.readUnsignedShort());
-
-        int rangePresent = message.readUnsignedShort();
-        nxActionNatBuilder.setRangePresent(rangePresent);
-        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MIN.getIntValue())) {
-            InetAddress address = InetAddresses.fromInteger((int)message.readUnsignedInt());
-            nxActionNatBuilder.setIpAddressMin(new IpAddress(address.getHostAddress().toCharArray()));
-        }
-        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MAX.getIntValue())) {
-            InetAddress address = InetAddresses.fromInteger((int)message.readUnsignedInt());
-            nxActionNatBuilder.setIpAddressMax(new IpAddress(address.getHostAddress().toCharArray()));
-        }
-        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMIN.getIntValue())) {
-            nxActionNatBuilder.setPortMin(message.readUnsignedShort());
-        }
-        if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMAX.getIntValue())) {
-            nxActionNatBuilder.setPortMax(message.readUnsignedShort());
-        }
-        NxActionNatCaseBuilder caseBuilder = new NxActionNatCaseBuilder();
-        caseBuilder.setNxActionNat(nxActionNatBuilder.build());
-        CtActionsBuilder ctActionsBuilder = new CtActionsBuilder();
-        ctActionsBuilder.setOfpactActions(caseBuilder.build());
+    private void dserializeCtAction(final ByteBuf message, final NxActionConntrackBuilder nxActionConntrackBuilder,
+            int ctActionsLength) {
         List<CtActions> ctActionsList = new ArrayList<>();
-        ctActionsList.add(ctActionsBuilder.build());
+        while (ctActionsLength > 0){
+            int startIndex = message.readerIndex();
+            int length = deserializeCtHeader(message);
+            ctActionsLength = ctActionsLength - length;
+            NxActionNatBuilder nxActionNatBuilder = new NxActionNatBuilder();
+            message.skipBytes(2);
+            nxActionNatBuilder.setFlags(message.readUnsignedShort());
+
+            int rangePresent = message.readUnsignedShort();
+            nxActionNatBuilder.setRangePresent(rangePresent);
+            if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MIN.getIntValue())) {
+                InetAddress address = InetAddresses.fromInteger((int)message.readUnsignedInt());
+                nxActionNatBuilder.setIpAddressMin(new IpAddress(address.getHostAddress().toCharArray()));
+            }
+            if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEIPV4MAX.getIntValue())) {
+                InetAddress address = InetAddresses.fromInteger((int)message.readUnsignedInt());
+                nxActionNatBuilder.setIpAddressMax(new IpAddress(address.getHostAddress().toCharArray()));
+            }
+            if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMIN.getIntValue())) {
+                nxActionNatBuilder.setPortMin(message.readUnsignedShort());
+            }
+            if (0 != (rangePresent & NxActionNatRangePresent.NXNATRANGEPROTOMAX.getIntValue())) {
+                nxActionNatBuilder.setPortMax(message.readUnsignedShort());
+            }
+            NxActionNatCaseBuilder caseBuilder = new NxActionNatCaseBuilder();
+            caseBuilder.setNxActionNat(nxActionNatBuilder.build());
+            CtActionsBuilder ctActionsBuilder = new CtActionsBuilder();
+            ctActionsBuilder.setOfpactActions(caseBuilder.build());
+            ctActionsList.add(ctActionsBuilder.build());
+            int pad = length - (message.readerIndex() - startIndex);
+            message.skipBytes(pad);
+        }
         nxActionConntrackBuilder.setCtActions(ctActionsList);
     }
 

diff --git a/extension/openflowjava-extension-nicira/src/test/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodecTest.java b/extension/openflowjava-extension-nicira/src/test/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodecTest.java
index 7c73d4a37306f644c442ca3beccfe39d1d420465..df699deb1d698ca794101242db8a87e5e25e32f7 100644 (file)
--- a/extension/openflowjava-extension-nicira/src/test/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodecTest.java
+++ b/extension/openflowjava-extension-nicira/src/test/java/org/opendaylight/openflowjava/nx/codec/action/ConntrackCodecTest.java
@@ -44,7 +44,8 @@ public class ConntrackCodecTest {
 
     private final int length = 24;
     private final byte nxastConntrackSubtype = 35;
-    private final int nxNatLength = 32;
+    private final int nxNatLengthAction1 = 32;
+    private final int nxNatLengthAction2 = 24;
     private final byte nxastNatSubtype = 36;
 
     @Before
@@ -58,9 +59,9 @@ public class ConntrackCodecTest {
         action = createAction();
         conntrackCodec.serialize(action, buffer);
 
-        Assert.assertEquals(56, buffer.readableBytes());
+        Assert.assertEquals(length + nxNatLengthAction1 + nxNatLengthAction2, buffer.readableBytes());
         Assert.assertEquals(EncodeConstants.EXPERIMENTER_VALUE, buffer.readUnsignedShort());
-        Assert.assertEquals(length + nxNatLength, buffer.readUnsignedShort());
+        Assert.assertEquals(length + nxNatLengthAction1 + nxNatLengthAction2, buffer.readUnsignedShort());
         Assert.assertEquals(NiciraConstants.NX_VENDOR_ID.intValue(), buffer.readUnsignedInt());
         Assert.assertEquals(nxastConntrackSubtype, buffer.readUnsignedShort());
         Assert.assertEquals(1, buffer.readUnsignedShort());
@@ -69,7 +70,7 @@ public class ConntrackCodecTest {
         Assert.assertEquals(4, buffer.readByte());
         buffer.skipBytes(5);
         Assert.assertEquals(EncodeConstants.EXPERIMENTER_VALUE, buffer.readUnsignedShort());
-        Assert.assertEquals(nxNatLength, buffer.readUnsignedShort());
+        Assert.assertEquals(nxNatLengthAction1, buffer.readUnsignedShort());
         Assert.assertEquals(NiciraConstants.NX_VENDOR_ID.intValue(), buffer.readUnsignedInt());
         Assert.assertEquals(nxastNatSubtype, buffer.readUnsignedShort());
         buffer.skipBytes(2);
@@ -80,6 +81,16 @@ public class ConntrackCodecTest {
         Assert.assertEquals(3000, buffer.readUnsignedShort());
         Assert.assertEquals(4000, buffer.readUnsignedShort());
         buffer.skipBytes(4);
+        Assert.assertEquals(EncodeConstants.EXPERIMENTER_VALUE, buffer.readUnsignedShort());
+        Assert.assertEquals(nxNatLengthAction2, buffer.readUnsignedShort());
+        Assert.assertEquals(NiciraConstants.NX_VENDOR_ID.intValue(), buffer.readUnsignedInt());
+        Assert.assertEquals(nxastNatSubtype, buffer.readUnsignedShort());
+        buffer.skipBytes(2);
+        Assert.assertEquals(5, buffer.readUnsignedShort());
+        Assert.assertEquals(0x21, buffer.readUnsignedShort());
+        Assert.assertEquals(3232235520L, buffer.readUnsignedInt());
+        Assert.assertEquals(4000, buffer.readUnsignedShort());
+        buffer.skipBytes(2);
     }
 
     @Test
@@ -119,7 +130,12 @@ public class ConntrackCodecTest {
         Assert.assertEquals("192.168.10.0", natAction.getIpAddressMax().getIpv4Address().getValue());
         Assert.assertEquals(3000, natAction.getPortMin().shortValue());
         Assert.assertEquals(4000, natAction.getPortMax().shortValue());
-
+        nxActionNatCase = (NxActionNatCase) ctActions.get(1).getOfpactActions();
+        natAction = nxActionNatCase.getNxActionNat();
+        Assert.assertEquals(5, natAction.getFlags().shortValue());
+        Assert.assertEquals(0x21, natAction.getRangePresent().intValue());
+        Assert.assertEquals("192.168.0.0", natAction.getIpAddressMin().getIpv4Address().getValue());
+        Assert.assertEquals(4000, natAction.getPortMax().shortValue());
     }
 
     @Test
@@ -156,6 +172,16 @@ public class ConntrackCodecTest {
         ctActionsBuilder.setOfpactActions(nxActionNatCaseBuilder.build());
         List<CtActions> ctActionsList = new  ArrayList<>();
         ctActionsList.add(ctActionsBuilder.build());
+        nxActionNatBuilder = new NxActionNatBuilder();
+        nxActionNatBuilder.setFlags(5);
+        nxActionNatBuilder.setRangePresent(0x21);
+        nxActionNatBuilder.setIpAddressMin(new IpAddress("192.168.0.0".toCharArray()));
+        nxActionNatBuilder.setPortMax(4000);
+        nxActionNatCaseBuilder = new NxActionNatCaseBuilder();
+        nxActionNatCaseBuilder.setNxActionNat(nxActionNatBuilder.build());
+        ctActionsBuilder = new CtActionsBuilder();
+        ctActionsBuilder.setOfpactActions(nxActionNatCaseBuilder.build());
+        ctActionsList.add(ctActionsBuilder.build());
         nxActionConntrackBuilder.setCtActions(ctActionsList);
 
         ExperimenterId experimenterId = new ExperimenterId(NiciraConstants.NX_VENDOR_ID);
@@ -188,7 +214,7 @@ public class ConntrackCodecTest {
 
     private void createBufer(ByteBuf message) {
         message.writeShort(EncodeConstants.EXPERIMENTER_VALUE);
-        message.writeShort(length + nxastNatSubtype);
+        message.writeShort(length + nxNatLengthAction1 + nxNatLengthAction2);
         message.writeInt(NiciraConstants.NX_VENDOR_ID.intValue());
         message.writeShort(nxastConntrackSubtype);
         //FLAG = 1
@@ -202,7 +228,7 @@ public class ConntrackCodecTest {
         //ADDS 5 empty bytes
         message.writeZero(5);
         message.writeShort(EncodeConstants.EXPERIMENTER_VALUE);
-        message.writeShort(nxNatLength);
+        message.writeShort(nxNatLengthAction1);
         message.writeInt(NiciraConstants.NX_VENDOR_ID.intValue());
         message.writeShort(nxastNatSubtype);
         message.writeZero(2);
@@ -218,6 +244,22 @@ public class ConntrackCodecTest {
         message.writeShort(3000);
         //PORT MAX
         message.writeShort(4000);
+        message.writeZero(4);
+
+        message.writeShort(EncodeConstants.EXPERIMENTER_VALUE);
+        message.writeShort(nxNatLengthAction2);
+        message.writeInt(NiciraConstants.NX_VENDOR_ID.intValue());
+        message.writeShort(nxastNatSubtype);
+        message.writeZero(2);
+        //NAT FLAG
+        message.writeShort(5);
+        //RANGE PRESENT
+        message.writeShort(0x21);
+        //IP ADDRESS MIN
+        message.writeBytes(IetfInetUtil.INSTANCE.ipv4AddressBytes(new Ipv4Address("192.168.0.0")));
+        //PORT MAX
+        message.writeShort(4000);
+        message.writeZero(2);
     }
 
     private void createBuferWIthoutCtAction(ByteBuf message) {