Skip to content

Change default exposed port to 49983 for E2B compatibility#207

Merged
fslongjin merged 3 commits into
TencentCloud:masterfrom
turtlemoji:change-default-exposed-port-49983
May 14, 2026
Merged

Change default exposed port to 49983 for E2B compatibility#207
fslongjin merged 3 commits into
TencentCloud:masterfrom
turtlemoji:change-default-exposed-port-49983

Conversation

@turtlemoji
Copy link
Copy Markdown
Contributor

Summary

The default exposed ports (8080, 32000) are not useful for E2B-compatible sandboxes. This PR replaces them with 49983 across all three affected components.

Changes

  • CubeMaster: Update defaultTemplateExposedPorts() and add tests verifying the new default and reserved port behavior
  • Cubelet: Update DefaultExposedPorts, shipped config.toml, and the validation error message to reflect one reserved port instead of two
  • network-agent: Update DefaultConfig() and add a test for the new default

Test plan

  • CubeMaster: go test ./pkg/templatecenter/ -run TestDefaultTemplateExposedPorts
  • CubeMaster: go test ./pkg/templatecenter/ -run TestCountCustomTemplateExposedPortsTreats49983AsReserved
  • Cubelet: go test ./network/ -run TestDefaultExposedPortsContainsOnly49983
  • network-agent: go test ./internal/service/ -run TestDefaultConfigExposedPorts
  • All existing exposed port tests still pass
  • gofmt clean on all changed files

Closes #204

Comment thread Cubelet/config/config.toml Outdated
mvm_gw_dest_ip = "169.254.68.5"
mvm_mtu = 1300
default_exposed_ports = [8080, 32000]
default_exposed_ports = [49983]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this issue.

I have a question in mind: Do we need these configurations in three components?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes @chenhengqi, all three need updating because each component uses its own default independently — CubeMaster uses it for template validation, Cubelet for building port mappings, and network-agent for the actual network enforcement. If we only changed one, the others would still use the old ports and wouldn't match. That said, consolidating them into a single file / source of truth would be cleaner.

Copy link
Copy Markdown
Contributor Author

@turtlemoji turtlemoji May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consolidating into a shared constant package tho, I recommend doing it on another PR. (Let me know 😃)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If CubeMaster already set default exposed port to 49983 on template creation, Cubelet should be able to obtain it through realReq.GetExposedPorts().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thats a better workflow than shared constant @chenhengqi, I think for that to work we will need to remove the overrides in cubelet and network-agent tho

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thats a better workflow than shared constant @chenhengqi, I think for that to work we will need to remove the overrides in cubelet and network-agent tho

Right, I think we should remove those ports from cubelet and network-agent.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to implement the changes, should I commit in this same pr with the same format? component: desc

Just do it in this PR.

Copy link
Copy Markdown
Contributor Author

@turtlemoji turtlemoji May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main fixes on 5ca7d31 and f9618d6

Little correction and go fmt e606b0e
32f0a42

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reorganize your commits. One commit per component.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done @chenhengqi.

}

if reqPorts > 3 {
if len(realReq.GetExposedPorts()) > 4 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

> 5?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenhengqi

  • Old: 2 defaults + 3 customs = 5 total
  • New: 1 default + 3 customs = 4 total

The old limit was 2 defaults + 3 custom = 5. Since we're down to 1 default (49983) and CubeMaster still allows max 3 custom ports (countCustomTemplateExposedPorts > 3), I went with 1 + 3 = 4. Happy to bump to 5 if you'd prefer to keep the old ceiling as to be more permissive.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the other 2 mentions of cubebot,

  1. Will push a new test, this is correct

  2. This is consistent with the previous behavior. The old code also didn't dedupe; it counted each occurrence of a non-default port, so [9000, 9000, 9000, 9000] would have hit the > 3 custom limit and been rejected. Cubelet and network-agent both dedupe via maps downstream (portReq in plugin_tap.go, byContainerPort in normalizePortMappings), so duplicates don't cause double mappings — only the count-based validation rejects them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed test fix for 2) and reorganized commits

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chenhengqi

  • Old: 2 defaults + 3 customs = 5 total
  • New: 1 default + 3 customs = 4 total

The old limit was 2 defaults + 3 custom = 5. Since we're down to 1 default (49983) and CubeMaster still allows max 3 custom ports (countCustomTemplateExposedPorts > 3), I went with 1 + 3 = 4. Happy to bump to 5 if you'd prefer to keep the old ceiling as to be more permissive.

Sounds reasonable. Let's keep it as 4 for now.

@chenhengqi
Copy link
Copy Markdown
Collaborator

/cubebot review this pr.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 13, 2026

