test(providers): serialize providers_v2 mutation with a suite-wide guard

The clone e2e's per-fixture lock only coordinated fixtures that acquired
it; other xdist workers hit the same gateway without it and could
observe the transiently-enabled providers_v2_enabled global during their
own sandbox creation (CWE-362).

Add an autouse readers-writer guard in conftest: every test holds a
shared lock on the gateway config, and a test marked
exclusive_gateway_config holds an exclusive lock. Mark the clone test
exclusive so no other worker is mid-test while it enables and restores
the gateway-global setting. Exact prior-value restoration is retained.

Refs #1769

Signed-off-by: Russell Bryant <rbryant@redhat.com>

Author: russellb

e2e/python/conftest.py

@@ -3,6 +3,7 @@
 
 from __future__ import annotations
 
+import fcntl
 import os
 import time
 from typing import TYPE_CHECKING
@@ -16,6 +17,38 @@
     from collections.abc import Callable, Iterator
 
 
+def pytest_configure(config: pytest.Config) -> None:
+    config.addinivalue_line(
+        "markers",
+        "exclusive_gateway_config: hold an exclusive lock on gateway-global "
+        "config so no other xdist worker observes a transient global setting",
+    )
+
+
+@pytest.fixture(autouse=True)
+def _gateway_config_guard(
+    request: pytest.FixtureRequest,
+    tmp_path_factory: pytest.TempPathFactory,
+) -> Iterator[None]:
+    """Readers-writer guard over gateway-global config mutations.
+
+    The e2e gateway is shared across xdist workers, so a test that flips a
+    gateway-global setting can leak that transient value into another worker's
+    sandbox creation. Every test holds a shared lock by default; a test marked
+    ``exclusive_gateway_config`` holds an exclusive lock, so while it mutates and
+    restores the setting no other worker is mid-test and none can observe the
+    transient value. The lock file lives in the run's shared base temp dir
+    (``getbasetemp().parent``), which is common to all xdist workers.
+    """
+    lock_path = tmp_path_factory.getbasetemp().parent / "gateway-config.lock"
+    exclusive = (
+        request.node.get_closest_marker("exclusive_gateway_config") is not None
+    )
+    with lock_path.open("w") as lock_file:
+        fcntl.flock(lock_file, fcntl.LOCK_EX if exclusive else fcntl.LOCK_SH)
+        yield
+
+
 @pytest.fixture(scope="session")
 def cluster_name() -> str | None:
     return os.environ.get("OPENSHELL_GATEWAY")

e2e/python/test_sandbox_providers.py

@@ -11,7 +11,6 @@
 
 from __future__ import annotations
 
-import fcntl
 import time
 from contextlib import contextmanager
 from typing import TYPE_CHECKING
@@ -103,69 +102,63 @@ def _delete_provider(stub: object, name: str) -> None:
 @pytest.fixture
 def providers_v2_enabled(
     sandbox_client: SandboxClient,
-    tmp_path_factory: pytest.TempPathFactory,
+    _gateway_config_guard: None,
 ) -> Iterator[None]:
     """Enable the gateway-global ``providers_v2_enabled`` opt-in for one test.
 
     Composing a provider's network policy onto a sandbox is gated behind this
     setting, which defaults off; the built-in github profile's git-transport
     rules only reach the sandbox with it enabled.
 
-    The setting is gateway-global, so this mutates shared state. To keep it safe
-    on an existing or shared gateway and under parallel workers:
-
-    - The mutation is serialized across xdist workers with an exclusive file lock
-      on the run's shared base temp dir (``getbasetemp().parent``), held for the
-      whole test so the read-modify-restore cannot interleave.
-    - The exact prior value (or absence) is captured via ``GetGatewayConfig`` and
-      restored in ``finally``, leaving the gateway as it was found.
-
-    ``global`` is a Python keyword, so it is passed through a dict expansion.
+    The setting is gateway-global. Exclusivity against other xdist workers is
+    provided by the ``exclusive_gateway_config`` marker plus the autouse
+    ``_gateway_config_guard`` guard (see conftest): no concurrent worker is
+    mid-test while this fixture mutates and restores the setting, so none can
+    observe the transient value. Depending on the guard here also orders the
+    exclusive lock acquisition before the mutation.
+
+    ``GetGatewayConfig`` returns known keys even when unset, with an empty
+    ``SettingValue`` (no populated oneof), so the setting is treated as present
+    only when its value oneof is set; otherwise restore is a delete. ``global``
+    is a Python keyword, so it is passed through a dict expansion.
     """
     stub = sandbox_client._stub
     key = "providers_v2_enabled"
-    lock_path = tmp_path_factory.getbasetemp().parent / "providers-v2-setting.lock"
-    with lock_path.open("w") as lock_file:
-        fcntl.flock(lock_file, fcntl.LOCK_EX)
-
-        # GetGatewayConfig returns known keys even when unset, with an empty
-        # SettingValue (no populated oneof). Only treat the setting as explicitly
-        # present when its value oneof is actually set; otherwise restore = delete.
-        config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest())
-        prior_value = sandbox_pb2.SettingValue()
-        had_prior = (
-            key in config.settings
-            and config.settings[key].WhichOneof("value") is not None
+    config = stub.GetGatewayConfig(sandbox_pb2.GetGatewayConfigRequest())
+    prior_value = sandbox_pb2.SettingValue()
+    had_prior = (
+        key in config.settings
+        and config.settings[key].WhichOneof("value") is not None
+    )
+    if had_prior:
+        prior_value.CopyFrom(config.settings[key])
+
+    stub.UpdateConfig(
+        openshell_pb2.UpdateConfigRequest(
+            setting_key=key,
+            setting_value=sandbox_pb2.SettingValue(bool_value=True),
+            **{"global": True},
         )
+    )
+    try:
+        yield
+    finally:
         if had_prior:
-            prior_value.CopyFrom(config.settings[key])
-
-        stub.UpdateConfig(
-            openshell_pb2.UpdateConfigRequest(
-                setting_key=key,
-                setting_value=sandbox_pb2.SettingValue(bool_value=True),
-                **{"global": True},
-            )
-        )
-        try:
-            yield
-        finally:
-            if had_prior:
-                stub.UpdateConfig(
-                    openshell_pb2.UpdateConfigRequest(
-                        setting_key=key,
-                        setting_value=prior_value,
-                        **{"global": True},
-                    )
+            stub.UpdateConfig(
+                openshell_pb2.UpdateConfigRequest(
+                    setting_key=key,
+                    setting_value=prior_value,
+                    **{"global": True},
                 )
-            else:
-                stub.UpdateConfig(
-                    openshell_pb2.UpdateConfigRequest(
-                        setting_key=key,
-                        delete_setting=True,
-                        **{"global": True},
-                    )
+            )
+        else:
+            stub.UpdateConfig(
+                openshell_pb2.UpdateConfigRequest(
+                    setting_key=key,
+                    delete_setting=True,
+                    **{"global": True},
                 )
+            )
 
 
 # ===========================================================================
@@ -560,6 +553,7 @@ def test_update_provider_rejects_type_change(
 # ===========================================================================
 
 
+@pytest.mark.exclusive_gateway_config
 @pytest.mark.usefixtures("providers_v2_enabled")
 def test_github_provider_allows_https_git_clone(
     sandbox: Callable[..., Sandbox],