ping_ip_address used without assertion in Neutron Tempest Plugin

[Assaf Muller]

The Neutron Tempest Plugin has an ping_ip_address helper in:

neutron_tempest_plugin/scenario/base.py

It's used in several places:

git grep -n ping_ip_address | cut -d":" -f1-2
neutron_tempest_plugin/scenario/base.py:313
neutron_tempest_plugin/scenario/test_basic.py:39
neutron_tempest_plugin/scenario/test_security_groups.py:118
neutron_tempest_plugin/scenario/test_security_groups.py:146
neutron_tempest_plugin/scenario/test_security_groups.py:152
neutron_tempest_plugin/scenario/test_security_groups.py:178
neutron_tempest_plugin/scenario/test_security_groups.py:193
neutron_tempest_plugin/scenario/test_security_groups.py:208
neutron_tempest_plugin/scenario/test_security_groups.py:261
neutron_tempest_plugin/scenario/test_security_groups.py:292

In all places it's used without an assertion. Meaning that if the ping fails, it'll timeout (CONF.validation.ping_timeout), then continue the test as if nothing happened. The test will not necessarily fail.

[Antonio Ojea (aojea)]

The ping command on that helper uses the options -c1 (count=1) and -w1 (deadline=1),

```
      If ping does not receive any reply packets at all it will exit with code 1. If a packet count and deadline are both specified, and fewer than count
       packets are received by the time the deadline has arrived, it will also exit with code 1. On other error it exits with code 2. Otherwise it exits
       with code 0. This makes it possible to use the exit code to see if a host is alive or not.
````

if the pings deson't receive 1 replay in 1 second the subprocess returns a returncode == 1 and makes the test fail

   def ping_ip_address(self, ip_address, should_succeed=True,
                        ping_timeout=None, mtu=None):
        # the code is taken from tempest/scenario/manager.py in tempest git
        timeout = ping_timeout or CONF.validation.ping_timeout
        cmd = ['ping', '-c1', '-w1']

        if mtu:
            cmd += [
                # don't fragment
                '-M', 'do',
                # ping receives just the size of ICMP payload
                '-s', str(net_utils.get_ping_payload_size(mtu, 4))
            ]
        cmd.append(ip_address)

        def ping():
            proc = subprocess.Popen(cmd,
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.PIPE)
            proc.communicate()

            return (proc.returncode == 0) == should_succeed

[Assaf Muller (amuller)]

> if the pings deson't receive 1 replay in 1 second the subprocess returns a returncode == 1 and makes the test fail

That's incorrect. The test will not fail.

Try it yourself... Create a new file and copy/paste this:

from neutron_tempest_plugin.scenario import base

class NetworkTempTest(base.BaseTempestTestCase):
    def test_if_ping_works(self):
        self.ping_ip_address('10.0.0.1', ping_timeout=1)

That test *passes* even though the ping will fail.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/640847

[Manjeet Singh Bhatia (manjeet-s-bhatia)]

actually it returns results TRUE or FALSE, I added some condition based assert in ping_ip_address or test consumer can also add assert statements like

def test_if_ping_works(self):
        stat = self.ping_ip_address('10.0.0.1', ping_timeout=1)
        self.assertTrue(stat) ?

or one I proposed would just work ?

[Assaf Muller (amuller)]

It's clearly dangerous to assume that the caller of the ping_ip_address method will know to use an assertTrue on the result, because of the 'git grep' results I showed above - we have several usages of the helper that don't also call an assert on the results. I think it's because the other ping helper (assert_connectivity), which has assert in the name, does have an internal assertion.

I think a good solution would be to insert an assertion in to the helper code, just like Manjeet proposed.

[Antonio Ojea (aojea)]

I see my mistake Assaf, you are right, the ping fails but the result is not checked with an assert and the test passes :-)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/640847
Committed: https://git.openstack.org/cgit/openstack/neutron-tempest-plugin/commit/?id=8bbf899ec0c1ce5a9a500d680dac2391b8149a96
Submitter: Zuul
Branch: master

commit 8bbf899ec0c1ce5a9a500d680dac2391b8149a96
Author: Manjeet Singh Bhatia <email address hidden>
Date: Mon Mar 4 11:59:57 2019 -0800

    add assert to ping test

    Change-Id: I24055036e6d45b5eff1cd127e2a2c78485287587
    Closes-Bug: #1818233

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/neutron-tempest-plugin 0.3.0 release.