From f912e0b5490769cf734734fe9757acb660b68754 Mon Sep 17 00:00:00 2001 From: Glyn Normington Date: Mon, 16 Sep 2019 12:18:26 +0100 Subject: [PATCH] Improve how relocation mappings are applied Also support a relocation mapping flag on the status command. Fixes https://github.com/deislabs/duffle/issues/858 Fixes https://github.com/deislabs/duffle/issues/859 --- cmd/duffle/install.go | 9 ++- cmd/duffle/main.go | 64 ++++++--------- cmd/duffle/main_test.go | 81 +++++++++++++++++++ cmd/duffle/run.go | 9 ++- cmd/duffle/status.go | 15 +++- .../testdata/oprelocator/badrelmap.json | 1 + cmd/duffle/testdata/oprelocator/relmap.json | 1 + cmd/duffle/uninstall.go | 9 ++- cmd/duffle/upgrade.go | 9 ++- 9 files changed, 145 insertions(+), 53 deletions(-) create mode 100644 cmd/duffle/testdata/oprelocator/badrelmap.json create mode 100644 cmd/duffle/testdata/oprelocator/relmap.json diff --git a/cmd/duffle/install.go b/cmd/duffle/install.go index 40b188ac..43054952 100644 --- a/cmd/duffle/install.go +++ b/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 diff --git a/cmd/duffle/main.go b/cmd/duffle/main.go index 3db37598..13ebd11b 100644 --- a/cmd/duffle/main.go +++ b/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) { diff --git a/cmd/duffle/main_test.go b/cmd/duffle/main_test.go index 09aa47f9..b5858a66 100644 --- a/cmd/duffle/main_test.go +++ b/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) + } +} diff --git a/cmd/duffle/run.go b/cmd/duffle/run.go index 0d07ef7b..25c79ec0 100644 --- a/cmd/duffle/run.go +++ b/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 diff --git a/cmd/duffle/status.go b/cmd/duffle/status.go index f5b21751..9128ce17 100644 --- a/cmd/duffle/status.go +++ b/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,7 +63,12 @@ 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 } @@ -70,11 +76,12 @@ reason, it may need the same credentials used to install. // 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 } diff --git a/cmd/duffle/testdata/oprelocator/badrelmap.json b/cmd/duffle/testdata/oprelocator/badrelmap.json new file mode 100644 index 00000000..81750b96 --- /dev/null +++ b/cmd/duffle/testdata/oprelocator/badrelmap.json @@ -0,0 +1 @@ +{ \ No newline at end of file diff --git a/cmd/duffle/testdata/oprelocator/relmap.json b/cmd/duffle/testdata/oprelocator/relmap.json new file mode 100644 index 00000000..46a54820 --- /dev/null +++ b/cmd/duffle/testdata/oprelocator/relmap.json @@ -0,0 +1 @@ +{"example.com/original":"example.com/relocated"} \ No newline at end of file diff --git a/cmd/duffle/uninstall.go b/cmd/duffle/uninstall.go index baa71457..0773a2ad 100644 --- a/cmd/duffle/uninstall.go +++ b/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) diff --git a/cmd/duffle/upgrade.go b/cmd/duffle/upgrade.go index f72ed926..ff8215a0 100644 --- a/cmd/duffle/upgrade.go +++ b/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)