Skip to content

SNOW-2912540: add mitmproxy capture fixture base - #4322

Draft
sfc-gh-fpawlowski wants to merge 9 commits into
SNOW-2912540-log-batch-mock-patchfrom
SNOW-2912540-mitmproxy-capture-fixture
Draft

SNOW-2912540: add mitmproxy capture fixture base#4322
sfc-gh-fpawlowski wants to merge 9 commits into
SNOW-2912540-log-batch-mock-patchfrom
SNOW-2912540-mitmproxy-capture-fixture

Conversation

@sfc-gh-fpawlowski

Copy link
Copy Markdown
  1. Which Jira issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

    Fixes SNOW-NNNNNNN

  2. Fill out the following pre-review checklist:

    • I am adding a new automated test(s) to verify correctness of my new code
      • If this test skips Local Testing mode, I'm requesting review from @snowflakedb/local-testing
    • I am adding new logging messages
    • I am adding a new telemetry message
    • I am adding new credentials
    • I am adding a new dependency
    • If this is a new feature/behavior, I'm adding the Local Testing parity changes.
    • I acknowledge that I have ensured my changes to be thread-safe. Follow the link for more information: Thread-safe Developer Guidelines
    • If adding any arguments to public Snowpark APIs or creating new public Snowpark APIs, I acknowledge that I have ensured my changes include AST support. Follow the link for more information: AST Support Guidelines
  3. Please describe how your code solves the related issue.

    Please write a short description of how your code change solves the related issue.

sfc-gh-fpawlowski commented Aug 20, 2026

Copy link
Copy Markdown
Author

@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-capture-fixture branch from 3d8fa0d to e0d3906 Compare August 20, 2026 17:14
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-log-batch-mock-patch branch from a450af0 to dc6bd66 Compare August 20, 2026 17:14
@sfc-gh-fpawlowski
sfc-gh-fpawlowski changed the base branch from SNOW-2912540-log-batch-mock-patch to graphite-base/4322 August 20, 2026 17:15
@codecov-commenter

codecov-commenter commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.23%. Comparing base (2257b69) to head (7a6d3eb).

Additional details and impacted files
@@                          Coverage Diff                          @@
##           SNOW-2912540-log-batch-mock-patch    #4322      +/-   ##
=====================================================================
- Coverage                              95.44%   95.23%   -0.21%     
=====================================================================
  Files                                    172      172              
  Lines                                  44771    44771              
  Branches                                7682     7682              
