-
Notifications
You must be signed in to change notification settings - Fork 219
fix: slot return on network fail and improve validation #1485
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
fix: slot return on network fail and improve validation #1485
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
2f85109 to
ebe407f
Compare
ebe407f to
453c7ae
Compare
tests/integration/internal/tests/api/sandboxes/sandbox_network_out_test.go
Show resolved
Hide resolved
f4ca868 to
1d4ecd2
Compare
6111213 to
1f9e84e
Compare
1f9e84e to
2b23561
Compare
083a4a8 to
4338833
Compare
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.
Bug: Premature Flag Corrupts Firewall State
The firewallCustomRules flag is set to true before the firewall configuration is actually applied. If any error occurs after setting the flag (such as failing to get the network namespace or failing to add firewall rules), the function returns an error but leaves the flag set to true. When the slot is later returned to the pool and ResetInternet is called, it will attempt to reset firewall rules that were never successfully applied, potentially causing firewall state corruption or unexpected behavior.
packages/orchestrator/internal/sandbox/network/slot.go#L262-L290
infra/packages/orchestrator/internal/sandbox/network/slot.go
Lines 262 to 290 in 4338833
| s.firewallCustomRules.Store(true) | |
| n, err := ns.GetNS(filepath.Join(netNamespacesDir, s.NamespaceID())) | |
| if err != nil { | |
| return fmt.Errorf("failed to get slot network namespace '%s': %w", s.NamespaceID(), err) | |
| } | |
| defer n.Close() | |
| err = n.Do(func(_ ns.NetNS) error { | |
| for _, cidr := range network.GetEgress().GetAllowedCidrs() { | |
| err = s.Firewall.AddAllowedCIDR(cidr) | |
| if err != nil { | |
| return fmt.Errorf("error setting firewall rules: %w", err) | |
| } | |
| } | |
| for _, cidr := range network.GetEgress().GetDeniedCidrs() { | |
| err = s.Firewall.AddDeniedCIDR(cidr) | |
| if err != nil { | |
| return fmt.Errorf("error setting firewall rules: %w", err) | |
| } | |
| } | |
| return nil | |
| }) | |
| if err != nil { | |
| return fmt.Errorf("failed execution in network namespace '%s': %w", s.NamespaceID(), err) | |
| } |
Note
Centralizes sandbox network helpers, refactors firewall to predefined/user allow/deny sets with 0.0.0.0/0 handling, validates maskRequestHost, and returns slots on network config failure with expanded integration tests and deps updates.
network.maskRequestHost(IDNA/ASCII, optional port) and surface clear400errors.AllInternetTrafficCIDRwhenallowInternetAccess=false.Pool.Get.ResetAllSets,ResetAllowedSets,ResetDeniedSets.0.0.0.0/0and incremental set updates.AllInternetTrafficCIDRin server create path and instance builder.sandbox-networkpackage withAllInternetTrafficCIDR, default denied ranges, and address→CIDR helpers.ngrok/firewall_toolkit,google/nftables,mdlayher/*,zerolog) inapi/sharedmodules.Written by Cursor Bugbot for commit 73a2bd2. This will update automatically on new commits. Configure here.