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

Update run to make fail2ban.local consistent with jail.local #508

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

Conversation

gineer01
Copy link

Updating /config/fail2ban/jail.local will update its copy in /etc/fail2ban/. Users would expect the same behavior with fail2ban.local

linuxserver.io


  • I have read the contributing guideline and understand that I have made the correct modifications

Description:

Updating /config/fail2ban/jail.local will update its copy in /etc/fail2ban/. Users would expect the same behavior with fail2ban.local

Benefits of this PR and context:

Users can update the /config/fail2ban/fail2ban.local and it should work like /config/fail2ban/jail.local

How Has This Been Tested?

Source / References:

Updating /config/fail2ban/jail.local will update its copy in /etc/fail2ban/. Users would expect the same behavior with fail2ban.local
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this pull request! Be sure to follow the pull request template!

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-cae6b147-dev-198b7358942ae3852daf9d498a962434183198b9-pr-508/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-cae6b147-dev-198b7358942ae3852daf9d498a962434183198b9-pr-508/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-cae6b147-dev-198b7358942ae3852daf9d498a962434183198b9-pr-508
arm64v8-2.11.0-pkg-cae6b147-dev-198b7358942ae3852daf9d498a962434183198b9-pr-508

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-f9d83926-dev-d28ffde85819af48a6fc97b5562df2f385cfe430-pr-508/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-f9d83926-dev-d28ffde85819af48a6fc97b5562df2f385cfe430-pr-508/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-f9d83926-dev-d28ffde85819af48a6fc97b5562df2f385cfe430-pr-508
arm64v8-2.11.0-pkg-f9d83926-dev-d28ffde85819af48a6fc97b5562df2f385cfe430-pr-508

@aptalca
Copy link
Member

aptalca commented Sep 28, 2024

It's there a specific reason you're trying to modify it? It wasn't designed to be user configurable as other init steps depend on it and modifications would likely break things.

@gineer01
Copy link
Author

I was trying to change logtarget location in fail2ban.local to another location (for indexing/retention). It took me quite some time and some debugging to realize fail2ban.local doesn't behave like jail.local. Changes to /config/fail2ban/jail.local are copied but changes in /config/fail2ban/fail2ban.local are ignored. Making fail2ban.local consistent with jail.local will save other users from surprises.

@aptalca
Copy link
Member

aptalca commented Sep 28, 2024

We never make fail2ban.local available in the config folder, only jail.local. And our docs state jail.local is user editable as well as any existing action or filter via user created .local files.

Our init and services expect the logs in the defined location. We even have logrotate set up. We don't intend to make that user configurable.

@gineer01
Copy link
Author

You're right. SWAG doc says "create .local files with the same name and edit those because .conf files get overwritten when the actions and filters are updated". I extrapolated that to include fail2ban.conf and expected adding fail2ban.local would behave the same. I suspect many will extrapolate the same.

What's the impact of moving fail2ban log location? Beside logrotate, what other things might be tied to the log location? If it's just logrotate, I'm not concerned logrotate has the wrong fail2ban log location because the new location has its own retention/rotation policy.

NGINX log location is easier to change: just add access_log to nginx.conf. So, I think it should be easy to change fail2ban log location as well.

@aptalca
Copy link
Member

aptalca commented Sep 29, 2024

You're the first person to ask about this in 8 years so I would argue it is not a common point of confusion.

https://www.linuxserver.io/blog/why-cant-you-just-implement-thing-i-want

https://www.linuxserver.io/blog/2019-09-14-customizing-our-containers

@gineer01
Copy link
Author

I actually did use the custom script approach mentioned in your second link, but this was only after wasting some time thinking fail2ban.local should work and tracing the root cause to this script. It's doable but it's not as easy or consistent with jail.local.

I created this PR even though I already had a workaround because I thought it's a violation of Principle of least astonishment. In the original script, fail2ban.local is surprisingly copied directly from /defaults/ while other config files are copied from /config/fail2ban/ to /etc/fail2ban/

cp -R /config/fail2ban/filter.d /etc/fail2ban/
cp -R /config/fail2ban/action.d /etc/fail2ban/
cp /defaults/fail2ban/fail2ban.local /etc/fail2ban/
cp /config/fail2ban/jail.local /etc/fail2ban/jail.local

So, I did the first option. This PR is the second option described in your first link 😄 . I'd argue making things consistent reduces complexity.

If you really, desperately, want particular functionality in one of our images, you've got a few options:

  1. Modify the container at runtime using our custom script logic or mods
  2. Convince us that your request would be beneficial to a broad range of other users and isn't going to introduce unnecessary complexity
    Even better, submit a PR that does the above

@LinuxServer-CI
Copy link
Contributor

I am a bot, here are the test results for this PR:
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-78c3aac3-dev-7b1f2fefca13de9e76ada5fd9251d86cdde38218-pr-508/index.html
https://ci-tests.linuxserver.io/lspipepr/swag/2.11.0-pkg-78c3aac3-dev-7b1f2fefca13de9e76ada5fd9251d86cdde38218-pr-508/shellcheck-result.xml

Tag Passed
amd64-2.11.0-pkg-78c3aac3-dev-7b1f2fefca13de9e76ada5fd9251d86cdde38218-pr-508
arm64v8-2.11.0-pkg-78c3aac3-dev-7b1f2fefca13de9e76ada5fd9251d86cdde38218-pr-508

@aptalca
Copy link
Member

aptalca commented Oct 25, 2024

I created this PR even though I already had a workaround because I thought it's a violation of Principle of least astonishment.

Changing it now would be a violation of the KISS principle.

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

Successfully merging this pull request may close these issues.

3 participants