Merge pull request #1086 from jodh-intel/2.0-dev-fix-annotations

annotations: Improve asset annotation handling

src/runtime/virtcontainers/hypervisor.go

@@ -612,11 +612,6 @@ func (conf *HypervisorConfig) HypervisorCtlAssetPath() (string, error) {
 	return conf.assetPath(types.HypervisorCtlAsset)
 }
 
-// JailerAssetPath returns the VM Jailer path
-func (conf *HypervisorConfig) JailerAssetPath() (string, error) {
-	return conf.assetPath(types.JailerAsset)
-}
-
 // CustomHypervisorAsset returns true if the hypervisor asset is a custom one, false otherwise.
 func (conf *HypervisorConfig) CustomHypervisorAsset() bool {
 	return conf.isCustomAsset(types.HypervisorAsset)
@@ -627,11 +622,6 @@ func (conf *HypervisorConfig) FirmwareAssetPath() (string, error) {
 	return conf.assetPath(types.FirmwareAsset)
 }
 
-// CustomFirmwareAsset returns true if the firmware asset is a custom one, false otherwise.
-func (conf *HypervisorConfig) CustomFirmwareAsset() bool {
-	return conf.isCustomAsset(types.FirmwareAsset)
-}
-
 func appendParam(params []Param, parameter string, value string) []Param {
 	return append(params, Param{parameter, value})
 }

src/runtime/virtcontainers/hypervisor_test.go

@@ -447,3 +447,41 @@ func TestGenerateVMSocket(t *testing.T) {
 	assert.NotZero(vsock.ContextID)
 	assert.NotZero(vsock.Port)
 }
+
+func TestAssetPath(t *testing.T) {
+	assert := assert.New(t)
+
+	// Minimal config containing values for all asset annotation options.
+	// The values are "paths" (start with a slash), but end with the
+	// annotation name.
+	cfg := HypervisorConfig{
+		HypervisorPath:    "/" + "io.katacontainers.config.hypervisor.path",
+		HypervisorCtlPath: "/" + "io.katacontainers.config.hypervisor.ctlpath",
+
+		KernelPath: "/" + "io.katacontainers.config.hypervisor.kernel",
+
+		ImagePath:  "/" + "io.katacontainers.config.hypervisor.image",
+		InitrdPath: "/" + "io.katacontainers.config.hypervisor.initrd",
+
+		FirmwarePath: "/" + "io.katacontainers.config.hypervisor.firmware",
+		JailerPath:   "/" + "io.katacontainers.config.hypervisor.jailer_path",
+	}
+
+	for _, asset := range types.AssetTypes() {
+		msg := fmt.Sprintf("asset: %v", asset)
+
+		annoPath, annoHash, err := asset.Annotations()
+		assert.NoError(err, msg)
+
+		msg += fmt.Sprintf(", annotation path: %v, annotation hash: %v", annoPath, annoHash)
+
+		p, err := cfg.assetPath(asset)
+		assert.NoError(err, msg)
+
+		assert.NotEqual(p, annoPath, msg)
+		assert.NotEqual(p, annoHash, msg)
+
+		expected := fmt.Sprintf("/%s", annoPath)
+		assert.Equal(expected, p, msg)
+	}
+}

src/runtime/virtcontainers/pkg/annotations/annotations.go

@@ -42,6 +42,9 @@ const (
 	// HypervisorPath is a sandbox annotation for passing a per container path pointing at the hypervisor that will run the container VM.
 	HypervisorPath = kataAnnotHypervisorPrefix + "path"
 
+	// HypervisorCtlPath is a sandbox annotation for passing a per container path pointing at the hypervisor control binary that will run the container VM.
+	HypervisorCtlPath = kataAnnotHypervisorPrefix + "ctlpath"
+
 	// JailerPath is a sandbox annotation for passing a per container path pointing at the jailer that will constrain the container VM.
 	JailerPath = kataAnnotHypervisorPrefix + "jailer_path"
 
@@ -63,6 +66,9 @@ const (
 	// HypervisorHash is an sandbox annotation for passing a container hypervisor binary SHA-512 hash value.
 	HypervisorHash = kataAnnotHypervisorPrefix + "hypervisor_hash"
 
+	// HypervisorCtlHash is a sandbox annotation for passing a container hypervisor control binary SHA-512 hash value.
+	HypervisorCtlHash = kataAnnotHypervisorPrefix + "hypervisorctl_hash"
+
 	// JailerHash is an sandbox annotation for passing a jailer binary SHA-512 hash value.
 	JailerHash = kataAnnotHypervisorPrefix + "jailer_hash"
 

src/runtime/virtcontainers/pkg/oci/utils.go

@@ -218,6 +218,7 @@ func checkPathIsInGlobs(globs []string, path string) bool {
 			}
 		}
 	}
+
 	return false
 }
 
@@ -226,6 +227,7 @@ func checkAnnotationNameIsValid(list []string, name string, prefix string) bool
 	if strings.HasPrefix(name, prefix) {
 		return regexpContains(list, strings.TrimPrefix(name, prefix))
 	}
+
 	return true
 }
 
@@ -367,7 +369,12 @@ func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime Runtim
 			return fmt.Errorf("annotation %v is not enabled", key)
 		}
 	}
-	addAssetAnnotations(ocispec, config)
+
+	err := addAssetAnnotations(ocispec, config)
+	if err != nil {
+		return err
+	}
+
 	if err := addHypervisorConfigOverrides(ocispec, config, runtime); err != nil {
 		return err
 	}
@@ -382,17 +389,10 @@ func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime Runtim
 	return nil
 }
 
-func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) {
+func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error {
-	assetAnnotations := []string{
+	assetAnnotations, err := types.AssetAnnotations()
-		vcAnnotations.KernelPath,
+	if err != nil {
-		vcAnnotations.ImagePath,
+		return err
-		vcAnnotations.InitrdPath,
-		vcAnnotations.FirmwarePath,
-		vcAnnotations.KernelHash,
-		vcAnnotations.ImageHash,
-		vcAnnotations.InitrdHash,
-		vcAnnotations.FirmwareHash,
-		vcAnnotations.AssetHashType,
 	}
 
 	for _, a := range assetAnnotations {
@@ -401,6 +401,8 @@ func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) {
 			config.Annotations[a] = value
 		}
 	}
+
+	return nil
 }
 
 func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig, runtime RuntimeConfig) error {

src/runtime/virtcontainers/pkg/oci/utils_test.go

@@ -659,13 +659,38 @@ func TestMain(m *testing.M) {
 func TestAddAssetAnnotations(t *testing.T) {
 	assert := assert.New(t)
 
+	tmpdir, err := ioutil.TempDir("", "")
+	assert.NoError(err)
+	defer os.RemoveAll(tmpdir)
+
+	// Create a pretend asset file
+	// (required since the existence of binary asset annotations is verified).
+	fakeAssetFile := filepath.Join(tmpdir, "fake-binary")
+
+	err = ioutil.WriteFile(fakeAssetFile, []byte(""), fileMode)
+	assert.NoError(err)
+
 	expectedAnnotations := map[string]string{
-		vcAnnotations.KernelPath:    "/abc/rgb/kernel",
+		vcAnnotations.FirmwarePath: fakeAssetFile,
-		vcAnnotations.ImagePath:     "/abc/rgb/image",
+		vcAnnotations.FirmwareHash: "ffff",
-		vcAnnotations.InitrdPath:    "/abc/rgb/initrd",
+
-		vcAnnotations.KernelHash:    "3l2353we871g",
+		vcAnnotations.HypervisorPath: fakeAssetFile,
+		vcAnnotations.HypervisorHash: "bbbbb",
+
+		vcAnnotations.HypervisorCtlPath: fakeAssetFile,
+		vcAnnotations.HypervisorCtlHash: "cc",
+
+		vcAnnotations.ImagePath: fakeAssetFile,
 		vcAnnotations.ImageHash: "52ss2550983",
-		vcAnnotations.AssetHashType: "sha",
+
+		vcAnnotations.InitrdPath: fakeAssetFile,
+		vcAnnotations.InitrdHash: "aaaa",
+
+		vcAnnotations.JailerPath: fakeAssetFile,
+		vcAnnotations.JailerHash: "dddd",
+
+		vcAnnotations.KernelPath: fakeAssetFile,
+		vcAnnotations.KernelHash: "3l2353we871g",
 	}
 
 	config := vc.SandboxConfig{
@@ -682,17 +707,29 @@ func TestAddAssetAnnotations(t *testing.T) {
 	}
 
 	// Try annotations without enabling them first
-	err := addAnnotations(ocispec, &config, runtimeConfig)
+	err = addAnnotations(ocispec, &config, runtimeConfig)
+
 	assert.Error(err)
 	assert.Exactly(map[string]string{}, config.Annotations)
 
 	// Check if annotation not enabled correctly
 	runtimeConfig.HypervisorConfig.EnableAnnotations = []string{"nonexistent"}
 	err = addAnnotations(ocispec, &config, runtimeConfig)
+
 	assert.Error(err)
 
-	// Check that it works if all annotation are enabled
+	// Ensure it fails if all annotations enabled but path lists are not set
 	runtimeConfig.HypervisorConfig.EnableAnnotations = []string{".*"}
+	err = addAnnotations(ocispec, &config, runtimeConfig)
+	assert.Error(err)
+
+	tmpdirGlob := tmpdir + "/*"
+
+	// Check that it works if all path lists are enabled
+	runtimeConfig.HypervisorConfig.HypervisorPathList = []string{tmpdirGlob}
+	runtimeConfig.HypervisorConfig.JailerPathList = []string{tmpdirGlob}
+	runtimeConfig.HypervisorConfig.HypervisorCtlPathList = []string{tmpdirGlob}
+
 	err = addAnnotations(ocispec, &config, runtimeConfig)
 	assert.NoError(err)
 	assert.Exactly(expectedAnnotations, config.Annotations)

src/runtime/virtcontainers/sandbox.go

@@ -384,31 +384,24 @@ func createAssets(ctx context.Context, sandboxConfig *SandboxConfig) error {
 	span, _ := trace(ctx, "createAssets")
 	defer span.Finish()
 
-	kernel, err := types.NewAsset(sandboxConfig.Annotations, types.KernelAsset)
+	for _, name := range types.AssetTypes() {
+		a, err := types.NewAsset(sandboxConfig.Annotations, name)
 		if err != nil {
 			return err
 		}
 
-	image, err := types.NewAsset(sandboxConfig.Annotations, types.ImageAsset)
-	if err != nil {
-		return err
-	}
-
-	initrd, err := types.NewAsset(sandboxConfig.Annotations, types.InitrdAsset)
-	if err != nil {
-		return err
-	}
-
-	if image != nil && initrd != nil {
-		return fmt.Errorf("%s and %s cannot be both set", types.ImageAsset, types.InitrdAsset)
-	}
-
-	for _, a := range []*types.Asset{kernel, image, initrd} {
 		if err := sandboxConfig.HypervisorConfig.addCustomAsset(a); err != nil {
 			return err
 		}
 	}
 
+	_, imageErr := sandboxConfig.HypervisorConfig.assetPath(types.ImageAsset)
+	_, initrdErr := sandboxConfig.HypervisorConfig.assetPath(types.InitrdAsset)
+
+	if imageErr != nil && initrdErr != nil {
+		return fmt.Errorf("%s and %s cannot be both set", types.ImageAsset, types.InitrdAsset)
+	}
+
 	return nil
 }
 

src/runtime/virtcontainers/sandbox_test.go

@@ -13,6 +13,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"strings"
 	"sync"
 	"syscall"
 	"testing"
@@ -641,51 +642,127 @@ var assetContentWrongHash = "92549f8d2018a95a294d28a65e795ed7d1a9d150009a28cea10
 func TestSandboxCreateAssets(t *testing.T) {
 	assert := assert.New(t)
 
+	type testData struct {
+		assetType   types.AssetType
+		annotations map[string]string
+	}
+
 	tmpfile, err := ioutil.TempFile("", "virtcontainers-test-")
 	assert.Nil(err)
 
+	filename := tmpfile.Name()
+
 	defer func() {
 		tmpfile.Close()
-		os.Remove(tmpfile.Name()) // clean up
+		os.Remove(filename) // clean up
 	}()
 
 	_, err = tmpfile.Write(assetContent)
 	assert.Nil(err)
 
 	originalKernelPath := filepath.Join(testDir, testKernel)
+	originalImagePath := filepath.Join(testDir, testImage)
+	originalInitrdPath := filepath.Join(testDir, testInitrd)
+	originalFirmwarePath := filepath.Join(testDir, testFirmware)
+	originalHypervisorPath := filepath.Join(testDir, testHypervisor)
+	originalHypervisorCtlPath := filepath.Join(testDir, testHypervisorCtl)
+	originalJailerPath := filepath.Join(testDir, testJailer)
 
 	hc := HypervisorConfig{
 		KernelPath:        originalKernelPath,
-		ImagePath:  filepath.Join(testDir, testImage),
+		ImagePath:         originalImagePath,
+		InitrdPath:        originalInitrdPath,
+		FirmwarePath:      originalFirmwarePath,
+		HypervisorPath:    originalHypervisorPath,
+		HypervisorCtlPath: originalHypervisorCtlPath,
+		JailerPath:        originalJailerPath,
 	}
 
-	p := &SandboxConfig{
+	data := []testData{
-		Annotations: map[string]string{
+		{
-			annotations.KernelPath: tmpfile.Name(),
+			types.FirmwareAsset,
+			map[string]string{
+				annotations.FirmwarePath: filename,
+				annotations.FirmwareHash: assetContentHash,
+			},
+		},
+		{
+			types.HypervisorAsset,
+			map[string]string{
+				annotations.HypervisorPath: filename,
+				annotations.HypervisorHash: assetContentHash,
+			},
+		},
+		{
+			types.HypervisorCtlAsset,
+			map[string]string{
+				annotations.HypervisorCtlPath: filename,
+				annotations.HypervisorCtlHash: assetContentHash,
+			},
+		},
+		{
+			types.ImageAsset,
+			map[string]string{
+				annotations.ImagePath: filename,
+				annotations.ImageHash: assetContentHash,
+			},
+		},
+		{
+			types.InitrdAsset,
+			map[string]string{
+				annotations.InitrdPath: filename,
+				annotations.InitrdHash: assetContentHash,
+			},
+		},
+		{
+			types.JailerAsset,
+			map[string]string{
+				annotations.JailerPath: filename,
+				annotations.JailerHash: assetContentHash,
+			},
+		},
+		{
+			types.KernelAsset,
+			map[string]string{
+				annotations.KernelPath: filename,
 				annotations.KernelHash: assetContentHash,
 			},
-
-		HypervisorConfig: hc,
-	}
-
-	err = createAssets(context.Background(), p)
-	assert.Nil(err)
-
-	a, ok := p.HypervisorConfig.customAssets[types.KernelAsset]
-	assert.True(ok)
-	assert.Equal(a.Path(), tmpfile.Name())
-
-	p = &SandboxConfig{
-		Annotations: map[string]string{
-			annotations.KernelPath: tmpfile.Name(),
-			annotations.KernelHash: assetContentWrongHash,
 		},
+	}
 
+	for i, d := range data {
+		msg := fmt.Sprintf("test[%d]: %+v", i, d)
+
+		config := &SandboxConfig{
+			Annotations:      d.annotations,
 			HypervisorConfig: hc,
 		}
 
-	err = createAssets(context.Background(), p)
+		err = createAssets(context.Background(), config)
-	assert.NotNil(err)
+		assert.NoError(err, msg)
+
+		a, ok := config.HypervisorConfig.customAssets[d.assetType]
+		assert.True(ok, msg)
+		assert.Equal(a.Path(), filename, msg)
+
+		// Now test with invalid hashes
+		badHashAnnotations := make(map[string]string)
+		for k, v := range d.annotations {
+			if strings.HasSuffix(k, "_hash") {
+				badHashAnnotations[k] = assetContentWrongHash
+			} else {
+				badHashAnnotations[k] = v
+			}
+		}
+
+		config = &SandboxConfig{
+			Annotations:      badHashAnnotations,
+			HypervisorConfig: hc,
+		}
+
+		err = createAssets(context.Background(), config)
+		assert.Error(err, msg)
+	}
 }
 
 func testFindContainerFailure(t *testing.T, sandbox *Sandbox, cid string) {

src/runtime/virtcontainers/types/asset.go

@@ -18,6 +18,60 @@ import (
 // AssetType describe a type of assets.
 type AssetType string
 
+const (
+	// KernelAsset is a kernel asset.
+	KernelAsset AssetType = "kernel"
+
+	// ImageAsset is an image asset.
+	ImageAsset AssetType = "image"
+
+	// InitrdAsset is an initrd asset.
+	InitrdAsset AssetType = "initrd"
+
+	// HypervisorAsset is an hypervisor asset.
+	HypervisorAsset AssetType = "hypervisor"
+
+	// HypervisorCtlAsset is a hypervisor control asset.
+	HypervisorCtlAsset AssetType = "hypervisorctl"
+
+	// JailerAsset is a jailer asset.
+	JailerAsset AssetType = "jailer"
+
+	// FirmwareAsset is a firmware asset.
+	FirmwareAsset AssetType = "firmware"
+)
+
+// AssetTypes returns a list of all known asset types.
+//
+// XXX: New asset types *MUST* be added here.
+func AssetTypes() []AssetType {
+	return []AssetType{
+		FirmwareAsset,
+		HypervisorAsset,
+		HypervisorCtlAsset,
+		ImageAsset,
+		InitrdAsset,
+		JailerAsset,
+		KernelAsset,
+	}
+}
+
+// AssetAnnotations returns all annotations for all asset types.
+func AssetAnnotations() ([]string, error) {
+	var result []string
+
+	for _, at := range AssetTypes() {
+		aPath, aHash, err := at.Annotations()
+		if err != nil {
+			return []string{}, err
+		}
+
+		result = append(result, []string{aPath, aHash}...)
+	}
+
+	return result, nil
+}
+
 // Annotations returns the path and hash annotations for a given Asset type.
 func (t AssetType) Annotations() (string, string, error) {
 	switch t {
@@ -29,6 +83,8 @@ func (t AssetType) Annotations() (string, string, error) {
 		return annotations.InitrdPath, annotations.InitrdHash, nil
 	case HypervisorAsset:
 		return annotations.HypervisorPath, annotations.HypervisorHash, nil
+	case HypervisorCtlAsset:
+		return annotations.HypervisorCtlPath, annotations.HypervisorCtlHash, nil
 	case JailerAsset:
 		return annotations.JailerPath, annotations.JailerHash, nil
 	case FirmwareAsset:
@@ -38,28 +94,6 @@ func (t AssetType) Annotations() (string, string, error) {
 	return "", "", fmt.Errorf("Wrong asset type %s", t)
 }
 
-const (
-	// KernelAsset is a kernel asset.
-	KernelAsset AssetType = "kernel"
-
-	// ImageAsset is an image asset.
-	ImageAsset AssetType = "image"
-
-	// InitrdAsset is an intird asset.
-	InitrdAsset AssetType = "initrd"
-
-	// HypervisorAsset is an hypervisor asset.
-	HypervisorAsset AssetType = "hypervisor"
-
-	// HypervisorCtlAsset is an hypervisor control asset.
-	HypervisorCtlAsset AssetType = "hypervisorctl"
-	// JailerAsset is an jailer asset.
-	JailerAsset AssetType = "jailer"
-
-	// FirmwareAsset is a firmware asset.
-	FirmwareAsset AssetType = "firmware"
-)
-
 // Asset represents a virtcontainers asset.
 type Asset struct {
 	path         string
@@ -83,20 +117,11 @@ func (a *Asset) Valid() bool {
 		return false
 	}
 
-	switch a.kind {
+	for _, at := range AssetTypes() {
-	case KernelAsset:
+		if at == a.kind {
-		return true
-	case ImageAsset:
-		return true
-	case InitrdAsset:
-		return true
-	case HypervisorAsset:
-		return true
-	case JailerAsset:
-		return true
-	case FirmwareAsset:
 			return true
 		}
+	}
 
 	return false
 }

src/runtime/virtcontainers/types/asset_test.go

@@ -116,6 +116,7 @@ func TestAssetNew(t *testing.T) {
 		{annotations.ImagePath, annotations.ImageHash, ImageAsset, assetContentHash, false, false},
 		{annotations.InitrdPath, annotations.InitrdHash, InitrdAsset, assetContentHash, false, false},
 		{annotations.HypervisorPath, annotations.HypervisorHash, HypervisorAsset, assetContentHash, false, false},
+		{annotations.HypervisorCtlPath, annotations.HypervisorCtlHash, HypervisorCtlAsset, assetContentHash, false, false},
 		{annotations.JailerPath, annotations.JailerHash, JailerAsset, assetContentHash, false, false},
 		{annotations.FirmwarePath, annotations.FirmwareHash, FirmwareAsset, assetContentHash, false, false},
 
@@ -124,6 +125,7 @@ func TestAssetNew(t *testing.T) {
 		{annotations.ImagePath, annotations.ImageHash, ImageAsset, assetContentWrongHash, true, false},
 		{annotations.InitrdPath, annotations.InitrdHash, InitrdAsset, assetContentWrongHash, true, false},
 		{annotations.HypervisorPath, annotations.HypervisorHash, HypervisorAsset, assetContentWrongHash, true, false},
+		{annotations.HypervisorCtlPath, annotations.HypervisorCtlHash, HypervisorCtlAsset, assetContentWrongHash, true, false},
 		{annotations.JailerPath, annotations.JailerHash, JailerAsset, assetContentWrongHash, true, false},
 		{annotations.FirmwarePath, annotations.FirmwareHash, FirmwareAsset, assetContentWrongHash, true, false},
 

src/runtime/virtcontainers/virtcontainers_test.go

@@ -28,6 +28,8 @@ const testKernel = "kernel"
 const testInitrd = "initrd"
 const testImage = "image"
 const testHypervisor = "hypervisor"
+const testJailer = "jailer"
+const testFirmware = "firmware"
 const testVirtiofsd = "virtiofsd"
 const testHypervisorCtl = "hypervisorctl"
 const testBundle = "bundle"