diff mbox series

[09/12] rules.pl: Do not try to restore the same ipset multiple times.

Message ID 20220214184257.2406-9-stefan.schantl@ipfire.org
State Accepted
Commit 278289690d50d6f28926742e29f5b005293132eb
Headers
Series [01/12] location-functions.pl: Rename and set the location for exported databases to "/var/lib/location/ipset/". |

Commit Message

Stefan Schantl Feb. 14, 2022, 6:42 p.m. UTC 
  When an ipset list get restored, this now will be documented in a hash
and this hash also will be checked before restoring a list if this has
not be done previously.

This will prevent from restoring the same list multiple times.

Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
---
 config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

Comments

Peter Müller Feb. 14, 2022, 9:05 p.m. UTC | #1 
Reviewed-by: Peter Müller <peter.mueller@ipfire.org>

> When an ipset list get restored, this now will be documented in a hash
> and this hash also will be checked before restoring a list if this has
> not be done previously.
> 
> This will prevent from restoring the same list multiple times.
> 
> Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
> ---
>  config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
> index d533ffb42..29990ee67 100644
> --- a/config/firewall/rules.pl
> +++ b/config/firewall/rules.pl
> @@ -70,6 +70,7 @@ my %confignatfw=();
>  my %locationsettings = (
>  	"LOCATIONBLOCK_ENABLED" => "off"
>  );
> +my %loaded_ipset_lists=();
>  
>  my @p2ps=();
>  
> @@ -405,8 +406,14 @@ sub buildrules {
>  						# Grab location code from hash.
>  						my $loc_src = $$hash{$key}[4];
>  
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_src);
> +						# Check if the network list for this country already has been loaded.
> +						unless($loaded_ipset_lists{$loc_src}) {
> +							# Call function to load the networks list for this country.
> +							&ipset_restore($loc_src);
> +
> +							# Store to the hash that this list has been loaded.
> +							$loaded_ipset_lists{$loc_src} = "1";
> +						}
>  
>  						push(@source_options, $source);
>  					} elsif($source) {
> @@ -419,8 +426,14 @@ sub buildrules {
>  						# Grab location code from hash.
>  						my $loc_dst = $$hash{$key}[6];
>  
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_dst);
> +						# Check if the network list for this country already has been loaded.
> +						unless($loaded_ipset_lists{$loc_dst}) {
> +							# Call function to load the networks list for this country.
> +							&ipset_restore($loc_dst);
> +
> +							# Store to the hash that this list has been loaded.
> +							$loaded_ipset_lists{$loc_dst} = "1";
> +						}
>  
>  						push(@destination_options,  $destination);
>  					} elsif ($destination) {
> @@ -683,8 +696,14 @@ sub locationblock {
>  	# is enabled.
>  	foreach my $location (@locations) {
>  		if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") {
> -			# Call function to load the networks list for this country.
> -			&ipset_restore($location);
> +			# Check if the network list for this country already has been loaded.
> +			unless($loaded_ipset_lists{$location}) {
> +				# Call function to load the networks list for this country.
> +				&ipset_restore($location);
> +
> +				# Store to the hash that this list has been loaded.
> +				$loaded_ipset_lists{$location} = "1";
> +			}
>  
>  			# Call iptables and create rule to use the loaded ipset list.
>  			run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP");
Michael Tremer Feb. 15, 2022, 12:39 p.m. UTC | #2 
Hello,

I would have implemented this differently.

Would it not be better to perform the check in ipset_restore() so that you won’t have to copy the code to everywhere you call ipset_restore?

This solution bloats the code slightly.

-Michael

> On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl@ipfire.org> wrote:
> 
> When an ipset list get restored, this now will be documented in a hash
> and this hash also will be checked before restoring a list if this has
> not be done previously.
> 
> This will prevent from restoring the same list multiple times.
> 
> Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
> ---
> config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
> index d533ffb42..29990ee67 100644
> --- a/config/firewall/rules.pl
> +++ b/config/firewall/rules.pl
> @@ -70,6 +70,7 @@ my %confignatfw=();
> my %locationsettings = (
> 	"LOCATIONBLOCK_ENABLED" => "off"
> );
> +my %loaded_ipset_lists=();
> 
> my @p2ps=();
> 
> @@ -405,8 +406,14 @@ sub buildrules {
> 						# Grab location code from hash.
> 						my $loc_src = $$hash{$key}[4];
> 
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_src);
> +						# Check if the network list for this country already has been loaded.
> +						unless($loaded_ipset_lists{$loc_src}) {
> +							# Call function to load the networks list for this country.
> +							&ipset_restore($loc_src);
> +
> +							# Store to the hash that this list has been loaded.
> +							$loaded_ipset_lists{$loc_src} = "1";
> +						}
> 
> 						push(@source_options, $source);
> 					} elsif($source) {
> @@ -419,8 +426,14 @@ sub buildrules {
> 						# Grab location code from hash.
> 						my $loc_dst = $$hash{$key}[6];
> 
> -						# Call function to load the networks list for this country.
> -						&ipset_restore($loc_dst);
> +						# Check if the network list for this country already has been loaded.
> +						unless($loaded_ipset_lists{$loc_dst}) {
> +							# Call function to load the networks list for this country.
> +							&ipset_restore($loc_dst);
> +
> +							# Store to the hash that this list has been loaded.
> +							$loaded_ipset_lists{$loc_dst} = "1";
> +						}
> 
> 						push(@destination_options,  $destination);
> 					} elsif ($destination) {
> @@ -683,8 +696,14 @@ sub locationblock {
> 	# is enabled.
> 	foreach my $location (@locations) {
> 		if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") {
> -			# Call function to load the networks list for this country.
> -			&ipset_restore($location);
> +			# Check if the network list for this country already has been loaded.
> +			unless($loaded_ipset_lists{$location}) {
> +				# Call function to load the networks list for this country.
> +				&ipset_restore($location);
> +
> +				# Store to the hash that this list has been loaded.
> +				$loaded_ipset_lists{$location} = "1";
> +			}
> 
> 			# Call iptables and create rule to use the loaded ipset list.
> 			run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP");
> -- 
> 2.30.2
>
Stefan Schantl Feb. 17, 2022, 5:35 a.m. UTC | #3 
Hello Michael,

thanks for reviewing and your feedback.

You are absolutely right, it would give us much cleaner code when
moving this kind of check into the ipset_restore() function.

I'll send a patch for this.

Best regards,

-Stefan
> Hello,
> 
> I would have implemented this differently.
> 
> Would it not be better to perform the check in ipset_restore() so
> that you won’t have to copy the code to everywhere you call
> ipset_restore?
> 
> This solution bloats the code slightly.
> 
> -Michael
> 
> > On 14 Feb 2022, at 18:42, Stefan Schantl
> > <stefan.schantl@ipfire.org> wrote:
> > 
> > When an ipset list get restored, this now will be documented in a
> > hash
> > and this hash also will be checked before restoring a list if this
> > has
> > not be done previously.
> > 
> > This will prevent from restoring the same list multiple times.
> > 
> > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
> > ---
> > config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
> > 1 file changed, 25 insertions(+), 6 deletions(-)
> > 
> > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
> > index d533ffb42..29990ee67 100644
> > --- a/config/firewall/rules.pl
> > +++ b/config/firewall/rules.pl
> > @@ -70,6 +70,7 @@ my %confignatfw=();
> > my %locationsettings = (
> >         "LOCATIONBLOCK_ENABLED" => "off"
> > );
> > +my %loaded_ipset_lists=();
> > 
> > my @p2ps=();
> > 
> > @@ -405,8 +406,14 @@ sub buildrules {
> >                                                 # Grab location
> > code from hash.
> >                                                 my $loc_src =
> > $$hash{$key}[4];
> > 
> > -                                               # Call function to
> > load the networks list for this country.
> > -
> >                                                &ipset_restore($loc_s
> > rc);
> > +                                               # Check if the
> > network list for this country already has been loaded.
> > +                                               unless($loaded_ipse
> > t_lists{$loc_src}) {
> > +                                                       # Call
> > function to load the networks list for this country.
> > +                                                       &ipset_rest
> > ore($loc_src);
> > +
> > +                                                       # Store to
> > the hash that this list has been loaded.
> > +                                                       $loaded_ips
> > et_lists{$loc_src} = "1";
> > +                                               }
> > 
> >                                                 push(@source_option
> > s, $source);
> >                                         } elsif($source) {
> > @@ -419,8 +426,14 @@ sub buildrules {
> >                                                 # Grab location
> > code from hash.
> >                                                 my $loc_dst =
> > $$hash{$key}[6];
> > 
> > -                                               # Call function to
> > load the networks list for this country.
> > -
> >                                                &ipset_restore($loc_d
> > st);
> > +                                               # Check if the
> > network list for this country already has been loaded.
> > +                                               unless($loaded_ipse
> > t_lists{$loc_dst}) {
> > +                                                       # Call
> > function to load the networks list for this country.
> > +                                                       &ipset_rest
> > ore($loc_dst);
> > +
> > +                                                       # Store to
> > the hash that this list has been loaded.
> > +                                                       $loaded_ips
> > et_lists{$loc_dst} = "1";
> > +                                               }
> > 
> >                                                 push(@destination_o
> > ptions,  $destination);
> >                                         } elsif ($destination) {
> > @@ -683,8 +696,14 @@ sub locationblock {
> >         # is enabled.
> >         foreach my $location (@locations) {
> >                 if(exists $locationsettings{$location} &&
> > $locationsettings{$location} eq "on") {
> > -                       # Call function to load the networks list
> > for this country.
> > -                       &ipset_restore($location);
> > +                       # Check if the network list for this
> > country already has been loaded.
> > +                       unless($loaded_ipset_lists{$location}) {
> > +                               # Call function to load the
> > networks list for this country.
> > +                               &ipset_restore($location);
> > +
> > +                               # Store to the hash that this list
> > has been loaded.
> > +                               $loaded_ipset_lists{$location} =
> > "1";
> > +                       }
> > 
> >                         # Call iptables and create rule to use the
> > loaded ipset list.
> >                         run("$IPTABLES -A LOCATIONBLOCK -m set --
> > match-set CC_$location src -j DROP");
> > -- 
> > 2.30.2
> > 
>
diff mbox series

Patch

diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl
index d533ffb42..29990ee67 100644
--- a/config/firewall/rules.pl
+++ b/config/firewall/rules.pl
@@ -70,6 +70,7 @@  my %confignatfw=();
 my %locationsettings = (
 	"LOCATIONBLOCK_ENABLED" => "off"
 );
+my %loaded_ipset_lists=();
 
 my @p2ps=();
 
@@ -405,8 +406,14 @@  sub buildrules {
 						# Grab location code from hash.
 						my $loc_src = $$hash{$key}[4];
 
-						# Call function to load the networks list for this country.
-						&ipset_restore($loc_src);
+						# Check if the network list for this country already has been loaded.
+						unless($loaded_ipset_lists{$loc_src}) {
+							# Call function to load the networks list for this country.
+							&ipset_restore($loc_src);
+
+							# Store to the hash that this list has been loaded.
+							$loaded_ipset_lists{$loc_src} = "1";
+						}
 
 						push(@source_options, $source);
 					} elsif($source) {
@@ -419,8 +426,14 @@  sub buildrules {
 						# Grab location code from hash.
 						my $loc_dst = $$hash{$key}[6];
 
-						# Call function to load the networks list for this country.
-						&ipset_restore($loc_dst);
+						# Check if the network list for this country already has been loaded.
+						unless($loaded_ipset_lists{$loc_dst}) {
+							# Call function to load the networks list for this country.
+							&ipset_restore($loc_dst);
+
+							# Store to the hash that this list has been loaded.
+							$loaded_ipset_lists{$loc_dst} = "1";
+						}
 
 						push(@destination_options,  $destination);
 					} elsif ($destination) {
@@ -683,8 +696,14 @@  sub locationblock {
 	# is enabled.
 	foreach my $location (@locations) {
 		if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") {
-			# Call function to load the networks list for this country.
-			&ipset_restore($location);
+			# Check if the network list for this country already has been loaded.
+			unless($loaded_ipset_lists{$location}) {
+				# Call function to load the networks list for this country.
+				&ipset_restore($location);
+
+				# Store to the hash that this list has been loaded.
+				$loaded_ipset_lists{$location} = "1";
+			}
 
 			# Call iptables and create rule to use the loaded ipset list.
 			run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP");