Hiera lookup

[PierreR]

I have spent a couple of hours to migrate our config to use hiera as indicated by https://github.com/jfryman/puppet-nginx/blob/master/docs/hiera.md

While I am testing this, I see no param change when I use nginx::config::parameterx.

Here is an example:

nginx::config::client_max_body_size: '100m'
nginx::config::worker_processes: 2

using:

class { 'nginx':
}

Of course It works when I use:

class { 'nginx':
  worker_processes => 2
}

Note also that if I use in hiera:

nginx::worker_processes: 2

then it works ...

Am I missing something ?

I am using the latest tag 0.2.1 and puppet 3.7.3

Thanks for your help.

[jharlow1]

Not sure if this is quite hijacking, but Is there any existing discussion out there as to the motivations behind this change other than the brief snippet in hiera.md ? I'm not really understanding why this methodology is better and what problem it's solving. I too have recently upgraded to this and have some concerns with the inability to use the module without having everything in Hiera (We do use hiera for some data, but there are some things that we prefer to keep in our profile modules for readability)

As such, in addition to the above, the recommended method for dealing with not wanting to do all configuration via hiera doesn't work consistently due to a parse-order dependency (if the class nginx::config instantiation inside init.pp happens prior to our instantiation of class nginx::config in our profile(s), then you get a resource already declared error, and putting the same logic around our instantiation means that our param overrides get thrown away).

[jfryman]

@jharlow1 The change that is being introduced is not to force everyone to use Hiera... rather quite the opposite. The module was built many 🌔s ago even before Hiera was a thing, and so as it came out and the community started using it... there is a mismatch between how the module is used with Hiera and without.

Likewise, with Puppet 3.x, Hiera data bindings were introduced that allows a lookup to any appropriately named parameter. Given the existing mismatch, I have chosen to clean up the module so the behavior with Hiera and declaring in-code follows the same conventions.

Long term, init.pp is mostly going away. It will be up to the user to declare nginx::config on their own, very similar to how other modules approach this problem. If you're running into issues today specifically, let's hash them out. The goal is to be minimally disruptive while we make big changes to the module internals.

[jfryman]

@PierreR You will need to define nginx::config in your manifests in order to use Hiera in that manner. The class is only declared today in init.pp and only if you haven't set it up somewhere else. So, wherever you are declaring Class[nginx], also include nginx::config.

It's a little clumsy today, but it's being cleaned up (see above).

Please let me know if this resolves things for you.

[jharlow1]

I guess I'm just trying to understand the end state you're working towards a little better. I'm able to work around the existing state of the module by moving some more things into Hiera and out of the class instantiation in the profiles I'm using....wasn't a big deal.

But, once this is fully complete, if we want the full install/config/service functionality, we'll basically have to explicitly include all 3 of those ? Are all the other bits of init.pp then also moving into the subclasses ? (i.e. the create_resources bits that gather all the other config pieces), or will we also have to do those manually ?

Just worried that (what I assume is) the most common use case for the module is going to now basically have to incorporate a more elaborate "driver" profile in front of the actual module in order to use it in a basic scenario (Rather than just doing a simple instantiation of the nginx class with a few params). And, was curious why not just keep the single point of entry but with flags on whether or not to manage the package and/or service.

Sorry again to hijack this issue, but wasn't sure where else to have this discussion. Feel free to point me at a forum or user group.

[jfryman]

@jharlow1 Yes, all of the bits are being moved out of init.pp. What will be left in the end will be more akin to the role/profile pattern, but adapted to an individual module.

Essentially, each of the three main modules (config.pp, package.pp, and service.pp) are going to be completely and individually declarable classes. The aim here is to isolate the various components such that the module no longer makes a gross assumption about its operating environment... that Puppet should be managing the entire state of the service. This is particularly true in container based environments, and Puppet is part of the build pipeline for assembling a container.... I may be using Puppet to install and create a brand new container for nginx (package + config), or to configure an already existing container (config only).

Surely this exists today with the attributes to toggle on and off the package and service class, but it creates a spaghetti class (init.pp) that suddenly needs to remain in sync with the actual classes that are doing the actual work.

