feat(sight): add ECS security group guide to dashboard command#1422
Conversation
Wire ecs_metadata module into lib.rs, dashboard.rs and server/mod.rs: - dashboard: probe ECS metadata (2s timeout), show instance console URL for security group configuration when remote access is blocked - server: hint about security group when binding to 0.0.0.0/:: - Add --no-open and --skip-sg-guide flags to dashboard subcommand - 5 unit tests for EcsMetadata (public_ip, instance_url, probe timeout)
The previous spawn+join pattern provided no timeout — join() blocks until the thread finishes, making the thread pointless. Replace with mpsc::channel + recv_timeout(PROBE_TIMEOUT=2s) so the caller is guaranteed to return within the deadline even if HTTP requests hang. Also add timeout() (overall request) to ureq agent instead of only timeout_connect() (TCP handshake), covering DNS resolution and response body reads.
Previously read_metadata() called get_imdsv2_token() for every field, resulting in 4 redundant PUT requests. On non-ECS hosts this added an extra connect timeout per field before the GET even tried. Now fetch_ecs_metadata() obtains the token once upfront and passes it as Option<&str> to read_metadata(), reducing HTTP round-trips from 4×(PUT+GET) to 1×PUT + 4×GET.
…ance_id.rs Move metadata_agent(), METADATA_BASE, and read_plain() into ecs_metadata.rs as pub(crate) shared primitives. genai/instance_id.rs now reuses them instead of maintaining its own ureq agent builder and hardcoded metadata URL — eliminating ~30 lines of duplicate code. Per AGENTS.md: '优先扩展现有模块,而非创建新文件'.
…ndary check ecs_metadata provides shared primitives (metadata_agent, read_plain, METADATA_BASE) used by multiple layers. Add it to CROSS_CUTTING set so any module may import it, matching the pattern of config/utils/discovery.
Address clippy::collapsible_if by using let-chain syntax (if auth.enabled && let Some(token) = ...) supported since Rust 1.87.
probe_returns_none_when_not_on_ecs fails when CI runners are on ECS instances where 100.100.100.200 metadata service is reachable. Mark as ignored for manual-only execution.
ecs_metadata.rs: - Refactor fetch/read functions to accept base_url parameter - Add MockServer helper using TcpListener for HTTP mocking - 9 new tests: read_plain, get_imdsv2_token, read_metadata, fetch_ecs_metadata, probe_with_mock_server (13 total, 1 ignored) dashboard.rs: - 5 new tests: find_executable, local_addresses, check_server_running instance_id.rs: - Update read_plain calls with METADATA_BASE parameter
- Extract resolve_display_url() and build_sg_message() pure functions - Add 6 new tests for dashboard URL resolution and SG messages - Add probe_times_out_when_server_is_slow test for ecs_metadata - Total new tests: 11 dashboard + 14 ecs_metadata = 25 tests
jfeng18
left a comment
There was a problem hiding this comment.
Reviewed in depth (independent sweep + adversarial pass, local build/test on the CI toolchain 1.89.0, and a real-ECS metadata E2E). The core is solid — approving. A few non-blocking notes below.
Verified:
- Builds clean (let-chains fine on 1.89); 25 new unit tests pass (14
ecs_metadata+ 11dashboard); off-ECS probe returnsNonein ~1.6s. - The
instance_idrefactor is behavior-equivalent — old and new both use plain IMDSv1 (no token, no XFF), same URL/timeout/trim/empty-handling and the sameCachedUid/OnceLocksemantics, so the SLSowner-account-iduid path is unchanged. The token/XFF machinery lives only in the new probe path, whichinstance_idnever calls. - Module registration (
ecs_metadataas cross-cutting) and the arch-boundary update are correct; the module has no outwardcrate::edges.
Non-blocking notes:
-
[low] On ECS,
dashboardprints the public IP with the live token.resolve_display_urlprefers the ECS public IP (EIP → public-ipv4), and the auth line printsURL with token: {display_url}/?token={token}. Verified on a real instance: the NIC carries a private IP (172.x) whileeipv4returns the routable public IP over IMDSv1, so the printed URL becomeshttp://<public-eip>:7396/?token=<token>— whereas the previous code paired the token with the private NIC IP. It's the operator's own terminal, but it's now a directly-usable public credential if that output is pasted into a log/screenshot/issue, and the same command's SG guide steers the user toward opening 7396 publicly. Consider masking the token on the public-IP line (or printing the token separately). -
[nit] Unexplained
X-Forwarded-For: Chinaheader in the IMDSv2 path (ecs_metadata.rs,read_with_token). On a non-hardened instance the tokenPUT /latest/api/tokenreturns 400 →ureqtreats it asErr→token = None→ the v2 path (and this header) is skipped entirely, IMDSv1 handles everything. So it's dormant here; I couldn't exercise the hardened-IMDS path. Worth a comment on why the header is there, or dropping it if it isn't needed. -
[nit]
instance_idfetch logs dropped the{e}cause (owner-account-id / instance-id warn/debug lines) in the refactor — mild loss of diagnostic detail when the SLS uid comes back empty. Behavior otherwise identical. -
[nit, informational] clippy note in the PR body is imprecise.
cargo clippy --all-targets -- -D warnings@1.89.0 does fail, but with 9 pre-existinguninlined_format_argsacross five files (probes/codex_offsets.rs,probes/elf_buildid.rs,health/checker.rs,server/token_savings.rs×5,unified.rs) — not justunified.rs, and none in this PR's changed files. The new code is clippy-clean and the CITest agentsightgate is green. Could be a nice separate cleanup.
Nice UX improvement for the ECS deployment path. LGTM.
Description
ECS users deploying AgentSight often cannot access the Dashboard remotely because port 7396 is not open in the instance security group. Currently there is no guidance — users see a timeout and have no idea what to do.
This PR adds ECS metadata probing to the
agentsight dashboardcommand so that when running on an ECS instance, it automatically detects the instance and outputs a direct console link for configuring the security group. It also shows a hint atagentsight servestartup when binding to0.0.0.0.The implementation extracts shared ECS metadata primitives (
metadata_agent(),read_plain(),METADATA_BASE) into a newecs_metadatamodule, whichgenai::instance_idnow reuses — eliminating ~30 lines of duplicate agent-builder and URL code.Related Issue
no-issue: new UX feature for ECS deployment workflow
Type of Change
Scope
sight(agentsight)Key Changes
src/agentsight/src/ecs_metadata.rs(new): shared ECS metadata client —metadata_agent(),read_plain(),probe_ecs_metadata()withmpsc::recv_timeout(2s)hard deadline, IMDSv2 token fetched once and reused across all fieldssrc/agentsight/src/bin/cli/dashboard.rs: probe ECS metadata ondashboardcommand, display instance console URL with security group hint when on ECS; added--no-openand--skip-sg-guideflagssrc/agentsight/src/server/mod.rs: eprintln security group hint atservestartup when binding to0.0.0.0or::src/agentsight/src/genai/instance_id.rs: reusemetadata_agent()andread_plain()fromecs_metadata, remove local agent builder and hardcoded URL (~30 lines deleted)src/agentsight/src/lib.rs: registerecs_metadatamodule (shared, not server-gated)src/agentsight/src/bin/agentsight.rs: update doc comments for dashboard subcommandChecklist
cargo fmt --checkpasscargo clippy --all-targets -- -D warningspass (pre-existing warnings inunified.rs, not from this change)cargo testpass (5 ecs_metadata + 2 instance_id tests, all green)Cargo.lock)Testing
Non-ECS environment:
agentsight dashboard --no-opencompletes within ~2s (probe_ecs_metadatareturnsNoneviarecv_timeout), shows fallback message. Verifiedprobe_returns_none_when_not_on_ecsunit test.ECS environment (cn-shenzhen, gn7i instance): deployed release binary, ran
agentsight serve --host 0.0.0.0(shows security group hint at startup), thenagentsight dashboard --no-open— output includes instance console URL with region.Unit tests: 5 tests in
ecs_metadata(public_ip preference, instance_url format, probe timeout) + 2 tests ininstance_id(CachedUid caching, MAX_RETRIES cap) — all pass.