Create a puppet 4 branch to adopt improved hiera lookups #46

tpdownes · 2016-08-02T19:23:46Z

Eliminate need for hiera_merge_values because advanced lookup features are now built-in to Puppet 4 and more recent versions of hiera.

Additionally, resolve weirdness of namespace where sysctl is not a class but sysctl::base is a class, by making sysctl a class and moving defined resource to sysctl::configuration. The new way of using this class is to

include sysctl

and set, e.g.,

sysctl::values:
  net.ipv4.ip_forward:
    value: '0'

Because Puppet 4 is a breaking upgrade in many ways I do not feel uncomfortable making this a breaking upgrade. There are a number of issues open that seem to stem from the fact that sysctl is a defined resource but not a class.

2. Resolve weirdness of namepsace where sysctl is not a class but sysctl::base by making sysctl a class and moving defined resource to sysctl::configuration.

tpdownes · 2016-08-02T19:24:11Z

P.S. I have 2 more commits which fixes a number of other problems.

Use alternative /etc/sysctl.d directory in creating the files.
Only create files in /etc/sysctl.d that pass inspection by sysctl through use of the validate_cmd parameter to file. This has the impact that the setting actually precedes the file creation but not such a big deal.
Add parameter named variable to defined resource that defaults to $title. This allows the logging to take on vernacular-friendly names.
Management of /etc/sysctl.conf is confusing. Variable is set in 2 places if it exists in /etc/sysctl.conf prior to execution. Modify behavior to remove entries from /etc/sysctl.conf.
refreshonly precludes use of onlyif in exec resource. Eliminate refreshonly and rely on combination of notify and onlyif to prevent unnecessary executions.
Create /etc/sysctl.d file prior to enforcing the value. Prevents another unnecessary exec resource from running.

P.P.S. I intend for this to be a new branch, not to overwrite master. But I can't express that in a pull request.

2. Do better job of ensuring that bad configuration files are not created 3. Variable type should not rely on title alone, but should default to the title. This allows the rule in the logs to take on a clear name based on purpose while also allowing for the simple use of kernel variable.

leave the notify (so that it runs after sysctl.d file is created) but rely on the onlyif to prevent unnecessary executions 2. Modify behavior of /etc/sysctl.conf management to REMOVE entries because we are managing them under /etc/sysctl.d. Current behavior is confusing because it will add a variable in two locations if that variable was previously present in /etc/sysctl.conf. 3. ensure that the file under /etc/sysctl.d gets created prior to running the enforcer. The enforcer should really only be running when a user has manually run sysctl Bla

tpdownes · 2016-08-02T22:07:05Z

If you're willing to introduce a dependency on puppetlabs-stdlib, you could replace the line removal exec with a file_line.

…n in all modern Debian/RHEL/FreeBSD

c33s

@tpdownes thanks for your effort.

basic usage works for me on debian 8.5

net.ipv4.conf.all.rp_filter: { value: '1' }

but it there are two errors for advanced usage

test-net.ipv6.conf.default.autoconf: { variable: 'net.ipv6.conf.default.autoconf', value: '0', prefix: '40' }

see the two rewview comments and
also in sysctl.d-file.erb the <%= @variable %> should be <%= @title %>

c33s · 2016-09-22T12:38:49Z

manifests/configuration.pp

