feat(agents): route gateway to container-mode agents via Deployment e…#644
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe gateway now resolves container deployment targets from projected HTTP/HTTPS endpoints, checks routability by deployment mode and status, and applies this logic to direct and agent-name proxying. Tests cover routing, unavailable deployments, missing deployments, and origin validation. ChangesContainer-mode gateway routing
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant AgentDeployment
participant Target
Client->>Gateway: Proxy request
Gateway->>AgentDeployment: Resolve routable endpoint
AgentDeployment-->>Gateway: HTTP/HTTPS endpoint
Gateway->>Target: Forward request
Target-->>Gateway: Proxy response
Gateway-->>Client: Return response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/nemo-agents/tests/unit/test_gateway.py (1)
488-617: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for protocol-filtering branch in
_get_deployment_endpoint.All container fixtures use an
httpendpoint; no test exercisesendpointscontaining onlygrpc/tcpentries (→None/503) or a mixed list where a non-http entry precedes an http one (→ correct skip-and-continue). This is the one untested branch of the new resolution logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/nemo-agents/tests/unit/test_gateway.py` around lines 488 - 617, Add tests in TestContainerModeByDeploymentName covering _get_deployment_endpoint protocol filtering: verify a running deployment with only grpc/tcp endpoints returns 503, and verify a mixed endpoint list with a non-http entry before an http entry skips the unsupported endpoint and routes to the HTTP URL. Use the existing _make_container_deployment, Endpoint, client, and HTTP mock patterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/nemo-agents/tests/unit/test_gateway.py`:
- Around line 488-617: Add tests in TestContainerModeByDeploymentName covering
_get_deployment_endpoint protocol filtering: verify a running deployment with
only grpc/tcp endpoints returns 503, and verify a mixed endpoint list with a
non-http entry before an http entry skips the unsupported endpoint and routes to
the HTTP URL. Use the existing _make_container_deployment, Endpoint, client, and
HTTP mock patterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 69189d08-62be-4912-9836-06c2dccd7d6e
📒 Files selected for processing (3)
plugins/nemo-agents/src/nemo_agents_plugin/api/v2/gateway.pyplugins/nemo-agents/src/nemo_agents_plugin/entities.pyplugins/nemo-agents/tests/unit/test_gateway.py
Contributor
|
5 tasks
…ndpoints Teach the agents-plugin gateway proxy to resolve a container-mode (docker/k8s) agent's address from the projected `AgentDeployment.endpoints` instead of the hardcoded loopback `endpoint`. Subprocess mode is unchanged and remains the default. This is the deliberate stop-gap that unblocks agents in k8s. - Promote `endpoint` -> `endpoints: list[Endpoint]` on AgentDeployment and add a `deployment_mode` discriminator (subprocess|docker|k8s). `endpoint` is kept as the subprocess loopback compat shim for one release. - Add `_get_deployment_endpoint` / `_is_deployment_routable` gateway helpers: the only place endpoint resolution branches on mode. Container modes read the first http(s) URL from `endpoints`; subprocess reads the loopback `endpoint`. - A container deployment is routable once its status is `running` (the controller projects the deployments-plugin READY status onto `running`); not-ready -> 503, missing -> 404, matching today's contract. - The SSRF origin guard is origin-relative, so it works unchanged for k8s Service DNS and docker host:port targets; documented, no allow-list change. - Unit tests: subprocess resolution unchanged (regression), docker/k8s resolution from `endpoints`, 503 not-ready, 404 missing, Service-DNS origin guard. Note: coordinate the `endpoint`->`endpoints` + `deployment_mode` entity change and the status/endpoint projection with the container producer work; that change may absorb the entity portion of this diff. Signed-off-by: Ben McCown <bmccown@nvidia.com>
benmccown
force-pushed
the
agent-gateway-container-routing
branch
from
bc7430e to
ad499f0
Compare
Signed-off-by: Ben McCown <bmccown@nvidia.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
2 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.
feat(agents): route gateway to container-mode agents via
Deployment.endpointsWhat & why
Teaches the agents-plugin gateway proxy to route to container-mode (docker/k8s) agents by resolving their address from the projected
AgentDeployment.endpoints, instead of the hardcoded loopbackendpoint. Subprocess mode is unchanged and stays the default.This is the stop-gap that unblocks agents-in-k8s: without it, a containerized agent can be deployed but inbound traffic can't reach it, since the gateway is hardwired to a loopback
127.0.0.1:<port>that's only valid for a same-host subprocess.Changes
entities.py— promoteendpoint→endpoints: list[Endpoint]and add adeployment_modediscriminator (subprocess|docker|k8s, defaultsubprocess).endpointstays as the subprocess loopback compat shim for one release.gateway.py— add_get_deployment_endpoint/_is_deployment_routable, the single place resolution branches on mode: container modes read the first http(s) URL fromendpoints; subprocess readsendpoint. Routable once status isrunning; not-ready →503, missing →404(unchanged contract).host:port.Out of scope
Coordination
The container producer path (compile →
DeploymentConfig, status/endpoint projection) lands separately and this PR consumes it. Preferably that change absorbs the self-containedentities.pyhunk, after which this branch rebases and drops it. It's assumed the controller projects the deployments-pluginREADYonto the agents-localrunning.Testing
endpoints(both routes),503not-ready,404missing, Service-DNS origin guard.ruff+tyclean.AgentDeploymentpointing at a docker-in-docker mock agent; reached it200via both agent-name and deployment-name routes; verified503(not-ready) and404(missing). k8s Service-DNS routing is covered by unit tests.Summary by CodeRabbit