Resource default statements in module

[wilful]

How to reproduce

In order to ensure safety, me set the resource in the base class

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

What are you seeing

Me get conflict with

  File[$_owned_dirs] {
    owner => $owner,
  }

Output log

Error: Could not retrieve catalog from remote server: Error 500 on SERVER: Server Error: Parameter 'owner' is already set on File[/var/lib/unbound] by #<Puppet::Resource::Type:0x1680ee80> at (file: ); cannot redefine (file: ) on node

Any additional information you'd like to impart

Please tell me how we can resolve this conflict? Thx

[igalic]

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

[wilful]

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

[igalic]

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

[igalic]

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.

[igalic]

@wilful which version of puppet are you on?

[natemccurdy]

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.

[alexjfisher]

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

[alexjfisher]

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.

[igalic]

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

[zachfi]

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?

[wilful]

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.

[igalic]

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

[alexjfisher]

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.

[zachfi]

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.

[b4ldr]

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

[zachfi]

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?

[b4ldr]

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)