fixes according to feedback + test cases

more test cases needed.
need some cleanup in Firewall and FirewallRule

src/main/java/net/floodlightcontroller/firewall/Firewall.java

@@ -160,7 +160,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
                 // most error checking done with ClassCastException
                 try {
                     // first, snag the required entries, for debugging info
-                	r.ruleid = (String)row.get(COLUMN_RULEID);
+                	r.ruleid = Integer.parseInt((String)row.get(COLUMN_RULEID));
                     r.switchid = Long.parseLong((String)row.get(COLUMN_SWITCHID));
                     
                     for (String key : row.keySet()) {
@@ -209,13 +209,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
                         }
                     }
                 } catch (ClassCastException e) {
-                    if (!r.ruleid.equals(""))
-                        logger.error(
-                                "skipping rule {} with bad data : "
-                                        + e.getMessage(), r.ruleid);
-                    else
-                        logger.error("skipping rule with bad data: {} :: {} ",
-                                e.getMessage(), e.getStackTrace());
+                    logger.error("skipping rule {} with bad data : " + e.getMessage(), r.ruleid);
                 }
                 l.add(r);
             }
@@ -331,7 +325,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
 	
 	@Override
 	public void addRule(FirewallRule rule) {
-		rule.ruleid = UUID.randomUUID().toString();
+		rule.ruleid = rule.genID();
 		this.rules.add(rule);
 		// now re-sort the rules
 	    Collections.sort(this.rules);
@@ -358,16 +352,16 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
 	    entry.put(COLUMN_WILDCARD_DST_IP, Boolean.toString(rule.wildcard_dst_ip));
 	    entry.put(COLUMN_PRIORITY, Integer.toString(rule.priority));
 	    entry.put(COLUMN_IS_DENYRULE, Boolean.toString(rule.is_denyrule));
-	    storageSource.insertRowAsync(TABLE_NAME, entry);
+	    storageSource.insertRow(TABLE_NAME, entry);
 	}
 	
 	@Override
