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

Add support the the iptables recent module. #206

Conversation

stephengrier
Copy link
Contributor

Add support for the iptables recent module.

@kbarber-jenkins-bot
Copy link

Can one of the admins verify this patch?

@kbarber
Copy link
Contributor

kbarber commented Jun 10, 2013

This is failing due to a known issue with rspec-system 1.x. If you rebase and push again, hopefully the unit tests will succeed.

@stephengrier
Copy link
Contributor Author

Rebased as requested.

@kbarber-jenkins-bot
Copy link

Can one of the admins verify this patch?

@apenney
Copy link
Contributor

apenney commented Jul 5, 2013

Hi,

I'm afraid the module marched on and this needs rebasing yet again. Any chance you could redo that for me and we'll get this reviewed.

@stephengrier
Copy link
Contributor Author

Rebased again, as requested. Could someone please take a look.

@stephengrier
Copy link
Contributor Author

Any update on this pull request? Do I need to do anything else?

@apenney
Copy link
Contributor

apenney commented Sep 13, 2013

Hi,

Sorry, I'm just running through PRs at the moment and trying to catch up. I haven't spent a lot of time in the firewall module so I'm always a bit cautious around it. I noticed you munge a bunch of stuff that doesn't take values in this. I was wondering if you've seen #208 ? It extends the "known booleans" stuff to be a bit more flexible.

Was I was thinking is if that code passes systems tests that I could merge that in, get you to rebase against master and then replace the munging with the boolean code that already exists so we don't have multiple ways to munge these.

Does that make sense? I know this PR has been lingering forever so I wouldn't be surprised if you're fed up of the whole thing. If you're not OK with those changes then I'll try to merge it in and attempt to fix it afterwards, but I'm not as experienced with the code as you are at this point so it'll probably go smoother without my bungling.

@iaslanidis
Copy link

How is this issue different from #120 ?

@stephengrier
Copy link
Contributor Author

Iaslanidis,

This does implement --set, --rcheck and friends. In fact, I use this code to implement port knocking.

You can supply set, update, rcheck or remove options like so:

firewall { 'knock1':
recent => 'set',
rname => 'DEFAULT',
}
firewall { 'knock2':
recent => 'update',
rseconds => 60,
rhitcount => 4,
rname => 'DEFAULT',
}
firewall { 'knock3':
recent => "rcheck",
rseconds => 5,
rname => "KNOCK1",
}
... etc.

@iaslanidis
Copy link

I was expecting the set, rcheck and so on to be parameters, not attributes. In any case, this is excellent news. Are we getting this into the documentation?

@stephengrier
Copy link
Contributor Author

OK, I've rebased again, and changed the code a little to use known_booleans and removed the munging, as requested. Can someone take a look at it.

@thias
Copy link
Contributor

thias commented Oct 9, 2013

I can't comment on the code itself, but it works as expected, and I've successfully adapted port knocking rules using these changes : https://github.com/thias/puppet-rhel/blob/master/manifests/firewall/portknock.pp

@stephengrier : Thanks a lot! I hope this gets reviewed and included soon.

@apenney
Copy link
Contributor

apenney commented Oct 16, 2013

Argh, I hate to be "that guy" again but can you rebase this a final time? I promise to merge it before anything else if you do.

@apenney
Copy link
Contributor

apenney commented Oct 16, 2013

I also just realized this has no unit tests or acceptance tests. Any chance you can take a look at copying some of the existing unit tests so we can be sure this doesn't break in future.

@mikebryant
Copy link
Contributor

@apenney I need this feature, so I've rebased this PR in #292 and added some basic unit and acceptance tests. Let me know if anything else needs doing.

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.

None yet

7 participants