-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(network): update sandbox egress policy in place, re-evaluating live flows #1399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,8 +12,8 @@ use crate::{ | |
| datetime_from_unix_nanos, extract_template_id, CreateSandboxRequest, CubeEgressRule, | ||
| CubeEgressRuleAction, CubeEgressRuleInject, CubeEgressRuleMatch, CubeMasterClient, | ||
| CubeMasterError, CubeNetworkConfig, DeleteSandboxRequest, ListSandboxRequest, SandboxInfo, | ||
| SandboxLogsRequest, SandboxRefreshRequest, SandboxStatus, SandboxTimeoutRequest, | ||
| SandboxUpdateRequest, VolumeSpec, | ||
| SandboxLogsRequest, SandboxNetworkRequest, SandboxRefreshRequest, SandboxStatus, | ||
| SandboxTimeoutRequest, SandboxUpdateRequest, VolumeSpec, | ||
| }, | ||
| error::{AppError, AppResult}, | ||
| models::{ | ||
|
|
@@ -493,6 +493,42 @@ impl SandboxService { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Replace a running sandbox's egress policy. | ||
| /// | ||
| /// The policy is validated and mapped by the same code as sandbox creation, | ||
| /// so an update cannot install anything create would have rejected. An | ||
| /// all-empty body is legal and clears the policy, which is why the mapper's | ||
| /// "nothing set" `None` is turned back into a default config rather than | ||
| /// treated as "no change". | ||
| pub async fn update_network( | ||
| &self, | ||
| sandbox_id: &str, | ||
| allow_internet_access: Option<bool>, | ||
| network: Option<&SandboxNetworkConfig>, | ||
| ) -> AppResult<()> { | ||
| let cube_network_config = | ||
| build_cube_network_config(allow_internet_access, network)?.unwrap_or_default(); | ||
|
|
||
| let req = SandboxNetworkRequest { | ||
| request_id: new_request_id(), | ||
| sandbox_id: sandbox_id.to_string(), | ||
| instance_type: self.instance_type.clone(), | ||
| cube_network_config, | ||
| }; | ||
|
|
||
| let resp = self | ||
| .cubemaster | ||
| .update_sandbox_network(&req) | ||
| .await | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 409 never reaches clients — a paused/not-active sandbox update surfaces as HTTP 500.
But the contract promises 409:
The sibling update/delete paths avoid exactly this by using |
||
| .map_err(|e| sandbox_not_found_or_internal(e, sandbox_id))?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High severity — paused/not-running sandbox returns HTTP 500, not the documented 409.
That contradicts the handler's own utoipa annotation (
|
||
|
|
||
| resp.ret | ||
| .into_result() | ||
| .map_err(|e| sandbox_not_found_or_internal(e, sandbox_id))?; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Medium severity — unknown sandbox ID returns HTTP 400, not 404.
The utoipa annotation documents |
||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub async fn refresh(&self, sandbox_id: &str, duration: i32) -> AppResult<()> { | ||
| let req = SandboxRefreshRequest { | ||
| request_id: new_request_id(), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low severity — an all-empty body opens internet access.
build_cube_network_config(None, None)returnsOk(None), and.unwrap_or_default()yields aCubeNetworkConfigwithallow_internet_access = None. On the Cubelet sidecubeVSTapRegistrationtreats a nilAllowInternetAccessas true (the create-time default), soPUT /sandboxes/{id}/networkwith body{}clearsallow_out/deny_out/rules and flips the sandbox to internet-allowed.The request-model doc says "any field left out clears what the sandbox currently has" — but omitting
allowInternetAccessclears it to default-true, which is likely not what a caller sending{}means (typically "clear restrictions", not "permit everything"). This is documented in the README (update_network(network={}, allow_internet_access=False)), so it's consistent behavior, but on an update endpoint where the natural reading of an empty body is "no change", this default is a footgun — at minimum worth a note in the OpenAPI/utoipa docs that omittingallowInternetAccessmeans "allow", and arguably worth requiring the field explicitly on this endpoint.