Repository navigation
fleet: serve the EC2 half's aggregator to browsers under agg-ec2 names - #1268
Merged
Merged
Conversation
The EC2 aggregator runs beside the coordinator in its Fargate task and was reachable only at --ec2-aggregate-url, through the coordinator's Cloudflare tunnel (Access-gated, and Cloudflare in the media path). `fcvm fleet coord --ec2-aggregate-tls` (with --ec2-aggregate-listen, --tls-secret and --dns-token-secret) gives it the task's own addresses: - src/fleet/coord/ecs.rs reads them at start: the global IPv6 address from the ECS task metadata, and the public IPv4 address of the task's network interface from EC2 DescribeNetworkInterfaces (filtered by its private address; the metadata does not carry it). A task with neither is an error. - The coordinator writes the certificate from --tls-secret as tls.json into --ec2-aggregate-tls-dir (the agent's tls::refresh/run, hourly) and runs the aggregator with --tls-listen on 0.0.0.0:443 and [<ipv6>]:443. - Config carries the two addresses; dns::facts adds `agg-ec2` (generation 0, both on 443), owned() recognises agg-ec2-<token>[-v4], and the published names are kept in Fleet::ec2_aggregate_published (cleared when a coordinator resumes, since they were for the previous task's addresses). - view: with the DNS writer the agg:ec2 entry's frame_url/frame_url_v4 are its published names, else null; without it, --ec2-aggregate-url as before. - --ec2-aggregate-listen now needs --ec2-aggregate-url or --ec2-aggregate-tls. The task must admit 443 in its security group and lower net.ipv4.ip_unprivileged_port_start (the image runs as 65534); that is the Terraform side. Staging is unchanged until it passes the flag. Tests: the_ec2_aggregator_is_published_at_the_coordinators_addresses (tests/test_fleet_coord.rs) waits for the AAAA and A records, sees no URL before confirmation, then wss://agg-ec2-<token>.<zone>/frames and the -v4 name with no port. Without the agg-ec2 fact it fails: no record is ever written. the_task_addresses_are_read_from_metadata_and_ec2 covers dual stack, IPv4 only, a ULA address and the EC2 answer.
A host aggregator's stream was reachable only at the plain URL its agent reported, which an https page cannot open, so AggregateView.frame_url was unusable and frame_url_v4 was always null. Agent (`--aggregate-tls-port <port>`, with --tls-secret): - runs the aggregator with --tls-file (the renderers' tls.json) and --tls-listen on <publish ip>:<port>, and with an IPv6 prefix also on [<prefix>::1]:443. <prefix>::1 is the host's side of the prefix, which fcvm never gives a VM; the agent puts it on the host's default IPv6 interface (ip -6 addr replace ... nodad) before it starts. - reports ipv6, and with --public-ipv4-from-imds ipv4 and v4_port, in the aggregate message (new optional fields). fleet-core: - Host keeps the reported addresses and its published names (aggregate_published). Event::Published for `agg-<host>` applies only to the addresses the host reports now. - aggregate_id(host): `agg-<host>` when the host name fits a DNS label. - canonical_ip and Published::names replace the two copies of the canonical comparison (dns.rs, view::frame_urls). - view: with published names an aggregate's frame_url/frame_url_v4 are its published names, else null; without DNS, the reported URL as before. Coordinator: - dns::facts includes each host aggregator (generation 0: one launch while its addresses stay the same), so its names go through the same mint/write/confirm/publish path and zone as slot names; owned() recognises agg-<host>-<token>[-v4]. The write/confirm loop is unchanged. - The poster reads a host aggregator at its published name when it has one: the reported URL can be an address only that host reaches. Staging is unchanged until an agent is given --aggregate-port and --aggregate-tls-port. Tests: - a_host_aggregator_is_published_under_its_own_names (tests/test_fleet_coord.rs): no URL for a plain aggregator, records written, URLs only after confirmation, a new address hides the old URLs and the old records go. With the view returning the reported URL it fails: "the plain URL reached an https page". - the_agent_runs_and_feeds_its_aggregator now runs the aggregator with TLS, checks the reported ipv4/v4_port, https /healthz and 404 for /layout over TLS. - only_fleet_names_are_owned, labels_are_owned_and_not_reused, a_host_aggregator_is_named_only_when_its_host_name_fits_a_label, a_slot_address_is_the_gpu_index_plus_two_in_the_prefix.
…vm-fleet The coordinator image runs as 65534 on a read-only root file system, so --ec2-aggregate-tls needs a directory it can write: --ec2-aggregate-tls-dir defaults to /run/fcvm-fleet/tls (the agent's own default is /run/fcvm-fleet/tls too), and the Containerfile creates /run/fcvm-fleet owned by 65534 for the task volume mounted there.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Oct 25, 2026
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14072a7b1d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ly one Codex on #1268: DescribeNetworkInterfaces filtered by the task's private IPv4 address can match an interface in another VPC of the account that uses the same address, and parse_public_ip took the first <publicIp> in the answer, so the EC2 aggregator could be named at another host's public address. The query now filters by mac-address, the interface's MAC from the task metadata (MACAddress), falling back to the private address only when the metadata has no MAC, and parse_public_ip gives an address only when the answer names exactly one interface. Test: an_answer_naming_two_interfaces_publishes_neither_address. Before the fix: left: Some("192.0.2.44"), right: None. With the single-interface check removed again it fails the same way.
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Owner
Author
|
NOT-A-DEFECT: Codex's review body on the earlier head carries the finding answered in its thread (fixed, RED-VERIFIED, and Codex cleared 14d0461); the CodeRabbit comment is a review-limit notice. |
Owner
Author
|
NOT-A-DEFECT: the CodeRabbit comment is a review-limit notice. The head is a clean automatic merge of the layer below (its tree equals |
…ggregate # Conflicts: # fleet/PROTOCOL.md # tests/test_fleet_coord.rs
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
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.
Third of three stacked PRs for #1232. The EC2 half's aggregator, which runs beside the coordinator in its Fargate task, gets the task's own addresses and
agg-ec2-<token>names instead of the coordinator's Cloudflare tunnel (Access-gated, and Cloudflare in the media path).Stacked on:
fix/issue-1232-host-aggregate(#1267), which is stacked on #1266.What changed
fcvm fleet coord --ec2-aggregate-tls(needs--ec2-aggregate-listen,--tls-secret,--dns-token-secret):src/fleet/coord/ecs.rsreads the task's addresses at start: the global IPv6 address from the ECS task metadata (ECS_CONTAINER_METADATA_URI_V4), and the public IPv4 address of the task's network interface from EC2DescribeNetworkInterfaces, filtered by its private address (the metadata does not carry it). A task with neither is a start error.--tls-secretastls.jsoninto--ec2-aggregate-tls-dir(default/run/fcvm-fleet/tls; the agent'stls::refresh/run, hourly) and runs the aggregator with--tls-listen 0.0.0.0:443and[<ipv6>]:443. The Fargate task is its own host, so both families use 443.Configcarries the two addresses;dns::factsaddsagg-ec2(generation 0, both on 443) through the same naming path;owned()recognisesagg-ec2-<token>[-v4]; published names are kept inFleet::ec2_aggregate_publishedand cleared when a coordinator resumes (they were for the previous task's addresses).agg:ec2entry's URLs are its published names, else null; without it,--ec2-aggregate-urlas before.--ec2-aggregate-listennow needs--ec2-aggregate-urlor--ec2-aggregate-tls.fleet/coord/Containerfile: creates/run/fcvm-fleetowned by 65534 for the task volume mounted there (root file system read-only).The task also needs 443 in its security group,
net.ipv4.ip_unprivileged_port_start=443(the image runs as 65534) and the ECSdualStackIPv6account setting for an IPv6 address: that is ejc3/aws#671, behind a switch that is off. Staging is unchanged until the coordinator is given--ec2-aggregate-tls.Contract and evidence
the_ec2_aggregator_is_published_at_the_coordinators_addresses(tests/test_fleet_coord.rs): with the addresses inConfigand one EC2 tile, the AAAA and A records foragg-ec2-<token>are written, no URL reaches the page before confirmation, thenwss://agg-ec2-<token>.<zone>/framesand the-v4name with no port. Without theagg-ec2fact it fails: no record is ever written (label_fortimes out on an empty zone).the_task_addresses_are_read_from_metadata_and_ec2: dual stack, IPv4 only, a ULA address ignored, an empty network list refused, the EC2 answer'spublicIp.Not verified (needs the live Fargate task): that a Fargate task volume takes
/run/fcvm-fleet's ownership from the image, and the ECS metadata field names on a dual-stack task (taken from the ECS task metadata v4 documentation). Either failing stops the coordinator at start with the error, only with--ec2-aggregate-tlspassed.Fixes #1232 (ejc3/aws#671 holds the switch that turns it on).