annotations: Improve asset annotation handling

Make `asset.go` the arbiter of asset annotations by removing all asset
annotations lists from other parts of the codebase.

This makes the code simpler, easier to maintain, and more robust.

Specifically, the previous behaviour was inconsistent as the following
ways:

- `createAssets()` in `sandbox.go` was not handling the following asset
  annotations:

    - firmware:
      - `io.katacontainers.config.hypervisor.firmware`
      - `io.katacontainers.config.hypervisor.firmware_hash`

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

- `addAssetAnnotations()` in the `oci` package was not handling the
  following asset annotations:

    - hypervisor:
      - `io.katacontainers.config.hypervisor.path`
      - `io.katacontainers.config.hypervisor.hypervisor_hash`

    - hypervisor control binary:
      - `io.katacontainers.config.hypervisor.ctlpath`
      - `io.katacontainers.config.hypervisor.hypervisorctl_hash`

    - jailer:
      - `io.katacontainers.config.hypervisor.jailer_path`
      - `io.katacontainers.config.hypervisor.jailer_hash`

This change fixes the bug where specifying a custom hypervisor path via an
asset annotation was having no effect.

Fixes: #1085.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>

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/oci/utils.go

@@ -210,6 +210,7 @@ func checkPathIsInGlobs(globs []string, path string) bool {
 			}
 		}
 	}
+
 	return false
 }
 
@@ -218,6 +219,7 @@ func checkAnnotationNameIsValid(list []string, name string, prefix string) bool
 	if strings.HasPrefix(name, prefix) {
 		return regexpContains(list, strings.TrimPrefix(name, prefix))
 	}
+
 	return true
 }
 
@@ -359,7 +361,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
 	}
@@ -374,17 +381,10 @@ func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig, runtime Runtim
 	return nil
 }
 
-func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) {
-	assetAnnotations := []string{
-		vcAnnotations.KernelPath,
-		vcAnnotations.ImagePath,
-		vcAnnotations.InitrdPath,
-		vcAnnotations.FirmwarePath,
-		vcAnnotations.KernelHash,
-		vcAnnotations.ImageHash,
-		vcAnnotations.InitrdHash,
-		vcAnnotations.FirmwareHash,
-		vcAnnotations.AssetHashType,
+func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error {
+	assetAnnotations, err := types.AssetAnnotations()
+	if err != nil {
+		return err
 	}
 
 	for _, a := range assetAnnotations {
@@ -393,6 +393,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.ImagePath:     "/abc/rgb/image",
-		vcAnnotations.InitrdPath:    "/abc/rgb/initrd",
-		vcAnnotations.KernelHash:    "3l2353we871g",
-		vcAnnotations.ImageHash:     "52ss2550983",
-		vcAnnotations.AssetHashType: "sha",
+		vcAnnotations.FirmwarePath: fakeAssetFile,
+		vcAnnotations.FirmwareHash: "ffff",
+
+		vcAnnotations.HypervisorPath: fakeAssetFile,
+		vcAnnotations.HypervisorHash: "bbbbb",
+
+		vcAnnotations.HypervisorCtlPath: fakeAssetFile,
+		vcAnnotations.HypervisorCtlHash: "cc",
+
+		vcAnnotations.ImagePath: fakeAssetFile,
+		vcAnnotations.ImageHash: "52ss2550983",
+
+		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)
-	if err != nil {
-		return err
-	}
+	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),
+		KernelPath:        originalKernelPath,
+		ImagePath:         originalImagePath,
+		InitrdPath:        originalInitrdPath,
+		FirmwarePath:      originalFirmwarePath,
+		HypervisorPath:    originalHypervisorPath,
+		HypervisorCtlPath: originalHypervisorCtlPath,
+		JailerPath:        originalJailerPath,
 	}
 
-	p := &SandboxConfig{
-		Annotations: map[string]string{
-			annotations.KernelPath: tmpfile.Name(),
-			annotations.KernelHash: assetContentHash,
+	data := []testData{
+		{
+			types.FirmwareAsset,
+			map[string]string{
+				annotations.FirmwarePath: filename,
+				annotations.FirmwareHash: 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,
+		{
+			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.NotNil(err)
+	for i, d := range data {
+		msg := fmt.Sprintf("test[%d]: %+v", i, d)
+
+		config := &SandboxConfig{
+			Annotations:      d.annotations,
+			HypervisorConfig: hc,
+		}
+
+		err = createAssets(context.Background(), config)
+		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,28 +18,6 @@ import (
 // AssetType describe a type of assets.
 type AssetType string
 
-// Annotations returns the path and hash annotations for a given Asset type.
-func (t AssetType) Annotations() (string, string, error) {
-	switch t {
-	case KernelAsset:
-		return annotations.KernelPath, annotations.KernelHash, nil
-	case ImageAsset:
-		return annotations.ImagePath, annotations.ImageHash, nil
-	case InitrdAsset:
-		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:
-		return annotations.FirmwarePath, annotations.FirmwareHash, nil
-	}
-
-	return "", "", fmt.Errorf("Wrong asset type %s", t)
-}
-
 const (
 	// KernelAsset is a kernel asset.
 	KernelAsset AssetType = "kernel"
@@ -63,6 +41,59 @@ const (
 	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 {
+	case KernelAsset:
+		return annotations.KernelPath, annotations.KernelHash, nil
+	case ImageAsset:
+		return annotations.ImagePath, annotations.ImageHash, nil
+	case InitrdAsset:
+		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:
+		return annotations.FirmwarePath, annotations.FirmwareHash, nil
+	}
+
+	return "", "", fmt.Errorf("Wrong asset type %s", t)
+}
+
 // Asset represents a virtcontainers asset.
 type Asset struct {
 	path         string
@@ -86,21 +117,10 @@ func (a *Asset) Valid() bool {
 		return false
 	}
 
-	switch a.kind {
-	case KernelAsset:
-		return true
-	case ImageAsset:
-		return true
-	case InitrdAsset:
-		return true
-	case HypervisorAsset:
-		return true
-	case HypervisorCtlAsset:
-		return true
-	case JailerAsset:
-		return true
-	case FirmwareAsset:
-		return true
+	for _, at := range AssetTypes() {
+		if at == a.kind {
+			return true
+		}
 	}
 
 	return 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"