Move libnetwork by cpuguy83 · Pull Request #42262 · moby/moby

cpuguy83 · 2021-04-06T23:35:58Z

This moves libnetwork in-tree as discussed in moby/libnetwork#2522

add checks on node LB in support.sh

Fix TestValidRemoteDriver GetCapabilities errors

RFC434 states that DNS Servers should be case insensitive This commit makes sure that all DNS queries will be translated to lower ASCII characters and all svcRecords will be saved in lower case to abide by the RFC Relates to moby#21169 Signed-off-by: Arko Dasgupta <[email protected]>

Make DNS records and queries case-insensitive

controller: Check if IPTables is enabled for arrangeUserFilterRule

- Add Euan and Elango - Remove inactive maintainers Signed-off-by: Madhu Venugopal <[email protected]>

Reformat vendor.conf, update docker/docker and dependencies

Changes included: - Allow index specification at link creation time - replace syscall with golang.org/x/sys/unix - related: Use IFF_MULTI_QUEUE from x/sys/unix to define TUNTAP_MULTI_QUEUE - related: Use IFLA_* constants from x/sys/unix - Fix index out of range when no metadata for gretap - added encapsulation attributes for Iptun and Sittun to support SIT tunnels - Expose xfrm state's statistics - Support invert in ip rules - Support LWTUNNEL_ENCAP_SEG6 - Support setting and retrieving route MTU/AdvMSS - Fix CalcRtable array parameter bug - added support for Foo-over-UDP netlink calls - Support num{tx,rx}queues and udp6zerocsum{tx,rx} - tuntap: Add multiqueue support - Retrieve VLAN ID when listing neighbour - Fix LinkAdd for sit tunnel on 3.10 kernel - Add support for managing source MACVLANs - Two functions: one for adding bond slave, one for getting veth peer index - Eliminate cgo from netlink - Don't overwrite the XDP file descriptor with flags - Fix reference to BPF instructions (on Kernel 4.13) - Add Matchall filter - Send IFA_CACHEINFO when setting up addresses - Support IPv6 GRE Tun and Tap - Add List option to RouteSubscribeWithOptions, AddrSubscribeWithOptions, and LinkSubscribeWithOptions - Add Fq and Fq_Codel Qdisc support Signed-off-by: Sebastiaan van Stijn <[email protected]>

…80abd Signed-off-by: Sebastiaan van Stijn <[email protected]>

Bump vishvananda/netlink to 1.0.0

Maintainers update

moby/moby commit b27f70d wraps the ErrNotFound error returned when a plugin cannot be found, to include a backtrace. This changes the type of the error, so contoller.loadIPAMDriver no longer converts it to a libnetwork plugin.NotFoundError. This is a similar patch as was merged in 9b11497 Signed-off-by: Sebastiaan van Stijn <[email protected]>

This commit updates the vendored ishidawataru/sctp and adapts its used types. Signed-off-by: Sascha Grunert <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

up to date

Signed-off-by: Smark <[email protected]>

clarifications and typo fixes for the design documentation

Change wording for Endpoint description

[carry 2370] Update sctp package

…handling controller.loadIPAMDriver: Unwrap error type returned by PluginGetter

Remove roadmap link from README.md

Signed-off-by: Tonis Tiigi <[email protected]>

Since docker container can be connected to combination of several internal and external networks change of default gateway of the internal ones breaks communication via the external ones. This fixes only macvlan network type Signed-off-by: Pavel Matěja <[email protected]>

bridge: add riscv64 build tags

Signed-off-by: Euan Harris <[email protected]>

Update to Golang 1.12.6

Removed unused integration test code

Add support for Internal and Private network types on windows

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 · 2021-06-02T20:27:09Z

This is all green

cpuguy83 · 2021-06-02T20:27:30Z

Minus DCO because some of these commits we are pulling in do not have DCO sign-offs.

thaJeztah · 2021-06-02T20:06:16Z

Jenkinsfile

                                  -e VALIDATE_BRANCH=${CHANGE_TARGET} \
                                  docker:${GIT_COMMIT} \
-                                  hack/test/unit
+                                  hack/test/unit"


thaJeztah · 2021-06-02T20:06:57Z

Jenkinsfile

                                  docker:${GIT_COMMIT} \
