Add 7.2 support and refactor module #215

nick-markowski · 2019-03-05T21:58:37Z

Supports systemd boot-start
Changed module design from Class[splunk], Class[splunk::forwarder] to
Class[splunk::enterprise] and Class[splunk::forwarder]
Removed legacy-style service management
Allow users to disable default config (splunk/forwarder_* types)
Removed ability to include enterprise and forwarder on the same machine
- Forwarder is simply a subset of enterprise capability and there is no need
  to have both
- Forwarder and enterprise potentially share common service names and other
  assets, which cause issues when including both
Bumped dependency on stdlib to 4.25.0 to ensure strong typing support
Allow users to specify splunk service user
Added password management to enterprise

Fixes #213
Fixes #212
Fixes #210

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

nick-markowski · 2019-03-05T21:59:36Z

Still need to:

Update in-module and README parameter documentation once design is approved
Update acceptance tests to test systemd changes
Optionally upgrade test, and default the version to the latest release

bastelfreak · 2019-03-07T17:58:49Z

manifests/addon.pp

  $package_manage                = true,
  Optional[String] $package_name = undef,
  $inputs                        = {},
 ) {

+  include '::splunk::params'


please remove the leading ::. That is legacy syntax that was required for puppet 3.

I have...a lot of these to remove lol

bastelfreak · 2019-03-07T17:59:03Z

manifests/addon.pp

+  include '::splunk::params'
+
+  if !defined($splunk_home) {
+    $_splunk_home = $::splunk::params::forwarder_homedir


same here, please remove the leading ::.

bastelfreak · 2019-03-07T17:59:55Z

manifests/enterprise.pp

+# Class splunk::enterprise
+#
+class splunk::enterprise (
+  String $version                          = $::splunk::params::version,


for all the strings, can you update it to String[1] to prohibit empty strings?

alexjfisher · 2019-03-07T19:56:44Z

manifests/addon.pp

  $package_manage                = true,
  Optional[String] $package_name = undef,
  $inputs                        = {},
 ) {

+  include '::splunk::params'
+
+  if !defined($splunk_home) {


or just unless $splunk_home {

Or if $splunk_home and reverse the blocks.

alexjfisher · 2019-03-07T20:00:11Z

@nick-markowski Huge thanks for taking this on! It's something I'm going to be needing myself shortly, so I should be able to test your PR sometime over the next week or so.

alexjfisher · 2019-03-07T20:01:41Z

manifests/enterprise.pp

+  String $service_file                     = $::splunk::params::enterprise_service_file,
+  Boolean $boot_start                      = $::splunk::params::boot_start,
+  Boolean $use_default_config              = true,
+  String $input_default_host               = $facts['clientcert'],


clientcert? That doesn't actually have to be a hostname. Probably better to stick with $facts['fqdn'] here.

alexjfisher · 2019-03-07T20:02:36Z

manifests/enterprise.pp

+  Optional[String] $package_provider       = $::splunk::params::package_provider,
+  Boolean $manage_package_source           = true,
+  Optional[String] $package_source         = undef,
+  Optional[Array[String]] $install_options = $::splunk::params::enterprise_install_options,


Instead of this being Optional, can we default to an empty array instead?

alexjfisher · 2019-03-07T20:03:35Z

manifests/enterprise.pp

+  Optional[String] $package_source         = undef,
+  Optional[Array[String]] $install_options = $::splunk::params::enterprise_install_options,
+  String $splunk_user                      = $::splunk::params::splunk_user,
+  String $enterprise_homedir               = $::splunk::params::enterprise_homedir,


Stdlib::Absolutepath instead of String?

alexjfisher · 2019-03-07T20:04:48Z

manifests/enterprise.pp

+  Boolean $use_default_config              = true,
+  String $input_default_host               = $facts['clientcert'],
+  String $input_connection_host            = 'dns',
+  String $splunkd_listen                   = '127.0.0.1',


Stdlib::IP::Address?

I need to comb through all of the strong typing, I winged it

nick-markowski · 2019-03-08T20:55:47Z

@alexjfisher @bastelfreak Splunk has made an assumption that cgroups are present in the systmed unit file, if enabled (boot-start). Cgroups are obviously not present in containers, so the test suite fails:

chown: cannot access ‘/sys/fs/cgroup/cpu/system.slice/Splunkd.service’: No such file or directory

One way we can probably get around it is to hack in the 2 cgroup files the until file expects to manage. It's ugly, but i'm not sure what else to do until Splunk fixes their code.

EDIT: If you're running splunk in a container IRL you would probably not use systemd to manage the service, you would use the binaries directly to kick off the container (or download the splunk container, per their docs). We may need to update the travis tests to utilize virtual hosts instead of containers to fully test the suite.

bastelfreak · 2019-03-08T23:16:58Z

manifests/enterprise/install.pp

+  }
+
+  if $splunk::enterprise::package_provider != undef and $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' {
+    include ::archive::staging


please use:

Suggested change

include ::archive::staging

include archive::staging

ody · 2019-03-22T21:18:58Z

Hey @nick-markowski,

How can I help get this over the line and merged? Working on a project and officially blocked by the lack of 7.2 support.

nick-markowski · 2019-03-25T13:11:30Z

Hey @ody ! I have a list of todo items I should be able to begin addressing this week:

Update in-module and README parameter documentation once design is approved
- I need to slog through copying the old parameter docs to the new classes and ensure new parameters are documented
Update acceptance tests to test systemd/backwards compatibility changes thoroughly
- This is a good place for input! I can only manually test so much, and the test suite needs upgrading
- Test add-on and password if possible, I don't have a good use case for either
- Optionally upgrade test, and default the version to the latest release (already done, but we need to update the test suite)

Before the tests can be completed, I need some resolution about how to handle issues I posted in the chain, above.

EDIT: while automated acceptance is a good end-goal, manually deploying the module in a test environment and ensuring the logic is not complete garbage would be very useful, too!

alexjfisher · 2019-03-25T15:31:23Z

@alexjfisher @bastelfreak Splunk has made an assumption that cgroups are present in the systmed unit file, if enabled (boot-start). Cgroups are obviously not present in containers, so the test suite fails:

chown: cannot access ‘/sys/fs/cgroup/cpu/system.slice/Splunkd.service’: No such file or directory

One way we can probably get around it is to hack in the 2 cgroup files the until file expects to manage. It's ugly, but i'm not sure what else to do until Splunk fixes their code.

EDIT: If you're running splunk in a container IRL you would probably not use systemd to manage the service, you would use the binaries directly to kick off the container (or download the splunk container, per their docs). We may need to update the travis tests to utilize virtual hosts instead of containers to fully test the suite.

Umm... I really don't know. I wonder if @ekohl has any ideas?

ekohl · 2019-03-25T15:38:28Z

One way we can probably get around it is to hack in the 2 cgroup files the until file expects to manage. It's ugly, but i'm not sure what else to do until Splunk fixes their code.

This sounds fair. The spec_acceptance_helper sounds like the correct place if they just need to exist and setting it up once is enough.

We may need to update the travis tests to utilize virtual hosts instead of containers to fully test the suite.

AFAIK we have no place to run VMs

nick-markowski · 2019-03-25T15:45:07Z

@ekohl I'll try to get the tests running in the containers and report any other blockers, thanks!

nick-markowski · 2019-03-26T20:23:37Z

@alexjfisher @bastelfreak looks like some dependencies within the debian nodeset are defunct, not sure how you want to handle it

ekohl · 2019-03-26T20:28:07Z

Debian 8 is EOL. Technically it's LTS, but in Debian that has a different meaning. It's fine to drop Debian 8 as a supported OS and only support Debian 9 IMHO.

bastelfreak · 2019-03-27T09:18:59Z

@nick-markowski are you happy with this as it is now? I don't want to review such a huge PR multiple times :D. Please remove the WIP from the title if this is ready.

nick-markowski · 2019-03-27T11:18:35Z

@bastelfreak understandable! I left it WIP because I need to update the README and in-module param documentation. I'll get that done this week.

juniorsysadmin · 2019-03-31T05:23:18Z

manifests/addon.pp

-  Optional[String] $package_name = undef,
-  $inputs                        = {},
+  Optional[Stdlib::Absolutepath] $splunk_home = undef,
+  Boolead $package_manage                     = true,


nick-markowski · 2019-04-02T11:40:53Z

@alexjfisher @bastelfreak @ekohl Looks like the review has already started and I need to add some testing - could I get some more 👀 on this? :)

alexjfisher · 2019-04-02T12:10:41Z

@nick-markowski I'm going to be trying this change out for real in the next few days. :)

bastelfreak · 2019-04-02T20:15:12Z

.sync.yml

@@ -4,7 +4,7 @@
  docker_sets:
  - set: centos6-64
  - set: centos7-64
-  - set: debian8-64
+  - set: debian9-64


to make the whole diff smaller, do you think it's possible to add debian9 in a seperate PR and drop debian 8 in a seperate PR?

I could re-introduce those changes as a separate PR (unless someone else beats me to it) and rebase this one to clean things up, for sure

Is a move to Debian 9 a requirement for Splunk 7.2? That would be the only reason to rebase. If it isn't a requirement then it should be able to be handled entirely separately.

I only updated because the debian 8 docker images were defunct and not letting the test suite pass, I'm fine with ditching the change and ignoring the deb8 tests

manifests/enterprise/config.pp

bastelfreak · 2019-04-02T20:17:53Z

manifests/enterprise/config.pp

+  }
+
+  if $facts['virtual'] == 'docker' {
+    ini_setting { 'OPTIMISTIC_ABOUT_FILE_LOCKING':


lol, nice name for an option :D

Lol, most of this is reused code I didn't find reason to modify :)

bastelfreak · 2019-04-02T20:20:33Z

manifests/enterprise/config.pp

+    }
+  }
+
+  File <| tag == 'splunk_enterprise' |>


I'm wondering if we can simplify the whole block of class dependencies 🤔

I hope so, it was painful to write these out! I guess, technically, we could order the types to consolidate them into one block; order is arbitrary anyways:

File <| tag == 'splunk_enterprise' |>
-> Splunk_authentication <| tag == 'splunk_enterprise' |>
-> Splunk_authorize <| tag == 'splunk_enterprise' |>
-> Splunk_deploymentclient <| tag == 'splunk_enterprise' |>
... etc
~> Class['splunk::enterprise::service']

Thoughts?

@alexjfisher @binford2k can you have a look please?

Removed ability to include enterprise and forwarder on the same machine

Was this the only reason for tagging the resources? Maybe much of this could be simplified??

bastelfreak · 2019-04-02T20:24:03Z

manifests/enterprise/install.pp

+    false => $splunk::enterprise::package_source
+  }
+
+  if $splunk::enterprise::package_provider != undef and $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' {


if $splunk::enterprise::package_provider != undef is equal to if $splunk::enterprise::package_provider

if $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' should be equal to if not $splunk::enterprise::package_provider in ['apt', 'chocolatey', 'yum'] (but I didn't test it).

