nftables-geoip icon indicating copy to clipboard operation
nftables-geoip copied to clipboard

Separate dictionary creation

Open sunshineplan opened this issue 4 years ago • 7 comments

Downloading db-ip.com geoip csv file... Writing country definition files... Writing nftables maps (geoip-ipv{4,6}.nft)... Killed

Machine: GCP f1-micro

sunshineplan avatar Feb 22 '20 09:02 sunshineplan

Hi sunshine, I think this is a good enhacement idea.

I just need to take some time organizing the project before, like adding some contribution guide and dealing with formatting issues (using autopep8 and pylint)

pvxe avatar Feb 23 '20 19:02 pvxe

Hi JMGuisadoG Thank you for reopening this issue.

In fact, even if this script after modified can be run successfully, it will also trigger oom when nft -f output file. This is an nft memory issue. The only way is add more memory or modify the script to fetch smaller list(In my case, I only need one country IP list). If choose add more memory, this issue will not happen. So I think this issue is not strongly needed.

It is kind if warning people use geoip nftables function required more than 300M free memory space.

sunshineplan avatar Feb 24 '20 01:02 sunshineplan

It's indeed interesting and a good opportunity to take into account use cases I did not foresee.

In fact, even if this script after modified can be run successfully, it will also trigger oom when nft -f output file. This is an nft memory issue. The only way is add more memory or modify the script to fetch smaller list(In my case, I only need one country IP list). If choose add more memory, this issue will not happen. So I think this issue is not strongly needed.

Output by continent should also be implemented and should alleviate memory usage when using nft -f. Probably I'll open a separate issue for that.

As you can see geoip-def-{continent}.nft are created but only geoip-def-all.nft can be used as of now. A filter is to be added to generate continent-specific eg. geoip-ipv4-{continent}

It is kind if warning people use geoip nftables function required more than 300M free memory space.

Maybe I should add this to the caveats section.

Thanks for your feedback!

pvxe avatar Feb 24 '20 08:02 pvxe

Another possibility, if I may suggest one, would be to limit the map to a limited set of countries (specified on the command line) instead than by continents.

weregoat avatar Feb 27 '20 10:02 weregoat

Another possibility, if I may suggest one, would be to limit the map to a limited set of countries (specified on the command line) instead than by continents.

This is a good idea, all this may fall into enhancements to the script functionality, adding parameters and modularizing the execution, so probably I'd open separate issues or create a GitHub project to track this properly.

pvxe avatar Feb 27 '20 11:02 pvxe

For the record, new changes have reduced memory usage of the script execution by 150MB more or less.

command time -v ./nft_geoip.py --download --file-location location.csv -o output/

Yields Maximum resident set size (kbytes): 168848


As of now, there exists other tools¹ that focus on generation of geoip sets for nftables. So, I think it reasonable to point to those tools if a set is preferred over a map.

I'd have no problem adding a country filter as arguments but I don't know how preferable is a few countries map over an address set for each country. I most probably will add optional flags to only write ipv4 or ipv6, writing both if none of the flags are present.

¹ geoipsets

pvxe avatar Jun 30 '20 15:06 pvxe

As of now, there exists other tools¹ that focus on generation of geoip sets for nftables. So, I think it reasonable to point to those tools if a set is preferred over a map.

It's a fair point, I agree.

weregoat avatar Jul 01 '20 06:07 weregoat