Bug 6365 - Bad handling of unknown AFI/SAFI in Open Message

Skip unsupported AFI/SAFI when parsing Open Message, instead of throwing
exception, which resulted in dopped connection.

Change-Id: Ib3a47ca9722ecf0f0d6eacd84b7fab168c5860a1
Signed-off-by: Milos Fabian <milfabia@cisco.com>
(cherry picked from commit ba17fbc05002d6980ab41476696b134d46871ac3)

diff --git a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/BGPRouteRefreshMessageParser.java b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/BGPRouteRefreshMessageParser.java
index 41c36c9bd193cc68a7814408f94da667e8a7f8cb..29f1bc6cfbdf5776da22bc5413f0e1cfdcc473d5 100644 (file)
--- a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/BGPRouteRefreshMessageParser.java
+++ b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/BGPRouteRefreshMessageParser.java
@@ -11,9 +11,9 @@ import com.google.common.base.Preconditions;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.ByteBufUtil;
 import io.netty.buffer.Unpooled;
+import java.util.Optional;
 import org.opendaylight.protocol.bgp.parser.BGPDocumentedException;
 import org.opendaylight.protocol.bgp.parser.BGPError;
-import org.opendaylight.protocol.bgp.parser.BGPParsingException;
 import org.opendaylight.protocol.bgp.parser.spi.AddressFamilyRegistry;
 import org.opendaylight.protocol.bgp.parser.spi.MessageParser;
 import org.opendaylight.protocol.bgp.parser.spi.MessageSerializer;
@@ -75,12 +75,10 @@ public final class BGPRouteRefreshMessageParser implements MessageParser, Messag
         if (body.readableBytes() < TRIPLET_BYTE_SIZE) {
             throw BGPDocumentedException.badMessageLength("RouteRefresh message is too small.", messageLength);
         }
-        try {
-            final BgpTableType parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(body, this.afiReg, this.safiReg);
-            return new RouteRefreshBuilder().setAfi(parsedAfiSafi.getAfi()).setSafi(parsedAfiSafi.getSafi()).build();
-        } catch (final BGPParsingException e) {
-            LOG.warn("Fail to parse BGP RouteRefresh message.", e);
+        final Optional<BgpTableType> parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(body, this.afiReg, this.safiReg);
+        if (!parsedAfiSafi.isPresent()) {
             throw new BGPDocumentedException("Unsupported afi/safi in Route Refresh message.", BGPError.WELL_KNOWN_ATTR_NOT_RECOGNIZED);
         }
+        return new RouteRefreshBuilder(parsedAfiSafi.get()).build();
     }
 }

diff --git a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/CapabilityParameterParser.java b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/CapabilityParameterParser.java
index cb35a54c0f138764fa775fa89f78de107bb8b33b..32dccd3bbe4286bdfc90164cac8bb829b8404ad0 100644 (file)
--- a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/CapabilityParameterParser.java
+++ b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/CapabilityParameterParser.java
@@ -66,7 +66,7 @@ public final class CapabilityParameterParser implements ParameterParser, Paramet
         final ByteBuf paramBody = buffer.readSlice(capLength);
         final CParameters ret = this.reg.parseCapability(capCode, paramBody);
         if (ret == null) {
-            LOG.debug("Ignoring unsupported capability {}", capCode);
+            LOG.info("Ignoring unsupported capability {}", capCode);
             return null;
         }
         return new OptionalCapabilitiesBuilder().setCParameters(ret).build();

diff --git a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/MultiProtocolCapabilityHandler.java b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/MultiProtocolCapabilityHandler.java
index 7e85914f4d8713d9911129dcc212be13e66c5c18..880593deadfe1c1b8527640335ce01e13d8175aa 100644 (file)
--- a/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/MultiProtocolCapabilityHandler.java
+++ b/bgp/parser-impl/src/main/java/org/opendaylight/protocol/bgp/parser/impl/message/open/MultiProtocolCapabilityHandler.java
@@ -10,6 +10,7 @@ package org.opendaylight.protocol.bgp.parser.impl.message.open;
 import com.google.common.base.Preconditions;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import java.util.Optional;
 import org.opendaylight.protocol.bgp.parser.BGPDocumentedException;
 import org.opendaylight.protocol.bgp.parser.BGPParsingException;
 import org.opendaylight.protocol.bgp.parser.spi.AddressFamilyRegistry;
