builder: Allow command with arguments via custom build options (#2453)

* allow buildcmd command to process arguments
* Add unit tests for builder with different scenarios

Signed-off-by: Sanket Sudake <sanketsudake@gmail.com>
Co-authored-by: Sanket Sudake <sanketsudake@gmail.com>

pkg/builder/builder.go

@@ -38,8 +38,8 @@ import (
 
 const (
 	// supported environment variables
-	envSrcPkg    = "SRC_PKG"
+	envSrcPkg    string = "SRC_PKG"
-	envDeployPkg = "DEPLOY_PKG"
+	envDeployPkg string = "DEPLOY_PKG"
 )
 
 type (
@@ -114,16 +114,27 @@ func (builder *Builder) Handler(w http.ResponseWriter, r *http.Request) {
 	}
 	builder.logger.Info("builder received request", zap.Any("request", req))
 
-	builder.logger.Info("starting build")
+	builder.logger.Debug("starting build")
 	srcPkgPath := filepath.Join(builder.sharedVolumePath, req.SrcPkgFilename)
-	deployPkgFilename := fmt.Sprintf("%v-%v", req.SrcPkgFilename, strings.ToLower(uniuri.NewLen(6)))
+	deployPkgFilename := fmt.Sprintf("%s-%s", req.SrcPkgFilename, strings.ToLower(uniuri.NewLen(6)))
 	deployPkgPath := filepath.Join(builder.sharedVolumePath, deployPkgFilename)
+
+	var buildArgs []string
 	buildCmd := req.BuildCommand
 	if len(buildCmd) == 0 {
 		// use default build command
 		buildCmd = "/build"
+	} else {
+		// split executable command and arguments
+		args := strings.Split(buildCmd, " ")
+		buildCmd = args[0] // get the executable command, executable command will always be on Zero index
+
+		// get all the arguments
+		for i := 1; i < len(args); i++ {
+			buildArgs = append(buildArgs, args[i])
+		}
 	}
-	buildLogs, err := builder.build(buildCmd, srcPkgPath, deployPkgPath)
+	buildLogs, err := builder.build(buildCmd, buildArgs, srcPkgPath, deployPkgPath)
 	if err != nil {
 		e := "error building source package"
 		builder.logger.Error(e, zap.Error(err))
@@ -146,7 +157,7 @@ func (builder *Builder) reply(w http.ResponseWriter, pkgFilename string, buildLo
 	rBody, err := json.Marshal(resp)
 	if err != nil {
 		e := errors.Wrap(err, "error encoding response body")
-		rBody = []byte(fmt.Sprintf(`{"buildLogs": "%v"}`, e.Error()))
+		rBody = []byte(fmt.Sprintf(`{"buildLogs": "%s"}`, e.Error()))
 		statusCode = http.StatusInternalServerError
 	}
 
@@ -163,8 +174,8 @@ func (builder *Builder) reply(w http.ResponseWriter, pkgFilename string, buildLo
 	}
 }
 
-func (builder *Builder) build(command string, srcPkgPath string, deployPkgPath string) (string, error) {
+func (builder *Builder) build(command string, args []string, srcPkgPath string, deployPkgPath string) (string, error) {
-	cmd := exec.Command(command)
+	cmd := exec.Command(command, args...)
 
 	fi, err := os.Stat(srcPkgPath)
 	if err != nil {
@@ -178,8 +189,8 @@ func (builder *Builder) build(command string, srcPkgPath string, deployPkgPath s
 
 	// set env variables for build command
 	cmd.Env = append(os.Environ(),
-		fmt.Sprintf("%v=%v", envSrcPkg, srcPkgPath),
+		fmt.Sprintf("%s=%s", envSrcPkg, srcPkgPath),
-		fmt.Sprintf("%v=%v", envDeployPkg, deployPkgPath),
+		fmt.Sprintf("%s=%s", envDeployPkg, deployPkgPath),
 	)
 
 	stdout, err := cmd.StdoutPipe()
@@ -192,12 +203,8 @@ func (builder *Builder) build(command string, srcPkgPath string, deployPkgPath s
 		return "", errors.Wrap(err, "error creating stderr pipe for cmd")
 	}
 
-	var buildLogs string
-
-	fmt.Printf("\n=== Build Logs ===")
 	// Init logs
-	fmt.Printf("command=%v\n", command)
+	builder.logger.Info("building source package", zap.String("command", command), zap.Strings("args", args), zap.Strings("env", cmd.Env))
-	fmt.Printf("env=%v\n", cmd.Env)
 
 	out := io.MultiReader(stdout, stderr)
 	scanner := bufio.NewScanner(out)
@@ -206,12 +213,14 @@ func (builder *Builder) build(command string, srcPkgPath string, deployPkgPath s
 	if err != nil {
 		return "", errors.Wrap(err, "error starting cmd")
 	}
-
+	fmt.Printf("========= START =========\n")
+	defer fmt.Printf("========= END ===========\n")
+	var buildLogs string
 	// Runtime logs
 	for scanner.Scan() {
 		output := scanner.Text()
 		fmt.Println(output)
-		buildLogs += fmt.Sprintf("%v\n", output)
+		buildLogs += fmt.Sprintf("%s\n", output)
 	}
 
 	if err := scanner.Err(); err != nil {
@@ -226,7 +235,5 @@ func (builder *Builder) build(command string, srcPkgPath string, deployPkgPath s
 		fmt.Println(cmdErr)
 		return buildLogs, cmdErr
 	}
-	fmt.Printf("==================\n")
-
 	return buildLogs, nil
 }

pkg/builder/builder_test.go

@@ -0,0 +1,171 @@
+/*
+Copyright 2022 The Fission Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+package builder
+
+import (
+	"bytes"
+	"encoding/json"
+	"io/ioutil"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/fission/fission/pkg/utils/loggerfactory"
+)
+
+func TestBuilder(t *testing.T) {
+	logger := loggerfactory.GetLogger()
+
+	dir, err := os.MkdirTemp("/tmp", "fission-builder-test")
+	if err != nil {
+		t.Fatal(err)
+	}
+	builder := MakeBuilder(logger, dir)
+
+	// Test VersionHandler
+	t.Run("VersionHandler", func(t *testing.T) {
+		t.Run("should return version", func(t *testing.T) {
+			w := &httptest.ResponseRecorder{}
+			r := &http.Request{}
+			builder.VersionHandler(w, r)
+			if w.Result().StatusCode != http.StatusOK {
+				t.Errorf("expected status code %d, got %d", http.StatusOK, w.Result().StatusCode)
+			}
+			if w.Result().Body == nil {
+				t.Error("expected body, got nil")
+			}
+		})
+	})
+
+	// Test BuildHandler
+	t.Run("BuildHandler", func(t *testing.T) {
+
+		for _, test := range []struct {
+			name         string
+			buildRequest *PackageBuildRequest
+			expected     *PackageBuildResponse
+			status       int
+		}{
+			{
+				name: "should work with build command without argument",
+				buildRequest: &PackageBuildRequest{
+					SrcPkgFilename: "test",
+					BuildCommand:   "ls",
+				},
+				expected: &PackageBuildResponse{
+					ArtifactFilename: "test",
+					BuildLogs:        "",
+				},
+				status: http.StatusOK,
+			},
+			{
+				name: "should work with build command with argument",
+				buildRequest: &PackageBuildRequest{
+					SrcPkgFilename: "test1",
+					BuildCommand:   "ls -la",
+				},
+				expected: &PackageBuildResponse{
+					ArtifactFilename: "test1",
+					BuildLogs:        "",
+				},
+				status: http.StatusOK,
+			},
+			{
+				name: "should fail with argument and pipe",
+				buildRequest: &PackageBuildRequest{
+					SrcPkgFilename: "test2",
+					BuildCommand:   "ps -ef | grep fission",
+				},
+				expected: &PackageBuildResponse{
+					ArtifactFilename: "test2",
+					BuildLogs:        "",
+				},
+				status: http.StatusInternalServerError,
+			},
+			{
+				name: "should fail with invalid build command",
+				buildRequest: &PackageBuildRequest{
+					SrcPkgFilename: "test3",
+					BuildCommand:   "lsalas -la",
+				},
+				expected: &PackageBuildResponse{
+					ArtifactFilename: "test3",
+					BuildLogs:        "",
+				},
+				status: http.StatusInternalServerError,
+			},
+			{
+				name: "should fail with valid command and invalid argument",
+				buildRequest: &PackageBuildRequest{
+					SrcPkgFilename: "test3",
+					BuildCommand:   "ls --la",
+				},
+				expected: &PackageBuildResponse{
+					ArtifactFilename: "test3",
+					BuildLogs:        "",
+				},
+				status: http.StatusInternalServerError,
+			},
+		} {
+			t.Run(test.name, func(t *testing.T) {
+				srcFile, err := os.Create(dir + "/" + test.buildRequest.SrcPkgFilename)
+				if err != nil {
+					t.Fatal(err)
+				}
+				defer srcFile.Close()
+				body, err := json.Marshal(test.buildRequest)
+				if err != nil {
+					t.Fatal(err)
+				}
+				w := httptest.NewRecorder()
+				r := httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(body))
+				builder.Handler(w, r)
+				resp := w.Result()
+				if resp.StatusCode != test.status {
+					t.Errorf("expected status code %d, got %d", test.status, resp.StatusCode)
+				}
+				body, err = ioutil.ReadAll(resp.Body)
+				if err != nil {
+					t.Fatal(err)
+				}
+				var buildResp PackageBuildResponse
+				err = json.Unmarshal(body, &buildResp)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if test.status == http.StatusOK {
+					if strings.Contains(buildResp.BuildLogs, "error") {
+						t.Errorf("expected build logs to not contain error, got %s", buildResp.BuildLogs)
+					}
+					artifacts := strings.Split(buildResp.ArtifactFilename, "-")
+					if len(artifacts) != 2 {
+						t.Errorf("expected artifact filename to be of the form <pkgname>-<build-id>, got %s", buildResp.ArtifactFilename)
+					}
+					if artifacts[0] != test.expected.ArtifactFilename {
+						t.Errorf("expected artifact filename to be %s, got %s", test.expected.ArtifactFilename, artifacts[0])
+					}
+				} else {
+					if !strings.Contains(buildResp.BuildLogs, "error") {
+						t.Errorf("expected build logs to contain error, got %s", buildResp.BuildLogs)
+					}
+				}
+
+			})
+		}
+	})
+}