GCS tests and features/bugfixes to support them (#1360)

Exposed data and functionality for testing GCS:
* `internal\guest\runtime\hcsv2.Container.InitProcess()`
* `internal\guest\runtime\hcsv2.GetOrAddNetworkNamespace()`
* `internal\guest\runtime\hcsv2.RemoveNetworkNamespace()`
* `internal\guest\runtime\hcsv2.Host.SecurityPolicyEnforcer()`
* `internal\guest\runtime\hcsv2.Host.Transport()`

Fixed bug where `host.RemoveContainer` did not remove the network
namespace for standalone and pod containers.

Updated go-winio version to include bugfixes for closing hvsockets,
specifically to close a socket for writing (needed by internal\cmd
to signal that the stdin stream has finished).

Added doc.go files to guest packages to prevent linter/compiler errors
under windows.

The tests themselves are broken out here:
#1352
#1351

Signed-off-by: Hamza El-Saawy <[email protected]>

Author: helsaawy

.gitignore

@@ -24,6 +24,7 @@ service/pkg/
 *.img
 *.vhd
 *.tar.gz
+*.tar
 
 # Make stuff
 .rootfs-done
@@ -36,5 +37,9 @@ rootfs-conv/*
 deps/*
 out/*
 
+# test results
+test/results
+
+# go workspace files
 go.work
-go.work.sum
+go.work.sum

functional_tests.ps1

@@ -4,7 +4,7 @@ go 1.17
 
 require (
 	github.com/BurntSushi/toml v0.3.1
-	github.com/Microsoft/go-winio v0.4.17
+	github.com/Microsoft/go-winio v0.5.2
 	github.com/cenkalti/backoff/v4 v4.1.1
 	github.com/containerd/cgroups v1.0.1
 	github.com/containerd/console v1.0.2

go.mod

@@ -44,8 +44,9 @@ github.com/Microsoft/go-winio v0.4.16-0.20201130162521-d1ffc52c7331/go.mod h1:XB
 github.com/Microsoft/go-winio v0.4.16/go.mod h1:XB6nPKklQyQ7GC9LdcBEcBl8PF76WugXOPRXwdLnMv0=
 github.com/Microsoft/go-winio v0.4.17-0.20210211115548-6eac466e5fa3/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
 github.com/Microsoft/go-winio v0.4.17-0.20210324224401-5516f17a5958/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
-github.com/Microsoft/go-winio v0.4.17 h1:iT12IBVClFevaf8PuVyi3UmZOVh4OqnaLxDTW2O6j3w=
 github.com/Microsoft/go-winio v0.4.17/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
+github.com/Microsoft/go-winio v0.5.2 h1:a9IhgEQBCUEk6QCdml9CiJGhAws+YwffDHEMp1VMrpA=
+github.com/Microsoft/go-winio v0.5.2/go.mod h1:WpS1mjBmmwHBEWmogvA2mj8546UReBk4v8QkMxJ6pZY=
 github.com/Microsoft/hcsshim v0.8.6/go.mod h1:Op3hHsoHPAvb6lceZHDtd9OkTew38wNoXnJs8iY7rUg=
 github.com/Microsoft/hcsshim v0.8.7-0.20190325164909-8abdbb8205e4/go.mod h1:Op3hHsoHPAvb6lceZHDtd9OkTew38wNoXnJs8iY7rUg=
 github.com/Microsoft/hcsshim v0.8.7/go.mod h1:OHd7sQqRFrYd3RmSgbgji+ctCwkbq2wbEYNSzOYtcBQ=

go.sum

@@ -0,0 +1,2 @@
+// Package linux contains definitions required for making a linux ioctl.
+package linux

internal/guest/linux/doc.go

@@ -1,7 +1,6 @@
 //go:build linux
 // +build linux
 
-// Package linux contains definitions required for making a linux ioctl.
 package linux
 
 import (

internal/guest/linux/ioctl.go

@@ -0,0 +1 @@
+package policy

internal/guest/policy/doc.go

@@ -123,14 +123,19 @@ func (c *Container) ExecProcess(ctx context.Context, process *oci.Process, conSe
 	return pid, nil
 }
 
+// InitProcess returns the container's init process
+func (c *Container) InitProcess() Process {
+	return c.initProcess
+}
+
 // GetProcess returns the Process with the matching 'pid'. If the 'pid' does
 // not exit returns error.
 func (c *Container) GetProcess(pid uint32) (Process, error) {
 	//todo: thread a context to this function call
 	logrus.WithFields(logrus.Fields{
 		logfields.ContainerID: c.id,
 		logfields.ProcessID:   pid,
-	}).Info("opengcs::Container::GetProcesss")
+	}).Info("opengcs::Container::GetProcess")
 	if c.initProcess.pid == pid {
 		return c.initProcess, nil
 	}
@@ -245,3 +250,7 @@ func (c *Container) getStatus() containerStatus {
 func (c *Container) setStatus(st containerStatus) {
 	atomic.StoreUint32((*uint32)(&c.status), uint32(st))
 }
+
+func (c *Container) ID() string {
+	return c.id
+}

internal/guest/runtime/hcsv2/container.go

@@ -51,9 +51,9 @@ func getNetworkNamespace(id string) (*namespace, error) {
 	return ns, nil
 }
 
-// getOrAddNetworkNamespace returns the namespace found by `id` or creates a new
+// GetOrAddNetworkNamespace returns the namespace found by `id` or creates a new
 // one and assigns `id.
-func getOrAddNetworkNamespace(id string) *namespace {
+func GetOrAddNetworkNamespace(id string) *namespace {
 	id = strings.ToLower(id)
 
 	namespaceSync.Lock()
@@ -69,8 +69,8 @@ func getOrAddNetworkNamespace(id string) *namespace {
 	return ns
 }
 
-// removeNetworkNamespace removes the in-memory `namespace` found by `id`.
-func removeNetworkNamespace(ctx context.Context, id string) (err error) {
+// RemoveNetworkNamespace removes the in-memory `namespace` found by `id`.
+func RemoveNetworkNamespace(ctx context.Context, id string) (err error) {
 	_, span := trace.StartSpan(ctx, "hcsv2::removeNetworkNamespace")
 	defer span.End()
 	defer func() { oc.SetSpanStatus(span, err) }()
@@ -123,7 +123,7 @@ func (n *namespace) AssignContainerPid(ctx context.Context, pid int) (err error)
 	defer n.m.Unlock()
 
 	if n.pid != 0 {
-		return errors.Errorf("previously assigned container pid: %d", n.pid)
+		return errors.Errorf("previously assigned container pid %d to network namespace %q", n.pid, n.id)
 	}
 
 	n.pid = pid

internal/guest/runtime/hcsv2/network.go

@@ -12,7 +12,7 @@ import (
 
 func Test_getNetworkNamespace_NotExist(t *testing.T) {
 	defer func() {
-		err := removeNetworkNamespace(context.Background(), t.Name())
+		err := RemoveNetworkNamespace(context.Background(), t.Name())
 		if err != nil {
 			t.Errorf("failed to remove ns with error: %v", err)
 		}
@@ -29,13 +29,13 @@ func Test_getNetworkNamespace_NotExist(t *testing.T) {
 
 func Test_getNetworkNamespace_PreviousExist(t *testing.T) {
 	defer func() {
-		err := removeNetworkNamespace(context.Background(), t.Name())
+		err := RemoveNetworkNamespace(context.Background(), t.Name())
 		if err != nil {
 			t.Errorf("failed to remove ns with error: %v", err)
 		}
 	}()
 
-	ns1 := getOrAddNetworkNamespace(t.Name())
+	ns1 := GetOrAddNetworkNamespace(t.Name())
 	if ns1 == nil {
 		t.Fatal("namespace ns1 should not be nil")
 	}
@@ -50,43 +50,43 @@ func Test_getNetworkNamespace_PreviousExist(t *testing.T) {
 
 func Test_getOrAddNetworkNamespace_NotExist(t *testing.T) {
 	defer func() {
-		err := removeNetworkNamespace(context.Background(), t.Name())
+		err := RemoveNetworkNamespace(context.Background(), t.Name())
 		if err != nil {
 			t.Errorf("failed to remove ns with error: %v", err)
 		}
 	}()
 
-	ns := getOrAddNetworkNamespace(t.Name())
+	ns := GetOrAddNetworkNamespace(t.Name())
 	if ns == nil {
 		t.Fatalf("namespace should not be nil")
 	}
 }
 
 func Test_getOrAddNetworkNamespace_PreviousExist(t *testing.T) {
 	defer func() {
-		err := removeNetworkNamespace(context.Background(), t.Name())
+		err := RemoveNetworkNamespace(context.Background(), t.Name())
 		if err != nil {
 			t.Errorf("failed to remove ns with error: %v", err)
 		}
 	}()
 
-	ns1 := getOrAddNetworkNamespace(t.Name())
-	ns2 := getOrAddNetworkNamespace(t.Name())
+	ns1 := GetOrAddNetworkNamespace(t.Name())
+	ns2 := GetOrAddNetworkNamespace(t.Name())
 	if ns1 != ns2 {
 		t.Fatalf("ns1 %+v != ns2 %+v", ns1, ns2)
 	}
 }
 
 func Test_removeNetworkNamespace_NotExist(t *testing.T) {
-	err := removeNetworkNamespace(context.Background(), t.Name())
+	err := RemoveNetworkNamespace(context.Background(), t.Name())
 	if err != nil {
 		t.Fatalf("failed to remove non-existing ns with error: %v", err)
 	}
 }
 
 func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
 	defer func() {
-		err := removeNetworkNamespace(context.Background(), t.Name())
+		err := RemoveNetworkNamespace(context.Background(), t.Name())
 		if err != nil {
 			t.Errorf("failed to remove ns with error: %v", err)
 		}
@@ -96,7 +96,7 @@ func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
 		networkInstanceIDToName = nsOld
 	}()
 
-	ns := getOrAddNetworkNamespace(t.Name())
+	ns := GetOrAddNetworkNamespace(t.Name())
 
 	networkInstanceIDToName = func(ctx context.Context, id string, _ bool) (string, error) {
 		return "/dev/sdz", nil
@@ -105,15 +105,15 @@ func Test_removeNetworkNamespace_HasAdapters(t *testing.T) {
 	if err != nil {
 		t.Fatalf("failed to add adapter: %v", err)
 	}
-	err = removeNetworkNamespace(context.Background(), t.Name())
+	err = RemoveNetworkNamespace(context.Background(), t.Name())
 	if err == nil {
 		t.Fatal("should have failed to delete namespace with adapters")
 	}
 	err = ns.RemoveAdapter(context.Background(), "test")
 	if err != nil {
 		t.Fatalf("failed to remove adapter: %v", err)
 	}
-	err = removeNetworkNamespace(context.Background(), t.Name())
+	err = RemoveNetworkNamespace(context.Background(), t.Name())
 	if err != nil {
 		t.Fatalf("should not have failed to delete empty namepace got: %v", err)
 	}

internal/guest/runtime/hcsv2/network_test.go

@@ -67,6 +67,8 @@ type containerProcess struct {
 	writersCalled bool
 }
 
+var _ Process = &containerProcess{}
+
 // newProcess returns a containerProcess struct that has been initialized with
 // an outstanding wait for process exit, and post exit an outstanding wait for
 // process cleanup to release all resources once at least 1 waiter has
@@ -262,6 +264,8 @@ type externalProcess struct {
 	remove     func(pid int)
 }
 
+var _ Process = &externalProcess{}
+
 func (ep *externalProcess) Kill(ctx context.Context, signal syscall.Signal) error {
 	if err := syscall.Kill(int(ep.cmd.Process.Pid), signal); err != nil {
 		if err == syscall.ESRCH {

internal/guest/runtime/hcsv2/process.go

@@ -103,7 +103,7 @@ func setupStandaloneContainerSpec(ctx context.Context, id string, spec *oci.Spec
 
 	// Write resolv.conf
 	if !specInternal.MountPresent("/etc/resolv.conf", spec.Mounts) {
-		ns := getOrAddNetworkNamespace(getNetworkNamespaceID(spec))
+		ns := GetOrAddNetworkNamespace(getNetworkNamespaceID(spec))
 		var searches, servers []string
 		for _, n := range ns.Adapters() {
 			if len(n.DNSSuffix) > 0 {

internal/guest/runtime/hcsv2/standalone_container.go

@@ -115,10 +115,29 @@ func (h *Host) SetSecurityPolicy(base64Policy string) error {
 	return nil
 }
 
+func (h *Host) SecurityPolicyEnforcer() securitypolicy.SecurityPolicyEnforcer {
+	return h.securityPolicyEnforcer
+}
+
+func (h *Host) Transport() transport.Transport {
+	return h.vsock
+}
+
 func (h *Host) RemoveContainer(id string) {
 	h.containersMutex.Lock()
 	defer h.containersMutex.Unlock()
 
+	c, ok := h.containers[id]
+	if !ok {
+		return
+	}
+
+	// delete the network namespace for standalone and sandbox containers
+	criType, isCRI := c.spec.Annotations[annotations.KubernetesContainerType]
+	if !isCRI || criType == "sandbox" {
+		RemoveNetworkNamespace(context.Background(), id)
+	}
+
 	delete(h.containers, id)
 }
 
@@ -580,15 +599,15 @@ func modifyCombinedLayers(ctx context.Context, rt guestrequest.RequestType, cl *
 func modifyNetwork(ctx context.Context, rt guestrequest.RequestType, na *guestresource.LCOWNetworkAdapter) (err error) {
 	switch rt {
 	case guestrequest.RequestTypeAdd:
-		ns := getOrAddNetworkNamespace(na.NamespaceID)
+		ns := GetOrAddNetworkNamespace(na.NamespaceID)
 		if err := ns.AddAdapter(ctx, na); err != nil {
 			return err
 		}
 		// This code doesnt know if the namespace was already added to the
 		// container or not so it must always call `Sync`.
 		return ns.Sync(ctx)
 	case guestrequest.RequestTypeRemove:
-		ns := getOrAddNetworkNamespace(na.ID)
+		ns := GetOrAddNetworkNamespace(na.ID)
 		if err := ns.RemoveAdapter(ctx, na.ID); err != nil {
 			return err
 		}