@@ -27,6 +28,7 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.mult
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.multiprotocol.rev130919.mp.capabilities.MultiprotocolCapabilityBuilder;
 
 public final class MultiProtocolCapabilityHandler implements CapabilityParser, CapabilitySerializer {
+
     public static final int CODE = 1;
 
     private final AddressFamilyRegistry afiReg;
@@ -39,10 +41,12 @@ public final class MultiProtocolCapabilityHandler implements CapabilityParser, C
 
     @Override
     public CParameters parseCapability(final ByteBuf buffer) throws BGPDocumentedException, BGPParsingException {
-        final BgpTableType parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(buffer, this.afiReg, this.safiReg);
-
+        final Optional<BgpTableType> parsedAfiSafiOptional = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(buffer, this.afiReg, this.safiReg);
+        if (!parsedAfiSafiOptional.isPresent()) {
+            return null;
+        }
         return new CParametersBuilder().addAugmentation(CParameters1.class,new CParameters1Builder().setMultiprotocolCapability(
-            new MultiprotocolCapabilityBuilder().setAfi(parsedAfiSafi.getAfi()).setSafi(parsedAfiSafi.getSafi()).build()).build()).build();
+            new MultiprotocolCapabilityBuilder(parsedAfiSafiOptional.get()).build()).build()).build();
     }
 
     @Override

diff --git a/bgp/parser-impl/src/test/java/org/opendaylight/protocol/bgp/parser/impl/MultiProtocolCapabilityHandlerTest.java b/bgp/parser-impl/src/test/java/org/opendaylight/protocol/bgp/parser/impl/MultiProtocolCapabilityHandlerTest.java
index e1d858018deef624a307aba7ceb13c6a1db07d9f..a4b753263b80c42ba0fc12cf8a3717d7c9a2b03f 100644 (file)
--- a/bgp/parser-impl/src/test/java/org/opendaylight/protocol/bgp/parser/impl/MultiProtocolCapabilityHandlerTest.java
+++ b/bgp/parser-impl/src/test/java/org/opendaylight/protocol/bgp/parser/impl/MultiProtocolCapabilityHandlerTest.java
@@ -69,20 +69,6 @@ public class MultiProtocolCapabilityHandlerTest {
         assertEquals(capabilityToSerialize.hashCode(), newCaps.hashCode());
     }
 
