fix(TestRun): if spec.arguments is invalid, pass error and stop reconciliation

Author: yorugac

api/v1alpha1/testrun_types.go

@@ -205,6 +205,12 @@ func init() {
 	SchemeBuilder.Register(&TestRun{}, &TestRunList{})
 }
 
+func (k6 *TestRunSpec) Validate() error {
+	// Currently, we validate "manually" only arguments field.
+	_, err := types.ParseCLI(k6.Arguments)
+	return err
+}
+
 // Parse extracts Script data bits from K6 spec and performs basic validation
 func (k6 TestRunSpec) ParseScript() (*types.Script, error) {
 	spec := k6.Script

internal/controller/k6_initialize.go

@@ -21,7 +21,8 @@ func InitializeJobs(ctx context.Context, log logr.Logger, k6 *v1alpha1.TestRun,
 	// initializer is a quick job so check in frequently
 	res = ctrl.Result{RequeueAfter: time.Second * 5}
 
-	cli := types.ParseCLI(k6.GetSpec().Arguments)
+	// validation has already happened, so error here can be ignored
+	cli, _ := types.ParseCLI(k6.GetSpec().Arguments)
 
 	var initializer *batchv1.Job
 	if initializer, err = jobs.NewInitializerJob(k6, cli.ArchiveArgs); err != nil {
@@ -50,7 +51,8 @@ func RunValidations(ctx context.Context, log logr.Logger, k6 *v1alpha1.TestRun,
 	// initializer is a quick job so check in frequently
 	res = ctrl.Result{RequeueAfter: time.Second * 5}
 
-	cli := types.ParseCLI(k6.GetSpec().Arguments)
+	// validation has already happened, so error here can be ignored
+	cli, _ := types.ParseCLI(k6.GetSpec().Arguments)
 
 	inspectOutput, inspectReady, err := inspectTestRun(ctx, log, k6, r.Client)
 	if err != nil {
@@ -63,7 +65,7 @@ func RunValidations(ctx context.Context, log logr.Logger, k6 *v1alpha1.TestRun,
 				WithAbort()
 			cloud.SendTestRunEvents(r.k6CloudClient, k6.TestRunID(), log, events)
 		} else {
-			// if there is any error, we have to reflect it on the K6 manifest
+			// if there is any error, we have to reflect it on the TestRun manifest
 			k6.GetStatus().Stage = "error"
 			if _, err := r.UpdateStatus(ctx, k6, log); err != nil {
 				return ctrl.Result{}, ready, err

internal/controller/testrun_controller.go

@@ -126,6 +126,14 @@ func (r *TestRunReconciler) reconcile(ctx context.Context, req ctrl.Request, log
 	case "":
 		log.Info("Initialize test")
 
+		if err := k6.GetSpec().Validate(); err != nil {
+			log.Error(err, "Invalid TestRun")
+			log.Info("Changing stage of TestRun status to error")
+			k6.GetStatus().Stage = "error"
+			_, err := r.UpdateStatus(ctx, k6, log)
+			return ctrl.Result{}, err
+		}
+
 		v1alpha1.Initialize(k6)
 
 		if _, err := r.UpdateStatus(ctx, k6, log); err != nil {

pkg/types/k6cli.go

@@ -1,6 +1,9 @@
 package types
 
-import "strings"
+import (
+	"fmt"
+	"strings"
+)
 
 // CLI is an internal type to support k6 invocation in initialization stage.
 // Not all k6 commands allow the same set of arguments so CLI is an object
@@ -12,7 +15,7 @@ type CLI struct {
 	HasCloudOut bool
 }
 
-func ParseCLI(arguments string) *CLI {
+func ParseCLI(arguments string) (*CLI, error) {
 	lastArgV := func(start int, args []string) (end int) {
 		end = start
 		for end < len(args) {
@@ -25,7 +28,10 @@ func ParseCLI(arguments string) *CLI {
 		return
 	}
 
-	var cli CLI
+	var (
+		cli CLI
+		err error
+	)
 
 	args := strings.Split(arguments, " ")
 	i := 0
@@ -66,8 +72,11 @@ func ParseCLI(arguments string) *CLI {
 				cli.ArchiveArgs += strings.Join(args[i:end], " ")
 			}
 			i = end
+		} else {
+			err = fmt.Errorf("encountered an invalid value for k6 CLI argument: `%s`", args[i])
+			break
 		}
 	}
 
-	return &cli
+	return &cli, err
 }

pkg/types/k6cli_test.go

@@ -8,42 +8,48 @@ import (
 
 func Test_ParseCLI(t *testing.T) {
 	tests := []struct {
-		name    string
-		argLine string
-		cli     CLI
+		name           string
+		argLine        string
+		cli            CLI
+		validArguments bool
 	}{
 		{
 			"EmptyArgs",
 			"",
 			CLI{},
+			false,
 		},
 		{
 			"ShortArchiveArgs",
 			"-u 10 -d 5",
 			CLI{
 				ArchiveArgs: "-u 10 -d 5",
 			},
+			false,
 		},
 		{
 			"LongArchiveArgs",
 			"--vus 10 --duration 5",
 			CLI{
 				ArchiveArgs: "--vus 10 --duration 5",
 			},
+			false,
 		},
 		{
 			"ShortNonArchiveArg",
 			"-u 10 -d 5 -l",
 			CLI{
 				ArchiveArgs: "-u 10 -d 5",
 			},
+			false,
 		},
 		{
 			"LongNonArchiveArgs",
 			"--vus 10 --duration 5 --linger",
 			CLI{
 				ArchiveArgs: "--vus 10 --duration 5",
 			},
+			false,
 		},
 		{
 			"OutWithoutCloudArgs",
@@ -52,6 +58,7 @@ func Test_ParseCLI(t *testing.T) {
 				ArchiveArgs: "--vus 10",
 				HasCloudOut: false,
 			},
+			false,
 		},
 		{
 			"OutWithCloudArgs",
@@ -60,6 +67,7 @@ func Test_ParseCLI(t *testing.T) {
 				ArchiveArgs: "--vus 10",
 				HasCloudOut: true,
 			},
+			false,
 		},
 		{
 			"VerboseOutWithCloudArgs",
@@ -68,6 +76,7 @@ func Test_ParseCLI(t *testing.T) {
 				ArchiveArgs: "--vus 10",
 				HasCloudOut: true,
 			},
+			false,
 		},
 		{
 			"OmitLogOutput",
@@ -76,15 +85,23 @@ func Test_ParseCLI(t *testing.T) {
 				ArchiveArgs: "--no-thresholds",
 				HasCloudOut: true,
 			},
+			false,
+		},
+		{
+			"InvalidArguments",
+			`run this-argument-does-not-matter.js -o json`,
+			CLI{},
+			true,
 		},
 	}
 
 	for _, test := range tests {
 		test := test
 		t.Run(test.name, func(t *testing.T) {
 			t.Parallel()
-			cli := ParseCLI(test.argLine)
+			cli, err := ParseCLI(test.argLine)
 
+			assert.Equal(t, test.validArguments, err != nil)
 			assert.Equal(t, test.cli.ArchiveArgs, cli.ArchiveArgs)
 			assert.Equal(t, test.cli.HasCloudOut, cli.HasCloudOut)
 		})