From eadf97765d24e42d1ea286e6c87ae7249a35b0fc Mon Sep 17 00:00:00 2001 From: Jianyong Wu Date: Wed, 13 Mar 2019 06:00:13 -0400 Subject: [PATCH] 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: #1333 Signed-off-by: Jianyong Wu --- cli/factory.go | 1 + cli/factory_test.go | 7 +++++++ virtcontainers/factory/factory.go | 5 ++++- virtcontainers/factory/factory_test.go | 11 +++++++++++ virtcontainers/factory/template/template.go | 15 +++++++-------- virtcontainers/factory/template/template_test.go | 11 +++++++++-- 6 files changed, 39 insertions(+), 11 deletions(-) diff --git a/cli/factory.go b/cli/factory.go index da1793dd7b..55316e35ae 100644 --- a/cli/factory.go +++ b/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") diff --git a/cli/factory_test.go b/cli/factory_test.go index faf5028d73..ff7609a8fb 100644 --- a/cli/factory_test.go +++ b/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) diff --git a/virtcontainers/factory/factory.go b/virtcontainers/factory/factory.go index 4bbe198372..15712c9540 100644 --- a/virtcontainers/factory/factory.go +++ b/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) diff --git a/virtcontainers/factory/factory_test.go b/virtcontainers/factory/factory_test.go index a8e08606c2..7ff3df8cc8 100644 --- a/virtcontainers/factory/factory_test.go +++ b/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) diff --git a/virtcontainers/factory/template/template.go b/virtcontainers/factory/template/template.go index 968b61c28c..26740e3c91 100644 --- a/virtcontainers/factory/template/template.go +++ b/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 { diff --git a/virtcontainers/factory/template/template_test.go b/virtcontainers/factory/template/template_test.go index d08a2ab69e..f2c4e9da84 100644 --- a/virtcontainers/factory/template/template_test.go +++ b/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)