Open bmhughes opened 3 years ago
I would like this to be reviewed with the thoughts of we will be shortly creating a PR to put this into Chef 17, so I have asked @lamont-granquist as well as @tas50 to review 👍
We have to keep support for centos6 on this one, just due to chefs customer base and us wanting to put this in core.
I had some issues with CentOS 6 testing (again!) and wanted to FIO but gathered we'd want to keep it even though it's EoL now.
Thoughts on the table
default value? I'd prefer to drop the default and make it required (needs doing for rule as well if so) so that things have to be explicitly set to reduce the setting a rule on the wrong table factor.
But I'll go with the general concensus, the way I do wrapper cookbooks it doesn't matter much to me either way.
This appears not to respect the
cookbook
property and will always use the template from this cookbook instead of a specified one.
Hmm i'll check this out but it should do, the only change that will effect it is it'll take the setting from the first resource called (service/chain/rule) as I only initialise the template resource if it doesn't already exist. Might refactor that thinking about it but then it'll come down the last resource declaration so i'm not 100% convinced it's actually better either.
Figured out why our wrapper template was not getting picked up: the
cookbook
property needs to be set on the_service
resource, not just on the_chain
and_rule
resources. IMO the_service
resource should not affect the rules template at all since it does not modify it, and let the first_chain
or_rule
call create it.The examples in the documentation for
_service
do not match what is needed for the service to correctly restart when the template changes. The test cookbook has thedelayed_action
and manualsubscribes
, which are not mentioned as needed in the docs at all and are required to work correctly (at least from my testing). Would having the template resourcenotify
the service be better, since that way the user does not have to remember tosubscribe
to the template?
Is this with the current release version of the cookbook or this PR? This is still a WIP so there may well be things missing/incorrect/will change so bear this in mind if you're attempting to use it.
The rulefile template resource is going to be created (and thus take it's source) by the first resource that requires it to exist (via the rulefile_resource_init
method), if you create a service resource before a rule or chain then this where the template will be sourced from hence the behaviour you see. The alternative is to update on every resource call which is debatably worse. The service needs the rulefile to exist to start so it has the ability to ensure it is present.
The trouble with adding notifications to these resources internally is that is hard couples them and removes the benefit of pushing everything into resources and removes flexibility for the implementer in their wrapper. How to do this can be added to the documentation for sure, but the overall idea is someone may choose not to do that with their implementation which is why we try to make these resources as abstract as possible.
@bmhughes can you please rebase and resolve the conflicts so we can properly review it?
Is this with the current release version of the cookbook or this PR? This is still a WIP so there may well be things missing/incorrect/will change so bear this in mind if you're attempting to use it.
This was with the current version of this branch.
The rulefile template resource is going to be created (and thus take it's source) by the first resource that requires it to exist (via the
rulefile_resource_init
method), if you create a service resource before a rule or chain then this where the template will be sourced from hence the behaviour you see. The alternative is to update on every resource call which is debatably worse. The service needs the rulefile to exist to start so it has the ability to ensure it is present.
Ah, I see.
The trouble with adding notifications to these resources internally is that is hard couples them and removes the benefit of pushing everything into resources and removes flexibility for the implementer in their wrapper. How to do this can be added to the documentation for sure, but the overall idea is someone may choose not to do that with their implementation which is why we try to make these resources as abstract as possible.
That does make sense, even if that use case is a bit odd.
@bmhughes can you please rebase and resolve the conflicts so we can properly review it?
Done
I'm having some trouble with getting the template to restart when the recipe calling iptables_service
is not the root recipe in the runlist. Our wrapper cookbooks structure follows this pattern:
node::recipe
- wrapper_resource
- wrapper::recipe
- iptables_service
- iptables_chain/_rule
- iptables_chain/_rule
I've created a POC repo that reproduces the problem: https://github.com/detjensrobert/firewall-wrapper-test. Is there something obvious missing with this setup that is preventing the service resource from getting notified?
(testing on C7/C8)
To be perfectly honest (trying not to sound a complete dick here), there's all sorts of things 'wrong' with that cookbook and the patterns it's following so i'm not completely surprised you're having issues and I'd be surprised if they were anything to do with the iptables cookbook itself.
I'm using this internally with the same wrapper I've used for the last couple of version without issue and without too many changes so I know it does work in practice.
Looking at that example my thoughts are:
extend
ing the helpers module into the service resource instance block is messy, you know what the template resource name is going to be, just use that.For constrast, my wrapper contains only a set of recipes that:
Other than those couple of recipes and some methods to set ip_family
automatically there's nothing else in the cookbook.
Also if someone else has 5 can they try the debian 9/ubuntu 1604/1804 tests locally as they pass for me but fall over on actions. It's going to be related to docker/kernel/modules no doubt.
Also if someone else has 5 can they try the debian 9/ubuntu 1604/1804 tests locally as they pass for me but fall over on actions. It's going to be related to docker/kernel/modules no doubt.
Tests pass here in Vagrant & Openstack :+1:
@bmhughes I can't replicate the current failures but I suspect it's related to a missing iptables kernel module not being loaded on the host system before docker runs. If you add something like the following it can probably give you some additional feedback:
@bmhughes Any chance you can get the DCO slapped on here and take a look at the failures in CI?
Description
iptables_packages
toiptables_package
(old name aliased)Issues Resolved
Check List