Skip to content
This repository was archived by the owner on May 12, 2021. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions cli/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ type proxy struct {
type runtime struct {
Debug bool `toml:"enable_debug"`
Tracing bool `toml:"enable_tracing"`
DisableNewNetNs bool `toml:"disable_new_netns"`
InterNetworkModel string `toml:"internetworking_model"`
}

Expand Down Expand Up @@ -598,9 +599,7 @@ func loadConfiguration(configPath string, ignoreLogging bool) (resolvedConfigPat
kataLog.Logger.Level = originalLoggerLevel
}

if tomlConf.Runtime.Tracing {
tracing = true
}
tracing = tomlConf.Runtime.Tracing
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're modifying something that's not really part of this commit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because its cyclomatic complexity reaches the top, I must reduce one to add an "if" for netns config.


if tomlConf.Runtime.InterNetworkModel != "" {
err = config.InterNetworkModel.SetModel(tomlConf.Runtime.InterNetworkModel)
Expand All @@ -626,6 +625,11 @@ func loadConfiguration(configPath string, ignoreLogging bool) (resolvedConfigPat
return "", config, err
}

config.DisableNewNetNs = tomlConf.Runtime.DisableNewNetNs
if err := checkNetNsConfig(config); err != nil {
return "", config, err
}

// use no proxy if HypervisorConfig.UseVSock is true
if config.HypervisorConfig.UseVSock {
kataLog.Info("VSOCK supported, configure to not use proxy")
Expand All @@ -640,6 +644,20 @@ func loadConfiguration(configPath string, ignoreLogging bool) (resolvedConfigPat
return resolved, config, nil
}

// checkNetNsConfig performs sanity checks on disable_new_netns config.
// Because it is an expert option and conflicts with some other common configs.
func checkNetNsConfig(config oci.RuntimeConfig) error {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this function :)
Please add a comment explaining what it does as this is an important sanity check of the configuration file.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if config.DisableNewNetNs {
if config.NetmonConfig.Enable {
return fmt.Errorf("config disable_new_netns conflicts with enable_netmon")
}
if config.InterNetworkModel != vc.NetXConnectNoneModel {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about NetXConnectEnlightenedModel? Do we require it to always run inside a new netns?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is not implemented, I think it should be in a new netns like other models.

return fmt.Errorf("config disable_new_netns only works with 'none' internetworking_model")
}
}
return nil
}

// checkHypervisorConfig performs basic "sanity checks" on the hypervisor
// config.
func checkHypervisorConfig(config vc.HypervisorConfig) error {
Expand Down
14 changes: 14 additions & 0 deletions cli/config/configuration.toml.in
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,23 @@ path = "@NETMONPATH@"
# - macvtap
# Used when the Container network interface can be bridged using
# macvtap.
#
# - none
# Used when customize network. Only creates a tap device. No veth pair.
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to add a link here to the following as that shows users visually more about how this will work:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I will add a PR in documentation repo.

#
internetworking_model="@DEFNETWORKMODEL@"

# If enabled, the runtime will create opentracing.io traces and spans.
# (See https://www.jaegertracing.io/docs/getting-started).
# (default: disabled)
#enable_tracing = true

# If enabled, the runtime will not create a network namespace for shim and hypervisor processes.
# This option may have some potential impacts to your host. It should only be used when you know what you're doing.
# `disable_new_netns` conflicts with `enable_netmon`
# `disable_new_netns` conflicts with `internetworking_model=bridged` and `internetworking_model=macvtap`. It works only
# with `internetworking_model=none`. The tap device will be in the host network namespace and can connect to a bridge
# (like OVS) directly.
# If you are using docker, `disable_new_netns` only works with `docker run --net=none`
# (default: false)
#disable_new_netns = true
31 changes: 27 additions & 4 deletions cli/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ type testRuntimeConfig struct {
LogPath string
}

func makeRuntimeConfigFileData(hypervisor, hypervisorPath, kernelPath, imagePath, kernelParams, machineType, shimPath, proxyPath, netmonPath, logPath string, disableBlock bool, blockDeviceDriver string, enableIOThreads bool, hotplugVFIOOnRootBus bool) string {
func makeRuntimeConfigFileData(hypervisor, hypervisorPath, kernelPath, imagePath, kernelParams, machineType, shimPath, proxyPath, netmonPath, logPath string, disableBlock bool, blockDeviceDriver string, enableIOThreads bool, hotplugVFIOOnRootBus, disableNewNetNs bool) string {
return `
# Runtime configuration file

Expand Down Expand Up @@ -77,7 +77,8 @@ func makeRuntimeConfigFileData(hypervisor, hypervisorPath, kernelPath, imagePath
enable_debug = ` + strconv.FormatBool(netmonDebug) + `

[runtime]
enable_debug = ` + strconv.FormatBool(runtimeDebug)
enable_debug = ` + strconv.FormatBool(runtimeDebug) + `
disable_new_netns= ` + strconv.FormatBool(disableNewNetNs)
}

func createConfig(configPath string, fileData string) error {
Expand Down Expand Up @@ -116,8 +117,9 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf
blockDeviceDriver := "virtio-scsi"
enableIOThreads := true
hotplugVFIOOnRootBus := true
disableNewNetNs := false

runtimeConfigFileData := makeRuntimeConfigFileData(hypervisor, hypervisorPath, kernelPath, imagePath, kernelParams, machineType, shimPath, proxyPath, netmonPath, logPath, disableBlockDevice, blockDeviceDriver, enableIOThreads, hotplugVFIOOnRootBus)
runtimeConfigFileData := makeRuntimeConfigFileData(hypervisor, hypervisorPath, kernelPath, imagePath, kernelParams, machineType, shimPath, proxyPath, netmonPath, logPath, disableBlockDevice, blockDeviceDriver, enableIOThreads, hotplugVFIOOnRootBus, disableNewNetNs)

configPath := path.Join(dir, "runtime.toml")
err = createConfig(configPath, runtimeConfigFileData)
Expand Down Expand Up @@ -192,7 +194,8 @@ func createAllRuntimeConfigFiles(dir, hypervisor string) (config testRuntimeConf
ShimType: defaultShim,
ShimConfig: shimConfig,

NetmonConfig: netmonConfig,
NetmonConfig: netmonConfig,
DisableNewNetNs: disableNewNetNs,
}

config = testRuntimeConfig{
Expand Down Expand Up @@ -1455,3 +1458,23 @@ func TestCheckHypervisorConfig(t *testing.T) {
kataLog.Logger.Out = savedOut
}
}

func TestCheckNetNsConfig(t *testing.T) {
assert := assert.New(t)

config := oci.RuntimeConfig{
DisableNewNetNs: true,
NetmonConfig: vc.NetmonConfig{
Enable: true,
},
}
err := checkNetNsConfig(config)
assert.Error(err)

config = oci.RuntimeConfig{
DisableNewNetNs: true,
InterNetworkModel: vc.NetXConnectDefaultModel,
}
err = checkNetNsConfig(config)
assert.Error(err)
}
20 changes: 11 additions & 9 deletions cli/kata-env.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
//
// XXX: Increment for every change to the output format
// (meaning any change to the EnvInfo type).
const formatVersion = "1.0.18"
const formatVersion = "1.0.19"

// MetaInfo stores information on the format of the output itself
type MetaInfo struct {
Expand Down Expand Up @@ -62,10 +62,11 @@ type RuntimeConfigInfo struct {

// RuntimeInfo stores runtime details.
type RuntimeInfo struct {
Version RuntimeVersionInfo
Config RuntimeConfigInfo
Debug bool
Path string
Version RuntimeVersionInfo
Config RuntimeConfigInfo
Debug bool
DisableNewNetNs bool
Path string
}

// RuntimeVersionInfo stores details of the runtime version
Expand Down Expand Up @@ -171,10 +172,11 @@ func getRuntimeInfo(configFile string, config oci.RuntimeConfig) RuntimeInfo {
runtimePath, _ := os.Executable()

return RuntimeInfo{
Debug: config.Debug,
Version: runtimeVersion,
Config: runtimeConfig,
Path: runtimePath,
Debug: config.Debug,
Version: runtimeVersion,
Config: runtimeConfig,
Path: runtimePath,
DisableNewNetNs: config.DisableNewNetNs,
}
}

Expand Down
7 changes: 5 additions & 2 deletions cli/kata-env_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC
blockStorageDriver := "virtio-scsi"
enableIOThreads := true
hotplugVFIOOnRootBus := true
disableNewNetNs := false

// override
defaultProxyPath = proxyPath
Expand Down Expand Up @@ -121,6 +122,7 @@ func makeRuntimeConfig(prefixDir string) (configFile string, config oci.RuntimeC
blockStorageDriver,
enableIOThreads,
hotplugVFIOOnRootBus,
disableNewNetNs,
)

configFile = path.Join(prefixDir, "runtime.toml")
Expand Down Expand Up @@ -293,8 +295,9 @@ func getExpectedRuntimeDetails(config oci.RuntimeConfig, configFile string) Runt
Config: RuntimeConfigInfo{
Path: configFile,
},
Path: runtimePath,
Debug: config.Debug,
Path: runtimePath,
Debug: config.Debug,
DisableNewNetNs: config.DisableNewNetNs,
}
}

Expand Down
5 changes: 5 additions & 0 deletions cli/network.go
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,11 @@ func hostNetworkingRequested(configNetNs string) (bool, error) {
}

func setupNetworkNamespace(config *vc.NetworkConfig) error {
if config.DisableNewNetNs {
kataLog.Info("DisableNewNetNs is on, shim and hypervisor are running in the host netns")
return nil
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be worth adding some log traces here to confirm that we're running without any netns!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be overkill to add this to every log call, but due to the delicate nature of namespaces + golang, how about we log the namespace details at various points in the code [*]? We already log the pid so this will give more detail than a simple "changed namspace" log call:

const nsPath = "/proc/self/ns"

func getNamespaces() (string, error) {
	files, err := ioutil.ReadDir(nsPath)
	if err != nil {
		return "", err
	}

	var ns []string

	for _, file := range files {
		filePath := path.Join(nsPath, file.Name())

		linkName, err := os.Readlink(filePath)
		if err != nil {
			return "", err
		}

		ns = append(ns, linkName)
	}

	sort.Sort(sort.StringSlice(ns))

	namespaces := strings.Join(ns, ",")

	return namespaces, nil
}

This will return something like:

cgroup:[4026531835],ipc:[4026531839],mnt:[4026531840],net:[4026532008],pid:[4026531836],pid:[4026531836],user:[4026531837],uts:[4026531838]

[*] - At runtime startup, whenever we change namespace, and in this function to show if we did or didn't switch ns.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's good to add some logs. But in this function we cannot get container id or sandbox id for log fields.

}

if config.NetNSPath == "" {
n, err := ns.NewNS()
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions cli/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,4 +217,9 @@ func TestSetupNetworkNamespace(t *testing.T) {
n.Close()
unix.Unmount(config.NetNSPath, unix.MNT_DETACH)
os.RemoveAll(config.NetNSPath)

// Config with DisableNewNetNs
config = &vc.NetworkConfig{DisableNewNetNs: true}
err = setupNetworkNamespace(config)
assert.NoError(err)
}
5 changes: 5 additions & 0 deletions virtcontainers/bridgedmacvlan_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,11 @@ func (endpoint *BridgedMacvlanEndpoint) PciAddr() string {
return endpoint.PCIAddr
}

// SetPciAddr sets the PCI address of the endpoint.
func (endpoint *BridgedMacvlanEndpoint) SetPciAddr(pciAddr string) {
endpoint.PCIAddr = pciAddr
}

// NetworkPair returns the network pair of the endpoint.
func (endpoint *BridgedMacvlanEndpoint) NetworkPair() *NetworkInterfacePair {
return &endpoint.NetPair
Expand Down
12 changes: 7 additions & 5 deletions virtcontainers/bridgedmacvlan_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,17 @@ func TestCreateBridgedMacvlanEndpoint(t *testing.T) {

expected := &BridgedMacvlanEndpoint{
NetPair: NetworkInterfacePair{
ID: "uniqueTestID-4",
Name: "br4_kata",
TapInterface: TapInterface{
ID: "uniqueTestID-4",
Name: "br4_kata",
TAPIface: NetworkInterface{
Name: "tap4_kata",
},
},
VirtIface: NetworkInterface{
Name: "eth4",
HardAddr: macAddr.String(),
},
TAPIface: NetworkInterface{
Name: "tap4_kata",
},
NetInterworkingModel: DefaultNetInterworkingModel,
},
EndpointType: BridgedMacvlanEndpointType,
Expand Down
5 changes: 0 additions & 5 deletions virtcontainers/default_network.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package virtcontainers

import (
"context"
"fmt"

"github.com/containernetworking/plugins/pkg/ns"
opentracing "github.com/opentracing/opentracing-go"
Expand Down Expand Up @@ -35,10 +34,6 @@ func (n *defNetwork) run(networkNSPath string, cb func() error) error {
span, _ := n.trace(context.Background(), "run")
defer span.Finish()

if networkNSPath == "" {
return fmt.Errorf("networkNSPath cannot be empty")
}

return doNetNS(networkNSPath, func(_ ns.NetNS) error {
return cb()
})
Expand Down
9 changes: 9 additions & 0 deletions virtcontainers/endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type Endpoint interface {
NetworkPair() *NetworkInterfacePair

SetProperties(NetworkInfo)
SetPciAddr(string)
Attach(hypervisor) error
Detach(netNsCreated bool, netNsPath string) error
HotAttach(h hypervisor) error
Expand All @@ -43,6 +44,9 @@ const (

// MacvtapEndpointType is macvtap network interface.
MacvtapEndpointType EndpointType = "macvtap"

// TapEndpointType is tap network interface.
TapEndpointType EndpointType = "tap"
)

// Set sets an endpoint type based on the input string.
Expand All @@ -63,6 +67,9 @@ func (endpointType *EndpointType) Set(value string) error {
case "macvtap":
*endpointType = MacvtapEndpointType
return nil
case "tap":
*endpointType = TapEndpointType
return nil
default:
return fmt.Errorf("Unknown endpoint type %s", value)
}
Expand All @@ -81,6 +88,8 @@ func (endpointType *EndpointType) String() string {
return string(BridgedMacvlanEndpointType)
case MacvtapEndpointType:
return string(MacvtapEndpointType)
case TapEndpointType:
return string(TapEndpointType)
default:
return ""
}
Expand Down
5 changes: 5 additions & 0 deletions virtcontainers/macvtap_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,11 @@ func (endpoint *MacvtapEndpoint) PciAddr() string {
return endpoint.PCIAddr
}

// SetPciAddr sets the PCI address of the endpoint.
func (endpoint *MacvtapEndpoint) SetPciAddr(pciAddr string) {
endpoint.PCIAddr = pciAddr
}

// NetworkPair returns the network pair of the endpoint.
func (endpoint *MacvtapEndpoint) NetworkPair() *NetworkInterfacePair {
return nil
Expand Down
Loading