Skip to content

Conversation

@Luap99
Copy link
Member

@Luap99 Luap99 commented Apr 22, 2025

see commits

Summary by Sourcery

Enhance network configuration to allow creating networks with the same subnet when using VLAN or unmanaged mode

New Features:

  • Allow creating multiple bridge networks with the same subnet when using VLAN option
  • Support creating multiple unmanaged networks with the same subnet or network interface

Enhancements:

  • Modify network creation logic to be more flexible with subnet and interface conflicts
  • Improve handling of network configuration options for bridge networks

Tests:

  • Add test cases for creating networks with same subnet using VLAN option
  • Add test cases for creating unmanaged networks with same subnet and interface

Luap99 added 3 commits April 22, 2025 19:41
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]>
@sourcery-ai
Copy link

sourcery-ai bot commented Apr 22, 2025

Reviewer's Guide by Sourcery

This pull request allows the creation of multiple networks with the same subnet or interface when using the vlan or unmanaged options. This is achieved by skipping subnet and bridge conflict checks when these options are present. New tests were added to verify the new functionality.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Allows the creation of multiple networks with the same subnet when using the vlan option.
  • Unsets usedNetworks when the vlan option is present to avoid subnet conflicts.
  • Skips bridge conflict checks when the vlan option is present.
libnetwork/netavark/config.go
Allows the creation of multiple networks with the same subnet or interface when using the unmanaged option.
  • Unsets usedNetworks when the mode=unmanaged option is present to avoid subnet conflicts.
  • Skips bridge conflict checks when the mode=unmanaged option is present.
libnetwork/netavark/config.go
Adds tests to verify the creation of multiple networks with the same subnet when using the vlan or unmanaged options.
  • Adds a test case for creating two networks with the vlan option and the same subnet.
  • Adds a test case for creating a network with mode=unmanaged and the same subnet as an existing network.
  • Adds a test case for creating a network with mode=unmanaged and the same interface as an existing network.
libnetwork/netavark/config_test.go

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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 usedNetworks is set to nil in certain cases.
  • The logic for checkBridgeConflict seems 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
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
Copy link

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:

  1. Extract common logic: Create a helper adjustBridgeOptions that processes VLAN and Mode options to handle the side effects (e.g., unsetting usedNetworks and adjusting checkBridgeConflict).
  2. Simplify the main logic: Remove the duplicated conditionals from the main switch branch and call the helper once.
  3. 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.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 22, 2025

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

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]>
@Luap99
Copy link
Member Author

Luap99 commented Apr 25, 2025

@mheon @baude PTAL

@baude
Copy link
Member

baude commented Apr 28, 2025

LGTM, thanks @Luap99

@mheon
Copy link
Member

mheon commented Apr 28, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Apr 28, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 9aeb335 into containers:main Apr 28, 2025
15 checks passed
@Luap99 Luap99 deleted the subnet-conflict branch April 28, 2025 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants