willdurand/puppet-composer

fix issues with duplicated declarations due to ensure_packages()

Ma27 opened this issue · 4 comments

Ma27 commented

Description

About a year ago I started maintaining this and willdurand/nodejs. Since then I've seen several bug reports because of duplicated declarations because of wrongly used package declarations in puppet in our or in other module's code.

Actual issue.

The issue can be seen when having a simple look at the docs: ensure_resource() docs. The ensure_* API only avoids crashes due to duplicated resources if the resource definition passed to the ensure_* API contains the same hash as the already declared resource if declared. So whenever someone uses ensure => installed for instance and another one uses ensure => present (which is the default of the ensure_packages() function) there'll be a crash (we started using ensure => installed as the docs do that as well).

However this causes a lot of issues for other users and any change in a minor or even a patch release could be a BC break because of this although there's no real API change.

Steps to reproduce

# class1.pp
class class1 {
  ensure_packages(['wget'])
}
# class2.pp
class class2 {
  ensure_packages(['wget'], {
    ensure => installed,
  })
}

Execute both of them and you'll get a duplicate resource failure during the catalog compilation:

Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[wget] is already declared; cannot redeclare at /tmp/vagrant-puppet/environments/dev/manifests/site.pp:9:3 on node localhost.sbde-40936.btopenzone.com

Expected behavior

We need an approach how to avoid such declarations without receiving a bug report all two months.

Notes to this topic

I think that this approach contains several issues because of a simple issue: many puppet modules require extra-packages that they aren't actually responsible for (in this case wget although this module installs composer, the nodejs module needs a gcc compiler and tar and wget as well), so it's quite likely that such collisions might happen.

Related issues

#43, #41, willdurand/puppet-nodejs#168, willdurand/puppet-nodejs#39

Possible solutions

do the if !defined(Package['foobar']) way

I reverted this in willdurand/nodejs already because I'm not a big fan of these bloated statements that messed up the code in the current oldstable 1.x versions of that module.

revert the changes

I think that this is the best approach (also suggested in several comments last week) although the docs propose the use of ensure => installed.
However I think that this just fixes the symptom.

Imagine the following case:

class class1 {
  if !defined(Package['wget']) {
    package { 'wget':
      ensure => installed,
    }
  }
}
class class2 {
  ensure_packages(['wget'])
}

class { '::class1': } ->
class { '::class2': }

If a module uses the ensure => installed approach and our code gets compiled after theirs (as shown in the example) these guys would have a problem as well:

Error: Evaluation Error: Error while evaluating a Function Call, Duplicate declaration: Package[wget] is already declared in file /tmp/vagrant-puppet/environments/dev/manifests/site.pp:6; cannot redeclare at /tmp/vagrant-puppet/environments/dev/manifests/site.pp:12:3 on node localhost.sbde-40936.btopenzone.com

Therefore I suggest to revert these changes regarding ensure => installed in both modules (as mentioned in some comments), but add another parameter to the init.pp class for these cases which avoids execution of these module setup if something still causes crashes.

class { '::composer':
  # all the other settings
  build_deps => false,
}

This would skip the entire package setup if there are any further problems.
Same with willdurand/nodejs:

class { '::nodejs':
  # all the other settings
  build_deps => false,
}

So reverting the changes should (hopefully) resolve most of the issues and in the remaining, rare cases you could do all the package setup on your own and tell the module to skip that.

Ma27 commented

/cc @willdurand @den-is @cdoublev @s12v @tdm4 you submitted bug reports or commented on them, so this might be interesting for you as well/

  • 👍1
Ma27 commented

ok I closed it here since I fixed the actual bug in puppet-composer.
On the weekend I'll release a 1.9.6 for willdurand/nodejs and add the fix to the next alpha of willdurand/nodejs@2.0, then each of the modules maintained by me has the needed fix.

Ma27 commented

ok, fixed on 1.9 and 2.0-alpha at puppet-nodejs for now...