[FAB-5329] Able to instantiate on a taken chaincode ID

This fix appends a hash of the Docker image name to the image name to ensure a chaincode with different capitalization does not override the container of an already instantiated chaincode. This problem was introduced by the need to convert all Docker image names to lowercase. The Docker container name will retain its capitalization as that restriction only applies to the image name. Change-Id: I854f80b2f0e0269d9bbc60725f82b7e5a804b6fd Signed-off-by: Will Lahti <[email protected]>
hyperledger · Jul 26, 2017 · 36e5140 · 36e5140
1 parent d6b54c8
commit 36e5140
Show file tree

Hide file tree

Showing 5 changed files with 104 additions and 34 deletions.
diff --git a/core/container/api/core.go b/core/container/api/core.go
@@ -27,11 +27,11 @@ import (
 type BuildSpecFactory func() (io.Reader, error)
 type PrelaunchFunc func() error
 
-//abstract virtual image for supporting arbitrary virual machines
+//VM is an abstract virtual image for supporting arbitrary virual machines
 type VM interface {
 	Deploy(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, reader io.Reader) error
 	Start(ctxt context.Context, ccid ccintf.CCID, args []string, env []string, builder BuildSpecFactory, preLaunchFunc PrelaunchFunc) error
 	Stop(ctxt context.Context, ccid ccintf.CCID, timeout uint, dontkill bool, dontremove bool) error
 	Destroy(ctxt context.Context, ccid ccintf.CCID, force bool, noprune bool) error
-	GetVMName(ccID ccintf.CCID) (string, error)
+	GetVMName(ccID ccintf.CCID, format func(string) (string, error)) (string, error)
 }
diff --git a/core/container/controller.go b/core/container/controller.go
@@ -245,7 +245,7 @@ func VMCProcess(ctxt context.Context, vmtype string, req VMCReqIntf) (interface{
 	go func() {
 		defer close(c)
 
-		id, err := v.GetVMName(req.getCCID())
+		id, err := v.GetVMName(req.getCCID(), nil)
 		if err != nil {
 			resp = VMCResp{Err: err}
 			return

diff --git a/core/container/dockercontroller/dockercontroller.go b/core/container/dockercontroller/dockercontroller.go
@@ -18,6 +18,7 @@ package dockercontroller
 
 import (
 	"bytes"
+	"encoding/hex"
 	"fmt"
 	"io"
 	"strings"
@@ -29,6 +30,7 @@ import (
 
 	"github.com/fsouza/go-dockerclient"
 	"github.com/hyperledger/fabric/common/flogging"
+	"github.com/hyperledger/fabric/common/util"
 	container "github.com/hyperledger/fabric/core/container/api"
 	"github.com/hyperledger/fabric/core/container/ccintf"
 	cutil "github.com/hyperledger/fabric/core/container/util"
@@ -40,6 +42,8 @@ import (
 var (
 	dockerLogger = flogging.MustGetLogger("dockercontroller")
 	hostConfig   *docker.HostConfig
+	vmRegExp     = regexp.MustCompile("[^a-zA-Z0-9-_.]")
+	imageRegExp  = regexp.MustCompile("^[a-z0-9]+(([._-][a-z0-9]+)+)?$")
 )
 
 // getClient returns an instance that implements dockerClient interface
@@ -163,7 +167,7 @@ func (vm *DockerVM) createContainer(ctxt context.Context, client dockerClient,
 
 func (vm *DockerVM) deployImage(client dockerClient, ccid ccintf.CCID,
 	args []string, env []string, reader io.Reader) error {
-	id, err := vm.GetVMName(ccid)
+	id, err := vm.GetVMName(ccid, formatImageName)
 	if err != nil {
 		return err
 	}
@@ -208,7 +212,7 @@ func (vm *DockerVM) Deploy(ctxt context.Context, ccid ccintf.CCID,
 //Start starts a container using a previously created docker image
 func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID,
 	args []string, env []string, builder container.BuildSpecFactory, prelaunchFunc container.PrelaunchFunc) error {
-	imageID, err := vm.GetVMName(ccid)
+	imageID, err := vm.GetVMName(ccid, formatImageName)
 	if err != nil {
 		return err
 	}
@@ -219,7 +223,11 @@ func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID,
 		return err
 	}
 
-	containerID := strings.Replace(imageID, ":", "_", -1)
+	containerID, err := vm.GetVMName(ccid, nil)
+	if err != nil {
+		return err
+	}
+
 	attachStdout := viper.GetBool("vm.docker.attachStdout")
 
 	//stop,force remove if necessary
@@ -349,7 +357,7 @@ func (vm *DockerVM) Start(ctxt context.Context, ccid ccintf.CCID,
 
 //Stop stops a running chaincode
 func (vm *DockerVM) Stop(ctxt context.Context, ccid ccintf.CCID, timeout uint, dontkill bool, dontremove bool) error {
-	id, err := vm.GetVMName(ccid)
+	id, err := vm.GetVMName(ccid, nil)
 	if err != nil {
 		return err
 	}
@@ -395,7 +403,7 @@ func (vm *DockerVM) stopInternal(ctxt context.Context, client dockerClient,
 
 //Destroy destroys an image
 func (vm *DockerVM) Destroy(ctxt context.Context, ccid ccintf.CCID, force bool, noprune bool) error {
-	id, err := vm.GetVMName(ccid)
+	id, err := vm.GetVMName(ccid, formatImageName)
 	if err != nil {
 		return err
 	}
@@ -418,34 +426,48 @@ func (vm *DockerVM) Destroy(ctxt context.Context, ccid ccintf.CCID, force bool,
 	return err
 }
 
-//GetVMName generates the docker image from peer information given the hashcode. This is needed to
-//keep image name's unique in a single host, multi-peer environment (such as a development environment)
-func (vm *DockerVM) GetVMName(ccid ccintf.CCID) (string, error) {
+// GetVMName generates the VM name from peer information. It accepts a format
+// function parameter to allow different formatting based on the desired use of
+// the name.
+func (vm *DockerVM) GetVMName(ccid ccintf.CCID, format func(string) (string, error)) (string, error) {
 	name := ccid.GetName()
 
-	//Convert to lowercase and replace any invalid characters with "-"
-	r := regexp.MustCompile("[^a-zA-Z0-9-_.]")
-
+	// replace any invalid characters with "-"
 	if ccid.NetworkID != "" && ccid.PeerID != "" {
-		name = strings.ToLower(
-			r.ReplaceAllString(
-				fmt.Sprintf("%s-%s-%s", ccid.NetworkID, ccid.PeerID, name), "-"))
+		name = vmRegExp.ReplaceAllString(
+			fmt.Sprintf("%s-%s-%s", ccid.NetworkID, ccid.PeerID, name), "-")
 	} else if ccid.NetworkID != "" {
-		name = strings.ToLower(
-			r.ReplaceAllString(
-				fmt.Sprintf("%s-%s", ccid.NetworkID, name), "-"))
+		name = vmRegExp.ReplaceAllString(
+			fmt.Sprintf("%s-%s", ccid.NetworkID, name), "-")
 	} else if ccid.PeerID != "" {
-		name = strings.ToLower(
-			r.ReplaceAllString(
-				fmt.Sprintf("%s-%s", ccid.PeerID, name), "-"))
+		name = vmRegExp.ReplaceAllString(
+			fmt.Sprintf("%s-%s", ccid.PeerID, name), "-")
 	}
 
-	// Check name complies with Docker's repository naming rules
-	r = regexp.MustCompile("^[a-z0-9]+(([._-][a-z0-9]+)+)?$")
+	if format != nil {
+		formattedName, err := format(name)
+		if err != nil {
+			return formattedName, err
+		}
+		// check to ensure format function didn't add any invalid characters
+		name = vmRegExp.ReplaceAllString(formattedName, "-")
+	}
+	return name, nil
+}
 
-	if !r.MatchString(name) {
+// formatImageName formats the docker image from peer information. This is
+// needed to keep image (repository) names unique in a single host, multi-peer
+// environment (such as a development environment). It computes the hash for the
+// supplied image name and then appends it to the lowercase image name to ensure
+// uniqueness.
+func formatImageName(name string) (string, error) {
+	imageName := strings.ToLower(fmt.Sprintf("%s-%s", name, hex.EncodeToString(util.ComputeSHA256([]byte(name)))))
+
+	// Check that name complies with Docker's repository naming rules
+	if !imageRegExp.MatchString(imageName) {
 		dockerLogger.Errorf("Error constructing Docker VM Name. '%s' breaks Docker's repository naming rules", name)
-		return name, fmt.Errorf("Error constructing Docker VM Name. '%s' breaks Docker's repository naming rules", name)
+		return imageName, fmt.Errorf("Error constructing Docker VM Name. '%s' breaks Docker's repository naming rules", imageName)
 	}
-	return name, nil
+
+	return imageName, nil
 }
diff --git a/core/container/dockercontroller/dockercontroller_test.go b/core/container/dockercontroller/dockercontroller_test.go
@@ -21,6 +21,7 @@ import (
 	"bytes"
 	"compress/gzip"
 	"context"
+	"encoding/hex"
 	"errors"
 	"fmt"
 	"io"
@@ -226,6 +227,39 @@ func Test_Destroy(t *testing.T) {
 	testerr(t, err, true)
 }
 
+type testCase struct {
+	name           string
+	ccid           ccintf.CCID
+	formatFunc     func(string) (string, error)
+	expectedOutput string
+}
+
+func TestGetVMName(t *testing.T) {
+	dvm := DockerVM{}
+	var tc []testCase
+
+	tc = append(tc,
+		testCase{"mycc", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "mycc"}}, NetworkID: "dev", PeerID: "peer0", Version: "1.0"}, formatImageName, fmt.Sprintf("%s-%s", "dev-peer0-mycc-1.0", hex.EncodeToString(util.ComputeSHA256([]byte("dev-peer0-mycc-1.0"))))},
+		testCase{"mycc-nonetworkid", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "mycc"}}, PeerID: "peer1", Version: "1.0"}, formatImageName, fmt.Sprintf("%s-%s", "peer1-mycc-1.0", hex.EncodeToString(util.ComputeSHA256([]byte("peer1-mycc-1.0"))))},
+		testCase{"myCC", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "myCC"}}, NetworkID: "Dev", PeerID: "Peer0", Version: "1.0"}, formatImageName, fmt.Sprintf("%s-%s", "dev-peer0-mycc-1.0", hex.EncodeToString(util.ComputeSHA256([]byte("Dev-Peer0-myCC-1.0"))))},
+		testCase{"mycc-nopeerid", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "mycc"}}, NetworkID: "dev", Version: "1.0"}, formatImageName, fmt.Sprintf("%s-%s", "dev-mycc-1.0", hex.EncodeToString(util.ComputeSHA256([]byte("dev-mycc-1.0"))))},
+		testCase{"myCC", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "myCC"}}, NetworkID: "dev", PeerID: "peer0", Version: "1.0"}, formatImageName, fmt.Sprintf("%s-%s", "dev-peer0-mycc-1.0", hex.EncodeToString(util.ComputeSHA256([]byte("dev-peer0-myCC-1.0"))))},
+		testCase{"myCC-preserveCase", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "myCC"}}, NetworkID: "Dev", PeerID: "Peer0", Version: "1.0"}, nil, fmt.Sprintf("%s", "Dev-Peer0-myCC-1.0")},
+		testCase{"invalidCharsFormatFunction", ccintf.CCID{ChaincodeSpec: &pb.ChaincodeSpec{ChaincodeId: &pb.ChaincodeID{Name: "myCC"}}, NetworkID: "Dev", PeerID: "Peer0", Version: "1.0"}, formatInvalidChars, fmt.Sprintf("%s", "inv-lid-character--")})
+
+	for _, test := range tc {
+		name, err := dvm.GetVMName(test.ccid, test.formatFunc)
+		assert.Nil(t, err, "Expected nil error")
+		assert.Equal(t, test.expectedOutput, name, "Unexpected output for test case name: %s", test.name)
+	}
+
+}
+
+func TestFormatImageName_invalidChars(t *testing.T) {
+	_, err := formatImageName("invalid*chars")
+	assert.NotNil(t, err, "Expected error")
+}
+
 func getCodeChainBytesInMem() io.Reader {
 	startTime := time.Now()
 	inputbuf := bytes.NewBuffer(nil)
@@ -322,3 +356,7 @@ func (c *mockClient) RemoveContainer(opts docker.RemoveContainerOptions) error {
 	}
 	return nil
 }
