From 04dc0d93d83a56f08eb3f4d63951fdb443bb8a2f Mon Sep 17 00:00:00 2001 From: "James O. D. Hunt" Date: Thu, 29 Oct 2020 09:51:13 +0000 Subject: [PATCH] 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: #3030. Signed-off-by: James O. D. Hunt (cherry picked from commit 6a5eb0ded542e4f10b88fc5a95b7c9d02959d84b) --- virtcontainers/hypervisor_test.go | 38 ++++++++ virtcontainers/pkg/oci/utils.go | 23 +++-- virtcontainers/pkg/oci/utils_test.go | 26 ++++-- virtcontainers/sandbox.go | 29 +++--- virtcontainers/sandbox_test.go | 122 +++++++++++++++++++++----- virtcontainers/types/asset.go | 94 ++++++++++++-------- virtcontainers/virtcontainers_test.go | 2 + 7 files changed, 238 insertions(+), 96 deletions(-) diff --git a/virtcontainers/hypervisor_test.go b/virtcontainers/hypervisor_test.go index 6f91287aa1..e4ce65df48 100644 --- a/virtcontainers/hypervisor_test.go +++ b/virtcontainers/hypervisor_test.go @@ -456,3 +456,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) + } +} diff --git a/virtcontainers/pkg/oci/utils.go b/virtcontainers/pkg/oci/utils.go index 9701df3d55..527c483c3f 100644 --- a/virtcontainers/pkg/oci/utils.go +++ b/virtcontainers/pkg/oci/utils.go @@ -335,7 +335,11 @@ func SandboxID(spec specs.Spec) (string, error) { } func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error { - addAssetAnnotations(ocispec, config) + err := addAssetAnnotations(ocispec, config) + if err != nil { + return err + } + if err := addHypervisorConfigOverrides(ocispec, config); err != nil { return err } @@ -350,17 +354,10 @@ func addAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) error { 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 { @@ -371,6 +368,8 @@ func addAssetAnnotations(ocispec specs.Spec, config *vc.SandboxConfig) { config.Annotations[a] = value } + + return nil } func addHypervisorConfigOverrides(ocispec specs.Spec, config *vc.SandboxConfig) error { diff --git a/virtcontainers/pkg/oci/utils_test.go b/virtcontainers/pkg/oci/utils_test.go index 1a90e8a145..851abfba7c 100644 --- a/virtcontainers/pkg/oci/utils_test.go +++ b/virtcontainers/pkg/oci/utils_test.go @@ -665,12 +665,26 @@ func TestAddAssetAnnotations(t *testing.T) { assert := assert.New(t) 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: "/some/where", + vcAnnotations.FirmwareHash: "ffff", + + vcAnnotations.HypervisorPath: "/some/where", + vcAnnotations.HypervisorHash: "bbbbb", + + vcAnnotations.HypervisorCtlPath: "/some/where/else", + vcAnnotations.HypervisorCtlHash: "cc", + + vcAnnotations.ImagePath: "/abc/rgb/image", + vcAnnotations.ImageHash: "52ss2550983", + + vcAnnotations.InitrdPath: "/abc/rgb/initrd", + vcAnnotations.InitrdHash: "aaaa", + + vcAnnotations.JailerPath: "/foo/bar", + vcAnnotations.JailerHash: "dddd", + + vcAnnotations.KernelPath: "/abc/rgb/kernel", + vcAnnotations.KernelHash: "3l2353we871g", } config := vc.SandboxConfig{ diff --git a/virtcontainers/sandbox.go b/virtcontainers/sandbox.go index a7420529fe..908e753ef6 100644 --- a/virtcontainers/sandbox.go +++ b/virtcontainers/sandbox.go @@ -416,31 +416,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 + if err := sandboxConfig.HypervisorConfig.addCustomAsset(a); err != nil { + return err + } } - initrd, err := types.NewAsset(sandboxConfig.Annotations, types.InitrdAsset) - if err != nil { - return err - } + _, imageErr := sandboxConfig.HypervisorConfig.assetPath(types.ImageAsset) + _, initrdErr := sandboxConfig.HypervisorConfig.assetPath(types.InitrdAsset) - if image != nil && initrd != nil { + if imageErr != nil && initrdErr != 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 - } - } - return nil } diff --git a/virtcontainers/sandbox_test.go b/virtcontainers/sandbox_test.go index 9f9d50ac1c..47f3b0d4a3 100644 --- a/virtcontainers/sandbox_test.go +++ b/virtcontainers/sandbox_test.go @@ -843,51 +843,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, + }, + }, + { + 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) + for i, d := range data { + msg := fmt.Sprintf("test[%d]: %+v", i, d) - a, ok := p.HypervisorConfig.customAssets[types.KernelAsset] - assert.True(ok) - assert.Equal(a.Path(), tmpfile.Name()) + config := &SandboxConfig{ + Annotations: d.annotations, + HypervisorConfig: hc, + } - p = &SandboxConfig{ - Annotations: map[string]string{ - annotations.KernelPath: tmpfile.Name(), - annotations.KernelHash: assetContentWrongHash, - }, + err = createAssets(context.Background(), config) + assert.NoError(err, msg) - HypervisorConfig: hc, - } + 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(), p) - assert.NotNil(err) + err = createAssets(context.Background(), config) + assert.Error(err, msg) + } } func testFindContainerFailure(t *testing.T, sandbox *Sandbox, cid string) { diff --git a/virtcontainers/types/asset.go b/virtcontainers/types/asset.go index 6f6241e3f0..b95b19aa65 100644 --- a/virtcontainers/types/asset.go +++ b/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 diff --git a/virtcontainers/virtcontainers_test.go b/virtcontainers/virtcontainers_test.go index 3d10476210..08eef22468 100644 --- a/virtcontainers/virtcontainers_test.go +++ b/virtcontainers/virtcontainers_test.go @@ -29,6 +29,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"