feat(shellExecute): cleanup, Jenkins step, docs … (#3313)

* remove functionality with script downloading (security issue), clean up code

* remove vault client creating, remove vault params

* fix go generate issue

* error handling and test updated

Co-authored-by: Oliver Nocon <33484802+OliverNocon@users.noreply.github.com>

cmd/shellExecute.go

@@ -1,28 +1,23 @@
 package cmd
 
 import (
-	"net/url"
+	"fmt"
 	"os/exec"
-	"strings"
 
-	"github.com/hashicorp/vault/api"
 	"github.com/pkg/errors"
 
 	"github.com/SAP/jenkins-library/pkg/command"
-	piperhttp "github.com/SAP/jenkins-library/pkg/http"
 	"github.com/SAP/jenkins-library/pkg/log"
 	"github.com/SAP/jenkins-library/pkg/piperutils"
 	"github.com/SAP/jenkins-library/pkg/telemetry"
-	"github.com/SAP/jenkins-library/pkg/vault"
 )
 
 type shellExecuteUtils interface {
 	command.ExecRunner
-	FileExists(filename string) (bool, error)
+	piperutils.FileUtils
 }
 
 type shellExecuteUtilsBundle struct {
-	*vault.Client
 	*command.Command
 	*piperutils.Files
 }
@@ -39,83 +34,33 @@ func newShellExecuteUtils() shellExecuteUtils {
 
 func shellExecute(config shellExecuteOptions, telemetryData *telemetry.CustomData) {
 	utils := newShellExecuteUtils()
-	fileUtils := &piperutils.Files{}
 
-	err := runShellExecute(&config, telemetryData, utils, fileUtils)
+	err := runShellExecute(&config, telemetryData, utils)
 	if err != nil {
 		log.Entry().WithError(err).Fatal("step execution failed")
 	}
 }
 
-func runShellExecute(config *shellExecuteOptions, telemetryData *telemetry.CustomData, utils shellExecuteUtils, fileUtils piperutils.FileUtils) error {
+func runShellExecute(config *shellExecuteOptions, telemetryData *telemetry.CustomData, utils shellExecuteUtils) error {
-	// create vault client
-	// try to retrieve existing credentials
-	// if it's impossible - will add it
-	vaultConfig := &vault.Config{
-		Config: &api.Config{
-			Address: config.VaultServerURL,
-		},
-		Namespace: config.VaultNamespace,
-	}
-	_, err := vault.NewClientWithAppRole(vaultConfig, GeneralConfig.VaultRoleID, GeneralConfig.VaultRoleSecretID)
-	if err != nil {
-		log.Entry().Info("could not create vault client:", err)
-	}
-
-	// piper http client for downloading scripts
-	httpClient := piperhttp.Client{}
-
-	// scripts for running locally
-	var e []string
-
 	// check input data
 	// example for script: sources: ["./script.sh"]
 	for _, source := range config.Sources {
-		// check it's a local script or remote
+		// check if the script is physically present
-		_, err := url.ParseRequestURI(source)
+		exists, err := utils.FileExists(source)
-		if err != nil {
-			// err means that it's not a remote script
-			// check if the script is physically present (for local scripts)
-			exists, err := fileUtils.FileExists(source)
 		if err != nil {
 			log.Entry().WithError(err).Error("failed to check for defined script")
-				return errors.Wrap(err, "failed to check for defined script")
+			return fmt.Errorf("failed to check for defined script: %w", err)
 		}
 		if !exists {
-				log.Entry().WithError(err).Error("the specified script could not be found")
+			log.Entry().WithError(err).Errorf("the script '%v' could not be found: %v", source, err)
-				return errors.New("the specified script could not be found")
+			return fmt.Errorf("the script '%v' could not be found", source)
 		}
-			e = append(e, source)
+		log.Entry().Info("starting running script:", source)
-		} else {
+		err = utils.RunExecutable(source)
-			// this block means that it's a remote script
-			// so, need to download it before
-			// get script name at first
-			path := strings.Split(source, "/")
-			err = httpClient.DownloadFile(source, path[len(path)-1], nil, nil)
 		if err != nil {
-				log.Entry().WithError(err).Errorf("the specified script could not be downloaded")
+			log.Entry().Errorln("starting running script:", source)
 		}
-			// make script executable
+		// handle exit code
-			exec.Command("/bin/sh", "chmod +x "+path[len(path)-1])
-
-			e = append(e, path[len(path)-1])
-
-		}
-	}
-
-	// if all ok - try to run them one by one
-	for _, script := range e {
-		log.Entry().Info("starting running script:", script)
-		err = utils.RunExecutable(script)
-		if err != nil {
-			log.Entry().Errorln("starting running script:", script)
-		}
-
-		// if it's an exit error, then check the exit code
-		// according to the requirements
-		// 0 - success
-		// 1 - fails the build (or > 2)
-		// 2 - build unstable - unsupported now
 		if ee, ok := err.(*exec.ExitError); ok {
 			switch ee.ExitCode() {
 			case 0:

cmd/shellExecute_generated.go

@@ -16,8 +16,6 @@ import (
 )
 
 type shellExecuteOptions struct {
-	VaultServerURL string   `json:"vaultServerUrl,omitempty"`
-	VaultNamespace string   `json:"vaultNamespace,omitempty"`
 	Sources []string `json:"sources,omitempty"`
 }
 
@@ -35,7 +33,7 @@ func ShellExecuteCommand() *cobra.Command {
 	var createShellExecuteCmd = &cobra.Command{
 		Use:   STEP_NAME,
 		Short: "Step executes defined script",
-		Long:  `Step executes defined script with Vault credentials, or created them on this step`,
+		Long:  `Step executes defined script with using test vault credentials`,
 		PreRunE: func(cmd *cobra.Command, _ []string) error {
 			startTime = time.Now()
 			log.SetStepName(STEP_NAME)
@@ -110,8 +108,6 @@ func ShellExecuteCommand() *cobra.Command {
 }
 
 func addShellExecuteFlags(cmd *cobra.Command, stepConfig *shellExecuteOptions) {
-	cmd.Flags().StringVar(&stepConfig.VaultServerURL, "vaultServerUrl", os.Getenv("PIPER_vaultServerUrl"), "The URL for the Vault server to use")
-	cmd.Flags().StringVar(&stepConfig.VaultNamespace, "vaultNamespace", os.Getenv("PIPER_vaultNamespace"), "The vault namespace that should be used (optional)")
 	cmd.Flags().StringSliceVar(&stepConfig.Sources, "sources", []string{}, "Scripts names for execution or links to scripts")
 
 }
@@ -127,24 +123,6 @@ func shellExecuteMetadata() config.StepData {
 		Spec: config.StepSpec{
 			Inputs: config.StepInputs{
 				Parameters: []config.StepParameters{
-					{
-						Name:        "vaultServerUrl",
-						ResourceRef: []config.ResourceReference{},
-						Scope:       []string{"GENERAL", "PARAMETERS", "STAGES", "STEPS"},
-						Type:        "string",
-						Mandatory:   false,
-						Aliases:     []config.Alias{},
-						Default:     os.Getenv("PIPER_vaultServerUrl"),
-					},
-					{
-						Name:        "vaultNamespace",
-						ResourceRef: []config.ResourceReference{},
-						Scope:       []string{"GENERAL", "PARAMETERS", "STAGES", "STEPS"},
-						Type:        "string",
-						Mandatory:   false,
-						Aliases:     []config.Alias{},
-						Default:     os.Getenv("PIPER_vaultNamespace"),
-					},
 					{
 						Name:        "sources",
 						ResourceRef: []config.ResourceReference{},
@@ -156,6 +134,9 @@ func shellExecuteMetadata() config.StepData {
 					},
 				},
 			},
+			Containers: []config.Container{
+				{Name: "shell", Image: "node:lts-stretch", WorkingDir: "/home/node"},
+			},
 		},
 	}
 	return theMetaData

cmd/shellExecute_test.go

@@ -52,32 +52,24 @@ func TestRunShellExecute(t *testing.T) {
 			Sources: []string{"path/to/script.sh"},
 		}
 		u := newShellExecuteTestsUtils()
-		fm := &shellExecuteFileMock{}
 
-		err := runShellExecute(c, nil, u, fm)
+		err := runShellExecute(c, nil, u)
-		assert.EqualError(t, err, "the specified script could not be found")
+		assert.EqualError(t, err, "the script 'path/to/script.sh' could not be found")
 	})
 
 	t.Run("success case - script is present", func(t *testing.T) {
 		o := &shellExecuteOptions{}
 		u := newShellExecuteTestsUtils()
-		m := &shellExecuteFileMock{
-			fileReadContent: map[string]string{"path/to/script/script.sh": ``},
-		}
 
-		err := runShellExecute(o, nil, u, m)
+		err := runShellExecute(o, nil, u)
 		assert.NoError(t, err)
 	})
 
 	t.Run("success case - script run successfully", func(t *testing.T) {
 		o := &shellExecuteOptions{}
 		u := newShellExecuteTestsUtils()
-		m := &shellExecuteFileMock{
-			fileReadContent: map[string]string{"path/to/script/script.sh": `#!/usr/bin/env sh
-print 'test'`},
-		}
 
-		err := runShellExecute(o, nil, u, m)
+		err := runShellExecute(o, nil, u)
 		assert.NoError(t, err)
 	})
 

documentation/docs/steps/shellExecute.md

@@ -0,0 +1,7 @@
+# ${docGenStepName}
+
+## ${docGenDescription}
+
+## ${docGenParameters}
+
+## ${docGenConfiguration}

documentation/mkdocs.yml

@@ -145,6 +145,7 @@ nav:
         - protecodeExecuteScan: steps/protecodeExecuteScan.md
         - seleniumExecuteTests: steps/seleniumExecuteTests.md
         - setupCommonPipelineEnvironment: steps/setupCommonPipelineEnvironment.md
+        - shellExecute: steps/shellExecute.md
         - slackSendNotification: steps/slackSendNotification.md
         - snykExecute: steps/snykExecute.md
         - sonarExecuteScan: steps/sonarExecuteScan.md

resources/metadata/shellExecute.yaml

@@ -1,26 +1,10 @@
 metadata:
   name: shellExecute
   description: Step executes defined script
-  longDescription: Step executes defined script with Vault credentials, or created them on this step
+  longDescription: Step executes defined script with using test vault credentials
 spec:
   inputs:
     params:
-      - name: vaultServerUrl
-        type: string
-        scope:
-          - GENERAL
-          - PARAMETERS
-          - STAGES
-          - STEPS
-        description: The URL for the Vault server to use
-      - name: vaultNamespace
-        type: string
-        scope:
-          - GENERAL
-          - PARAMETERS
-          - STAGES
-          - STEPS
-        description: The vault namespace that should be used (optional)
       - name: sources
         type: "[]string"
         scope:
@@ -28,3 +12,7 @@ spec:
           - STAGES
           - STEPS
         description: Scripts names for execution or links to scripts
+  containers:
+    - name: shell
+      image: node:lts-stretch
+      workingDir: /home/node

test/groovy/CommonStepsTest.groovy

@@ -205,6 +205,7 @@ public class CommonStepsTest extends BasePiperTest{
         'golangBuild', //implementing new golang pattern without fields
         'apiProxyDownload', //implementing new golang pattern without fields
         'apiKeyValueMapDownload', //implementing new golang pattern without fields
+        'shellExecute', //implementing new golang pattern without fields
     ]
 
     @Test

vars/shellExecute.groovy

@@ -0,0 +1,9 @@
+import groovy.transform.Field
+
+@Field String STEP_NAME = getClass().getName()
+@Field String METADATA_FILE = 'metadata/shellExecute.yaml'
+
+void call(Map parameters = [:]) {
+    List credentials = []
+    piperExecuteBin(parameters, STEP_NAME, METADATA_FILE, credentials)
+}