-    @Test(expected=BGPParsingException.class)
-    public void testAfiException() throws BGPDocumentedException, BGPParsingException {
-        final ByteBuf bytes = this.serializedBytes.copy();
-        final MultiProtocolCapabilityHandler handler = new MultiProtocolCapabilityHandler(this.afirExpection, this.safir);
-        handler.parseCapability(bytes);
-    }
-
-    @Test(expected=BGPParsingException.class)
-    public void testSafiException() throws BGPDocumentedException, BGPParsingException {
-        final ByteBuf bytes = this.serializedBytes.copy();
-        final MultiProtocolCapabilityHandler handler = new MultiProtocolCapabilityHandler(this.afir, this.safirException);
-        handler.parseCapability(bytes);
-    }
-
     @Test(expected=IllegalArgumentException.class)
     public void testUnhandledAfi() {
         final CParameters capabilityToSerialize = new CParametersBuilder().addAugmentation(CParameters1.class, new CParameters1Builder().setMultiprotocolCapability(

diff --git a/bgp/parser-spi/src/main/java/org/opendaylight/protocol/bgp/parser/spi/MultiprotocolCapabilitiesUtil.java b/bgp/parser-spi/src/main/java/org/opendaylight/protocol/bgp/parser/spi/MultiprotocolCapabilitiesUtil.java
index 0bcb5973bcf77dc125244bdf8027b0ae56559f1b..6f1452d2fdc7e204c78ce8e4e948eb8fe860ea2c 100644 (file)
--- a/bgp/parser-spi/src/main/java/org/opendaylight/protocol/bgp/parser/spi/MultiprotocolCapabilitiesUtil.java
+++ b/bgp/parser-spi/src/main/java/org/opendaylight/protocol/bgp/parser/spi/MultiprotocolCapabilitiesUtil.java
@@ -9,7 +9,7 @@ package org.opendaylight.protocol.bgp.parser.spi;
 
 import com.google.common.base.Preconditions;
 import io.netty.buffer.ByteBuf;
-import org.opendaylight.protocol.bgp.parser.BGPParsingException;
+import java.util.Optional;
 import org.opendaylight.protocol.bgp.parser.BgpTableTypeImpl;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.message.rev130919.open.message.bgp.parameters.optional.capabilities.CParameters;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.message.rev130919.open.message.bgp.parameters.optional.capabilities.CParametersBuilder;
@@ -19,9 +19,13 @@ import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.mult
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.multiprotocol.rev130919.mp.capabilities.RouteRefreshCapabilityBuilder;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.AddressFamily;
 import org.opendaylight.yang.gen.v1.urn.opendaylight.params.xml.ns.yang.bgp.types.rev130919.SubsequentAddressFamily;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public final class MultiprotocolCapabilitiesUtil {
 
+    private static final Logger LOG = LoggerFactory.getLogger(MultiprotocolCapabilitiesUtil.class);
+
     public static final CParameters RR_CAPABILITY = new CParametersBuilder().addAugmentation(CParameters1.class,
         new CParameters1Builder().setRouteRefreshCapability(new RouteRefreshCapabilityBuilder().build()).build()).build();
 
@@ -42,19 +46,21 @@ public final class MultiprotocolCapabilitiesUtil {
         capBuffer.writeByte(safival);
     }
 
-    public static BgpTableType parseMPAfiSafi(final ByteBuf buffer, final AddressFamilyRegistry afiReg, final SubsequentAddressFamilyRegistry safiReg) throws BGPParsingException {
+    public static Optional<BgpTableType> parseMPAfiSafi(final ByteBuf buffer, final AddressFamilyRegistry afiReg, final SubsequentAddressFamilyRegistry safiReg) {
         final int afiVal = buffer.readUnsignedShort();
         final Class<? extends AddressFamily> afi = afiReg.classForFamily(afiVal);
         if (afi == null) {
-            throw new BGPParsingException("Address Family Identifier: '" + afiVal + "' not supported.");
+            LOG.info("Unsupported AFI {} parsed.", afiVal);
+            return Optional.empty();
         }
         // skip reserved
         buffer.skipBytes(RESERVED);
         final int safiVal = buffer.readUnsignedByte();
         final Class<? extends SubsequentAddressFamily> safi = safiReg.classForFamily(safiVal);
         if (safi == null) {
-            throw new BGPParsingException("Subsequent Address Family Identifier: '" + safiVal + "' not supported.");
+            LOG.info("Unsupported SAFI {} parsed.", safiVal);
+            return Optional.empty();
         }
-        return new BgpTableTypeImpl(afi, safi);
+        return Optional.of(new BgpTableTypeImpl(afi, safi));
     }
 }

diff --git a/bgp/parser-spi/src/test/java/org/opendaylight/protocol/bgp/parser/spi/UtilsTest.java b/bgp/parser-spi/src/test/java/org/opendaylight/protocol/bgp/parser/spi/UtilsTest.java
index fb5f9d0024b322ee174d7c30dac73fa479d6ec73..d581f27afacf579da60db7a81f8ba1493bb58707 100644 (file)
--- a/bgp/parser-spi/src/test/java/org/opendaylight/protocol/bgp/parser/spi/UtilsTest.java
+++ b/bgp/parser-spi/src/test/java/org/opendaylight/protocol/bgp/parser/spi/UtilsTest.java
@@ -9,12 +9,15 @@ package org.opendaylight.protocol.bgp.parser.spi;
 
 import static org.junit.Assert.assertArrayEquals;
 import static org.junit.Assert.assertEquals;
+
 import com.google.common.primitives.UnsignedBytes;
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
 import java.util.Arrays;
+import java.util.Optional;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.Mock;
@@ -36,9 +39,11 @@ public class UtilsTest {
         MockitoAnnotations.initMocks(this);
         Mockito.doReturn(1).when(this.afiReg).numberForClass(Ipv4AddressFamily.class);
         Mockito.doReturn(Ipv4AddressFamily.class).when(this.afiReg).classForFamily(1);
+        Mockito.doReturn(null).when(this.afiReg).classForFamily(2);
 
         Mockito.doReturn(1).when(this.safiReg).numberForClass(UnicastSubsequentAddressFamily.class);
         Mockito.doReturn(UnicastSubsequentAddressFamily.class).when(this.safiReg).classForFamily(1);
+        Mockito.doReturn(null).when(this.safiReg).classForFamily(3);
     }
 
     @Test
@@ -93,7 +98,7 @@ public class UtilsTest {
     public void testMultiprotocolCapabilitiesUtil() throws BGPParsingException {
         final byte[] bytes = new byte[] {0, 1, 0, 1};
         final ByteBuf bytesBuf = Unpooled.copiedBuffer(bytes);
-        final BgpTableType parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(bytesBuf, this.afiReg, this.safiReg);
+        final BgpTableType parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(bytesBuf, this.afiReg, this.safiReg).get();
         assertEquals(Ipv4AddressFamily.class, parsedAfiSafi.getAfi());
         assertEquals(UnicastSubsequentAddressFamily.class, parsedAfiSafi.getSafi());
 
@@ -102,6 +107,22 @@ public class UtilsTest {
         assertArrayEquals(bytes, serializedAfiSafi.array());
     }
 
+    @Test
+    public void testUnsupportedAfi() {
+        final byte[] bytes = new byte[] {0, 2, 0, 1};
+        final ByteBuf bytesBuf = Unpooled.copiedBuffer(bytes);
+        final Optional<BgpTableType> parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(bytesBuf, this.afiReg, this.safiReg);
+        Assert.assertFalse(parsedAfiSafi.isPresent());
+    }
+
+    @Test
+    public void testUnsupportedSafi() {
+        final byte[] bytes = new byte[] {0, 1, 0, 3};
+        final ByteBuf bytesBuf = Unpooled.copiedBuffer(bytes);
+        final Optional<BgpTableType> parsedAfiSafi = MultiprotocolCapabilitiesUtil.parseMPAfiSafi(bytesBuf, this.afiReg, this.safiReg);
+        Assert.assertFalse(parsedAfiSafi.isPresent());
+    }
+
     @Test(expected=UnsupportedOperationException.class)
     public void testAttributeUtilPrivateConstructor() throws Throwable {
         final Constructor<AttributeUtil> c = AttributeUtil.class.getDeclaredConstructor();