(MODULES-8543) Remove nftables' backend warning from iptables_save outtput

[NITEMAN]

Rel https://tickets.puppetlabs.com/browse/MODULES-8543

[rayderua]

Work only on ipv4 iptables :(
ipv6 warning is: # Warning: ip6tables-legacy tables present, use ip6tables-legacy-save to see them\n

[NITEMAN]

@rayderua extended to support ip6tables. It should work but testing is always appreciated

[rayderua]

Thanks!
Waiting for release

[david22swan]

@NITEMAN Apologies but I don't think this is something that I can merge. While I can see why you wish this change made, the suppression and removal of warnings is not something that we would like within our code. Rather than simply removing the warns outright if you could instead in turn throw a puppet warn when they are detected I think that that would be more beneficial overall.

[cFire]

Just my 2 cents:
It looks like it isn't so much an issue of having to suppress the the warning. From the errors we see (and as they are shown in the issue linked earlier) it looks like it's a problem with STDERR and STDOUT getting mixed together.

The STDERR output (ie. the warnings about iptables-legacy) are spliced into STDOUT by whatever random timing it happens to produce the warning at, causing them to show up halfway into an iptables rule. After all, these warnings start with a #, which would mean they'd normally get ignored by the rule parser if they started at the start of the line like they should.

I suspect if we properly split out STDOUT and STDERR (maybe concat STDERR to the end of STDOUT if we care about parsing it) that the problem should go away.

disclaimer: I've not yet tested ANY of this. It's just what it looks like to me from experience.

[NITEMAN]

@david22swan the problem is exactly what @cFire points (and that's also mentioned on the original issue with links tu Puppet's core code) .

I might try to raise a puppet warning when ip6?tables-save output contains the warning but:

• If we want different warnings for iptables-save / ip6tables-save, detection logic will complicate.
• IMHO the warning will be useless for the user, since the module doesn't support ip6?tables-legacy-save
• We'll need to strip the warning from the output anyway to make the parser work.

[NITEMAN]

Also, this code gets called a bunch of times, causing in one test machine the warning to be printed more than 20 times if introduced

[NITEMAN]

FTR, my warning try consisted on inserting this block between the two lines:

    if iptables_save =~ %r{#{nf_warning_msg}}
      warning "Sripping '#{nf_warning_msg}' form ip6?tables-save combined output"
    end 

[cFire]

How about something like this?

--- a/lib/puppet/provider/firewall/iptables.rb
+++ b/lib/puppet/provider/firewall/iptables.rb
@@ -406,7 +406,7 @@ Puppet::Type.type(:firewall).provide :iptables, parent: Puppet::Provider::Firewa
     counter = 1
 
     # String#lines would be nice, but we need to support Ruby 1.8.5
-    iptables_save.split("\n").each do |line|
+    self.execute('iptables-save', { :combine => false }).split("\n").each do |line|
       unless line =~ %r{^\#\s+|^\:\S+|^COMMIT|^FATAL}
         if line =~ %r{^\*}
           table = line.sub(%r{\*}, '')

The way the exec is done here is not very pretty, but I've not found another way to tell Puppet::Provider to drop the stderr output.

I've not seen any way to make it behave cleaner either as it calls Puppet::Util::Execution under the hood and this uses IO.select to read 4096 bytes from whichever IO buffer is ready to provide and appends it to an output variable. which assigns stdout and stderr to the same IO.pipe causing them to get mixed by the timing the with which the process outputs to either pipe.
(See https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/execution.rb#L212 for implementation.)

This is a long winded way of saying "I don't think we can keep stderr and not have this problem."

[NITEMAN]

@cFire honestly I don't know what would be better... in one hand your fix is future proof and output agnostic but in the other hand it might lead to other issues going unnoticed.

Anyway, I will welcome any solution that fix the module

[cFire]

Also, let me be perfectly clear that this is something that should be fixed in puppet (and time permitting, I may give that a shot). The best we can do here is a workaround that's not too ugly.

As for the concerns @david22swan expressed about suppressing warnings; I don't think it's something this change would affect. Since iptables-save is in the habit of prefixing any warnings with a # so they don't break anything when you iptables-restore it, and since the line parser already completely ignores any lines starting with #, all errors are already silently ignored as it is.

Now you might argue that this is bad, and you may be right. But I think that discussion is academic here since we would not be changing the current behavior by dropping stderr.

If the solution I proposed in my diff is found to be acceptable, I'll create a new PR for it. (Since if it's not found to be acceptable I'm not going to put time into fixing the spec tests of course.)

[david22swan]

@NITEMAN After some discussion with the team we have decided to merge this as it is. Thank you for the work you have put in.

[cFire]

@NITEMAN After some discussion with the team we have decided to merge this as it is.

I would recommend against this, as it is not technically correct. The fix as-is discards any lines of output that happens to get corrupted by the error, masking its effects. But I suspect this may lead to duplicate rules getting inserted and/or partially parsed rules getting incorrectly purged.

[NITEMAN]

@cFire my fix doesn't discard the line but strips the string from the output (before splitting). If it worths anything my fix is already production tested without issues for nearly a month in ~10 different servers.

@david22swan thank you very much for your continued work.

[cFire]

@cFire my fix doesn't discard the line but strips the string from the output (before splitting).

Apologies. I apparently miss-remembered what your patch did exactly.