fix(sdk/python): stop probing cloud metadata / ipify when a callback URL is configured, add AGENTFIELD_DISABLE_IP_DETECTION - #969
Merged
Conversation
Constructing an Agent() inside a container made _build_callback_candidates() call _detect_container_ip(), which fires HTTP requests at 169.254.169.254, metadata.google.internal and https://api.ipify.org. It did so even when the operator had already told the SDK where the control plane should call back, so on Kubernetes every agent start produced a burst of NetworkPolicy deny entries for egress nobody had asked for. The probe now only runs when it can still contribute something: - A callback URL configured through the `callback_url` constructor argument or AGENT_CALLBACK_URL suppresses it. Such a URL always normalizes to the first candidate and the control plane keeps the remaining candidates as fallbacks, so the detected public IP could never have been selected. A value that fails to normalize is not treated as configured, so malformed input still falls back to full auto-detection. - AGENTFIELD_DISABLE_IP_DETECTION=1 (also `true`/`yes`, case-insensitive, surrounding whitespace tolerated) suppresses it unconditionally, for clusters that want the guarantee without pinning a URL. Everything else is untouched: the Railway internal hostname, the local network address, the container hostname, host.docker.internal and the localhost fallbacks are still offered, the probe still never runs outside a container, and with neither knob set the candidate list is what it was. Fixes #624 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Records the new opt-out under the Python SDK agents section, along with the fact that configuring a callback URL already suppresses the probe and which callback candidates remain when it is off. Refs #624 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… callback URL is configured The existing opt-out tests stub `_detect_container_ip` itself, so they assert that the helper is not called. The symptom reported in #624 is one level lower: no HTTP request should leave the process. Those tests would still pass if the probe grew a second outbound call outside the helper. Add a test that leaves `_detect_container_ip` in place and puts a tripwire on `requests.get` — the helper's only outbound entry point, since it imports `requests` lazily and makes no other network call. It runs with `_is_running_in_container` forced True and covers all three configured forms: the `callback_url` argument, `AGENT_CALLBACK_URL`, and `AGENTFIELD_DISABLE_IP_DETECTION`. The tripwire records each attempt as well as raising `AssertionError`, because `_detect_container_ip` wraps every request in `except Exception: pass` and would swallow the raise. Verified against a locally reverted fix: all three parameters fail, and the recorded list shows all four probe targets attempted with the raise absorbed — so the recorded list, not the raise, is what gives the test teeth. A control test patches `requests.get` with a fake 200 response and asserts the probe does reach it when nothing is configured, pinning down that the tripwire would fire if the skip regressed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
gtg from my side |
Contributor
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
Contributor
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
Contributor
Performance
✓ No regressions detected |
The entry said the variable "is for clusters that want the guarantee without pinning a URL", which overstates what it changes today. Reading the SDK: _detect_container_ip() has exactly one caller (_build_callback_candidates), and in a running agent that function is only reached when the Agent was constructed with callback_url=... — Agent.__init__ resolves the callback URL eagerly only in that case, and AgentServer.serve() derives base_url itself (honouring AGENT_CALLBACK_URL directly) without touching callback discovery. So a stock agent with no callback URL never reaches the probe with or without the flag. Rewrite the entry so every sentence holds: what it disables (the probe at its single call site, container-only), when someone would actually set it (a guarantee that survives however the agent is wired, or code calling the discovery helpers directly), what it does NOT disable (the UDP socket.connect() toward 8.8.8.8 used to read the local source address, which sends no packets and does no DNS; registration and heartbeats are unaffected), and what it costs (only the public-IP candidate). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three wording fixes, no behaviour change: - _build_callback_candidates: say that _detect_container_ip is the only step that puts a request on the wire (the local-IP lookup opens a connectionless UDP socket and sends nothing), and that this function is the probe's only call site, which is why AGENTFIELD_DISABLE_IP_DETECTION suppresses it everywhere. - _detect_container_ip: name the single caller instead of "callers are expected to", and mention the opt-out. - Agent.__init__: the previous wording implied AGENT_CALLBACK_URL takes part in the constructor's resolution. It does not — __init__ resolves only when the callback_url argument is supplied. Say that, and describe AGENT_CALLBACK_URL's effect where it actually applies (any caller that goes through callback discovery). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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
Constructing an
Agent()inside a container made_build_callback_candidates()call_detect_container_ip(), which fires HTTP requests at169.254.169.254,metadata.google.internalandhttps://api.ipify.org— even when the operator had already told the SDK exactly where the control plane should call back. On Kubernetes every agent start therefore produced a burst ofNetworkPolicydeny entries for egress nobody asked for (#624).This PR makes the probe run only when it can still contribute something: it is skipped when a callback URL is already configured (the
callback_urlconstructor argument orAGENT_CALLBACK_URL), and a newAGENTFIELD_DISABLE_IP_DETECTIONopt-out skips it unconditionally. With neither knob set the candidate list is exactly what it was before.The diagnosis that
_detect_container_ip()is the source of these requests was worked out earlier by @AmirF194 on #921; the implementation, tests and docs here are written independently.Where the probe is actually reachable
_detect_container_ip()has exactly one caller,_build_callback_candidates(), and that function is reached from three places:_resolve_callback_url(),AgentFieldHandler.register_with_agentfield_server()andAgentFieldHandler.register_with_fast_lifecycle().In a running agent today only the first of those fires, and only in one configuration:
Agent.__init__resolves eagerly (_resolve_callback_url) only when thecallback_url=argument is supplied — exactly the case this PR now skips the probe for.AGENT_CALLBACK_URLinstead, or with neither,AgentServer.serve()buildsbase_urlitself (it readsAGENT_CALLBACK_URLdirectly, else falls back to localhost) and never enters callback discovery.ConnectionManager.connect()then registers with thatbase_url.register_with_agentfield_server()has no production caller (it is used by the SDK's own tests and functional harnesses);register_with_fast_lifecycle()has no caller at all.Two consequences worth being explicit about, because an earlier revision of this description overstated them:
AGENTFIELD_DISABLE_IP_DETECTIONis defence in depth on a stock agent rather than an observable behaviour change: with no callback URL, a stock agent does not reach the probe with or without the flag. The flag gates the probe at its single call site, so it holds for code that calls the discovery helpers directly and it keeps holding if startup is ever re-wired through them. The docs entry in this PR now says that plainly instead of implying that unflagged clusters are probing.This reachability gap is pre-existing and identical on
main; this PR neither causes nor worsens it, and does not try to fix it.What is actually lost when an explicit callback URL is set
Worth stating plainly, because "we stopped collecting a candidate" deserves scrutiny:
_build_callback_candidates()returns a prioritized list and_resolve_callback_url()takescandidates[0]. A configured callback URL is added first (constructor argument ahead ofAGENT_CALLBACK_URL), so the detected public IP was always strictly behind it.gatherCallbackCandidates()→resolveCallbackCandidates()(control-plane/internal/handlers/nodes_register.go) never probes candidates for reachability and always takes the first one:resolveCallbackCandidates()normalizes and de-duplicates the list, returnsnormalized[0], and its[]types.CallbackTestResultreturn is alwaysnil. The remaining candidates are persisted onCallbackDiscovery.Candidatesfor observability only and are never retried against.callback_candidatesis non-empty (_build_callback_discovery_payload()returnsNoneotherwise), andcallback_candidatesis populated only by the two handler registration methods that have no production caller. So a stock agent registers withdiscoveryunset, and the control plane short-circuits on the non-emptyBaseURL. Thepython-sdk:autopath described above applies to callers that go throughregister_with_agentfield_server().So no selection outcome changes: the public-IP candidate could only ever have been chosen when no explicit URL was configured, which is the case this PR leaves untouched. The concrete loss is that the persisted discovery metadata for an explicitly-configured agent no longer lists
http://<detected-public-ip>:<port>as a diagnostic entry, and a hypothetical future control plane that walked the list would not see it. Every other candidate is kept: the Railway internal hostname, the local-network address, the container hostname,host.docker.internaland the localhost fallbacks.A callback URL that fails to normalize (empty, whitespace-only, unparseable) is deliberately not treated as configured, so malformed input still falls back to full auto-detection rather than leaving the agent with no usable candidate.
What the opt-out does not do
AGENTFIELD_DISABLE_IP_DETECTIONis not a network kill switch, and the docs entry now says so. It does not stop_detect_local_ip(), which opens a connectionlessSOCK_DGRAMsocket andconnect()s toward8.8.8.8:80purely to read back the source address the kernel would use — no packets are sent, no DNS lookup happens. Registration and heartbeat traffic to the control plane is likewise unaffected.Type of change
Test plan
All commands run from
sdk/pythonunless noted. Python SDK anddocs/are the only surfaces touched.uvx --from ruff==0.15.22 ruff check .→All checks passed!uv run --extra dev ./scripts/run_pytest.sh -p no:cacheprovider(CI's literal entrypoint, coverage on) →2023 passed, 4 skipped, 38 deselected, 37 warnings in 117.45s, exit0; coverageTOTAL 2002 stmts, 116 miss, 94%uv run --extra dev python -m pytest tests/test_agent_networking.py tests/test_agent_helpers.py tests/test_agent_bigfiles_final90.py tests/test_agent_field_handler.py -p no:cacheprovider --no-cov -q(every test file that touches callback discovery) →69 passeduvx --from ruff==0.15.22 ruff format --diff agentfield/agent.py→ 23 hunks, all of them pre-existing drift below line 800; none of the lines this PR adds or edits report drift. The repo'sFormat (auto-fix)CI step iscontinue-on-error: true, and no untouched code was reformatted.pyproject.tomladdopts(whichscripts/coverage-surface.sh sdk-pythonreuses verbatim) measuresagentfield.client,agent_field_handler,execution_context,execution_state,memory,rate_limiter,result_cache—agentfield.agentis not in the measured set, so this change cannot move the gated number in either direction.Validation contract
Behaviours the change must exhibit, and the test that covers each. All live in
sdk/python/tests/test_agent_networking.pyunless noted. Every row is a statement about_build_callback_candidates()— see "Where the probe is actually reachable" above for which of them a stock agent exercises._detect_container_ip()is called and its result is offered as a candidate — default behaviour unchangedtest_container_ip_probe_runs_when_nothing_is_configuredcallback_urlconstructor argument set, the probe is never called and the normalized explicit URL iscandidates[0]test_container_ip_probe_skipped_for_explicit_callback_argumentAGENT_CALLBACK_URLset, the probe is never called and the normalized env URL iscandidates[0]test_container_ip_probe_skipped_for_callback_url_env_varAGENT_CALLBACK_URL, and both suppress the probetest_explicit_callback_argument_is_preferred_over_env_varAGENTFIELD_DISABLE_IP_DETECTIONset to1,true,TRUE,Yesoryessuppresses the probe with no callback URL configuredtest_disable_flag_truthy_values_skip_the_probe(parametrized)"",0,false,no,off,maybeare not opt-ins — the probe still runstest_disable_flag_non_truthy_values_leave_the_probe_enabled(parametrized)" ") is not "configured" and does not suppress the probetest_unusable_callback_url_still_allows_the_probehost.docker.internaland the localhost fallbacks are all still offeredtest_skipping_the_probe_keeps_every_other_candidatetest_probe_never_runs_outside_a_container_resolve_callback_url()still returns a usable local URL rather than failingtest_resolve_callback_url_with_detection_disabled_falls_back_locallyAGENT_CALLBACK_URL, or the disable flag), building the candidate list issues no HTTP request — asserted againstrequests.getwith_detect_container_ipleft in place, not stubbedtest_no_http_request_is_made_when_the_callback_url_is_known(parametrized over all three forms)requests.get, so the tripwire in #11 would fire if the skip regressedtest_http_request_is_made_when_nothing_is_configuredContract items 1–9 stub
_detect_container_ipitself, which proves the helper is not called — a level above the literal symptom in #624. Items 11 and 12 close that gap by leaving the helper in place and putting a tripwire onrequests.get, its only outbound entry point (it importsrequestslazily and makes no other network call). The tripwire records each attempt in addition to raisingAssertionError, because_detect_container_ipwraps every request inexcept Exception: passand would otherwise swallow the raise — the recorded list is what gives the test teeth. Verified by reverting the fix locally: all three parameters of item 11 fail, and the recording shows all four probe targets (AWS, GCP, Azure,api.ipify.org) attempted with theAssertionErrorabsorbed.Two pre-existing tests asserted the behaviour this PR deliberately changes — that the detected public IP appears as a candidate while an explicit callback URL is set. Both were updated to assert the new contract (that the address is absent) rather than dropped, so the assertion still has teeth:
test_build_callback_candidates_prefers_envandtests/test_agent_bigfiles_final90.py::test_callback_candidate_helpers. Amonkeypatch.delenv("AGENTFIELD_DISABLE_IP_DETECTION")was added to those two and totests/test_agent_helpers.py::test_resolve_callback_url_fallback_to_detected_ipsso they stay hermetic on a machine that exports the new variable.Verification round
An independent round re-tested the branch live and found no defect in the code; the follow-up commits are documentation-accuracy fixes only. What was exercised:
In-process network tripwire.
requests.get,requests.Session.request,urllib.request.urlopen,socket.create_connectionandsocket.socket.connect/connect_exare wrapped (recording, not blocking) beforeimport agentfield; each case then runs in a fresh process withCONTAINER=1so the in-container branch is live, constructs anAgent(...)and calls_build_callback_candidates(). Probe destinations counted per case, this branch vsmain:Agent.__init__maina_kwargcallback_url="http://my-agent.svc:<port>"__init__, 20 totalb_envAGENT_CALLBACK_URLsetc_disable_trueAGENTFIELD_DISABLE_IP_DETECTION=trued_neitherd_neitheris the control: default behaviour is unchanged, and the 10 recorded destinations are the four probe URLs (AWS, GCP, Azure,api.ipify.org) plus their socket-level connects.a_kwargis the fix: 10 → 0 in the constructor, which is the only route into the probe a running agent takes. The0ind_neither's__init__column on both branches is the reachability gap described above.In the suppressed cases the only non-local destination recorded is
('8.8.8.8', 80)— the_detect_local_ip()UDPconnect(), unchanged by this PR and present onmaintoo. That is why the docs entry now calls it out explicitly.Candidate-list delta. Exactly one entry disappears in the skipped cases, the public-IP one, and it was never first in either build. Railway internal host, local-network address, container hostname,
host.docker.internaland the localhost fallbacks are all still present.Black-box A/B against a real control plane. A real agent process (
app.serve()) behind a logging HTTP proxy, registering with an isolated control plane and executing a reasoner end to end. The proxy was first proven to log the probe URLs when they are requested. Withmain's SDK the proxy recorded 4 probe requests (169.254.169.254×2,metadata.google.internal,CONNECT api.ipify.org:443); with this branch's SDK, under identical env and the same explicit callback URL, it recorded 0. Both builds registered with the same callback URL, so onmainthe probe result was pure wasted egress. [Python SDK] Unexpected internal endpoints probe on start #624 reproduced in a real process, and fixed.Negative control on the tests. The test file from this PR, run against
main, fails 14 tests including all three parameters of contract item 11. The same 14 pass here.No LLM or paid-API call was made at any point in this verification.
Test coverage
coverage-baseline.jsonin this PR only if the removal caused a legitimate regression and I called it out in the summary above. — n/a, no code removed and no baseline change.sdk-pythonnumber.Checklist
Related issues / PRs
Fixes #624
Refs #921 (earlier diagnosis of the same probe by @AmirF194)