Feature/bump netlink#main by s4ke · Pull Request #43718 · moby/moby

s4ke · 2022-06-12T22:39:04Z

Attempting to fix #43551

Will update according to contribution guidlines if the fix is fine.

- What I did

Bumped netlink dependencies to newest release (currently in beta)

- How to verify it

still in progress, but if everyone agrees that this is the correct course of action, I will verify this on currently affected servers.

- Description for the changelog

Bump netlink dependency to fix issues where encrypted overlay networks did not work with newer Linux kernel releases.

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

s4ke · 2022-06-12T22:48:51Z

Since I am new to working on this project, I would greatly appreciate some input on these:

Is moby/moby the correct repo for this fix or should I have done this in libnetwork?
Can I test the encryption behaviour with two docker daemons running in dind or do I have to spin up VMs for this?
Did I manipulate vendor.mod correctly?

s4ke · 2022-06-13T14:23:00Z

I feel like a unit test in https://github.com/moby/moby/tree/master/libnetwork/test/integration/dnet would be helpful to create a test to validate that encryption works properly, as I haven't found any other test (new to this project).

@thaJeztah what do you think?

s4ke · 2022-06-13T16:19:22Z

closing this as the issue seems to be more complex

thaJeztah · 2022-06-16T11:05:56Z

vendor.mod

 	github.com/prometheus/client_golang => github.com/prometheus/client_golang v1.6.0
 	github.com/prometheus/procfs => github.com/prometheus/procfs v0.0.11
-	github.com/vishvananda/netlink => github.com/vishvananda/netlink v1.1.0
+	github.com/vishvananda/netlink => github.com/vishvananda/netlink v1.2.1-beta.2


We can remove this replace rule if we decide to update to a new release; ISTR we added this to not update to a non-released version if there was no need. As there's a (beta) release now, perhaps we should consider updating to a newer version either way 🤔

s4ke · 2022-06-16T11:17:02Z

@thaJeztah Thanks for the review :).

Even though this probably might not be directly fixing #43551 I think this is still a useful upgrade, right? If yes, I can improve the PR according to your review? wdyt?

thaJeztah · 2022-06-16T11:54:52Z

Yes, I think it'd be ok to update the dependency to a more current version; if you have time to work on updating the PR; please do 👍 (please amend the existing commit so that there's only a single commit in the PR - but happy to help with that if needed!)

thaJeztah · 2022-06-16T15:20:13Z

Looks like there's some linting failures, because .Delete() was deprecated in favour of .Close() (the former now being just a wrapper for it, so easy to resolve by just s/Delete/Close/

libnetwork/drivers/bridge/setup_device_test.go:23:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_device_test.go:49:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_device_test.go:71:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_ipv4_test.go:37:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_ipv4_test.go:70:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_ipv6_test.go:24:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/bridge/setup_ipv6_test.go:74:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^
libnetwork/drivers/overlay/ov_network.go:439:10: SA1019: nlh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
			defer nlh.Delete()
			      ^
libnetwork/drivers/overlay/ov_utils.go:142:9: SA1019: nlh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
		defer nlh.Delete()
		      ^
libnetwork/osl/namespace_linux.go:237:3: SA1019: n.nlHandle.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
		n.nlHandle.Delete()
		^
libnetwork/osl/namespace_linux.go:290:3: SA1019: n.nlHandle.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
		n.nlHandle.Delete()
		^
libnetwork/osl/namespace_linux.go:472:3: SA1019: n.nlHandle.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
		n.nlHandle.Delete()
		^
libnetwork/osl/sandbox_linux_test.go:137:8: SA1019: nh.Delete is deprecated: use Close instead which is in line with typical resource release patterns for files and other resources. (staticcheck)
	defer nh.Delete()
	      ^

s4ke · 2022-06-16T17:37:24Z

locally the tests are failing for me, so I will have to check this out further:

=== Skipped
=== SKIP: libnetwork/iptables TestFirewalldInit (0.00s)
    firewalld_test.go:14: firewalld is not running

=== Failed
=== FAIL: libnetwork/drivers/bridge TestPortMappingV6Config (0.12s)
time="2022-06-16T17:32:48Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge/dummy/ is not added to the store"
time="2022-06-16T17:32:48Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge-endpoint/ep1/ is not added to the store"
time="2022-06-16T17:32:48Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge-endpoint/ep1/ is not added to the store"
    port_mapping_test.go:163: Failed to store the port bindings into the sandbox info. Found: [{udp 172.19.0.11 400 0.0.0.0 54000 54000} {tcp 172.19.0.11 500 0.0.0.0 65000 65000} {sctp 172.19.0.11 500 0.0.0.0 65000 65000}]

=== FAIL: libnetwork/ipam TestParallelPredefinedRequest2 (0.00s)
panic: send on closed channel [recovered]
        panic: send on closed channel

goroutine 27 [running]:
testing.tRunner.func1.2({0x64b7c0, 0x6dfb10})
        /usr/local/go/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1392 +0x39f
panic({0x64b7c0, 0x6dfb10})
        /usr/local/go/src/runtime/panic.go:838 +0x207
github.com/docker/docker/libnetwork/ipam.runParallelTests(0xc000503520, 0x1)
        /go/src/github.com/docker/docker/libnetwork/ipam/allocator_test.go:1503 +0x28b
github.com/docker/docker/libnetwork/ipam.TestParallelPredefinedRequest2(0x5ed747?)
        /go/src/github.com/docker/docker/libnetwork/ipam/allocator_test.go:1607 +0x1e
testing.tRunner(0xc000503520, 0x6a2390)
        /usr/local/go/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
        /usr/local/go/src/testing/testing.go:1486 +0x35f

