FAB-17170 externalbuilder pass metadata unmangled

Today, the external builder receives a structure representing the three
fields of the metadata necessary to invoke the old docker platform
builds.  It then takes these fields and reserializes them to the JSON
metadata document.

In fact, the externalbuilder has no need for these deserialized fields,
and should pass the JSON document through without modifying it or losing
any additional fields.  This CR causes the package retrieving code to
return both the JSON document and the underlying bytes.  The underlying
bytes are passed to the externalbuilder while the three field structure
is passed to the dockerbuilder.

These paths are really quite bifurcated and could probably be further
split, with different package retrieving calls, it didn't seem worth it
purely to address this bug.

Signed-off-by: Jason Yellick <[email protected]>

core/chaincode/chaincode_support_test.go

@@ -188,7 +188,7 @@ func initMockPeer(channelIDs ...string) (*peer.Peer, *ChaincodeSupport, func(),
 	}
 
 	containerRouter := &container.Router{
-		DockerVM: &dockercontroller.DockerVM{
+		DockerBuilder: &dockercontroller.DockerVM{
 			PlatformBuilder: &platforms.Builder{
 				Registry: platforms.NewRegistry(&golang.Platform{}),
 				Client:   client,

core/chaincode/exectransaction_test.go

@@ -118,7 +118,7 @@ func initPeer(channelIDs ...string) (*cm.Lifecycle, net.Listener, *ChaincodeSupp
 	}
 
 	containerRouter := &container.Router{
-		DockerVM: &dockercontroller.DockerVM{
+		DockerBuilder: &dockercontroller.DockerVM{
 			PeerID:       "",
 			NetworkID:    "",
 			BuildMetrics: dockercontroller.NewBuildMetrics(&disabled.Provider{}),

core/chaincode/persistence/chaincode_package.go

@@ -51,31 +51,45 @@ type FallbackPackageLocator struct {
 	LegacyCCPackageLocator  LegacyCCPackageLocator
 }
 
-func (fpl *FallbackPackageLocator) GetChaincodePackage(packageID string) (*ChaincodePackageMetadata, io.ReadCloser, error) {
+func (fpl *FallbackPackageLocator) GetChaincodePackage(packageID string) (*ChaincodePackageMetadata, []byte, io.ReadCloser, error) {
+	// XXX, this path has too many return parameters.  We could split it into two calls,
+	// or, we could deserialize the metadata where it's needed.  But, as written was the
+	// fastest path to fixing a bug around the mutation of metadata.
 	streamer := fpl.ChaincodePackageLocator.ChaincodePackageStreamer(packageID)
 	if streamer.Exists() {
 		metadata, err := streamer.Metadata()
 		if err != nil {
-			return nil, nil, errors.WithMessagef(err, "error retrieving chaincode package metadata '%s'", packageID)
+			return nil, nil, nil, errors.WithMessagef(err, "error retrieving chaincode package metadata '%s'", packageID)
+		}
+
+		mdBytes, err := streamer.MetadataBytes()
+		if err != nil {
+			return nil, nil, nil, errors.WithMessagef(err, "error retrieving chaincode package metadata bytes '%s'", packageID)
 		}
 
 		tarStream, err := streamer.Code()
 		if err != nil {
-			return nil, nil, errors.WithMessagef(err, "error retrieving chaincode package code '%s'", packageID)
+			return nil, nil, nil, errors.WithMessagef(err, "error retrieving chaincode package code '%s'", packageID)
 		}
 
-		return metadata, tarStream, nil
+		return metadata, mdBytes, tarStream, nil
 	}
 
 	cds, err := fpl.LegacyCCPackageLocator.GetChaincodeDepSpec(string(packageID))
 	if err != nil {
-		return nil, nil, errors.WithMessagef(err, "could not get legacy chaincode package '%s'", packageID)
+		return nil, nil, nil, errors.WithMessagef(err, "could not get legacy chaincode package '%s'", packageID)
+	}
+
+	md := &ChaincodePackageMetadata{
+		Path:  cds.ChaincodeSpec.ChaincodeId.Path,
+		Type:  cds.ChaincodeSpec.Type.String(),
+		Label: cds.ChaincodeSpec.ChaincodeId.Name,
 	}
 
-	return &ChaincodePackageMetadata{
-			Path: cds.ChaincodeSpec.ChaincodeId.Path,
-			Type: cds.ChaincodeSpec.Type.String(),
-		},
+	mdBytes, err := json.Marshal(md)
+
+	return md,
+		mdBytes,
 		ioutil.NopCloser(bytes.NewBuffer(cds.CodePackage)),
 		nil
 }
@@ -116,6 +130,22 @@ func (cps *ChaincodePackageStreamer) Metadata() (*ChaincodePackageMetadata, erro
 	return metadata, nil
 }
 
+func (cps *ChaincodePackageStreamer) MetadataBytes() ([]byte, error) {
+	tarFileStream, err := cps.File(MetadataFile)
+	if err != nil {
+		return nil, errors.WithMessage(err, "could not get metadata file")
+	}
+
+	defer tarFileStream.Close()
+
+	md, err := ioutil.ReadAll(tarFileStream)
+	if err != nil {
+		return nil, errors.WithMessage(err, "could read metadata file")
+	}
+
+	return md, nil
+}
+
 func (cps *ChaincodePackageStreamer) Code() (*TarFileStream, error) {
 	tarFileStream, err := cps.File(CodePackageFile)
 	if err != nil {

core/chaincode/persistence/chaincode_package_test.go

@@ -38,14 +38,15 @@ var _ = Describe("FallbackPackageLocator", func() {
 
 	Describe("GetChaincodePackage", func() {
 		It("gets the chaincode package metadata and stream", func() {
-			md, stream, err := fpl.GetChaincodePackage("good-package")
+			md, mdBytes, stream, err := fpl.GetChaincodePackage("good-package")
 			Expect(err).NotTo(HaveOccurred())
 			defer stream.Close()
 			Expect(md).To(Equal(&persistence.ChaincodePackageMetadata{
 				Type:  "Fake-Type",
 				Path:  "Fake-Path",
 				Label: "Real-Label",
 			}))
+			Expect(mdBytes).To(MatchJSON(`{"type":"Fake-Type","path":"Fake-Path","label":"Real-Label","extra_field":"extra-field-value"}`))
 			code, err := ioutil.ReadAll(stream)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(code).To(Equal([]byte("package")))
@@ -54,14 +55,14 @@ var _ = Describe("FallbackPackageLocator", func() {
 
 		Context("when the package has bad metadata", func() {
 			It("wraps and returns the error", func() {
-				_, _, err := fpl.GetChaincodePackage("bad-metadata")
+				_, _, _, err := fpl.GetChaincodePackage("bad-metadata")
 				Expect(err).To(MatchError(ContainSubstring("error retrieving chaincode package metadata 'bad-metadata'")))
 			})
 		})
 
 		Context("when the package has bad code", func() {
 			It("wraps and returns the error", func() {
-				_, _, err := fpl.GetChaincodePackage("missing-codepackage")
+				_, _, _, err := fpl.GetChaincodePackage("missing-codepackage")
 				Expect(err).To(MatchError(ContainSubstring("error retrieving chaincode package code 'missing-codepackage'")))
 			})
 		})
@@ -82,13 +83,14 @@ var _ = Describe("FallbackPackageLocator", func() {
 			})
 
 			It("falls back to the legacy retriever", func() {
-				md, stream, err := fpl.GetChaincodePackage("legacy-package")
+				md, mdBytes, stream, err := fpl.GetChaincodePackage("legacy-package")
 				Expect(err).NotTo(HaveOccurred())
 				defer stream.Close()
 				Expect(md).To(Equal(&persistence.ChaincodePackageMetadata{
 					Path: "legacy-path",
 					Type: "GOLANG",
 				}))
+				Expect(mdBytes).To(MatchJSON(`{"type":"GOLANG","path":"legacy-path","label":""}`))
 				code, err := ioutil.ReadAll(stream)
 				Expect(err).NotTo(HaveOccurred())
 				Expect(code).To(Equal([]byte("legacy-code")))
@@ -100,7 +102,7 @@ var _ = Describe("FallbackPackageLocator", func() {
 				})
 
 				It("wraps and returns the error", func() {
-					_, _, err := fpl.GetChaincodePackage("legacy-package")
+					_, _, _, err := fpl.GetChaincodePackage("legacy-package")
 					Expect(err).To(MatchError("could not get legacy chaincode package 'legacy-package': fake-error"))
 				})
 			})

core/container/container.go

@@ -19,13 +19,20 @@ import (
 
 var vmLogger = flogging.MustGetLogger("container")
 
-//go:generate counterfeiter -o mock/vm.go --fake-name VM . VM
+//go:generate counterfeiter -o mock/docker_builder.go --fake-name DockerBuilder . DockerBuilder
 
-//VM is an abstract virtual image for supporting arbitrary virual machines
-type VM interface {
+// DockerBuilder is what is exposed by the dockercontroller
+type DockerBuilder interface {
 	Build(ccid string, metadata *persistence.ChaincodePackageMetadata, codePackageStream io.Reader) (Instance, error)
 }
 
+//go:generate counterfeiter -o mock/external_builder.go --fake-name ExternalBuilder . ExternalBuilder
+
+// ExternalBuilder is what is exposed by the dockercontroller
+type ExternalBuilder interface {
+	Build(ccid string, metadata []byte, codePackageStream io.Reader) (Instance, error)
+}
+
 //go:generate counterfeiter -o mock/instance.go --fake-name Instance . Instance
 
 // Instance represents a built chaincode instance, because of the docker legacy, calling this a
@@ -60,12 +67,12 @@ func (UninitializedInstance) Wait() (int, error) {
 
 // PackageProvider gets chaincode packages from the filesystem.
 type PackageProvider interface {
-	GetChaincodePackage(packageID string) (*persistence.ChaincodePackageMetadata, io.ReadCloser, error)
+	GetChaincodePackage(packageID string) (md *persistence.ChaincodePackageMetadata, mdBytes []byte, codeStream io.ReadCloser, err error)
 }
 
 type Router struct {
-	ExternalVM      VM
-	DockerVM        VM
+	ExternalBuilder ExternalBuilder
+	DockerBuilder   DockerBuilder
 	containers      map[string]Instance
 	PackageProvider PackageProvider
 	mutex           sync.Mutex
@@ -89,29 +96,29 @@ func (r *Router) getInstance(ccid string) Instance {
 func (r *Router) Build(ccid string) error {
 	var instance Instance
 
-	if r.ExternalVM != nil {
+	if r.ExternalBuilder != nil {
 		// for now, the package ID we retrieve from the FS is always the ccid
 		// the chaincode uses for registration
-		metadata, codeStream, err := r.PackageProvider.GetChaincodePackage(ccid)
+		_, mdBytes, codeStream, err := r.PackageProvider.GetChaincodePackage(ccid)
 		if err != nil {
 			return errors.WithMessage(err, "failed to get chaincode package for external build")
 		}
 		defer codeStream.Close()
 
-		instance, err = r.ExternalVM.Build(ccid, metadata, codeStream)
+		instance, err = r.ExternalBuilder.Build(ccid, mdBytes, codeStream)
 		if err != nil {
 			return errors.WithMessage(err, "external builder failed")
 		}
 	}
 
 	if instance == nil {
-		metadata, codeStream, err := r.PackageProvider.GetChaincodePackage(ccid)
+		metadata, _, codeStream, err := r.PackageProvider.GetChaincodePackage(ccid)
 		if err != nil {
 			return errors.WithMessage(err, "failed to get chaincode package for docker build")
 		}
 		defer codeStream.Close()
 
-		instance, err = r.DockerVM.Build(ccid, metadata, codeStream)
+		instance, err = r.DockerBuilder.Build(ccid, metadata, codeStream)
 		if err != nil {
 			return errors.WithMessage(err, "docker build failed")
 		}

core/container/container_test.go

@@ -22,58 +22,56 @@ import (
 
 var _ = Describe("Router", func() {
 	var (
-		fakeDockerVM        *mock.VM
-		fakeExternalVM      *mock.VM
+		fakeDockerBuilder   *mock.DockerBuilder
+		fakeExternalBuilder *mock.ExternalBuilder
 		fakePackageProvider *mock.PackageProvider
 		fakeInstance        *mock.Instance
 		router              *container.Router
 	)
 
 	BeforeEach(func() {
-		fakeDockerVM = &mock.VM{}
-		fakeExternalVM = &mock.VM{}
+		fakeDockerBuilder = &mock.DockerBuilder{}
+		fakeExternalBuilder = &mock.ExternalBuilder{}
 		fakeInstance = &mock.Instance{}
 		fakePackageProvider = &mock.PackageProvider{}
 		fakePackageProvider.GetChaincodePackageReturns(
 			&persistence.ChaincodePackageMetadata{
 				Type: "package-type",
 				Path: "package-path",
 			},
+			[]byte(`{"some":"json"}`),
 			ioutil.NopCloser(bytes.NewBuffer([]byte("code-bytes"))),
 			nil,
 		)
 
 		router = &container.Router{
-			DockerVM:        fakeDockerVM,
-			ExternalVM:      fakeExternalVM,
+			DockerBuilder:   fakeDockerBuilder,
+			ExternalBuilder: fakeExternalBuilder,
 			PackageProvider: fakePackageProvider,
 		}
 	})
 
 	Describe("Build", func() {
 		BeforeEach(func() {
-			fakeExternalVM.BuildReturns(fakeInstance, nil)
+			fakeExternalBuilder.BuildReturns(fakeInstance, nil)
 		})
 
 		It("calls the external builder with the correct args", func() {
 			err := router.Build("package-id")
 			Expect(err).NotTo(HaveOccurred())
 
-			Expect(fakeExternalVM.BuildCallCount()).To(Equal(1))
-			ccid, md, codeStream := fakeExternalVM.BuildArgsForCall(0)
+			Expect(fakeExternalBuilder.BuildCallCount()).To(Equal(1))
+			ccid, md, codeStream := fakeExternalBuilder.BuildArgsForCall(0)
 			Expect(ccid).To(Equal("package-id"))
-			Expect(md).To(Equal(&persistence.ChaincodePackageMetadata{
-				Type: "package-type",
-				Path: "package-path",
-			}))
+			Expect(md).To(Equal([]byte(`{"some":"json"}`)))
 			codePackage, err := ioutil.ReadAll(codeStream)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(codePackage).To(Equal([]byte("code-bytes")))
 		})
 
 		Context("when the package provider returns an error before calling the external builder", func() {
 			BeforeEach(func() {
-				fakePackageProvider.GetChaincodePackageReturns(nil, nil, errors.New("fake-package-error"))
+				fakePackageProvider.GetChaincodePackageReturns(nil, nil, nil, errors.New("fake-package-error"))
 			})
 
 			It("wraps and returns the error", func() {
@@ -83,14 +81,14 @@ var _ = Describe("Router", func() {
 
 			It("does not call the external builder", func() {
 				router.Build("package-id")
-				Expect(fakeExternalVM.BuildCallCount()).To(Equal(0))
+				Expect(fakeExternalBuilder.BuildCallCount()).To(Equal(0))
 			})
 		})
 
 		Context("when the external builder returns an error", func() {
 			BeforeEach(func() {
-				fakeExternalVM.BuildReturns(nil, errors.New("fake-external-error"))
-				fakeDockerVM.BuildReturns(fakeInstance, nil)
+				fakeExternalBuilder.BuildReturns(nil, errors.New("fake-external-error"))
+				fakeDockerBuilder.BuildReturns(fakeInstance, nil)
 			})
 
 			It("wraps and returns the error", func() {
@@ -104,23 +102,23 @@ var _ = Describe("Router", func() {
 				err := router.Build("package-id")
 				Expect(err).NotTo(HaveOccurred())
 
-				Expect(fakeExternalVM.BuildCallCount()).To(Equal(1))
-				Expect(fakeDockerVM.BuildCallCount()).To(Equal(0))
+				Expect(fakeExternalBuilder.BuildCallCount()).To(Equal(1))
+				Expect(fakeDockerBuilder.BuildCallCount()).To(Equal(0))
 			})
 		})
 
 		Context("when the exxternal builder returns a nil instance", func() {
 			BeforeEach(func() {
-				fakeExternalVM.BuildReturns(nil, nil)
-				fakeDockerVM.BuildReturns(fakeInstance, nil)
+				fakeExternalBuilder.BuildReturns(nil, nil)
+				fakeDockerBuilder.BuildReturns(fakeInstance, nil)
 			})
 
 			It("falls back to the docker impl", func() {
 				err := router.Build("package-id")
 				Expect(err).NotTo(HaveOccurred())
 
-				Expect(fakeDockerVM.BuildCallCount()).To(Equal(1))
-				ccid, md, codeStream := fakeDockerVM.BuildArgsForCall(0)
+				Expect(fakeDockerBuilder.BuildCallCount()).To(Equal(1))
+				ccid, md, codeStream := fakeDockerBuilder.BuildArgsForCall(0)
 				Expect(ccid).To(Equal("package-id"))
 				Expect(md).To(Equal(&persistence.ChaincodePackageMetadata{
 					Type: "package-type",
@@ -133,7 +131,7 @@ var _ = Describe("Router", func() {
 
 			Context("when the package provider returns an error before calling the docker builder", func() {
 				BeforeEach(func() {
-					fakePackageProvider.GetChaincodePackageReturnsOnCall(1, nil, nil, errors.New("fake-package-error"))
+					fakePackageProvider.GetChaincodePackageReturnsOnCall(1, nil, nil, nil, errors.New("fake-package-error"))
 				})
 
 				It("wraps and returns the error", func() {
@@ -145,21 +143,21 @@ var _ = Describe("Router", func() {
 
 		Context("when an external builder is not provided", func() {
 			BeforeEach(func() {
-				router.ExternalVM = nil
-				fakeDockerVM.BuildReturns(fakeInstance, nil)
+				router.ExternalBuilder = nil
+				fakeDockerBuilder.BuildReturns(fakeInstance, nil)
 			})
 
 			It("uses the docker vm builder", func() {
 				err := router.Build("package-id")
 				Expect(err).NotTo(HaveOccurred())
-				Expect(fakeDockerVM.BuildCallCount()).To(Equal(1))
+				Expect(fakeDockerBuilder.BuildCallCount()).To(Equal(1))
 			})
 		})
 	})
 
 	Describe("Post-build operations", func() {
 		BeforeEach(func() {
-			fakeExternalVM.BuildReturns(fakeInstance, nil)
+			fakeExternalBuilder.BuildReturns(fakeInstance, nil)
 			err := router.Build("fake-id")
 			Expect(err).NotTo(HaveOccurred())
 		})