From da2b0a806f597ab320ce396ad4296324e92e8ad7 Mon Sep 17 00:00:00 2001 From: Daniel Milchev Date: Thu, 26 Oct 2023 11:39:28 +0300 Subject: [PATCH] Improve containerd client unit tests (#203) [#51] Improve containerd client unit tests Signed-off-by: Daniel Milchev fixed-term.daniel.milchev@bosch.io --- containerm/ctr/ctrd_checkpoint.go | 17 ++-- containerm/ctr/ctrd_cio_mgr_internal.go | 3 +- containerm/ctr/ctrd_client_internal_test.go | 50 ++++++++++ containerm/ctr/ctrd_client_test.go | 93 +++++++++++++----- .../ctr/ctrd_container_create_opts_test.go | 97 +++++++++++++++++++ 5 files changed, 224 insertions(+), 36 deletions(-) create mode 100644 containerm/ctr/ctrd_container_create_opts_test.go diff --git a/containerm/ctr/ctrd_checkpoint.go b/containerm/ctr/ctrd_checkpoint.go index 3364ff5..97c11cc 100644 --- a/containerm/ctr/ctrd_checkpoint.go +++ b/containerm/ctr/ctrd_checkpoint.go @@ -83,7 +83,6 @@ func withCheckpointOpt(checkpoint *containerdtypes.Descriptor) containerd.NewTas // getCheckpointDir verifies checkpoint directory for create,remove, list options and checks if checkpoint already exists func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir string, create bool) (string, error) { var checkpointDir string - var err2 error if checkDir != "" { checkpointDir = checkDir } else { @@ -94,27 +93,27 @@ func getCheckpointDir(checkDir, checkpointID, ctrName, ctrID, ctrCheckpointDir s if create { switch { case err == nil && stat.IsDir(): - err2 = fmt.Errorf("checkpoint with name %s already exists for container %s", checkpointID, ctrName) + return checkpointAbsDir, fmt.Errorf("checkpoint with name %s already exists for container %s", checkpointID, ctrName) case err != nil && os.IsNotExist(err): - err2 = os.MkdirAll(checkpointAbsDir, 0700) + return checkpointAbsDir, os.MkdirAll(checkpointAbsDir, 0700) case err != nil: - err2 = err + return checkpointAbsDir, err case err == nil: - err2 = fmt.Errorf("%s exists and is not a directory", checkpointAbsDir) + return checkpointAbsDir, fmt.Errorf("%s exists and is not a directory", checkpointAbsDir) default: // should never get here } } else { switch { case err != nil: - err2 = fmt.Errorf("checkpoint %s does not exist for container %s", checkpointID, ctrName) + return checkpointAbsDir, fmt.Errorf("checkpoint %s does not exist for container %s", checkpointID, ctrName) case err == nil && stat.IsDir(): - err2 = nil + return checkpointAbsDir, nil case err == nil: - err2 = fmt.Errorf("%s exists and is not a directory", checkpointAbsDir) + return checkpointAbsDir, fmt.Errorf("%s exists and is not a directory", checkpointAbsDir) default: // should never get here } } - return checkpointAbsDir, err2 + return checkpointAbsDir, nil } diff --git a/containerm/ctr/ctrd_cio_mgr_internal.go b/containerm/ctr/ctrd_cio_mgr_internal.go index b2116f4..b36e668 100644 --- a/containerm/ctr/ctrd_cio_mgr_internal.go +++ b/containerm/ctr/ctrd_cio_mgr_internal.go @@ -19,7 +19,6 @@ package ctr import ( "context" "fmt" - "io/ioutil" "os" "path/filepath" "sync" @@ -36,7 +35,7 @@ func (mgr *cioMgr) newFIFOSet(processID string, withStdin bool, withTerminal boo return nil, err } - fifoDir, err := ioutil.TempDir(mgr.fifoRootDir, "") + fifoDir, err := os.MkdirTemp(mgr.fifoRootDir, "") if err != nil { return nil, err } diff --git a/containerm/ctr/ctrd_client_internal_test.go b/containerm/ctr/ctrd_client_internal_test.go index dddf6ef..c6368b7 100644 --- a/containerm/ctr/ctrd_client_internal_test.go +++ b/containerm/ctr/ctrd_client_internal_test.go @@ -113,6 +113,56 @@ func TestClientInternalGenerateNewContainerOpts(t *testing.T) { } } +func TestConfigureRuncRuntime(t *testing.T) { + ctrdClient := &containerdClient{ + runcRuntime: types.RuntimeTypeV2runcV2, + } + tests := map[string]struct { + container *types.Container + runtime types.Runtime + }{ + "test_RuntimeTypeV1": { + container: &types.Container{ + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV1, + }, + }, + runtime: ctrdClient.runcRuntime, + }, + "RuntimeTypeV2runcV1": { + container: &types.Container{ + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV2runcV1, + }, + }, + runtime: ctrdClient.runcRuntime, + }, + "RuntimeTypeV2runcV2": { + container: &types.Container{ + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV2runcV2, + }, + }, + runtime: ctrdClient.runcRuntime, + }, + "test_default": { + container: &types.Container{ + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV2kataV2, + }, + }, + runtime: types.RuntimeTypeV2kataV2, + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + ctrdClient.configureRuncRuntime(test.container) + testutil.AssertEqual(t, test.runtime, test.container.HostConfig.Runtime) + }) + } +} + func TestClientInternalGenerateRemoteOpts(t *testing.T) { const containerImageRef = "some.repo/image:tag" testImageInfo := types.Image{ diff --git a/containerm/ctr/ctrd_client_test.go b/containerm/ctr/ctrd_client_test.go index 9c58c3e..ec03eed 100644 --- a/containerm/ctr/ctrd_client_test.go +++ b/containerm/ctr/ctrd_client_test.go @@ -14,6 +14,7 @@ package ctr import ( "context" + "io" "reflect" "testing" "time" @@ -24,7 +25,9 @@ import ( "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/matchers" containerdMocks "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/mocks/containerd" ctrdMocks "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/mocks/ctrd" + ioMocks "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/mocks/io" loggerMocks "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/mocks/logger" + streamsMocks "github.com/eclipse-kanto/container-management/containerm/pkg/testutil/mocks/streams" "github.com/eclipse-kanto/container-management/containerm/streams" "github.com/eclipse-kanto/container-management/containerm/util" @@ -140,9 +143,7 @@ func TestCtrdClientCreateContainer(t *testing.T) { for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { - expectedError := testCase.mockExec() - actualError := testClient.CreateContainer(ctx, testCtr, "") - testutil.AssertError(t, expectedError, actualError) + testutil.AssertError(t, testCase.mockExec(), testClient.CreateContainer(ctx, testCtr, "")) }) } } @@ -555,12 +556,16 @@ func TestAttachContainer(t *testing.T) { defer mockCtrl.Finish() mockIoMgr := NewMockcontainerIOManager(mockCtrl) + mockReadCloser := ioMocks.NewMockReadCloser(mockCtrl) + mockStream := streamsMocks.NewMockStream(mockCtrl) + mockIO := NewMockIO(mockCtrl) + attachConfig := &streams.AttachConfig{UseStdin: true, Stdin: mockReadCloser} + ctx := context.Background() testClient := &containerdClient{ ioMgr: mockIoMgr, } - ctx := context.Background() testCtr := &types.Container{ ID: "test-id", IOConfig: &types.IOConfig{ @@ -569,13 +574,60 @@ func TestAttachContainer(t *testing.T) { }, } - expectedError := log.NewErrorf("failed to initialise IO for container ID = test-id") + tests := map[string]struct { + attachConfig *streams.AttachConfig + mockExec func() error + }{ + "test_containerIO_nil": { + attachConfig: attachConfig, + mockExec: func() error { + mockIoMgr.EXPECT().GetIO(testCtr.ID).Return(nil) + mockIoMgr.EXPECT().InitIO(testCtr.ID, testCtr.IOConfig.OpenStdin).Return(nil, log.NewError("failed to initialise IO for container ID = test-id")) + return log.NewError("failed to initialise IO for container ID = test-id") + }, + }, + "test_container_stdin_true": { + attachConfig: attachConfig, + mockExec: func() error { + errChan := make(chan error) + go func() { + errChan <- nil + close(errChan) + }() + + mockIoMgr.EXPECT().GetIO(testCtr.ID).Return(mockIO) + mockReadCloser.EXPECT().Read(gomock.Any()).DoAndReturn(func(p []byte) (int, error) { + return -1, io.EOF + }) + mockIO.EXPECT().Stream().Return(mockStream) + mockStream.EXPECT().Attach(ctx, gomock.AssignableToTypeOf(attachConfig)).Return(errChan) + + return nil + }, + }, + "test_container_stdin_false": { + attachConfig: &streams.AttachConfig{Stdin: mockReadCloser}, + mockExec: func() error { + errChan := make(chan error) + go func() { + errChan <- nil + close(errChan) + }() - mockIoMgr.EXPECT().GetIO(testCtr.ID).Return(nil) - mockIoMgr.EXPECT().InitIO(testCtr.ID, testCtr.IOConfig.OpenStdin).Return(nil, expectedError) + mockIoMgr.EXPECT().GetIO(testCtr.ID).Return(mockIO) + mockIO.EXPECT().Stream().Return(mockStream) + mockStream.EXPECT().Attach(ctx, gomock.AssignableToTypeOf(attachConfig)).Return(errChan) - actualError := testClient.AttachContainer(ctx, testCtr, &streams.AttachConfig{}) - testutil.AssertError(t, expectedError, actualError) + return nil + }, + }, + } + + for testName, testCase := range tests { + t.Run(testName, func(t *testing.T) { + testutil.AssertError(t, testCase.mockExec(), testClient.AttachContainer(ctx, testCtr, testCase.attachConfig)) + }) + } } func TestPauseContainer(t *testing.T) { @@ -633,9 +685,7 @@ func TestPauseContainer(t *testing.T) { for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { - expectedError := testCase.mockExec(ctx, mockTask) - actualError := testClient.PauseContainer(ctx, testCase.arg) - testutil.AssertError(t, expectedError, actualError) + testutil.AssertError(t, testCase.mockExec(ctx, mockTask), testClient.PauseContainer(ctx, testCase.arg)) }) } } @@ -695,9 +745,7 @@ func TestUnpauseContainer(t *testing.T) { for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { - expectedError := testCase.mockExec(ctx, mockTask) - actualError := testClient.UnpauseContainer(ctx, testCase.arg) - testutil.AssertError(t, expectedError, actualError) + testutil.AssertError(t, testCase.mockExec(ctx, mockTask), testClient.UnpauseContainer(ctx, testCase.arg)) }) } } @@ -886,9 +934,7 @@ func TestRestoreContainer(t *testing.T) { for testName, testCase := range tests { t.Run(testName, func(t *testing.T) { - expectedError := testCase.mockExec() - actualError := testClient.RestoreContainer(ctx, testCtr) - testutil.AssertError(t, expectedError, actualError) + testutil.AssertError(t, testCase.mockExec(), testClient.RestoreContainer(ctx, testCtr)) }) } } @@ -923,11 +969,10 @@ func TestSetContainerExitHooks(t *testing.T) { testClient.SetContainerExitHooks(arg) - got := testClient.ctrdCache.containerExitHooks - testutil.AssertEqual(t, 1, len(got)) + testutil.AssertEqual(t, 1, len(testClient.ctrdCache.containerExitHooks)) - if reflect.ValueOf(got[0]).Pointer() != reflect.ValueOf(arg).Pointer() { - t.Errorf("SetContainerExitHooks() = %v, want %v", reflect.ValueOf(got[0]), reflect.ValueOf(arg)) + if reflect.ValueOf(testClient.ctrdCache.containerExitHooks[0]).Pointer() != reflect.ValueOf(arg).Pointer() { + t.Errorf("SetContainerExitHooks() = %v, want %v", reflect.ValueOf(testClient.ctrdCache.containerExitHooks[0]), reflect.ValueOf(arg)) } } @@ -1118,9 +1163,7 @@ func TestCtrdClientUpdateContainer(t *testing.T) { task: mockTask, } - expectedErr := testCase.mockExec() - actualErr := testClient.UpdateContainer(ctx, testCase.ctr, testCase.resources) - testutil.AssertError(t, expectedErr, actualErr) + testutil.AssertError(t, testCase.mockExec(), testClient.UpdateContainer(ctx, testCase.ctr, testCase.resources)) }) } } diff --git a/containerm/ctr/ctrd_container_create_opts_test.go b/containerm/ctr/ctrd_container_create_opts_test.go new file mode 100644 index 0000000..3711879 --- /dev/null +++ b/containerm/ctr/ctrd_container_create_opts_test.go @@ -0,0 +1,97 @@ +// Copyright (c) 2023 Contributors to the Eclipse Foundation +// +// See the NOTICE file(s) distributed with this work for additional +// information regarding copyright ownership. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 + +package ctr + +import ( + "testing" + + "github.com/eclipse-kanto/container-management/containerm/containers/types" + "github.com/eclipse-kanto/container-management/containerm/pkg/testutil" + + "github.com/containerd/containerd" + "github.com/containerd/containerd/images" +) + +func TestWithRuntimeOpts(t *testing.T) { + tests := map[string]struct { + container *types.Container + }{ + "test_runtime_type_v1": { + &types.Container{ + ID: testCtrID1, + Name: testContainerName, + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV1, + }, + }, + }, + "test_runtime_type_v2": { + &types.Container{ + ID: testCtrID1, + Name: testContainerName, + HostConfig: &types.HostConfig{ + Runtime: types.RuntimeTypeV2runcV1, + }, + }, + }, + "testing_default": { + &types.Container{ + ID: testCtrID1, + Name: testContainerName, + HostConfig: &types.HostConfig{ + Runtime: "", + }, + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + testutil.AssertNotNil(t, WithRuntimeOpts(test.container, "")) + }) + } +} + +func TestWithSpecOpts(t *testing.T) { + tests := map[string]struct { + container *types.Container + }{ + "test_config": { + &types.Container{ + Config: &types.ContainerConfiguration{ + Cmd: []string{"test"}, + Env: []string{"test"}, + }, + HostConfig: &types.HostConfig{}, + }, + }, + "test_privileged": { + &types.Container{ + HostConfig: &types.HostConfig{ + Privileged: true, + }, + }, + }, + "test_extra_capabilities": { + &types.Container{ + HostConfig: &types.HostConfig{ + ExtraCapabilities: []string{"CAP_NET_ADMIN"}, + }, + }, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + testutil.AssertNotNil(t, WithSpecOpts(test.container, containerd.NewImage(&containerd.Client{}, images.Image{}), "/tmp/test")) + }) + } +}