-	public void deleteRule(String ruleid) {
+	public void deleteRule(int ruleid) {
 		boolean found = false;
 		Iterator<FirewallRule> iter = this.rules.iterator();
 		while (iter.hasNext()) {
 			FirewallRule r = iter.next();
-			if (r.ruleid.equalsIgnoreCase(ruleid)) {
+			if (r.ruleid == ruleid) {
 				// found the rule, now remove it
 				iter.remove();
 				found = true;
@@ -379,7 +373,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
 			Collections.sort(this.rules);
 		}
 		// delete from database
-		storageSource.deleteRowAsync(TABLE_NAME, ruleid);
+		storageSource.deleteRow(TABLE_NAME, ruleid);
 	}
 	
 	protected List<Object> matchWithRule(IOFSwitch sw, OFPacketIn pi, FloodlightContext cntx) {
@@ -558,8 +552,12 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
 	public Command processPacketInMessage(IOFSwitch sw, OFPacketIn pi, IRoutingDecision decision, FloodlightContext cntx) {
 		Ethernet eth = IFloodlightProviderService.bcStore.get(cntx, IFloodlightProviderService.CONTEXT_PI_PAYLOAD);
 		
+		// TODO - add a check for a case where L2 is broadcast and L3 is unicast, i.e. malformed packet, deny it
+		
 		if (eth.getEtherType() == Ethernet.TYPE_ARP || eth.isBroadcast() == true) {
 			logger.info("allowing ARP and L2 broadcast traffic");
+			decision = new FirewallDecision(IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+			decision.addToContext(cntx);
 			return Command.CONTINUE;
 		}
 		

src/main/java/net/floodlightcontroller/firewall/FirewallRule.java

 package net.floodlightcontroller.firewall;
 
+import java.util.Arrays;
+
 public class FirewallRule implements Comparable {
-	public String ruleid;
+	public int ruleid;
 	public short src_inport;
 	public long src_mac;
 	public int src_ip_prefix;
@@ -44,6 +46,16 @@ public class FirewallRule implements Comparable {
 		this.wildcard_switchid = true;
 		this.priority = 32767;
 		this.is_denyrule = false;
+		this.ruleid = this.genID();
+	}
+	
+	public int genID() {
+		int uid = this.hashCode();
+		if (uid <= 0) {
+			uid = Math.abs(uid);
+			uid *= 15551;
+		}
+		return uid;
 	}
 	
 	public int compareTo(Object rule) {
@@ -72,4 +84,31 @@ public class FirewallRule implements Comparable {
 		}
 		return true;
 	}
+	
+	@Override
+    public int hashCode() {
+        final int prime = 2521;
+        int result = super.hashCode();
+        result = prime * result + src_inport;
+        result = prime * result + (int)src_mac;
+        result = prime * result + src_ip_prefix;
+        result = prime * result + src_ip_bits;
+        result = prime * result + proto_type;
+        result = prime * result + proto_srcport;
+        result = prime * result + proto_dstport;
+        result = prime * result + (int)dst_mac;
+        result = prime * result + dst_ip_prefix;
+        result = prime * result + dst_ip_bits;
+        result = prime * result + (int)switchid;
+        result = prime * result + priority;
+        result = prime * result + (new Boolean(is_denyrule)).hashCode();
+        result = prime * result + (new Boolean(wildcard_switchid)).hashCode();
+        result = prime * result + (new Boolean(wildcard_src_inport)).hashCode();
+        result = prime * result + (new Boolean(wildcard_src_ip)).hashCode();
+        result = prime * result + (new Boolean(wildcard_src_mac)).hashCode();
+        result = prime * result + (new Boolean(wildcard_proto_type)).hashCode();
+        result = prime * result + (new Boolean(wildcard_dst_ip)).hashCode();
+        result = prime * result + (new Boolean(wildcard_dst_mac)).hashCode();
+        return result;
+    }
 }

src/main/java/net/floodlightcontroller/firewall/FirewallRulesResource.java

@@ -90,7 +90,7 @@ public class FirewallRulesResource extends ServerResource {
     	Iterator<FirewallRule> iter = firewall.getRules().iterator();
     	while (iter.hasNext()) {
     		FirewallRule r = iter.next();
-    		if (r.ruleid.equalsIgnoreCase(rule.ruleid)) {
+    		if (r.ruleid == rule.ruleid) {
     			exists = true;
     			break;
     		}
@@ -140,7 +140,7 @@ public class FirewallRulesResource extends ServerResource {
             
             String tmp;
             if (n == "ruleid") {
-                rule.ruleid = jp.getText();
+                rule.ruleid = Integer.parseInt((String)jp.getText());
             } else if (n == "switchid") {
             		tmp = jp.getText();
             		if (tmp.equalsIgnoreCase("-1") == false) {

src/main/java/net/floodlightcontroller/firewall/IFirewallService.java

@@ -39,5 +39,5 @@ public interface IFirewallService extends IFloodlightService {
 	/**
      * Deletes a Firewall rule
      */
-	public void deleteRule(String ruleid);
+	public void deleteRule(int ruleid);
 }

src/test/java/net/floodlightcontroller/firewall/FirewallTest.java

+package net.floodlightcontroller.firewall;
+
+import static org.easymock.EasyMock.createMock;
+import static org.easymock.EasyMock.expect;
+import static org.easymock.EasyMock.replay;
+import static org.easymock.EasyMock.verify;
+
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import net.floodlightcontroller.core.FloodlightContext;
+import net.floodlightcontroller.core.IFloodlightProviderService;
+import net.floodlightcontroller.core.IOFSwitch;
+import net.floodlightcontroller.core.module.FloodlightModuleContext;
+import net.floodlightcontroller.core.module.FloodlightModuleException;
+import net.floodlightcontroller.core.test.MockFloodlightProvider;
+import net.floodlightcontroller.packet.Data;
+import net.floodlightcontroller.packet.Ethernet;
+import net.floodlightcontroller.packet.IPacket;
+import net.floodlightcontroller.packet.IPv4;
+import net.floodlightcontroller.packet.TCP;
+import net.floodlightcontroller.packet.UDP;
+import net.floodlightcontroller.restserver.IRestApiService;
+import net.floodlightcontroller.restserver.RestApiServer;
+import net.floodlightcontroller.routing.IRoutingDecision;
+import net.floodlightcontroller.storage.IStorageSourceService;
+import net.floodlightcontroller.storage.memory.MemoryStorageSource;
+import net.floodlightcontroller.test.FloodlightTestCase;
+import net.floodlightcontroller.util.MACAddress;
+
+import org.easymock.EasyMock;
+import org.junit.Before;
+import org.junit.Test;
+import org.openflow.protocol.OFPacketIn;
+import org.openflow.protocol.OFPacketIn.OFPacketInReason;
+import org.openflow.protocol.OFType;
+import org.openflow.util.HexString;
+
+public class FirewallTest extends FloodlightTestCase {
+	protected MockFloodlightProvider mockFloodlightProvider;
+    protected FloodlightContext cntx;
+	protected OFPacketIn packetIn;
+	protected IOFSwitch sw;
+    protected IPacket tcpPacket;
+    protected byte[] tcpPacketSerialized;
+    protected IPacket broadcastPacket;
+    protected byte[] broadcastPacketSerialized;
+    protected IPacket tcpPacketReply;
+    protected byte[] tcpPacketReplySerialized;
+    private Firewall firewall;
+    public static String TestSwitch1DPID = "00:00:00:00:00:00:00:01";
+    
+    @Before
+    public void setUp() throws Exception {
+        super.setUp();
+        cntx = new FloodlightContext();
+        mockFloodlightProvider = getMockFloodlightProvider();
+        firewall = new Firewall();
+        IStorageSourceService storageService = new MemoryStorageSource();
+        RestApiServer restApi = new RestApiServer();
+        
+        // Mock switches
+        long dpid = HexString.toLong(TestSwitch1DPID);
+        sw = EasyMock.createNiceMock(IOFSwitch.class);
+        expect(sw.getId()).andReturn(dpid).anyTimes();
+        expect(sw.getStringId()).andReturn(TestSwitch1DPID).anyTimes();
+        replay(sw);
+        // Load the switch map
+        Map<Long, IOFSwitch> switches = new HashMap<Long, IOFSwitch>();
+        switches.put(dpid, sw);
+        mockFloodlightProvider.setSwitches(switches);
+        
+        FloodlightModuleContext fmc = new FloodlightModuleContext();
+        fmc.addService(IFloodlightProviderService.class, 
+                       mockFloodlightProvider);
+        fmc.addService(IFirewallService.class, firewall);
+        fmc.addService(IStorageSourceService.class, storageService);
+        fmc.addService(IRestApiService.class, restApi);
+        
+        try {
+            restApi.init(fmc);
+        } catch (FloodlightModuleException e) {
+            e.printStackTrace();
+        }
+
+        firewall.init(fmc);
+        firewall.startUp(fmc);
+       
+        // Build our test packet
+        this.tcpPacket = new Ethernet()
+            .setDestinationMACAddress("00:11:22:33:44:55")
+            .setSourceMACAddress("00:44:33:22:11:00")
+            .setVlanID((short) 42)
+            .setEtherType(Ethernet.TYPE_IPv4)
+            .setPayload(
+                new IPv4()
+                .setTtl((byte) 128)
+                .setSourceAddress("192.168.1.1")
+                .setDestinationAddress("192.168.1.2")
+                .setPayload(new TCP()
+                            .setSourcePort((short) 81)
+                            .setDestinationPort((short) 80)
+                            .setPayload(new Data(new byte[] {0x01}))));
+        this.tcpPacketSerialized = tcpPacket.serialize();
+        // Build a broadcast packet
+        this.broadcastPacket = new Ethernet()
+            .setDestinationMACAddress("FF:FF:FF:FF:FF:FF")
+            .setSourceMACAddress("00:44:33:22:11:00")
+            .setVlanID((short) 42)
+            .setEtherType(Ethernet.TYPE_IPv4)
+            .setPayload(
+                new IPv4()
+                .setTtl((byte) 128)
+                .setSourceAddress("192.168.1.1")
+                .setDestinationAddress("192.168.255.255")
+                .setPayload(new UDP()
+                        .setSourcePort((short) 5000)
+                        .setDestinationPort((short) 5001)
+                        .setPayload(new Data(new byte[] {0x01}))));
+
+        this.broadcastPacketSerialized = broadcastPacket.serialize();
+        this.tcpPacketReply = new Ethernet()
+            .setDestinationMACAddress("00:44:33:22:11:00")
+            .setSourceMACAddress("00:11:22:33:44:55")
+            .setVlanID((short) 42)
+            .setEtherType(Ethernet.TYPE_IPv4)
+            .setPayload(
+                    new IPv4()
+                    .setTtl((byte) 128)
+                    .setSourceAddress("192.168.1.2")
+                    .setDestinationAddress("192.168.1.1")
+                    .setPayload(new TCP()
+                    .setSourcePort((short) 80)
+                    .setDestinationPort((short) 81)
+                    .setPayload(new Data(new byte[] {0x02}))));
+        this.tcpPacketReplySerialized = tcpPacketReply.serialize();
+    }
+    
+    protected void setPacketIn(IPacket packet) {
+    	byte[] serializedPacket = packet.serialize();
+    	// Build the PacketIn
+        this.packetIn = ((OFPacketIn) mockFloodlightProvider.getOFMessageFactory().getMessage(OFType.PACKET_IN))
+            .setBufferId(-1)
+            .setInPort((short) 1)
+            .setPacketData(serializedPacket)
+            .setReason(OFPacketInReason.NO_MATCH)
+            .setTotalLength((short) serializedPacket.length);
+        
+        // Add the packet to the context store
+        IFloodlightProviderService.bcStore.
+            put(cntx, 
+                IFloodlightProviderService.CONTEXT_PI_PAYLOAD, 
+                (Ethernet)packet);
+    }
+    
+    @Test
+    public void testNoRules() throws Exception {
+    	// enable firewall first
+    	firewall.enableFirewall();
+    	// simulate a packet-in event
+    	this.setPacketIn(tcpPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	assertEquals(0, firewall.countRules());
+    	
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	// no rules to match, so firewall should deny
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.DROP);
+    }
+    
+    @Test
+    public void testRuleInsertionIntoStorage() throws Exception {
+    	// add TCP rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	
+    	List<Map<String, Object>> rulesFromStorage = firewall.getStorageRules();
+    	assertEquals(1, rulesFromStorage.size());
+    	assertEquals(rulesFromStorage.get(0).get("ruleid"), rule.ruleid);
+    }
+    
+    @Test
+    public void testRuleDeletion() throws Exception {
+    	// add TCP rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	int rid = rule.ruleid;
+    	
+    	List<Map<String, Object>> rulesFromStorage = firewall.getStorageRules();
+    	assertEquals(1, rulesFromStorage.size());
+    	assertEquals(rulesFromStorage.get(0).get("ruleid"), rid);
+    	
+    	// delete rule
+    	firewall.deleteRule(rid);
+    	rulesFromStorage = firewall.getStorageRules();
+    	assertEquals(0, rulesFromStorage.size());
+    }
+    
+    @Test
+    public void testFirewallDisabled() throws Exception {
+    	// firewall isn't enabled by default
+    	// so, it shouldn't make any decision
+    	
+    	// add TCP rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	
+    	this.setPacketIn(tcpPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	assertEquals(1, firewall.countRules());
+    	
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertNull(decision);
+    }
+    
+    @Test
+    public void testSimpleAllowRule() throws Exception {
+    	// enable firewall first
+    	firewall.enableFirewall();
+    	
+    	// add TCP rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	// source is IP 192.168.1.2
+    	rule.src_ip_prefix = IPv4.toIPv4Address("192.168.1.2");
+    	rule.wildcard_src_ip = false;
+    	// dest is network 192.168.1.0/24
+    	rule.dst_ip_prefix = IPv4.toIPv4Address("192.168.1.0");
+    	rule.dst_ip_bits = 24;
+    	rule.wildcard_dst_ip = false;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	
+    	// simulate a packet-in events
+    	
+    	this.setPacketIn(tcpPacketReply);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+    	
+    	// clear decision
+    	IRoutingDecision.rtStore.remove(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	
+    	this.setPacketIn(tcpPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.DROP);
+    }
+    
+    @Test
+    public void testOverlappingRules() throws Exception {
+    	firewall.enableFirewall();
+    	
+    	// add TCP port 80 (destination only) allow rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	rule.proto_dstport = 80;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	
+    	// add block all rule
+    	rule = new FirewallRule();
+    	rule.is_denyrule = true;
+    	rule.priority = 2;
+    	firewall.addRule(rule);
+    	
+    	assertEquals(2, firewall.countRules());
+    	
+    	// packet destined to TCP port 80 - should be allowed
+    	
+    	this.setPacketIn(tcpPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+    	
+    	// clear decision
+    	IRoutingDecision.rtStore.remove(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	
+    	// packet destined for port 81 - should be denied
+    	
+    	this.setPacketIn(tcpPacketReply);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.DROP);
+    }
+    
+    @Test
+    public void testBroadcast() throws Exception {
+    	// enable firewall first
+    	firewall.enableFirewall();
+    	
+    	// no rules inserted so all traffic other than broadcast and ARP should be blocked
+    	
+    	// simulate a packet-in event
+    	
+    	this.setPacketIn(broadcastPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	// broadcast traffic should be allowed
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+    }
+    
+    @Test
+    public void testLayer2Rule() throws Exception {
+    	// enable firewall first
+    	firewall.enableFirewall();
+    	
+    	// add L2 rule
+    	FirewallRule rule = new FirewallRule();
+    	rule.src_mac = MACAddress.valueOf("00:44:33:22:11:00").toLong();
+    	rule.wildcard_src_mac = false;
+    	rule.dst_mac = MACAddress.valueOf("00:11:22:33:44:55").toLong();
+    	rule.wildcard_dst_mac = false;
+    	rule.priority = 1;
+    	firewall.addRule(rule);
+    	
+    	// add TCP deny all rule
+    	rule = new FirewallRule();
+    	rule.proto_type = IPv4.PROTOCOL_TCP;
+    	rule.wildcard_proto_type = false;
+    	rule.priority = 2;
+    	rule.is_denyrule = true;
+    	firewall.addRule(rule);
+    	
+    	// simulate a packet-in event
+    	
+    	this.setPacketIn(tcpPacket);
+    	firewall.receive(sw, this.packetIn, cntx);
+    	verify(sw);
+    	
+    	IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
+    	assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+    }
+    
+    // TODO - add cases for stress testing and malformed packets (e.g. L2 broadcast with L3 unicast)
+}