+
+func formatInvalidChars(name string) (string, error) {
+	return "inv@lid*character$/", nil
+}
diff --git a/core/container/inproccontroller/inproccontroller.go b/core/container/inproccontroller/inproccontroller.go
@@ -93,7 +93,7 @@ func (vm *InprocVM) Deploy(ctxt context.Context, ccid ccintf.CCID, args []string
 		return fmt.Errorf(fmt.Sprintf("%s system chaincode does not contain chaincode instance", path))
 	}
 
-	instName, _ := vm.GetVMName(ccid)
+	instName, _ := vm.GetVMName(ccid, nil)
 	_, err := vm.getInstance(ctxt, ipctemplate, instName, args, env)
 
 	//FUTURE ... here is where we might check code for safety
@@ -163,7 +163,7 @@ func (vm *InprocVM) Start(ctxt context.Context, ccid ccintf.CCID, args []string,
 		return fmt.Errorf(fmt.Sprintf("%s not registered", path))
 	}
 
-	instName, _ := vm.GetVMName(ccid)
+	instName, _ := vm.GetVMName(ccid, nil)
 
 	ipc, err := vm.getInstance(ctxt, ipctemplate, instName, args, env)
 
@@ -211,7 +211,7 @@ func (vm *InprocVM) Stop(ctxt context.Context, ccid ccintf.CCID, timeout uint, d
 		return fmt.Errorf("%s not registered", path)
 	}
 
-	instName, _ := vm.GetVMName(ccid)
+	instName, _ := vm.GetVMName(ccid, nil)
 
 	ipc := instRegistry[instName]
 
@@ -236,7 +236,17 @@ func (vm *InprocVM) Destroy(ctxt context.Context, ccid ccintf.CCID, force bool,
 	return nil
 }
 
-//GetVMName ignores the peer and network name as it just needs to be unique in process
-func (vm *InprocVM) GetVMName(ccid ccintf.CCID) (string, error) {
-	return ccid.GetName(), nil
+// GetVMName ignores the peer and network name as it just needs to be unique in
+// process.  It accepts a format function parameter to allow different
+// formatting based on the desired use of the name.
+func (vm *InprocVM) GetVMName(ccid ccintf.CCID, format func(string) (string, error)) (string, error) {
+	name := ccid.GetName()
+	if format != nil {
+		formattedName, err := format(name)
+		if err != nil {
+			return formattedName, err
+		}
+		name = formattedName
+	}
+	return name, nil
 }