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

Feature/rebuild fully tested #108

Merged
merged 36 commits into from
Feb 18, 2020
Merged

Feature/rebuild fully tested #108

merged 36 commits into from
Feb 18, 2020

Conversation

xorima
Copy link
Member

@xorima xorima commented Jan 11, 2020

Description

The current iteration of the cookbook does not expose all the common parts of a rule and has used -m comment while match does not prefix with the -m.

This commit totally rewrites the cookbook and is still a work in progress, opening for comments and thoughts

This will also migrate the cookbook to github actions for testing

Things still to do:

  • Have resource to handle the install of iptables (default recipe)
  • Have a resource to clear the rules (disabled recipe)
  • See if we can use the cli instead of a rewrite to disk and reload as a better option (Parsing issues maybe)
  • Write some other resources which are wrappers on iptables_rule, such as a iptables_tcp_rule which will add properties for the options the tcp matcher provides
  • Update the readme with all this documentation (What would your thoughts be on having a documentation folder similar to sous-chefs with each resources documented individually?)

Issues Resolved

[List any existing issues this PR resolves]

Check List

This commit creates a chain resource which will handle default chains, different values and other situations without throwing errors
Also includes full unit testing of the resource and library

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
.github/workflows/ci.yml Outdated Show resolved Hide resolved
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
This commit restores and attempts to retain compatability with existing resources, while not all are possible it does alert where items are no longer supported and have been deprecated

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
…let's keep that for testing

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
@xorima xorima requested a review from tas50 January 12, 2020 22:35
@xorima
Copy link
Member Author

xorima commented Jan 12, 2020

@tas50 will write some docs around this tomorrow if I get time,
What is odd is that while dokken and kitchen pass on my local they do not pass on github actions (Local is fedora) so I am just leaving centos-8 for now, we also have unit tests for this

Documentation plans are:

  • Changelog
  • Upgrading notes
  • Resources documented individually

I have restored as well travis for now (Note, travis also only ran delivery so there has never been testing on this cookbook for kitchen/integration testing)

resources/chain.rb Outdated Show resolved Hide resolved
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
@xorima xorima marked this pull request as ready for review January 13, 2020 08:03
@xorima xorima requested a review from tas50 January 13, 2020 08:25
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
@xorima
Copy link
Member Author

xorima commented Jan 13, 2020

@tas50 ready for a review on this one, will sort out resources for the other recipes if this goes ahead

This commit will line up the properties with the properties that previously existed

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Adds the resource iptables_packages to manage installing iptables on the desired system

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
resources/packages.rb Outdated Show resolved Hide resolved
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Work in progress but adds tests for everything pretty much

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
@xorima
Copy link
Member Author

xorima commented Jan 15, 2020

Left to fix is Centos-6 as some packages do not exist there we use, and any other tests that break, hopefully I can get this all green tomorrow and ready to merge in with full testing supported.

Also want to extend the unit tests on the helper library for the few additional functions now in there

Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
Signed-off-by: Jason Field <jason@avon-lea.co.uk>
@xorima
Copy link
Member Author

xorima commented Jan 16, 2020

@bmhughes do you have any idea how to get this working on centos 6, I keep getting modules cannot be loaded errors (note dokken does not seem to work, but kitchen we get that far ...)

@bmhughes
Copy link
Contributor

bmhughes commented Jan 16, 2020

@xorima ignore the last comment I'd help if I was on the right branch 🤦🏻‍♂️.

It converges for me on vagrant, it fails on stopping when trying to unload the iptables kernel modules which I presume is the same error. It failed with module in use so I'm wondering if it's possible to do in docker? There are a few google hits around this.

@xorima
Copy link
Member Author

xorima commented Jan 16, 2020

Yeah, the tests also fail mate, just banging my head against a wall, centos 6 support ends in november

@bmhughes
Copy link
Contributor

bmhughes commented Jan 16, 2020

On vagrant it's failing the test because the iptable modules are missing and on docker it won't unload them, it's catch 22.

The CentOS 6 vagrant image is missing the ip_conntrack module being loaded by default if you modprobe ip_conntrack then service iptables restart the rules will be loaded and the inspec test will then pass. Not sure how best to work around this with, might be easiest to add an execute resource to the test cookbook to load it as it's going to be a bento PR otherwise.

Docker I'm looking into but that seems more difficult with how the networking side of it works.

@xorima
Copy link
Member Author

xorima commented Jan 16, 2020

If you want to take a stab at getting it in you have write access to my repo :)

@bmhughes
Copy link
Contributor

bmhughes commented Jan 16, 2020

If you want to take a stab at getting it in you have write access to my repo :)

Will do, I have something that may work. I want to test with it a bit more tomorrow and I'll push it, still haven't given up on docker yet.

Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
Change CentOS 6 default behaviour to not unload the kernel modules on
service stop. This fixes testing on Dokken and matches CentOS 7 and later and Fedora.

Add the centos-6-helper recipe to the test cookbook to load the required
modules that are missing in the default bento image.

Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
@bmhughes
Copy link
Contributor

bmhughes commented Jan 16, 2020

It's not perfect but that passes for me on Vagrant and Dokken now.

edit: Hmm doesn't work on azure though with the image they have, I guess if we aren't super fussed about Vagrant testing we can remove the module part.

Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
Signed-off-by: <bmhughes@bmhughes.co.uk>
Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
Signed-off-by: Ben Hughes <bmhughes@bmhughes.co.uk>
@bmhughes
Copy link
Contributor

That needs a serious squash but it passes on CI/dokken/vagrant now.

@xorima
Copy link
Member Author

xorima commented Jan 17, 2020

@bmhughes AMAZING!

Now should that functionality be automatically done by our resources?

@tas50 enjoy the review ;)

@bmhughes
Copy link
Contributor

bmhughes commented Jan 17, 2020

I don't think so as it's all a workaround for Kitchen on various platforms and I believe it shouldn't be relevant for a 'proper' system. Without spinning up a full CentOS 6 VM to test with I never remember having to manually load the modules.

  1. Vagrant/Bento the modules aren't loaded (but should be, there's a quirk there with the packer template I guess). I have no idea why the init script doesn't load them if missing, even more helpful that is successfully exits without actually loading the rules.

  2. The dokken and azure's images load them correctly but for some reason the CentOS 6 init scripts default to unloading the modules as well as flushing the rules which I don't think will be possible with docker as it's dependant on netfilter for networking afaik so there's various differences there between dokken and a vagrant VM. In a container there's always a dependency on at least one of the modules even with all the rules flushed so they won't unload.

I don't feel bad changing the default behaviour on CentOS 6 a bit as they removed unloading the modules upstream going forward anyway.

@tas50 tas50 merged commit c2d5143 into chef-cookbooks:master Feb 18, 2020
@tas50
Copy link
Contributor

tas50 commented Feb 18, 2020

I'm ok with retaining the history. It's huge, but this way we don't squash out valuable information later. Thanks for doing all this @xorima.

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.

3 participants