Improve how relocation mappings are applied (#862)

Also support a relocation mapping flag on the status command.

Fixes #858
Fixes #859

cmd/duffle/install.go

@@ -122,7 +122,7 @@ func (i *installCmd) run() error {
 		return err
 	}
 
-	driverImpl, err := prepareDriver(i.driver, i.relocationMapping)
+	driverImpl, err := prepareDriver(i.driver)
 	if err != nil {
 		return err
 	}
@@ -147,11 +147,16 @@ func (i *installCmd) run() error {
 		return err
 	}
 
+	opRelocator, err := makeOpRelocator(i.relocationMapping)
+	if err != nil {
+		return err
+	}
+
 	inst := &action.Install{
 		Driver: driverImpl,
 	}
 	fmt.Fprintf(i.out, "Executing install action...\n")
-	err = inst.Run(c, creds, setOut(i.out))
+	err = inst.Run(c, creds, setOut(i.out), opRelocator)
 
 	// Even if the action fails, we want to store a claim. This is because
 	// we cannot know, based on a failure, whether or not any resources were

cmd/duffle/main.go

@@ -9,6 +9,7 @@ import (
 	"runtime"
 	"strings"
 
+	"github.com/deislabs/cnab-go/action"
 	"github.com/deislabs/cnab-go/bundle"
 	"github.com/deislabs/cnab-go/bundle/loader"
 	"github.com/deislabs/cnab-go/claim"
@@ -111,7 +112,7 @@ func must(err error) {
 }
 
 // prepareDriver prepares a driver per the user's request.
-func prepareDriver(driverName string, relMap string) (driver.Driver, error) {
+func prepareDriver(driverName string) (driver.Driver, error) {
 	driverImpl, err := lookup.Lookup(driverName)
 	if err != nil {
 		return nil, err
@@ -126,55 +127,36 @@ func prepareDriver(driverName string, relMap string) (driver.Driver, error) {
 		configurable.SetConfig(driverCfg)
 	}
 
-	rm, err := loadRelMapping(relMap)
+	return driverImpl, nil
+}
+
+func makeOpRelocator(relMapping string) (action.OperationConfigFunc, error) {
+	rm, err := loadRelMapping(relMapping)
 	if err != nil {
 		return nil, err
 	}
 
-	// wrap the driver so any relocation mapping is mounted
-	return &driverWithRelocationMapping{
-		driver:     driverImpl,
-		relMapping: rm,
-	}, nil
-}
-
-type driverWithRelocationMapping struct {
-	driver     driver.Driver
-	relMapping string
-}
-
-func (d *driverWithRelocationMapping) Run(op *driver.Operation) (driver.OperationResult, error) {
-	// if there is a relocation mapping, ensure it is mounted and relocate the invocation image
-	if d.relMapping != "" {
-		op.Files["/cnab/app/relocation-mapping.json"] = d.relMapping
-
-		var err error
-		op.Image.Image, err = d.relocateImage(op.Image.Image)
+	relMap := make(map[string]string)
+	if rm != "" {
+		err := json.Unmarshal([]byte(rm), &relMap)
 		if err != nil {
-			return driver.OperationResult{}, err
+			return nil, fmt.Errorf("failed to unmarshal relocation mapping: %v", err)
 		}
 	}
 
-	return d.driver.Run(op)
-}
-
-func (d *driverWithRelocationMapping) Handles(it string) bool {
-	return d.driver.Handles(it)
-}
-
-func (d *driverWithRelocationMapping) relocateImage(im string) (string, error) {
-	relMap := make(map[string]string)
-	err := json.Unmarshal([]byte(d.relMapping), &relMap)
-	if err != nil {
-		return "", fmt.Errorf("failed to unmarshal relocation mapping: %v", err)
-	}
-
-	mapped, ok := relMap[im]
-	if !ok {
-		return "", fmt.Errorf("invocation image %s not present in relocation mapping %v", im, relMap)
-	}
+	return func(op *driver.Operation) error {
+		// if there is a relocation mapping, ensure it is mounted and relocate the invocation image
+		if rm != "" {
+			op.Files["/cnab/app/relocation-mapping.json"] = rm
 
-	return mapped, nil
+			im, ok := relMap[op.Image.Image]
+			if !ok {
+				return fmt.Errorf("invocation image %s not present in relocation mapping %v", op.Image.Image, relMap)
+			}
+			op.Image.Image = im
+		}
+		return nil
+	}, nil
 }
 
 func loadRelMapping(relMap string) (string, error) {

cmd/duffle/main_test.go

@@ -6,6 +6,8 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/deislabs/cnab-go/driver"
+
 	"github.com/deislabs/duffle/pkg/duffle/home"
 
 	"github.com/deislabs/cnab-go/bundle"
@@ -162,3 +164,82 @@ func TestFindCreds(t *testing.T) {
 		os.RemoveAll(filepath.Join(credDir, "*"))
 	}
 }
+
+func TestMakeOpRelocator(t *testing.T) {
+	is := assert.New(t)
+
+	tests := map[string]struct {
+		relMapFile                      string
+		invocationImage                 string
+		expectedErrorMessage            string
+		expectedInvocationImage         string
+		expectedOpRelocatorErrorMessage string
+	}{
+		"bad relocation mapping file": {
+			relMapFile:                      "no-such-file",
+			invocationImage:                 "example.com/original",
+			expectedErrorMessage:            "failed to read relocation mapping from no-such-file:",
+			expectedInvocationImage:         "",
+			expectedOpRelocatorErrorMessage: "",
+		},
+		"valid relocation mapping file": {
+			relMapFile:                      "testdata/oprelocator/relmap.json",
+			invocationImage:                 "example.com/original",
+			expectedErrorMessage:            "",
+			expectedInvocationImage:         "example.com/relocated",
+			expectedOpRelocatorErrorMessage: "",
+		},
+		"omitted relocation mapping file": {
+			relMapFile:                      "",
+			invocationImage:                 "example.com/original",
+			expectedErrorMessage:            "",
+			expectedInvocationImage:         "example.com/original",
+			expectedOpRelocatorErrorMessage: "",
+		},
+		"relocation mapping file with malformed contents": {
+			relMapFile:                      "testdata/oprelocator/badrelmap.json",
+			invocationImage:                 "example.com/original",
+			expectedErrorMessage:            "failed to unmarshal relocation mapping:",
+			expectedInvocationImage:         "example.com/original",
+			expectedOpRelocatorErrorMessage: "",
+		},
+		"invocation image not in relocation mapping": {
+			relMapFile:                      "testdata/oprelocator/relmap.json",
+			invocationImage:                 "example.com/other",
+			expectedErrorMessage:            "",
+			expectedInvocationImage:         "example.com/other",
+			expectedOpRelocatorErrorMessage: "invocation image example.com/other not present in relocation mapping map[",
+		},
+	}
+
+withNextTest:
+	for name, testCase := range tests {
+		opRelocator, err := makeOpRelocator(testCase.relMapFile)
+		if testCase.expectedErrorMessage != "" {
+			is.Contains(err.Error(), testCase.expectedErrorMessage, "Failed on test: "+name)
+			continue withNextTest
+		}
+		is.Nil(err, "Failed on test: "+name)
+
+		op := driver.Operation{
+			Files: make(map[string]string),
+			Image: bundle.InvocationImage{
+				BaseImage: bundle.BaseImage{
+					Image: testCase.invocationImage,
+				},
+			},
+		}
+		err = opRelocator(&op)
+		if testCase.expectedOpRelocatorErrorMessage != "" {
+			is.Contains(err.Error(), testCase.expectedOpRelocatorErrorMessage, "Failed on test: "+name)
+			continue withNextTest
+		}
+		is.Nil(err, "Failed on test: "+name)
+
+		is.Equal(testCase.expectedInvocationImage, op.Image.Image, "Failed on test: "+name)
+
+		// In the success case, a relocation mapping should be mounted if and only if a relocation mapping was specified
+		_, mounted := op.Files["/cnab/app/relocation-mapping.json"]
+		is.Equal(testCase.relMapFile != "", mounted, "Failed on test: "+name)
+	}
+}

cmd/duffle/run.go

@@ -61,7 +61,7 @@ Credentials and parameters may be passed to the bundle during a target action.
 				return err
 			}
 
-			driverImpl, err := prepareDriver(driver, relocationMapping)
+			driverImpl, err := prepareDriver(driver)
 			if err != nil {
 				return err
 			}
@@ -74,13 +74,18 @@ Credentials and parameters may be passed to the bundle during a target action.
 				}
 			}
 
+			opRelocator, err := makeOpRelocator(relocationMapping)
+			if err != nil {
+				return err
+			}
+
 			action := &action.RunCustom{
 				Driver: driverImpl,
 				Action: target,
 			}
 
 			fmt.Fprintf(w, "Executing custom action %q for release %q", target, claimName)
-			err = action.Run(&c, creds, setOut(cmd.OutOrStdout()))
+			err = action.Run(&c, creds, setOut(cmd.OutOrStdout()), opRelocator)
 			if actionDef := c.Bundle.Actions[target]; !actionDef.Modifies {
 				// Do not store a claim for non-mutating actions.
 				return err

cmd/duffle/status.go

@@ -21,8 +21,9 @@ action will restart the CNAB image and ask it to query for status. For that
 reason, it may need the same credentials used to install.
 `
 	var (
-		statusDriver     string
-		credentialsFiles []string
+		statusDriver      string
+		credentialsFiles  []string
+		relocationMapping string
 	)
 
 	cmd := &cobra.Command{
@@ -62,19 +63,25 @@ reason, it may need the same credentials used to install.
 				return err
 			}
 
-			driverImpl, err := prepareDriver(statusDriver, "")
+			driverImpl, err := prepareDriver(statusDriver)
+			if err != nil {
+				return err
+			}
+
+			opRelocator, err := makeOpRelocator(relocationMapping)
 			if err != nil {
 				return err
 			}
 
 			// TODO: Do we pass new values in here? Or just from Claim?
 			action := &action.Status{Driver: driverImpl}
 			fmt.Println("Executing status action in bundle...")
-			return action.Run(&c, creds, setOut(cmd.OutOrStdout()))
+			return action.Run(&c, creds, setOut(cmd.OutOrStdout()), opRelocator)
 		},
 	}
 	cmd.Flags().StringVarP(&statusDriver, "driver", "d", "docker", "Specify a driver name")
 	cmd.Flags().StringArrayVarP(&credentialsFiles, "credentials", "c", []string{}, "Specify credentials to use inside the CNAB bundle. This can be a credentialset name or a path to a file.")
+	cmd.Flags().StringVarP(&relocationMapping, "relocation-mapping", "m", "", "Path of relocation mapping JSON file")
 
 	return cmd
 }

cmd/duffle/testdata/oprelocator/badrelmap.json

@@ -0,0 +1 @@
+{

cmd/duffle/testdata/oprelocator/relmap.json

@@ -0,0 +1 @@
+{"example.com/original":"example.com/relocated"}

cmd/duffle/uninstall.go

@@ -96,7 +96,7 @@ func (un *uninstallCmd) run() error {
 		claim.Parameters = params
 	}
 
-	driverImpl, err := prepareDriver(un.driver, un.relocationMapping)
+	driverImpl, err := prepareDriver(un.driver)
 	if err != nil {
 		return fmt.Errorf("could not prepare driver: %s", err)
 	}
@@ -106,12 +106,17 @@ func (un *uninstallCmd) run() error {
 		return fmt.Errorf("could not load credentials: %s", err)
 	}
 
+	opRelocator, err := makeOpRelocator(un.relocationMapping)
+	if err != nil {
+		return err
+	}
+
 	uninst := &action.Uninstall{
 		Driver: driverImpl,
 	}
 
 	fmt.Fprintln(un.out, "Executing uninstall action...")
-	if err := uninst.Run(&claim, creds, setOut(un.out)); err != nil {
+	if err := uninst.Run(&claim, creds, setOut(un.out), opRelocator); err != nil {
 		return fmt.Errorf("could not uninstall %q: %s", un.name, err)
 	}
 	return claimStorage().Delete(un.name)

cmd/duffle/upgrade.go

@@ -105,7 +105,7 @@ func (up *upgradeCmd) run() error {
 		return err
 	}
 
-	driverImpl, err := prepareDriver(up.driver, up.relocationMapping)
+	driverImpl, err := prepareDriver(up.driver)
 	if err != nil {
 		return err
 	}
@@ -123,10 +123,15 @@ func (up *upgradeCmd) run() error {
 		}
 	}
 
+	opRelocator, err := makeOpRelocator(up.relocationMapping)
+	if err != nil {
+		return err
+	}
+
 	upgr := &action.Upgrade{
 		Driver: driverImpl,
 	}
-	err = upgr.Run(&claim, creds, setOut(up.out))
+	err = upgr.Run(&claim, creds, setOut(up.out), opRelocator)
 
 	// persist the claim, regardless of the success of the upgrade action
 	persistErr := claimStorage().Store(claim)