From add510701c0809cf89494434c1dccdfc3271df47 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 11:44:51 -0500 Subject: [PATCH 01/10] Run integration tests both with and without HTTP storage protocol. --- .github/workflows/ci.yml | 6 +++++- integration/util.py | 17 +++++++++-------- newsfragments/3937.minor | 0 src/allmydata/protocol_switch.py | 16 ++++++++++++++++ src/allmydata/testing/__init__.py | 18 ++++++++++++++++++ 5 files changed, 48 insertions(+), 9 deletions(-) create mode 100644 newsfragments/3937.minor diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0327014ca..26574066c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -229,7 +229,11 @@ jobs: # aren't too long. On Windows tox won't pass it through so it has no # effect. On Linux it doesn't make a difference one way or another. TMPDIR: "/tmp" - run: tox -e integration + run: | + # Run with Foolscap forced: + __TAHOE_INTEGRATION_FORCE_FOOLSCAP=1 tox -e integration + # Run with Foolscap not forced, which should result in HTTP being used. + __TAHOE_INTEGRATION_FORCE_FOOLSCAP=0 tox -e integration - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v1 diff --git a/integration/util.py b/integration/util.py index ad9249e45..cde837218 100644 --- a/integration/util.py +++ b/integration/util.py @@ -1,14 +1,6 @@ """ Ported to Python 3. """ -from __future__ import unicode_literals -from __future__ import absolute_import -from __future__ import division -from __future__ import print_function - -from future.utils import PY2 -if PY2: - from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 import sys import time @@ -38,6 +30,7 @@ from allmydata.util.configutil import ( write_config, ) from allmydata import client +from allmydata.testing import foolscap_only_for_integration_testing import pytest_twisted @@ -300,6 +293,14 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam u'log_gatherer.furl', flog_gatherer, ) + force_foolscap = foolscap_only_for_integration_testing() + if force_foolscap is not None: + set_config( + config, + 'storage', + 'force_foolscap', + str(force_foolscap), + ) write_config(FilePath(config_path), config) created_d.addCallback(created) diff --git a/newsfragments/3937.minor b/newsfragments/3937.minor new file mode 100644 index 000000000..e69de29bb diff --git a/src/allmydata/protocol_switch.py b/src/allmydata/protocol_switch.py index b0af84c33..d88863fdb 100644 --- a/src/allmydata/protocol_switch.py +++ b/src/allmydata/protocol_switch.py @@ -30,6 +30,7 @@ from foolscap.api import Tub from .storage.http_server import HTTPServer, build_nurl from .storage.server import StorageServer +from .testing import foolscap_only_for_integration_testing class _PretendToBeNegotiation(type): @@ -170,6 +171,21 @@ class _FoolscapOrHttps(Protocol, metaclass=_PretendToBeNegotiation): # and later data, otherwise assume HTTPS. self._timeout.cancel() if self._buffer.startswith(b"GET /id/"): + if foolscap_only_for_integration_testing() == False: + # Tahoe will prefer HTTP storage protocol over Foolscap when possible. + # + # If this is branch is taken, we are running a test that should + # be using HTTP for the storage protocol. As such, we + # aggressively disable Foolscap to ensure that HTTP is in fact + # going to be used. If we hit this branch that means our + # expectation that HTTP will be used was wrong, suggesting a + # bug in either the code of the integration testing setup. + # + # This branch should never be hit in production! + self.transport.loseConnection() + print("FOOLSCAP IS DISABLED, I PITY THE FOOLS WHO SEE THIS MESSAGE") + return + # We're a Foolscap Negotiation server protocol instance: transport = self.transport buf = self._buffer diff --git a/src/allmydata/testing/__init__.py b/src/allmydata/testing/__init__.py index e69de29bb..119ae4101 100644 --- a/src/allmydata/testing/__init__.py +++ b/src/allmydata/testing/__init__.py @@ -0,0 +1,18 @@ +import os +from typing import Optional + + +def foolscap_only_for_integration_testing() -> Optional[bool]: + """ + Return whether HTTP storage protocol has been disabled / Foolscap + forced, for purposes of integration testing. + + This is determined by the __TAHOE_INTEGRATION_FORCE_FOOLSCAP environment + variable, which can be 1, 0, or not set, corresponding to results of + ``True``, ``False`` and ``None`` (i.e. default). + """ + force_foolscap = os.environ.get("__TAHOE_INTEGRATION_FORCE_FOOLSCAP") + if force_foolscap is None: + return None + + return bool(int(force_foolscap)) From 7afd821efc826f2ee644ed85369b0bc6b8dbb482 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Wed, 16 Nov 2022 13:28:26 -0500 Subject: [PATCH 02/10] Sigh --- src/allmydata/test/test_storage_https.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/allmydata/test/test_storage_https.py b/src/allmydata/test/test_storage_https.py index bacb40290..a9421c3e5 100644 --- a/src/allmydata/test/test_storage_https.py +++ b/src/allmydata/test/test_storage_https.py @@ -179,6 +179,10 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") + # We keep getting TLSMemoryBIOProtocol being left around, so try harder + # to wait for it to finish. + await deferLater(reactor, 0.01) + @async_to_deferred async def test_server_certificate_not_valid_yet(self): """ From 4c8e8a74a4920359617bb4471f97eb3817eed37a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Thu, 17 Nov 2022 12:25:37 -0500 Subject: [PATCH 03/10] Not needed. --- src/allmydata/test/test_storage_https.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/allmydata/test/test_storage_https.py b/src/allmydata/test/test_storage_https.py index a9421c3e5..88435bf89 100644 --- a/src/allmydata/test/test_storage_https.py +++ b/src/allmydata/test/test_storage_https.py @@ -202,10 +202,6 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") - # We keep getting TLSMemoryBIOProtocol being left around, so try harder - # to wait for it to finish. - await deferLater(reactor, 0.001) - # A potential attack to test is a private key that doesn't match the # certificate... but OpenSSL (quite rightly) won't let you listen with that # so I don't know how to test that! See From 98e25507df5fdde29b5047c7d325607cb3906b5a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:43:36 -0500 Subject: [PATCH 04/10] A different approach to forcing foolscap in integration tests. --- .github/workflows/ci.yml | 27 +++++++++++++-------------- integration/conftest.py | 16 +++++++--------- integration/util.py | 17 ++++++++--------- src/allmydata/protocol_switch.py | 16 ---------------- src/allmydata/testing/__init__.py | 18 ------------------ 5 files changed, 28 insertions(+), 66 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 41de7baed..afbe5c7a4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -161,19 +161,21 @@ jobs: strategy: fail-fast: false matrix: - os: - - windows-latest - # 22.04 has some issue with Tor at the moment: - # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943 - - ubuntu-20.04 - python-version: - - 3.7 - - 3.9 include: - # On macOS don't bother with 3.7, just to get faster builds. - os: macos-latest python-version: 3.9 - + extra-tox-options: "" + - os: windows-latest + python-version: 3.10 + extra-tox-options: "" + # 22.04 has some issue with Tor at the moment: + # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943 + - os: ubuntu-20.04 + python-version: 3.8 + extra-tox-options: "--force-foolscap integration/" + - os: ubuntu-20.04 + python-version: 3.10 + extra-tox-options: "" steps: - name: Install Tor [Ubuntu] @@ -232,10 +234,7 @@ jobs: # effect. On Linux it doesn't make a difference one way or another. TMPDIR: "/tmp" run: | - # Run with Foolscap forced: - __TAHOE_INTEGRATION_FORCE_FOOLSCAP=1 tox -e integration - # Run with Foolscap not forced, which should result in HTTP being used. - __TAHOE_INTEGRATION_FORCE_FOOLSCAP=0 tox -e integration + tox -e integration ${{ matrix.extra-tox-options }} - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v1 diff --git a/integration/conftest.py b/integration/conftest.py index e284b5cba..5cbe9ad6b 100644 --- a/integration/conftest.py +++ b/integration/conftest.py @@ -1,15 +1,6 @@ """ Ported to Python 3. """ -from __future__ import unicode_literals -from __future__ import absolute_import -from __future__ import division -from __future__ import print_function - -from future.utils import PY2 -if PY2: - from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401 - import sys import shutil from time import sleep @@ -66,6 +57,13 @@ def pytest_addoption(parser): "--coverage", action="store_true", dest="coverage", help="Collect coverage statistics", ) + parser.addoption( + "--force-foolscap", action="store_true", default=False, + dest="force_foolscap", + help=("If set, force Foolscap only for the storage protocol. " + + "Otherwise HTTP will be used.") + ) + @pytest.fixture(autouse=True, scope='session') def eliot_logging(): diff --git a/integration/util.py b/integration/util.py index cde837218..7d885ee6c 100644 --- a/integration/util.py +++ b/integration/util.py @@ -30,7 +30,6 @@ from allmydata.util.configutil import ( write_config, ) from allmydata import client -from allmydata.testing import foolscap_only_for_integration_testing import pytest_twisted @@ -293,14 +292,14 @@ def _create_node(reactor, request, temp_dir, introducer_furl, flog_gatherer, nam u'log_gatherer.furl', flog_gatherer, ) - force_foolscap = foolscap_only_for_integration_testing() - if force_foolscap is not None: - set_config( - config, - 'storage', - 'force_foolscap', - str(force_foolscap), - ) + force_foolscap = request.config.getoption("force_foolscap") + assert force_foolscap in (True, False) + set_config( + config, + 'storage', + 'force_foolscap', + str(force_foolscap), + ) write_config(FilePath(config_path), config) created_d.addCallback(created) diff --git a/src/allmydata/protocol_switch.py b/src/allmydata/protocol_switch.py index d88863fdb..b0af84c33 100644 --- a/src/allmydata/protocol_switch.py +++ b/src/allmydata/protocol_switch.py @@ -30,7 +30,6 @@ from foolscap.api import Tub from .storage.http_server import HTTPServer, build_nurl from .storage.server import StorageServer -from .testing import foolscap_only_for_integration_testing class _PretendToBeNegotiation(type): @@ -171,21 +170,6 @@ class _FoolscapOrHttps(Protocol, metaclass=_PretendToBeNegotiation): # and later data, otherwise assume HTTPS. self._timeout.cancel() if self._buffer.startswith(b"GET /id/"): - if foolscap_only_for_integration_testing() == False: - # Tahoe will prefer HTTP storage protocol over Foolscap when possible. - # - # If this is branch is taken, we are running a test that should - # be using HTTP for the storage protocol. As such, we - # aggressively disable Foolscap to ensure that HTTP is in fact - # going to be used. If we hit this branch that means our - # expectation that HTTP will be used was wrong, suggesting a - # bug in either the code of the integration testing setup. - # - # This branch should never be hit in production! - self.transport.loseConnection() - print("FOOLSCAP IS DISABLED, I PITY THE FOOLS WHO SEE THIS MESSAGE") - return - # We're a Foolscap Negotiation server protocol instance: transport = self.transport buf = self._buffer diff --git a/src/allmydata/testing/__init__.py b/src/allmydata/testing/__init__.py index 119ae4101..e69de29bb 100644 --- a/src/allmydata/testing/__init__.py +++ b/src/allmydata/testing/__init__.py @@ -1,18 +0,0 @@ -import os -from typing import Optional - - -def foolscap_only_for_integration_testing() -> Optional[bool]: - """ - Return whether HTTP storage protocol has been disabled / Foolscap - forced, for purposes of integration testing. - - This is determined by the __TAHOE_INTEGRATION_FORCE_FOOLSCAP environment - variable, which can be 1, 0, or not set, corresponding to results of - ``True``, ``False`` and ``None`` (i.e. default). - """ - force_foolscap = os.environ.get("__TAHOE_INTEGRATION_FORCE_FOOLSCAP") - if force_foolscap is None: - return None - - return bool(int(force_foolscap)) From c5c616afd5146f8cde9dddead3bdbeb092890992 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:44:49 -0500 Subject: [PATCH 05/10] Garbage. --- src/allmydata/test/test_storage_https.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/allmydata/test/test_storage_https.py b/src/allmydata/test/test_storage_https.py index 3d2a31143..a11b0eed5 100644 --- a/src/allmydata/test/test_storage_https.py +++ b/src/allmydata/test/test_storage_https.py @@ -181,10 +181,6 @@ class PinningHTTPSValidation(AsyncTestCase): response = await self.request(url, certificate) self.assertEqual(await response.content(), b"YOYODYNE") - # We keep getting TLSMemoryBIOProtocol being left around, so try harder - # to wait for it to finish. - await deferLater(reactor, 0.01) - @async_to_deferred async def test_server_certificate_not_valid_yet(self): """ From 106df423be0e864100ba2d96cdb91558d206160a Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:52:01 -0500 Subject: [PATCH 06/10] Another approach. --- .github/workflows/ci.yml | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2ffc260df..8c3eaf29e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -156,18 +156,18 @@ jobs: include: - os: macos-latest python-version: 3.9 - extra-tox-options: "" + force-foolscap: false - os: windows-latest python-version: 3.10 - extra-tox-options: "" + force-foolscap: false # 22.04 has some issue with Tor at the moment: # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943 - os: ubuntu-20.04 python-version: 3.8 - extra-tox-options: "--force-foolscap integration/" + force-foolscap: true - os: ubuntu-20.04 python-version: 3.10 - extra-tox-options: "" + force-foolscap: false steps: - name: Install Tor [Ubuntu] @@ -208,14 +208,24 @@ jobs: run: python misc/build_helpers/show-tool-versions.py - name: Run "Python 3 integration tests" + if: "${{ !matrix.force-foolscap }}" env: # On macOS this is necessary to ensure unix socket paths for tor # aren't too long. On Windows tox won't pass it through so it has no # effect. On Linux it doesn't make a difference one way or another. TMPDIR: "/tmp" run: | - tox -e integration ${{ matrix.extra-tox-options }} + tox -e integration + - name: Run "Python 3 integration tests (force Foolscap)" + if: "${{ matrix.force-foolscap }}" + env: + # On macOS this is necessary to ensure unix socket paths for tor + # aren't too long. On Windows tox won't pass it through so it has no + # effect. On Linux it doesn't make a difference one way or another. + TMPDIR: "/tmp" + run: | + tox -e integration -- --force-foolscap integration/ - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v3 if: failure() From 742b352861629a0d1f1b900c4c71d6e1ba22a0f2 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:52:17 -0500 Subject: [PATCH 07/10] Whitespace. --- .github/workflows/ci.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c3eaf29e..e87337a1b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -226,6 +226,7 @@ jobs: TMPDIR: "/tmp" run: | tox -e integration -- --force-foolscap integration/ + - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v3 if: failure() From d05a1313d1773d8f4bf7041d51f4bca1ab0de4b3 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:54:23 -0500 Subject: [PATCH 08/10] Don't change versions for now, use strings so it'll be future compatible with 3.10. --- .github/workflows/ci.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e87337a1b..01f0890da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -155,18 +155,18 @@ jobs: matrix: include: - os: macos-latest - python-version: 3.9 + python-version: "3.9" force-foolscap: false - os: windows-latest - python-version: 3.10 + python-version: "3.9" force-foolscap: false # 22.04 has some issue with Tor at the moment: # https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3943 - os: ubuntu-20.04 - python-version: 3.8 + python-version: "3.7" force-foolscap: true - os: ubuntu-20.04 - python-version: 3.10 + python-version: "3.9" force-foolscap: false steps: From 366cbf90017874ec24d60bd0df3b3d9f75d79182 Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 10:55:07 -0500 Subject: [PATCH 09/10] Tox is bad? --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01f0890da..37f41d06b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -225,7 +225,7 @@ jobs: # effect. On Linux it doesn't make a difference one way or another. TMPDIR: "/tmp" run: | - tox -e integration -- --force-foolscap integration/ + tox -e integration -- --force-foolscap,integration/ - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v3 From 6a1f49551b6683f64b110c2060dcd309c21bdd8d Mon Sep 17 00:00:00 2001 From: Itamar Turner-Trauring Date: Mon, 12 Dec 2022 11:05:09 -0500 Subject: [PATCH 10/10] No, that's not it. --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 37f41d06b..01f0890da 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -225,7 +225,7 @@ jobs: # effect. On Linux it doesn't make a difference one way or another. TMPDIR: "/tmp" run: | - tox -e integration -- --force-foolscap,integration/ + tox -e integration -- --force-foolscap integration/ - name: Upload eliot.log in case of failure uses: actions/upload-artifact@v3