Merge pull request #34 from zdc/T2189-equuleus

ipset: T2189: optimized firewall groups performance

3 files changed, 99 insertions, 29 deletions

diff --git a/lib/Vyatta/IpTables/IpSet.pm b/lib/Vyatta/IpTables/IpSet.pm
index d7a014a..be50472 100755
--- a/lib/Vyatta/IpTables/IpSet.pm
+++ b/lib/Vyatta/IpTables/IpSet.pm
@@ -28,6 +28,7 @@ use Vyatta::Config;
 use Vyatta::TypeChecker;
 use Vyatta::Misc;
 use NetAddr::IP;
+use XML::LibXML;
 
 use strict;
 use warnings;
@@ -49,11 +50,9 @@ our %grouptype_hash = (
 
 my $logger = 'logger -t IpSet.pm -p local0.warn --';
 
-# Currently we restrict an address range to a /24 even
-# though ipset would support a /16.  The main reason is
-# due to the long time it takes to make that many calls
-# to add each individual member to the set.
-my $addr_range_mask = 24;
+# Restrict an address range to a /16
+# because this is the maximum size of ipset
+my $addr_range_mask = 16;
 my $lockfile = "/opt/vyatta/config/.lock";
 
 # remove lock file to avoid commit blockade on interrupt
@@ -92,7 +91,7 @@ sub debug {
 sub run_cmd {
     my ($self, $cmd) = @_;
 
-    my $rc = system("sudo $cmd");
+    my $rc = system("$cmd");
     if (defined $self->{_debug}) {
         my $func = (caller(1))[3];
         system("$logger [$func] [$cmd] = [$rc]");
@@ -407,24 +406,46 @@ sub check_member {
     return; #undef
 }
 
+# return a hash with all set members
+sub members_list {
+    my ($self) = @_;
+
+    my %elements_list = ();
+
+    my $ipset_cmd = "ipset -o xml -L $self->{_name} 2> /dev/null";
+    my $ipset_output = qx/$ipset_cmd/;
+    # return an empty hash if a command failed
+    if ($?) {
+        return %elements_list;
+    }
+    # parse the output otherwise
+    my $parsed_out = XML::LibXML->load_xml(string => $ipset_output);
+    foreach my $node ($parsed_out->findnodes('/ipsets/ipset/members/member/elem/text()')) {
+        $elements_list{$node} = undef;
+    }
+
+    return %elements_list;
+}
+
+# check if a member is already in a set
+# return 1 if it exists, 0 if not
 sub member_exists {
     my ($self, $member) = @_;
 
-    # check if a member is a port range and roll through all members it is
+    # get current members
+    my %members_current = $self->members_list();
+
+    # check if a member is a port range and check each element of a range
     if ($member =~ /([\d]+)-([\d]+)/) {
         foreach my $port ($1..$2) {
-            # test port with ipset
-            my $cmd = "ipset -T $self->{_name} $port -q";
-            my $rc = $self->run_cmd($cmd);
-            # return true if port was found
-            return 1 if !$rc;
+            # check a port
+            return 1 if exists $members_current{$port};
         }
         # return false if ports was not found in set
         return 0;
     } else {
-        my $cmd = "ipset -T $self->{_name} $member -q";
-        my $rc = $self->run_cmd($cmd);
-        return $rc ? 0 : 1;
+        return 1 if exists $members_current{$member};
+        return 0;
     }
 }
 
@@ -449,21 +470,29 @@ sub add_member {
 sub delete_member_range {
     my ($self, $start, $stop) = @_;
 
+    my %members_current = $self->members_list();
+
+    # check if all elements exist, this is necessary to remove a range in one command
     if ($self->{_type} eq 'port') {
+        # check if all ports exist
         foreach my $member ($start .. $stop) {
-            my $rc = $self->delete_member($member);
-            return $rc if defined $rc;
+            if (not exists $members_current{$member}) {
+                return "Error: member [$member] does not exist in [$self->{_name}]\n";
+            }
         }
     } elsif ($self->{_type} eq 'address') {
-        my $start_ip = new NetAddr::IP("$start/$addr_range_mask");
-        my $stop_ip  = new NetAddr::IP("$stop/$addr_range_mask");
+        my $start_ip = new NetAddr::IP("$start/0");
+        my $stop_ip  = new NetAddr::IP("$stop/0");
+        # check if all IP addresses exist
         for (; $start_ip <= $stop_ip; $start_ip++) {
-            my $rc = $self->delete_member($start_ip->addr());
-            return $rc if defined $rc;
-            last if $start_ip->cidr() eq $start_ip->broadcast();
+            my $current_addr = $start_ip->addr();
+            if (not exists $members_current{$current_addr}) {
+                return "Error: member [$current_addr] does not exist in [$self->{_name}]\n";
+            }
         }
     }
-    return;
+    # remove whole range at once
+    return $self->delete_member_unsafe("$start-$stop");
 }
 
 sub delete_member {
@@ -479,12 +508,20 @@ sub delete_member {
     }
 
     if (!$self->member_exists($member)) {
-        return "Error: member [$member] doesn't exists in [$self->{_name}]\n";
+        return "Error: member [$member] does not exist in [$self->{_name}]\n";
     }
+    return $self->delete_member_unsafe($member);
+}
+
+# delete a member without additional checks,
+# assuming all of them already were performed
+sub delete_member_unsafe {
+    my ($self, $member) = @_;
+
     my $cmd = "ipset -D $self->{_name} $member";
     my $rc = $self->run_cmd($cmd);
-    return "Error: call to ipset failed [$rc]" if $rc;
-    return; # undef
+    return "Error: call to ipset [$cmd] returned [$rc] exit code" if $rc;
+    return;
 }
 
 sub get_description {
diff --git a/scripts/firewall/vyatta-ipset.pl b/scripts/firewall/vyatta-ipset.pl
index a5375dc..43322f3 100755
--- a/scripts/firewall/vyatta-ipset.pl
+++ b/scripts/firewall/vyatta-ipset.pl
@@ -33,6 +33,7 @@ use Vyatta::Misc;
 use Vyatta::IpTables::IpSet;
 use Sort::Versions;
 use IO::Prompt;
+use NetAddr::IP;
 
 use warnings;
 use strict;
@@ -403,23 +404,51 @@ sub check_duplicates {
       # check if this is a port range
       if ($item =~ /([\d]+)-([\d]+)/) {
         foreach my $port ($1..$2) {
-          return "Port $port exist in more than one item\n" if (exists $portlist{$port});
+          return "Port $port exists in more than one configuration enrty\n" if (exists $portlist{$port});
           $portlist{$port} = undef;
         }
 
       # check if this is an alphabetic port name
       } elsif ($item =~ /^\D+/) {
         my $port = getservbyname($item, "");
-        return "Port $port exist in more than one item\n" if (exists $portlist{$port});
+        return "Port $port exists in more than one configuration enrty\n" if (exists $portlist{$port});
         $portlist{$port} = undef;
 
       # process simple numeric ports
       } else {
-        return "Port $item exist in more than one item\n" if (exists $portlist{$item});
+        return "Port $item exists in more than one configuration enrty\n" if (exists $portlist{$item});
         $portlist{$item} = undef;
       }
     }
   }
+  # check duplicates in address-group
+  if ($set_type eq "address") {
+    # define hash with addresses as keys
+    my %addresslist;
+
+    for my $item (@vals) {
+      # check if this is an address range
+      if ($item =~ /(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})-(\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3})/) {
+        my $first_ip = new NetAddr::IP("$1/0");
+        my $last_ip = new NetAddr::IP("$2/0");
+
+        for (; $first_ip <= $last_ip; $first_ip++) {
+            my $current_addr = $first_ip->addr();
+            # check if an address already listed
+            if (exists $addresslist{$current_addr}) {
+                return "Address $current_addr exists in more than one configuration enrty\n";
+            }
+            # add an address to a list
+            $addresslist{$current_addr} = undef;
+        }
+      # process single addresses
+      } else {
+        return "Address $item exists in more than one configuration enrty\n" if (exists $addresslist{$item});
+        # add an address to a list
+        $addresslist{$item} = undef;
+      }
+    }
+  }
 
   # do not return anything if there are no duplicates
   return;
diff --git a/templates/firewall/group/address-group/node.def b/templates/firewall/group/address-group/node.def
index d89233d..b210fc1 100644
--- a/templates/firewall/group/address-group/node.def
+++ b/templates/firewall/group/address-group/node.def
@@ -19,6 +19,10 @@ syntax:expression: exec "/opt/vyatta/sbin/vyatta-ipset.pl --action=is-group-defi
         --set-type=address --set-family=inet"; \
       "Firewall group name already used as Ipv6 group address"       
 
+commit:expression:exec "/opt/vyatta/sbin/vyatta-ipset.pl --action=check-duplicates --set-name=$VAR(@) \
+        --set-type=address --set-family=inet"; \
+       "There are duplicates inside address-group $VAR(@)"
+
 end: if sudo /opt/vyatta/sbin/vyatta-ipset.pl --action=update-set \
         --set-name="$VAR(@)" --set-type=address --set-family=inet; then
        ${vyatta_sbindir}/vyatta-firewall-trap.pl --level="firewall group address-group $VAR(@)"