Skip to content

Commit

Permalink
Refactor run command networking options for Windows support.
Browse files Browse the repository at this point in the history 
Refactor the loading and application of the networking-related
arguments for the `nerdctl run` command in order to enable
Windows container networking support through CNI.

To facilitate this, the following major changes were made:
    * moved all networking-related container spec definitions
      to pkg/containerutil/container_network_manager_*
    * refactored network-related argiment loading and application
      in cmd/nerdctl/container_run.go
    * enabled some of the networking-related tests on Windows

Signed-off-by: Nashwan Azhari <[email protected]>
  • Loading branch information
@aznashwan
aznashwan committed Mar 16, 2023
1 parent 63aed47 commit 67ff631
Show file tree
Hide file tree
Showing 26 changed files with 1,956 additions and 585 deletions.
13 changes: 12 additions & 1 deletion cmd/nerdctl/container_create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"runtime"

"github.com/containerd/nerdctl/pkg/clientutil"
"github.com/containerd/nerdctl/pkg/containerutil"
"github.com/spf13/cobra"
)

Expand Down Expand Up @@ -73,7 +74,17 @@ func createAction(cmd *cobra.Command, args []string) error {
return err
}

container, gc, err := createContainer(ctx, cmd, client, globalOptions, args, platform, false, flagT, true)
netFlags, err := loadNetworkFlags(cmd)
if err != nil {
return fmt.Errorf("failed to load networking flags: %s", err)
}

netManager, err := containerutil.NewNetworkingOptionsManager(globalOptions, netFlags)
if err != nil {
return err
}

