Repository navigation
Conversation
Bind the unauthenticated status dashboard to loopback and remove its port from the router Service to prevent other pods from reading recent requests and process configuration. Retain diagnostic access through pod port-forwarding and document health checks through the existing metrics listener.
|
ping aiqubits I'd like to get this merged. |
|
Greg Castle (@destijl) I'm very happy that you can do this. |
Jeff Luo (JeffLuoo)
left a comment
Jeff Luo (JeffLuoo)
left a comment
There was a problem hiding this comment.
In manifests/ate-install/atenet-router.yaml (lines 214–215), the Deployment still declares - name: status / containerPort: 4040. Nothing listens on the pod IP at that port now. A PodMonitor, a NetworkPolicy, or a health check that uses the named port gets a refused connection. Please remove the entry (matching atenet-egress.yaml), or add a comment that the port is loopback-only.
| port: 8444 | ||
| targetPort: 8444 | ||
| protocol: TCP | ||
| - name: status |
There was a problem hiding this comment.
This removal breaks the e2e StatuszClient. internal/e2e/statusz.go calls portforward.ServicePortForward(..., ResourceName("atenet-router"), routerStatusPort) with port 4040, and resolveTargetPort looks up that port in the Service spec. With no 4040 port on the Service, NewStatuszClient returns an error, and every parking e2e test that reads the parked-request gauge fails.
Please port-forward to the pod directly. One option is to export podPortForward and use it with firstReadyPodForService. Please also update the comment on routerStatusPort, which calls 4040 "the atenet-router Service port".
| listener, err := net.Listen("tcp", statusListenAddr) | ||
| if err != nil { | ||
| return fmt.Errorf("failed binding Router HTTP status server port: %w", err) | ||
| return fmt.Errorf("failed binding Router HTTP status server address %q: %w", statusListenAddr, err) |
There was a problem hiding this comment.
If the status listener fails to bind (for example, a typo or unbracketed IPv6 literal in --status-address), Run returns here before drainOnShutdown and g.Wait(). Because startDataplane, s.health.Start, and extprocGRPC have already been launched above, those goroutines keep running and holding their ports (18000 and 50051).
Please bind the status listener (with defer listener.Close()) before startDataplane and the g.Go calls so a bad bind fails before starting the background servers.
| // Run() dials ateapi in mtls mode, which requires real TLS material; generate it. | ||
| caPath, clientCertPath := writeTestTLSMaterial(t) | ||
|
|
||
| statusAddr, err := NewRouterCmd().Flags().GetString("status-address") |
There was a problem hiding this comment.
This reads the default value of the flag, but nothing checks the args in the manifest. If someone adds --status-address=0.0.0.0 to atenet-router.yaml or to an overlay, all tests stay green and the dashboard is on the pod IP again.
Can you parse the container args from the manifest with NewRouterCmd().ParseFlags, as routerShutdownBudget does, and assert that the status address is loopback?
| } | ||
|
|
||
| // A wildcard listener would also accept connections on this address. | ||
| conn, err := net.DialTimeout("tcp", fmt.Sprintf("127.0.0.2:%d", httpPort), time.Second) |
There was a problem hiding this comment.
nit: The 127.0.0.2 dial catches a bind to all interfaces only on Linux. On macOS, lo0 has only 127.0.0.1, so the dial always fails. CI runs on Linux, so this is not a blocker. An assertion on the bind address of the listener works on every OS.
| } | ||
| } | ||
|
|
||
| func TestRouterServiceDoesNotExposeStatus(t *testing.T) { |
There was a problem hiding this comment.
nit: This loop copies the split, kind check, and decode logic of routerShutdownBudget in config_test.go. Can you move it into one shared helper, for example routerManifestObject(t, kind, name, into)? Then a change to the manifest path or format needs only one fix.
| continue | ||
| } | ||
| for _, port := range service.Spec.Ports { | ||
| if port.Name == "status" || port.Port == 4040 || port.TargetPort.IntValue() == 4040 || port.TargetPort.StrVal == "status" { |
There was a problem hiding this comment.
This hard-codes 4040. If --status-port changes in the Deployment, this guard no longer checks the correct port. Can you read the port from the --status-port arg in the manifest?
|
Closed in favor #2573 |
This picks up agent-substrate#2297 from @aiqubits, rebased on main, with the e2e failure fixed. The router's unauthenticated /statusz dashboard shows recent cross-tenant requests, worker destinations, and process configuration to any pod that can reach it. This binds it to 127.0.0.1 by default through a new --status-address flag and removes its port from the router Service. Admins can still get to it with kubectl -n ate-system port-forward deployment/atenet-router 4040:4040. On top of the original change, the e2e parking suite now port-forwards to the status port on a router pod rather than the Service, since the Service no longer publishes it. #### Breaking change GKE HealthCheckPolicy configs using port 4040 and /statusz should move to port 9091 and /readyz. The router Deployment no longer declares the status container port either. Fixes agent-substrate#2275 - [x] Tests pass - [x] Appropriate changes to documentation are included in the PR --------- Co-authored-by: aiqubits <[email protected]>
The router's unauthenticated
/statuszdashboard exposes recent cross-tenant requests, worker destinations, and process configuration to other pods. Bind it to127.0.0.1by default through the new--status-addressflag, and remove its port from the router Service. Administrators can still access the HTML and JSON dashboard through a pod port-forward.Fixes #2275
Access the dashboard with
kubectl -n ate-system port-forward deployment/atenet-router 4040:4040instead of the Service or Pod IP. Migrate GKEHealthCheckPolicyconfigurations using port 4040 and/statuszto the existing port 9090 and/readyz. GKE probes Pod IPs directly, so the metrics port does not need to be published by the Service. Please apply thebreaking-changelabel.Validation:
Regression tests fail before the fix and pass afterward under the race detector, covering local HTML/JSON access, listener isolation, and Service exposure.
All 14 repository verification scripts pass. Related race tests pass with the known IPv6 admin baseline failure excluded.
Additional review checks pass for IPv4/IPv6 address overrides, health-listener isolation, and the rendered Envoy and AgentGateway Services.
Tests pass (targeted tests pass; full-suite baseline failures are listed above)
Appropriate changes to documentation are included in the PR