I'll try cleaning this up, I have a local centos7 build I'm using yum providers with

bastelfreak · 2019-04-02T20:24:28Z

manifests/enterprise/install.pp

+  }
+
+  if $splunk::enterprise::package_provider != undef and $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' {
+    include archive::staging


is this include actually required? 🤔

Good question, most of the crazy install logic I just forklifted and plopped into place...

I was unable to find a reason for its inclusion aside from it including the archive class but I am also not even sure the archive class is required, it just installs awscli or 7zip on Windows.

This module is listed as supporting Windows though.

bastelfreak · 2019-04-02T20:26:35Z

manifests/enterprise/install/nix.pp

+
+  # Required for splunk 7.2.4.2
+  if versioncmp($splunk::enterprise::version, '7.2.4.2') >= 0 {
+    if !defined(Package['net-tools']) {


this could be simplified by using https://github.com/puppetlabs/puppetlabs-stdlib#ensure_packages

bastelfreak · 2019-04-02T20:28:53Z

manifests/enterprise/service.pp

+  # This is a module that supports multiple platforms. For some platforms
+  # there is non-generic configuration that needs to be declared in addition
+  # to the agnostic resources declared here.
+  if $facts['kernel'] == 'Linux' or $facts['kernel'] == 'SunOS' {


Untetested but should work and is shorter:

Suggested change

if $facts['kernel'] == 'Linux' or $facts['kernel'] == 'SunOS' {

if $facts['kernel'] in ['Linux', 'SunOS'] {

bastelfreak · 2019-04-02T20:30:34Z

manifests/forwarder/install.pp

+    false => $splunk::forwarder::package_source
+  }
+
+  if $splunk::forwarder::package_provider != undef and $splunk::forwarder::package_provider != 'yum' and $splunk::forwarder::package_provider != 'apt' and $splunk::forwarder::package_provider != 'chocolatey' {


same here as mentioned above

alexjfisher · 2019-04-03T12:21:00Z

manifests/forwarder.pp

-  # If the purge parameters have been set, remove all unmanaged entries from
-  # the respective config files.
+# Class splunk::forwarder
+# 


nit. Trailing whitespace on every other line.

Not sure why puppet-lint doesn't complain.

Maybe comments are linted differently?

alexjfisher · 2019-04-03T13:10:07Z

manifests/forwarder.pp

+    )
+  }
+
+  include 'splunk::forwarder::install'


Should these use contain?

eg what if I'm doing something like...

pulp_rpmbind {'internal-splunk-7': } class { 'splunk::forwarder': server => $server, manage_package_source => false, package_provider => 'yum', package_ensure => 'latest', require => Pulp_rpmbind['internal-splunk-7'], }

alexjfisher · 2019-04-03T13:21:05Z

README.md

+
+#### `addons`
+
+Manage a splunk addons, see `splunk::addons`.  Defaults to an empty Hash.


nit. Should read Manage splunk addons (without the a).

alexjfisher · 2019-04-03T14:01:33Z

manifests/forwarder/config.pp

+
+  # Declare inputs and outputs specific to the forwarder profile
+  $_tag_resources = { tag => 'splunk_forwarder' }
+  if $splunk::forwarder::forwarder_input {


This will always be truthy as $splunk::forwarder::forwarder_input is of type Hash.

alexjfisher · 2019-04-03T14:02:29Z

manifests/forwarder/config.pp

+  # Declare inputs and outputs specific to the forwarder profile
+  $_tag_resources = { tag => 'splunk_forwarder' }
+  if $splunk::forwarder::forwarder_input {
+    create_resources( 'splunkforwarder_input', $splunk::forwarder::forwarder_input, $_tag_resources)


You could/should use iteration instead of create_resources.

ody · 2019-04-03T20:03:00Z

manifests/addon.pp

+  Optional[Stdlib::Absolutepath] $splunk_home = undef,
+  Boolead $package_manage                     = true,
+  Optional[String[1]] $package_name           = undef,
+  $inputs                                     = {},


Could you strong type this parameter too so its consistent?

ody · 2019-04-03T20:39:41Z

manifests/enterprise/install.pp

+  }
+
+  if $splunk::enterprise::package_provider != undef and $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' {
+    include archive::staging


I was unable to find a reason for its inclusion aside from it including the archive class but I am also not even sure the archive class is required, it just installs awscli or 7zip on Windows.

ody · 2019-04-03T20:40:15Z

manifests/enterprise/install.pp

+  if $splunk::enterprise::package_provider != undef and $splunk::enterprise::package_provider != 'yum' and $splunk::enterprise::package_provider != 'apt' and $splunk::enterprise::package_provider != 'chocolatey' {
+    include archive::staging
+    $_src_package_filename = basename($_package_source)
+    $_package_path_parts   = [$archive::path, $splunk::enterprise::staging_subdir, $_src_package_filename]


$archive::path isn't ever set by Class[archive]

ody · 2019-04-04T20:59:06Z

Basic functionality of standing up a new instance of 7.2 and installing forwarder validated

Thanks @nick-markowski!

alexjfisher · 2019-04-05T08:39:55Z

manifests/enterprise/service/nix.pp


  if $splunk::enterprise::boot_start {
-    # Eensure splunk services *not* managed by the system service file are
+    # Ensuure splunk services *not* managed by the system service file are


So close! ;)

alexjfisher · 2019-04-05T08:43:21Z

Basic functionality of standing up a new instance of 7.2 and installing forwarder validated

Thanks @nick-markowski!

@ody Thanks for testing! It's been looking good for me too.

alexjfisher · 2019-04-05T10:49:29Z

manifests/forwarder/config.pp

+  ~> Class['splunk::forwarder::service']
+
+  File <| tag == 'splunk_forwarder' |>
+  -> Splunkforwarder_input            <| tag == 'splunk_forwarder' |>


What's the deal with only looking for resources with the specific tag?
Previously, you could declare these resources without a tag and they would notify the service.
See #50

* Default splunk version to 7.2.4.2 * Supports systemd boot-start * sysvinit file ensured absent on systemd machines when supported * net-tools ensured present on enterprise nodes splunk >= 7.2.4.2 * Removed legacy-style service management * Changed module design from Class[splunk], Class[splunk::forwarder] to Class[splunk::enterprise] and Class[splunk::forwarder] * Removed inclusion of the ::archive class * Cleaned up install logic * Allow users to disable default config (splunk/forwarder_* types) * Generate splunkforwarder_input/output with iteration, instead of create_resources * Removed ability to include enterprise and forwarder on the same machine * Forwarder and enterprise potentially share common service names and other assets, which cause issues when including both * Forwarder is simply a subset of enterprise capability and there is no need to have both * Bumped dependency on stdlib to 4.25.0 to ensure strong typing support * Allow users to specify splunk service user * Added password management to enterprise Fixes #213 Fixes #212 Fixes #210

nick-markowski changed the title ~~Add 7.2 support and refactor module~~ WIP: Add 7.2 support and refactor module Mar 5, 2019

bastelfreak reviewed Mar 7, 2019

View reviewed changes

alexjfisher reviewed Mar 7, 2019

View reviewed changes

bastelfreak reviewed Mar 8, 2019

View reviewed changes

nick-markowski changed the title ~~WIP: Add 7.2 support and refactor module~~ Add 7.2 support and refactor module Mar 29, 2019

juniorsysadmin reviewed Mar 31, 2019

View reviewed changes

bastelfreak reviewed Apr 2, 2019

View reviewed changes

manifests/enterprise/config.pp Show resolved Hide resolved

bastelfreak reviewed Apr 2, 2019

View reviewed changes

alexjfisher reviewed Apr 3, 2019

View reviewed changes

alexjfisher requested changes Apr 3, 2019

View reviewed changes

ody reviewed Apr 3, 2019

View reviewed changes

alexjfisher reviewed Apr 5, 2019

View reviewed changes

alexjfisher requested changes Apr 5, 2019

View reviewed changes

alexjfisher added enhancement New feature or request backwards-incompatible needs-rebase needs-squash labels Apr 5, 2019

alexjfisher removed needs-rebase needs-squash labels Apr 5, 2019

alexjfisher approved these changes Apr 5, 2019

View reviewed changes

alexjfisher merged commit d3312a2 into voxpupuli:master Apr 5, 2019

alexjfisher mentioned this pull request Apr 12, 2019

Module does not seem to work with splunkforwarder 7.X #186

Closed

	if $facts['kernel'] == 'Linux' or $facts['kernel'] == 'SunOS' {
	if $facts['kernel'] in ['Linux', 'SunOS'] {


		#### `addons`

		Manage a splunk addons, see `splunk::addons`. Defaults to an empty Hash.

Add 7.2 support and refactor module #215

Add 7.2 support and refactor module #215

Conversation

nick-markowski commented Mar 5, 2019

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

nick-markowski commented Mar 5, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher commented Mar 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-markowski commented Mar 8, 2019 • edited Loading

Choose a reason for hiding this comment

ody commented Mar 22, 2019

nick-markowski commented Mar 25, 2019 • edited Loading

alexjfisher commented Mar 25, 2019

ekohl commented Mar 25, 2019

nick-markowski commented Mar 25, 2019

nick-markowski commented Mar 26, 2019

ekohl commented Mar 26, 2019

bastelfreak commented Mar 27, 2019

nick-markowski commented Mar 27, 2019

Choose a reason for hiding this comment

nick-markowski commented Apr 2, 2019

alexjfisher commented Apr 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-markowski Apr 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ody commented Apr 4, 2019

Choose a reason for hiding this comment

alexjfisher commented Apr 5, 2019

Choose a reason for hiding this comment

nick-markowski commented Mar 5, 2019 •

edited

Loading

nick-markowski commented Mar 8, 2019 •

edited

Loading

nick-markowski commented Mar 25, 2019 •

edited

Loading

nick-markowski Apr 3, 2019 •

edited

Loading