overide default logging with filebeat version 7

[freibuis]

This PR overrides default logging with filebeat version 7 running on systems running systemd

Since filebeat version 7, systemd service file hard codes the flag -e. this option sets the logging to syslog and will ignore any logging set by this puppet module.

This change creates a systemd drop-in file for filebeat that drops the -e flag if a $logging hash is set when using the class

[pcfens]

Thanks for the work on this. I've made a few comments, plus we should add some documentation around what's happening.

We need to be careful about changing existing installs too much so we don't break backwards compatibility too. While the default behavior might not be intuitive, I'm hesitant to change how the service runs on a minor version upgrade. I'm open to suggestions, but maybe this change should be opt-in?

[freibuis]

Agreed. this totally needs to be opt in. just not sure how to approach this.

Also I think the code supplied might fail on an older system or a system that does not have systemd installed. because it will try to install a file into the service drop in location.. if the base directories are not installed it will fail..... sooooooo this also need to test for systemd is part of the system

[freibuis]

create a bolean param

$systemd_log_opt_override      = false

I am terrible at naming things.

if $systemd_log_opt_override and version 7+
add drop in service file
else
remove drop in service file
end

how does this sound?

[pcfens]

Thinking about this a bit more, we need to cover a few more things. Before doing anything we need to check that systemd is in use (is the service_provider fact always around?). If we create an override file, we need to reload the systemd configuration too.

The camptocamp/systemd module does a nice job of dropping the files in place and reloading systemd, but I'm not sure it makes sense to force everyone to use the same systemd module either (is that something people feel strongly about?).

What if we create a single new string parameter named systemd_beat_log_opts? If it's left undefined we do nothing at all, regardless of what the logging parameter is set to. If the parameter has a value (including an empty string), we use that in the override file. Of course we still need to check for systemd in case someone is installing on a box without systemd (including Windows).

[freibuis]

fyi,as of puppet 6.1 systemctl daemon-reload is handled when units change already (6.1 release_notes) ... I don't think adding the camptocamp/systemd dependency added to the dep manifest is needed. but add documentation that they should add this if they have older puppet versions.. I doubt that everyone is like me and keeps puppet bleeding-edge like me though. but I do agree the puppet-systemd module will do a better job managing systemd unit files

with the proposed var systemd_beat_log_opts I suggest defaults be

String $systemd_beat_log_opts  = '-e'

the reason I say this is,

• its the default for filebeat 7.0+ with systemd
• having the drop-in file being the same will not make any system changes if the drop in file is created
• this would allow the add extra options (as seen in filebeat docs) like -e -d elastic
• setting this to undef would set the opt to empty and allow the logging hash to work

I also personally think we should not touch the original systemd service file as this gets replaced on each upgrade (which I just noticed upgrading systems to 7.3) this means we might need to make a params logic that if in the future they change the default, we supply that default

I think this logic would be better with the optional camptocamp/systemd module is available. we will also not need to check for systemd systems as the module handles this.

# nothing happens if the module is not included 
if defined('systemd')  {
  systemd::dropin_file { 'logging.conf':
  unit   => 'filebeat.service',
  source => template logic here,
}

we would need to add documentation to the $logging and the $systemd_beat_log_opts that the camptocamp/systemd module would need to be added if you want to change the behaviour on systemd systems

[pcfens]

I didn't know about the systemd change, but that certainly makes things easier. Fewer dependencies is always a little better too, especially on something like systemd. A note in the docs that you'll need Puppet 6.1 or higher if you're using that parameter is probably enough.

I'd rather not manage an override file at all if we're not overriding anything, which is why I'm thinking a default undef value might be easier than having to actively keep up with default value. Maybe a better parameter name is systemd_beat_log_opts_override?

[freibuis]

the more and more I think about it.. I agree with your last message.

here is what I will change with PR.

• check that puppet version
  $::clientversion >= 6.1
• use StdLib $::service_provider fact to detect what service provider is being used
  service_provider == 'systemd'
  ->
• check if filebeat >= 7 is being used
  ->
• if $systemd_beat_log_opts_override != undef create dropin file and if == undef make sure the file is absent

this will actually simplify future version.

[freibuis]

Changed the pr. I also updated the README for the class information and added to the Limitations section.
I think the puppet-systemd module usage in the documentation provided is accurate. but I think it should be used against filebeat::service instead

let me know if you want me to change it and put the override in the service class instead

class{'filebeat::service':
 systemd_beat_log_opts => ""
}

[pcfens]

I'm not sure we should always drop a file in place when Filebeat 7+ is installing without a way to ensure absent. We need to consider someone managing this file outside of this module, or wanting it excluded entirely.

[pcfens]

The ensure_resource approach to avoid duplicate declaration works, but requires a specific execution order that otherwise won't exist.

[pcfens]

I think it makes sense to use ensure resources for the directory just in case someone is managing the directory elsewhere.

[pcfens]

We probably should let people override even if they're running a version of Puppet older than 6.1 (maybe print a warning?)

[freibuis]

so if puppet is less then 6.1 check to see if systemd class not defined and warn that they will need systemd class and follow the documation ?

[pcfens]

We should probably use a regular file resource here instead of using the ensure_resource function. If someone is managing an override file outside of this module and they also create a file named logging.conf then we should probably throw an error .

[freibuis]

ok that makes sense. I was more worried that some one else would have managed the file resource.. if they had this would not have thrown an error.. just ignored it..

but yes the user should be made aware that this module uses that resource. 👍

[pcfens]

Can we rename this to systemd_override_dir?

[freibuis]

agreed
this makes a lot of sense. not many people will under stand what a drop in directory is
I will change it to systemd_override_dir

[freibuis]

I will have this pull request updated with the recommendations soon.

[pcfens]

Thanks for all the work on this. I'm not going to be near a computer for the rest of the week so I'm going to cut a new release on Monday in case we missed something.

[dforste]

Would it be better to use elastic's preferred method rather than overriding the systemd file wholesale?
/etc/systemd/system/filebeat.service.d/override.conf
https://www.elastic.co/guide/en/beats/filebeat/current/running-with-systemd.html

[dforste]

I ended up creating file resources like this:

  /etc/systemd/system/filebeat.service.d:
    ensure: directory
    owner: root
  /etc/systemd/system/filebeat.service.d/override.conf:
    ensure: file
    owner: root
    group: root
    content: |
      [Service]
      Environment="BEAT_LOG_OPTS="
    notify:
      - Exec[systemctl-daemon-reload]
      - Service[filebeat]

The Exec[systemctl-daemon-reload] is from camptocamp/systemd however if you have puppet 6.1 it sounds like that can be dispensed with.

[pcfens]

@dforste That's essentially what's been done here, but everything has been scoped to logging for now, and the file values have been setup in a template so you can override things beyond just -e or blank if you'd like.

[pcfens]

Released as 4.1.0