-                                  hack/test/unit"
+                                  hack/test/unit
                                '''


Ah! Hm.. if you need to do another fix, perhaps you could fix the first commit

thaJeztah · 2021-06-02T20:07:54Z

libnetwork/drivers/windows/windows_test.go

 }

 func TestNAT(t *testing.T) {
+	t.Skip("Test does not work on CI and was never running to begin with")


Perhaps we should create tracking issues for these

thaJeztah · 2021-06-02T20:10:00Z

libnetwork/resolver_test.go


 func TestDNSProxyServFail(t *testing.T) {
+	if runtime.GOARCH == "arm64" {
+		t.Skip("This test fails on arm64 foor some reason... this need to be fixed")


Same for this one; we should create a tracking ticket

I resolved this later on... guess I missed the commit this was in.

thaJeztah · 2021-06-02T20:14:51Z

libcontainerd/remote/client.go

 	for _, m := range index.Manifests {
 		if m.MediaType == images.MediaTypeContainerd1Checkpoint {
-			cpDesc = &m
+			cpDesc = &m // nolint:gosec


I think this one may be legit, and should probably be:

Suggested change

for _, m := range index.Manifests {

if m.MediaType == images.MediaTypeContainerd1Checkpoint {

cpDesc = &m

cpDesc = &m // nolint:gosec

for _, m := range index.Manifests {

m := m

if m.MediaType == images.MediaTypeContainerd1Checkpoint {

cpDesc = &m

The break on the next line makes me think this is irrelevant. The warning is that m will have the same address through every iteration of the loop and taking the address could mean reading a different value than you intend; however, since we're immediately ending the loop and no new data will be written into m we have the correct value in cpDesc.

The m := m suggestion should silence the warning and be resilient to further changes in this function so it might be slightly better from a maintenance perspective, but I'm not really concerned with the code as-is.

thaJeztah · 2021-06-02T20:25:09Z

libnetwork/diagnostic/server.go

 	defer s.Unlock()

-	s.srv.Shutdown(context.Background())
+	s.srv.Shutdown(context.Background()) // nolint:errcheck


Note: nolint comments should not have a leading whitespace (comments for automation don't have a leading whitespace)

Probably slightly more cleaner to use _ to ignore the error than the //nolint comments

thaJeztah · 2021-06-02T20:26:26Z

libnetwork/drivers/bridge/bridge.go

 		iptables.OnReloaded(func() {
 			logrus.Debugf("Recreating iptables chains on firewall reload")
-			setupIPChains(config, iptables.IPv4)
+			if _, _, _, _, err := setupIPChains(config, iptables.IPv4); err != nil {


hehe, I recalled the lib network code when I enabled the dogsled linter (best name ever); https://github.com/alexkohler/dogsled

thaJeztah · 2021-06-02T20:28:05Z

libnetwork/endpoint.go

+	// TODO(cpuguy83): So yeah, this isn't checking any errors anywhere.
+	// Seems like we should be checking errors even because of memory related issues that can arise.
+	// Alas it seems like given the nature of this data we could introduce problems if we start checking these errors.
+	//
+	// If anyone ever comes here and figures out one way or another if we can/should be checking these errors and it turns out we can't... then please document *why*
+


Probably should do a tracking issue for this if important

thaJeztah · 2021-06-02T20:31:20Z

libnetwork/networkdb/cluster.go

 func (nDB *NetworkDB) triggerFunc(stagger time.Duration, C <-chan time.Time, f func()) {
 	// Use a random stagger to avoid synchronizing
-	randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger))
+	randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger)) // nolint:gosec


Do you recall what it was complaining about here (i suspect rnd.Int63() not being random?) If you do, would be great to leave it as a comment. You can use this format:

Suggested change

randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger)) // nolint:gosec

randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger)) //nolint:gosec // some comment here what we're ignoring

Ah was trying to add extra comments, didn't realize a 2nd // was needed.
This is do to rand use.

libnetwork/osl/interface_linux.go

tianon

Generally still +1 😄

tianon · 2021-06-02T20:36:41Z

hack/test/unit

+echo "${pkg_list}" | grep --fixed-strings "libnetwork/drivers/bridge" \
+	&& if ! type docker-proxy; then
+		hack/make.sh binary-proxy install-proxy
+	fi


Suggested change

echo "${pkg_list}" | grep --fixed-strings "libnetwork/drivers/bridge" \

&& if ! type docker-proxy; then

hack/make.sh binary-proxy install-proxy

fi

if grep <<<"${pkg_list}" --quiet --fixed-strings 'libnetwork/drivers/bridge' && ! command -v docker-proxy > /dev/null; then

hack/make.sh binary-proxy install-proxy

fi

tianon · 2021-06-02T20:39:37Z

libnetwork/CHANGELOG.md

tianon · 2021-06-02T20:40:27Z

libnetwork/MAINTAINERS

👀❓

Thoughts on whether it still makes sense to keep this file in this repository?

tianon · 2021-06-02T20:41:46Z

vendor.conf

@@ -33,7 +33,7 @@ github.com/imdario/mergo                            1afb36080aec31e0d1528973ebe6
 golang.org/x/sync                                   6e8e738ad208923de99951fe0b48239bfd864f28

 # buildkit
-github.com/moby/buildkit                            244e8cde639f71a05a1a2e0670bd88e0206ce55c # v0.8.3-3-g244e8cde
+github.com/moby/buildkit                            7e03277b32d4f0150bed0e081d4253b3a8557f13 https://github.com/cpuguy83/buildkit.git # v0.8.3-3-g244e8cde + libnetwork changes


tianon · 2021-06-02T20:42:00Z

vendor.conf

@@ -145,7 +146,7 @@ github.com/klauspost/compress                       a3b7545c88eea469c2246bee0e6c
 github.com/pelletier/go-toml                        65ca8064882c8c308e5c804c5d5443d409e0738c # v1.8.1

 # cluster
-github.com/docker/swarmkit                          5a5494a9a7b408b790533a5e4e1cb43ca1c32aad
+github.com/docker/swarmkit                          60d87cb7cdb070801ec550d7f4d7dc1210fb7e9f git://github.com/cpuguy83/swarmkit.git


Yeah, we'll need to update these projects once merged.

tianon · 2021-06-02T20:43:12Z

vendor.conf

@@ -62,6 +62,7 @@ github.com/docker/libkv                             458977154600b9f23984d9f4b82e
 github.com/vishvananda/netns                        db3c7e526aae966c4ccfa6c8189b693d6ac5d202
 github.com/vishvananda/netlink                      f049be6f391489d3f374498fe0c8df8449258372 # v1.1.0
 github.com/moby/ipvs                                4566ccea0e08d68e9614c3e7a64a23b850c4bb35 # v1.0.1
+github.com/urfave/cli                               a65b733b303f0055f8d324d805f393cd3e7a7904


😬 fun times

(How many different flag parsers are involved in moby/moby post-merge here? 😩)

Yeah, we should move this to cobra. I recall when we moved moby to use Cobra, we created a copy of the mflags package to libnetwork.

thaJeztah · 2021-06-02T21:19:31Z

DCO is failing because committer name doesn't match sign-off; wondering if adding an entry to .mailmap works for that?

Commit sha: 9ced389, Author: akim01, Committer: akim01; Expected "akim01 [email protected]", but got "Andrew Kim [email protected]".

(we can also mark it to "pass" manually; mostly curious if it takes .mailmap into account)

cpuguy83 · 2021-06-02T21:20:40Z

There's some missing sign-offs as well.

samuelkarp

I've gotten about a quarter of the way through this so far.

For anyone else looking to review: I found it helpful to check out this PR and the tip of libnetwork and use git diff to compare them. For example: git diff libnetwork/config/config.go docker/libnetwork/config/config.go will show me that just the imports have changed even though GitHub's diff view is confused.

samuelkarp · 2021-06-02T21:15:17Z

libnetwork/.circleci/config.yml

@@ -0,0 +1,79 @@
+version: 2


Do we have CircleCI set up here? Will CircleCI correctly read a file in libnetwork/.circleci rather than .circleci?

No this can be removed. All testing is being done by the jenkins runner.

libnetwork/client/client_service_test.go

libnetwork/api/api.go

libnetwork/cmd/ovrouter/ovrouter.go

AkihiroSuda

LGTM, let's merge this ASAP to avoid rebase hell

He asked me to dismiss this.

cpuguy83 · 2021-06-03T19:09:13Z

Merged per @thaJeztah and @tianon

Will follow-up to address PR comments.

AkihiroSuda · 2021-06-04T05:52:33Z

Can we cherry-pick this into 20.10, and archive the old libnetwork repo?

mavenugo and others added 30 commits June 4, 2019 09:22

Merge pull request moby#2372 from xinfengliu/improve-support.sh

fc69adf

add checks on node LB in support.sh

Merge pull request moby#2364 from thaJeztah/fix_TestValidRemoteDriver

a926e65

Fix TestValidRemoteDriver GetCapabilities errors

Merge pull request moby#2394 from arkodg/dns-lookup-case-insensitive

bdd0b7b

Make DNS records and queries case-insensitive

Merge pull request moby#2339 from phyber/iptables-check

8d76333

controller: Check if IPTables is enabled for arrangeUserFilterRule

Maintainers update

0fce3b3

- Add Euan and Elango - Remove inactive maintainers Signed-off-by: Madhu Venugopal <[email protected]>

Merge pull request moby#2361 from thaJeztah/bump_engine

97d292b

Reformat vendor.conf, update docker/docker and dependencies

bump github.com/vishvananda/netns 13995c7128ccc8e51e9a6bd2b551020a271…

e9e18d9

…80abd Signed-off-by: Sebastiaan van Stijn <[email protected]>

Merge pull request moby#2366 from thaJeztah/bump_netlink_1.0

0286b01

Bump vishvananda/netlink to 1.0.0

Merge pull request moby#2396 from mavenugo/um

d9ac962

Maintainers update

Update sctp package

c5c8653

This commit updates the vendored ishidawataru/sctp and adapts its used types. Signed-off-by: Sascha Grunert <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

Merge pull request #1 from docker/master

3ba3118

up to date

road map loss tracing, just keep use 'Design' describe the 'Future'

1f1d989

Signed-off-by: Smark <[email protected]>

Merge pull request moby#2279 from mrbeskin/clarify-design-doc

c0c586a

clarifications and typo fixes for the design documentation

Merge pull request moby#1723 from sebradloff/patch-1

d297a25

Change wording for Endpoint description

Merge pull request moby#2403 from thaJeztah/carry_2370_sctp_update

c3d402c

[carry 2370] Update sctp package

Merge pull request moby#2365 from thaJeztah/fix_loadIPAMDriver_error_…

1e476e7

…handling controller.loadIPAMDriver: Unwrap error type returned by PluginGetter

Merge pull request moby#2404 from smarkm/master

5c8048b

Remove roadmap link from README.md

bridge: add riscv64 build tags

75bc82f

Signed-off-by: Tonis Tiigi <[email protected]>

Merge pull request moby#2389 from tonistiigi/bridge-riscv

3780098

bridge: add riscv64 build tags

vet: Fix composite literal uses unkeyed fields warnings

0275b00

Signed-off-by: Euan Harris <[email protected]>

vet: Fix format string warnings

746e680

Signed-off-by: Euan Harris <[email protected]>

gofmt: Fix formatting warnings

8441458

Signed-off-by: Euan Harris <[email protected]>

Dockerfile: Update to Go 1.12.6, matching moby/moby

3eeda3d

Signed-off-by: Euan Harris <[email protected]>

Merge pull request moby#2408 from euanh/golang-1.12.6

92611bb

Update to Golang 1.12.6

Merge pull request moby#1656 from huikang/remove-unused-testcode

587783f

Removed unused integration test code

Merge pull request moby#2231 from msabansal/winnetwork

46bed87

Add support for Internal and Private network types on windows

Fix flakey test TestNetworkDBIslands

0dd8bc6

Signed-off-by: Brian Goff <[email protected]>

cpuguy83 force-pushed the move_libnetwork branch from 76454db to 0dd8bc6 Compare June 2, 2021 16:53

cpuguy83 requested review from thaJeztah and tianon June 2, 2021 20:27

thaJeztah previously requested changes Jun 2, 2021

View reviewed changes

tianon reviewed Jun 2, 2021

View reviewed changes

samuelkarp reviewed Jun 2, 2021

View reviewed changes

AkihiroSuda approved these changes Jun 3, 2021

View reviewed changes

AkihiroSuda added this to the 21.xx milestone Jun 3, 2021

AkihiroSuda added the area/networking Networking label Jun 3, 2021

AkihiroSuda requested a review from thaJeztah June 3, 2021 05:47

tianon approved these changes Jun 3, 2021

View reviewed changes

cpuguy83 merged commit a773178 into moby:master Jun 3, 2021

cpuguy83 deleted the move_libnetwork branch June 3, 2021 19:06

thaJeztah mentioned this pull request Jun 3, 2021

[testing] ~update~ fix linting issues found by golangci-lint v1.40.1 #42445

Merged

AkihiroSuda mentioned this pull request Jun 4, 2021

Update docs to clarify that the repo has moved to moby/moby/libnetwork moby/libnetwork#2641

Closed

This was referenced Jun 5, 2021

Post-libnetwork integration tasks #42471

Open

Update docker/docker to master / v21.xx-dev (post libnetwork integration) moby/buildkit#2150

Merged

thaJeztah mentioned this pull request Jun 29, 2021

[master] fix build-script for docker-proxy (after libnetwork integration) and update to Go 1.16.5 docker/docker-ce-packaging#554

Merged

This was referenced Oct 13, 2021

panic: when resolve dns (github.com/docker/docker/libnetwork.(*resolver).ServeDNS) #42929

Closed

libnetwork: ServeDNS(): don't panic on unsupported query types #42931

Merged

This was referenced Apr 4, 2024

Improve the documentation examples and advice for native IPv6 Docker networking docker/docs#19556

Open

Use Go 1.21 and update modules KatharaFramework/NetworkPlugin#13

Merged

	randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger)) // nolint:gosec
	randStagger := time.Duration(uint64(rnd.Int63()) % uint64(stagger)) //nolint:gosec // some comment here what we're ignoring

Conversation

cpuguy83 commented Apr 6, 2021

Uh oh!

cpuguy83 commented Jun 2, 2021

Uh oh!

cpuguy83 commented Jun 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 2, 2021

Uh oh!

cpuguy83 commented Jun 2, 2021

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AkihiroSuda left a comment

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented Jun 3, 2021

Uh oh!

AkihiroSuda commented Jun 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants