Adding netns to jailer #305

xibz · 2019-10-25T03:55:50Z

This adds netns specification per create vm request. In the event that
no netns was found in the runc config, we will then make sure of what
was passed into the create vm request

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tools/docker/Dockerfile

sipsma

Something we need to think about: right now this doesn't really work with the CNI support. When the CNI support was originally added to the Go SDK, it was written such that:

If a CNI network interface was provided for the VM, the netns specified in the machine.JailerConfig would be used unless one wasn't specified, in which case a new one would be created.
When starting a VM, if there was no machine.JailerConfig but a netns was specified (i.e. you are using a CNI interface but w/out JailerConfig being specified) then we just directly start the machine in the netns.

However, I just realized that the jailer support in firecracker-containerd actually doesn't use the machine.JailerCfg field in the go sdk, it uses its own JailerConfig that doesn't get translated to the go sdk's JailerCfg. So right now I think weird things happen if you use a CNI interface w/ a firecracker-containerd JailerConfig netns specified in CreateVM:

The netns specified in the CreateVM call will be set in the runc config, but the go sdk code will not know about that, so then a new netns will be created due to 1. above and then runc itself will be started inside that new netns due to 2. above.
But then runc is going to start the actual Firecracker process inside the separate netns specified as part of CreateVM, which is not where CNI interfaces were actually created, so the VM won't see any of them.

There are probably multiple ways to address this, not sure yet which one is best:

Is it possible to have firecracker-containerd actually set the netns field in the go sdk's machine.JailerConfig when it was set in the CreateVM's JailerConfig?
If the above isn't possible, could we change the Go SDK to move the existing machine.JailerConfig.NetNS field to just be directly in the machine object? Then we can conditionally set that field in firecracker-containerd w/out creating a whole machine.JailerConfig object.

Let me know what you think.

runtime/firecracker-runc-config.json.example

xibz · 2019-10-26T01:28:18Z

If the above isn't possible, could we change the Go SDK to move the existing machine.JailerConfig.NetNS field to just be directly in the machine object? Then we can conditionally set that field in firecracker-containerd w/out creating a whole machine.JailerConfig object.

I think what I can do is modify the SDK code to create a netns handler by passing in the netns path. This will use a default netns in the default handlers, then during jailing it'll create and swap handlers with its netns. Then in the firecracker-containerd code I just need to inject the correct handlers.

xibz · 2019-11-04T20:29:17Z

Waiting on firecracker-microvm/firecracker-go-sdk#155 to be merged

sipsma

Can we also update one of the tests in cni_integ_test.go to use a runc jailer config? Just to make sure it interacts with the network namespacing as expected

tools/docker/Dockerfile

kzys · 2019-11-14T19:09:41Z

go.sum

 github.com/firecracker-microvm/firecracker-go-sdk v0.17.1-0.20191029213755-dbf9a1e05f09 h1:JDfRpK+V2J1Es+Xm6aMJjCqvA4xv1kuWnJfeSopyDwo=
 github.com/firecracker-microvm/firecracker-go-sdk v0.17.1-0.20191029213755-dbf9a1e05f09/go.mod h1:tVXziw7GjioCKVjI5/agymYxUaqJM6q7cp9e6kwjo8Q=


go mod tidy may remove the lines.

Good catch! Fixed

runtime/runc_jailer.go

kzys · 2019-11-14T19:15:27Z

runtime/runc_jailer.go

