Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate dictionary creation #1

Closed
sunshineplan opened this issue Feb 22, 2020 · 9 comments
Closed

Separate dictionary creation #1

sunshineplan opened this issue Feb 22, 2020 · 9 comments
Labels
enhancement New feature or request

Comments

@sunshineplan
Copy link
Contributor

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

Machine: GCP f1-micro

@pvxe
Copy link
Owner

pvxe commented Feb 23, 2020

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 pvxe reopened this Feb 23, 2020
@pvxe pvxe changed the title MemoryError or trigger OOM Killer on low-memory machine Separate dictionary creation Feb 23, 2020
@pvxe pvxe added the enhancement New feature or request label Feb 23, 2020
@sunshineplan
Copy link
Contributor Author

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.

@pvxe
Copy link
Owner

pvxe commented Feb 24, 2020

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!

@weregoat
Copy link

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.

@pvxe
Copy link
Owner

pvxe commented Feb 27, 2020

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
Copy link
Owner

pvxe commented Jun 30, 2020

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

@weregoat
Copy link

weregoat commented Jul 1, 2020

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.

@TCB13
Copy link

TCB13 commented Oct 11, 2023

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.

Yes! Because I guess most people want to use this on a basis of "I want to block traffic from everywhere except for country A and B". Separate files like geoip-def-ipv4-{contry}.nft / geoip-def-ipv6-{contry}.nft would be perfect.

@pvxe
Copy link
Owner

pvxe commented Jan 24, 2024

This PR has been merged into the script: #7

Now there is the possibility to generate nft maps containing only a set of specified countries using the -c/--country-filter parameter.

@pvxe pvxe closed this as completed Jan 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants