From 469e098543b02be93a29c08768b8fd1cc3f3cd73 Mon Sep 17 00:00:00 2001 From: Eric Ernst Date: Mon, 23 May 2022 12:27:55 -0700 Subject: [PATCH 1/4] katautils: don't do validation when loading hypervisor config Policy for whats valid/invalid within the config varies by VMM, host, and by silicon architecture. Let's keep katautils simple for just translating a toml to the hypervisor config structure, and leave validation to virtcontainers. Without this change, we're doing duplicate validation. Signed-off-by: Eric Ernst --- src/runtime/pkg/katautils/config.go | 43 +++++++++++------------- src/runtime/pkg/katautils/config_test.go | 12 +++---- src/runtime/virtcontainers/qemu.go | 1 - 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/runtime/pkg/katautils/config.go b/src/runtime/pkg/katautils/config.go index dbdfdac303..fd6df4c3ca 100644 --- a/src/runtime/pkg/katautils/config.go +++ b/src/runtime/pkg/katautils/config.go @@ -220,7 +220,7 @@ func (h hypervisor) initrd() (string, error) { p := h.Initrd if p == "" { - return "", errors.New("initrd is not set") + return "", nil } return ResolvePath(p) @@ -230,7 +230,7 @@ func (h hypervisor) image() (string, error) { p := h.Image if p == "" { - return "", errors.New("image is not set") + return "", nil } return ResolvePath(p) @@ -474,24 +474,6 @@ func (h hypervisor) vhostUserStorePath() string { return h.VhostUserStorePath } -func (h hypervisor) getInitrdAndImage() (initrd string, image string, err error) { - initrd, errInitrd := h.initrd() - - image, errImage := h.image() - - if h.ConfidentialGuest && h.MachineType == vc.QemuCCWVirtio { - if image != "" || initrd != "" { - return "", "", errors.New("Neither the image nor initrd path may be set for Secure Execution") - } - } else if image != "" && initrd != "" { - return "", "", errors.New("having both an image and an initrd defined in the configuration file is not supported") - } else if errInitrd != nil && errImage != nil { - return "", "", fmt.Errorf("Either initrd or image must be set to a valid path (initrd: %v) (image: %v)", errInitrd, errImage) - } - - return -} - func (h hypervisor) getDiskRateLimiterBwMaxRate() int64 { return h.DiskRateLimiterBwMaxRate } @@ -601,7 +583,12 @@ func newFirecrackerHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{}, err } - initrd, image, err := h.getInitrdAndImage() + initrd, err := h.initrd() + if err != nil { + return vc.HypervisorConfig{}, err + } + + image, err := h.image() if err != nil { return vc.HypervisorConfig{}, err } @@ -663,7 +650,12 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{}, err } - initrd, image, err := h.getInitrdAndImage() + initrd, err := h.initrd() + if err != nil { + return vc.HypervisorConfig{}, err + } + + image, err := h.image() if err != nil { return vc.HypervisorConfig{}, err } @@ -857,7 +849,12 @@ func newClhHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) { return vc.HypervisorConfig{}, err } - initrd, image, err := h.getInitrdAndImage() + initrd, err := h.initrd() + if err != nil { + return vc.HypervisorConfig{}, err + } + + image, err := h.image() if err != nil { return vc.HypervisorConfig{}, err } diff --git a/src/runtime/pkg/katautils/config_test.go b/src/runtime/pkg/katautils/config_test.go index 0bcac2137f..a33e37a1a4 100644 --- a/src/runtime/pkg/katautils/config_test.go +++ b/src/runtime/pkg/katautils/config_test.go @@ -1017,7 +1017,7 @@ func TestHypervisorDefaultsKernel(t *testing.T) { assert.Equal(h.kernelParams(), kernelParams, "custom hypervisor kernel parameterms wrong") } -// The default initrd path is not returned by h.initrd() +// The default initrd path is not returned by h.initrd(), it isn't an error if path isn't provided func TestHypervisorDefaultsInitrd(t *testing.T) { assert := assert.New(t) @@ -1041,18 +1041,18 @@ func TestHypervisorDefaultsInitrd(t *testing.T) { defaultInitrdPath = testInitrdPath h := hypervisor{} p, err := h.initrd() - assert.Error(err) + assert.NoError(err) assert.Equal(p, "", "default Image path wrong") // test path resolution defaultInitrdPath = testInitrdLinkPath h = hypervisor{} p, err = h.initrd() - assert.Error(err) + assert.NoError(err) assert.Equal(p, "") } -// The default image path is not returned by h.image() +// The default image path is not returned by h.image(), it isn't an error if path isn't provided func TestHypervisorDefaultsImage(t *testing.T) { assert := assert.New(t) @@ -1076,14 +1076,14 @@ func TestHypervisorDefaultsImage(t *testing.T) { defaultImagePath = testImagePath h := hypervisor{} p, err := h.image() - assert.Error(err) + assert.NoError(err) assert.Equal(p, "", "default Image path wrong") // test path resolution defaultImagePath = testImageLinkPath h = hypervisor{} p, err = h.image() - assert.Error(err) + assert.NoError(err) assert.Equal(p, "") } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index c8e48953f6..a91505d285 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -513,7 +513,6 @@ func (q *qemu) CreateVM(ctx context.Context, id string, network Network, hypervi span, ctx := katatrace.Trace(ctx, q.Logger(), "CreateVM", qemuTracingTags, map[string]string{"VM_ID": q.id}) defer span.End() - // Breaks hypervisor abstraction Has Kata Specific logic: See within if err := q.setup(ctx, id, hypervisorConfig); err != nil { return err } From bdf5e5229b751c868d6c49ab966f6de8547cb38e Mon Sep 17 00:00:00 2001 From: Eric Ernst Date: Mon, 23 May 2022 15:58:47 -0700 Subject: [PATCH 2/4] virtcontainers: validate hypervisor config outside of hypervisor itself Depending on the user of it, the hypervisor from hypervisor interface could have differing view on what is valid or not. To help decouple, let's instead check the hypervisor config validity as part of the sandbox creation, rather than as part of the CreateVM call within the hypervisor interface implementation. Fixes: #4251 Signed-off-by: Eric Ernst --- src/runtime/virtcontainers/acrn.go | 4 - src/runtime/virtcontainers/clh.go | 5 - src/runtime/virtcontainers/fc.go | 5 - src/runtime/virtcontainers/hypervisor.go | 55 -------- src/runtime/virtcontainers/hypervisor_test.go | 118 ------------------ src/runtime/virtcontainers/mock_hypervisor.go | 4 - .../virtcontainers/mock_hypervisor_test.go | 16 +-- src/runtime/virtcontainers/qemu.go | 5 - src/runtime/virtcontainers/sandbox.go | 60 +++++++++ src/runtime/virtcontainers/vm.go | 3 +- 10 files changed, 64 insertions(+), 211 deletions(-) diff --git a/src/runtime/virtcontainers/acrn.go b/src/runtime/virtcontainers/acrn.go index f6da05ec58..1c3ebc1476 100644 --- a/src/runtime/virtcontainers/acrn.go +++ b/src/runtime/virtcontainers/acrn.go @@ -348,10 +348,6 @@ func (a *Acrn) createDummyVirtioBlkDev(ctx context.Context, devices []Device) ([ } func (a *Acrn) setConfig(config *HypervisorConfig) error { - if err := config.Valid(); err != nil { - return err - } - a.config = *config return nil diff --git a/src/runtime/virtcontainers/clh.go b/src/runtime/virtcontainers/clh.go index 6984b73be5..85ec05f15a 100644 --- a/src/runtime/virtcontainers/clh.go +++ b/src/runtime/virtcontainers/clh.go @@ -268,11 +268,6 @@ var clhDebugKernelParams = []Param{ //########################################################### func (clh *cloudHypervisor) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - clh.config = *config return nil diff --git a/src/runtime/virtcontainers/fc.go b/src/runtime/virtcontainers/fc.go index 25012d3f3d..434e320417 100644 --- a/src/runtime/virtcontainers/fc.go +++ b/src/runtime/virtcontainers/fc.go @@ -189,11 +189,6 @@ func (fc *firecracker) truncateID(id string) string { } func (fc *firecracker) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - fc.config = *config return nil diff --git a/src/runtime/virtcontainers/hypervisor.go b/src/runtime/virtcontainers/hypervisor.go index 119c4667c3..18ccd7ae26 100644 --- a/src/runtime/virtcontainers/hypervisor.go +++ b/src/runtime/virtcontainers/hypervisor.go @@ -568,61 +568,6 @@ func (conf *HypervisorConfig) CheckTemplateConfig() error { return nil } -func (conf *HypervisorConfig) Valid() error { - // Kata specific checks. Should be done outside the hypervisor - if conf.KernelPath == "" { - return fmt.Errorf("Missing kernel path") - } - - if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { - if conf.ImagePath != "" || conf.InitrdPath != "" { - fmt.Println("yes, failing") - return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") - } - } else if conf.ImagePath == "" && conf.InitrdPath == "" { - return fmt.Errorf("Missing image and initrd path") - } else if conf.ImagePath != "" && conf.InitrdPath != "" { - return fmt.Errorf("Image and initrd path cannot be both set") - } - - if err := conf.CheckTemplateConfig(); err != nil { - return err - } - - if conf.NumVCPUs == 0 { - conf.NumVCPUs = defaultVCPUs - } - - if conf.MemorySize == 0 { - conf.MemorySize = defaultMemSzMiB - } - - if conf.DefaultBridges == 0 { - conf.DefaultBridges = defaultBridges - } - - if conf.BlockDeviceDriver == "" { - conf.BlockDeviceDriver = defaultBlockDriver - } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { - conf.BlockDeviceDriver = config.VirtioBlockCCW - } - - if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { - conf.DefaultMaxVCPUs = defaultMaxVCPUs - } - - if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { - hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) - conf.DefaultMaxVCPUs = conf.NumVCPUs - } - - if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { - conf.Msize9p = defaultMsize9p - } - - return nil -} - // AddKernelParam allows the addition of new kernel parameters to an existing // hypervisor configuration. func (conf *HypervisorConfig) AddKernelParam(p Param) error { diff --git a/src/runtime/virtcontainers/hypervisor_test.go b/src/runtime/virtcontainers/hypervisor_test.go index ec475e86eb..31452200d3 100644 --- a/src/runtime/virtcontainers/hypervisor_test.go +++ b/src/runtime/virtcontainers/hypervisor_test.go @@ -86,124 +86,6 @@ func TestNewHypervisorFromUnknownHypervisorType(t *testing.T) { assert.Nil(hy) } -func testHypervisorConfigValid(t *testing.T, hypervisorConfig *HypervisorConfig, success bool) { - err := hypervisorConfig.Valid() - assert := assert.New(t) - assert.False(success && err != nil) - assert.False(!success && err == nil) -} - -func TestHypervisorConfigNoKernelPath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: "", - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigNoImagePath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: "", - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigNoHypervisorPath(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - } - - testHypervisorConfigValid(t, hypervisorConfig, true) -} - -func TestHypervisorConfigIsValid(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - - testHypervisorConfigValid(t, hypervisorConfig, true) -} - -func TestHypervisorConfigBothInitrdAndImage(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), - HypervisorPath: "", - } - - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigSecureExecution(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), - ConfidentialGuest: true, - HypervisorMachineType: QemuCCWVirtio, - } - - // Secure Execution should only specify a kernel (encrypted image contains all components) - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigValidTemplateConfig(t *testing.T) { - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - BootToBeTemplate: true, - BootFromTemplate: true, - } - testHypervisorConfigValid(t, hypervisorConfig, false) - - hypervisorConfig.BootToBeTemplate = false - testHypervisorConfigValid(t, hypervisorConfig, false) - hypervisorConfig.MemoryPath = "foobar" - testHypervisorConfigValid(t, hypervisorConfig, false) - hypervisorConfig.DevicesStatePath = "foobar" - testHypervisorConfigValid(t, hypervisorConfig, true) - - hypervisorConfig.BootFromTemplate = false - hypervisorConfig.BootToBeTemplate = true - testHypervisorConfigValid(t, hypervisorConfig, true) - hypervisorConfig.MemoryPath = "" - testHypervisorConfigValid(t, hypervisorConfig, false) -} - -func TestHypervisorConfigDefaults(t *testing.T) { - assert := assert.New(t) - hypervisorConfig := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - } - testHypervisorConfigValid(t, hypervisorConfig, true) - - hypervisorConfigDefaultsExpected := &HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: "", - NumVCPUs: defaultVCPUs, - MemorySize: defaultMemSzMiB, - DefaultBridges: defaultBridges, - BlockDeviceDriver: defaultBlockDriver, - DefaultMaxVCPUs: defaultMaxVCPUs, - Msize9p: defaultMsize9p, - } - - assert.Exactly(hypervisorConfig, hypervisorConfigDefaultsExpected) -} - func TestAppendParams(t *testing.T) { assert := assert.New(t) paramList := []Param{ diff --git a/src/runtime/virtcontainers/mock_hypervisor.go b/src/runtime/virtcontainers/mock_hypervisor.go index 83e776d289..f4a0b934ec 100644 --- a/src/runtime/virtcontainers/mock_hypervisor.go +++ b/src/runtime/virtcontainers/mock_hypervisor.go @@ -31,10 +31,6 @@ func (m *mockHypervisor) HypervisorConfig() HypervisorConfig { } func (m *mockHypervisor) setConfig(config *HypervisorConfig) error { - if err := config.Valid(); err != nil { - return err - } - return nil } diff --git a/src/runtime/virtcontainers/mock_hypervisor_test.go b/src/runtime/virtcontainers/mock_hypervisor_test.go index 5e89ae8a69..0159a993dd 100644 --- a/src/runtime/virtcontainers/mock_hypervisor_test.go +++ b/src/runtime/virtcontainers/mock_hypervisor_test.go @@ -21,9 +21,9 @@ func TestMockHypervisorCreateVM(t *testing.T) { config: &SandboxConfig{ ID: "mock_sandbox", HypervisorConfig: HypervisorConfig{ - KernelPath: "", - ImagePath: "", - HypervisorPath: "", + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), }, }, } @@ -33,16 +33,6 @@ func TestMockHypervisorCreateVM(t *testing.T) { ctx := context.Background() - // wrong config - err = m.CreateVM(ctx, sandbox.config.ID, network, &sandbox.config.HypervisorConfig) - assert.Error(err) - - sandbox.config.HypervisorConfig = HypervisorConfig{ - KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), - ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), - HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), - } - err = m.CreateVM(ctx, sandbox.config.ID, network, &sandbox.config.HypervisorConfig) assert.NoError(err) } diff --git a/src/runtime/virtcontainers/qemu.go b/src/runtime/virtcontainers/qemu.go index a91505d285..7440c82587 100644 --- a/src/runtime/virtcontainers/qemu.go +++ b/src/runtime/virtcontainers/qemu.go @@ -461,11 +461,6 @@ func (q *qemu) setupFileBackedMem(knobs *govmmQemu.Knobs, memory *govmmQemu.Memo } func (q *qemu) setConfig(config *HypervisorConfig) error { - err := config.Valid() - if err != nil { - return err - } - q.config = *config return nil diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index a995f1f77b..550362afdd 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -593,6 +593,10 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor s.Logger().WithError(err).Debug("restore sandbox failed") } + if err := validateHypervisorConfig(&sandboxConfig.HypervisorConfig); err != nil { + return nil, err + } + // store doesn't require hypervisor to be stored immediately if err = s.hypervisor.CreateVM(ctx, s.id, s.network, &sandboxConfig.HypervisorConfig); err != nil { return nil, err @@ -605,6 +609,62 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor return s, nil } +func validateHypervisorConfig(conf *HypervisorConfig) error { + if conf.KernelPath == "" { + return fmt.Errorf("Missing kernel path") + } + + QemuCCWVirtio := "" + if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { + if conf.ImagePath != "" || conf.InitrdPath != "" { + fmt.Println("yes, failing") + return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") + } + } else if conf.ImagePath == "" && conf.InitrdPath == "" { + return fmt.Errorf("Missing image and initrd path") + } else if conf.ImagePath != "" && conf.InitrdPath != "" { + return fmt.Errorf("Image and initrd path cannot be both set") + } + + if err := conf.CheckTemplateConfig(); err != nil { + return err + } + + if conf.NumVCPUs == 0 { + conf.NumVCPUs = defaultVCPUs + } + + if conf.MemorySize == 0 { + conf.MemorySize = defaultMemSzMiB + } + + if conf.DefaultBridges == 0 { + conf.DefaultBridges = defaultBridges + } + + if conf.BlockDeviceDriver == "" { + conf.BlockDeviceDriver = defaultBlockDriver + } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { + conf.BlockDeviceDriver = config.VirtioBlockCCW + } + + if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { + conf.DefaultMaxVCPUs = defaultMaxVCPUs + } + + if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { + hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) + conf.DefaultMaxVCPUs = conf.NumVCPUs + } + + if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { + conf.Msize9p = defaultMsize9p + } + + return nil + +} + func (s *Sandbox) createResourceController() error { var err error cgroupPath := "" diff --git a/src/runtime/virtcontainers/vm.go b/src/runtime/virtcontainers/vm.go index a9db8efc06..a96661d434 100644 --- a/src/runtime/virtcontainers/vm.go +++ b/src/runtime/virtcontainers/vm.go @@ -42,9 +42,8 @@ type VMConfig struct { HypervisorConfig HypervisorConfig } -// Valid Check VMConfig validity. func (c *VMConfig) Valid() error { - return c.HypervisorConfig.Valid() + return validateHypervisorConfig(&c.HypervisorConfig) } // ToGrpc convert VMConfig struct to grpc format pb.GrpcVMConfig. From 5f936f268f0fc878963c80e2c75aa5ac9117f341 Mon Sep 17 00:00:00 2001 From: Eric Ernst Date: Mon, 23 May 2022 17:15:50 -0700 Subject: [PATCH 3/4] virtcontainers: config validation is host specific Ideally this config validation would be in a seperate package (katautils?), but that would introduce circular dependency since we'd call it from vc, and it depends on vc types (which, shouldn't be vc, but probably a hypervisor package instead). Signed-off-by: Eric Ernst --- .../hypervisor_config_darwin.go | 33 +++++ .../virtcontainers/hypervisor_config_linux.go | 67 ++++++++++ .../hypervisor_config_linux_test.go | 114 ++++++++++++++++++ .../virtcontainers/hypervisor_config_test.go | 30 +++++ src/runtime/virtcontainers/sandbox.go | 56 --------- 5 files changed, 244 insertions(+), 56 deletions(-) create mode 100644 src/runtime/virtcontainers/hypervisor_config_darwin.go create mode 100644 src/runtime/virtcontainers/hypervisor_config_linux.go create mode 100644 src/runtime/virtcontainers/hypervisor_config_linux_test.go create mode 100644 src/runtime/virtcontainers/hypervisor_config_test.go diff --git a/src/runtime/virtcontainers/hypervisor_config_darwin.go b/src/runtime/virtcontainers/hypervisor_config_darwin.go new file mode 100644 index 0000000000..e074d59ea6 --- /dev/null +++ b/src/runtime/virtcontainers/hypervisor_config_darwin.go @@ -0,0 +1,33 @@ +// Copyright (c) 2022 Apple Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "fmt" +) + +func validateHypervisorConfig(conf *HypervisorConfig) error { + + if conf.KernelPath == "" { + return fmt.Errorf("Missing kernel path") + } + + if conf.ImagePath == "" && conf.InitrdPath == "" { + return fmt.Errorf("Missing image and initrd path") + } else if conf.ImagePath != "" && conf.InitrdPath != "" { + return fmt.Errorf("Image and initrd path cannot be both set") + } + + if conf.NumVCPUs == 0 { + conf.NumVCPUs = defaultVCPUs + } + + if conf.MemorySize == 0 { + conf.MemorySize = defaultMemSzMiB + } + + return nil +} diff --git a/src/runtime/virtcontainers/hypervisor_config_linux.go b/src/runtime/virtcontainers/hypervisor_config_linux.go new file mode 100644 index 0000000000..f1f20d3151 --- /dev/null +++ b/src/runtime/virtcontainers/hypervisor_config_linux.go @@ -0,0 +1,67 @@ +// Copyright (c) 2022 Apple Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "fmt" + + "github.com/kata-containers/kata-containers/src/runtime/pkg/device/config" +) + +func validateHypervisorConfig(conf *HypervisorConfig) error { + + if conf.KernelPath == "" { + return fmt.Errorf("Missing kernel path") + } + + if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { + if conf.ImagePath != "" || conf.InitrdPath != "" { + fmt.Println("yes, failing") + return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") + } + } else if conf.ImagePath == "" && conf.InitrdPath == "" { + return fmt.Errorf("Missing image and initrd path") + } else if conf.ImagePath != "" && conf.InitrdPath != "" { + return fmt.Errorf("Image and initrd path cannot be both set") + } + + if err := conf.CheckTemplateConfig(); err != nil { + return err + } + + if conf.NumVCPUs == 0 { + conf.NumVCPUs = defaultVCPUs + } + + if conf.MemorySize == 0 { + conf.MemorySize = defaultMemSzMiB + } + + if conf.DefaultBridges == 0 { + conf.DefaultBridges = defaultBridges + } + + if conf.BlockDeviceDriver == "" { + conf.BlockDeviceDriver = defaultBlockDriver + } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { + conf.BlockDeviceDriver = config.VirtioBlockCCW + } + + if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { + conf.DefaultMaxVCPUs = defaultMaxVCPUs + } + + if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { + hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) + conf.DefaultMaxVCPUs = conf.NumVCPUs + } + + if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { + conf.Msize9p = defaultMsize9p + } + + return nil +} diff --git a/src/runtime/virtcontainers/hypervisor_config_linux_test.go b/src/runtime/virtcontainers/hypervisor_config_linux_test.go new file mode 100644 index 0000000000..609e52fd73 --- /dev/null +++ b/src/runtime/virtcontainers/hypervisor_config_linux_test.go @@ -0,0 +1,114 @@ +// Copyright (c) 2022 Apple Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestHypervisorConfigNoImagePath(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: "", + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), + } + + testHypervisorConfigValid(t, hypervisorConfig, false) +} + +func TestHypervisorConfigNoHypervisorPath(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: "", + } + + testHypervisorConfigValid(t, hypervisorConfig, true) +} + +func TestHypervisorConfigIsValid(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), + } + + testHypervisorConfigValid(t, hypervisorConfig, true) +} + +func TestHypervisorConfigBothInitrdAndImage(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), + HypervisorPath: "", + } + + testHypervisorConfigValid(t, hypervisorConfig, false) +} + +func TestHypervisorConfigSecureExecution(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + InitrdPath: fmt.Sprintf("%s/%s", testDir, testInitrd), + ConfidentialGuest: true, + HypervisorMachineType: QemuCCWVirtio, + } + + // Secure Execution should only specify a kernel (encrypted image contains all components) + testHypervisorConfigValid(t, hypervisorConfig, false) +} + +func TestHypervisorConfigValidTemplateConfig(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), + BootToBeTemplate: true, + BootFromTemplate: true, + } + testHypervisorConfigValid(t, hypervisorConfig, false) + + hypervisorConfig.BootToBeTemplate = false + testHypervisorConfigValid(t, hypervisorConfig, false) + hypervisorConfig.MemoryPath = "foobar" + testHypervisorConfigValid(t, hypervisorConfig, false) + hypervisorConfig.DevicesStatePath = "foobar" + testHypervisorConfigValid(t, hypervisorConfig, true) + + hypervisorConfig.BootFromTemplate = false + hypervisorConfig.BootToBeTemplate = true + testHypervisorConfigValid(t, hypervisorConfig, true) + hypervisorConfig.MemoryPath = "" + testHypervisorConfigValid(t, hypervisorConfig, false) +} + +func TestHypervisorConfigDefaults(t *testing.T) { + assert := assert.New(t) + hypervisorConfig := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: "", + } + testHypervisorConfigValid(t, hypervisorConfig, true) + + hypervisorConfigDefaultsExpected := &HypervisorConfig{ + KernelPath: fmt.Sprintf("%s/%s", testDir, testKernel), + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: "", + NumVCPUs: defaultVCPUs, + MemorySize: defaultMemSzMiB, + DefaultBridges: defaultBridges, + BlockDeviceDriver: defaultBlockDriver, + DefaultMaxVCPUs: defaultMaxVCPUs, + Msize9p: defaultMsize9p, + } + + assert.Exactly(hypervisorConfig, hypervisorConfigDefaultsExpected) +} diff --git a/src/runtime/virtcontainers/hypervisor_config_test.go b/src/runtime/virtcontainers/hypervisor_config_test.go new file mode 100644 index 0000000000..49558f6a97 --- /dev/null +++ b/src/runtime/virtcontainers/hypervisor_config_test.go @@ -0,0 +1,30 @@ +// Copyright (c) 2022 Apple Inc. +// +// SPDX-License-Identifier: Apache-2.0 +// + +package virtcontainers + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func testHypervisorConfigValid(t *testing.T, hypervisorConfig *HypervisorConfig, success bool) { + err := validateHypervisorConfig(hypervisorConfig) + assert := assert.New(t) + assert.False(success && err != nil) + assert.False(!success && err == nil) +} + +func TestHypervisorConfigNoKernelPath(t *testing.T) { + hypervisorConfig := &HypervisorConfig{ + KernelPath: "", + ImagePath: fmt.Sprintf("%s/%s", testDir, testImage), + HypervisorPath: fmt.Sprintf("%s/%s", testDir, testHypervisor), + } + + testHypervisorConfigValid(t, hypervisorConfig, false) +} diff --git a/src/runtime/virtcontainers/sandbox.go b/src/runtime/virtcontainers/sandbox.go index 550362afdd..d53de26566 100644 --- a/src/runtime/virtcontainers/sandbox.go +++ b/src/runtime/virtcontainers/sandbox.go @@ -609,62 +609,6 @@ func newSandbox(ctx context.Context, sandboxConfig SandboxConfig, factory Factor return s, nil } -func validateHypervisorConfig(conf *HypervisorConfig) error { - if conf.KernelPath == "" { - return fmt.Errorf("Missing kernel path") - } - - QemuCCWVirtio := "" - if conf.ConfidentialGuest && conf.HypervisorMachineType == QemuCCWVirtio { - if conf.ImagePath != "" || conf.InitrdPath != "" { - fmt.Println("yes, failing") - return fmt.Errorf("Neither the image or initrd path may be set for Secure Execution") - } - } else if conf.ImagePath == "" && conf.InitrdPath == "" { - return fmt.Errorf("Missing image and initrd path") - } else if conf.ImagePath != "" && conf.InitrdPath != "" { - return fmt.Errorf("Image and initrd path cannot be both set") - } - - if err := conf.CheckTemplateConfig(); err != nil { - return err - } - - if conf.NumVCPUs == 0 { - conf.NumVCPUs = defaultVCPUs - } - - if conf.MemorySize == 0 { - conf.MemorySize = defaultMemSzMiB - } - - if conf.DefaultBridges == 0 { - conf.DefaultBridges = defaultBridges - } - - if conf.BlockDeviceDriver == "" { - conf.BlockDeviceDriver = defaultBlockDriver - } else if conf.BlockDeviceDriver == config.VirtioBlock && conf.HypervisorMachineType == QemuCCWVirtio { - conf.BlockDeviceDriver = config.VirtioBlockCCW - } - - if conf.DefaultMaxVCPUs == 0 || conf.DefaultMaxVCPUs > defaultMaxVCPUs { - conf.DefaultMaxVCPUs = defaultMaxVCPUs - } - - if conf.ConfidentialGuest && conf.NumVCPUs != conf.DefaultMaxVCPUs { - hvLogger.Warnf("Confidential guests do not support hotplugging of vCPUs. Setting DefaultMaxVCPUs to NumVCPUs (%d)", conf.NumVCPUs) - conf.DefaultMaxVCPUs = conf.NumVCPUs - } - - if conf.Msize9p == 0 && conf.SharedFS != config.VirtioFS { - conf.Msize9p = defaultMsize9p - } - - return nil - -} - func (s *Sandbox) createResourceController() error { var err error cgroupPath := "" From e5be5cb086515ca7539cc6ee35e9d56bddea89f8 Mon Sep 17 00:00:00 2001 From: Eric Ernst Date: Mon, 27 Jun 2022 10:30:35 -0700 Subject: [PATCH 4/4] runtime: device: cleanup outdated comments Prior device config move didn't update the comments. Let's address this, and make sure comments match the new path... Signed-off-by: Eric Ernst --- src/runtime/pkg/device/config/config.go | 4 ++-- src/runtime/pkg/device/config/pmem.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/runtime/pkg/device/config/config.go b/src/runtime/pkg/device/config/config.go index 748c5b4e51..61f9236b9c 100644 --- a/src/runtime/pkg/device/config/config.go +++ b/src/runtime/pkg/device/config/config.go @@ -444,7 +444,7 @@ func getVhostUserDevName(dirname string, majorNum, minorNum uint32) (string, err // DeviceState is a structure which represents host devices // plugged to a hypervisor, one Device can be shared among containers in POD -// Refs: virtcontainers/device/drivers/generic.go:GenericDevice +// Refs: pkg/device/drivers/generic.go:GenericDevice type DeviceState struct { // DriverOptions is specific options for each device driver // for example, for BlockDevice, we can set DriverOptions["block-driver"]="virtio-blk" @@ -459,7 +459,7 @@ type DeviceState struct { ID string // Type is used to specify driver type - // Refs: virtcontainers/device/config/config.go:DeviceType + // Refs: pkg/device/config/config.go:DeviceType Type string // Type of device: c, b, u or p diff --git a/src/runtime/pkg/device/config/pmem.go b/src/runtime/pkg/device/config/pmem.go index 44fd321873..cdd4db9988 100644 --- a/src/runtime/pkg/device/config/pmem.go +++ b/src/runtime/pkg/device/config/pmem.go @@ -26,7 +26,7 @@ const ( ) var ( - pmemLog = logrus.WithField("source", "virtcontainers/device/config") + pmemLog = logrus.WithField("source", "pkg/device/config") ) // SetLogger sets up a logger for this pkg