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.

Sign up for GitHub

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

Jump to bottom

Add Docker-CE default rules #80

Merged
merged 6 commits into from
Mar 24, 2021
Merged

Add Docker-CE default rules #80

merged 6 commits into from
Mar 24, 2021

Conversation

luisfdez
Copy link

@luisfdez luisfdez commented Mar 8, 2021

Add Docker-CE default rules

This Pull Request adds the set of rules and chains that docker-ce daemon sets on a CentOS8 machine by default.

These rules are defined in ip-filter, not playing nicely with the defaults shipped for inet-filter.

This merge request adds the set of rules that Docker deploys by default. I don't know how relevant all of them are, but I just followed what the daemon was doing.

This Pull Request (PR) fixes the following issues

None

@nbarrientos nbarrientos added the enhancement New feature or request label Mar 8, 2021
@nbarrientos
Copy link
Collaborator

Thanks!

Could you please amend the lint errors the CI suggests and also add the new class to the all_rules acceptance test?

@luisfdez luisfdez force-pushed the dockerce branch 2 times, most recently from 9a5e15e to 9eac334 Compare March 8, 2021 21:06
@luisfdez
Copy link
Author

luisfdez commented Mar 8, 2021

Thanks Nacho, I have fixed the lint errors and added the new class to the acceptance tests.

@nbarrientos
Copy link
Collaborator

nbarrientos commented Mar 9, 2021 •
edited
Loading

Thanks! Maybe it'd be a good idea to add some unit test coverage to the new class to help us catching regressions in the future and to make sure for instance that the parameters are correctly taken into account. I've written some that you can just import here if you like them or use as skeleton for your own:

nbarrientos@2b7935f

traylenator
traylenator requested changes Mar 9, 2021
View reviewed changes
manifests/rules/docker_ce.pp Outdated Show resolved Hide resolved
@traylenator
Copy link
Collaborator

Presumably as machines are created and destroyed new rules will be created and deleted ?
Do we need some docs to to set the noflush_tables parameter ?

@luisfdez
Copy link
Author

Thanks! Maybe it'd be a good idea to add some unit test coverage to the new class to help us catching regressions in the future and to make sure for instance that the parameters are correctly taken into account. I've written some that you can just import here if you like them or use as skeleton for your own:

cernops@2b7935f

Thanks Nacho, I've taken your example and expanded with other rules.

@luisfdez
Copy link
Author

Presumably as machines are created and destroyed new rules will be created and deleted ?
Do we need some docs to to set the noflush_tables parameter ?

I don't know what would be the right approach to make it rock solid. In principle in a default/basic usage there shouldn't be rules added as it relies on the interface and the address space. In any case, I'm sure that there will be cases not covered.

traylenator
traylenator previously approved these changes Mar 22, 2021
View reviewed changes
@tmanninger
Copy link

If the OUTPUT-NAT chain is already defined, i get the a duplication declaration error.
The declaration should be optional.

nbarrientos
nbarrientos requested changes Mar 23, 2021
View reviewed changes
manifests/rules/docker_ce.pp Outdated Show resolved Hide resolved
manifests/rules/docker_ce.pp Outdated Show resolved Hide resolved
@luisfdez luisfdez force-pushed the dockerce branch 4 times, most recently from 7b3f937 to 9ee40e3 Compare March 23, 2021 10:50
@luisfdez
Copy link
Author

If the OUTPUT-NAT chain is already defined, i get the a duplication declaration error.
The declaration should be optional.

@tmanninger I have added a couple of class parameters to handle the creation of chains. Does it look good to you now? Thanks for taking a look :-)

@duritong duritong merged commit 18b211e into voxpupuli:master Mar 24, 2021
figless pushed a commit to figless/puppet-nftables that referenced this pull request Aug 25, 2021
@nbarrientos
Squashed 'code/' changes from 05c7f19..7b8e463
d48af6c 
cae7912 Merge pull request voxpupuli#86 from cernops/release_1_3_0
804b96e Prepare release 1.3.0
3f2f50c Merge pull request voxpupuli#85 from cernops/qemu
cd2a3cb Add rules for QEMU/libvirt guests
18b211e Merge pull request voxpupuli#80 from luisfdez/dockerce
1bf717d Add optional handling of chains
c86e270 Merge pull request voxpupuli#84 from traylenator/version
4d95ea8 Add fact section to README.md
9dca9bc Fix doc defaults
032387d Add nftables.version to structured fact.
b61ccb4 Fix rulename spec in spec
283e1c3 Fix syntax
c351549 Add newline & more tests
6be2adf Add Docker-CE default rules
7a77d75 Merge pull request voxpupuli#82 from cernops/ibarrien_activemq
771b325 Add rules for Apache ActiveMQ
502b9da Merge pull request voxpupuli#81 from cernops/emacs_readme
b1b6150 Add pointer to Yasnippets for some defined types
2fda87a Improve sections' formatting
812ca77 Release 1.2.1-rc0 (voxpupuli#77)

git-subtree-dir: code
git-subtree-split: 7b8e4635092853a892bd247a05e0025492262b6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak approved these changes

@nbarrientos nbarrientos nbarrientos approved these changes

@duritong duritong duritong approved these changes

@traylenator traylenator traylenator left review comments

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@luisfdez @nbarrientos @traylenator @tmanninger @duritong @bastelfreak