Implement support for RUN --network=none|default|host

[bossmc]

Following a similar pattern to #1081, this adds support in the experimental Dockerfile for RUN instructions with customised networking modes:

RUN --network=none ip link list

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00

Allowed arguments for --network are:

• none - No networking is available during the invocation (useful for preventing external effects)
• host - Expose the host's network stack to the invocation (requires the network.host entitlement)
• default - The invocation is run in the standard internal network (equivalent to not specifying the network)

[GordonTheTurtle]

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "run-network-mode" git@github.com:Metaswitch/buildkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

[tonistiigi]

@AkihiroSuda @tiborvass @justincormack @thaJeztah

Not quite sure about the --network=default name. Definitely, don't want to call it bridge. Maybe we should call it sandbox like the security mode? Although actually this is just a default that is chosen on daemon startup.

Needs docs update as well. These can wait until the syntax is confirmed.

[bossmc]

Thanks for the comments @tonistiigi, yeah I didn't write docs for that exact reason, obviously I'll add some before the final merge.

[thaJeztah]

Yeah, the default name isn't great (I think it's used on Windows, and I recall we had some todos to get rid of that)

The only concern I'd have with this feature is that it's diverging from how we treated Dockerfiles in the past, where certain options had to be past from the commandline (with the reasoning that a Dockerfile by itself shouldn't be able to gain more privileges than the default). Is this still gated by some command line option?

(disclaimer: haven't caught up with the latest changes in this repository)

[tonistiigi]

Is this still gated by some command line option?

@thaJeztah Yes, you can't use RUN --network=host without allowing it from the command line.

@bossmc Regarding command line flags, we should also verify that docker build --network=host|none continues to work as expected, eg. defaulting back to that value if no flag is set in the Dockerfile(or "default" is set).

[bossmc]

Ok @tonistiigi, I've applied your review comments (using RunOptions, adding tests that the flag only applies to one command, proper test for hostNetworking support)

I've not yet added a test for checking that the command line option is used if the flag is not set, could you give me some pointers for what such a test would look like?

[bossmc]

Possible alternative to default - unspecified? Might make it more obvious that a higher level config setting will decide the behaviour (e.g. docker build --net host).

[tiborvass]

@tonistiigi I don't have a problem with default, it is very precisely that. It's not sandbox in the build --net host case.

[tonistiigi]

I've not yet added a test for checking that the command line option is used if the flag is not set, could you give me some pointers for what such a test would look like?

@bossmc Set "force-network-mode" frontend opt to set the value that cli --network flag would configure https://github.com/moby/buildkit/blob/master/frontend/dockerfile/builder/build.go#L47

[bossmc]

Hmmm, adding that last test has exposed that I think we might need both "sandbox" and "default/unspecified" - this PR currently implements:

• docker build:
  • RUN --network=none => No network
  • RUN --network=host => Host network
  • RUN --network=default => Sandbox network
  • RUN => Sandbox network
• docker build --network=host:
  • RUN --network=none => No network
  • RUN --network=host => Host network
  • RUN --network=default => Host network
  • RUN => Host network

There's no way to overload docker build --network host with RUN --network to get the invocation to happen in a sandboxed network, maybe that's deliberate? If we added sandbox we'd have:

• docker build:
  • RUN --network=none => No network
  • RUN --network=host => Host network
  • RUN --network=sandbox => Sandbox network
  • RUN --network=default => Sandbox network
  • RUN => Sandbox network
• docker build --network=host:
  • RUN --network=none => No network
  • RUN --network=host => Host network
  • RUN --network=sandbox => Sandbox network
  • RUN --network=default => Host network
  • RUN => Host network

But maybe it's weird to allow the Dockerfile to mandate a sandbox network when the client asked to use the host? Which field takes priority here?

[thaJeztah]

If we had sandbox, would we still need default? (As it's the same as "no flag set")

[tonistiigi]

The "sandbox" isn't really guaranteed by buildkit(that's because it does not exist in LLB). It is just a default mode configured by daemon(eg. in buildkitd it is actually host unless cni is configured).

I think we should keep "default" (or "unset") and it is the same as passed in docker build --network. So on its own, it is quite useless and there only for completeness (same as --security=sandbox for example).

[bossmc]

@thaJeztah due to the way Flags works, the default value of the flag has to be allowed to be provided explicitly (since there's no way after the parse to know if the flag was provided or defaulted).

@tonistiigi I found that if I set the network to UNSET in the LLB, then that prevented docker build --network=host from taking effect for that stage, what behaviour would you expect in that case? I've changed to just not set the per-stage network mode if Dockerfile asks for default (equivalently, doesn't specify) which definitely allows the host-networking to be inherited.

It feels like we're converging on default to mean "whatever would happen if you didn't pass a flag" (and none/host for the two explicit options). Is everyone happy with that?

[tonistiigi]

I found that if I set the network to UNSET in the LLB, then that prevented docker build --network=host from taking effect for that stage, what behaviour would you expect in that case?

I would expect docker build --network=host + RUN --network=default to use host networking for that process. LLB is correct though as there is no separate way to set it and the last one should just take precedence.

It feels like we're converging on default to mean "whatever would happen if you didn't pass a flag" (and none/host for the two explicit options). Is everyone happy with that?

I think that is the best option.

[bossmc]

Ok, sounds like the implementation is doing the right thing (:tm:). I'll write some docs and then I guess this needs one final review?

[bossmc]

Docs updated, over to you 😄

[bossmc]

Oops, just noticed a bug (in the norunnetwork/norunsecurity code), I'll fix tomorrow.

[bossmc]

I do not understand the UT failure, I can reproduce it sporadically locally, but I've checked and the LLB definitely doesn't enable host networking for the second RUN command, so I'd expect the command to always fail (and thus for the RUN to pass, thanks to the !).

[tonistiigi]

@bossmc What test was failing?

[bossmc]

   --- FAIL: TestIntegration/RunHostNetwork/worker=oci-rootless/frontend=gateway/network.host=granted (3.11s)
        require.go:794: 
            	Error Trace:	dockerfile_runnetwork_test.go:123
            	            				run.go:172
            	Error:      	Received unexpected error:
            	            	rpc error: code = Unknown desc = failed to solve with frontend gateway.v0: rpc error: code = Unknown desc = failed to build LLB: executor failed running [/bin/sh -c ! nc 127.0.0.1 34289 | grep foo]: exit code: 1
            	            	failed to solve
            	            	github.com/moby/buildkit/client.(*Client).solve.func2
            	            		/src/client/solve.go:203
            	            	golang.org/x/sync/errgroup.(*Group).Go.func1
            	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:58
            	            	runtime.goexit
            	            		/usr/local/go/src/runtime/asm_amd64.s:1337
            	Test:       	TestIntegration/RunHostNetwork/worker=oci-rootless/frontend=gateway/network.host=granted

[bossmc]

That test is covering:

FROM busybox
RUN --network=host nc 127.0.0.1 %s | grep foo
RUN ! nc 127.0.0.1 %s | grep foo

With host-networking allowed.

[tonistiigi]

The issue with the test is

2019/08/26 18:17:22 buildkitd: CNI setup error: failed to create bridge "buildkit0": could not add "buildkit0": operation not permitted

not sure atm what is causing this but it shows up with this patch that makes sure cni is enabled when tests run. Currently, in auto mode it would switch to host mode and cause this test failure. @crosbymichael Any ideas? it should be already protected for concurrent usage by file lock.

diff --git a/util/testutil/integration/containerd.go b/util/testutil/integration/containerd.go
index c1157dd1..eae04245 100644
--- a/util/testutil/integration/containerd.go
+++ b/util/testutil/integration/containerd.go
@@ -130,6 +130,10 @@ disabled_plugins = ["cri"]
 		"--containerd-worker-labels=org.mobyproject.buildkit.worker.sandbox=true", // Include use of --containerd-worker-labels to trigger https://github.com/moby/buildkit/pull/603
 	}
 
+	if os.Getenv("BUILDKIT_RUN_NETWORK_INTEGRATION_TESTS") == "1" {
+		buildkitdArgs = append(buildkitdArgs, "--containerd-worker-net=cni")
+	}
+
 	var upt []ConfigUpdater
 
 	for _, v := range conf.mv.values {
diff --git a/util/testutil/integration/oci.go b/util/testutil/integration/oci.go
index 23d42fe0..ce92dc4c 100644
--- a/util/testutil/integration/oci.go
+++ b/util/testutil/integration/oci.go
@@ -63,6 +63,10 @@ func (s *oci) New(opt ...SandboxOpt) (Sandbox, func() error, error) {
 	// Include use of --oci-worker-labels to trigger https://github.com/moby/buildkit/pull/603
 	buildkitdArgs := []string{"buildkitd", "--oci-worker=true", "--containerd-worker=false", "--oci-worker-gc=false", "--oci-worker-labels=org.mobyproject.buildkit.worker.sandbox=true"}
 
+	if os.Getenv("BUILDKIT_RUN_NETWORK_INTEGRATION_TESTS") == "1" {
+		buildkitdArgs = append(buildkitdArgs, "--oci-worker-net=cni")
+	}
+
 	deferF := &multiCloser{}
 
 	var upt []ConfigUpdater

edit: actually, a better patch would be to always fail daemon on auto mode if there is a cni configuration but cni fails to start.

[tonistiigi]

@AkihiroSuda I believe the issue with the cni initialization is

buildkit/util/testutil/integration/oci.go

Line 98 in 6885427

buildkitdArgs = append([]string{"sudo", "-u", fmt.Sprintf("#%d", s.uid), "-i", "--", "rootlesskit"}, buildkitdArgs...)

where it loses the environment variables when process is loaded through sudo with these flags. What's the best solution to make sure daemon config is preserved?

[AkihiroSuda]

sudo FOO=BAR baz ?

[tonistiigi]

@AkihiroSuda That's a bit problematic with the current functions because the env are created later inside runBuildkitd. Also not sure if some of the builtin ones like $HOME would start to interfer with rootless.

[tonistiigi]

this feature LGTM but we should find some fix for the sudo issue so these tests do not become flaky.

[tonistiigi]

I guess with the BUILDKIT_RUN_NETWORK_INTEGRATION_TESTS protection the sudo problem does not affect this PR anymore as the BUILDKIT_RUN_NETWORK_INTEGRATION_TESTS env is not forwarded either on rootless mode. So we can track that as a follow-up issue.