From f526f07b5498ab911c32611cada046e24563542c Mon Sep 17 00:00:00 2001 From: Milos Fabian Date: Fri, 30 Jan 2015 14:28:59 +0100 Subject: [PATCH] Bug-2631 - Fixed BGP connection collision detection -have to examine all connections, in OpenConfirm state -before, only fully established connections were examined -connections are kept in map -remove connection from map, when session goes down -avoids https://bugs.opendaylight.org/show_bug.cgi?id=2109 -brings dependency of BGPSessionImpl on BGPPeerRegistry -fixed bgp-identifier conversion to long Change-Id: I700e8e5a0fb0874731ab2fe013cd7853e44428da Signed-off-by: Milos Fabian --- .../impl/AbstractBGPSessionNegotiator.java | 2 +- .../protocol/bgp/rib/impl/BGPSessionImpl.java | 14 ++- .../bgp/rib/impl/StrictBGPPeerRegistry.java | 91 ++++++++----------- .../bgp/rib/impl/ApplicationPeerTest.java | 2 +- .../bgp/rib/impl/BGPDispatcherImplTest.java | 1 + .../bgp/rib/impl/BGPSessionImplTest.java | 2 +- .../bgp/rib/impl/SpeakerSessionMock.java | 2 +- .../rib/impl/StrictBGPPeerRegistryTest.java | 28 +++++- 8 files changed, 82 insertions(+), 60 deletions(-) diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java index b1766e8449..8f5328360c 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/AbstractBGPSessionNegotiator.java @@ -182,7 +182,7 @@ public abstract class AbstractBGPSessionNegotiator extends AbstractSessionNegoti try { final BGPSessionListener peer = this.registry.getPeer(getRemoteIp(), getSourceId(openObj, getPreferences()), getDestinationId(openObj, getPreferences())); this.sendMessage(new KeepaliveBuilder().build()); - this.session = new BGPSessionImpl(peer, this.channel, openObj, getPreferences().getHoldTime()); + this.session = new BGPSessionImpl(peer, this.channel, openObj, getPreferences().getHoldTime(), this.registry); this.state = State.OpenConfirm; LOG.debug("Channel {} moved to OpenConfirm state with remote proposal {}", this.channel, openObj); } catch (final BGPDocumentedException e) { diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java index f5191d7df5..fc931456ee 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImpl.java @@ -23,6 +23,7 @@ import javax.annotation.concurrent.GuardedBy; import org.opendaylight.protocol.bgp.parser.AsNumberUtil; import org.opendaylight.protocol.bgp.parser.BGPError; import org.opendaylight.protocol.bgp.parser.BgpTableTypeImpl; +import org.opendaylight.protocol.bgp.rib.impl.spi.BGPPeerRegistry; import org.opendaylight.protocol.bgp.rib.spi.BGPSession; import org.opendaylight.protocol.bgp.rib.spi.BGPSessionListener; import org.opendaylight.protocol.bgp.rib.spi.BGPTerminationReason; @@ -98,14 +99,17 @@ public class BGPSessionImpl extends AbstractProtocolSession implem private final int keepAlive; private final AsNumber asNumber; private final Ipv4Address bgpId; + private final BGPPeerRegistry peerRegistry; - public BGPSessionImpl(final BGPSessionListener listener, final Channel channel, final Open remoteOpen, final int localHoldTimer) { + public BGPSessionImpl(final BGPSessionListener listener, final Channel channel, final Open remoteOpen, final int localHoldTimer, + final BGPPeerRegistry peerRegitry) { this.listener = Preconditions.checkNotNull(listener); this.channel = Preconditions.checkNotNull(channel); this.holdTimerValue = (remoteOpen.getHoldTimer() < localHoldTimer) ? remoteOpen.getHoldTimer() : localHoldTimer; LOG.info("BGP HoldTimer new value: {}", this.holdTimerValue); this.keepAlive = this.holdTimerValue / 3; this.asNumber = AsNumberUtil.advertizedAsNumber(remoteOpen); + this.peerRegistry = peerRegitry; final Set tts = Sets.newHashSet(); final Set tats = Sets.newHashSet(); @@ -147,6 +151,7 @@ public class BGPSessionImpl extends AbstractProtocolSession implem LOG.info("Closing session: {}", this); if (this.state != State.Idle) { this.sendMessage(new NotifyBuilder().setErrorCode(BGPError.CEASE.getCode()).setErrorSubcode((short)0).build()); + removePeerSession(); this.channel.close(); this.state = State.Idle; } @@ -214,6 +219,7 @@ public class BGPSessionImpl extends AbstractProtocolSession implem private synchronized void closeWithoutMessage() { LOG.debug("Closing session: {}", this); + removePeerSession(); this.channel.close(); this.state = State.Idle; } @@ -231,6 +237,12 @@ public class BGPSessionImpl extends AbstractProtocolSession implem this.listener.onSessionTerminated(this, new BGPTerminationReason(error)); } + private void removePeerSession() { + if (this.peerRegistry != null) { + this.peerRegistry.removePeerSession(StrictBGPPeerRegistry.getIpAddress(this.channel.remoteAddress())); + } + } + /** * If HoldTimer expires, the session ends. If a message (whichever) was received during this period, the HoldTimer * will be rescheduled by HOLD_TIMER_VALUE + the time that has passed from the start of the HoldTimer to the time at diff --git a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistry.java b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistry.java index d33e8be8b3..6eb975b120 100644 --- a/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistry.java +++ b/bgp/rib-impl/src/main/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistry.java @@ -8,10 +8,11 @@ package org.opendaylight.protocol.bgp.rib.impl; -import com.google.common.base.CharMatcher; import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.collect.Maps; +import com.google.common.net.InetAddresses; +import com.google.common.primitives.UnsignedInts; import java.net.Inet4Address; import java.net.Inet6Address; import java.net.InetAddress; @@ -42,8 +43,6 @@ public final class StrictBGPPeerRegistry implements BGPPeerRegistry { private static final Logger LOG = LoggerFactory.getLogger(StrictBGPPeerRegistry.class); - private static final CharMatcher NONDIGIT = CharMatcher.inRange('0', '9').negate(); - // TODO remove backwards compatibility public static final StrictBGPPeerRegistry GLOBAL = new StrictBGPPeerRegistry(); @@ -68,6 +67,7 @@ public final class StrictBGPPeerRegistry implements BGPPeerRegistry { this.peers.remove(ip); } + @Override public synchronized void removePeerSession(final IpAddress ip) { Preconditions.checkNotNull(ip); this.sessionIds.remove(ip); @@ -96,45 +96,41 @@ public final class StrictBGPPeerRegistry implements BGPPeerRegistry { final BGPSessionId currentConnection = new BGPSessionId(sourceId, remoteId); final BGPSessionListener p = this.peers.get(ip); - if (this.sessionIds.containsKey(ip)) { - if (p.isSessionActive()) { - - LOG.warn("Duplicate BGP session established with {}", ip); - - final BGPSessionId previousConnection = this.sessionIds.get(ip); - - // Session reestablished with different ids - if (!previousConnection.equals(currentConnection)) { - LOG.warn("BGP session with {} {} has to be dropped. Same session already present {}", ip, currentConnection, previousConnection); - throw new BGPDocumentedException( - String.format("BGP session with %s %s has to be dropped. Same session already present %s", - ip, currentConnection, previousConnection), - BGPError.CEASE); - - // Session reestablished with lower source bgp id, dropping current - } else if (previousConnection.isHigherDirection(currentConnection)) { - LOG.warn("BGP session with {} {} has to be dropped. Opposite session already present", ip, currentConnection); - throw new BGPDocumentedException( - String.format("BGP session with %s initiated %s has to be dropped. Opposite session already present", - ip, currentConnection), - BGPError.CEASE); - - // Session reestablished with higher source bgp id, dropping previous - } else if (currentConnection.isHigherDirection(previousConnection)) { - LOG.warn("BGP session with {} {} released. Replaced by opposite session", ip, previousConnection); - this.peers.get(ip).releaseConnection(); - return this.peers.get(ip); - - // Session reestablished with same source bgp id, dropping current as duplicate - } else { - LOG.warn("BGP session with %s initiated from %s to %s has to be dropped. Same session already present", ip, sourceId, remoteId); - throw new BGPDocumentedException( - String.format("BGP session with %s initiated %s has to be dropped. Same session already present", - ip, currentConnection), - BGPError.CEASE); - } + final BGPSessionId previousConnection = this.sessionIds.get(ip); + + if (previousConnection != null) { + + LOG.warn("Duplicate BGP session established with {}", ip); + + // Session reestablished with different ids + if (!previousConnection.equals(currentConnection)) { + LOG.warn("BGP session with {} {} has to be dropped. Same session already present {}", ip, currentConnection, previousConnection); + throw new BGPDocumentedException( + String.format("BGP session with %s %s has to be dropped. Same session already present %s", + ip, currentConnection, previousConnection), + BGPError.CEASE); + + // Session reestablished with lower source bgp id, dropping current + } else if (previousConnection.isHigherDirection(currentConnection)) { + LOG.warn("BGP session with {} {} has to be dropped. Opposite session already present", ip, currentConnection); + throw new BGPDocumentedException( + String.format("BGP session with %s initiated %s has to be dropped. Opposite session already present", + ip, currentConnection), + BGPError.CEASE); + + // Session reestablished with higher source bgp id, dropping previous + } else if (currentConnection.isHigherDirection(previousConnection)) { + LOG.warn("BGP session with {} {} released. Replaced by opposite session", ip, previousConnection); + this.peers.get(ip).releaseConnection(); + return this.peers.get(ip); + + // Session reestablished with same source bgp id, dropping current as duplicate } else { - removePeerSession(ip); + LOG.warn("BGP session with %s initiated from %s to %s has to be dropped. Same session already present", ip, sourceId, remoteId); + throw new BGPDocumentedException( + String.format("BGP session with %s initiated %s has to be dropped. Same session already present", + ip, currentConnection), + BGPError.CEASE); } } @@ -187,6 +183,7 @@ public final class StrictBGPPeerRegistry implements BGPPeerRegistry { * Session identifier that contains (source Bgp Id) -> (destination Bgp Id) */ private static final class BGPSessionId { + private final Ipv4Address from, to; BGPSessionId(final Ipv4Address from, final Ipv4Address to) { @@ -230,20 +227,12 @@ public final class StrictBGPPeerRegistry implements BGPPeerRegistry { * Check if this connection is equal to other and if it contains higher source bgp id */ boolean isHigherDirection(final BGPSessionId other) { - Preconditions.checkState(!this.isSameDirection(other), "Equal sessions with same direction"); return toLong(this.from) > toLong(other.from); } private long toLong(final Ipv4Address from) { - return Long.parseLong(NONDIGIT.removeFrom(from.getValue())); - } - - /** - * Check if 2 connections are equal and face same direction - */ - boolean isSameDirection(final BGPSessionId other) { - Preconditions.checkState(this.equals(other), "Only equal sessions can be compared"); - return this.from.equals(other.from); + final int i = InetAddresses.coerceToInteger(InetAddresses.forString(from.getValue())); + return UnsignedInts.toLong(i); } @Override diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeerTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeerTest.java index 98636354de..c279ae40db 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeerTest.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/ApplicationPeerTest.java @@ -264,7 +264,7 @@ public class ApplicationPeerTest { Mockito.doReturn(null).when(this.channel).close(); final List params = Lists.newArrayList(new BgpParametersBuilder().setCParameters(new MultiprotocolCaseBuilder() .setMultiprotocolCapability(new MultiprotocolCapabilityBuilder().setAfi(Ipv4AddressFamily.class).setSafi(UnicastSubsequentAddressFamily.class).build()).build()).build()); - this.session = new BGPSessionImpl(this.classic, this.channel, new OpenBuilder().setBgpIdentifier(new Ipv4Address("1.1.1.1")).setHoldTimer(50).setMyAsNumber(72).setBgpParameters(params).build(), 30); + this.session = new BGPSessionImpl(this.classic, this.channel, new OpenBuilder().setBgpIdentifier(new Ipv4Address("1.1.1.1")).setHoldTimer(50).setMyAsNumber(72).setBgpParameters(params).build(), 30, null); assertEquals(this.r, this.classic.getRib()); assertEquals("testPeer", this.classic.getName()); Mockito.doAnswer(new Answer() { diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPDispatcherImplTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPDispatcherImplTest.java index 1ab2de76e2..07e005906d 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPDispatcherImplTest.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPDispatcherImplTest.java @@ -81,6 +81,7 @@ public class BGPDispatcherImplTest { final BGPSessionPreferences prefs = new BGPSessionPreferences(AS_NUMBER, (short) 90, new Ipv4Address(ADDRESS.getAddress().getHostAddress()), tlvs); Mockito.doReturn(true).when(this.registry).isPeerConfigured(Mockito.any(IpAddress.class)); Mockito.doReturn(prefs).when(this.registry).getPeerPreferences(Mockito.any(IpAddress.class)); + Mockito.doNothing().when(this.registry).removePeerSession(Mockito.any(IpAddress.class)); Mockito.doReturn(this.sessionListener).when(this.registry).getPeer(Mockito.any(IpAddress.class), Mockito.any(Ipv4Address.class), Mockito.any(Ipv4Address.class)); this.dispatcher = new BGPDispatcherImpl(ServiceLoaderBGPExtensionProviderContext.getSingletonInstance().getMessageRegistry(), group, group); diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImplTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImplTest.java index 367e0e5fa4..413dd4c1d2 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImplTest.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/BGPSessionImplTest.java @@ -124,7 +124,7 @@ public class BGPSessionImplTest { doReturn(this.pipeline).when(this.pipeline).replace(any(ChannelHandler.class), any(String.class), any(ChannelHandler.class)); doReturn(mock(ChannelFuture.class)).when(this.speakerListener).close(); this.listener = new SimpleSessionListener(); - this.bgpSession = new BGPSessionImpl(this.listener, this.speakerListener, this.classicOpen, this.classicOpen.getHoldTimer()); + this.bgpSession = new BGPSessionImpl(this.listener, this.speakerListener, this.classicOpen, this.classicOpen.getHoldTimer(), null); } @Test diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/SpeakerSessionMock.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/SpeakerSessionMock.java index 72bdf15dce..09ef537bf0 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/SpeakerSessionMock.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/SpeakerSessionMock.java @@ -22,7 +22,7 @@ public class SpeakerSessionMock extends BGPSessionImpl { private final BGPSessionListener client; SpeakerSessionMock(final BGPSessionListener listener, final BGPSessionListener client) { - super(listener, mock(Channel.class), new OpenBuilder().setHoldTimer(5).build(), 10); + super(listener, mock(Channel.class), new OpenBuilder().setHoldTimer(5).build(), 10, null); this.client = client; } diff --git a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistryTest.java b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistryTest.java index 004ab5dada..cee7899dd4 100644 --- a/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistryTest.java +++ b/bgp/rib-impl/src/test/java/org/opendaylight/protocol/bgp/rib/impl/StrictBGPPeerRegistryTest.java @@ -17,6 +17,7 @@ import org.junit.Before; import org.junit.Test; import org.mockito.Mockito; import org.opendaylight.protocol.bgp.parser.BGPDocumentedException; +import org.opendaylight.protocol.bgp.parser.BGPError; import org.opendaylight.protocol.bgp.rib.impl.spi.BGPSessionPreferences; import org.opendaylight.protocol.bgp.rib.impl.spi.ReusableBGPPeer; import org.opendaylight.protocol.bgp.rib.spi.BGPSessionListener; @@ -53,8 +54,8 @@ public class StrictBGPPeerRegistryTest { this.droppingBGPSessionRegistry.getPeer(remoteIp, from, to); try { this.droppingBGPSessionRegistry.getPeer(remoteIp, from, to); - } catch (final IllegalStateException e) { - Mockito.verify(session1).isSessionActive(); + } catch (final BGPDocumentedException e) { + assertEquals(BGPError.CEASE, e.getError()); return; } @@ -111,7 +112,7 @@ public class StrictBGPPeerRegistryTest { try { this.droppingBGPSessionRegistry.getPeer(remoteIp, lower, higher); } catch (final BGPDocumentedException e) { - Mockito.verify(session1).isSessionActive(); + assertEquals(BGPError.CEASE, e.getError()); return; } @@ -132,10 +133,29 @@ public class StrictBGPPeerRegistryTest { Mockito.verify(session1).releaseConnection(); } + @Test + public void testDuplicateDifferentIds() throws Exception { + final Ipv4Address from = new Ipv4Address("0.0.0.1"); + final IpAddress remoteIp = new IpAddress(from); + final Ipv4Address to = new Ipv4Address("255.255.255.255"); + + final ReusableBGPPeer session1 = getMockSession(); + this.droppingBGPSessionRegistry.addPeer(remoteIp, session1, this.mockPreferences); + + this.droppingBGPSessionRegistry.getPeer(remoteIp, from, to); + try { + this.droppingBGPSessionRegistry.getPeer(remoteIp, to, to); + } catch (final BGPDocumentedException e) { + assertEquals(BGPError.CEASE, e.getError()); + return; + } + + fail("Same peer cannot be connected twice"); + } + private ReusableBGPPeer getMockSession() { final ReusableBGPPeer mock = Mockito.mock(ReusableBGPPeer.class); Mockito.doNothing().when(mock).releaseConnection(); - Mockito.doReturn(Boolean.TRUE).when(mock).isSessionActive(); return mock; } -- 2.36.6