Repository: iter8-tools/iter8
Pull request #1449 (2023-04-24 13:17:46)

Fix nil dereference error

Alan-Cha: Fixes #1451

Patch:
diff --git a/base/collect_grpc.go b/base/collect_grpc.go
index 69b4b808..60afcedc 100644
--- a/base/collect_grpc.go
+++ b/base/collect_grpc.go
@@ -1,7 +1,6 @@
 package base
 
 import (
-	"errors"
 	"fmt"
 	"time"
 
@@ -102,18 +101,12 @@ func (t *collectGRPCTask) resultForVersion() (map[string]*runner.Report, error)
 			}
 			eOpts := runner.WithConfig(&endpoint) // endpoint options
 
+			log.Logger.Trace("run ghz gRPC test")
 			igr, err := runner.Run(call, host, eOpts)
 			if err != nil {
-				e := fmt.Errorf("ghz run failed for endpoint \"%s\"", endpointID)
-				log.Logger.WithStackTrace(err.Error()).Error(e)
-				if igr == nil {
-					e = fmt.Errorf("failed to get results for endpoint \"%s\" since ghz run was aborted", endpointID)
-					log.Logger.Error(e)
-				}
-				return nil, e
+				log.Logger.WithStackTrace(err.Error()).Error(err)
+				continue
 			}
-			log.Logger.Trace("ran ghz gRPC test")
-			log.Logger.Trace(igr.ErrorDist)
 
 			results[gRPCMetricPrefix+"-"+endpointID] = igr
 		}
@@ -121,18 +114,12 @@ func (t *collectGRPCTask) resultForVersion() (map[string]*runner.Report, error)
 		// TODO: supply all the allowed options
 		opts := runner.WithConfig(&t.With.Config)
 
+		log.Logger.Trace("run ghz gRPC test")
 		igr, err := runner.Run(t.With.Call, t.With.Host, opts)
 		if err != nil {
-			e := errors.New("ghz run failed")
-			log.Logger.WithStackTrace(err.Error()).Error(e)
-			if igr == nil {
-				e = errors.New("failed to get results since ghz run was aborted")
-				log.Logger.Error(e)
-			}
-			return nil, e
+			log.Logger.WithStackTrace(err.Error()).Error(err)
+			return results, err
 		}
-		log.Logger.Trace("ran ghz gRPC test")
-		log.Logger.Trace(igr.ErrorDist)
 
 		results[gRPCMetricPrefix] = igr
 	}
diff --git a/base/collect_http.go b/base/collect_http.go
index 0e7d2e92..5d32679b 100644
--- a/base/collect_http.go
+++ b/base/collect_http.go
@@ -247,16 +247,13 @@ func (t *collectHTTPTask) getFortioResults() (map[string]*fhttp.HTTPRunnerResult
 			log.Logger.Trace("got fortio options")
 			log.Logger.Trace("URL: ", efo.URL)
 
+			log.Logger.Trace("run fortio HTTP test")
 			ifr, err := fhttp.RunHTTPTest(efo)
 			if err != nil {
 				log.Logger.WithStackTrace(err.Error()).Error("fortio failed")
-				if ifr == nil {
-					log.Logger.Error("failed to get results since fortio run was aborted")
-				}
+				continue
 			}
 
-			log.Logger.Trace("ran fortio http test")
-
 			results[httpMetricPrefix+"-"+endpointID] = ifr
 		}
 	} else {
@@ -269,16 +266,13 @@ func (t *collectHTTPTask) getFortioResults() (map[string]*fhttp.HTTPRunnerResult
 		log.Logger.Trace("got fortio options")
 		log.Logger.Trace("URL: ", fo.URL)
 
+		log.Logger.Trace("run fortio HTTP test")
 		ifr, err := fhttp.RunHTTPTest(fo)
 		if err != nil {
 			log.Logger.WithStackTrace(err.Error()).Error("fortio failed")
-			if ifr == nil {
-				log.Logger.Error("failed to get results since fortio run was aborted")
-			}
+			return results, err
 		}
 
-		log.Logger.Trace("ran fortio http test")
-
 		results[httpMetricPrefix] = ifr
 	}
 
@@ -424,6 +418,7 @@ func (t *collectHTTPTask) run(exp *Experiment) error {
 			return err
 		}
 	}
