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

Fix rpfilter parameter #1013

Merged
merged 4 commits into from
May 16, 2022
Merged

Conversation

onyxmaster
Copy link
Contributor

Hi,
In the current version of the module, the rpfilter parameter works as a no-op, no matter which value is provided since the feature is not specified. This commit fixes this issue.

Unfortunately, this is not the only issue with the parameter in question. The xt_rpfilter is one of the few extensions that perform meaningful work even when there are no extra parameters specified (in fact the default strict mode is invoked by just the -m rpfilter stanza), and currently, the Puppet module requires for the mode to be set to one of loose/validmark/accept-local/invert. In fact, all of these are optional flags and can be enabled simultaneously or not enabled at all. I'm not sure how to fix this and whether I should report this issue somewhere else.

So while this PR addresses part of the problem with rpfilter parameter just doing nothing, it doesn't fix the main issue with rpfilter support. Currently, to use default strict matching one can work around the issue by using one of the flags that do not significantly affect the processing most of the time (such as setting rpfilter => 'validmark'), but this is only a hacky workaround, not a complete solution. Please advise on where should I report the issue so it could be fixed completely.

@onyxmaster onyxmaster requested a review from a team as a code owner September 6, 2021 17:33
@puppet-community-rangefinder
Copy link

firewall is a type

Breaking changes to this file WILL impact these 121 modules (exact match):
Breaking changes to this file MAY impact these 143 modules (near match):

This module is declared in 108 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@onyxmaster
Copy link
Contributor Author

It looks like the proposed improvement highlighted the issue with testing environment, the CentOS-6 doesn't appear to have support for the xt_rpfilter extension :(

@onyxmaster
Copy link
Contributor Author

Well, it looks like I fixed the problem with CentOS 6 by adding proper version checks. @adrianiurca, could you please take a look?

@github-actions
Copy link

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label Apr 26, 2022
@onyxmaster
Copy link
Contributor Author

This PR is important, but there is no feedback on it :(

@LukasAud
Copy link
Contributor

Hi @onyxmaster, sorry for the delay in feedback. Our team has been undergoing some major changes over the last few months and that has translated into a lower level of engagement from our side with the community.

I will remove the 'stale' label and, hopefully, a member from our team might be able to take a look soon at your PR.

@LukasAud LukasAud removed the stale label Apr 26, 2022
@LukasAud
Copy link
Contributor

Hi @onyxmaster, your changes make sense to me and all checks are green, happy to merge this!

Regarding the bigger issue affecting the module, you can report it by creating a new issue here in GH. Please make sure you add as much relevant information as you can (even linking this PR would help) so that the next person that starts working on it has an easier time troubleshooting.

Thanks for your contribution.

@LukasAud LukasAud merged commit 048da12 into puppetlabs:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants