From 528ffa9cae2aeafec9223cc84686e0623c9e50e8 Mon Sep 17 00:00:00 2001 From: andrew <> Date: Sun, 4 Feb 2024 01:50:40 +0900 Subject: [PATCH] Allow multiple macvlan networks to share a parent The only case where macvlan interfaces are unable to share a parent is when the macvlan mode is passthru. This change tightens the check to that situation. It also makes the error message more specific to avoid suggesting that sharing parents is never correct. Signed-off-by: Andrew Baxter <423qpsxzhh8k3h@s.rendaw.me> Signed-off-by: Sebastiaan van Stijn --- integration/internal/network/ops.go | 13 ++ integration/network/helpers.go | 9 ++ integration/network/macvlan/macvlan_test.go | 141 +++++++++++++++++- libnetwork/drivers/macvlan/macvlan_network.go | 69 ++++++--- 4 files changed, 211 insertions(+), 21 deletions(-) diff --git a/integration/internal/network/ops.go b/integration/internal/network/ops.go index cc07e842fe..49f762ecae 100644 --- a/integration/internal/network/ops.go +++ b/integration/internal/network/ops.go @@ -45,6 +45,19 @@ func WithMacvlan(parent string) func(*network.CreateOptions) { } } +// WithMacvlanPassthru sets the network as macvlan with the specified parent in passthru mode +func WithMacvlanPassthru(parent string) func(options *network.CreateOptions) { + return func(n *network.CreateOptions) { + n.Driver = "macvlan" + n.Options = map[string]string{ + "macvlan_mode": "passthru", + } + if parent != "" { + n.Options["parent"] = parent + } + } +} + // WithIPvlan sets the network as ipvlan with the specified parent and mode func WithIPvlan(parent, mode string) func(*network.CreateOptions) { return func(n *network.CreateOptions) { diff --git a/integration/network/helpers.go b/integration/network/helpers.go index 771a4b4cd8..cf823cf86d 100644 --- a/integration/network/helpers.go +++ b/integration/network/helpers.go @@ -42,6 +42,15 @@ func LinkExists(ctx context.Context, t *testing.T, master string) { testutil.RunCommand(ctx, "ip", "link", "show", master).Assert(t, icmd.Success) } +// LinkDoesntExist verifies that a link doesn't exist +func LinkDoesntExist(ctx context.Context, t *testing.T, master string) { + // verify the specified link doesn't exist, ip link show . + testutil.RunCommand(ctx, "ip", "link", "show", master).Assert(t, icmd.Expected{ + ExitCode: 1, + Err: "does not exist", + }) +} + // IsNetworkAvailable provides a comparison to check if a docker network is available func IsNetworkAvailable(ctx context.Context, c client.NetworkAPIClient, name string) cmp.Comparison { return func() cmp.Result { diff --git a/integration/network/macvlan/macvlan_test.go b/integration/network/macvlan/macvlan_test.go index 538cdf0fb1..929f785505 100644 --- a/integration/network/macvlan/macvlan_test.go +++ b/integration/network/macvlan/macvlan_test.go @@ -62,6 +62,18 @@ func TestDockerNetworkMacvlan(t *testing.T) { }, { name: "OverlapParent", test: testMacvlanOverlapParent, + }, { + name: "OverlapParentPassthruFirst", + test: testMacvlanOverlapParentPassthruFirst, + }, { + name: "OverlapParentPassthruSecond", + test: testMacvlanOverlapParentPassthruSecond, + }, { + name: "OverlapDeleteCreatedSecond", + test: testMacvlanOverlapDeleteCreatedSecond, + }, { + name: "OverlapKeepExistingParent", + test: testMacvlanOverlapKeepExisting, }, { name: "NilParent", test: testMacvlanNilParent, @@ -99,7 +111,8 @@ func TestDockerNetworkMacvlan(t *testing.T) { } func testMacvlanOverlapParent(t *testing.T, ctx context.Context, client client.APIClient) { - // verify the same parent interface cannot be used if already in use by an existing network + // verify the same parent interface can be used if already in use by an existing network + // as long as neither are passthru master := "dm-dummy0" n.CreateMasterDummy(ctx, t, master) defer n.DeleteInterface(ctx, t, master) @@ -110,6 +123,42 @@ func testMacvlanOverlapParent(t *testing.T, ctx context.Context, client client.A net.WithMacvlan(parentName), ) assert.Check(t, n.IsNetworkAvailable(ctx, client, netName)) + n.LinkExists(ctx, t, parentName) + + overlapNetName := "dm-parent-net-overlap" + _, err := net.Create(ctx, client, overlapNetName, + net.WithMacvlan(parentName), + ) + assert.Check(t, err == nil) + + // delete the second network while preserving the parent link + err = client.NetworkRemove(ctx, overlapNetName) + assert.NilError(t, err) + assert.Check(t, n.IsNetworkNotAvailable(ctx, client, overlapNetName)) + n.LinkExists(ctx, t, parentName) + + // delete the first network + err = client.NetworkRemove(ctx, netName) + assert.NilError(t, err) + assert.Check(t, n.IsNetworkNotAvailable(ctx, client, netName)) + n.LinkDoesntExist(ctx, t, parentName) + + // verify the network delete did not delete the root link + n.LinkExists(ctx, t, master) +} + +func testMacvlanOverlapParentPassthruFirst(t *testing.T, ctx context.Context, client client.APIClient) { + // verify creating a second interface sharing a parent with another passthru interface is rejected + master := "dm-dummy0" + n.CreateMasterDummy(ctx, t, master) + defer n.DeleteInterface(ctx, t, master) + + netName := "dm-subinterface" + parentName := "dm-dummy0.40" + net.CreateNoError(ctx, t, client, netName, + net.WithMacvlanPassthru(parentName), + ) + assert.Check(t, n.IsNetworkAvailable(ctx, client, netName)) _, err := net.Create(ctx, client, "dm-parent-net-overlap", net.WithMacvlan(parentName), @@ -125,6 +174,96 @@ func testMacvlanOverlapParent(t *testing.T, ctx context.Context, client client.A n.LinkExists(ctx, t, master) } +func testMacvlanOverlapParentPassthruSecond(t *testing.T, ctx context.Context, client client.APIClient) { + // verify creating a passthru interface sharing a parent with another interface is rejected + master := "dm-dummy0" + n.CreateMasterDummy(ctx, t, master) + defer n.DeleteInterface(ctx, t, master) + + netName := "dm-subinterface" + parentName := "dm-dummy0.40" + net.CreateNoError(ctx, t, client, netName, + net.WithMacvlan(parentName), + ) + assert.Check(t, n.IsNetworkAvailable(ctx, client, netName)) + + _, err := net.Create(ctx, client, "dm-parent-net-overlap", + net.WithMacvlanPassthru(parentName), + ) + assert.Check(t, err != nil) + + // delete the network while preserving the parent link + err = client.NetworkRemove(ctx, netName) + assert.NilError(t, err) + + assert.Check(t, n.IsNetworkNotAvailable(ctx, client, netName)) + // verify the network delete did not delete the predefined link + n.LinkExists(ctx, t, master) +} + +func testMacvlanOverlapDeleteCreatedSecond(t *testing.T, ctx context.Context, client client.APIClient) { + // verify that a shared created parent interface is kept when the original interface is deleted first + master := "dm-dummy0" + n.CreateMasterDummy(ctx, t, master) + defer n.DeleteInterface(ctx, t, master) + + netName := "dm-subinterface" + parentName := "dm-dummy0.40" + net.CreateNoError(ctx, t, client, netName, + net.WithMacvlan(parentName), + ) + assert.Check(t, n.IsNetworkAvailable(ctx, client, netName)) + + overlapNetName := "dm-parent-net-overlap" + _, err := net.Create(ctx, client, overlapNetName, + net.WithMacvlan(parentName), + ) + assert.Check(t, err == nil) + + // delete the original network while preserving the parent link + err = client.NetworkRemove(ctx, netName) + assert.NilError(t, err) + assert.Check(t, n.IsNetworkNotAvailable(ctx, client, netName)) + n.LinkExists(ctx, t, parentName) + + // delete the second network + err = client.NetworkRemove(ctx, overlapNetName) + assert.NilError(t, err) + assert.Check(t, n.IsNetworkNotAvailable(ctx, client, overlapNetName)) + n.LinkDoesntExist(ctx, t, parentName) + + // verify the network delete did not delete the root link + n.LinkExists(ctx, t, master) +} + +func testMacvlanOverlapKeepExisting(t *testing.T, ctx context.Context, client client.APIClient) { + // verify that deleting interfaces sharing a previously existing parent doesn't delete the + // parent + master := "dm-dummy0" + n.CreateMasterDummy(ctx, t, master) + defer n.DeleteInterface(ctx, t, master) + + netName := "dm-subinterface" + net.CreateNoError(ctx, t, client, netName, + net.WithMacvlan(master), + ) + assert.Check(t, n.IsNetworkAvailable(ctx, client, netName)) + + overlapNetName := "dm-parent-net-overlap" + _, err := net.Create(ctx, client, overlapNetName, + net.WithMacvlan(master), + ) + assert.Check(t, err == nil) + + err = client.NetworkRemove(ctx, overlapNetName) + assert.NilError(t, err) + err = client.NetworkRemove(ctx, netName) + assert.NilError(t, err) + + // verify the network delete did not delete the root link + n.LinkExists(ctx, t, master) +} + func testMacvlanSubinterface(t *testing.T, ctx context.Context, client client.APIClient) { // verify the same parent interface cannot be used if already in use by an existing network master := "dm-dummy0" diff --git a/libnetwork/drivers/macvlan/macvlan_network.go b/libnetwork/drivers/macvlan/macvlan_network.go index 75313dd834..7413b3fb20 100644 --- a/libnetwork/drivers/macvlan/macvlan_network.go +++ b/libnetwork/drivers/macvlan/macvlan_network.go @@ -60,8 +60,20 @@ func (d *driver) createNetwork(config *configuration) (bool, error) { for _, nw := range networkList { if config.Parent == nw.config.Parent { if config.ID != nw.config.ID { - return false, fmt.Errorf("network %s is already using parent interface %s", - getDummyName(stringid.TruncateID(nw.config.ID)), config.Parent) + if config.MacvlanMode == modePassthru { + return false, fmt.Errorf( + "cannot use mode passthru, macvlan network %s is already using parent interface %s", + stringid.TruncateID(nw.config.ID), + config.Parent, + ) + } else if nw.config.MacvlanMode == modePassthru { + return false, fmt.Errorf( + "macvlan network %s is already using parent interface %s in mode passthru", + stringid.TruncateID(nw.config.ID), + config.Parent, + ) + } + continue } log.G(context.TODO()).Debugf("Create Network for the same ID %s\n", config.ID) foundExisting = true @@ -90,6 +102,15 @@ func (d *driver) createNetwork(config *configuration) (bool, error) { // if driver created the networks slave link, record it for future deletion config.CreatedSlaveLink = true } + } else { + // Check and mark this network if the interface was created for another network + networkList := d.getNetworks() + for _, testN := range networkList { + if config.Parent == testN.config.Parent && testN.config.CreatedSlaveLink { + config.CreatedSlaveLink = true + break + } + } } if !foundExisting { n := &network{ @@ -105,30 +126,38 @@ func (d *driver) createNetwork(config *configuration) (bool, error) { return foundExisting, nil } +func (d *driver) parentHasSingleUser(n *network) bool { + users := 0 + networkList := d.getNetworks() + for _, testN := range networkList { + if n.config.Parent == testN.config.Parent { + users += 1 + } + } + return users == 1 +} + // DeleteNetwork deletes the network for the specified driver type func (d *driver) DeleteNetwork(nid string) error { n := d.network(nid) if n == nil { return fmt.Errorf("network id %s not found", nid) } - // if the driver created the slave interface, delete it, otherwise leave it - if ok := n.config.CreatedSlaveLink; ok { - // if the interface exists, only delete if it matches iface.vlan or dummy.net_id naming - if ok := parentExists(n.config.Parent); ok { - // only delete the link if it is named the net_id - if n.config.Parent == getDummyName(stringid.TruncateID(nid)) { - err := delDummyLink(n.config.Parent) - if err != nil { - log.G(context.TODO()).Debugf("link %s was not deleted, continuing the delete network operation: %v", - n.config.Parent, err) - } - } else { - // only delete the link if it matches iface.vlan naming - err := delVlanLink(n.config.Parent) - if err != nil { - log.G(context.TODO()).Debugf("link %s was not deleted, continuing the delete network operation: %v", - n.config.Parent, err) - } + // if the driver created the slave interface and this network is the last user, delete it, otherwise leave it + if n.config.CreatedSlaveLink && parentExists(n.config.Parent) && d.parentHasSingleUser(n) { + // only delete the link if it is named the net_id + if n.config.Parent == getDummyName(stringid.TruncateID(nid)) { + err := delDummyLink(n.config.Parent) + if err != nil { + log.G(context.TODO()).Debugf("link %s was not deleted, continuing the delete network operation: %v", + n.config.Parent, err) + } + } else { + // only delete the link if it matches iface.vlan naming + err := delVlanLink(n.config.Parent) + if err != nil { + log.G(context.TODO()).Debugf("link %s was not deleted, continuing the delete network operation: %v", + n.config.Parent, err) } } }