[REVIEW] [MODULES-3520] refactored types and providers and added purging

[crayfishx]

Description

This is rather a large PR that refactors a lot of stuff internally, though should keep the existing API to users intact. It addresses the main issue raised in MODULES-3520 regarding purging.

Purging

As described in the ticket, purging as it stands does not work well with the new dynamic file_path stuff to allow overriding of the conf file locations. To use regular instance() based purging (eg: with the resources resource) then you have no option other than to have hard coded path names in the providers. Rather than a trade-off between the two I've taken a similar approach used in other modules to use a the generate method of a core type (in this case splunk_config) to create resources as absent. Key things here that have changed are:

• Purging now works in the same consistent way for all types across server and forwarder
• File path are defaulted in params.pp and the purge options are added to the splunk_config type by the classes that inherit from it (init.pp and forwarder.pp)
• splunk_config sets the file paths on the providers based on what params.pp has defined
• splunk_config uses it's generate() function to purge unwanted resources
• Purging is done by comparing the namevars (see below) rather than just the resource refs

Namevars

Splunk types are a combination of setting and section, with a value. The current implementation is open to odd behaviour since the namevar is essentially discarded. This opens up scenarios such as the following.

splunkforwarder_output { 'foo':
  ensure => present,
  setting => 'xyz',
  section => 'bar',
  value   => 'tango',
}

splunkforwarder_output { 'bar':
  ensure => present,
  setting => 'xyz',
  section => 'bar',
  value   => 'delta',
}

The end result here is that Puppet ends up constantly changing the values because it doesn't see the resource declarations as conflicts.

Notice: /Stage[main]/Main/Splunkforwarder_output[foo]/value: value changed 'delta' to 'tango'
Notice: /Stage[main]/Main/Splunkforwarder_output[bar]/value: value changed 'tango' to 'delta'

Notice: /Stage[main]/Main/Splunkforwarder_output[foo]/value: value changed 'delta' to 'tango'
Notice: /Stage[main]/Main/Splunkforwarder_output[bar]/value: value changed 'tango' to 'delta'

The solution is to make all the types a composite namevar of setting and section. The name parameter has been left in because it Puppet::Type.type(:foo).new (used for purging) seems not to work if there is not a name parameter.

Now the above code fails as expected....

Error: Cannot alias Splunkforwarder_output[bar] to [nil, "bar", "xyz"] at /home/vagrant/test.pp:33; resource ["Splunkforwarder_output", nil, "bar", "xyz"] already declared at /home/vagrant/test.pp:26

Title patterns

Following on from making the resource type support composite namevars, I've also added a title_pattern that allows for some shorter resource declarations. This could possibly be viewed as an API change, but I can't think of many circumstances where existing declarations would break. (needs decision). The new title patterns match /^([^\/]*)$/ as "section" or /^(.*)\/(.*)$/ as "section/setting" meaning the following three types of declarations can be used (and do the same thing)

splunkforwarder_output { 'useless title':
  ensure => present,
  setting => 'xyz',
  section => 'bar',
  value   => 'tango',
}

splunkforwarder_output { 'bar':
  ensure => present,
  setting => 'xyz',
  value   => 'tango',
}

splunkforwarder_output { 'bar/xyz':
  ensure => present,
  value   => 'tango',
}

Code clean up

Finally, all of the types and providers are identical and do exactly the same thing, the only difference is which file they do it in. This has resulted in alot of duplicated code both in the types and providers. I've added some boilerplating to this

• Types use a kind of mixin pattern provided by PuppetX::Puppetlabs::Splunk::Type.clone so the parameters, properties and methods of the types are centrally managed - but you can still add additional ones, or documentation, to individual types.
• Providers have been simplified a bit, they are only responsible for defining what their filename is, and they inherit from Puppet::Type.type(:ini_file).provider(:splunk) where generic stuff around file_path is set (by splunk_config). This then inturn inherits from the :ini_file provider in puppetlabs-inifile
• 250 fewer lines of code!... 37 files changed, 439 insertions(+), 689 deletions(-)

[bastelfreak]

@crayfishx hai :)
as promised... now in voxpupuli. Can we merge this as is? I would like to do a modulesync after this PR is done. Than do a fresh release. AFAIK your PR only requires a new minor release? Can you already add a description into the README.md please?

[crayfishx]

@bastelfreak that was quick!.... hold off on merging - I think the requires should be changed to relative requires otherwise this will cause ruby load errors under some circumstances

[crayfishx]

Anytime we do this, we should be doing require File.join(File.dirname(__FILE__),.....etc.etc.

[bastelfreak]

@crayfishx hi, any progress here?

[bastelfreak]

ups

[nickperry]

Thanks so much for this work. I wish I'd had the foresight to do it like this in the first place.

[nickperry]

On my fork I still have nearly all of the file paths hard coded. I have a provider and type for managing splunk-launch.conf. This actually sits in /opt/splunk/etc, not /opt/splunk/etc/system/local. I'm now wondering if confdir in splunk_config should actually be set to /opt/splunk/etc and the providers include system/local in the join where appropriate, thus letting us manage files like splunk-launch which sit outside of system/local.

confdir is relative to forwarder_dir and server_dir. It isn't used anywhere else in the module (although it's referenced in the forwarder manifest). I think it is safe to change.

I think this would be a worthwhile change so we have the ability to manage conf files that sit above system/local in a consistent, elegant way. As well as etc/splunk-launch.conf it could for example be used to manage etc/apps/splunk_http/local/inputs.conf to manage HTTP event collector tokens and other files in the future.

[crayfishx]

8483add should stop potential problems with requiring files from the ruby load path (eg: require 'foo/bar) and reference them explicitly or relative to __FILE__ instead.

@bastelfreak FYI

[nickperry]

Please consider #49 (comment) as this PR would be a good place to make that change.

Also, could you add a splunkforwarder_limits type please?

[crayfishx]

@nickperry I'll take a look at implementing that ...

[crayfishx]

@nickperry is this what you meant? ea37d3d

[nickperry]

@crayfishx Yes, perfect, thanks

[crayfishx]

@bastelfreak i have no more pending changes........

[crayfishx]

@nickperry one thing I thought of last night - this PR adds a title pattern to be able to define splunkforwarder_something { 'section/setting': } - this may be flawed because sections could contain forward slashes.... eg:

[tcp://127.0.0.1:19500]
...

So that would cause the section to be tcp: and the setting to be /127.0.0.1:19500 - it would be solvable by not using title pattern approach for any section that contains slashes, and I actually documented it like that in the README, but is there a better approach here? Space instead of slash?

[crayfishx]

I've added another scenario to title_patterns to match '//' - if the string contains '//' it's assumed to be just the section and won't get split.

irb(main):002:0> ['foo', 'foo://bar', 'foo/bar'].each do |example|
irb(main):003:1*   type=Puppet::Type.type(:splunkforwarder_input).new(:title => example)
irb(main):004:1>   puts "#{example} -> #{type[:section]}"
irb(main):005:1> end

foo -> foo
foo://bar -> foo://bar
foo/bar -> foo

[nickperry]

@crayfishx That's cool. I've been using my fork of the module with

def section
%r{^(.)/}.match(resource[:name])[1]
end
def setting
%r{^./(.*?)$}.match(resource[:name])[1]
end
def separator
'='
end

in a large scale, complex environment, using the module from profiles with no problems for more than 6 months.