vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 #40596

thaJeztah · 2020-02-28T00:17:12Z

full diff: moby/libnetwork@264bffc...bf2bd42

relevant changes:

Macvlan internal network should not change default gateway libnetwork#2407 Macvlan internal network should not change default gateway
- fixes Internal macvlan network overrides default gateway libnetwork#2406 Internal macvlan network overrides default gateway
vendor godbus/dbus v5
Fix InhibitIPv4 nil panic
Cleanup VFP during overlay network removal
- fixes VFP leak in windows overlay network deletion

this should be the last one listed in #39392 (comment) to be vendored, so

fixes #39392 on master

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2020-02-28T01:06:51Z

Looks like CI is broken @StefanScherer

cpuguy83 · 2020-02-28T17:38:17Z

Lots of macvlan/ipvlan failures.

thaJeztah · 2020-02-28T18:00:32Z

Yes, noticed them; looks like it's related to that change. Not familiar enough with them to tell if the tests are incorrect, or if it's breaking things.

@arkodg @lemrouch PTAL

arkodg · 2020-02-28T20:02:12Z

@lemrouch couple of things I've realized after looking at

moby/integration/network/macvlan/macvlan_test.go

Line 153 in 40b2b4b

func testMacvlanInternalMode(client client.APIClient) func(*testing.T) {

and

moby/integration/network/macvlan/macvlan_test.go

Line 135 in 40b2b4b

func testMacvlanNilParent(client client.APIClient) func(*testing.T) {

We might still want to keep the dummyParent in case config.ConfigFrom is empty which will be needed so moby/libnetwork@43a5e91 will be needed to reworked to take care of that case

arkodg · 2020-02-28T20:07:34Z

@lemrouch Looking at

moby/integration/network/macvlan/macvlan_test.go

Line 179 in 40b2b4b

func testMacvlanMultiSubnet(client client.APIClient) func(*testing.T) {

and

moby/integration/network/macvlan/macvlan_test.go

Line 247 in 40b2b4b

func testMacvlanAddressing(client client.APIClient) func(*testing.T) {

moby/libnetwork@8d1f97e
will be needed to be reworked to make sure we skip creating a gateway for an internal macvlan/ipvlan network only if an existing non-internal network exists

lemrouch · 2020-03-02T10:49:35Z

moby/integration/network/macvlan/macvlan_test.go

Line 153 in 40b2b4b

func testMacvlanInternalMode(client client.APIClient) func(*testing.T) {

and

moby/integration/network/macvlan/macvlan_test.go

Line 135 in 40b2b4b

func testMacvlanNilParent(client client.APIClient) func(*testing.T) {

Those tests are broken by design. They are basically saying: make sure isolated network is not isolated at all and can reach public ip address.

lemrouch · 2020-03-02T12:04:39Z

@lemrouch Looking at

moby/integration/network/macvlan/macvlan_test.go

Line 179 in 40b2b4b

func testMacvlanMultiSubnet(client client.APIClient) func(*testing.T) {

and

moby/integration/network/macvlan/macvlan_test.go

Line 247 in 40b2b4b

func testMacvlanAddressing(client client.APIClient) func(*testing.T) {

docker/libnetwork@8d1f97e
will be needed to be reworked to make sure we skip creating a gateway for an internal macvlan/ipvlan network only if an existing non-internal network exists

Again, internal networks should not mess with default gateways as this will break routing via regular network. The test is wrong.

arkodg · 2020-03-02T17:16:04Z

@lemrouch Looking at

moby/integration/network/macvlan/macvlan_test.go

Line 179 in 40b2b4b

func testMacvlanMultiSubnet(client client.APIClient) func(*testing.T) {

and

moby/integration/network/macvlan/macvlan_test.go

Line 247 in 40b2b4b

func testMacvlanAddressing(client client.APIClient) func(*testing.T) {

docker/libnetwork@8d1f97e
will be needed to be reworked to make sure we skip creating a gateway for an internal macvlan/ipvlan network only if an existing non-internal network exists

Again, internal networks should not mess with default gateways as this will break routing via regular network. The test is wrong.

but @lemrouch these are not internal networks, just a macvlan network with an empty parent

arkodg · 2020-03-02T17:39:42Z

also this err should not be nil, the test code looks fine to me

	_, err := container.Exec(timeoutCtx, client, id1, []string{"ping", "-c", "1", "-w", "1", "8.8.8.8"})
		// FIXME(vdemeester) check the time of error ?
		assert.Check(t, err != nil)

but the err is nil

macvlan_test.go:171: assertion failed: err is nil

arkodg · 2020-03-03T01:33:44Z

+++ b/vendor/github.com/docker/libnetwork/drivers/macvlan/macvlan_network.go
@@ -61,8 +61,6 @@ func (d *driver) CreateNetwork(nid string, option map[string]interface{}, nInfo
        // if parent interface not specified, create a dummy type link to use named dummy+net_id
        if config.Parent == "" {
                config.Parent = getDummyName(stringid.TruncateID(config.ID))
-               // empty parent and --internal are handled the same. Set here to update k/v
-               config.Internal = true
        }
        foundExisting, err := d.createNetwork(config)
        if err != nil {
@@ -100,18 +98,16 @@ func (d *driver) createNetwork(config *configuration) (bool, error) {
                }
        }
        if !parentExists(config.Parent) {
-               // if the --internal flag is set, create a dummy link
-               if config.Internal {
+               // Create a dummy link
+               if config.Parent == getDummyName(stringid.TruncateID(config.ID)) {
                        err := createDummyLink(config.Parent, getDummyName(stringid.TruncateID(config.ID)))
                        if err != nil {
                                return false, err
                        }
                        config.CreatedSlaveLink = true
                        // notify the user in logs they have limited communications
-                       if config.Parent == getDummyName(stringid.TruncateID(config.ID)) {
-                               logrus.Debugf("Empty -o parent= and --internal flags limit communications to other containers inside of network: %s",
-                                       config.Parent)
-                       }
+                       logrus.Debugf("Empty -o parent= limit communications to other containers inside of network: %s",
+                               config.Parent)

fixes
--- PASS: TestDockerNetworkMacvlan/MultiSubnet (24.72s)
--- PASS: TestDockerNetworkMacvlan/Addressing (6.99s)

The above code tries to further separate the difference in meaning between empty Parent and internal network which can or cannot be the same

unclear why the below test is failing

=== FAIL: amd64.integration.network.macvlan TestDockerNetworkMacvlan/InternalMode (13.45s)
    --- FAIL: TestDockerNetworkMacvlan/InternalMode (13.45s)
        macvlan_test.go:171: assertion failed: err is nil
        macvlan_test.go:172: assertion failed: expression is false: timeoutCtx.Err() == context.DeadlineExceeded

lemrouch · 2020-03-03T09:34:19Z

unclear why the below test is failing

=== FAIL: amd64.integration.network.macvlan TestDockerNetworkMacvlan/InternalMode (13.45s)
    --- FAIL: TestDockerNetworkMacvlan/InternalMode (13.45s)
        macvlan_test.go:171: assertion failed: err is nil
        macvlan_test.go:172: assertion failed: expression is false: timeoutCtx.Err() == context.DeadlineExceeded

It's test for internal network. That means there is no default gateway anymore.
My guess is:

the ping used to try to pass ICMP packet to nonexistent gw (internal MACLAN network imagined one, remeber?) and timed out
now it has no default route so the ping gets ping: connect: Network is unreachable immediately

arkodg · 2020-03-03T18:09:02Z

@lemrouch you're right, it still does give a nil error so the code can be changed to

-               timeoutCtx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
-               defer cancel()
-               _, err := container.Exec(timeoutCtx, client, id1, []string{"ping", "-c", "1", "-w", "1", "8.8.8.8"})
-               // FIXME(vdemeester) check the time of error ?
-               assert.Check(t, err != nil)
-               assert.Check(t, timeoutCtx.Err() == context.DeadlineExceeded)
+               result, _ := container.Exec(ctx, client, id1, []string{"ping", "-c", "1", "8.8.8.8"})
+               assert.Check(t, strings.Contains(result.Combined(), "Network is unreachable"))

which fixes the issue, will raise a PR for these soon

moby#2419 and moby#2407 attempted to seperate out empty parent and internal for macvlan and ipvlan networks However it didnt pass the integration tests in moby moby/moby#40596 and exposed some more plumbing that needed to be done to make sure we seperate the two things If the -o parent is empty we create a dummylink and if internal is set we dont add a default gateway and make sure north-south communication cannot take place (only east-west / container-container can) Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>

moby#2419 and moby#2407 attempted to seperate out empty parent and internal for macvlan and ipvlan networks However it didnt pass the integration tests in moby moby/moby#40596 and exposed some more plumbing that needed to be done to make sure we separate the two things If the -o parent is empty we create a dummylink and if internal is set we dont add a default gateway and make sure north-south communication cannot take place (only east-west / container-container can) Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>

thaJeztah · 2020-03-05T23:06:59Z

Updated to current master, to include moby/libnetwork#2523, which should fix the CI failures

AkihiroSuda · 2020-03-06T01:12:43Z

still failing

thaJeztah · 2020-03-06T07:25:30Z

😢 @arkodg PTAL 🤗

tao12345666333

LGTM

I encountered a similar problem and fixed by this PR moby/libnetwork#2519
(included in this vendor)

arkodg · 2020-03-06T15:28:30Z

@thaJeztah TestDockerNetworkMacvlan/InternalMode and TestDockerNetworkIpvlan/L3InternalMode need to be rewritten to incorporate #40596 (comment)

thaJeztah · 2020-03-06T15:36:34Z

@arkodg ah, sorry, overlooked that one, thought it was being handled by the changes in libnetwork 👍

arkodg

LGTM , thanks for incorporating the test changes

arkodg · 2020-03-06T19:53:10Z

@thaJeztah similar changes needed in TestDockerNetworkIpvlan/L2InternalMode as well, my bad

full diff: moby/libnetwork@264bffc...bf2bd42 relevant changes: - moby/libnetwork#2407 Macvlan internal network should not change default gateway - fixes moby/libnetwork#2406 Internal macvlan network overrides default gateway - vendor godbus/dbus v5 - Fix InhibitIPv4 nil panic - Cleanup VFP during overlay network removal - fixes VFP leak in windows overlay network deletion Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2020-03-06T23:36:39Z

d'oh! I saw that test and wanted to check it, but was in a hurry and forgot. Thanks!

trapier · 2020-03-09T15:24:33Z

@thaJeztah: would like to see this in CE 19.03.8. Looks like the pattern for 19.03 is to backport from master. Would like to confirm intent to backport this for 19.03.8, as well as offer any assistance from Mirantis to help make it so.

thaJeztah · 2020-03-09T16:11:54Z

Looks like the pattern for 19.03 is to backport from master. Would like to confirm intent to backport this for 19.03.8, as well as offer any assistance from Mirantis to help make it so.

For libnetwork, the 19.03 branch vendors from the corresponding bump_v19.03 branch in libnetwork (and 18.09 from the bump_v18.09 libnetwork branch).

I can open a bump PR for the 19.03 branch, or perhaps @SamWhited can update #40617 to get the latest changes from libnetwork

thaJeztah · 2020-03-09T16:12:48Z

@cpuguy83 @AkihiroSuda @arkodg this is green now, PTAL 👍

SamWhited · 2020-03-09T16:13:32Z

I can open a bump PR for the 19.03 branch, or perhaps @SamWhited can update #40617 to get the latest changes from libnetwork

Wilco.

arkodg · 2020-03-09T16:17:58Z

@thaJeztah lets merge this PR
@SamWhited the failures in your PR for #40617 are unrelated to the failures encountered in this PR (and changes made here)

SamWhited · 2020-03-09T16:20:03Z

Updated. Will take a look at any failures when CI finishes running.

moby#2419 and moby#2407 attempted to seperate out empty parent and internal for macvlan and ipvlan networks However it didnt pass the integration tests in moby moby/moby#40596 and exposed some more plumbing that needed to be done to make sure we separate the two things If the -o parent is empty we create a dummylink and if internal is set we dont add a default gateway and make sure north-south communication cannot take place (only east-west / container-container can) Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>

moby/libnetwork#2419 and moby/libnetwork#2407 attempted to seperate out empty parent and internal for macvlan and ipvlan networks However it didnt pass the integration tests in moby moby#40596 and exposed some more plumbing that needed to be done to make sure we separate the two things If the -o parent is empty we create a dummylink and if internal is set we dont add a default gateway and make sure north-south communication cannot take place (only east-west / container-container can) Signed-off-by: Arko Dasgupta <arko.dasgupta@docker.com>

thaJeztah added area/networking status/2-code-review labels Feb 28, 2020

thaJeztah added this to the 20.03.0 milestone Feb 28, 2020

thaJeztah mentioned this pull request Feb 28, 2020

Containers can't communicate over macvlan internal network in swarm #39392

Closed

AkihiroSuda approved these changes Feb 28, 2020

View reviewed changes

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 28, 2020

arkodg mentioned this pull request Mar 3, 2020

Macvlan: Separate empty parent and internal moby/libnetwork#2523

Merged

thaJeztah force-pushed the bump_libnetwork branch from 84fcdd9 to 42ef405 Compare March 5, 2020 23:04

thaJeztah requested a review from tianon as a code owner March 5, 2020 23:04

thaJeztah changed the title ~~vendor: bump libnetwork d8334ccdb9be7f471e0ac15ff30f9d74da758ea3~~ vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 Mar 5, 2020

thaJeztah added impact/changelog and removed status/failing-ci Indicates that the PR in its current state fails the test suite labels Mar 5, 2020

AkihiroSuda approved these changes Mar 6, 2020

View reviewed changes

tao12345666333 approved these changes Mar 6, 2020

View reviewed changes

thaJeztah force-pushed the bump_libnetwork branch from 42ef405 to 44c4f6f Compare March 6, 2020 15:44

arkodg approved these changes Mar 6, 2020

View reviewed changes

thaJeztah force-pushed the bump_libnetwork branch from 44c4f6f to e1710b4 Compare March 6, 2020 23:36

thaJeztah merged commit d5595a5 into moby:master Mar 9, 2020

thaJeztah deleted the bump_libnetwork branch March 9, 2020 16:23

SamWhited mentioned this pull request Mar 11, 2020

[19.03] Macvlan: Separate empty parent and internal moby/libnetwork#2534

Closed

trapier mentioned this pull request Mar 13, 2020

[19.03 backport] Update DNS library #40617

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 #40596

vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 #40596

thaJeztah commented Feb 28, 2020 •

edited

Loading

thaJeztah commented Feb 28, 2020

cpuguy83 commented Feb 28, 2020

thaJeztah commented Feb 28, 2020

arkodg commented Feb 28, 2020

arkodg commented Feb 28, 2020

lemrouch commented Mar 2, 2020 •

edited

Loading

lemrouch commented Mar 2, 2020

arkodg commented Mar 2, 2020

arkodg commented Mar 2, 2020

arkodg commented Mar 3, 2020 •

edited

Loading

lemrouch commented Mar 3, 2020

arkodg commented Mar 3, 2020 •

edited by thaJeztah

Loading

thaJeztah commented Mar 5, 2020

AkihiroSuda commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

tao12345666333 left a comment

arkodg commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

arkodg left a comment

arkodg commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

trapier commented Mar 9, 2020

thaJeztah commented Mar 9, 2020

thaJeztah commented Mar 9, 2020

SamWhited commented Mar 9, 2020

arkodg commented Mar 9, 2020

SamWhited commented Mar 9, 2020

vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 #40596

vendor: bump libnetwork bf2bd42abc0a3734f12b5ec724e571434e42c669 #40596

Conversation

thaJeztah commented Feb 28, 2020 • edited Loading

thaJeztah commented Feb 28, 2020

cpuguy83 commented Feb 28, 2020

thaJeztah commented Feb 28, 2020

arkodg commented Feb 28, 2020

arkodg commented Feb 28, 2020

lemrouch commented Mar 2, 2020 • edited Loading

lemrouch commented Mar 2, 2020

arkodg commented Mar 2, 2020

arkodg commented Mar 2, 2020

arkodg commented Mar 3, 2020 • edited Loading

lemrouch commented Mar 3, 2020

arkodg commented Mar 3, 2020 • edited by thaJeztah Loading

thaJeztah commented Mar 5, 2020

AkihiroSuda commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

tao12345666333 left a comment

Choose a reason for hiding this comment

arkodg commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

arkodg left a comment

Choose a reason for hiding this comment

arkodg commented Mar 6, 2020

thaJeztah commented Mar 6, 2020

trapier commented Mar 9, 2020

thaJeztah commented Mar 9, 2020

thaJeztah commented Mar 9, 2020

SamWhited commented Mar 9, 2020

arkodg commented Mar 9, 2020

SamWhited commented Mar 9, 2020

thaJeztah commented Feb 28, 2020 •

edited

Loading

lemrouch commented Mar 2, 2020 •

edited

Loading

arkodg commented Mar 3, 2020 •

edited

Loading

arkodg commented Mar 3, 2020 •

edited by thaJeztah

Loading