Factory: Fix fake return value issue on creating template

Now, function NewFactory will return nil even create template
does't complete. As for this, it will tell user that factory
has been initialized no matter whether the template is created
or not. This patch correct it by adding another return value
of error in NewFactory.

Testing initFactoryCommand when enable template will need root
privilege to mount tmpfs. So skip it for no-root user.

Testing initFactoryCommand func will create template, but no
proxy type assigned to VMconfig which will using katabuiltinProxy
instead. this will lead to failure for this type of proxy will
check proxyparams which contains many null value. This commit
fix it by substitute katabuiltinProxy as noopProxy when for test
purpose.

Fixes: kata-containers#1333
Signed-off-by: Jianyong Wu <[email protected]>

cli/factory.go

@@ -185,6 +185,7 @@ var initFactoryCommand = cli.Command{
 					HypervisorConfig: runtimeConfig.HypervisorConfig,
 					AgentType:        runtimeConfig.AgentType,
 					AgentConfig:      runtimeConfig.AgentConfig,
+					ProxyType:        runtimeConfig.ProxyType,
 				},
 			}
 			kataLog.WithField("factory", factoryConfig).Info("create vm factory")

cli/factory_test.go

@@ -17,6 +17,8 @@ import (
 	vc "github.com/kata-containers/runtime/virtcontainers"
 )
 
+const testDisabledAsNonRoot = "Test disabled as requires root privileges"
+
 func TestFactoryCLIFunctionNoRuntimeConfig(t *testing.T) {
 	assert := assert.New(t)
 
@@ -63,9 +65,14 @@ func TestFactoryCLIFunctionInit(t *testing.T) {
 	assert.Nil(err)
 
 	// With template
+	if os.Geteuid() != 0 {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
 	runtimeConfig.FactoryConfig.Template = true
 	runtimeConfig.HypervisorType = vc.MockHypervisor
 	runtimeConfig.AgentType = vc.NoopAgentType
+	runtimeConfig.ProxyType = vc.NoopProxyType
 	ctx.App.Metadata["runtimeConfig"] = runtimeConfig
 	fn, ok = initFactoryCommand.Action.(func(context *cli.Context) error)
 	assert.True(ok)

virtcontainers/factory/factory.go

@@ -67,7 +67,10 @@ func NewFactory(ctx context.Context, config Config, fetchOnly bool) (vc.Factory,
 				return nil, err
 			}
 		} else {
-			b = template.New(ctx, config.VMConfig)
+			b, err = template.New(ctx, config.VMConfig)
+			if err != nil {
+				return nil, err
+			}
 		}
 	} else if config.VMCache && config.Cache == 0 {
 		b, err = grpccache.New(ctx, config.VMCacheEndpoint)

virtcontainers/factory/factory_test.go

@@ -8,6 +8,7 @@ package factory
 import (
 	"context"
 	"io/ioutil"
+	"os"
 	"testing"
 
 	vc "github.com/kata-containers/runtime/virtcontainers"
@@ -17,6 +18,8 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+const testDisabledAsNonRoot = "Test disabled as requires root privileges"
+
 func TestNewFactory(t *testing.T) {
 	var config Config
 
@@ -53,6 +56,10 @@ func TestNewFactory(t *testing.T) {
 	f.CloseFactory(ctx)
 
 	// template
+	if os.Geteuid() != 0 {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
 	config.Template = true
 	f, err = NewFactory(ctx, config, false)
 	assert.Nil(err)
@@ -187,6 +194,10 @@ func TestFactoryGetVM(t *testing.T) {
 	ctx := context.Background()
 
 	// direct factory
+	if os.Geteuid() != 0 {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
 	f, err := NewFactory(ctx, Config{VMConfig: vmConfig}, false)
 	assert.Nil(err)
 

virtcontainers/factory/template/template.go

@@ -15,7 +15,6 @@ import (
 
 	vc "github.com/kata-containers/runtime/virtcontainers"
 	"github.com/kata-containers/runtime/virtcontainers/factory/base"
-	"github.com/kata-containers/runtime/virtcontainers/factory/direct"
 	"github.com/kata-containers/runtime/virtcontainers/store"
 )
 
@@ -42,14 +41,13 @@ func Fetch(config vc.VMConfig) (base.FactoryBase, error) {
 }
 
 // New creates a new VM template factory.
-func New(ctx context.Context, config vc.VMConfig) base.FactoryBase {
+func New(ctx context.Context, config vc.VMConfig) (base.FactoryBase, error) {
 	statePath := store.RunVMStoragePath + "/template"
 	t := &template{statePath, config}
 
 	err := t.prepareTemplateFiles()
 	if err != nil {
-		// fallback to direct factory if template is not supported.
-		return direct.New(ctx, config)
+		return nil, err
 	}
 	defer func() {
 		if err != nil {
@@ -59,11 +57,10 @@ func New(ctx context.Context, config vc.VMConfig) base.FactoryBase {
 
 	err = t.createTemplateVM(ctx)
 	if err != nil {
-		// fallback to direct factory if template is not supported.
-		return direct.New(ctx, config)
+		return nil, err
 	}
 
-	return t
+	return t, nil
 }
 
 // Config returns template factory's configuration.
@@ -116,7 +113,9 @@ func (t *template) createTemplateVM(ctx context.Context) error {
 	config.HypervisorConfig.MemoryPath = t.statePath + "/memory"
 	config.HypervisorConfig.DevicesStatePath = t.statePath + "/state"
 	// template vm uses builtin proxy
-	config.ProxyType = templateProxyType
+	if config.ProxyType != "noopProxy" {
+		config.ProxyType = templateProxyType
+	}
 
 	vm, err := vc.NewVM(ctx, config)
 	if err != nil {

virtcontainers/factory/template/template_test.go

@@ -17,7 +17,13 @@ import (
 	vc "github.com/kata-containers/runtime/virtcontainers"
 )
 
+const testDisabledAsNonRoot = "Test disabled as requires root privileges"
+
 func TestTemplateFactory(t *testing.T) {
+	if os.Geteuid() != 0 {
+		t.Skip(testDisabledAsNonRoot)
+	}
+
 	assert := assert.New(t)
 
 	templateWaitForAgent = 1 * time.Microsecond
@@ -40,7 +46,8 @@ func TestTemplateFactory(t *testing.T) {
 	ctx := context.Background()
 
 	// New
-	f := New(ctx, vmConfig)
+	f, err := New(ctx, vmConfig)
+	assert.Nil(err)
 
 	// Config
 	assert.Equal(f.Config(), vmConfig)
@@ -74,7 +81,7 @@ func TestTemplateFactory(t *testing.T) {
 	assert.Nil(err)
 
 	err = tt.createTemplateVM(ctx)
-	assert.Error(err)
+	assert.Nil(err)
 
 	templateProxyType = vc.NoopProxyType
 	vm, err = tt.GetBaseVM(ctx, vmConfig)