(MODULES-7990) Merge multiple comments into one while parsing rules

[mateusz-gozdek-sociomantic]

As iptables/iptables-save accepts multiple '-m comment --comment' parameters,
we should find and merge them all together to avoid generating warnings.

Since puppet resource allows you to create only single comment, this should only
affect rules, which are not managed by puppet.

[david22swan]

@mateusz-gozdek-sociomantic I am so sorry, that was meant to go on another PR. A little bit stressed at the moment.
I meant to ask whether you could add a quick acceptance test. Sorry for the confusion.

[mateusz-gozdek-sociomantic]

Pushed with acceptance test added based on https://github.com/puppetlabs/puppetlabs-firewall/pull/789/files#diff-65287c59210687fbd22cf68976ae1ad1R52 (which BTW I'm not sure why assumes comment "http" is invalid).

[david22swan]

@mateusz-gozdek-sociomantic Unfortunately your new test seems to have failed when run against both redhat 5 and centos 5. This is the stacktrace that was given back:

Failure/Error: shell('iptables -A INPUT -j ACCEPT -p tcp --dport 80 -m comment --comment "http" -m comment --comment "http"')
Beaker::Host::CommandFailure:
  Host 'm6fm4j2caj3sfr8.delivery.puppetlabs.net' exited with 2 running:
   iptables -A INPUT -j ACCEPT -p tcp --dport 80 -m comment --comment "http" -m comment --comment "http"
  Last 10 lines of output were:
  	iptables v1.3.5: Unknown arg `http'
  	Try `iptables -h' or 'iptables --help' for more information.
  
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/host.rb:375:in `exec'
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/dsl/helpers/host_helpers.rb:83:in `block in on'
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/shared/host_manager.rb:130:in `run_block_on'
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/dsl/patterns.rb:37:in `block_on'
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/dsl/helpers/host_helpers.rb:63:in `on'
./.bundle/gems/gems/beaker-4.1.0/lib/beaker/dsl/helpers/host_helpers.rb:125:in `shell'
./spec/acceptance/resource_cmd_spec.rb:70:in `block (3 levels) in <top (required)>'

Sorry I don't have better news.

[mateusz-gozdek-sociomantic]

Ugh, it seems that iptables v1.3.5 does not allow you to specify multiple comments... Any suggestions how to resolve this?

[david22swan]

@mateusz-gozdek-sociomantic If it is a limitation of the code and not just an error, then the only thing you can really do is note it in the documentation and put an exception into the test. Here's an examle of one such exception that is used in the module:

describe 'bytecode property', unless: (fact('osfamily') == 'RedHat' && fact('operatingsystemmajrelease') < '7') ||
                                      (fact('operatingsystem') == 'OracleLinux' && fact('operatingsystemmajrelease') <= '7') ||
                                      (fact('operatingsystem') == 'SLES' && fact('operatingsystemmajrelease') <= '11') do

The above exception excludes all RedHat family OS lower than version 7, Oracle Linux OS equal to or lower than 7 and SLES OS equal to or lower than 11.
You can easily exclude RedHat 5 and Centos 5 in this manner.

[mateusz-gozdek-sociomantic]

Ok, I'll add it and run tests on CentOS 5. The code itself is OK, it's just this test, which will fail on this platform, as it is not possible to get multiple comments there. Could we maybe run this test only for iptables version higher than X?

[mateusz-gozdek-sociomantic]

What do you think @david22swan?

[david22swan]

@mateusz-gozdek-sociomantic Looks good but RedHat 5 needs to be excluded as well. Also I would prefer it if you where to use the syntax that I gave you, just to maintain consistency with the rest of the module.

[mateusz-gozdek-sociomantic]

Right, I forgot about RedHat 5. Hmm, speaking of consistency, it seems that since we don't want to disable describe, but only one specific context, if should be used instead of unless. Example: https://github.com/puppetlabs/puppetlabs-firewall/blob/master/spec/acceptance/purge_spec.rb#L128-L129

It seems for me, that using el-5 should be the right choice in this case.

BTW, feel free to push any code to this PR or suggest specific changes if this can make things faster :)

[david22swan]

@mateusz-gozdek-sociomantic When you excluded RedHat 5 your allowed Centos 5 back in. To speed things up I'm just gonna give you some code that I tested, should work fine:

context 'when accepts rules with multiple comments', unless: (fact('operatingsystem') == 'RedHat' && fact('operatingsystemmajrelease') <= '5') ||
                                                               (fact('operatingsystem') == 'CentOS' && fact('operatingsystemmajrelease') <= '5') do
    before(:all) do
      iptables_flush_all_tables
      shell('iptables -A INPUT -j ACCEPT -p tcp --dport 80 -m comment --comment "http" -m comment --comment "http"')
    end

    it do
      shell('puppet resource firewall') do |r|
        r.exit_code.should be_zero
        # don't check stdout, testing preexisting rules, output is normal
        r.stderr.should be_empty
      end
    end
  end

Sorry for the wait, had some other stuff to handle, also if I push to your fork then I can't merge your pr, it's why I have been avoiding doing it.

[mateusz-gozdek-sociomantic]

Hm, I thought matching for el-5 will disable all follwoing platforms from testing:

spec/acceptance/nodesets/new/pe/centos-5-64mda.yml:    platform: el-5-x86_64
spec/acceptance/nodesets/new/pe/oracle-5-64mda.yml:    platform: el-5-x86_64
spec/acceptance/nodesets/new/pe/redhat-5-64mda.yml:    platform: el-5-x86_64
spec/acceptance/nodesets/new/pe/scientific-5-64mda.yml:    platform: el-5-x86_64

Anyway, that's for posting the snippet, I'll give it a try locally and then push if it works.

Sorry for the wait, had some other stuff to handle, also if I push to your fork then I can't merge your pr, it's why I have been avoiding doing it.

No worries about the timing, but out of curiosity, why couldn't you merge it then?

[david22swan]

It's a company rule, except in certain situations, i..e. on new modules you are making or simple doc fixes, you are not allowed to merge your own PRs or any PR you have contributed to. It's to ensure that no error's are missed.

[david22swan]

@mateusz-gozdek-sociomantic Have kicked your changes into the adhoc pipeline but their packed right now so are unlikely to finish today, so have a good weekend and if all goes well I should have your changes merged in on monday.

[mateusz-gozdek-sociomantic]

@mateusz-gozdek-sociomantic Have kicked your changes into the adhoc pipeline but their packed right now so are unlikely to finish today, so have a good weekend and if all goes well I should have your changes merged in on monday.

Great news @david22swan, thanks a lot for all help! I'm also aligning #790 to this PR, as I didn't know few things and that one is slightly larger.

[david22swan]

@mateusz-gozdek-sociomantic Adhoc finished earlier than I thought so happy to merge :)

[david22swan]

Thanks for the pr, it's always nice to have a contributor get back to you so promptly. Will take a look at you other pr on monday.

[mateusz-gozdek-sociomantic]

it's always nice to have a contributor get back to you so promptly

I can tell the same for maintainer ;)

Thanks for merging.