Enable port mapping and network configuration for jobs #4855
Conversation
WalkthroughThis pull request introduces several network-related modifications across the codebase. Test cases have been updated to replace references to Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Executor
participant PortAllocator
participant NetworkSetup
participant Container
Client->>Executor: Submit RunCommandRequest
Executor->>PortAllocator: AllocatePorts(execution)
PortAllocator-->>Executor: Return port mappings
Executor->>NetworkSetup: setupNetworkForJob(params)
NetworkSetup-->>Executor: Configure network (host/bridge)
Executor->>Container: Start container with mapped ports
Container-->>Executor: Execution complete
Executor->>PortAllocator: ReleasePorts(execution)
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2){"Issues":[{"FromLinter":"typecheck","Text":"pattern build/: no matching files found","Severity":"","SourceLines":["//go:embed build/"],"Replacement":null,"Pos":{"Filename":"webui/webui.go","Offset":0,"Line":23,"Column":12},"ExpectNoLint":false,"ExpectedNoLintLinter":""}],"Report":{"Linters":[{"Name":"asasalint"},{"Name":"asciicheck"},{"Name":"bidichk"},{"Name":"bodyclose","Enabled":true},{"Name":"canonicalheader"},{"Name":"containedctx"},{"Name":"contextcheck"},{"Name":"copyloopvar","Enabled":true},{"Name":"cyclop"},{"Name":"decorder"},{"Name":"deadcode"},{"Name":"depguard","Enabled":true},{"Name":"dogsled","Enabled":true},{"Name":"dupl"},{"Name":"dupword"},{"Name":"durationcheck"},{"Name":"errcheck","Enabled":true,"EnabledByDefault":true},{"Name":"errchkjson"},{"Name":"errname"},{"Name":"errorlint"},{"Name":"execinquery"},{"Name":"exhaustive"},{"Name":"exhaustivestruct"},{"Name":"exhaustruct"},{"Name":"exportloopref"},{"Name":"forbidigo","Enabled":true},{"Name":"forcetypeassert"},{"Name":"fatcontext"},{"Name":"funlen","Enabled":true},{"Name":"gci"},{"Name":"ginkgolinter"},{"Name":"gocheckcompilerdirectives"},{"Name":"gochecknoglobals"},{"Name":"gochecknoinits","Enabled":true},{"Name":"gochecksumtype"},{"Name":"gocognit"},{"Name":"goconst","Enabled":true},{"Name":"gocritic"},{"Name":"gocyclo","Enabled":true},{"Name":"godot"},{"Name":"godox"},{"Name":"err113"},{"Name":"gofmt","Enabled":true},{"Name":"gofumpt"},{"Name":"goheader"},{"Name":"goimports","Enabled":true},{"Name":"golint"},{"Name":"mnd","Enabled":true},{"Name":"gomnd"},{"Name":"gomoddirectives"},{"Name":"gomodguard"},{"Name":"goprintffuncname","Enabled":true},{"Name":"gosec","Enabled":true},{"Name":"gosimple","Enabled":true,"EnabledByDefault":true},{"Name":"gosmopolitan"},{"Name":"govet","Enabled":true,"EnabledByDefault":true},{"Name":"grouper"},{"Name":"ifshort"},{"Name":"iface"},{"Name":"importas"},{"Name":"inamedparam"},{"Name":"ineffassign","Enabled":true,"EnabledByDefault":true},{"Name":"interfacebloat"},{"Name":"interfacer"},{"Name":"intrange"},{"Name":"ireturn"},{"Name":"lll","Enabled":true},{"Name":"loggercheck"},{"Name":"maintidx"},{"Name":"makezero"},{"Name":"maligned"},{"Name":"mirror"},{"Name":"misspell"},{"Name":"musttag"},{"Name":"nakedret","Enabled":true},{"Name":"nestif"},{"Name":"nilerr"},{"Name":"nilnil"},{"Name":"nlreturn"},{"Name":"noctx","Enabled":true},{"Name":"nonamedreturns"},{"Name":"nosnakecase"},{"Name":"nosprintfhostport"},{"Name":"paralleltest"},{"Name":"perfsprint"},{"Name":"prealloc"},{"Name":"predeclared"},{"Name":"promlinter"},{"Name":"protogetter"},{"Name":"reassign"},{"Name":"recvcheck"},{"Name":"revive"},{"Name":"rowserrcheck"},{"Name":"sloglint"},{"Name":"scopelint"},{"Name":"sqlclosecheck"},{"Name":"spancheck"},{"Name":"staticcheck","Enabled":true,"EnabledByDefault":true},{"Name":"structcheck"},{"Name":"stylecheck","Enabled":true},{"Name":"tagalign"},{"Name":"tagliatelle"},{"Name":"tenv"},{"Name":"testableexamples"},{"Name":"testifylint"},{"Name":"testpackage"},{"Name":"thelper"},{"Name":"tparallel"},{"Name":"typecheck","Enabled":true,"EnabledByDefault":true},{"Name":"unconvert","Enabled":true},{"Name":"unparam"},{"Name":"unused","Enabled":true,"EnabledByDefault":true},{"Name":"usestdlibvars"},{"Name":"varcheck"},{"Name":"varnamelen"},{"Name":"wastedassign"},{"Name":"whitespace","Enabled":true},{"Name":"wrapcheck"},{"Name":"wsl"},{"Name":"zerologlint"},{"Name":"nolintlint","Enabled":true}]}} ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (2)
webui/lib/api/schema/swagger.json (1)
1879-1893: 🧹 Nitpick (assertive)Update Network enum documentation.
The
models.Networkenum has been updated to replaceNetworkFullwithNetworkHostand addNetworkBridge. Consider adding descriptions for each enum value to improve API documentation.Add descriptions using the
x-enum-descriptionsfield:"x-enum-varnames": [ "NetworkNone", "NetworkHost", "NetworkHTTP", "NetworkBridge" + ], + "x-enum-descriptions": [ + "No network access", + "Direct access to host network stack (Linux only)", + "Restricted HTTP(S) access to specified domains", + "Isolated network with port mapping support" ]pkg/swagger/swagger.json (1)
1880-1892: 🧹 Nitpick (assertive)Update the Network enumeration values.
The updatedmodels.Networkenum now lists:NetworkNone,NetworkHost,NetworkHTTP, andNetworkBridge. Note that the legacy mode has been replaced byNetworkHostand a newNetworkBridgehas been added. However, the appearance ofNetworkHTTPis not mentioned in the PR summary (which specifies only three network modes: bridge, host, none). Please verify that the inclusion ofNetworkHTTPis intentional and that all references in the codebase (and documentation) have been updated accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
webui/lib/api/generated/types.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (26)
cmd/cli/docker/docker_run_cli_test.go(1 hunks)go.mod(1 hunks)pkg/bidstrategy/semantic/networking_allowlist_test.go(2 hunks)pkg/bidstrategy/semantic/networking_test.go(2 hunks)pkg/compute/envvars.go(1 hunks)pkg/compute/envvars_test.go(1 hunks)pkg/compute/executor.go(8 hunks)pkg/compute/node_info_decorator.go(4 hunks)pkg/compute/port_allocator.go(1 hunks)pkg/compute/port_allocator_test.go(1 hunks)pkg/compute/types.go(1 hunks)pkg/config/defaults.go(1 hunks)pkg/config/types/compute.go(2 hunks)pkg/executor/docker/executor.go(1 hunks)pkg/executor/docker/executor_test.go(3 hunks)pkg/executor/docker/network.go(3 hunks)pkg/models/execution.go(1 hunks)pkg/models/network.go(8 hunks)pkg/models/network_string.go(1 hunks)pkg/models/node_info.go(1 hunks)pkg/node/compute.go(4 hunks)pkg/swagger/docs.go(6 hunks)pkg/swagger/swagger.json(6 hunks)pkg/test/executor/test_runner.go(2 hunks)pkg/test/utils/platform.go(1 hunks)webui/lib/api/schema/swagger.json(6 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
pkg/executor/docker/executor_test.go
[warning] 792-792: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: lint / lint
- GitHub Check: build / Build Binary (linux, amd64)
🔇 Additional comments (38)
pkg/compute/port_allocator.go (3)
55-109: Potential leftover used ports on partial failure
The cleanup mechanism is good. However, ifAllocatePortsallocates some ports but fails midway, are you certain all partially allocated ports are freed? This logic appears to handle that (cleanup()function), but double-check if any subsequent error path might skip thecleanup()call.
165-175: Port release workflow
ReleasePortssuccessfully cleans up allocated ports from both the global map and the per-execution map. Looks correct. Ensure that if an execution ID is reused or re-run, it does not rely on previously allocated ports if it was never properly released. Monitoring code flow for a guaranteed call toReleasePortsupon job completion is crucial.
177-178: Nice interface compliance check
Verifyingvar _ PortAllocator = &portAllocator{}is helpful for compile-time guarantees.pkg/executor/docker/network.go (3)
71-74: Confirm usage of single struct parameter
Adoptingparams *executor.RunCommandRequestsimplifies parameter passing, but ensure that all dependent fields (e.g., environment, volumes, etc.) remain accessible or require no further changes.
75-75: Toggle NetworkDisabled explicitly
containerConfig.NetworkDisabled = params.Network.Disabled()is clear. Just be sure that you never need partial or device-only networking. If partial network usage arises in the future, you might need more granular toggles.
114-114: Re-check references to job vs. executionID
Passingparams.JobID, params.ExecutionID, params.NetworktocreateHTTPGatewayis correct, but confirm if there are other references to the old signature in this file or tests.pkg/models/network.go (7)
21-23: Renamed NetworkFull to NetworkHost
Renaming improves clarity. The extra comment specifying direct host network access is helpful for maintainers.
56-59: Bridge mode addition
IntroducingNetworkBridgeclarifies default container networking. This is a solid addition, ensuring bridging is recognized explicitly in the code.
92-95: Ports field is a valuable extension
AddingPortstoNetworkConfigis a great enhancement. This paves the way for more flexible networking configurations.
114-115: Ensure nil-slice normalization
Lines 114-115: Initializingn.Portsto an empty slice avoids nil pointer checks later. Good housekeeping.
136-185: Strict domain rules
The additional domain checks are thorough. The logic thatDomainsare only valid forNetworkHTTPis consistent with the intended design. Mirror this approach forPortsusage for other modes.
295-310: PortMapping struct
Names must start with a letter. This is a good practice to avoid environment variable conflicts. The separation betweenStaticandTargetclarifies host vs. container concepts.
312-341: Validate method
Validation ensures ports remain in 0–65535 range. That’s crucial. The name pattern is also enforced, which helps unify environment variable naming. This is well-designed.pkg/swagger/docs.go (6)
1888-1889: Enum values for network modes are consistent.The updated enumeration for
NetworkBridgealigns with the revised network type definitions.
1893-1895: Enum varnames alignment confirmed.Adding "NetworkHost", "NetworkHTTP", and "NetworkBridge" consistently reflects the newly introduced network modes.
1907-1913: Validate port usage for different network modes.While the
Portsproperty is well-documented, ensure that any constraints (e.g., requiring Bridge mode to set container ports) are correctly enforced at runtime.
2030-2046: PortMapping documentation is clear.The field descriptions are thorough. Verify that the uniqueness of the
Namefield is enforced if needed.
2575-2582: Network configuration field addition is consistent.This addition meets the PR's objective of configurable networking. Be mindful of default behaviors in tasks that do not specify network requirements.
2902-2918: Port range configuration is well-defined.Introducing
AdvertisedAddressand the port range parameters aligns with dynamic port allocation. Ensure that collisions are handled robustly.pkg/test/utils/platform.go (1)
8-15: Provide cross-platform alternatives where needed.This helper is effective for Linux-only tests. Consider whether separate logic or alternative tests might be necessary on other platforms.
pkg/models/network_string.go (2)
12-14: Enumeration references are updated.The added references to
NetworkHostandNetworkBridgeare consistent with the newly defined constants.
17-19: String representation indices updated.Renaming
_Network_nameand adjusting_Network_indexfor the new enum entry is correct. Double-check index boundaries to avoid off-by-one errors.pkg/bidstrategy/semantic/networking_test.go (1)
36-38: LGTM! Network type updates align with the new networking model.The changes correctly reflect the renaming of 'full' mode to 'host' mode while maintaining the same test coverage and validation logic.
pkg/bidstrategy/semantic/networking_allowlist_test.go (1)
38-43: LGTM! Network type updates maintain domain filtering behavior.The changes consistently reflect the renaming of network types while preserving the same domain filtering logic and test coverage.
pkg/compute/node_info_decorator.go (1)
44-44: LGTM! Node address advertisement implementation.The implementation correctly handles the advertised address field, enabling node discovery as described in the PR objectives.
Also applies to: 62-62
pkg/compute/types.go (1)
51-64: Well-designed interface for port allocation!The
PortAllocatorinterface is well-documented and follows good design principles:
- Clear separation of concerns between allocation and release
- Comprehensive documentation of behavior
- Thread-safety consideration for concurrent allocations
- Proper error handling for allocation failures
pkg/compute/envvars_test.go (2)
149-186: LGTM! Well-structured test case for network ports.The test case thoroughly validates the environment variable generation for network ports, including:
- Named port mappings with different target ports
- Port mapping with same target port
- Multiple port mappings in a single execution
187-213: LGTM! Good negative test case.The test case correctly verifies that no port environment variables are set when a network configuration exists but has no ports defined.
pkg/compute/port_allocator_test.go (3)
15-21: LGTM! Well-structured test suite setup.The test suite is properly set up using the
testify/suitepackage, which provides a clean and organized way to group related test cases.
23-45: LGTM! Good negative test cases.The test cases correctly verify that empty mappings are returned when:
- No network configuration is provided
- Network type is set to
None
47-83: LGTM! Comprehensive port allocation test.The test case thoroughly validates both static and dynamic port allocation scenarios:
- Dynamic port allocation within range
- Static port outside range
- Static port inside range
pkg/node/compute.go (1)
108-114: LGTM! Port allocator initialization looks good.The port allocator is properly initialized with configuration parameters and includes error handling.
pkg/compute/executor.go (1)
181-193: LGTM! Port allocation and cleanup are properly handled.The implementation:
- Allocates ports using the port allocator
- Updates the execution with allocated ports
- Includes cleanup to release ports after execution
pkg/executor/docker/executor.go (1)
370-370: LGTM! Simplified network setup function call.The refactoring improves maintainability by passing the entire request object instead of individual fields.
cmd/cli/docker/docker_run_cli_test.go (1)
405-405: LGTM! Updated network type assertion.The test has been correctly updated to use
models.NetworkHostinstead ofmodels.NetworkFull, aligning with the PR's objective to rename the 'full' mode to 'host' for better clarity.pkg/swagger/swagger.json (3)
1377-1380: Add theaddressproperty in ComputeNodeInfo.
The newaddressproperty undermodels.ComputeNodeInfoprovides the network location (IPv4 or hostname) where the compute node can be reached, which aligns with the enhanced network configuration requirements. Ensure that downstream components correctly consume and validate this value.
1903-1909: Introduce thePortsproperty in NetworkConfig.
Adding thePortsproperty to themodels.NetworkConfigdefinition enables tasks to specify port mappings, as required for dynamic and static port allocation. Ensure that clients populating this field also conform to the schema defined inmodels.PortMapping.
2898-2914: Enhance types.NetworkConfig with AdvertisedAddress and port range settings.
The updatedtypes.NetworkConfignow includes:
•AdvertisedAddress– the address advertised to other nodes (with auto-discovery as a fallback),
•PortRangeStartandPortRangeEnd– defining the inclusive range from which ports can be allocated to jobs.These additions improve the network configuration capabilities for compute nodes. Please verify that the rest of the system (e.g. dynamic port allocation logic) correctly consumes and enforces these new configurations.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
pkg/test/devstack/networking_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
- GitHub Check: lint / lint
🔇 Additional comments (11)
pkg/test/devstack/networking_test.go (11)
1-4: Build tag and package definition look good.
No issues with the//go:build integration || !unitapproach for integration tests.
5-24: Imports appear correct and consistent.
All imported packages seem necessary for integration tests.
25-33: Test suite struct initialization is straightforward.
No concurrency or initialization issues detected when storing the server and port fields.
69-73: Graceful teardown.
Closing the HTTP server inTearDownSuiteensures no orphan resources remain.
75-95: Scenario creation is well-structured.
The helper function nicely configures DevStack with the required network settings for each test.
97-114: Host networking test is validated properly.
Skipping on non-Linux platforms is appropriate, and verifying the "OK" response confirms local server accessibility.
116-131: Bridge networking test mirrors host mode checks.
The usage ofhost.docker.internalto reach the local server is correct for bridge mode.
133-151: Network none test ensures isolation.
Verifying an empty stdout and curl's exit code 7 confirms that no external networking is available.
191-228: Host mode port mapping test is thorough.
Confirmed that the container port can be reached directly on the host.
230-251: Privileged port test is a solid negative scenario.
Verifies that mapping to restricted ports fails as expected in bridge mode.
153-189:❓ Verification inconclusive
Bridge mode port mapping is correctly validated.
Good approach to verify the mapped host port. One minor check: ensurencis available inbusybox:1.37.0.
🏁 Script executed:
#!/bin/bash # Confirm that busybox:1.37.0 includes nc docker run --rm busybox:1.37.0 which ncLength of output: 138
Action Required: Manually verify the availability of
ncin busybox:1.37.0.
Bridge mode port mapping is validated and the testing approach is solid. However, we could not automatically confirm that thebusybox:1.37.0image includes thenccommand because the automated environment did not have Docker available. Please verify manually thatncis present in the image to ensure the test’s reliability.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
pkg/models/task_test.go (1)
19-46: 🧹 Nitpick (assertive)Consider adding more network normalization test cases.
The test could be expanded to verify normalization of:
- Different network types
- Port mapping defaults
- Domain configurations
♻️ Duplicate comments (1)
pkg/test/devstack/networking_test.go (1)
35-67: 🧹 Nitpick (assertive)Consider adding retry mechanism for port allocation.
While the
Eventuallyblock ensures server readiness, the port allocation could fail in concurrent test environments.Consider wrapping the port allocation in a retry mechanism:
- // Get a free port - port, err := network.GetFreePort() - s.Require().NoError(err) - s.port = port + // Get a free port with retry + var port int + s.Require().Eventually(func() bool { + var err error + port, err = network.GetFreePort() + return err == nil + }, 5*time.Second, 100*time.Millisecond) + s.port = port
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
pkg/compute/port_allocator.go(1 hunks)pkg/lib/validate/numbers.go(1 hunks)pkg/lib/validate/numbers_test.go(1 hunks)pkg/models/network.go(8 hunks)pkg/models/network_test.go(0 hunks)pkg/models/task.go(1 hunks)pkg/models/task_test.go(5 hunks)pkg/test/devstack/networking_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- pkg/models/network_test.go
🧰 Additional context used
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
pkg/models/network.go
[failure] 336-336:
The line is 155 characters long, which exceeds the maximum of 140 characters. (lll)
[failure] 340-340:
Magic number: 256, in detected (mnd)
[failure] 346-346:
Magic number: 1024, in detected (mnd)
[failure] 349-349:
Magic number: 65535, in detected (mnd)
🪛 GitHub Actions: Main Pipeline
pkg/models/network.go
[error] 336-336: The line is 155 characters long, which exceeds the maximum of 140 characters. (lll)
🔇 Additional comments (15)
pkg/lib/validate/numbers.go (2)
41-49: LGTM! Well-implemented less-than comparison.The implementation is clean, follows the established pattern, and includes clear documentation.
51-59: LGTM! Well-implemented less-than-or-equal comparison.The implementation is clean, follows the established pattern, and includes clear documentation.
pkg/lib/validate/numbers_test.go (2)
119-145: LGTM! Comprehensive test coverage for IsLessThan.The test cases cover all essential scenarios including boundary conditions and different numeric types.
147-173: LGTM! Comprehensive test coverage for IsLessOrEqual.The test cases cover all essential scenarios including boundary conditions and different numeric types.
pkg/compute/port_allocator.go (4)
22-31: Consider clarifying concurrency usage in code comments.Adding a short note in the struct-level comments about how the mutex protects against concurrent allocations would help future maintainers understand the concurrency guarantees at a glance.
119-136: Consider providing an ephemeral fallback for static ports.When a static port is specified but unavailable at runtime, the allocator currently errors out. As previously mentioned in past reviews, having an optional fallback to a dynamic port (when the user has not strictly required the specific port) can improve resiliency under port conflicts.
127-132: Remember “time-of-check vs. time-of-use” pitfalls for port binding.By calling
net.Listenhere, there is still a small window where a previously open port can be claimed by another process. This can become more common under heavy system load or parallel job submissions. Consider documenting these edge cases or adopting an OS-allocated ephemeral port approach for robust collision avoidance.
138-182: Validate retry or backoff logic for high-scale port collisions.The round-robin search is straightforward but may still collide if the system is under load and many ports are temporarily bound. Past reviews have highlighted this approach as generally good, yet you might consider a retry mechanism or a short backoff if collisions are frequent at scale.
pkg/models/network.go (2)
21-23: Rename from NetworkFull to NetworkHost is clear and consistent.Renaming the legacy “full” mode to
NetworkHostclarifies the intent. This change is a welcome improvement for readability and future maintenance.
56-59: The addition of NetworkBridge is well-structured.Defining an explicit
NetworkBridgemode is a clear approach to containerized network isolation. This is beneficial for clarity and extends the range of supported network setups.pkg/models/task.go (1)
158-160: Network validation addition looks correct.Ensuring the network configuration is valid early in the submission lifecycle reduces potential runtime errors and maintains consistency with other validated fields.
pkg/test/devstack/networking_test.go (2)
75-95: LGTM! Well-structured helper method.The helper method effectively sets up test scenarios with networking enabled.
191-228: Same cleanup concern as bridge mode test.The cleanup recommendation from the bridge mode test applies here as well.
pkg/models/task_test.go (2)
216-280: LGTM! Comprehensive network validation test cases.The test cases effectively cover various network validation scenarios including:
- Invalid network types
- Port mapping validation
- Domain validation
320-357: LGTM! Thorough deep copy verification.The test effectively ensures that network configuration and port mappings are properly deep copied.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (1)
pkg/swagger/docs.go (1)
1883-1897: 🧹 Nitpick (assertive)Add descriptions for network types.
Consider adding descriptions for each network type in the enum to help API consumers understand when to use each mode. For example:
NetworkNone: Complete network isolationNetworkHost: Direct access to host network stackNetworkBridge: Isolated network with port mapping supportNetworkHTTP: HTTP-only network access
♻️ Duplicate comments (7)
pkg/compute/port_allocator.go (5)
13-22: 🧹 Nitpick (assertive)Consider documenting concurrency behavior more explicitly.
You already use a mutex, but adding a brief comment explaining how concurrent allocations are handled helps future maintainers understand the thread-safety guarantees.
24-44: 🛠️ Refactor suggestionValidate the start port’s upper bound.
Though you checkstart >= MinimumPortandend <= MaximumPort, you don’t verifystart <= MaximumPort. This can silently allow invalid ranges ifstartis above the max port.if err := errors.Join( validate.IsGreaterOrEqual(start, models.MinimumPort, "start port must be >= %d", models.MinimumPort), validate.IsLessOrEqual(end, models.MaximumPort, "end port must be <= %d", models.MaximumPort), + validate.IsLessOrEqual(start, models.MaximumPort, "start port must be <= %d", models.MaximumPort), validate.IsLessThan(start, end, "start port must be less than end port"), ); err != nil {
46-108: 🧹 Nitpick (assertive)Address race conditions in port checking.
Usingnet.Listen()to verify port availability can still introduce a time-of-check vs. time-of-use race if another process binds that port between checks. Consider ephemeral port binding or more robust collision handling when concurrency is high.
110-127: 🧹 Nitpick (assertive)Offer fallback for unavailable static ports.
Currently, if a requested static port is taken, you return an error. Some users might want fallback to an ephemeral port (unless they strictly require the static port). Consider making this configurable.
129-173: 🧹 Nitpick (assertive)Revisit single-pass round-robin logic.
If you’re near the end of the range and do one modulo-based pass, collisions can occur under high load. You might want a retry or backoff mechanism for better long-term resilience.pkg/compute/envvars.go (1)
57-67:⚠️ Potential issuePort name validation needed.
The implementation correctly adds environment variables for port discovery. However, port names are used directly in environment variable names without validation.
Apply this diff to validate port names:
if execution.Job.Task().Network != nil { for _, port := range execution.Job.Task().Network.Ports { + // Validate port name contains only valid characters for environment variables + if !isValidEnvVarName(port.Name) { + return nil, fmt.Errorf("invalid port name %q: must contain only alphanumeric and underscore characters", port.Name) + } if port.Static > 0 { sysEnv[models.EnvVarHostPortPrefix+port.Name] = fmt.Sprintf("%d", port.Static) }pkg/compute/port_allocator_test.go (1)
47-83: 🧹 Nitpick (assertive)Consider adding more edge cases.
While the test covers basic functionality well, consider adding these edge cases:
- Invalid port numbers (negative ports, ports > 65535)
- Duplicate port names
- Empty port names
- Concurrent port allocation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
webui/lib/api/generated/types.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (10)
cmd/cli/docker/docker_run_cli_test.go(2 hunks)pkg/compute/envvars.go(1 hunks)pkg/compute/port_allocator.go(1 hunks)pkg/compute/port_allocator_test.go(1 hunks)pkg/models/constants.go(1 hunks)pkg/models/network.go(8 hunks)pkg/swagger/docs.go(6 hunks)pkg/swagger/swagger.json(6 hunks)pkg/test/devstack/networking_test.go(1 hunks)webui/lib/api/schema/swagger.json(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (34)
pkg/compute/port_allocator.go (3)
1-12: Looks good!
All initial declarations and imports are sensible, and the package scoping is appropriate.
175-185: Clean and concise release mechanism.
Releasing ports by removing them from bothusedPortsandusedExecutionPortsis straightforward and appears correct.
187-188: Interface compliance confirmed.
The compile-time check reliably ensuresportAllocatorsatisfiesPortAllocator.pkg/test/devstack/networking_test.go (10)
1-2: Build tags are correct.
The test is clearly marked to run in integration or non-unit contexts.
35-67: Robust server setup.
Using a free port and verifying availability viaEventuallyprovides stable test initialization.
69-73: Graceful server teardown confirmed.
The suite properly shuts down the HTTP server inTearDownSuite.
75-95: Scenario configuration approach looks good.
This helper neatly centralizes environment setup for networked jobs.
97-114: Host networking coverage is sufficient.
Skipping on non-Linux avoids platform quirks. The test verifies local host connectivity correctly.
116-131: Bridge networking coverage is solid.
Usinghost.docker.internalis a standard approach; the test logic is straightforward and verifies the correct connectivity outcome.
133-151: None networking mode test is correct.
Ensuring the job cannot contact the host server aligns with the expected outcome forNetworkNone.
153-189: Dynamic port mapping in Bridge mode is tested well.
The continuous listen and eventual check confirm the exposure on the host port. This approach addresses ephemeral port reuse.
191-228: Host mode port mapping coverage is valid.
Verifying direct host port exposure ensures consistency with the networking design.
230-251: Negative test scenario for privileged port usage.
This effectively ensures that invalid mappings are rejected, covering a key error path.pkg/models/network.go (10)
15-25: Use of constants is appropriate.
DefiningMinimumPortandMaximumPortclarifies port-bound constraints for easy maintenance.
33-35: RenamingNetworkFulltoNetworkHostis clear and consistent.
Maintaining a comment about legacy usage helps with backward compatibility.
68-71: New network type recognized.
NetworkBridgeclarifies container-level isolation while preserving outbound connectivity.
103-107: NewPortsfield expands flexibility.
Providing explicit port mappings inNetworkConfigis a valuable enhancement.
125-127: Nil slice handling is correct.
Initializingn.Portsto an empty slice prevents nil-pointer usage later.
141-141: Cloning ports ensures safe copies.
IncludingPorts: slices.Clone(n.Ports)is consistent with the rest of the copy logic.
151-151: Network type bounds check.
Validatingn.Typeagainst known values prevents invalid or out-of-range enumerations.
153-155: HTTP domains check is appropriately enforced.
Restricting domain usage toNetworkHTTPaligns with the design constraints.
169-195: Port mappings validated thoroughly.
RequiringNetworkHostorNetworkBridgefor ports is consistent, while also checking duplicates and gatingTargetusage.
307-380: PortMapping struct is well-defined.
All validation rules make sense, from naming constraints to checking port ranges and IP validity. Good completeness.pkg/models/constants.go (1)
17-21: LGTM! Well-documented port environment variable prefixes.The new constants follow the existing naming pattern and are properly documented.
pkg/compute/port_allocator_test.go (1)
23-31: LGTM! Clean test setup.The test suite setup is clean and follows testing best practices.
cmd/cli/docker/docker_run_cli_test.go (2)
405-405: LGTM! Network type updated correctly.The test correctly reflects the renaming of 'full' network mode to 'host' as described in the PR objectives.
438-441: LGTM! Correct error handling for invalid network configuration.The test now correctly expects an error when domains are specified with 'none' network type.
pkg/swagger/docs.go (2)
2030-2050: Well-documented port mapping model!The
PortMappingmodel is clearly documented with comprehensive descriptions for each field, making it easy for API consumers to understand the port mapping configuration options.
2906-2922: LGTM: Clear network configuration model!The
NetworkConfigmodel is well-structured with clear documentation for port range configuration and address advertisement.webui/lib/api/schema/swagger.json (2)
1377-1380: New Compute Node Address Property AddedA new property
addresshas been introduced inmodels.ComputeNodeInfoto specify the network location of a compute node. This addition improves node discoverability by clearly defining its network identity.
1884-1891: Updated Network EnumerationThe enumeration for
models.Networkhas been updated to includeNetworkNone,NetworkHost,NetworkHTTP, andNetworkBridge. This change reflects the renaming of the legacyfullmode tohostand adds additional network types. Please verify that all documentation and dependent code are updated accordingly to match this new schema.pkg/swagger/swagger.json (3)
1375-1379: New Property Addition in ComputeNodeInfo.
The new"address"property has been added tomodels.ComputeNodeInfoto provide the network location (IPv4 or hostname) where the compute node can be reached. Please ensure that API consumers and client libraries are updated accordingly and that any validation (if required) is implemented consistently.
1880-1893: Clarify Network Enumeration Values.
Themodels.Networkenum now lists four values:"NetworkNone","NetworkHost","NetworkHTTP", and"NetworkBridge". According to the PR objectives, only three modes (none, host, and bridge) are intended. Please confirm whether"NetworkHTTP"is an additional, intended option or if further documentation/adjustment is required to align the enum with the objectives.
2902-2918: New Types.NetworkConfig Definition.
The newtypes.NetworkConfigdefinition (with properties"AdvertisedAddress","PortRangeEnd", and"PortRangeStart") provides the necessary configuration for compute nodes’ networking. Please verify that the auto-discovery mechanism for the advertised address and the port range allocation logic render correct network behavior in all deployment scenarios.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
pkg/models/network.go(8 hunks)pkg/models/network_test.go(1 hunks)pkg/test/devstack/networking_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build / Build Binary (windows, amd64)
- GitHub Check: build / Build Binary (darwin, arm64)
- GitHub Check: build / Build Binary (darwin, amd64)
- GitHub Check: build / Build Binary (linux, armv6)
- GitHub Check: build / Build Binary (linux, armv7)
- GitHub Check: build / Build Binary (linux, arm64)
- GitHub Check: build / Build Binary (linux, amd64)
🔇 Additional comments (3)
pkg/test/devstack/networking_test.go (1)
44-46: Consider adding a retry mechanism for ephemeral port conflicts
Even though a free port is obtained, parallel test runs may still cause conflicts. A retry or fallback to a different port upon failure would reduce flakiness in large test environments.pkg/models/network.go (1)
148-208: Excellent comprehensive validation
Your approach to validating domains, port mappings, and network types is robust, leveragingerrors.Jointo aggregate multiple errors. This level of detail significantly helps diagnose user configuration issues.pkg/models/network_test.go (1)
123-317: Great test coverage
The test suite comprehensively covers valid and invalid scenarios for bothNetworkConfigandPortMapping, which enhances confidence in the code’s correctness and error handling.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
pkg/models/network.go (1)
147-221: 🛠️ Refactor suggestionRefactor the
Validatemethod to reduce cyclomatic complexity
The gocyclo report indicates a complexity of 21. Consider extracting domain validation, port mapping checks, etc., into smaller helper functions to simplify this logic.🧰 Tools
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
[failure] 147-147:
cyclomatic complexity 21 of func(*NetworkConfig).Validateis high (> 18) (gocyclo)🪛 GitHub Actions: Main Pipeline
[warning] 147-147: cyclomatic complexity 21 of func
(*NetworkConfig).Validateis high (> 18) (gocyclo)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
pkg/compute/port_allocator.go(1 hunks)pkg/compute/port_allocator_test.go(1 hunks)pkg/models/execution.go(1 hunks)pkg/models/execution_test.go(1 hunks)pkg/models/network.go(8 hunks)pkg/models/network_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint / go-lint (ubuntu-latest)
pkg/models/network.go
[failure] 147-147:
cyclomatic complexity 21 of func (*NetworkConfig).Validate is high (> 18) (gocyclo)
🪛 GitHub Actions: Main Pipeline
pkg/models/network.go
[warning] 147-147: cyclomatic complexity 21 of func (*NetworkConfig).Validate is high (> 18) (gocyclo)
🔇 Additional comments (14)
pkg/compute/port_allocator.go (3)
13-23: Add concurrency rationale in struct documentation
It would be helpful to briefly describe how concurrent port allocations are handled, clarifying the mutex usage and potential interactions with other processes that may also bind to ports.
111-129: Revisit time-of-check vs. time-of-use on static port allocation
Usingnet.Listenand then closing it introduces a window where another external process could claim the port. Consider documenting, or referencing, how the system manages this risk or how frequently it occurs.
130-174: Consider race conditions and fallback for dynamic port allocation
The round-robin approach combined with a quick bind-and-release can collide with external processes. You might consider a persistent bind pattern or OS-allocated ephemeral ports to reduce collisions.pkg/models/network.go (2)
15-25: Nice introduction of shared port constants
Centralizing numeric port bounds improves clarity and avoids “magic numbers.” Good job.
377-379: Align error message with computed max port name
The error message still says “max 256 characters,” yetmaxPortNameis256 - len(EnvVarHostPortPrefix). Update the message to reflect the actual limit.pkg/models/network_test.go (7)
123-449: Comprehensive test coverage for NetworkConfig_Validate
This set of tests covers a wide range of valid/invalid networking configurations, ensuring the new logic holds up well.
451-603: Thorough tests for PortMapping validation
These scenarios capture the key failure paths (e.g., privileged ports, invalid names), improving reliability.
605-641: DomainSet tests look good
The coverage ensures that distinct and normalized domains behave as expected.
643-645: Use a standard substring check
The current implementation omits the last character and can produce false negatives. Consider usingstrings.Containsinstead.
647-657: Simplicity of equalStringSlices
This helper is straightforward and works well for these test assertions.
659-690: Proper deep copy checks
Verifying that changes to the original config do not affect the copy ensures safe usage.
692-741: Normalization test coverage
Ensuring domains are trimmed, lowercased, and that ports are initialized prevents subtle errors. Good coverage.pkg/models/execution_test.go (1)
17-208: Consider adding more test cases.The test suite has good coverage but could benefit from additional test cases:
- Test with invalid port numbers (e.g., negative ports, ports > 65535)
- Test with duplicate port names
- Test with empty port names
- Test concurrent port allocation
pkg/models/execution.go (1)
265-278: Add validation for port values.The method correctly updates port mappings but should validate that:
- Port values are within valid range (0-65535)
- Port names are not empty
- Port names are unique within the network configuration
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (10)
pkg/compute/port_allocator.go (2)
46-109: 🧹 Nitpick (assertive)Consider fallback for unavailable static ports.
When a static port is unavailable, the current logic returns an error. Some users might prefer a fallback to an ephemeral/dynamic port if the static one can’t be bound.
130-174: 🧹 Nitpick (assertive)Round-robin logic correctness.
The single pass through the range using round-robin helps distribute allocations, but if the range is nearly exhausted under heavy concurrency, collisions could still occur. Consider additional retry or backoff to manage race conditions more gracefully.
pkg/models/network.go (1)
344-417: 🧹 Nitpick (assertive)Synchronize error message with maxPortName.
When
len(p.Name) > maxPortName, the error message still says “max 256 characters.” Consider using the constant in the formatted string:- return fmt.Errorf("port name too long (max 256 characters)") + return fmt.Errorf("port name too long (max %d characters)", maxPortName)pkg/models/execution.go (1)
265-277: 🛠️ Refactor suggestionAdd validation for port values.
The method correctly updates port mappings but should validate that:
- Port values are within valid range (0-65535)
- Port names are not empty
- Port names are unique within the network configuration
Apply this diff to add validation:
func (e *Execution) AllocatePorts(portMap PortMap) { if e.Job == nil || e.Job.Task().Network == nil { return } + // Validate port mappings + seenNames := make(map[string]bool) + for _, port := range portMap { + if port.Name == "" { + return // Skip empty port names + } + if port.Static < 0 || port.Static > 65535 { + return // Skip invalid static ports + } + if port.Target < 0 || port.Target > 65535 { + return // Skip invalid target ports + } + if seenNames[port.Name] { + return // Skip duplicate port names + } + seenNames[port.Name] = true + } // Replace the existing port mappings with the allocated ones if portMap == nil { e.Job.Task().Network.Ports = make(PortMap, 0) } else { e.Job.Task().Network.Ports = portMap.Copy() } }pkg/models/network_test.go (1)
677-679: 🛠️ Refactor suggestionRevise the 'contains' helper.
The current implementation removes the last character and performs multiple extra checks, potentially causing false negatives. Consider using a standard substring approach with
strings.Contains.-func contains(s, substr string) bool { - return s != "" && substr != "" && s != substr && s[len(s)-1] != '.' && ... -} +func contains(s, substr string) bool { + return strings.Contains(s, substr) +}pkg/executor/docker/executor_test.go (4)
657-692: 🧹 Nitpick (assertive)Ensure host mode test is properly guarded for Linux-only execution.
The test correctly uses
SkipIfNotLinuxas Docker host mode networking is only supported on Linux platforms. However, there are a few suggestions to improve the test:
- Add a comment explaining why host mode is Linux-only.
- Consider using a more descriptive port name than "http" since this is a test case.
Apply this diff to improve the test:
func (s *ExecutorTestSuite) TestPortMappingInHostMode() { + // Docker host mode networking is only supported on Linux as it requires direct access + // to the host's network namespace testutils.SkipIfNotLinux(s.T(), "docker host mode is not supported on non-linux platforms") port, err := network.GetFreePort() s.Require().NoError(err) es, err := dockermodels.NewDockerEngineBuilder("busybox:1.37.0"). WithEntrypoint("sh", "-c", fmt.Sprintf(`while true; do echo -e "HTTP/1.1 200 OK\n\nOK" | nc -l -p %d; done`, port)). Build() s.Require().NoError(err) task := mock.Task() task.Engine = es task.Network = &models.NetworkConfig{ Type: models.NetworkHost, Ports: []*models.PortMapping{ { - Name: "http", + Name: "test-port", Static: port, }, }, }
731-784: 🧹 Nitpick (assertive)Improve test reliability by using separate test servers.
The test uses a single shell script to start two netcat servers on different ports. This could lead to race conditions or one server failing to start without affecting the other.
Apply this diff to improve test reliability:
- es, err := dockermodels.NewDockerEngineBuilder("busybox:1.37.0"). - WithEntrypoint("sh", "-c", ` - (while true; do echo -e "HTTP/1.1 200 OK\n\nOK" | nc -l -p 80; done) & - (while true; do echo -e "HTTP/1.1 200 OK\n\nOK" | nc -l -p 81; done) - `). + es, err := dockermodels.NewDockerEngineBuilder("busybox:1.37.0"). + WithEntrypoint("sh", "-c", ` + # Start first server with proper error handling + start_server() { + while true; do + echo -e "HTTP/1.1 200 OK\n\nOK" | nc -l -p $1 || { + echo "Server on port $1 failed, retrying..." >&2 + sleep 1 + } + done + } + start_server 80 & + start_server 81 & + wait + `). Build()
786-844: 🛠️ Refactor suggestionImprove test coverage for host service accessibility.
The test verifies host service access but has a few areas for improvement:
- The test server is bound to all interfaces (
:0), which could be a security concern.- The test doesn't verify the response body matches expectations.
Apply these changes to improve the test:
// Start a simple HTTP server on the host handler := http.NewServeMux() handler.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { - w.Write([]byte("host-service")) + w.Header().Set("Content-Type", "text/plain") + w.Write([]byte("host-service-response")) }) - listener, err := net.Listen("tcp", ":0") // Let OS choose port + // Bind to localhost for security + listener, err := net.Listen("tcp", "127.0.0.1:0") s.Require().NoError(err) defer listener.Close() server := &http.Server{Handler: handler} go server.Serve(listener) defer server.Close() // Get the port that was assigned hostPort := listener.Addr().(*net.TCPAddr).Port tests := []struct { name string networkType models.Network hostURL string + expected string }{ { name: "host network mode", networkType: models.NetworkHost, hostURL: fmt.Sprintf("http://localhost:%d", hostPort), + expected: "host-service-response", }, { name: "bridge network mode", networkType: models.NetworkBridge, hostURL: fmt.Sprintf("http://host.docker.internal:%d", hostPort), + expected: "host-service-response", }, } for _, tt := range tests { s.Run(tt.name, func() { if tt.networkType == models.NetworkHost { testutils.SkipIfNotLinux(s.T(), "docker host mode is not supported on non-linux platforms") } // Create a task that tries to connect to our test server es, err := dockermodels.NewDockerEngineBuilder(CurlDockerImage). WithEntrypoint("curl", "-s", tt.hostURL). Build() s.Require().NoError(err) task := mock.Task() task.Engine = es task.Network = &models.NetworkConfig{ Type: tt.networkType, } stdout, err := s.runJobGetStdout(task, fmt.Sprintf("access-host-%s", tt.networkType)) s.Require().NoError(err) - s.Equal("host-service", stdout) + s.Equal(tt.expected, stdout) }) }🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 792-792: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control(avoid-bind-to-all-interfaces-go)
694-729: 🧹 Nitpick (assertive)Add test coverage for dynamic port allocation in bridge mode.
The test verifies port mapping in bridge mode with a static port, but it would be beneficial to also test dynamic port allocation when
Staticis not specified.Add a new test case for dynamic port allocation:
func (s *ExecutorTestSuite) TestPortMappingInBridgeModeWithDynamicPort() { // Create a container that listens on the target port es, err := dockermodels.NewDockerEngineBuilder("busybox:1.37.0"). WithEntrypoint("sh", "-c", `while true; do echo -e "HTTP/1.1 200 OK\n\nOK" | nc -l -p 80; done`). Build() s.Require().NoError(err) task := mock.Task() task.Engine = es task.Network = &models.NetworkConfig{ Type: models.NetworkBridge, Ports: []*models.PortMapping{ { Name: "http", Target: 80, // Container port }, }, } // Start the container s.startJob(task, "bridge-network-dynamic-port-test") // Get the allocated port from environment variables // TODO: Add logic to retrieve the dynamically allocated port // Verify the port is accessible s.Require().Eventually(func() bool { resp, err := http.Get(fmt.Sprintf("http://localhost:%d", allocatedPort)) if err != nil { return false } defer resp.Body.Close() return resp.StatusCode == http.StatusOK }, 5*time.Second, 100*time.Millisecond) }webui/lib/api/schema/swagger.json (1)
2025-2045: 🧹 Nitpick (assertive)Enhance Port Mapping Schema
The
models.Portdefinition now documents properties for"HostNetwork","Name","Static", and"Target". It is recommended to explicitly add a"required": ["Name"]constraint to enforce the mandatory nature of the port mapping identifier. Additionally, consider applying numeric validations (for example, setting"minimum": 1and"maximum": 65535") for the"Static"and"Target"fields and including examples to improve schema clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
webui/lib/api/generated/types.gen.tsis excluded by!**/generated/**
📒 Files selected for processing (14)
pkg/compute/envvars_test.go(1 hunks)pkg/compute/port_allocator.go(1 hunks)pkg/compute/port_allocator_test.go(1 hunks)pkg/compute/types.go(1 hunks)pkg/executor/docker/executor_test.go(3 hunks)pkg/models/execution.go(1 hunks)pkg/models/execution_test.go(1 hunks)pkg/models/network.go(8 hunks)pkg/models/network_test.go(3 hunks)pkg/models/task_test.go(5 hunks)pkg/swagger/docs.go(6 hunks)pkg/swagger/swagger.json(6 hunks)pkg/test/devstack/networking_test.go(1 hunks)webui/lib/api/schema/swagger.json(6 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
pkg/executor/docker/executor_test.go
[warning] 792-792: "Detected a network listener listening on 0.0.0.0 or an empty string.
This could unexpectedly expose the server publicly as it binds to all
available interfaces. Instead, specify another IP address that is not
0.0.0.0 nor the empty string."
Context: net.Listen("tcp", ":0")
Note: [CWE-200] Exposure of Sensitive Information to an Unauthorized Actor [REFERENCES]
- https://owasp.org/Top10/A01_2021-Broken_Access_Control
(avoid-bind-to-all-interfaces-go)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (30)
pkg/compute/port_allocator.go (3)
24-44: Range validation looks good.The constructor properly checks start < end and adheres to [1024, 65535]. No further issues found here.
111-128: Time-of-check vs time-of-use race condition.Binding the port via
net.Listenat allocation time mitigates some conflicts but still can’t fully guarantee the port remains free by actual container startup. If multiple processes or external services allocate ports concurrently, collisions may arise between allocation and container bind time.
176-186: Release mechanism is clear.The release logic properly frees ports associated with the execution, which allows reusability. Make sure all call sites consistently invoke
ReleasePortsto avoid leaks.pkg/test/devstack/networking_test.go (5)
35-67: Server initialization approach looks fine.Using
Eventuallyto verify readiness is a reliable pattern, preventing transient test failures from ephemeral port conflicts.
97-114: Host networking test is well-structured.The test coverage for host networking is sufficient. No issues found.
116-131: Bridge networking test is straightforward.The test confirms outbound connectivity via
host.docker.internal:; no further concerns.
133-151: Networking-none test is valid.Verifying that localhost remains inaccessible in none mode helps ensure correct isolation.
230-344: Negative tests are comprehensive.This covers privileged ports, out-of-range ports, target-port misuse, and duplicate mappings. Great job ensuring multiple invalid configurations lead to job submission failures.
pkg/models/network.go (6)
30-35: RenamingNetworkFulltoNetworkHostis clear.The updated naming helps reduce confusion.
68-71: Bridge mode documentation is helpful.The introduction of
NetworkBridgeclarifies the default container networking scenario.
103-106: New Ports field is a useful addition.Centralizing port mappings under
NetworkConfigensures clarity and easier validation.
124-126: PortMap initialization is consistent.Normalizing
Portsto an empty slice instead of nil prevents potential nil-pointer checks downstream.
146-182: Validation logic for network modes is solid.Enforcing that ports can only be set for Host or Bridge is consistent with your design.
292-335: PortMap validation is thorough.Checking for duplicate names, static ports, and host/bridge constraints ensures correct usage.
pkg/compute/types.go (1)
51-64: LGTM! Well-designed interface for port allocation.The
PortAllocatorinterface is well-structured with clear responsibilities, good documentation, and consideration for thread safety.pkg/models/execution_test.go (1)
17-208: Add more test cases for edge scenarios.While the test coverage is good, consider adding the following test cases:
- Invalid port numbers (negative ports, ports > 65535)
- Duplicate port names
- Empty port names
- Concurrent port allocation
pkg/compute/port_allocator_test.go (3)
178-196: Enhance port range exhaustion tests.Add test cases for:
- Exhaustion with a mix of static and dynamic ports
- Recovery after port release
- Concurrent allocation attempts
61-97: Add validation for port values.The test should validate:
- Port values are within valid range (0-65535)
- Port names are unique
- Host network bindings are preserved
247-283: LGTM! Thorough testing of bridge mode port allocation.The test case effectively validates:
- Dynamic port allocation within range
- Static port preservation
- Target port defaulting behavior
- Multiple port configurations
pkg/models/task_test.go (2)
42-42: LGTM! Good defensive programming.The assertion ensures that port mappings are properly initialized during task normalization, preventing potential nil pointer dereferences.
216-280: LGTM! Comprehensive test coverage for network validation.The test cases thoroughly validate network configurations including:
- Network type validation
- Port mapping name validation
- Privileged port restrictions
- Domain configuration rules
pkg/models/network_test.go (1)
485-621: LGTM! Excellent test coverage for port mapping validation.The test cases thoroughly validate port mappings including:
- Name validation (empty, invalid chars, length)
- Port range validation (privileged, max value)
- Host network validation
- Edge cases (zero ports, IPv6)
pkg/swagger/docs.go (4)
1883-1897: LGTM! Network type enum updated correctly.The changes to the
Networkenum align well with the PR objectives:
- Added
NetworkBridgefor bridge mode support- Renamed
NetworkFulltoNetworkHostfor better clarity
1898-1917: LGTM! Network configuration updated to support port mapping.The addition of the
Portsfield toNetworkConfigenables the port mapping functionality described in the PR objectives.
2029-2049: LGTM! Port mapping model is well-designed.The
Portstruct provides a comprehensive model for port mapping with:
- Required name identifier for environment variables
- Flexible port allocation with optional static host port
- Bridge mode support through target container port
- Network interface binding control
2578-2585: LGTM! Compute node configuration updated for network support.The addition of the
Networkfield enables compute nodes to specify their network configuration.webui/lib/api/schema/swagger.json (1)
1880-1893: Verify Updated Network EnumerationThe updated
models.Networkenumeration now lists four values:"NetworkNone","NetworkHost","NetworkHTTP", and"NetworkBridge". Since the PR objectives mention supporting only three modes—none,host, andbridge(withhostreplacing the legacy"full"mode)—please verify that"NetworkHTTP"is an intended additional mode and not an oversight.pkg/swagger/swagger.json (3)
1375-1378: NewaddressField inmodels.ComputeNodeInfo
The addition of theaddressproperty enhances node discovery by allowing the compute node to explicitly advertise its reachable network address. Consider adding validation (for example, a regex pattern) to enforce correct formatting (IPv4 address or hostname) if your validation framework supports schema constraints.
1884-1891: Review Network Mode Enumeration
The updatedmodels.Networkenum now includes four values:"NetworkNone","NetworkHost","NetworkHTTP", and"NetworkBridge". However, the PR objectives mention only three network modes (bridge,host, andnone), with legacy values being renamed. Please verify that the inclusion of"NetworkHTTP"is intentional and, if so, ensure that corresponding documentation and implementation details are updated to reflect this additional mode.
1903-1908: Ports Array inmodels.NetworkConfig
The newPortsproperty withinmodels.NetworkConfigcorrectly references the newly definedmodels.Porttype. Make sure that downstream consumers correctly implement the auto-allocation of ports when theStaticproperty is omitted and default theTargetport to the host port in Bridge mode as expected.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
pkg/models/network.go(8 hunks)pkg/models/network_test.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: testcontainers-suite / tests
🔇 Additional comments (16)
pkg/models/network.go (10)
15-25: Good use of descriptive constants.
DefiningMinimumPort,MaximumPort, andmaxPortNameis a clean approach that replaces magic numbers with clear, maintainable values, aligning with best practices.
30-35: Renaming NetworkFull to NetworkHost is well-handled.
This change clarifies the intended behavior and retains backward compatibility through explicit naming.
68-71: Clear introduction of NetworkBridge.
The added documentation effectively explains the new bridge-based networking mode.
76-82: Legacy “full” fallback
Great job preserving backward compatibility by mapping the legacy "full" value toNetworkHost. Consider documenting a deprecation plan if this is only transitional.
124-126: Consistent Ports initialization.
Ensuringn.Portsis never nil helps avoid panics and keeps the code consistent with the rest of the struct’s normalization behavior.
140-140: Deep copying of Ports.
Referencingn.Ports.Copy()inside theCopy()method is a robust approach to ensure immutability across copies.
146-182: Comprehensive validation for NetworkConfig.
This logical check ensures domains and ports are restricted to appropriate network modes, providing clarity and preventing misconfigurations. No issues spotted.
292-335: Robust PortMap validation.
The checks for duplicate names, static ports, and target ports are well-structured and improve reliability. Good job leveragingerrors.Joinfor aggregated errors.
337-342: Safe Copy for PortMap.
Returning a copied slice ensures changes to one PortMap don't leak into another. Looks good.
344-417: Refine error messages to use shared constants.
Using65535directly in errors (e.g., line 400) instead ofMaximumPortcan lead to inconsistencies if the constant changes. Similarly, consider referencingMinimumPortin error messages for the privileged port range.pkg/models/network_test.go (6)
9-18: Smooth integration with testify/suite.
Defining a suite struct and running it withsuite.Runneatly organizes these tests for maintainability and readability.
20-71: Thorough domain validation tests.
The table-driven approach covers a range of valid and invalid domains, including IP addresses and corner cases like dot-prefixed domains. This well verifiesNetworkHTTPdomain constraints.
114-152: Effective testing of DomainSet functionality.
Ensuring wildcard and duplicate domain handling is properly tested eliminates potential surprises in filtering.
154-196: Comprehensive domain matching tests.
Verifying comparisons for equality and non-equality across wildcard and case differences ensures robust coverage ofmatchDomain.
198-228: Deep copy validation.
These tests confirm that changes to the originalNetworkConfigdo not affect the copy, preventing unintended side effects.
230-279: NetworkConfig normalization tests.
Excellent approach verifying nil inputs, empty slices, and domain trimming. This ensures consistent state handling under a variety of conditions.
| func (s *NetworkTestSuite) TestPortValidation() { | ||
| tests := []struct { | ||
| name string | ||
| port Port | ||
| wantErr bool | ||
| errMsg string | ||
| }{ | ||
| { | ||
| name: "valid port", | ||
| port: Port{ | ||
| Name: "http", | ||
| Static: 8080, | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "missing name", | ||
| port: Port{ | ||
| Static: 8080, | ||
| }, | ||
| wantErr: true, | ||
| errMsg: "port mapping name is required", | ||
| }, | ||
| // ... other test cases ... | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| s.Run(tt.name, func() { | ||
| err := tt.port.Validate() | ||
| if tt.wantErr { | ||
| s.Error(err) | ||
| if tt.errMsg != "" { | ||
| s.Contains(err.Error(), tt.errMsg) | ||
| } | ||
| } else { | ||
| assert.NoError(t, err) | ||
| s.NoError(err) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Port validation test coverage.
The test table adequately checks for required names, verifying typical constraints. Consider adding explicit cases for out-of-range ports and negative port values to confirm error handling.
This PR introduces networking capabilities to Bacalhau jobs, enabling containerized workloads to expose services and communicate with host networks. This is a significant enhancement that opens up new use cases for Bacalhau and enabling inter-job communication.
Core Changes
1. Network Modes
bridgehost: (Linux only) Direct access to host network stacknone: Complete network isolation2. Port Mapping System
PortMappingconfiguration:3. Environment Variables
Added standardized environment variables for port discovery:
BACALHAU_HOST_PORT_{name}: Host port mappingBACALHAU_PORT_{name}: Container port mappingExample Usage:
Testing:
Future Work:
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation