Resource default statements in module about puppet-unbound HOT 17 CLOSED

The File[$_owned_dirs line is a resource amendment, which behaves differently from a resource default.

https://puppet.com/docs/puppet/5.5/lang_resources_advanced.html#amending-attributes-with-a-resource-reference

The important bit from here is:

it cannot override already-specified attributes.

Since @wilful 's resource default is early in the Puppet code, it is parsed before this unbound code. So the file resources have a specified attribute. Then comes along the resource amendment from the unbound module, and the error gets thrown.

from puppet-unbound.

that answers my usual question of, "can you provide us with a fix?" 😅

from puppet-unbound.

This doesn't seem like a problem to be solved by this module to me. Setting global file defaults seems like an anti-pattern, and doesn't Puppet already set exactly those defaults on File resources?

from puppet-unbound.

i have added a #260 which solves this issue using a different hack. Im not sure i really like it but im not sure i have really liked any of the iterations this module has used to manage the piddir's (and im likely responsible for at least half of them) .

i think the main issue is trying to manage permissions on the dirname($pidfile). perhaps we just stop making that commitment, doing so would allow us to drop most of the directory creation preamble

from puppet-unbound.

that's a pattern only recommended to be used in your site.pp, so we should remove it from the module

from puppet-unbound.

Thank for quick answer. I will wait for a commit that will fix this "problem"

from puppet-unbound.

So, i understand why this code exists:

  $_base_dirs = [$confdir, $conf_d, $keys_d, $runtime_dir]
  $_piddir = if $pidfile { dirname($pidfile) } else { undef }
  if $_piddir and !($_piddir in ['/run', '/var/run']) {
    $dirs = unique($_base_dirs + [$_piddir])
    $_owned_dirs = unique([$runtime_dir, $_piddir])
  } else {
    $dirs = unique($_base_dirs)
    $_owned_dirs = [$runtime_dir]
  }
  ensure_resource('file', $dirs, { ensure => directory })
  File[$_owned_dirs] {
    owner => $owner,
  }

but i don't understand why it conflicts with your global default, it should merge nicely, given that this is highly specific.

from puppet-unbound.

@wilful which version of puppet are you on?

from puppet-unbound.

that's a pattern only recommended to be used in your site.pp, so we should remove it from the module

That's not the pattern we're using. We're using the equally terrible https://puppet.com/docs/puppet/5.5/lang_resources_advanced.html#amending-attributes-with-a-resource-reference

from puppet-unbound.

The File[$_owned_dirs line is a resource amendment, which behaves differently from a resource default.

https://puppet.com/docs/puppet/5.5/lang_resources_advanced.html#amending-attributes-with-a-resource-reference

The important bit from here is:

it cannot override already-specified attributes.

Since @wilful 's resource default is early in the Puppet code, it is parsed before this unbound code. So the file resources have a specified attribute. Then comes along the resource amendment from the unbound module, and the error gets thrown.

We could unset any previously set resource default, with

File {
  owner => undef,
}

I think the scope of that should be fairly limited.

from puppet-unbound.

perhaps we can find a way to ensure those directories exist, with the correct owner, without having to dip into either of those patterns?

from puppet-unbound.

I think declaring generic options (default statements) in a module is worse pattern. I need to guarantee the default state of files and not depend on Puppet or modules. Thx.

from puppet-unbound.

I need to guarantee the default state of files and not depend on Puppet or modules.

that's not what

File {
  mode  => '0644',
  owner => 'root',
  group => 'root',
}

does. it can't do that.

Puppet can only guarantee that files created by puppet will have those defaults.
But those things you set are already puppet defaults!
Puppet cannot act as a replace for umask.

and puppet modules that guarantee that all files have a specific set of permissions do so with an exec that does a find / chown / chmod

from puppet-unbound.

They're the defaults if the file resource is creating a new file. There are plenty of cases where file resources are used to amend individual file properties (eg. perhaps just updating the mode) of an existing file. If you insist on putting in a resource default like you have, I'd expect lots of things to break.

from puppet-unbound.

Ah good point @alexjfisher about just the new files being created with those defaults, I forgot about that. What is the end goal with that default file statement anyway? The OP says for safety, but I don't know what that means.

from puppet-unbound.

Its been a while since I've looked at that part of the code, but are we managing the pidfile because some package manager doesn't create the file correctly? Do we still need to manage it at all?

from puppet-unbound.

Its been a while since I've looked at that part of the code, but are we managing the pidfile because some package manager doesn't create the file correctly? Do we still need to manage it at all?

Its also been a while sinece i looked at this however i just took a quick look and i can see that the defaults have the following configurations

data/common.yaml:unbound::pidfile: '/var/run/unbound/unbound.pid'
data/common.yaml:unbound::confdir: '/etc/unbound'
data/common.yaml:unbound::runtime_dir: "%{hiera('unbound::confdir')}"

In this case we need to create basename($pidfile) as we don't manage '/var/run/unbound' explicitly anywhere.

I also see the Debian configuration is set to

data/os/Debian.yaml:unbound::pidfile: '/run/unbound.pid'
data/os/Debian.yaml:unbound::runtime_dir: '/var/lib/unbound'

In this case we explicitly don't want to change the ownership of /run as that should be owned by root

In freebsd and solaris the pidfile is in the run dir so basedir(pidfile) == $runtime_dir

data/os/FreeBSD.yaml:unbound::confdir: '/usr/local/etc/unbound'
data/os/FreeBSD.yaml:unbound::pidfile: '/usr/local/etc/unbound/unbound.pid'

OpenBSD has a config simlar to debian (in this regard) so i think that would only leave Suse, Darwin, Arch, NetBSD (which should probably use the freebsd config) and AIX. That all have the default or similar configuration and requires us to manage ``basename($pidfile)`

from puppet-unbound.