=== FAIL: libnetwork/ipam TestParallelPredefinedRequest4 (unknown)

=== FAIL: libnetwork/ipam TestParallelPredefinedRequest1 (unknown)

=== FAIL: libnetwork/ipam TestParallelPredefinedRequest3 (unknown)

DONE 326 tests, 1 skipped, 5 failures in 202.256s
make: *** [Makefile:235: test-unit] Error 1

thaJeztah · 2022-06-16T17:53:34Z

Let's see what CI does; I know we inherited some, erm, "weird" tests from libnetwork, some of which are not-so-very-unit-tests that may require some setting up, so perhaps they're ok.

vendor.mod

thaJeztah · 2022-06-16T20:11:57Z

Hmm... funny; so for some reason, go modules now expects the github.com/Azure/go-ansiterm package to be a direct requires, no longer an indirect, making validate fail;

The result of go mod vendor differs

 M vendor.mod

Please vendor your package with hack/vendor.sh.

diff --git a/vendor.mod b/vendor.mod
index b72c3c3398..a5018087e0 100644
--- a/vendor.mod
+++ b/vendor.mod
@@ -86,9 +86,10 @@ require (
 	gotest.tools/v3 v3.2.0
 )
 
+require github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1
+
 require (
 	code.cloudfoundry.org/clock v1.0.0 // indirect
-	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
 	github.com/agext/levenshtein v1.2.3 // indirect
 	github.com/armon/circbuf v0.0.0-20190214190532-5111143e8da2 // indirect

Let me know if you need a hand fixing that

thaJeztah · 2022-06-16T20:12:45Z

(fwiw; don't put it as a separate require line, but move it into the first require section if the script doesn't automatically put it there)

bump netlink to 1.2.1 change usages of netlink handle .Delete() to Close() remove superfluous replace in vendor.mod make requires of github.com/Azure/go-ansiterm direct Signed-off-by: Martin Braun <[email protected]>

s4ke · 2022-06-16T20:27:17Z

I think I got it now. I greatly appreciate the handholding here :). Great first commiter experience I must say :)

s4ke · 2022-06-17T08:37:57Z

CI seems happy with the latest changes :)

EDIT: seems like I misinterpreted GitHub CI. There seem to be some issues it seems. But they look unrelated-ish?

s4ke · 2022-06-20T09:38:22Z

Hmm, seems like CI is failing for windows-2022? Tbh, I am having a hard time reading the test logs here on GitHub, but to me these errors look unrelated to the PR?

thaJeztah · 2022-06-21T07:30:41Z

I gave CI a kick to run again 👍 🤞

s4ke · 2022-06-22T19:08:53Z

CI is green 🤖 🥳

thaJeztah

whoop!

LGTM

thaJeztah · 2022-06-22T19:18:28Z

@tianon @corhere PTAL

thaJeztah · 2022-06-22T22:03:19Z

Thx! Let me bring this one in

s4ke force-pushed the feature/bump-netlink#main branch 2 times, most recently from 127545c to 39012bd Compare June 12, 2022 22:41

s4ke closed this Jun 13, 2022

thaJeztah reviewed Jun 16, 2022

View reviewed changes

s4ke reopened this Jun 16, 2022

thaJeztah added status/2-code-review area/networking Networking labels Jun 16, 2022

s4ke force-pushed the feature/bump-netlink#main branch from 39012bd to da41f92 Compare June 16, 2022 17:19

thaJeztah reviewed Jun 16, 2022

View reviewed changes

vendor.mod Outdated Show resolved Hide resolved

s4ke force-pushed the feature/bump-netlink#main branch from 67b2596 to 0e88a6b Compare June 16, 2022 20:15

bump netlink library

5edfd6d

bump netlink to 1.2.1 change usages of netlink handle .Delete() to Close() remove superfluous replace in vendor.mod make requires of github.com/Azure/go-ansiterm direct Signed-off-by: Martin Braun <[email protected]>

s4ke force-pushed the feature/bump-netlink#main branch from 0e88a6b to 5edfd6d Compare June 16, 2022 20:26

thaJeztah approved these changes Jun 22, 2022

View reviewed changes

corhere approved these changes Jun 22, 2022

View reviewed changes

thaJeztah merged commit a347f79 into moby:master Jun 22, 2022

thaJeztah added this to the 22.06.0 milestone Jun 22, 2022

romainreignier mentioned this pull request Oct 11, 2022

[fix included] dockerd[753]: http: panic serving @: runtime error: invalid memory address or nil pointer dereference #43034

Open

Conversation

s4ke commented Jun 12, 2022

Uh oh!

s4ke commented Jun 12, 2022

Uh oh!

s4ke commented Jun 13, 2022

Uh oh!

s4ke commented Jun 13, 2022

Uh oh!

thaJeztah Jun 16, 2022

Choose a reason for hiding this comment

Uh oh!

s4ke commented Jun 16, 2022

Uh oh!

thaJeztah commented Jun 16, 2022

Uh oh!

thaJeztah commented Jun 16, 2022

Uh oh!

s4ke commented Jun 16, 2022

Uh oh!

thaJeztah commented Jun 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

thaJeztah commented Jun 16, 2022

Uh oh!

thaJeztah commented Jun 16, 2022

Uh oh!

s4ke commented Jun 16, 2022

Uh oh!

s4ke commented Jun 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s4ke commented Jun 20, 2022

Uh oh!

thaJeztah commented Jun 21, 2022

Uh oh!

s4ke commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 22, 2022

Uh oh!

thaJeztah commented Jun 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Jun 16, 2022 •

edited

Loading

s4ke commented Jun 17, 2022 •

edited

Loading

s4ke commented Jun 22, 2022 •

edited

Loading