-	}
+func (j *runcJailer) overwriteConfig(cfg *Config, machineConfig *firecracker.Config, socketPath, configPath string) error {
+	var err error
+	j.once.Do(func() {


What is the purpose of the once here? The all operations inside the closure look have no side-effects.

caches the config so multiple reads on the file do not occur.

Added some docs to clarify the intention

But the function is only called by BuildJailedMachine() -> BuildJailedRootHandler() (this one cloud be private) -> overwriteConfig(). The cache won't be invalidated, even confgPath is different.

Can we remove this one for now? I don't think this file read would be a hot spot.

Was worried about the file system call, but if you think it isn't going to be a hot spot I can remove it.

runtime/runc_jailer.go

runtime/cni_integ_test.go

sipsma · 2019-11-14T22:24:15Z

runtime/runc_jailer.go

+func (j *runcJailer) overwriteConfig(cfg *Config, machineConfig *firecracker.Config, socketPath, configPath string) error {
+	var err error
+	// here we attempt to cache the runc config. If the config has already been
+	// cached, we will return immediately


Is this comment still relevant? Otherwise LGTM

Good catch, fixed!

kzys · 2019-11-14T23:58:01Z

runtime/runc_jailer.go

+
+	if err = json.Unmarshal(configBytes, &spec); err != nil {
+		return nil, errors.Wrapf(err, "failed to unmarshal firecracker-runc-config.json")
+	}


Please use runcConfigPath instead of hard-coding firecracker-runc-config.json here

You don't have to use Sprintf inside Wrapf :)

kzys · 2019-11-15T17:39:05Z

runtime/runc_jailer.go

+
+	if err = json.Unmarshal(configBytes, &spec); err != nil {
+		return nil, errors.Wrapf(err, "failed to unmarshal firecracker-runc-config.json")
+	}


You don't have to use Sprintf inside Wrapf :)

xibz · 2019-11-15T20:29:25Z

You don't have to use Sprintf inside Wrapf :)

lol, fixed!

This change allows for users to pass in a custom network namespace in the create vm request. This will then pass that netns to the SDK which will run the VM in that netns. Signed-off-by: xibz <impactbchang@gmail.com>

xibz requested review from kzys, nmeyerhans, samuelkarp, mxpv, IRCody and sipsma October 25, 2019 03:55

xibz changed the title ~~Jailer netns clean~~ Adding netns to jailer Oct 25, 2019

xibz force-pushed the jailer-netns-clean branch from 6810029 to a7af606 Compare October 25, 2019 03:58

nmeyerhans reviewed Oct 25, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

sipsma reviewed Oct 25, 2019

View reviewed changes

runtime/firecracker-runc-config.json.example Outdated Show resolved Hide resolved

xibz force-pushed the jailer-netns-clean branch from a7af606 to 56ed692 Compare October 26, 2019 01:11

sipsma mentioned this pull request Nov 5, 2019

Moves NetNS to Config from JailerConfig firecracker-microvm/firecracker-go-sdk#155

Merged

xibz force-pushed the jailer-netns-clean branch 10 times, most recently from 554d64c to b7b87f3 Compare November 8, 2019 22:45

sipsma reviewed Nov 8, 2019

View reviewed changes

tools/docker/Dockerfile Outdated Show resolved Hide resolved

xibz force-pushed the jailer-netns-clean branch 4 times, most recently from bebea92 to 6e4ccce Compare November 9, 2019 00:21

xibz requested review from sipsma and nmeyerhans November 12, 2019 19:46

xibz force-pushed the jailer-netns-clean branch 2 times, most recently from 0e6cbbf to b825062 Compare November 14, 2019 18:52

kzys reviewed Nov 14, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch from b825062 to 0337126 Compare November 14, 2019 19:12

sipsma reviewed Nov 14, 2019

View reviewed changes

runtime/runc_jailer.go Outdated Show resolved Hide resolved

kzys reviewed Nov 14, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch 2 times, most recently from 2fe34e8 to 3381183 Compare November 14, 2019 19:21

sipsma reviewed Nov 14, 2019

View reviewed changes

runtime/runc_jailer.go Outdated Show resolved Hide resolved

xibz force-pushed the jailer-netns-clean branch from 3381183 to 9a9c431 Compare November 14, 2019 19:29

sipsma reviewed Nov 14, 2019

View reviewed changes

runtime/cni_integ_test.go Show resolved Hide resolved

xibz force-pushed the jailer-netns-clean branch 2 times, most recently from 8d3c678 to e18daaa Compare November 14, 2019 21:14

sipsma approved these changes Nov 14, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch from e18daaa to 23204f6 Compare November 14, 2019 23:26

kzys suggested changes Nov 15, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch from 23204f6 to 8903ba8 Compare November 15, 2019 00:11

kzys suggested changes Nov 15, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch from 8903ba8 to cc43916 Compare November 15, 2019 20:29

xibz force-pushed the jailer-netns-clean branch from cc43916 to 5dc0b68 Compare November 18, 2019 19:03

kzys approved these changes Nov 18, 2019

View reviewed changes

xibz force-pushed the jailer-netns-clean branch from 5dc0b68 to 0100a64 Compare November 18, 2019 19:25

Add support for netns

e1c305d

This change allows for users to pass in a custom network namespace in the create vm request. This will then pass that netns to the SDK which will run the VM in that netns. Signed-off-by: xibz <impactbchang@gmail.com>

xibz force-pushed the jailer-netns-clean branch from 0100a64 to e1c305d Compare November 18, 2019 19:33

xibz merged commit 52dc331 into firecracker-microvm:master Nov 18, 2019

xibz deleted the jailer-netns-clean branch November 18, 2019 19:46

xibz mentioned this pull request Dec 10, 2019

jailer improvements #258

Open

		github.com/firecracker-microvm/firecracker-go-sdk v0.17.1-0.20191029213755-dbf9a1e05f09 h1:JDfRpK+V2J1Es+Xm6aMJjCqvA4xv1kuWnJfeSopyDwo=
		github.com/firecracker-microvm/firecracker-go-sdk v0.17.1-0.20191029213755-dbf9a1e05f09/go.mod h1:tVXziw7GjioCKVjI5/agymYxUaqJM6q7cp9e6kwjo8Q=

Adding netns to jailer #305

Adding netns to jailer #305

Uh oh!

Conversation

xibz commented Oct 25, 2019

Uh oh!

Uh oh!

sipsma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xibz commented Oct 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xibz commented Nov 4, 2019

Uh oh!

sipsma left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xibz Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kzys Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

xibz commented Nov 15, 2019

Uh oh!

Uh oh!

xibz commented Oct 26, 2019 •

edited

Loading

xibz Nov 14, 2019 •

edited

Loading

kzys Nov 14, 2019 •

edited

Loading