Repository: istio/fortio
Pull request #270 (2018-07-05 18:17:05)

Payload size flag is added for GRPC.
Using payload size flag, payload field will be generated randomly as a string for grpc ping message.
Random payload generator method is moved to network from http_server for reusing.

istio-testing: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: 
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: 

Assign the PR to them by writing `/assign ` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/istio/fortio/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
istio-testing: Hi @olimpias. Thanks for your PR.

I'm waiting for a [istio](https://github.com/orgs/istio/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://github.com/orgs/istio/people) to skip this step.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

codecov[bot]: # [Codecov](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=h1) Report
> Merging [#270](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=desc) into [master](https://codecov.io/gh/istio/fortio/commit/adf7d03241806aab8295550ad1d745356f56e29b?src=pr&el=desc) will **decrease** coverage by `0.1%`.
> The diff coverage is `81%`.

[![Impacted file tree graph](https://codecov.io/gh/istio/fortio/pull/270/graphs/tree.svg?width=650&height=150&src=pr&token=6HLno2jpvU)](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=tree)

```diff
@@           Coverage Diff            @@
##           master    #270     +/-   ##
========================================
- Coverage    90.6%   90.5%   -0.1%     
========================================
  Files          10      10             
  Lines        1952    1954      +2     
========================================
  Hits         1769    1769             
- Misses        117     118      +1     
- Partials       66      67      +1
```


| [Impacted Files](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [fhttp/http\_server.go](https://codecov.io/gh/istio/fortio/pull/270/diff?src=pr&el=tree#diff-Zmh0dHAvaHR0cF9zZXJ2ZXIuZ28=) | `82.8% <100%> (ø)` | :arrow_up: |
| [fhttp/http\_utils.go](https://codecov.io/gh/istio/fortio/pull/270/diff?src=pr&el=tree#diff-Zmh0dHAvaHR0cF91dGlscy5nbw==) | `100% <100%> (ø)` | :arrow_up: |
| [fgrpc/pingsrv.go](https://codecov.io/gh/istio/fortio/pull/270/diff?src=pr&el=tree#diff-ZmdycGMvcGluZ3Nydi5nbw==) | `86.4% <33.3%> (-1.7%)` | :arrow_down: |
| [fnet/network.go](https://codecov.io/gh/istio/fortio/pull/270/diff?src=pr&el=tree#diff-Zm5ldC9uZXR3b3JrLmdv) | `93.3% <81.8%> (-1.4%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=footer). Last update [adf7d03...9352ab1](https://codecov.io/gh/istio/fortio/pull/270?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ldemailly: also let's increase/keep coverage

Patch:
diff --git a/README.md b/README.md
index 43585d1..88764cd 100644
--- a/README.md
+++ b/README.md
@@ -175,6 +175,8 @@ target is a url (http load tests) or host:port (grpc health test).  flags are:
 	List of pXX to calculate (default "50,75,90,99,99.9")
   -payload string
 	Payload string to send along
+  -payload-size int
+    Additional random payload size, replaces -payload when set > 0, must be smaller than -maxpayloadsizekb
   -ping
 	grpc load test: use ping instead of health
   -profile string
diff --git a/fgrpc/pingsrv.go b/fgrpc/pingsrv.go
index 98f6629..b042904 100644
--- a/fgrpc/pingsrv.go
+++ b/fgrpc/pingsrv.go
@@ -90,6 +90,14 @@ func PingServer(port, cert, key, healthServiceName string, maxConcurrentStreams
 	return addr.Port
 }
 
+// PingClientCallPayloadSize calls the ping service. It is similar to PingClientCall instead of using payload, it uses
+// payload size to generate random payload. Returns the average round trip in seconds.
+func PingClientCallPayloadSize(serverAddr, cacert string, n int, payloadSize int, delay time.Duration) (float64, error) {
+	fnet.ValidatePayloadSize(&payloadSize)
+	payload := string(fnet.Payload[:payloadSize])
+	return PingClientCall(serverAddr, cacert, n, payload, delay)
+}
+
 // PingClientCall calls the ping service (presumably running as PingServer on
 // the destination). returns the average round trip in seconds.
 func PingClientCall(serverAddr, cacert string, n int, payload string, delay time.Duration) (float64, error) {
diff --git a/fhttp/http_server.go b/fhttp/http_server.go
index 571205f..98e36d4 100644
--- a/fhttp/http_server.go
+++ b/fhttp/http_server.go
@@ -18,7 +18,6 @@ import (
 	"bytes"
 	"fmt"
 	"io/ioutil"
-	"math/rand"
 	"net"
 	"net/http"
 	"os"
@@ -42,11 +41,6 @@ var (
 	startTime time.Time
 	// EchoRequests is the number of request received. Only updated in Debug mode.
 	EchoRequests int64
-	// MaxPayloadSize is the maximum size of payload to be generated by the
-	// EchoHandler size= argument. In bytes.
-	MaxPayloadSize = 256 * 1024
-	// Payload that is returned during echo call
-	payload []byte
 )
 
 // EchoHandler is an http server handler echoing back the input.
@@ -113,32 +107,11 @@ func EchoHandler(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func init() {
-	ChangeMaxPayloadSize(MaxPayloadSize)
-}
-
-// ChangeMaxPayloadSize is used to change max payload size and fill it with pseudorandom content
-func ChangeMaxPayloadSize(newMaxPayloadSize int) {
-	if newMaxPayloadSize >= 0 {
-		MaxPayloadSize = newMaxPayloadSize
-	} else {
-		MaxPayloadSize = 0
-	}
-	payload = make([]byte, MaxPayloadSize)
-	// One shared and 'constant' (over time) but pseudo random content for payload
-	// (to defeat compression). We don't need crypto strength here, just low cpu
-	// and speed:
-	_, err := rand.Read(payload)
-	if err != nil {
-		log.Errf("Error changing paylaod size, while generating random paylaod")
-	}
-}
-
 func writePayload(w http.ResponseWriter, status int, size int) {
 	w.Header().Set("Content-Type", "application/octet-stream")
 	w.Header().Set("Content-Length", strconv.Itoa(size))
 	w.WriteHeader(status)
-	n, err := w.Write(payload[:size])
+	n, err := w.Write(fnet.Payload[:size])
 	if err != nil || n != size {
 		log.Errf("Error writing payload of size %d: %d %v", size, n, err)
 	}
diff --git a/fhttp/http_utils.go b/fhttp/http_utils.go
index da29992..b599dd3 100644
--- a/fhttp/http_utils.go
+++ b/fhttp/http_utils.go
@@ -287,10 +287,7 @@ func generateSize(sizeInput string) (size int) {
 		}
 		size = s
 		log.Debugf("Parsed size %s -> %d", sizeInput, size)
-		if size > MaxPayloadSize {
-			log.Warnf("Requested size %d greater than max size %d, using max instead", size, MaxPayloadSize)
-			size = MaxPayloadSize
-		}
+		fnet.ValidatePayloadSize(&size)
 		return size
 	}
 	weights := make([]float32, len(lst))
@@ -308,10 +305,7 @@ func generateSize(sizeInput string) (size int) {
 			log.Warnf("Bad input size %v -> %v, not a number before colon", sizeInput, l2[0])
 			return size
 		}
-		if s > MaxPayloadSize {
-			log.Warnf("Requested size %d greater than max size %d, using max instead", s, MaxPayloadSize)
-			s = MaxPayloadSize
-		}
+		fnet.ValidatePayloadSize(&s)
 		percStr := removeTrailingPercent(l2[1])
 		p, err := strconv.ParseFloat(percStr, 32)
 		if err != nil || p < 0 || p > 100 {
diff --git a/fnet/network.go b/fnet/network.go
index 2b9326d..a7320db 100644
--- a/fnet/network.go
+++ b/fnet/network.go
@@ -17,6 +17,7 @@ package fnet // import "istio.io/fortio/fnet"
 import (
 	"fmt"
 	"io"
+	"math/rand"
 	"net"
 	"strings"
 	"sync"
@@ -38,6 +39,35 @@ const (
 	PrefixHTTPS = "https://"
 )
 
+var (
+	// MaxPayloadSize is the maximum size of payload to be generated by the
+	// EchoHandler size= argument. In bytes.
+	MaxPayloadSize = 256 * 1024
+	// Payload that is returned during echo call
+	Payload []byte
+)
+
+func init() {
+	ChangeMaxPayloadSize(MaxPayloadSize)
+}
+
+// ChangeMaxPayloadSize is used to change max payload size and fill it with pseudorandom content
+func ChangeMaxPayloadSize(newMaxPayloadSize int) {
+	if newMaxPayloadSize >= 0 {
+		MaxPayloadSize = newMaxPayloadSize
+	} else {
+		MaxPayloadSize = 0
+	}
+	Payload = make([]byte, MaxPayloadSize)
+	// One shared and 'constant' (over time) but pseudo random content for payload
+	// (to defeat compression). We don't need crypto strength here, just low cpu
+	// and speed:
+	_, err := rand.Read(Payload)
+	if err != nil {
+		log.Errf("Error changing paylaod size, while generating random paylaod")
+	}
+}
+
 // NormalizePort parses port and returns host:port if port is in the form
 // of host:port already or :port if port is only a port (doesn't contain :).
 func NormalizePort(port string) string {
@@ -183,3 +213,15 @@ func NormalizeHostPort(inputPort string, addr *net.TCPAddr) string {
 	}
 	return urlHostPort
 }
+
+// ValidatePayloadSize compares input size with MaxPayLoadSize. If size exceeds the MaxPayloadSize
+// size will set to MaxPayLoadSize
+func ValidatePayloadSize(size *int) {
+	if *size > MaxPayloadSize && *size > 0 {
+		log.Warnf("Requested size %d greater than max size %d, using max instead", *size, MaxPayloadSize)
+		*size = MaxPayloadSize
+	} else if *size < 0 {
+		log.Warnf("Requested size %d is negative, using 0 (no additional payload) instead.", *size)
+		*size = 0
+	}
+}
diff --git a/fortio_main.go b/fortio_main.go
index 1ab5344..e782c36 100644
--- a/fortio_main.go
+++ b/fortio_main.go
@@ -126,7 +126,7 @@ var (
 
 	baseURLFlag = flag.String("base-url", "",
 		"base URL used as prefix for data/index.tsv generation. (when empty, the url from the first request is used)")
-	newMaxPayloadSizeKb = flag.Int("maxpayloadsizekb", fhttp.MaxPayloadSize/1024,
+	newMaxPayloadSizeKb = flag.Int("maxpayloadsizekb", fnet.MaxPayloadSize/1024,
 		"MaxPayloadSize is the maximum size of payload to be generated by the EchoHandler size= argument. In Kbytes.")
 
 	// GRPC related flags
@@ -141,6 +141,9 @@ var (
 
 	maxStreamsFlag = flag.Uint("grpc-max-streams", 0,
 		"MaxConcurrentStreams for the grpc server. Default (0) is to leave the option unset.")
+
+	payloadSizeFlag = flag.Int("payload-size", 0, "Additional random payload size, replaces -payload when set > 0,"+
+		" must be smaller than -maxpayloadsizekb")
 )
 
 func main() {
@@ -152,7 +155,7 @@ func main() {
 	command := os.Args[1]
 	os.Args = append([]string{os.Args[0]}, os.Args[2:]...)
 	flag.Parse()
-	fhttp.ChangeMaxPayloadSize(*newMaxPayloadSizeKb * 1024)
+	fnet.ChangeMaxPayloadSize(*newMaxPayloadSizeKb * 1024)
 	if *bincommon.QuietFlag {
 		log.SetLogLevelQuiet(log.Error)
 	}
@@ -366,7 +369,12 @@ func grpcClient() {
 	if *doHealthFlag {
 		_, err = fgrpc.GrpcHealthCheck(host, cert, *healthSvcFlag, count)
 	} else {
-		_, err = fgrpc.PingClientCall(host, cert, count, *payloadFlag, *pingDelayFlag)
+		payloadSize := *payloadSizeFlag
+		if payloadSize > 0 {
+			_, err = fgrpc.PingClientCallPayloadSize(host, cert, count, payloadSize, *pingDelayFlag)
+		} else {
+			_, err = fgrpc.PingClientCall(host, cert, count, *payloadFlag, *pingDelayFlag)
+		}
 	}
 	if err != nil {
 		// already logged


Test patch:
diff --git a/fgrpc/pingsrv_test.go b/fgrpc/pingsrv_test.go
index 08644b6..57fec74 100644
--- a/fgrpc/pingsrv_test.go
+++ b/fgrpc/pingsrv_test.go
@@ -60,6 +60,12 @@ func TestPingServer(t *testing.T) {
 	if creds, err := credentials.NewServerTLSFromFile(failCrt, failKey); err == nil {
 		t.Errorf("Should have had an error instead of result %f for ping server", creds)
 	}
+	if latency, err := PingClientCallPayloadSize(sAddr, caCrt, 7, 10, 0); err != nil || latency <= 0 {
+		t.Errorf("Unexpected result %f, %v with ping calls", latency, err)
+	}
+	if latency, err := PingClientCallPayloadSize(iAddr, caCrt, 1, 0, 0); err == nil {
+		t.Errorf("Should have had an error instead of result %f for secure ping to insecure port", latency)
+	}
 	serving := grpc_health_v1.HealthCheckResponse_SERVING
 	if r, err := GrpcHealthCheck(iAddr, "", "", 1); err != nil || (*r)[serving] != 1 {
 		t.Errorf("Unexpected result %+v, %v with empty service health check", r, err)
diff --git a/fhttp/http_test.go b/fhttp/http_test.go
index ab7a613..5e8ba10 100644
--- a/fhttp/http_test.go
+++ b/fhttp/http_test.go
@@ -27,6 +27,7 @@ import (
 	"testing"
 	"time"
 
+	"istio.io/fortio/fnet"
 	"istio.io/fortio/log"
 )
 
@@ -481,7 +482,7 @@ func TestGenerateSize(t *testing.T) {
 		{"512:45,512:55", 512},
 		{"0", 0}, // and not -1
 		{"262144", 262144},
-		{"262145", MaxPayloadSize}, // MaxSize test
+		{"262145", fnet.MaxPayloadSize}, // MaxSize test
 		{"1000000:10,2000000:90", 262144},
 	}
 	for _, tst := range tests {
@@ -919,28 +920,6 @@ func TestDefaultHeadersAndOptionsInit(t *testing.T) {
 	}
 }
 
-func TestChangeMaxPayloadSize(t *testing.T) {
-	var tests = []struct {
-		input    int
-		expected int
-	}{
-		// negative test cases
-		{-1, 0},
-		// lesser than current default
-		{0, 0},
-		{64, 64},
-		// Greater than current default
-		{987 * 1024, 987 * 1024},
-	}
-	for _, tst := range tests {
-		ChangeMaxPayloadSize(tst.input)
-		actual := len(payload)
-		if len(payload) != tst.expected {
-			t.Errorf("Got %d, expected %d for ChangeMaxPayloadSize(%d)", actual, tst.expected, tst.input)
-		}
-	}
-}
-
 func TestAddHTTPS(t *testing.T) {
 	var tests = []struct {
 		input    string
diff --git a/fnet/network_test.go b/fnet/network_test.go
index cdfd2a4..cee0ee2 100644
--- a/fnet/network_test.go
+++ b/fnet/network_test.go
@@ -213,6 +213,48 @@ func TestJoinHostAndPort(t *testing.T) {
 	}
 }
 
+func TestChangeMaxPayloadSize(t *testing.T) {
+	var tests = []struct {
+		input    int
+		expected int
+	}{
+		// negative test cases
+		{-1, 0},
+		// lesser than current default
+		{0, 0},
+		{64, 64},
+		// Greater than current default
+		{987 * 1024, 987 * 1024},
+	}
+	for _, tst := range tests {
+		ChangeMaxPayloadSize(tst.input)
+		actual := len(Payload)
+		if len(Payload) != tst.expected {
+			t.Errorf("Got %d, expected %d for ChangeMaxPayloadSize(%d)", actual, tst.expected, tst.input)
+		}
+	}
+}
+
+func TestValidatePayloadSize(t *testing.T) {
+	ChangeMaxPayloadSize(256 * 1024)
+	var tests = []struct {
+		input    int
+		expected int
+	}{
+		{257 * 1024, MaxPayloadSize},
+		{10, 10},
+		{0, 0},
+		{-1, 0},
+	}
+	for _, test := range tests {
+		size := test.input
+		ValidatePayloadSize(&size)
+		if size != test.expected {
+			t.Errorf("Got %d, expected %d for ValidatePayloadSize(%d)", size, test.expected, test.input)
+		}
+	}
+}
+
 // --- max logging for tests
 
 func init() {