Skip to content

Commit

Permalink
Minor restructure of externalbuilders
Browse files Browse the repository at this point in the history 
- extract Instance to its own file
- convert NewCommand to a method on Builder
- minor white space changes

FAB-16108

Change-Id: I8cf70f1a43a565fc3715027e0d93021c1b656f02
Signed-off-by: Matthew Sykes <[email protected]>
  • Loading branch information
@sykesm @mastersingh24
sykesm authored and mastersingh24 committed Nov 19, 2019
1 parent 1b99fdd commit af55876
Show file tree
Hide file tree
Showing 4 changed files with 175 additions and 160 deletions.
94 changes: 19 additions & 75 deletions core/container/externalbuilders/externalbuilders.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,38 +33,6 @@ var (

const MetadataFile = "metadata.json"

type Instance struct {
PackageID string
BldDir string
Builder *Builder
RunStatus *RunStatus
}

func (i *Instance) Start(peerConnection *ccintf.PeerConnection) error {
rs, err := i.Builder.Run(i.PackageID, i.BldDir, peerConnection)
if err != nil {
return errors.WithMessage(err, "could not execute run")
}
i.RunStatus = rs
return nil
}

func (i *Instance) Stop() error {
return errors.Errorf("stop is not implemented for external builders yet")
}

func (i *Instance) Wait() (int, error) {
if i.RunStatus == nil {
return 0, errors.Errorf("instance was not successfully started")
}
<-i.RunStatus.Done()
err := i.RunStatus.Err()
if exitErr, ok := errors.Cause(err).(*exec.ExitError); ok {
return exitErr.ExitCode(), err
}
return 0, err
}

type BuildInfo struct {
BuilderName string `json:"builder_name"`
}
Expand All @@ -91,7 +59,6 @@ func (d *Detector) CachedBuild(ccid string) (*Instance, error) {
if os.IsNotExist(err) {
return nil, nil
}

if err != nil {
return nil, errors.WithMessage(err, "existing build detected, but something went wrong inspecting it")
}
Expand Down Expand Up @@ -209,12 +176,6 @@ type BuildContext struct {
BldDir string
}

var pkgIDreg = regexp.MustCompile("[<>:\"/\\\\|\\?\\*&]")

func SanitizeCCIDPath(ccid string) string {
return pkgIDreg.ReplaceAllString(ccid, "-")
}

func NewBuildContext(ccid string, md *persistence.ChaincodePackageMetadata, codePackage io.Reader) (bc *BuildContext, err error) {
scratchDir, err := ioutil.TempDir("", "fabric-"+SanitizeCCIDPath(ccid))
if err != nil {
Expand Down Expand Up @@ -268,6 +229,16 @@ func NewBuildContext(ccid string, md *persistence.ChaincodePackageMetadata, code
}, nil
}

func (bc *BuildContext) Cleanup() {
os.RemoveAll(bc.ScratchDir)
}

var pkgIDreg = regexp.MustCompile("[<>:\"/\\\\|\\?\\*&]")

func SanitizeCCIDPath(ccid string) string {
return pkgIDreg.ReplaceAllString(ccid, "-")
}

type buildMetadata struct {
Path string `json:"path"`
Type string `json:"type"`
Expand All @@ -288,10 +259,6 @@ func writeMetadataFile(ccid string, md *persistence.ChaincodePackageMetadata, ds
return ioutil.WriteFile(filepath.Join(dst, MetadataFile), mdBytes, 0700)
}

func (bc *BuildContext) Cleanup() {
os.RemoveAll(bc.ScratchDir)
}

type Builder struct {
EnvWhitelist []string
Location string
Expand All @@ -300,7 +267,7 @@ type Builder struct {
}

func CreateBuilders(builderConfs []peer.ExternalBuilder) []*Builder {
builders := []*Builder{}
var builders []*Builder
for _, builderConf := range builderConfs {
builders = append(builders, &Builder{
Location: builderConf.Path,
Expand All @@ -314,18 +281,11 @@ func CreateBuilders(builderConfs []peer.ExternalBuilder) []*Builder {

func (b *Builder) Detect(buildContext *BuildContext) bool {
detect := filepath.Join(b.Location, "bin", "detect")
cmd := NewCommand(
detect,
b.EnvWhitelist,
buildContext.SourceDir,
buildContext.MetadataDir,
)
cmd := b.NewCommand(detect, buildContext.SourceDir, buildContext.MetadataDir)

err := RunCommand(b.Logger, cmd)
if err != nil {
logger.Debugf("Detection for builder '%s' detect failed: %s", b.Name, err)
// XXX, we probably also want to differentiate between a 'not detected'
// and a 'I failed nastily', but, again, good enough for now
logger.Debugf("builder '%s' detect failed: %s", b.Name, err)
return false
}

Expand All @@ -334,13 +294,7 @@ func (b *Builder) Detect(buildContext *BuildContext) bool {

func (b *Builder) Build(buildContext *BuildContext) error {
build := filepath.Join(b.Location, "bin", "build")
cmd := NewCommand(
build,
b.EnvWhitelist,
buildContext.SourceDir,
buildContext.MetadataDir,
buildContext.BldDir,
)
cmd := b.NewCommand(build, buildContext.SourceDir, buildContext.MetadataDir, buildContext.BldDir)

err := RunCommand(b.Logger, cmd)
if err != nil {
Expand All @@ -363,12 +317,7 @@ func (b *Builder) Release(buildContext *BuildContext) error {
return errors.WithMessagef(err, "could not stat release binary '%s'", release)
}

cmd := NewCommand(
release,
b.EnvWhitelist,
buildContext.BldDir,
buildContext.ReleaseDir,
)
cmd := b.NewCommand(release, buildContext.BldDir, buildContext.ReleaseDir)

if err := RunCommand(b.Logger, cmd); err != nil {
return errors.Wrapf(err, "builder '%s' release failed", b.Name)
Expand Down Expand Up @@ -442,12 +391,7 @@ func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection
}

run := filepath.Join(b.Location, "bin", "run")
cmd := NewCommand(
run,
b.EnvWhitelist,
bldDir,
launchDir,
)
cmd := b.NewCommand(run, bldDir, launchDir)

rs := NewRunStatus()

Expand All @@ -466,10 +410,10 @@ func (b *Builder) Run(ccid, bldDir string, peerConnection *ccintf.PeerConnection

// NewCommand creates an exec.Cmd that is configured to prune the calling
// environment down to the environment variables specified in the external
// builder's EnvironmentWhitelist and the DefaultEnvWhitelist defined above.
func NewCommand(name string, envWhiteList []string, args ...string) *exec.Cmd {
// builder's EnvironmentWhitelist and the DefaultEnvWhitelist.
func (b *Builder) NewCommand(name string, args ...string) *exec.Cmd {
cmd := exec.Command(name, args...)
whitelist := appendDefaultWhitelist(envWhiteList)
whitelist := appendDefaultWhitelist(b.EnvWhitelist)
for _, key := range whitelist {
if val, ok := os.LookupEnv(key); ok {
cmd.Env = append(cmd.Env, fmt.Sprintf("%s=%s", key, val))
Expand Down
101 changes: 16 additions & 85 deletions core/container/externalbuilders/externalbuilders_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,12 +152,12 @@ var _ = Describe("Externalbuilders", func() {

Context("when the durable path cannot be created", func() {
BeforeEach(func() {
detector.DurablePath = "fake/path/to/nowhere"
detector.DurablePath = "path/to/nowhere"
})

It("wraps and returns the error", func() {
_, err := detector.Build("fake-package-id", md, codePackage)
Expect(err).To(MatchError("could not create dir 'fake/path/to/nowhere/fake-package-id' to persist build ouput: mkdir fake/path/to/nowhere/fake-package-id: no such file or directory"))
Expect(err).To(MatchError("could not create dir 'path/to/nowhere/fake-package-id' to persist build ouput: mkdir path/to/nowhere/fake-package-id: no such file or directory"))
})
})
})
Expand Down Expand Up @@ -355,24 +355,24 @@ var _ = Describe("Externalbuilders", func() {
})
})
})
})

Describe("NewCommand", func() {
It("only propagates expected variables", func() {
var expectedEnv []string
for _, key := range externalbuilders.DefaultEnvWhitelist {
if val, ok := os.LookupEnv(key); ok {
expectedEnv = append(expectedEnv, fmt.Sprintf("%s=%s", key, val))
Describe("NewCommand", func() {
It("only propagates expected variables", func() {
var expectedEnv []string
for _, key := range externalbuilders.DefaultEnvWhitelist {
if val, ok := os.LookupEnv(key); ok {
expectedEnv = append(expectedEnv, fmt.Sprintf("%s=%s", key, val))
}
}
}

cmd := externalbuilders.NewCommand("/usr/bin/env", externalbuilders.DefaultEnvWhitelist)
Expect(cmd.Env).To(ConsistOf(expectedEnv))
cmd := builder.NewCommand("/usr/bin/env")
Expect(cmd.Env).To(ConsistOf(expectedEnv))

output, err := cmd.CombinedOutput()
Expect(err).NotTo(HaveOccurred())
env := strings.Split(strings.TrimSuffix(string(output), "\n"), "\n")
Expect(env).To(ConsistOf(expectedEnv))
output, err := cmd.CombinedOutput()
Expect(err).NotTo(HaveOccurred())
env := strings.Split(strings.TrimSuffix(string(output), "\n"), "\n")
Expect(env).To(ConsistOf(expectedEnv))
})
})
})

Expand Down Expand Up @@ -447,75 +447,6 @@ var _ = Describe("Externalbuilders", func() {
})
})

Describe("Instance", func() {
var i *externalbuilders.Instance

BeforeEach(func() {
i = &externalbuilders.Instance{
PackageID: "test-ccid",
Builder: &externalbuilders.Builder{
Location: "testdata/goodbuilder",
Logger: logger,
},
}
})

Describe("Start", func() {
It("invokes the builder's run command and sets the run status", func() {
err := i.Start(&ccintf.PeerConnection{
Address: "fake-peer-address",
})
Expect(err).NotTo(HaveOccurred())
Expect(i.RunStatus).NotTo(BeNil())
Eventually(i.RunStatus.Done()).Should(BeClosed())
})
})

Describe("Stop", func() {
It("statically returns an error", func() {
err := i.Stop()
Expect(err).To(MatchError("stop is not implemented for external builders yet"))
})
})

Describe("Wait", func() {
BeforeEach(func() {
err := i.Start(&ccintf.PeerConnection{
Address: "fake-peer-address",
TLSConfig: &ccintf.TLSConfig{
ClientCert: []byte("fake-client-cert"),
ClientKey: []byte("fake-client-key"),
RootCert: []byte("fake-root-cert"),
},
})
Expect(err).NotTo(HaveOccurred())
})

It("returns the exit status of the run", func() {
code, err := i.Wait()
Expect(err).NotTo(HaveOccurred())
Expect(code).To(Equal(0))
})

When("run exits with a non-zero status", func() {
BeforeEach(func() {
i.Builder.Location = "testdata/failbuilder"
i.Builder.Name = "failbuilder"
err := i.Start(&ccintf.PeerConnection{
Address: "fake-peer-address",
})
Expect(err).NotTo(HaveOccurred())
})

It("returns the exit status of the run and accompanying error", func() {
code, err := i.Wait()
Expect(err).To(MatchError("builder 'failbuilder' run failed: exit status 1"))
Expect(code).To(Equal(1))
})
})
})
})

Describe("SanitizeCCIDPath", func() {
It("forbids the set of forbidden windows characters", func() {
sanitizedPath := externalbuilders.SanitizeCCIDPath(`<>:"/\|?*&`)
Expand Down
46 changes: 46 additions & 0 deletions core/container/externalbuilders/instance.go
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
Copyright IBM Corp. All Rights Reserved.
SPDX-License-Identifier: Apache-2.0
*/

package externalbuilders

import (
"os/exec"

"github.com/hyperledger/fabric/core/container/ccintf"
"github.com/pkg/errors"
)

type Instance struct {
PackageID string
BldDir string
Builder *Builder
RunStatus *RunStatus
}

func (i *Instance) Start(peerConnection *ccintf.PeerConnection) error {
rs, err := i.Builder.Run(i.PackageID, i.BldDir, peerConnection)
if err != nil {
return errors.WithMessage(err, "could not execute run")
}
i.RunStatus = rs
return nil
}

func (i *Instance) Stop() error {
return errors.Errorf("stop is not implemented for external builders yet")
}

func (i *Instance) Wait() (int, error) {
if i.RunStatus == nil {
return 0, errors.Errorf("instance was not successfully started")
}
<-i.RunStatus.Done()
err := i.RunStatus.Err()
if exitErr, ok := errors.Cause(err).(*exec.ExitError); ok {
return exitErr.ExitCode(), err
}
return 0, err
}
Loading

0 comments on commit af55876

Please sign in to comment.