=====================================================================
- Hits                                   42730    42638      -92     
- Misses                                  1264     1311      +47     
- Partials                                 777      822      +45     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-capture-fixture branch from d7af463 to 9084ff3 Compare August 21, 2026 08:35
@sfc-gh-fpawlowski
sfc-gh-fpawlowski changed the base branch from graphite-base/4322 to SNOW-2912540-log-batch-mock-patch August 21, 2026 08:35
@sfc-gh-fpawlowski
sfc-gh-fpawlowski changed the base branch from SNOW-2912540-log-batch-mock-patch to graphite-base/4322 August 21, 2026 09:37
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-capture-fixture branch from aa3e36c to a45ae31 Compare August 21, 2026 09:45
@sfc-gh-fpawlowski
sfc-gh-fpawlowski changed the base branch from graphite-base/4322 to SNOW-2912540-log-batch-mock-patch August 21, 2026 09:45
Passive HTTPS-capture test infrastructure so telemetry integration tests
can verify what Snowpark actually sends over the wire without mocking any
backend response. tests/mitmproxy_client.py runs mitmdump as a real forward
proxy (traffic reaches its real destination untouched); tests/mitmproxy_addon.py
records every decrypted request to a JSON-lines file the client reads via
get_requests()/wait_for_requests(), mirroring the shape of the connector
world's WiremockClient. New conftest.py fixtures (_mitmproxy_session,
mitmproxy, mitmproxy_session) wire proxy_host/proxy_port and REQUESTS_CA_BUNDLE
into a real session/connection. test_mitmproxy_fixture.py proves the whole
path end-to-end before the telemetry tests depend on it.
Dict/List/Optional are genuinely used (return-type and attribute
annotations under `from __future__ import annotations`), but this repo's
pinned flake8==5.0.4/pyflakes doesn't detect usage within these specific
annotation forms -- confirmed by comparing against
_internal/code_generation.py, the only other file combining `from
__future__ import annotations` with typing generics, which only uses them
in plain parameter/local-variable annotations, never return-type generics
like this file does.
Superseded my own previous fix attempt (noqa: F401) -- that treated the
wrong symptom. Reproduced the actual CI failure locally: pyupgrade (which
runs before flake8 in this repo's pre-commit chain) rewrites Optional[X]/
List[X]/Dict[X] to X | None/list[X]/dict[X] and drops string forward-refs,
since from __future__ import annotations makes both unnecessary here. Any
file modification during pre-commit's CI check run fails the job regardless
of what flake8 itself would say, so the noqa comment never addressed the
actual failure. Verified clean with a full local `pre-commit run --all-files`.
tests/mitmproxy_client.py requires the mitmdump binary on PATH; without it,
CI fails every test depending on the mitmproxy fixture with
"RuntimeError: mitmdump not found on PATH."
…ease

Unpinned "mitmproxy" resolved to 0.15 in CI, whose transitive urwid<1.4
dependency fails to build under modern setuptools (use_2to3 is invalid).
Match the version range the universal-driver's own CI already pins for the
same tool (mitmproxy>=11,<13).
This file lives under tests/, so pytest's doctest/AST-scanning jobs import
it as part of their broad module walk (regardless of test-file naming
conventions), executing the module-level os.environ[...] access outside
the mitmdump subprocess context where that var is actually set --
failing collection with KeyError across every "Doctest AST"/"Test AST
Encoding" job. Defer the lookup to when the hook actually fires.
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-mitmproxy-capture-fixture branch from a45ae31 to 8f91d4a Compare August 21, 2026 09:59
@sfc-gh-fpawlowski
sfc-gh-fpawlowski force-pushed the SNOW-2912540-log-batch-mock-patch branch from 98c0936 to 2257b69 Compare August 21, 2026 09:59
…e's teardown

A bad hunk placement in the mitmproxy_session fixture's initial commit moved
this fixture's teardown tail (clean_up_external_access_integration_resources()
+ session.close()) into mitmproxy_session's finally block, where `session` was
undefined. The later NameError fix removed the dangling reference but never
restored it here, so `session` (module-scoped, used by nearly every
integration test) never actually closed. Since Session.close() is the only
thing that removes a session from the process-wide active-session registry,
every module leaked one session per xdist worker -- once a worker crossed two
leaked sessions, any bare udf()/getOrCreate() call raised
MORE_THAN_ONE_ACTIVE_SESSIONS for the rest of that worker's queue.
mitmproxy re-signs TLS traffic with its own CA-issued leaf certificate to
decrypt and observe it. The connector's revocation check treats that
certificate as genuinely revoked/unvalidatable (254007), not as an
unreachable-responder case ocsp_fail_open would paper over, so every
mitmproxy_session connection fails outright: "Could not connect to
Snowflake backend after 11 attempt(s)." Disable revocation checking for
this proxy-routed session only -- it never touches a real leaf cert, so
there is nothing for OCSP/CRL to meaningfully validate.
…mproxy_session

The bad hunk placement from mitmproxy_session's initial commit (fixed for
try_add_log_to_batch's teardown by restoring session.close() to the `session`
fixture in the previous commit) left a stale, still-broken copy of that same
teardown tail inside mitmproxy_session's own finally block. There, `session`
resolves to the module-level `session` fixture *function* (mitmproxy_session
has no local named `session`), so tearing down any mitmproxy_session-based
test raised AttributeError: 'function' object has no attribute 'close'.
mitmproxy_session already closes its own proxied_session two lines above;
this block never belonged here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants