-
Notifications
You must be signed in to change notification settings - Fork 219
feat: add sandbox network out configuration #1447
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
Conversation
a0928bb to
1c34d0d
Compare
1c34d0d to
09d9394
Compare
8d392ab to
5c95692
Compare
007f4f8 to
35fdb12
Compare
35fdb12 to
33c5a57
Compare
4559b70 to
b74976f
Compare
b74976f to
1c4264b
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.
💡 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".
4404e80 to
abb52a3
Compare
4c45801 to
e62ca88
Compare
e62ca88 to
d9e12b3
Compare
0468847 to
01a4356
Compare
|
|
||
| // Handle the case where internet access is explicitly disabled | ||
| // This should be applied after copying the network config to preserve allowed addresses | ||
| if allowInternetAccess != nil && !*allowInternetAccess { |
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.
I think we should return error when both network egress allow/block lists and allow internet are provided. It can bring unexpected behavior for users.
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.
The allowOut takes precedence over blockOut
also, internet is defined by default, allowInternetAccess false does the same as blockOut=[0.0.0.0/0]
|
|
||
| network := &types.SandboxNetworkConfig{ | ||
| Egress: &types.SandboxNetworkEgressConfig{ | ||
| AllowedAddresses: config.GetNetwork().GetEgress().GetAllowedAddresses(), |
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.
Are we sure this call chain will work correctly when communicating with orchestrators that are not yet supporting network configuration?
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.
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.
Thing was if we will not receive nil. It not, that is okay.
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.
we'll receive nil for the list, but nil is a valid empty list
|
|
||
| // AddBlockedIP adds a single CIDR to the block set at runtime. | ||
| func (fw *Firewall) AddBlockedIP(cidr string) error { | ||
| func (fw *Firewall) AddBlockedIP(address string) error { |
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.
We switched to inserting blocked IPs and not using CIDRs, which seems cleaner to me.
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.
Function comment still refers to CIDRs
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.
you can insert:
- One IP
- Cidr
- IP range (1.1.1.0-1.1.1.1)
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.
Can we make it cidr only to simplify thinking around? I think api should move all three types (i dont like IP range tho) to CIDR format.
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.
removed support for IP ranges, passing now addresses only as CIDR blocks to the orchestrator. In the DB, they're saved as user provided
|
Btw @dobrac can we check with deny all hyperloop server is still working? |
5857131 to
a0f052f
Compare
a0f052f to
61047a4
Compare
| network.Egress = &orchestrator.SandboxNetworkEgressConfig{} | ||
| } | ||
| network.Egress.DeniedCidrs = []string{internetBlockCIDR} | ||
| } |
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: Global Config Overwrites User Network Access
Global config AllowSandboxInternet can override user-provided network configuration when AllowInternetAccess is not explicitly set in the request. If a user provides custom network.denyOut addresses without setting allowInternetAccess, and the global config is false, the orchestrator overwrites DeniedCidrs with ["0.0.0.0/0"], discarding the user's explicit configuration. This logic duplicates what the API layer already does and contradicts the TODO comment indicating it should be removed now that network config is passed from the API.
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.
I think here we should fallback to s.config.AllowSandboxInternet only when customers configuration is not provided at all (both allow internet access flag and egress internet config).
cc @dobrac
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.
thats desired
|
Looks like hyperloop server is not accessible with the internet disabled. |
Add sandbox network out traffic configuration. The usage would look then something like this:
Note
Adds configurable egress allow/deny for sandboxes, wires it through API → orchestrator, enforces via firewall, and persists in snapshots; updates proto/spec and tests.
SandboxNetworkConfig(allowOut,denyOut) to spec and generated types; includenetworkinNewSandbox. Clarifyallow_internet_accesssemantics (equivalent todenyOut: ["0.0.0.0/0"]when false).networkto orchestrator on create/resume/connect.allowInternetAccessis false, setdeny 0.0.0.0/0.SandboxNetworkConfigand egress fields.snapshots.config(JSONB) via migration; definePausedSandboxConfigandSandboxNetworkConfigtypes; store network config on pause; update queries/models.Written by Cursor Bugbot for commit 738177c. This will update automatically on new commits. Configure here.