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

Introduce a systemd service file #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

melroy89
Copy link
Contributor

  • Introducing a service file
  • Update README with additional setup of systemd service file

Remember that /etc/rc.local is discontinued and obsolete.

@melroy89
Copy link
Contributor Author

@trick77 Could you please merge this? It would help myself in the upcoming future again..

@trick77
Copy link
Owner

trick77 commented Apr 14, 2023

Hey @melroy89
I think there is a problem with that PR. It currently removes the first rule in the INPUT chain, which may not always be the intended rule. This makes the script non-idempotent and could cause unexpected behavior. Please revise the script to safely delete only the specific rule added by the ExecStart command. If you prefer, you can always fork the project and include your changes in your own repository.

@melroy89
Copy link
Contributor Author

melroy89 commented Apr 14, 2023

Hey @melroy89 I think there is a problem with that PR. It currently removes the first rule in the INPUT chain, which may not always be the intended rule. This makes the script non-idempotent and could cause unexpected behavior. Please revise the script to safely delete only the specific rule added by the ExecStart command. If you prefer, you can always fork the project and include your changes in your own repository.

But that is exactly what your snippet is doing now as well in your Readme: https://github.com/trick77/ipset-blacklist#iptables-filter-rule

The problem is that you now also tell people to use /etc/rc.local which should really not be used any anymore.

@trick77
Copy link
Owner

trick77 commented Apr 14, 2023

The README.md says to delete the top rule in the INPUT chain?

@melroy89
Copy link
Contributor Author

Ow sorry your Readme only saids to add the blacklist to top chain it seems.

@trick77
Copy link
Owner

trick77 commented Apr 14, 2023

I'm with you though that the rc.local approach is outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants