Fix bug checking forwarding ports. Give better debug message (include protocol).

BUG=414501

Review URL: https://codereview.chromium.org/572793002

Cr-Commit-Position: refs/heads/master@{#296109}

tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder.py

@@ -2,6 +2,7 @@
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
+import contextlib
 import logging
 import socket
 
@@ -9,6 +10,21 @@ from telemetry.core import forwarders
 from telemetry.core import util
 
 
+class Error(Exception):
+  """Base class for exceptions in this module."""
+  pass
+
+
+class PortsMismatchError(Error):
+  """Raised when local and remote ports are not equal."""
+  pass
+
+
+class ConnectionError(Error):
+  """Raised when unable to connect to local TCP ports."""
+  pass
+
+
 class DoNothingForwarderFactory(forwarders.ForwarderFactory):
 
   def Create(self, port_pairs):
@@ -16,16 +32,44 @@ class DoNothingForwarderFactory(forwarders.ForwarderFactory):
 
 
 class DoNothingForwarder(forwarders.Forwarder):
+  """Check that no forwarding is needed for the given port pairs.
+
+  The local and remote ports must be equal. Otherwise, the "do nothing"
+  forwarder does not make sense. (Raises PortsMismatchError.)
+
+  Also, check that all TCP ports support connections.  (Raises ConnectionError.)
+  """
 
   def __init__(self, port_pairs):
     super(DoNothingForwarder, self).__init__(port_pairs)
+    self._CheckPortPairs()
 
-    for port_pair in port_pairs:
+  def _CheckPortPairs(self):
+    # namedtuple._asdict() is a public method. The method starts with an
+    # underscore to avoid conflicts with attribute names. pylint: disable=W0212
+    for protocol, port_pair in self._port_pairs._asdict().items():
       if not port_pair:
         continue
       local_port, remote_port = port_pair
-      assert local_port == remote_port, 'Local port forwarding is not supported'
-      def IsStarted():
-        return not socket.socket().connect_ex((self.host_ip, self.host_port))
-      util.WaitFor(IsStarted, 10)
-      logging.debug('Server started on %s:%d' % (self.host_ip, self.host_port))
+      if local_port != remote_port:
+        raise PortsMismatchError('Local port forwarding is not supported')
+      if protocol == 'dns':
+        logging.debug('Connection test SKIPPED for DNS: %s:%d',
+                      self.host_ip, local_port)
+        continue
+      try:
+        self._WaitForConnectionEstablished(
+            (self.host_ip, local_port), timeout=10)
+        logging.debug(
+            'Connection test succeeded for %s: %s:%d',
+            protocol.upper(), self.host_ip, local_port)
+      except util.TimeoutException:
+        raise ConnectionError(
+            'Unable to connect to %s address: %s:%d',
+            protocol.upper(), self.host_ip, local_port)
+
+  def _WaitForConnectionEstablished(self, address, timeout):
+    def CanConnect():
+      with contextlib.closing(socket.socket()) as s:
+        return s.connect_ex(address) == 0
+    util.WaitFor(CanConnect, timeout)

tools/telemetry/telemetry/core/forwarders/do_nothing_forwarder_unittest.py

+# Copyright 2012 The Chromium Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import unittest
+
+from telemetry.core import forwarders
+from telemetry.core import util
+from telemetry.core.forwarders import do_nothing_forwarder
+
+
+class TestDoNothingForwarder(do_nothing_forwarder.DoNothingForwarder):
+  """Override _WaitForConnect to avoid actual socket connection."""
+
+  def __init__(self, port_pairs):
+    self.connected_addresses = []
+    super(TestDoNothingForwarder, self).__init__(port_pairs)
+
+  def _WaitForConnectionEstablished(self, address, timeout):
+    self.connected_addresses.append(address)
+
+
+class TestErrorDoNothingForwarder(do_nothing_forwarder.DoNothingForwarder):
+  """Simulate a connection error."""
+
+  def _WaitForConnectionEstablished(self, address, timeout):
+    raise util.TimeoutException
+
+
+class CheckPortPairsTest(unittest.TestCase):
+  def testChecksOnlyHttpHttps(self):
+    port_pairs = forwarders.PortPairs(
+        http=forwarders.PortPair(80, 80),
+        https=forwarders.PortPair(443, 443),
+        dns=forwarders.PortPair(53, 53))
+    f = TestDoNothingForwarder(port_pairs)
+    expected_connected_addresses = [
+        ('127.0.0.1', 80),
+        ('127.0.0.1', 443),
+        # Port 53 is skipped because it is UDP and does not support connections.
+        ]
+    self.assertEqual(expected_connected_addresses, f.connected_addresses)
+
+  def testNoDnsStillChecksHttpHttps(self):
+    port_pairs = forwarders.PortPairs(
+        http=forwarders.PortPair(5566, 5566),
+        https=forwarders.PortPair(7788, 7788),
+        dns=None)
+    f = TestDoNothingForwarder(port_pairs)
+    expected_connected_addresses = [
+        ('127.0.0.1', 5566),
+        ('127.0.0.1', 7788),
+        ]
+    self.assertEqual(expected_connected_addresses, f.connected_addresses)
+
+  def testPortMismatchRaisesPortsMismatchError(self):
+    # The do_nothing_forward cannot forward from one port to another.
+    port_pairs = forwarders.PortPairs(
+        http=forwarders.PortPair(80, 80),
+        https=forwarders.PortPair(8443, 443),
+        dns=None)
+    with self.assertRaises(do_nothing_forwarder.PortsMismatchError):
+      TestDoNothingForwarder(port_pairs)
+
+  def testConnectionTimeoutRaisesConnectionError(self):
+    port_pairs = forwarders.PortPairs(
+        http=forwarders.PortPair(80, 80),
+        https=forwarders.PortPair(8443, 443),
+        dns=None)
+    with self.assertRaises(do_nothing_forwarder.ConnectionError):
+      TestErrorDoNothingForwarder(port_pairs)