+
+  # If we have a prefix, then add the dash to it
+  if $prefix {
+    $_sysctl_d_file = "${prefix}-${variable}${suffix}"


in line 30: variable should be title

c33s · 2016-09-22T12:40:04Z

manifests/configuration.pp

+  if $prefix {
+    $_sysctl_d_file = "${prefix}-${variable}${suffix}"
+  } else {
+    $_sysctl_d_file = "${variable}${suffix}"


also here variable should be title

No, I don't think they should. variable is what I intend to be used, and it takes on title as its default value. And that is working just fine in your "basic usage" example. You don't quote the errors - what are they?

You should also be careful about using periods as the search key in a hiera lookup. Or rather, you shouldn't be careful about it, you shouldn't do it at all. e.g.

https://groups.google.com/forum/#!topic/puppet-users/cYOLQ9pE0Lw
https://docs.puppet.com/hiera/3.2/lookup_types.html

(periods are interpreted as subkey lookups within a hash)

I assume those two variables are part of a larger hash?

thanks for the tip with the periods, worked fine for me without quoting, but now i quoted everything just to don't get in trouble later.

concerning the variable & title stuff, i am quite sure that my fix is correct.

for the following hiera code:

'test-net.ipv6.conf.default.autoconf': { variable: 'net.ipv6.conf.default.autoconf', value: '0', prefix: '40' }

without the fix in the sysctl.d-file.erb file i get the following error:

==> default: Error: Execution of '/sbin/sysctl -p /etc/sysctl.d/40-net.ipv6.conf.default.autoconf.conf20160922-29874-1qu71uf' returned 255: sysctl: cannot stat /proc/sys/test-net/ipv6/conf/default/autoconf: No such file or directory ==> default: Error: /Stage[main]/Sysctl/Sysctl::Configuration[test-net.ipv6.conf.default.autoconf]/File[/etc/sysctl.d/40-net.ipv6.conf.default.autoconf.conf]/ensure: change from absent to present failed: Execution of '/sbin/sysctl -p /etc/sysctl.d/40-net.ipv6.conf.default.autoconf.conf20160922-29874-1qu71uf' returned 255: sysctl: cannot stat /proc/sys/test-net/ipv6/conf/default/autoconf: No such file or directory ==> default: Notice: /Stage[main]/Sysctl/Sysctl::Configuration[test-net.ipv6.conf.default.autoconf]/Exec[enforce-sysctl-value-net.ipv6.conf.default.autoconf]: Dependency File[/etc/sysctl.d/40-net.ipv6.conf.default.autoconf.conf] has failures: true ==> default: Warning: /Stage[main]/Sysctl/Sysctl::Configuration[test-net.ipv6.conf.default.autoconf]/Exec[enforce-sysctl-value-net.ipv6.conf.default.autoconf]: Skipping because of failed dependencies ==> default: Notice: /Stage[main]/Sysctl/Sysctl::Configuration[test-net.ipv6.conf.default.autoconf]/Exec[remove-net.ipv6.conf.default.autoconf-from-sysctl.conf]: Dependency File[/etc/sysctl.d/40-net.ipv6.conf.default.autoconf.conf] has failures: true ==> default: Warning: /Stage[main]/Sysctl/Sysctl::Configuration[test-net.ipv6.conf.default.autoconf]/Exec[remove-net.ipv6.conf.default.autoconf-from-sysctl.conf]: Skipping because of failed dependencies

with the fix in the sysctl.d-file.erb file but without the fix in the configuration.pp i get no error but the filename is wrong:

i get 40-net.ipv6.conf.default.autoconf.conf but would assume 40-test-net.ipv6.conf.default.autoconf.conf

after the fix i get no error and the correct filename 40-test-net.ipv6.conf.default.autoconf.conf.

you can try it out with my fork: https://github.com/c33s/puppet-sysctl/tree/puppet4

Ah. It's creating a file with the correct name, but the wrong contents. fe15776 on my fork should fix it.

And your behavior is wrong "test-net.ipv6.conf.default.autconf.conf" should never be created because it's not a kernel variable that exists. My fork prevents is intended to prevent creation of files with bad variables and that's exactly why it was failing.

Maybe English isn't your first language. You wrote

"also in sysctl.d-file.erb the <%= @variable %> should be <%= @title %>

when what I think you did is the right thing:

also in sysctl.d-file.erb the <%= @title %> should be <%= @variable %>

But the changes in configuration.pp are wrong. My fork now has the right combination of variables in the right places. Thanks for pointing this out!

Try something like

sysctl::values: disable-ipv4-forwarding: variable: 'net.ipv4.ip_forward' value: '0'

hm the new review system is nice but i don't get notifiaction emails..

i just installed your fork, which makes again a wrong config.

'ipv6_disable_net.ipv6.conf.eth0.disable_ipv6': { variable: 'net.ipv6.conf.eth0.disable_ipv6', value: '1', prefix: '90', comment: 'debian disable ipv6 https://wiki.debian.org/DebianIPv6' }

creates

90-net.ipv6.conf.eth0.disable_ipv6.conf

with my fork it creates the correct file, names:

90-ipv6_disable_net.ipv6.conf.eth0.disable_ipv6.conf

also tried your example with my fork:

#filename: disable-ipv4-forwarding.conf net.ipv4.ip_forward = 0

it makes no sense to me, if there is a puppet variable that is named $_sysctl_d_file (file!), it should handle the name of the file, which is set via the name/title parameter and should not be named after the variable.
can you try my fork please? what os & what puppet version are you using?

You have just documented the correct behavior.

c33s · 2016-09-22T14:33:01Z

@thias are you willing to accept this PR?
@tpdownes if @thias won't accept your PR would you add your fork to the puppetforge as own module?

Previous commits allow the user to define a sysctl resource title in the vernacular rather than compel them to set the title equal to the name of the kernel variables. Improves read-ability of logs.

variables that are multi-valued, so long as the user provides their value with a single space between the values.

1. Create a puppet 4 branch for development of improved hiera lookups

fa6e829

2. Resolve weirdness of namepsace where sysctl is not a class but sysctl::base by making sysctl a class and moving defined resource to sysctl::configuration.

tpdownes added 2 commits August 2, 2016 15:49

Point to correct location of sysctl binary on RHEL6-derived platforms

91cdc9e

tpdownes mentioned this pull request Aug 20, 2016

Not working in Debian 8 #45

Open

Make /sbin/sysctl the default path since it appears to be the locatio…

20c07f5

…n in all modern Debian/RHEL/FreeBSD

c33s suggested changes Sep 22, 2016

View reviewed changes

c33s added a commit to c33s/puppet-sysctl that referenced this pull request Sep 22, 2016

fix error discussed in thias#46

65df0a4

tpdownes and others added 3 commits September 22, 2016 09:59

Use proper variable in sysctl.conf.d template

fe15776

Previous commits allow the user to define a sysctl resource title in the vernacular rather than compel them to set the title equal to the name of the kernel variables. Improves read-ability of logs.

Improve sysctl enforcement so that it works for kernel

a944b26

variables that are multi-valued, so long as the user provides their value with a single space between the values.

Eliminate Unrecognized escape sequence warning in puppetserver logs

a6e61d4

tpdownes closed this Oct 28, 2016

tpdownes deleted the puppet4 branch October 28, 2016 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a puppet 4 branch to adopt improved hiera lookups #46

Create a puppet 4 branch to adopt improved hiera lookups #46

tpdownes commented Aug 2, 2016

tpdownes commented Aug 2, 2016 •

edited

Loading

tpdownes commented Aug 2, 2016

c33s left a comment

c33s Sep 22, 2016 •

edited

Loading

c33s Sep 22, 2016

tpdownes Sep 22, 2016

tpdownes Sep 22, 2016

c33s Sep 22, 2016

c33s Sep 22, 2016

tpdownes Sep 22, 2016

tpdownes Sep 22, 2016 •

edited

Loading

tpdownes Sep 22, 2016

c33s Sep 22, 2016

tpdownes Sep 22, 2016

c33s commented Sep 22, 2016

Create a puppet 4 branch to adopt improved hiera lookups #46

Create a puppet 4 branch to adopt improved hiera lookups #46

Conversation

tpdownes commented Aug 2, 2016

tpdownes commented Aug 2, 2016 • edited Loading

tpdownes commented Aug 2, 2016

c33s left a comment

Choose a reason for hiding this comment

c33s Sep 22, 2016 • 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

tpdownes Sep 22, 2016 • 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

c33s commented Sep 22, 2016

tpdownes commented Aug 2, 2016 •

edited

Loading

c33s Sep 22, 2016 •

edited

Loading

tpdownes Sep 22, 2016 •

edited

Loading