+
 	return nil
 }
 


Test patch:
diff --git a/base/collect_grpc_test.go b/base/collect_grpc_test.go
index 9a007817..2e70a9cf 100644
--- a/base/collect_grpc_test.go
+++ b/base/collect_grpc_test.go
@@ -2,6 +2,7 @@ package base
 
 import (
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -79,6 +80,37 @@ func TestRunCollectGRPCUnary(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+// If the endpoint does not exist, fail gracefully
+// Should not return an nil pointer dereference error (see #1451)
+func TestRunCollectGRPCUnaryNoEndpoint(t *testing.T) {
+	// valid collect GRPC task... should succeed
+	ct := &collectGRPCTask{
+		TaskMeta: TaskMeta{
+			Task: StringPointer(CollectGRPCTaskName),
+		},
+		With: collectGRPCInputs{
+			Config: runner.Config{
+				Data: map[string]interface{}{"name": "bob"},
+				Call: "helloworld.Greeter.SayHello",
+				Host: internal.LocalHostPort,
+			},
+		},
+	}
+
+	log.Logger.Debug("dial timeout before defaulting... ", ct.With.DialTimeout.String())
+
+	exp := &Experiment{
+		Spec:   []Task{ct},
+		Result: &ExperimentResult{},
+	}
+	exp.initResults(1)
+	err := ct.run(exp)
+
+	// Error should be a connection error, not a nil pointer dereference error
+	// Test written like this because of conversion between localhost and 127.0.0.1
+	assert.True(t, strings.HasPrefix(err.Error(), "rpc error: code = Unavailable desc = connection error: desc = \"transport: Error while dialing: dial tcp"))
+}
+
 // Credit: Several of the tests in this file are based on
 // https://github.com/bojand/ghz/blob/master/runner/run_test.go
 func TestRunCollectGRPCEndpoints(t *testing.T) {
@@ -157,6 +189,55 @@ func TestRunCollectGRPCEndpoints(t *testing.T) {
 	}
 }
 
+// If the endpoints cannot be reached, then do not throw an error
+// Should not return an nil pointer dereference error (see #1451)
+func TestRunCollectGRPCMultipleNoEndpoints(t *testing.T) {
+	// valid collect GRPC task... should succeed
+	ct := &collectGRPCTask{
+		TaskMeta: TaskMeta{
+			Task: StringPointer(CollectGRPCTaskName),
+		},
+		With: collectGRPCInputs{
+			Config: runner.Config{
+				Host: internal.LocalHostPort,
+			},
+			Endpoints: map[string]runner.Config{
+				unary: {
+					Data: map[string]interface{}{"name": "bob"},
+					Call: "helloworld.Greeter.SayHello",
+				},
+				server: {
+					Data: map[string]interface{}{"name": "bob"},
+					Call: "helloworld.Greeter.SayHelloCS",
+				},
+				client: {
+					Data: map[string]interface{}{"name": "bob"},
+					Call: "helloworld.Greeter.SayHellos",
+				},
+				bidirectional: {
+					Data: map[string]interface{}{"name": "bob"},
+					Call: "helloworld.Greeter.SayHelloBidi",
+				},
+			},
+		},
+	}
+
+	log.Logger.Debug("dial timeout before defaulting... ", ct.With.DialTimeout.String())
+
+	exp := &Experiment{
+		Spec:   []Task{ct},
+		Result: &ExperimentResult{},
+	}
+	exp.initResults(1)
+	err := ct.run(exp)
+	assert.NoError(t, err)
+
+	// No metrics should be collected
+	assert.Equal(t, 0, len(exp.Result.Insights.NonHistMetricValues[0]))
+	assert.Equal(t, 0, len(exp.Result.Insights.HistMetricValues[0]))
+	assert.Equal(t, 0, len(exp.Result.Insights.SummaryMetricValues[0]))
+}
+
 func TestMockGRPCWithSLOsAndPercentiles(t *testing.T) {
 	_ = os.Chdir(t.TempDir())
 	callType := helloworld.Unary
diff --git a/base/collect_http_test.go b/base/collect_http_test.go
index 3c35b808..7a377570 100644
--- a/base/collect_http_test.go
+++ b/base/collect_http_test.go
@@ -74,6 +74,38 @@ func TestRunCollectHTTP(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+// If the endpoint does not exist, fail gracefully
+// Should not return an nil pointer dereference error (see #1451)
+func TestRunCollectHTTPNoEndpoint(t *testing.T) {
+	_, addr := fhttp.DynamicHTTPServer(false)
+
+	baseURL := fmt.Sprintf("http://localhost:%d/", addr.Port)
+
+	// valid collect HTTP task... should succeed
+	ct := &collectHTTPTask{
+		TaskMeta: TaskMeta{
+			Task: StringPointer(CollectHTTPTaskName),
+		},
+		With: collectHTTPInputs{
+			endpoint: endpoint{
+				Duration:    StringPointer("1s"),
+				PayloadFile: StringPointer(CompletePath("../", "testdata/payload/ukpolice.json")),
+				Headers:     map[string]string{},
+				URL:         baseURL,
+			},
+		},
+	}
+
+	exp := &Experiment{
+		Spec:   []Task{ct},
+		Result: &ExperimentResult{},
+	}
+	exp.initResults(1)
+	err := ct.run(exp)
+
+	assert.EqualError(t, err, fmt.Sprintf("error 404 for %s (176 bytes)", baseURL))
+}
+
 // Multiple endpoints are provided
 // Test both the /foo/ and /bar/ endpoints
 // Test both endpoints have their respective header values
@@ -236,6 +268,53 @@ func TestRunCollectHTTPSingleEndpointMultipleCalls(t *testing.T) {
 	assert.NoError(t, err)
 }
 
+// If the endpoints cannot be reached, then do not throw an error
+// Should not return an nil pointer dereference error (see #1451)
+func TestRunCollectHTTPMultipleNoEndpoints(t *testing.T) {
+	_, addr := fhttp.DynamicHTTPServer(false)
+
+	baseURL := fmt.Sprintf("http://localhost:%d/", addr.Port)
+
+	// valid collect HTTP task... should succeed
+	ct := &collectHTTPTask{
+		TaskMeta: TaskMeta{
+			Task: StringPointer(CollectHTTPTaskName),
+		},
+		With: collectHTTPInputs{
+			endpoint: endpoint{
+				Duration: StringPointer("1s"),
+			},
+			Endpoints: map[string]endpoint{
+				endpoint1: {
+					URL: baseURL + foo,
+					Headers: map[string]string{
+						from: foo,
+					},
+				},
+				endpoint2: {
+					URL: baseURL + bar,
+					Headers: map[string]string{
+						from: bar,
+					},
+				},
+			},
+		},
+	}
+
+	exp := &Experiment{
+		Spec:   []Task{ct},
+		Result: &ExperimentResult{},
+	}
+	exp.initResults(1)
+	err := ct.run(exp)
+	assert.NoError(t, err)
+
+	// No metrics should be collected
+	assert.Equal(t, 0, len(exp.Result.Insights.NonHistMetricValues[0]))
+	assert.Equal(t, 0, len(exp.Result.Insights.HistMetricValues[0]))
+	assert.Equal(t, 0, len(exp.Result.Insights.SummaryMetricValues[0]))
+}
+
 func TestErrorCode(t *testing.T) {
 	task := collectHTTPTask{}
 	assert.True(t, task.errorCode(-1))