Now, by this matter, there will be a set of default profiles that will be included (for example: nginx::profile::full_install, nginx::profile::container, that will assemble the various installation profiles, and potentially some neat mixing of sane default adjustments (`nginx::profile::high_request_config').

So, yes... you'll have to use sort of a driver, but I really do not see it being so much more complex than it is today. The end state would look like:

# These should really be in some site profile too. :)
include nginx::profile::full_install

# If config is needed...
class { 'nginx::config':
   ... # all them cool configs!
}

All the magic will happen in the config class, as that really is the core of what we're expecting Puppet to do: configure the app. So, all the Hiera calls to declare nginx resources will be moved there. As a module, these profiles should be configured so that there exist sane defaults, and someone can get started ASAP without having to do much more than include a profile.

This is all in effort to make this code more adapt to model the nginx config, trying to get Puppet out of the way a little bit so others can contribute, and make it easy to consume by our end users (of which, there are quite a few). So, the change is slow, and deliberate. For two reasons: 1) it gives time for changes to bake for those who want to run with the most up-to-date, and 2) this is totally a volunteer effort and time is a premium.

Certainly appreciate any and all comments, and also love help when you can!

Anyway, I hope that helps explain.

[PierreR]

@jfryman I have tried your suggestion:

  class { 'nginx': }
  include nginx::config

  nginx::resource::upstream { $upstream:
    members => ['localhost:7990', ],
  }

  nginx::resource::vhost { $vhosts:
    proxy   => "http://${upstream}",
  }

But I get this complain from language-puppet :

Can't include class nginx::config twice when using the resource-like syntax (first occurence at # "./modules/nginx/manifests/init.pp" (line 217, column 13)) at # "./modules/application/manifests/profile/stash.pp" (line 10, column 3)

What wrong with using nginx::worker_processes: 2 instead of nginx::config::worker_processes: 2

Thanks for your help.

[Hufschmidt]

@PierreR Was having the same issue. The problem is that you need to make sure that "your" nginx::config class is proccessed before nginx is evaluated/compiled, because it includes its own nginx::config class (from parameters given to the nginx-class) IFF its non is defined.

The problem is you cannot do Class['nginx::config'] -> Class['nginx'] because this would lead to a cyclic dependency, because inside init.pp:

    Class['::nginx::package'] -> Class['::nginx::config'] ~> Class['::nginx::service']
    [...]
    anchor{ 'nginx::begin':
        before => Class['::nginx::package'],
        notify => Class['::nginx::service'],
      }

Luckily this also "contains" (hihi) the solution for the ordering problem:

      class { 'nginx' :
        manage_repo   => false,
      }

      Anchor['nginx::begin']
      ->
      class { 'nginx::config' :
        confd_purge   => true,
        vhost_purge   => true,
      }

This SHOULD work, at least this seems to have fixed the problem for me.
It guarantees the correct order on "instantiation" if my head isn't totally messed now.

[PierreR]

I think #655 is related.

Does it need to be that messy ? I am currently using hiera (same as #655) and the notice message is rather off.

On the other hand, I don't want to start messing with Anchor in client (user not lib) code as suggested by @Hufschmidt . I don't really understand why setting params needs to be so convoluted ;-)

[PierreR]

@Hufschmidt I have tried your suggested trick but I have got (with puppet apply)

Error: Duplicate declaration: Class[Nginx::Config] is already declared in file /puppet/modules/nginx/manifests/init.pp:287; cannot redeclare at /puppet/modules/application/manifests/profile/localproxy.pp:19 on node a5e9ba79bf86.la

[3flex]

@PierreR unfortunately the defined function used in init.pp to determine whether the nginx::config class has been declared yet is parse order dependent.

@jfryman can you weigh in here? Looks like people either MUST use hiera, or will have to find another way to make their personal class declaration get parsed before this module's init.pp. Or the module will need to be adjusted so the solution is more robust.

[Hufschmidt]

@PierreR Yes my bad, I'm actually using

  Anchor['nginx::begin']
  ->
  class { 'nginx::config' :
    confd_purge   => true,
    vhost_purge   => true,
  }

  class { 'nginx' :
    manage_repo   => false,
  }

myself...

I'll make a PR for hiera.md...

[jfryman]

The Anchor is certainly non-ideal, and not the behavior I intended. For now, the doc update that @Hufschmidt made (thanks!) is merged to have something, but there needs to be a better solution.

[PierreR]

As a note following #655 I have checked again with the latest source (puppet 3.8) and to my surprise nginx::config::worker_processes: 2 in hiera does work (so it seems) without using any extra nginx::config declaration . This is quite ideal for me as I do use hiera. I will check if this behavior is deterministic.

[wyardley]

It seems like the initial issue is resolved now, and the (ugly) anchor pattern is documented, so I'm going to close this one, though lots of interesting discussion / background in here.