Changed ProxyArpManager to make use of EdgeManager.
Change-Id: I05193146490aba6736c1815bf0d9022df8628973
diff --git a/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java b/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
index 23da89d..45e467d 100644
--- a/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
+++ b/core/net/src/main/java/org/onosproject/net/edgeservice/impl/EdgeManager.java
@@ -174,6 +174,8 @@
}
});
} else {
+ //FIXME special case of preexisting edgeport & no triggerless events could cause this to never hit and
+ //never discover an edgeport that should have been discovered.
loadAllEdgePorts();
}
}
@@ -198,6 +200,7 @@
// Processes a device event by adding or removing its end-points in our cache.
private void processDeviceEvent(DeviceEvent event) {
+ //FIXME handle the case where a device is suspended, this may or may not come up
DeviceEvent.Type type = event.type();
DeviceId id = event.subject().id();
diff --git a/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java b/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
index 6411a4d..ecfb71c 100644
--- a/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
+++ b/core/net/src/main/java/org/onosproject/net/proxyarp/impl/ProxyArpManager.java
@@ -15,9 +15,6 @@
*/
package org.onosproject.net.proxyarp.impl;
-import com.google.common.collect.HashMultimap;
-import com.google.common.collect.Lists;
-import com.google.common.collect.Multimap;
import org.apache.felix.scr.annotations.Activate;
import org.apache.felix.scr.annotations.Component;
import org.apache.felix.scr.annotations.Deactivate;
@@ -38,22 +35,15 @@
import org.onlab.packet.ndp.NeighborSolicitation;
import org.onosproject.core.Permission;
import org.onosproject.net.ConnectPoint;
-import org.onosproject.net.Device;
import org.onosproject.net.Host;
import org.onosproject.net.HostId;
-import org.onosproject.net.Link;
-import org.onosproject.net.Port;
-import org.onosproject.net.PortNumber;
-import org.onosproject.net.device.DeviceEvent;
-import org.onosproject.net.device.DeviceListener;
import org.onosproject.net.device.DeviceService;
+import org.onosproject.net.edge.EdgePortService;
import org.onosproject.net.flow.DefaultTrafficTreatment;
import org.onosproject.net.flow.TrafficTreatment;
import org.onosproject.net.host.HostService;
import org.onosproject.net.host.InterfaceIpAddress;
import org.onosproject.net.host.PortAddresses;
-import org.onosproject.net.link.LinkEvent;
-import org.onosproject.net.link.LinkListener;
import org.onosproject.net.link.LinkService;
import org.onosproject.net.packet.DefaultOutboundPacket;
import org.onosproject.net.packet.InboundPacket;
@@ -64,14 +54,13 @@
import java.nio.ByteBuffer;
import java.util.HashSet;
-import java.util.List;
-import java.util.Map.Entry;
import java.util.Set;
+import java.util.stream.Collectors;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
-import static org.slf4j.LoggerFactory.getLogger;
import static org.onosproject.security.AppGuard.checkPermission;
+import static org.slf4j.LoggerFactory.getLogger;
@Component(immediate = true)
@@ -87,6 +76,9 @@
private static final String NOT_ARP_REPLY = "ARP is not a reply.";
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
+ protected EdgePortService edgeService;
+
+ @Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected HostService hostService;
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
@@ -98,29 +90,18 @@
@Reference(cardinality = ReferenceCardinality.MANDATORY_UNARY)
protected DeviceService deviceService;
- private final Multimap<Device, PortNumber> internalPorts = HashMultimap.create();
- private final Multimap<Device, PortNumber> externalPorts = HashMultimap.create();
-
- private final DeviceListener deviceListener = new InternalDeviceListener();
- private final InternalLinkListener linkListener = new InternalLinkListener();
-
/**
* Listens to both device service and link service to determine
* whether a port is internal or external.
*/
@Activate
public void activate() {
- deviceService.addListener(deviceListener);
- linkService.addListener(linkListener);
- determinePortLocations();
log.info("Started");
}
@Deactivate
public void deactivate() {
- deviceService.removeListener(deviceListener);
- linkService.removeListener(linkListener);
log.info("Stopped");
}
@@ -243,7 +224,7 @@
for (InterfaceIpAddress ia : addresses.ipAddresses()) {
if (ia.ipAddress().equals(targetAddress)) {
Ethernet ndpReply =
- buildNdpReply(targetAddress, addresses.mac(), eth);
+ buildNdpReply(targetAddress, addresses.mac(), eth);
sendTo(ndpReply, inPort);
}
}
@@ -303,16 +284,16 @@
Ethernet ndpReply = buildNdpReply(targetAddress, dst.mac(), eth);
sendTo(ndpReply, inPort);
}
+ //TODO checkpoint
/**
* Outputs the given packet out the given port.
*
- * @param packet the packet to send
+ * @param packet the packet to send
* @param outPort the port to send it out
*/
private void sendTo(Ethernet packet, ConnectPoint outPort) {
- if (internalPorts.containsEntry(
- deviceService.getDevice(outPort.deviceId()), outPort.port())) {
+ if (!edgeService.isEdgePoint(outPort)) {
// Sanity check to make sure we don't send the packet out an
// internal port and create a loop (could happen due to
// misconfiguration).
@@ -332,13 +313,10 @@
* @return a set of PortAddresses describing ports in the subnet
*/
private Set<PortAddresses> findPortsInSubnet(IpAddress target) {
- Set<PortAddresses> result = new HashSet<PortAddresses>();
+ Set<PortAddresses> result = new HashSet<>();
for (PortAddresses addresses : hostService.getAddressBindings()) {
- for (InterfaceIpAddress ia : addresses.ipAddresses()) {
- if (ia.subnetAddress().contains(target)) {
- result.add(addresses);
- }
- }
+ result.addAll(addresses.ipAddresses().stream().filter(ia -> ia.subnetAddress().contains(target)).
+ map(ia -> addresses).collect(Collectors.toList()));
}
return result;
}
@@ -432,71 +410,30 @@
* Flood the arp request at all edges in the network.
*
* @param request the arp request
- * @param inPort the connect point the arp request was received on
+ * @param inPort the connect point the arp request was received on
*/
private void flood(Ethernet request, ConnectPoint inPort) {
TrafficTreatment.Builder builder = null;
ByteBuffer buf = ByteBuffer.wrap(request.serialize());
- synchronized (externalPorts) {
- for (Entry<Device, PortNumber> entry : externalPorts.entries()) {
- ConnectPoint cp = new ConnectPoint(entry.getKey().id(), entry.getValue());
- if (isOutsidePort(cp) || cp.equals(inPort)) {
- continue;
- }
+ for (ConnectPoint connectPoint : edgeService.getEdgePoints()) {
+ if (isOutsidePort(connectPoint) || connectPoint.equals(inPort)) {
+ continue;
+ }
- builder = DefaultTrafficTreatment.builder();
- builder.setOutput(entry.getValue());
- packetService.emit(new DefaultOutboundPacket(entry.getKey().id(),
- builder.build(), buf));
- }
- }
- }
-
- /**
- * Determines the location of all known ports in the system.
- */
- private void determinePortLocations() {
- Iterable<Device> devices = deviceService.getDevices();
- Iterable<Link> links = null;
- List<PortNumber> ports = null;
- for (Device d : devices) {
- ports = buildPortNumberList(deviceService.getPorts(d.id()));
- links = linkService.getLinks();
- for (Link l : links) {
- // for each link, mark the concerned ports as internal
- // and the remaining ports are therefore external.
- if (l.src().deviceId().equals(d.id())
- && ports.contains(l.src().port())) {
- ports.remove(l.src().port());
- internalPorts.put(d, l.src().port());
- }
- if (l.dst().deviceId().equals(d.id())
- && ports.contains(l.dst().port())) {
- ports.remove(l.dst().port());
- internalPorts.put(d, l.dst().port());
- }
- }
- synchronized (externalPorts) {
- externalPorts.putAll(d, ports);
- }
+ builder = DefaultTrafficTreatment.builder();
+ builder.setOutput(connectPoint.port());
+ packetService.emit(new DefaultOutboundPacket(connectPoint.deviceId(),
+ builder.build(), buf));
}
}
- private List<PortNumber> buildPortNumberList(List<Port> ports) {
- List<PortNumber> portNumbers = Lists.newLinkedList();
- for (Port p : ports) {
- portNumbers.add(p.number());
- }
- return portNumbers;
- }
-
/**
* Builds an Neighbor Discovery reply based on a request.
*
- * @param srcIp the IP address to use as the reply source
- * @param srcMac the MAC address to use as the reply source
+ * @param srcIp the IP address to use as the reply source
+ * @param srcMac the MAC address to use as the reply source
* @param request the Neighbor Solicitation request we got
* @return an Ethernet frame containing the Neighbor Advertisement reply
*/
@@ -524,91 +461,11 @@
nadv.setSolicitedFlag((byte) 1);
nadv.setOverrideFlag((byte) 1);
nadv.addOption(NeighborDiscoveryOptions.TYPE_TARGET_LL_ADDRESS,
- srcMac.toBytes());
+ srcMac.toBytes());
icmp6.setPayload(nadv);
ipv6.setPayload(icmp6);
eth.setPayload(ipv6);
return eth;
}
-
- public class InternalLinkListener implements LinkListener {
-
- @Override
- public void event(LinkEvent event) {
- Link link = event.subject();
- Device src = deviceService.getDevice(link.src().deviceId());
- Device dst = deviceService.getDevice(link.dst().deviceId());
- switch (event.type()) {
- case LINK_ADDED:
- synchronized (externalPorts) {
- externalPorts.remove(src, link.src().port());
- externalPorts.remove(dst, link.dst().port());
- internalPorts.put(src, link.src().port());
- internalPorts.put(dst, link.dst().port());
- }
-
- break;
- case LINK_REMOVED:
- synchronized (externalPorts) {
- externalPorts.put(src, link.src().port());
- externalPorts.put(dst, link.dst().port());
- internalPorts.remove(src, link.src().port());
- internalPorts.remove(dst, link.dst().port());
- }
-
- break;
- case LINK_UPDATED:
- // don't care about links being updated.
- break;
- default:
- break;
- }
-
- }
-
- }
-
- public class InternalDeviceListener implements DeviceListener {
-
- @Override
- public void event(DeviceEvent event) {
- Device device = event.subject();
- switch (event.type()) {
- case DEVICE_ADDED:
- case DEVICE_AVAILABILITY_CHANGED:
- case DEVICE_SUSPENDED:
- case DEVICE_UPDATED:
- // nothing to do in these cases; handled when links get reported
- break;
- case DEVICE_REMOVED:
- synchronized (externalPorts) {
- externalPorts.removeAll(device);
- internalPorts.removeAll(device);
- }
- break;
- case PORT_ADDED:
- case PORT_UPDATED:
- synchronized (externalPorts) {
- if (event.port().isEnabled()) {
- externalPorts.put(device, event.port().number());
- internalPorts.remove(device, event.port().number());
- }
- }
- break;
- case PORT_REMOVED:
- synchronized (externalPorts) {
- externalPorts.remove(device, event.port().number());
- internalPorts.remove(device, event.port().number());
- }
- break;
- default:
- break;
-
- }
-
- }
-
- }
-
}
diff --git a/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java b/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
index c79d44c..4a1c446 100644
--- a/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
+++ b/core/net/src/test/java/org/onosproject/net/proxyarp/impl/ProxyArpManagerTest.java
@@ -15,6 +15,7 @@
*/
package org.onosproject.net.proxyarp.impl;
+import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import org.junit.Before;
import org.junit.Test;
@@ -36,6 +37,7 @@
import org.onosproject.net.PortNumber;
import org.onosproject.net.device.DeviceListener;
import org.onosproject.net.device.DeviceService;
+import org.onosproject.net.edgeservice.impl.EdgeManager;
import org.onosproject.net.flow.instructions.Instruction;
import org.onosproject.net.flow.instructions.Instructions.OutputInstruction;
import org.onosproject.net.host.HostService;
@@ -53,8 +55,14 @@
import java.util.List;
import java.util.Set;
-import static org.easymock.EasyMock.*;
-import static org.junit.Assert.*;
+import static org.easymock.EasyMock.anyObject;
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
/**
* Tests for the {@link ProxyArpManager} class.
@@ -85,6 +93,11 @@
private static final HostLocation LOC2 = new HostLocation(DID2, P1, 123L);
private static final byte[] ZERO_MAC_ADDRESS = MacAddress.ZERO.toBytes();
+ //Return values used for various functions of the TestPacketService inner class.
+ private boolean isEdgePointReturn;
+ private List<ConnectPoint> getEdgePointsNoArg;
+
+
private ProxyArpManager proxyArp;
private TestPacketService packetService;
@@ -98,6 +111,8 @@
packetService = new TestPacketService();
proxyArp.packetService = packetService;
+ proxyArp.edgeService = new TestEdgePortService();
+
// Create a host service mock here. Must be replayed by tests once the
// expectations have been set up
hostService = createMock(HostService.class);
@@ -112,7 +127,7 @@
/**
* Creates a fake topology to feed into the ARP module.
- * <p/>
+ * <p>
* The default topology is a unidirectional ring topology. Each switch has
* 3 ports. Ports 2 and 3 have the links to neighbor switches, and port 1
* is free (edge port).
@@ -205,12 +220,12 @@
InterfaceIpAddress ia2 = new InterfaceIpAddress(addr2, prefix2);
PortAddresses pa1 =
new PortAddresses(cp, Sets.newHashSet(ia1),
- MacAddress.valueOf(2 * i - 1),
- VlanId.vlanId((short) 1));
+ MacAddress.valueOf(2 * i - 1),
+ VlanId.vlanId((short) 1));
PortAddresses pa2 =
new PortAddresses(cp, Sets.newHashSet(ia2),
- MacAddress.valueOf(2 * i),
- VlanId.NONE);
+ MacAddress.valueOf(2 * i),
+ VlanId.NONE);
addresses.add(pa1);
addresses.add(pa2);
@@ -223,7 +238,7 @@
for (int i = 1; i <= NUM_FLOOD_PORTS; i++) {
ConnectPoint cp = new ConnectPoint(getDeviceId(i + NUM_ADDRESS_PORTS),
- P1);
+ P1);
expect(hostService.getAddressBindingsForPort(cp))
.andReturn(Collections.<PortAddresses>emptySet()).anyTimes();
}
@@ -266,11 +281,14 @@
*/
@Test
public void testReplyKnown() {
+ //Set the return value of isEdgePoint from the edgemanager.
+ isEdgePointReturn = true;
+
Host replyer = new DefaultHost(PID, HID1, MAC1, VLAN1, getLocation(4),
- Collections.singleton(IP1));
+ Collections.singleton(IP1));
Host requestor = new DefaultHost(PID, HID2, MAC2, VLAN1, getLocation(5),
- Collections.singleton(IP2));
+ Collections.singleton(IP2));
expect(hostService.getHostsByIp(IP1))
.andReturn(Collections.singleton(replyer));
@@ -294,17 +312,25 @@
*/
@Test
public void testReplyUnknown() {
+ isEdgePointReturn = true;
+
Host requestor = new DefaultHost(PID, HID2, MAC2, VLAN1, getLocation(5),
- Collections.singleton(IP2));
+ Collections.singleton(IP2));
expect(hostService.getHostsByIp(IP1))
.andReturn(Collections.<Host>emptySet());
expect(hostService.getHost(HID2)).andReturn(requestor);
+
replay(hostService);
Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, IP2, IP1);
+ //Setup the set of edge ports to be used in the reply method
+ getEdgePointsNoArg = Lists.newLinkedList();
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
+
proxyArp.reply(arpRequest, getLocation(6));
verifyFlood(arpRequest);
@@ -318,11 +344,12 @@
*/
@Test
public void testReplyDifferentVlan() {
+
Host replyer = new DefaultHost(PID, HID1, MAC1, VLAN2, getLocation(4),
- Collections.singleton(IP1));
+ Collections.singleton(IP1));
Host requestor = new DefaultHost(PID, HID2, MAC2, VLAN1, getLocation(5),
- Collections.singleton(IP2));
+ Collections.singleton(IP2));
expect(hostService.getHostsByIp(IP1))
.andReturn(Collections.singleton(replyer));
@@ -332,6 +359,10 @@
Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, IP2, IP1);
+ //Setup for flood test
+ getEdgePointsNoArg = Lists.newLinkedList();
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
proxyArp.reply(arpRequest, getLocation(6));
verifyFlood(arpRequest);
@@ -346,13 +377,13 @@
MacAddress secondMac = MacAddress.valueOf(2L);
Host requestor = new DefaultHost(PID, HID2, MAC2, VLAN1, LOC1,
- Collections.singleton(theirIp));
+ Collections.singleton(theirIp));
expect(hostService.getHost(HID2)).andReturn(requestor);
replay(hostService);
Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC2, null, theirIp, ourFirstIp);
-
+ isEdgePointReturn = true;
proxyArp.reply(arpRequest, LOC1);
assertEquals(1, packetService.packets.size());
@@ -378,7 +409,7 @@
// Request for a valid external IP address but coming in the wrong port
Ethernet arpRequest = buildArp(ARP.OP_REQUEST, MAC1, null, theirIp,
- Ip4Address.valueOf("10.0.3.1"));
+ Ip4Address.valueOf("10.0.3.1"));
proxyArp.reply(arpRequest, LOC1);
assertEquals(0, packetService.packets.size());
@@ -402,8 +433,10 @@
// This is a request from something inside our network (like a BGP
// daemon) to an external host.
Ethernet arpRequest = buildArp(ARP.OP_REQUEST, ourMac, null, ourIp, theirIp);
- proxyArp.reply(arpRequest, getLocation(5));
+ //Ensure the packet is allowed through (it is not to an internal port)
+ isEdgePointReturn = true;
+ proxyArp.reply(arpRequest, getLocation(5));
assertEquals(1, packetService.packets.size());
verifyPacketOut(arpRequest, getLocation(1), packetService.packets.get(0));
@@ -421,7 +454,7 @@
@Test
public void testForwardToHost() {
Host host1 = new DefaultHost(PID, HID1, MAC1, VLAN1, LOC1,
- Collections.singleton(IP1));
+ Collections.singleton(IP1));
expect(hostService.getHost(HID1)).andReturn(host1);
replay(hostService);
@@ -448,6 +481,13 @@
Ethernet arpRequest = buildArp(ARP.OP_REPLY, MAC2, MAC1, IP2, IP1);
+ //populate the list of edges when so that when forward hits flood in the manager it contains the values
+ //that should continue on
+ getEdgePointsNoArg = Lists.newLinkedList();
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("3"), PortNumber.portNumber(1)));
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("5"), PortNumber.portNumber(1)));
+ getEdgePointsNoArg.add(new ConnectPoint(DeviceId.deviceId("4"), PortNumber.portNumber(1)));
+
proxyArp.forward(arpRequest, getLocation(6));
verifyFlood(arpRequest);
@@ -464,17 +504,17 @@
assertEquals(NUM_FLOOD_PORTS - 1, packetService.packets.size());
Collections.sort(packetService.packets,
- new Comparator<OutboundPacket>() {
- @Override
- public int compare(OutboundPacket o1, OutboundPacket o2) {
- return o1.sendThrough().uri().compareTo(o2.sendThrough().uri());
- }
- });
+ new Comparator<OutboundPacket>() {
+ @Override
+ public int compare(OutboundPacket o1, OutboundPacket o2) {
+ return o1.sendThrough().uri().compareTo(o2.sendThrough().uri());
+ }
+ });
for (int i = 0; i < NUM_FLOOD_PORTS - 1; i++) {
ConnectPoint cp = new ConnectPoint(getDeviceId(NUM_ADDRESS_PORTS + i + 1),
- PortNumber.portNumber(1));
+ PortNumber.portNumber(1));
OutboundPacket outboundPacket = packetService.packets.get(i);
verifyPacketOut(packet, cp, outboundPacket);
@@ -572,4 +612,17 @@
}
}
+
+ class TestEdgePortService extends EdgeManager {
+
+ @Override
+ public boolean isEdgePoint(ConnectPoint connectPoint) {
+ return isEdgePointReturn;
+ }
+
+ @Override
+ public Iterable<ConnectPoint> getEdgePoints() {
+ return getEdgePointsNoArg;
+ }
+ }
}