Claude finished @chenhengqi's task in 3m 59s —— View job


PR Review: Change default exposed port to 49983

  • Gather context: read changed files
  • Understand the changes and review discussion
  • Provide detailed code review feedback

Overall Assessment

The architectural direction is correct — making CubeMaster the single source of truth for the default exposed port (49983) and removing the redundant defaults from Cubelet and network-agent is a clean improvement. The cleanup is thorough: DefaultExposedPorts is fully removed from Cubelet's network plugin, the config.toml field is gone, and network-agent no longer injects default ports.

However, there are a few issues to address:


Issues

1. Exposed port limit inconsistency (addressing @chenhengqi's > 5 comment)

In Cubelet/services/cubebox/check.go:55:

if len(realReq.GetExposedPorts()) > 4 {

The old logic allowed 3 custom ports + 2 defaults = 5 total. The new limit of 4 reduces the user-facing budget. Meanwhile, CubeMaster still allows 3 custom ports (countCustomTemplateExposedPorts > 3 at template_image.go:2255). Since the request now carries both the default (49983) and custom ports, CubeMaster would allow 4 ports total (49983 + 3 custom), which aligns with Cubelet's > 4.

But consider whether the intent is to preserve the total port budget (5 = 4 custom + 1 default) or the custom port budget (3). If the goal is to give users the same number of custom ports as before (3), then > 4 is correct. If the goal is to keep the same total (5), it should be > 5. This is a product decision, but the CubeMaster limit of 3 custom ports suggests > 4 was intentional. A comment explaining the arithmetic would help future readers.

2. Missing test for Cubelet's exposed port limit

The exposed port validation in check.go:55 changed from a custom-port-count check to a total-port-count check, but no test was added for this new behavior. The existing check_test.go has tests for volumes and container names but nothing for exposed ports. Consider adding a test like:

func TestCheckParamExposedPortLimit(t *testing.T) {
    // 4 ports should pass
    // 5 ports should fail
    // port 0 and 65536 should fail
}

3. Duplicate port handling removed silently

The old code in check.go iterated through DefaultExposedPorts and skipped them when counting, which implicitly handled the case where a user explicitly included a default port in their request — it wouldn't double-count. The new code uses len(realReq.GetExposedPorts()), which counts all ports. If a user includes 49983 in their request AND the template already adds 49983, the port could appear in the list twice (depending on upstream deduplication). Verify that GetExposedPorts() returns deduplicated values, or add deduplication here.


Minor Nits

4. Error message phrasingcheck.go:57: "exposed ports should be at most 4" is clearer than the old message, good improvement.

5. Clean removal — The network.DefaultExposedPorts variable, the config override logic in plugin_tap.go:345-348, and the port injection in plugin_tap.go:768-770 are all cleanly removed. No dead references remain (verified by grepping for DefaultExposedPorts and default_exposed_ports across Cubelet and network-agent).

6. Test coverage in CubeMaster — The new tests TestDefaultTemplateExposedPortsContainsOnly49983 and TestCountCustomTemplateExposedPortsTreats49983AsReserved are well-structured and test the right things.

7. network-agent config removalDefaultExposedPorts field, TOML parsing, normalizePortMappings injection, and config summary logging are all consistently removed. The config_test.go test fixture was updated to match.


Summary

The core change is clean and the architecture is improved. Main items to address:

  1. Clarify whether the port limit should be 4 or 5 (align with @chenhengqi's feedback)
  2. Add a test for the Cubelet exposed port validation
  3. Confirm that duplicate ports (e.g., user specifying 49983 explicitly) are handled correctly

The old defaults (8080, 32000) are not useful for E2B-compatible sandboxes. Replace them with 49983 to match the E2B sandbox protocol.

Closes TencentCloud#204

Signed-off-by: turtlemoji <turtlemoji@protonmail.com>
CubeMaster is the single source of truth for default exposed ports. Remove the independent defaults from Cubelet so it only uses ports received in the request.

Signed-off-by: turtlemoji <turtlemoji@protonmail.com>
CubeMaster is the single source of truth for default exposed ports. Remove the independent defaults and port seeding from network-agent.

Signed-off-by: turtlemoji <turtlemoji@protonmail.com>
@turtlemoji turtlemoji force-pushed the change-default-exposed-port-49983 branch from 19b0135 to c96e030 Compare May 13, 2026 07:52
Copy link
Copy Markdown
Collaborator

@chenhengqi chenhengqi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@chenhengqi
Copy link
Copy Markdown
Collaborator

@fslongjin Please take a look.

@fslongjin fslongjin merged commit df8d6c4 into TencentCloud:master May 14, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[good first issue] Change default exposed port to 49983

3 participants