-
Notifications
You must be signed in to change notification settings - Fork 225
libnetwork/netavark: allow existing host subnet/interface with the vlan and unmanaged option #2426
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
As we expect users to use a pre existing bridge with mode=unmanaged we should not error on a subnet conflict. Fixes: containers#2322 Signed-off-by: Paul Holzinger <[email protected]>
As the unmnaged mode is intended to be used with existing bridges and there isn't really a reason why a user could should not be allowed to add two networks on the same bridge with it. Signed-off-by: Paul Holzinger <[email protected]>
When a vlan is used there should be no subnet conflict check. As the vlan separates the routing it should be valid. Fixes: containers/podman#25736 Signed-off-by: Paul Holzinger <[email protected]>
Reviewer's Guide by SourceryThis pull request allows the creation of multiple networks with the same subnet or interface when using the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @Luap99 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding comments to explain why
usedNetworksis set to nil in certain cases. - The logic for
checkBridgeConflictseems a bit complex; can it be simplified?
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| internalutil.MapDockerBridgeDriverOptions(newNetwork) | ||
|
|
||
| var vlan int | ||
| checkBridgeConflict := true |
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.
issue (complexity): Consider extracting the repeated logic for unsetting usedNetworks and adjusting checkBridgeConflict into a helper function to reduce nesting and duplicated conditional logic, improving code clarity and maintainability without changing functionality
Consider extracting the repeated logic for unsetting usedNetworks and adjusting checkBridgeConflict into a helper function. This can reduce the overall nesting and duplicated conditional logic while keeping functionality unchanged. For example:
func adjustBridgeOptions(options map[string]string, usedNetworks *interface{}) (bool, error) {
checkBridgeConflict := true
for key, value := range options {
switch key {
case types.VLANOption:
// Validate VLAN
if _, err := internalutil.ParseVlan(value); err != nil {
return false, err
}
// When VLAN is set, allow multiple bridge configs on the same bridge.
*usedNetworks = nil
checkBridgeConflict = false
case types.ModeOption:
switch value {
case types.BridgeModeManaged:
// No changes needed here.
case types.BridgeModeUnmanaged:
// For unmanaged mode, also unset usedNetworks and relax conflict check.
*usedNetworks = nil
checkBridgeConflict = false
default:
return false, fmt.Errorf("unknown bridge mode %q", value)
}
}
}
return checkBridgeConflict, nil
}Then in your main flow you can simplify the switch branch:
case types.BridgeNetworkDriver:
internalutil.MapDockerBridgeDriverOptions(newNetwork)
checkBridgeConflict := true
// Validate options and adjust bridge configuration flags.
// For reference, make sure to process VLAN and Mode options first.
if got, err := adjustBridgeOptions(newNetwork.Options, &usedNetworks); err != nil {
return nil, err
} else {
checkBridgeConflict = got
}
// Process the rest of the options which don't need extra handling.
for key, value := range newNetwork.Options {
switch key {
case types.MTUOption:
if _, err := internalutil.ParseMTU(value); err != nil {
return nil, err
}
case types.IsolateOption:
if val, err := internalutil.ParseIsolate(value); err != nil {
return nil, err
} else {
newNetwork.Options[types.IsolateOption] = val
}
case types.MetricOption:
if _, err := strconv.ParseUint(value, 10, 32); err != nil {
return nil, err
}
// ... other options ...
}
}
if err := internalutil.CreateBridge(n, newNetwork, usedNetworks, n.defaultsubnetPools, checkBridgeConflict); err != nil {
return nil, err
}Actionable Steps:
- Extract common logic: Create a helper
adjustBridgeOptionsthat processes VLAN and Mode options to handle the side effects (e.g., unsettingusedNetworksand adjustingcheckBridgeConflict). - Simplify the main logic: Remove the duplicated conditionals from the main switch branch and call the helper once.
- Maintain functionality: Ensure that validations for other options remain in the loop, keeping their logic intact.
This refactor should reduce nesting and clarify the configuration flow while keeping all existing behaviors.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Do not use Default() in unit tests, that caches the config in a global var so it conflicts with other tests also calling it. Now only the Reload test should test that. The Default() call got broken by commit ee04b4f because now the test only unsets the env after the last Reload() call which should already use the actual default. Fixes: ee04b4f ("*_test.go: use t.TempDir, t.Setenv") Signed-off-by: Paul Holzinger <[email protected]>
|
LGTM, thanks @Luap99 |
|
/lgtm |
see commits
Summary by Sourcery
Enhance network configuration to allow creating networks with the same subnet when using VLAN or unmanaged mode
New Features:
Enhancements:
Tests: