some fix #23

abraham1901 · 2020-12-09T18:05:13Z

fix environment path for exec
add params
blacklist
blacklist_defaults
tcrules
tcrules_defaults

* add params blacklist blacklist_defaults tcrules tcrules_defaults

duritong · 2020-12-09T21:48:02Z

manifests/base.pp

+ '/usr/bin',
+ '/sbin',
+ '/bin',
+ ]


You should not do that: https://www.puppetcookbook.com/posts/set-global-exec-path.html

Yes, of course it is possible. But many modules set a default value for a path because it adds autonomy and versatility to the module.

For example:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/service.pp

Imho it’s not bad practice.

If you want autonomy then why not the full path?

But if you clutter all over your modules the same piece of code, something smells.

If you want autonomy then why not the full path?

The full path will not allow the command to be executed in different environments.
For example:
Ubuntu 20.04:
/usr/sbin/shorewall
Ubuntu 18.04:
/sbin/shorewall

But if you clutter all over your modules the same piece of code, something smells.

It’s true. I do not fully understand why the function exec does not set the default path parameter.

duritong · 2020-12-09T21:48:36Z

manifests/base.pp

@@ -1,8 +1,10 @@
 # base things for shorewall
 class shorewall::base {

- package { 'shorewall':
- ensure => $shorewall::ensure_version,
+ if ! defined(Package['shorewall']) {


Why? What if someone defined 'package{'shorewall': ensure => absent }` somewhere else?

It’s not easy question =)

I’m have a problem deploy new node because first time fact shorewall_major_version is empty and define shorewall::rule_section generate wrong config. As a result failed start service shorewall and failed many dependences.
I solved this by breaking the installation into two steps.

Install packages shorewall

Install class shorewall
Same as:
if ! defined(Package['shorewall']) {
package { [shorewall, shorewall6]: }
}

if $shorewall_major_version != '' {
class { 'shorewall': }
}

But if do it, puppet agent result duplicate declaration Package['shorewall']
It’s crutch, but it’s work.

Ah so you rung things twice?

Well you could do the following:

if $shorewall_major_version != '' { class { 'shorewall': } } else { package { [shorewall, shorewall6]: } }

Yes, you are right, this is a more correct solution. Rolled back the change. Thanks!

duritong · 2020-12-09T21:49:10Z

manifests/base.pp

 require => Package['shorewall6'],
 } ~> exec{'shorewall6_try':
- command => 'shorewall6 try /etc/shorewall6/puppet',
+ command => 'shorewall6 try /etc/shorewall6',


why? you will not test anymore the right thing

shorewall try search shorewall.conf. In manifest base you declare:
file { '/etc/shorewall6/shorewall6.conf':

shorewall6 try /etc/shorewall6/puppet
return error:
ERROR: /etc/shorewall6/puppet/shorewall6.conf does not exist!
shorewall6 try /etc/shorewall6 work fine for me.

Which version is this?

I get:

# shorewall6 try /etc/shorewall6/ Compiling using Shorewall 5.1.10.2... Processing /etc/shorewall6/params ... Processing /etc/shorewall6/shorewall6.conf... Loading Modules... Compiling /etc/shorewall6/zones... ERROR: No IP zones defined /etc/shorewall6/zones (EOF)

vs.

# shorewall6 try /etc/shorewall6/puppet/ Compiling using Shorewall 5.1.10.2... Processing /etc/shorewall6/puppet/params ... Processing /etc/shorewall6/shorewall6.conf... Loading Modules... Compiling /etc/shorewall6/puppet/zones... Compiling /etc/shorewall6/puppet/interfaces... Determining Hosts in Zones... Locating Action Files... Compiling /etc/shorewall6/puppet/policy... Adding Anti-smurf Rules Compiling TCP Flags filtering... Compiling MAC Filtration -- Phase 1... Compiling /etc/shorewall6/puppet/rules... Compiling /usr/share/shorewall/action.dropNotSyn for chain dropNotSyn... Compiling /etc/shorewall6/conntrack... Compiling MAC Filtration -- Phase 2... Applying Policies... Compiling /usr/share/shorewall/action.AllowICMPs for chain AllowICMPs... Compiling /usr/share/shorewall/action.Broadcast for chain Broadcast... Compiling /usr/share/shorewall/action.Multicast for chain Multicast... Generating Rule Matrix... Optimizing Ruleset... Creating ip6tables-restore input... Shorewall configuration compiled to /var/lib/shorewall6/.reload Currently-running Configuration Saved to /var/lib/shorewall6/.try Reloading... Reloading Shorewall6.... Initializing... Processing /etc/shorewall6/init ... Setting up Proxy NDP... Preparing ip6tables-restore input... Running /sbin/ip6tables-restore --wait 60... Processing /etc/shorewall6/start ... Processing /etc/shorewall6/started ... done.

Oh..
My solution don’t work on major version 4. I’m fixed this in new commit, thanks.

duritong · 2020-12-09T21:52:56Z

templates/blrules.erb

@@ -7,9 +7,13 @@
 # information.
 #
 ###############################################################################
+<% if @whitelists -%>


We could just default them to empty, then we would not need that.

Sorry, I’m not understand how it’s work. If whitelist not empty? Right now I am using the whitelist as a list of the protected perimeter networks. If the list is empty, then we do nothing.

If we make sure that the whitelist parameter must always be an array, then we do not need the wrap with the if that checks if whitelists is set.

If the array is empty, we won't set any whitelist anyways.

This can be achieved with Puppet Data types.

You are right! Reverted.

duritong · 2020-12-09T21:55:07Z

Thank you! There are a few things, that I will never merge, since I think they are wrong (some conceptually, some changing what the module is doing now.).

But if you break things up, we could individually work on the contributions.

abraham1901 · 2020-12-10T08:38:37Z

Hi!
Thanks for the module and the quick feedback.
I will try to answer your questions.

abraham1901 · 2020-12-10T08:41:30Z

I'm used you module on ubuntu 14.04/16.04/18.04/20.04.
Last testing on Ubuntu 20.04

* fix compatibility with major version 4 * fix some puppet lint

* fix environment path for exec

97eb5ca

* add params blacklist blacklist_defaults tcrules tcrules_defaults

duritong requested changes Dec 9, 2020

View reviewed changes

* revert some changes

79e72b2

* fix compatibility with major version 4 * fix some puppet lint

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

some fix #23

some fix #23

abraham1901 commented Dec 9, 2020

duritong Dec 9, 2020

abraham1901 Dec 10, 2020

duritong Dec 12, 2020

abraham1901 Dec 14, 2020 •

edited

Loading

duritong Dec 9, 2020

abraham1901 Dec 10, 2020

duritong Dec 12, 2020

abraham1901 Dec 14, 2020

duritong Dec 9, 2020

abraham1901 Dec 10, 2020

duritong Dec 12, 2020

abraham1901 Dec 14, 2020

duritong Dec 9, 2020

abraham1901 Dec 10, 2020

duritong Dec 12, 2020

abraham1901 Dec 14, 2020

duritong commented Dec 9, 2020

abraham1901 commented Dec 10, 2020

abraham1901 commented Dec 10, 2020

some fix #23

Are you sure you want to change the base?

some fix #23

Conversation

abraham1901 commented Dec 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abraham1901 Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

duritong commented Dec 9, 2020

abraham1901 commented Dec 10, 2020

abraham1901 commented Dec 10, 2020

abraham1901 Dec 14, 2020 •

edited

Loading