WIP: Fix issues 230, 232, 235, and 249 #250

nick-markowski · 2019-07-31T20:02:35Z

Moved defaults to module data, and removed the params class
Privatized all splunk::enterprise:: and splunk::forwarder:: install, config, and service classes
Added a $release param, which replaces the $version param
- For ensurable package_providers, the release is used as the Splunk package ensure, if specified
- The release no longer defaults to a specific version and build, instead, the Splunk package resource defaults ensure to 'installed'
- Added a Splunk::Release type
Added a service_ensure param, per Add a $service_ensure param to splunk::enterprise and splunk::forwarder #249
Modified splunk*_version facts to be part of splunkforwarder and splunkenterprise fact hashes
Removed init.pp, which only served to confuse
$[enterprise,forwarder]_package_src and $package_source params renamed to $managed_package_source and $unmanaged_package_source, for clarity
Fixed: enterprise and forwarder password classes cross-referenced params

Fixed #230
Fixed #232
Fixed #235
Fixed #249

* Moved defaults to module data, and removed the params class * Privatized all `splunk::enterprise::` and `splunk::forwarder::` install, config, and service classes * Added a `$release` param, which replaces the `$version` param * For ensurable package_providers, the release is used as the Splunk package ensure, if specified * The release no longer defaults to a specific version and build, instead, the Splunk package resource defaults ensure to 'installed' * Added a Splunk::Release type * Added a service_ensure param, per voxpupuli#249 * Modified splunk*_version facts to be part of splunkforwarder and splunkenterprise fact hashes * Removed init.pp, which only served to confuse * `$[enterprise,forwarder]_package_src` and `$package_source` params renamed to `$managed_package_source` and `$unmanaged_package_source`, for clarity * Fixed: enterprise and forwarder password classes cross-referenced params Fixed voxpupuli#230 Fixed voxpupuli#232 Fixed voxpupuli#235 Fixed voxpupuli#249

nick-markowski · 2019-07-31T20:07:28Z

The bulk of the changes are done, but here are a few notes about the initial commit:

This commit finalizes what the original refactor should have been. The module much more closely follows the module best practices, and the refactor was necessary to fix a number of the issues reported by users.
Still need to expand upon the README and update REFERENCES.md.
I've been experiencing an issue with splunk_config redefining types and throwing errors during puppet-applys, for instance:

  Info: Redefining splunk_distsearch in Puppet::Type
  Error: Could not autoload puppet/type/splunk_config: Attempt to redefine entity 'http://puppet.com/2016.1/runtime/type/splunk_distsearch'. Originally set at file:/etc/puppetlabs/code/modules/splunk/lib/puppet/type/splunk_distsearch.rb?line=3.
  Error: Evaluation Error: Error while evaluating a Resource Statement, Could not autoload puppet/type/splunk_config: Attempt to redefine entity 'http://puppet.com/2016.1/runtime/type/splunk_distsearch'. Originally set at file:/etc/puppetlabs/code/modules/splunk/lib/puppet/type/splunk_distsearch.rb?line=3. (file: /etc/puppetlabs/code/modules/splunk/manifests/enterprise.pp, line: 311, column: 3) on node centos7-64-1.home

bastelfreak · 2019-08-06T18:34:49Z

manifests/enterprise/password/manage.pp

-  String[1] $secret                          = $splunk::params::secret,
-  String[1] $splunk_user                     = $splunk::params::splunk_user,
-  String[1] $service                         = $splunk::params::enterprise_service,
+  Boolean $manage_password                   = lookup($splunk::enterprise::manage_password),


Why did you add all the lookup() calls?

After thinking about it, that was silly, and I'm going to remove them

ody

Overall looks really good. Nice work!

ody · 2019-08-30T20:19:42Z

data/common.yaml

+splunk::enterprise::reset_seeded_password: false
+splunk::enterprise::secret: 'hhy9DOGqli4.aZWCuGvz8stcqT2/OSJUZuyWHKc4wnJtQ6IZu2bfjeElgYmGHN9RWIT3zs5hRJcX1wGerpMNObWhFue78jZMALs3c3Mzc6CzM98/yGYdfcvWMo1HRdKn82LVeBJI5dNznlZWfzg6xdywWbeUVQZcOZtODi10hdxSJ4I3wmCv0nmkSWMVOEKHxti6QLgjfuj/MOoh8.2pM0/CqF5u6ORAzqFZ8Qf3c27uVEahy7ShxSv2K4K41z'
+splunk::enterprise::password_hash: '$6$pIE/xAyP9mvBaewv$4GYFxC0SqonT6/x8qGcZXVCRLUVKODj9drDjdu/JJQ/Iw0Gg.aTkFzCjNAbaK4zcCHbphFz1g1HK18Z2bI92M0'
+splunk::enterprise::password_content: ":admin:${password_hash}::Administrator:admin:changeme@example.com::"


Does embedding variables in this way work hiera the same as it did with manifest interpolation? Documentation seems to indicate, no.

https://puppet.com/docs/puppet/latest/hiera_merging.html#the-lookup-and-hiera-function

Corroborated by the fact that this was caught in other data files, e.g. Linux.yaml

Yeah, this should certainly be a lookup

ody · 2019-08-30T20:47:15Z

lib/facter/splunkenterprise.rb

+Facter.add(:splunkenterprise) do
  setcode do
-    value = nil
+    splunk_hash = {}


I've never encountered another fact that returns an empty hash like this. The behavior I'd expect is that if there is no data related to splunk then the fact should be undefined within a manifest. If the behavior isn't followed then this fact will exist on all nodes within an infrastructure due to the way pluginsync works.

Agreed, I'll make some changes

nick-markowski · 2019-10-08T19:58:53Z

@ody Thank you for the review! I've really dragged my feet digging into this PR after submitting. By now it'll probably need a nasty rebase, but I'll try to get the feedback addressed and get things rolling again.

pccibot · 2019-10-08T20:22:31Z

Dear @nick-markowski, thanks for the PR!