acc: add rejecting proxy for local tests#5483
Merged
Merged
Conversation
Collaborator
|
Commit: a26f59d
22 interesting tests: 15 SKIP, 7 KNOWN
Top 29 slowest tests (at least 2 minutes):
|
denik
changed the title
denik
changed the title
Installs a per-test blocking HTTP proxy for all Local=true acceptance tests. Unintentional outbound internet access now fails fast with a clear diagnostic instead of silently hanging or timing out. The proxy listens on a random loopback port and responds with HTTP 400 (not a TCP RST) so the Databricks SDK's httpclient doesn't treat the failure as a retriable IO error (ECONNREFUSED triggers a 5-minute retry loop). For each blocked request it calls t.Errorf with method, host, and User-Agent so the failure is attributed to the right test. Exemptions (t.Logf only, no test failure): - RFC 2606 §2 reserved TLDs (.test, .example, .invalid) — intentional unreachable fixtures used in negative test cases - Loopback IPs (127.x.x.x, ::1) — the Terraform provider routes its HTTP requests to the local test server through HTTPS_PROXY even though Go's standard library skips the proxy for loopback destinations Other changes in this commit: - HTTPS_PROXY only (not HTTP_PROXY): the local test server is plain HTTP, so setting HTTP_PROXY would intercept its traffic - NO_PROXY=127.0.0.1,localhost: Python's urllib doesn't auto-bypass the proxy for loopback addresses, so the helper scripts need this exemption - CHECKPOINT_DISABLE=1: stops Terraform from phoning home to checkpoint-api.hashicorp.com on every terraform-engine test - Replace real Databricks hostnames with .test TLD equivalents in six auth test fixtures that were accidentally relying on internet access - bundle/deploy/mlops-stacks: Local=false (clones from github.com) - bundle/templates-machinery/wrong-url: use .invalid TLD Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
- wrong-url/test.toml: update replacement rules for the .invalid TLD (old rules referenced the old .com URL); normalize proxy error to a stable "(redacted)" form instead of relying on system-specific paths - cmd/auth/profiles/script: sort hostmetadata warning lines before writing to stdout so the golden file is stable across concurrent profile loading (warnings appeared in non-deterministic order because profiles are loaded in parallel goroutines) - blocking_proxy.go: use fmt.Fprintf instead of []byte(fmt.Sprintf) (modernize linter fix) Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
Previously a new TCP listener was started per test. Now a single shared listener is created lazily via sync.OnceValue; errors are attributed to the top-level TestAccept t. Pass -debugsandbox to get a per-test proxy that attributes failures to the exact subtest. Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
Replace SetupSharedProxy + StartBlockingProxy with a single StartBlockingProxy(t, hint). The caller decides scope: testAccept passes the top-level t (shared for the run) and a hint suggesting -debugsandbox; runTest passes its own subtest t when -debugsandbox is set. This removes the sync.OnceValue / atomic.Pointer machinery. Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
Co-authored-by: Denis Bilenko <denis.bilenko@databricks.com>
denik
force-pushed
the
denik/internet-sandbo
branch
from
c213591 to
a26f59d
Compare
denik
requested review from
andrewnester,
janniklasrose,
pietern and
shreyas-goenka
janniklasrose
approved these changes
Jun 11, 2026
Comment on lines
-20
to
+28
| $CLI auth profiles --skip-validate --output json | ||
| # auth profiles loads profiles concurrently so hostmetadata warnings appear in | ||
| # non-deterministic order; sort them for a stable golden file. | ||
| $CLI auth profiles --skip-validate --output json 2>&1 | python3 -c " | ||
| import sys | ||
| lines = sys.stdin.readlines() | ||
| meta = sorted(l for l in lines if '[hostmetadata]' in l) | ||
| rest = [l for l in lines if '[hostmetadata]' not in l] | ||
| sys.stdout.writelines(meta + rest) | ||
| " |
Contributor
There was a problem hiding this comment.
This seems a redundant change. I thought --skip-validate doesn't make network calls? Also there's no hostmetadata warning in the output.txt
pietern
reviewed
Jun 11, 2026
| } | ||
| // Only block HTTPS: the local test server is plain HTTP (http://127.0.0.1:PORT) | ||
| // so HTTP_PROXY would intercept its traffic. All real external calls use HTTPS. | ||
| cmd.Env = append(cmd.Env, "HTTPS_PROXY="+proxyURL) |
Contributor
There was a problem hiding this comment.
I think we should set HTTP_PROXY as well.NO_PROXY below will avoid intercepting testserver calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Installs a per-test blocking HTTP proxy for all
Local=trueacceptance tests. Unintentional outbound internet access now fails fast with a clear diagnostic (method, host, User-Agent) instead of silently hanging or timing out.How it works:
127.0.0.1:0and returnsHTTP 400 Bad Requestto every requestt.Errorfis called for real external hosts, so the test is immediately marked as failed with a useful messaget.Logf(no failure) is used for RFC 2606 reserved TLDs (.test,.example,.invalid) and loopback IPs — these are intentional unreachable fixtures or the Terraform provider talking to the local test serverProxy settings injected for local runs:
HTTPS_PROXY=<blocking proxy>— only HTTPS, because the local test server is plain HTTPNO_PROXY=127.0.0.1,localhost— Python'surllib(used by helper scripts) doesn't auto-bypass loopbackCHECKPOINT_DISABLE=1— stops Terraform from phoning home tocheckpoint-api.hashicorp.comTest fixture fixes (tests that were accidentally relying on real internet access):
accounts.cloud.databricks.com,unified.databricks.com,non.existing.subdomain.databricks.com) with.testTLD equivalents per the RFC 2606 repo conventionbundle/deploy/mlops-stacks: setLocal=false— it clones fromgithub.com/databricks/mlops-stacksbundle/templates-machinery/wrong-url: switch from.comto.invalidTLDTest plan
go test ./acceptance -count=1passes locally (pre-existing flaky tests in parallel runs are unrelated — they also fail without this change)This pull request and its description were written by Isaac.