container, gc, err := createContainer(ctx, cmd, client, netManager, globalOptions, args, platform, false, flagT, true)
if err != nil {
if gc != nil {
gc()
Expand Down
69 changes: 35 additions & 34 deletions cmd/nerdctl/container_create_linux_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,45 +67,46 @@ func TestCreateWithMACAddress(t *testing.T) {
}
for i, test := range tests {
containerName := fmt.Sprintf("%s_%d", tID, i)
macAddress, err := nettestutil.GenerateMACAddress()
if err != nil {
t.Errorf("failed to generate MAC address: %s", err)
}
if test.Expect == "" && !test.WantErr {
test.Expect = macAddress
}
t.Cleanup(func() {
base.Cmd("rm", "-f", containerName).Run()
})
cmd := base.Cmd("create", "--network", test.Network, "--mac-address", macAddress, "--name", containerName, testutil.CommonImage, "cat", "/sys/class/net/eth0/address")
if !test.WantErr {
cmd.AssertOK()
base.Cmd("start", containerName).AssertOK()
cmd = base.Cmd("logs", containerName)
cmd.AssertOK()
cmd.AssertOutContains(test.Expect)
} else {
if (testutil.GetTarget() == testutil.Docker && test.Network == networkIPvlan) || test.Network == "none" {
// 1. unlike nerdctl
// when using network ipvlan in Docker
// it delays fail on executing start command
// 2. start on network none will success in both
// nerdctl and Docker
testName := fmt.Sprintf("%s_container:%s_network:%s_expect:%s", tID, containerName, test.Network, test.Expect)
t.Run(testName, func(tt *testing.T) {
macAddress, err := nettestutil.GenerateMACAddress()
if err != nil {
tt.Errorf("failed to generate MAC address: %s", err)
}
if test.Expect == "" && !test.WantErr {
test.Expect = macAddress
}
defer base.Cmd("rm", "-f", containerName).Run()
cmd := base.Cmd("create", "--network", test.Network, "--mac-address", macAddress, "--name", containerName, testutil.CommonImage, "cat", "/sys/class/net/eth0/address")
if !test.WantErr {
cmd.AssertOK()
cmd = base.Cmd("start", containerName)
base.Cmd("start", containerName).AssertOK()
cmd = base.Cmd("logs", containerName)
cmd.AssertOK()
cmd.AssertOutContains(test.Expect)
} else {
if (testutil.GetTarget() == testutil.Docker && test.Network == networkIPvlan) || test.Network == "none" {
// 1. unlike nerdctl
// when using network ipvlan in Docker
// it delays fail on executing start command
// 2. start on network none will success in both
// nerdctl and Docker
cmd.AssertOK()
cmd = base.Cmd("start", containerName)
if test.Network == "none" {
// we check the result on logs command
cmd.AssertOK()
cmd = base.Cmd("logs", containerName)
}
}
cmd.AssertCombinedOutContains(test.Expect)
if test.Network == "none" {
// we check the result on logs command
cmd.AssertOK()
cmd = base.Cmd("logs", containerName)
} else {
cmd.AssertFail()
}
}
cmd.AssertCombinedOutContains(test.Expect)
if test.Network == "none" {
cmd.AssertOK()
} else {
cmd.AssertFail()
}
}
})
}
}

Expand Down
141 changes: 78 additions & 63 deletions cmd/nerdctl/container_run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import (
"github.com/containerd/nerdctl/pkg/cmd/container"
"github.com/containerd/nerdctl/pkg/cmd/image"
"github.com/containerd/nerdctl/pkg/consoleutil"
"github.com/containerd/nerdctl/pkg/containerutil"
"github.com/containerd/nerdctl/pkg/defaults"
"github.com/containerd/nerdctl/pkg/errutil"
"github.com/containerd/nerdctl/pkg/flagutil"
Expand Down Expand Up @@ -328,7 +329,17 @@ func runAction(cmd *cobra.Command, args []string) error {
return errors.New("flags -d and --rm cannot be specified together")
}

c, gc, err := createContainer(ctx, cmd, client, globalOptions, args, platform, flagI, flagT, flagD)
netFlags, err := loadNetworkFlags(cmd)
if err != nil {
return fmt.Errorf("failed to load networking flags: %s", err)
}

netManager, err := containerutil.NewNetworkingOptionsManager(globalOptions, netFlags)
if err != nil {
return err
}

c, gc, err := createContainer(ctx, cmd, client, netManager, globalOptions, args, platform, flagI, flagT, flagD)
if err != nil {
if gc != nil {
defer gc()
Expand All @@ -339,6 +350,14 @@ func runAction(cmd *cobra.Command, args []string) error {
id := c.ID()
if rm && !flagD {
defer func() {
// NOTE: OCI hooks (which are used for CNI network setup/teardown on Linux)
// are not currently supported on Windows, so we must explicitly call
// network setup/cleanup from the main nerdctl executable.
if runtime.GOOS == "windows" {
if err := netManager.CleanupNetworking(ctx, c); err != nil {
logrus.Warnf("failed to clean up container networking: %s", err)
}
}
if err := container.RemoveContainer(ctx, c, globalOptions, true, true); err != nil {
logrus.WithError(err).Warnf("failed to remove container %s", id)
}
Expand Down Expand Up @@ -406,7 +425,7 @@ func runAction(cmd *cobra.Command, args []string) error {
}

// FIXME: split to smaller functions
func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd.Client, globalOptions types.GlobalCommandOptions, args []string, platform string, flagI, flagT, flagD bool) (containerd.Container, func(), error) {
func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd.Client, netManager containerutil.NetworkOptionsManager, globalOptions types.GlobalCommandOptions, args []string, platform string, flagI, flagT, flagD bool) (containerd.Container, func(), error) {
// simulate the behavior of double dash
newArg := []string{}
if len(args) >= 2 && args[1] == "--" {
Expand Down Expand Up @@ -439,7 +458,7 @@ func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd
return nil, nil, err
}

stateDir, err := getContainerStateDirPath(cmd, globalOptions, dataStore, id)
stateDir, err := containerutil.ContainerStateDirPath(globalOptions, dataStore, id)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -577,52 +596,40 @@ func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd
}
cOpts = append(cOpts, withStop(stopSignal, stopTimeout, ensuredImage))

hostname := id[0:12]
customHostname, err := cmd.Flags().GetString("hostname")
if err != nil {
return nil, nil, err
}

uts, err := cmd.Flags().GetString("uts")
err = netManager.VerifyNetworkOptions(ctx)
if err != nil {
return nil, nil, err
return nil, nil, fmt.Errorf("failed to verify networking settings: %s", err)
}

if customHostname != "" {
// Docker considers this a validation error so keep compat.
if uts != "" {
return nil, nil, errors.New("conflicting options: hostname and UTS mode")
}
hostname = customHostname
}
if uts == "" {
opts = append(opts, oci.WithHostname(hostname))
internalLabels.hostname = hostname
// `/etc/hostname` does not exist on FreeBSD
if runtime.GOOS == "linux" {
hostnamePath := filepath.Join(stateDir, "hostname")
if err := os.WriteFile(hostnamePath, []byte(hostname+"\n"), 0644); err != nil {
return nil, nil, err
}
opts = append(opts, withCustomEtcHostname(hostnamePath))
}
}

netOpts, netSlice, ipAddress, ports, macAddress, err := generateNetOpts(cmd, globalOptions, dataStore, stateDir, globalOptions.Namespace, id)
netOpts, netNewContainerOpts, err := netManager.ContainerNetworkingOpts(ctx, id)
if err != nil {
return nil, nil, err
return nil, nil, fmt.Errorf("failed to generate networking spec options: %s", err)
}
internalLabels.networks = netSlice
internalLabels.ipAddress = ipAddress
internalLabels.ports = ports
internalLabels.macAddress = macAddress
opts = append(opts, netOpts...)
cOpts = append(cOpts, netNewContainerOpts...)

hookOpt, err := withNerdctlOCIHook(cmd, id)
netLabelOpts, err := netManager.InternalNetworkingOptionLabels(ctx)
if err != nil {
return nil, nil, err
return nil, nil, fmt.Errorf("failed to generate internal networking labels: %s", err)
}
// TODO(aznashwan): more formal way to load net opts into internalLabels:
internalLabels.hostname = netLabelOpts.Hostname
internalLabels.ports = netLabelOpts.PortMappings
internalLabels.ipAddress = netLabelOpts.IPAddress
internalLabels.networks = netLabelOpts.NetworkSlice
internalLabels.macAddress = netLabelOpts.MACAddress

// NOTE: OCI hooks are currently not supported on Windows so we skip setting them altogether.
// The OCI hooks we define (whose logic can be found in pkg/ocihook) primarily
// perform network setup and teardown when using CNI networking.
// On Windows, we are forced to set up and tear down the networking from within nerdctl.
if runtime.GOOS != "windows" {
hookOpt, err := withNerdctlOCIHook(cmd, id)
if err != nil {
return nil, nil, err
}
opts = append(opts, hookOpt)
}
opts = append(opts, hookOpt)

user, err := cmd.Flags().GetString("user")
if err != nil {
Expand Down Expand Up @@ -734,31 +741,50 @@ func createContainer(ctx context.Context, cmd *cobra.Command, client *containerd

var s specs.Spec
spec := containerd.WithSpec(&s, opts...)

cOpts = append(cOpts, spec)

container, err := client.NewContainer(ctx, id, cOpts...)
if err != nil {
container, containerErr := client.NewContainer(ctx, id, cOpts...)
var netSetupErr error
// NOTE: on non-Windows platforms, network setup is performed by OCI hooks.
// Seeing as though Windows does not currently support OCI hooks, we must explicitly
// perform network setup/teardown in the main nerdctl executable.
if containerErr == nil && runtime.GOOS == "windows" {
netSetupErr = netManager.SetupNetworking(ctx, id)
logrus.WithError(netSetupErr).Warnf("networking setup error has occurred")
}

if containerErr != nil || netSetupErr != nil {
gcContainer := func() {
var isErr bool
if errE := os.RemoveAll(stateDir); errE != nil {
isErr = true
if containerErr == nil {
netGcErr := netManager.CleanupNetworking(ctx, container)
if netGcErr != nil {
logrus.WithError(netGcErr).Warnf("failed to revert container %q networking settings", id)
}
}

if rmErr := os.RemoveAll(stateDir); rmErr != nil {
logrus.WithError(rmErr).Warnf("failed to remove container %q state dir %q", id, stateDir)
}

if name != "" {
var errE error
if containerNameStore, errE = namestore.New(dataStore, globalOptions.Namespace); errE != nil {
isErr = true
logrus.WithError(errE).Warnf("failed to instantiate container name store during cleanup for container %q", id)
}
if errE = containerNameStore.Release(name, id); errE != nil {
isErr = true
logrus.WithError(errE).Warnf("failed to release container name store for container %q (%s)", name, id)
}

}
if isErr {
logrus.Warnf("failed to remove container %q", id)
}
}
return nil, gcContainer, err

returnedError := containerErr
if netSetupErr != nil {
returnedError = netSetupErr // mutually exclusive
}
return nil, gcContainer, returnedError
}

return container, nil, nil
}

Expand Down Expand Up @@ -974,17 +1000,6 @@ func withNerdctlOCIHook(cmd *cobra.Command, id string) (oci.SpecOpts, error) {
}, nil
}

func getContainerStateDirPath(cmd *cobra.Command, globalOptions types.GlobalCommandOptions, dataStore, id string) (string, error) {

if globalOptions.Namespace == "" {
return "", errors.New("namespace is required")
}
if strings.Contains(globalOptions.Namespace, "/") {
return "", errors.New("namespace with '/' is unsupported")
}
return filepath.Join(dataStore, "containers", globalOptions.Namespace, id), nil
}

func withContainerLabels(cmd *cobra.Command) ([]containerd.NewContainerOpts, error) {
labelMap, err := readKVStringsMapfFromLabel(cmd)
if err != nil {
Expand Down
12 changes: 12 additions & 0 deletions cmd/nerdctl/container_run_log_driver_syslog_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ func runSyslogTest(t *testing.T, networks []string, syslogFacilities map[string]
}

func TestSyslogNetwork(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("no syslog on Windows")
}

var syslogFacilities = map[string]syslog.Priority{
"user": syslog.LOG_USER,
}
Expand Down Expand Up @@ -146,6 +150,10 @@ func TestSyslogNetwork(t *testing.T) {
}

func TestSyslogFacilities(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("no syslog on Windows")
}

var syslogFacilities = map[string]syslog.Priority{
"kern": syslog.LOG_KERN,
"user": syslog.LOG_USER,
Expand Down Expand Up @@ -192,6 +200,10 @@ func TestSyslogFacilities(t *testing.T) {
}

func TestSyslogFormat(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("no syslog on Windows")
}

var syslogFacilities = map[string]syslog.Priority{
"user": syslog.LOG_USER,
}
Expand Down
Loading

0 comments on commit 67ff631

Please sign in to comment.