From a2bffd829b472164288b07785244c8ffd74872ae Mon Sep 17 00:00:00 2001
From: Alex Reimers <alex@bigswitch.com>
Date: Wed, 29 Aug 2012 17:13:04 -0700
Subject: [PATCH] FL-82 - Firewall.java - Protect the firewall rules with
synchronization.
---
.../firewall/Firewall.java | 63 +++++++------------
.../firewall/FirewallTest.java | 10 +--
2 files changed, 28 insertions(+), 45 deletions(-)
diff --git a/src/main/java/net/floodlightcontroller/firewall/Firewall.java b/src/main/java/net/floodlightcontroller/firewall/Firewall.java
index a1335a51b..5a270430a 100644
--- a/src/main/java/net/floodlightcontroller/firewall/Firewall.java
+++ b/src/main/java/net/floodlightcontroller/firewall/Firewall.java
@@ -40,7 +40,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
protected IStorageSourceService storageSource;
protected IRestApiService restApi;
protected static Logger logger;
- protected ArrayList<FirewallRule> rules;
+ protected List<FirewallRule> rules; // protected by synchornized
protected boolean enabled;
protected int subnet_mask = IPv4.toIPv4Address("255.255.255.0");
@@ -119,19 +119,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
l.add(IRestApiService.class);
return l;
}
-
- public IFloodlightProviderService getFloodlightProvider() {
- return floodlightProvider;
- }
-
- public void setFloodlightProvider(IFloodlightProviderService floodlightProvider) {
- this.floodlightProvider = floodlightProvider;
- }
-
- public void setStorageSource(IStorageSourceService storageSource) {
- this.storageSource = storageSource;
- }
-
+
/**
* Reads the rules from the storage and creates a sorted arraylist of FirewallRule
* from them.
@@ -223,14 +211,6 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
return l;
}
- /**
- * used for debugging and unittests
- * @return the number of rules
- */
- public int countRules() {
- return this.rules.size();
- }
-
@Override
public void init(FloodlightModuleContext context)
throws FloodlightModuleException {
@@ -254,12 +234,14 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
// storage, create table and read rules
storageSource.createTable(TABLE_NAME, null);
storageSource.setTablePrimaryKeyName(TABLE_NAME, COLUMN_RULEID);
- this.rules = readRulesFromStorage();
+ synchronized (rules) {
+ this.rules = readRulesFromStorage();
+ }
}
@Override
public Command receive(IOFSwitch sw, OFMessage msg, FloodlightContext cntx) {
- if (this.enabled == false) return Command.CONTINUE;
+ if (!this.enabled) return Command.CONTINUE;
switch (msg.getType()) {
case PACKET_IN:
@@ -284,7 +266,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
public void enableFirewall() {
// check if the firewall module is not listening for events, if not, then start listening (enable it)
List<IOFMessageListener> listeners = floodlightProvider.getListeners().get(OFType.PACKET_IN);
- if (listeners != null && listeners.contains(this) == false) {
+ if ((listeners != null) && (!listeners.contains(this))) {
// enable firewall, i.e. listen for packet-in events
floodlightProvider.addOFMessageListener(OFType.PACKET_IN, this);
}
@@ -295,7 +277,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
public void disableFirewall() {
// check if the firewall module is listening for events, if yes, then remove it from listeners (disable it)
List<IOFMessageListener> listeners = floodlightProvider.getListeners().get(OFType.PACKET_IN);
- if (listeners != null && listeners.contains(this) == true) {
+ if ((listeners != null) && (listeners.contains(this))) {
// disable firewall, i.e. stop listening for packet-in events
floodlightProvider.removeOFMessageListener(OFType.PACKET_IN, this);
}
@@ -336,7 +318,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
}
@Override
- public void addRule(FirewallRule rule) {
+ public synchronized void addRule(FirewallRule rule) {
rule.ruleid = rule.genID();
int i = 0;
// locate the position of the new rule in the sorted arraylist
@@ -376,7 +358,7 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
}
@Override
- public void deleteRule(int ruleid) {
+ public synchronized void deleteRule(int ruleid) {
Iterator<FirewallRule> iter = this.rules.iterator();
while (iter.hasNext()) {
FirewallRule r = iter.next();
@@ -419,18 +401,19 @@ public class Firewall implements IFirewallService, IOFMessageListener, IFloodlig
Ethernet eth = IFloodlightProviderService.bcStore.get(cntx, IFloodlightProviderService.CONTEXT_PI_PAYLOAD);
WildcardsPair wildcards = new WildcardsPair();
- Iterator<FirewallRule> iter = this.rules.iterator();
-
- FirewallRule rule = null;
- // iterate through list to find a matching firewall rule
- while (iter.hasNext()) {
- // get next rule from list
- rule = iter.next();
-
- // check if rule matches
- if (rule.matchesFlow(sw.getId(), pi.getInPort(), eth, wildcards) == true) {
- matched_rule = rule;
- break;
+ synchronized (rules) {
+ Iterator<FirewallRule> iter = this.rules.iterator();
+ FirewallRule rule = null;
+ // iterate through list to find a matching firewall rule
+ while (iter.hasNext()) {
+ // get next rule from list
+ rule = iter.next();
+
+ // check if rule matches
+ if (rule.matchesFlow(sw.getId(), pi.getInPort(), eth, wildcards) == true) {
+ matched_rule = rule;
+ break;
+ }
}
}
diff --git a/src/test/java/net/floodlightcontroller/firewall/FirewallTest.java b/src/test/java/net/floodlightcontroller/firewall/FirewallTest.java
index 532da8fd8..2919d4eb7 100644
--- a/src/test/java/net/floodlightcontroller/firewall/FirewallTest.java
+++ b/src/test/java/net/floodlightcontroller/firewall/FirewallTest.java
@@ -215,7 +215,7 @@ public class FirewallTest extends FloodlightTestCase {
firewall.receive(sw, this.packetIn, cntx);
verify(sw);
- assertEquals(0, firewall.countRules());
+ assertEquals(0, firewall.rules.size());
IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
// no rules to match, so firewall should deny
@@ -272,7 +272,7 @@ public class FirewallTest extends FloodlightTestCase {
firewall.receive(sw, this.packetIn, cntx);
verify(sw);
- assertEquals(1, firewall.countRules());
+ assertEquals(1, firewall.rules.size());
IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
assertNull(decision);
@@ -335,7 +335,7 @@ public class FirewallTest extends FloodlightTestCase {
rule.priority = 2;
firewall.addRule(rule);
- assertEquals(2, firewall.countRules());
+ assertEquals(2, firewall.rules.size());
// packet destined to TCP port 80 - should be allowed
@@ -374,7 +374,7 @@ public class FirewallTest extends FloodlightTestCase {
// broadcast-ARP traffic should be allowed
IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
- assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+ assertEquals(IRoutingDecision.RoutingAction.MULTICAST, decision.getRoutingAction());
// clear decision
IRoutingDecision.rtStore.remove(cntx, IRoutingDecision.CONTEXT_DECISION);
@@ -408,7 +408,7 @@ public class FirewallTest extends FloodlightTestCase {
// broadcast traffic should be allowed
IRoutingDecision decision = IRoutingDecision.rtStore.get(cntx, IRoutingDecision.CONTEXT_DECISION);
- assertEquals(decision.getRoutingAction(), IRoutingDecision.RoutingAction.FORWARD_OR_FLOOD);
+ assertEquals(IRoutingDecision.RoutingAction.MULTICAST, decision.getRoutingAction());
}
@Test
--
GitLab