Add dynamic Softnet policy control - #181
Conversation
![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: 8ac47c46ff
ℹ️ 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".
| continue; | ||
| } | ||
|
|
||
| self.enqueue(handle_request(policy, &line[..newline]))?; |
There was a problem hiding this comment.
When the peer applies write backpressure and pending responses approach the 4 MiB limit, handle_request executes before enqueue checks that limit. A softnet.policy.set can therefore replace the live egress rules and then fail to enqueue its response; Proxy::service_control subsequently disables the channel, leaving the controller without an acknowledgment or a way to query which revision is active. Stop reading while output is blocked or stage the mutation until its response can be queued.
Useful? React with 👍 / 👎.
|
|
||
| while bytes_read < MAX_SERVICE_BYTES { | ||
| match self.stream.read(&mut buf) { | ||
| Ok(0) => return Ok(false), |
There was a problem hiding this comment.
When a client sends a complete newline-framed request and then half-closes its write side, the first read processes the request and queues its response, but the next read returns EOF here and service returns false without flushing that newly queued response. The proxy then drops the control channel, so a valid duplex client waiting for the acknowledgment never receives it even though a policy update may already have been applied.
Useful? React with 👍 / 👎.
| let bridge_isolation = !allow.contains(&Target::Prefix(Ipv4Net::zero())); | ||
| let rules = build_rules(self.gateway_ip, &allow, &block); | ||
|
|
||
| if self.desired_revision.as_deref() == Some(desired_revision.as_str()) { |
There was a problem hiding this comment.
After revisions A and then B have been accepted, a request that reuses A with different policy contents bypasses this check because only the current revision B is retained. The request silently overwrites the live policy and rebinds A, contrary to the documented guarantee that reusing a revision with a different policy returns a conflict; retain prior revision bindings or otherwise reject their reoccurrence.
Useful? React with 👍 / 👎.
edi-oai
left a comment
edi-oai
left a comment
There was a problem hiding this comment.
I think lib/proxy/control.rs is more complex than it needs to be, but let's move forward and address this in separate PRs.
2 participants
Summary
Adds an optional, bounded JSON-RPC 2.0 control channel for updating a running Softnet instance's complete allow/block policy without restarting the VM. The protocol intentionally exposes only two methods for now:
softnet.policy.getandsoftnet.policy.set. The existing VM packet socket remains on stdin; control uses a separate connected Unix stream socket.Follow-on integrations: openai/tart#1287
Changes
--control-fdwith strict connected-AF_UNIX/SOCK_STREAM validation and safe descriptor ownershipsoftnet.policy.getand atomicsoftnet.policy.set; there is no capabilities RPCjsonrpsee-typesfor request parsing, parameter decoding, standard error codes, typed errors, and response construction while retaining the existing nonblocking FD reader, framing, and backpressure bounds@host, preserve longest-prefix/block precedence, validate revisions, and bound requests, targets, and queued outputCargo.lockforjsonrpsee-typesand its requiredserde_jsonversion without adding a Tokio/HTTP-server stackValidation
cargo fmt --checkcargo check --locked --all-targets --all-featurescargo clippy --locked --all-targets --all-features -- -D warningscargo tree --locked -i jsonrpsee-typesgit diff --checkThree existing vmnet-backed tests require privileged execution and were skipped locally because passwordless sudo is unavailable. The existing
block v0.1.6future-incompatibility warning remains.