BUG: Requirements for $logpath makes it impossible to set $backend to 'systemd'

[sigurdurdahl]

Hi,

I think the changes done in 50327e1c6361bedf5dee361f48570c3be6d63555 by @jkroepke introduces a bug.

Line 112 now requires that logpath is set

Variant[String, Array[String]] $logpath            = [],

while the if block at line 127 requires logpath to be unset if backend is systemd:

if $backend == 'systemd' {
    if $logpath {
      fail('logpath must not be set when $backend is \'systemd\'')
    }
  }

At least I have been unable to find a value for logpath that combines with systemd as chosen backend after my last upgrade of puppet-fail2ban. Workaround for now is using an older version.

Thanks in advance for looking into this!

Br, Sigurd

[jkroepke]

an empty array returns true and a if condition?

I would expect if [] {} (empty array) would evaluate to false

[sigurdurdahl]

Hi, I expected the same, but it won't it seems:

sigurdur@app-priv01-stage:~$ cat test.pp

$foo = []

if $foo {
  notice('foo is true')
}

sigurdur@app-priv01-stage:~$
sigurdur@app-priv01-stage:~$ puppet apply test.pp
Notice: Scope(Class[main]): foo is true
Notice: Compiled catalog for app-priv01-stage.yx.c.bitbit.net in environment production in 0.01 seconds
Notice: Applied catalog in 0.00 seconds
sigurdur@app-priv01-stage:~$ puppet --version

6.19.1

According to https://puppet.com/docs/puppet/6/lang_data_abstract.html

Optional[] is equivalent to Variant[ , Undef ].

Would it be a solution to change $logpath back to Optional and default it to undef?

[lelutin]

oh wow that is a weird property of arrays.. I was also expecting if $array { to be "false-ish".

I can see that puppet 5.5.0 has introduced a new builtin function empty() (which was available in stdlib before that) that we can use in this manner:

$foo = []

if ! empty($foo) {
  notice('foo is true')
}

I'll patch that up real quick. thanks for catching that and notifying us!

[lelutin]

@sigurdurdahl I've pushed a commit that had a mention of closing this issue. from my own testing I believe that the issue is fixed, however I'd like to also get your confirmation if this is true before I make a new forge release for the module.

note that I've also expanded the data type change made by @jkroepke to the logpath param on the module's main class. This way the data types are consistent for the same setting whether it's made on the global scope or on individual jails.

I'll await for your feedback regarding the fix!

[sigurdurdahl]

Sorry for the late reply @lelutin !

I can confirm that this resolves the issue. Thank you!

[lelutin]

Sorry for the late reply @lelutin !

there's no issue at all, we can't always be in front of github ready to respond, right? ;) actually I think your response was pretty quick hehe.

I can confirm that this resolves the issue. Thank you!

great, thanks for testing it out!

I'll try and find some time soon to make a release of the module then.