Repository: fortio/fortio
Pull request #412 (2020-10-08 18:42:26)

code cleanup first, same functionality + ability to not have to speci…
…fy http:// also avoids double / in outgoing urls

somehow linter crashed silently on previous PR


codeclimate[bot]: Code Climate has analyzed commit 951b52cf and detected **0 issues** on this pull request.

View more on [Code Climate](https://codeclimate.com/github/fortio/fortio/pull/412).

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

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

```diff
@@           Coverage Diff            @@
##           master    #412     +/-   ##
========================================
- Coverage    86.5%   86.5%   -0.0%     
========================================
  Files          25      25             
  Lines        2620    2594     -26     
========================================
- Hits         2266    2243     -23     
+ Misses        236     233      -3     
  Partials      118     118             
```


| [Impacted Files](https://codecov.io/gh/fortio/fortio/pull/412?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [fhttp/http\_forwarder.go](https://codecov.io/gh/fortio/fortio/pull/412/diff?src=pr&el=tree#diff-Zmh0dHAvaHR0cF9mb3J3YXJkZXIuZ28=) | `90.2% <100.0%> (+1.5%)` | :arrow_up: |
| [fgrpc/pingsrv.go](https://codecov.io/gh/fortio/fortio/pull/412/diff?src=pr&el=tree#diff-ZmdycGMvcGluZ3Nydi5nbw==) | `85.7% <0.0%> (-2.3%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/fortio/fortio/pull/412?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/fortio/fortio/pull/412?src=pr&el=footer). Last update [7e86560...951b52c](https://codecov.io/gh/fortio/fortio/pull/412?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/fhttp/http_forwarder.go b/fhttp/http_forwarder.go
index 27c27c5..f2e5184 100644
--- a/fhttp/http_forwarder.go
+++ b/fhttp/http_forwarder.go
@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+// Tee off traffic
+
 package fhttp // import "fortio.org/fortio/fhttp"
 
 import (
@@ -20,12 +22,22 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"net/textproto"
 	"strconv"
 	"strings"
 
 	"fortio.org/fortio/fnet"
 	"fortio.org/fortio/log"
-) // Tee off traffic
+)
+
+var (
+	// EnvoyRequestID is the header set by envoy and we need to propagate for distributed tracing.
+	EnvoyRequestID = textproto.CanonicalMIMEHeaderKey("x-request-id")
+	// TraceHeader is the single aggregated open tracing header to propagate when present.
+	TraceHeader = textproto.CanonicalMIMEHeaderKey("b3")
+	// TraceHeadersPrefix is the prefix for the multi header version of open zipkin.
+	TraceHeadersPrefix = textproto.CanonicalMIMEHeaderKey("x-b3-")
+)
 
 // TargetConf is the structure to configure one of the multiple targets for MultiServer.
 type TargetConf struct {
@@ -43,6 +55,46 @@ type MultiServerConfig struct {
 	client *http.Client
 }
 
+func makeMirrorRequest(baseURL string, r *http.Request, data []byte) *http.Request {
+	url := baseURL + r.RequestURI
+	bodyReader := ioutil.NopCloser(bytes.NewReader(data))
+	req, err := http.NewRequestWithContext(r.Context(), r.Method, url, bodyReader)
+	if err != nil {
+		log.Warnf("new mirror request error for %q: %v", url, err)
+		return nil
+	}
+	// Copy all headers
+	// Host header is not in Header so safe to copy
+	copyHeaders(req, r, true)
+	return req
+}
+
+func copyHeaders(req, r *http.Request, all bool) {
+	// Copy only trace headers unless all is true.
+	for k, v := range r.Header {
+		if all || k == EnvoyRequestID || k == TraceHeader || strings.HasPrefix(k, TraceHeadersPrefix) {
+			for _, vv := range v {
+				req.Header.Add(k, vv)
+			}
+			log.Debugf("Adding header %q = %q", k, v)
+		} else {
+			log.Debugf("Skipping header %q", k)
+		}
+	}
+}
+
+func makeSimpleRequest(url string, r *http.Request) *http.Request {
+	req, err := http.NewRequestWithContext(r.Context(), "GET", url, nil)
+	if err != nil {
+		log.Warnf("new request error for %q: %v", url, err)
+		return nil
+	}
+	// Copy only trace headers
+	copyHeaders(req, r, false)
+	req.Header.Add("User-Agent", userAgent)
+	return req
+}
+
 // TeeHandler handles teeing off traffic.
 func (mcfg *MultiServerConfig) TeeHandler(w http.ResponseWriter, r *http.Request) {
 	if log.LogVerbose() {
@@ -56,44 +108,17 @@ func (mcfg *MultiServerConfig) TeeHandler(w http.ResponseWriter, r *http.Request
 		return
 	}
 	for i, t := range mcfg.Targets {
-		url := t.Destination
 		var req *http.Request
-		var err error
 		if t.MirrorOrigin {
-			url = url + r.RequestURI
-			bodyReader := ioutil.NopCloser(bytes.NewReader(data))
-			req, err = http.NewRequest(r.Method, url, bodyReader)
-			if err != nil {
-				log.Warnf("new mirror request error for %q: %v", url, err)
-				continue
-			}
-			// Copy all headers
-			// Host header is not in Header so safe to copy
-			for k, v := range r.Header {
-				log.Debugf("Adding (all) headers - %q = %q", k, v)
-				for _, vv := range v {
-					req.Header.Add(k, vv)
-				}
-			}
+			req = makeMirrorRequest(t.Destination, r, data)
 		} else {
-			req, err = http.NewRequest("GET", url, nil)
-			if err != nil {
-				log.Warnf("new request error for %q: %v", url, err)
-				continue
-			}
-			// Copy only trace headers
-			for k, v := range r.Header {
-				lk := strings.ToLower(k)
-				if lk == "x-request-id" || lk == "b3" || strings.HasPrefix(lk, "x-b3-") {
-					for _, vv := range v {
-						req.Header.Add(k, vv)
-					}
-					log.Debugf("Adding header %q = %q", k, v)
-				} else {
-					log.Debugf("Skipping header %q", k)
-				}
-			}
+			req = makeSimpleRequest(t.Destination, r)
 		}
+		if req == nil {
+			// error already logged
+			continue
+		}
+		url := req.URL.String()
 		req.Header.Add("X-On-Behalf-Of", r.RemoteAddr)
 		req.Header.Add("X-Fortio-Multi-ID", strconv.Itoa(i+1))
 		log.LogVf("Going to %s", url)
@@ -105,8 +130,8 @@ func (mcfg *MultiServerConfig) TeeHandler(w http.ResponseWriter, r *http.Request
 				w.WriteHeader(http.StatusServiceUnavailable)
 				first = false
 			}
-			w.Write([]byte(msg))
-			w.Write([]byte("\n"))
+			_, _ = w.Write([]byte(msg))
+			_, _ = w.Write([]byte("\n"))
 			continue
 		}
 		if first {
@@ -123,7 +148,7 @@ func (mcfg *MultiServerConfig) TeeHandler(w http.ResponseWriter, r *http.Request
 	r.Body.Close()
 }
 
-// createClient http client for connection reuse
+// createClient http client for connection reuse.
 func createClient() *http.Client {
 	client := &http.Client{
 		Transport: &http.Transport{
@@ -154,6 +179,16 @@ func MultiServer(port string, cfg *MultiServerConfig) (*http.ServeMux, net.Addr)
 		cfg.Name = "Multi on " + aStr
 	}
 	cfg.client = createClient()
+	for i := range cfg.Targets {
+		t := &cfg.Targets[i]
+		if t.MirrorOrigin {
+			t.Destination = strings.TrimSuffix(t.Destination, "/") // remove trailing / because we will concatenate the request URI
+		}
+		if !strings.HasPrefix(t.Destination, "https://") && !strings.HasPrefix(t.Destination, "http://") {
+			log.Infof("Assuming http:// on missing scheme for '%s'", t.Destination)
+			t.Destination = "http://" + t.Destination
+		}
+	}
 	log.Infof("Multi-server on %s running with %+v", aStr, cfg)
 	mux.HandleFunc("/", cfg.TeeHandler)
 	return mux, addr
diff --git a/fortio_main.go b/fortio_main.go
index d343962..179afaa 100644
--- a/fortio_main.go
+++ b/fortio_main.go
@@ -47,6 +47,7 @@ type proxiesFlagList struct {
 func (f *proxiesFlagList) String() string {
 	return ""
 }
+
 func (f *proxiesFlagList) Set(value string) error {
 	proxies = append(proxies, value)
 	return nil
@@ -54,19 +55,20 @@ func (f *proxiesFlagList) Set(value string) error {
 
 // -- End of functions for -P support.
 
-// -- Same for -M
+// -- Same for -M.
 type httpMultiFlagList struct {
 }
 
 func (f *httpMultiFlagList) String() string {
 	return ""
 }
+
 func (f *httpMultiFlagList) Set(value string) error {
 	httpMulties = append(httpMulties, value)
 	return nil
 }
 
-// -- End of -M support
+// -- End of -M support.
 
 // Usage to a writer.
 func usage(w io.Writer, msgs ...interface{}) {
@@ -131,7 +133,7 @@ var (
 	dataDirFlag   = flag.String("data-dir", defaultDataDir, "`Directory` where JSON results are stored/read")
 	proxiesFlags  proxiesFlagList
 	proxies       = make([]string, 0)
-	// -M flag
+	// -M flag.
 	httpMultiFlags httpMultiFlagList
 	httpMulties    = make([]string, 0)
 
@@ -168,12 +170,13 @@ var (
 	jitterFlag = flag.Bool("jitter", false, "set to true to de-synchronize parallel clients' requests")
 	// nc mode flag(s).
 	ncDontStopOnCloseFlag = flag.Bool("nc-dont-stop-on-eof", false, "in netcat (nc) mode, don't abort as soon as remote side closes")
-	// Mirror origin global setting (should be per destination eventually)
+	// Mirror origin global setting (should be per destination eventually).
 	mirrorOriginFlag = flag.Bool("multi-mirror-origin", true, "Mirror the request url to the target for multi proxies (-M)")
 )
 
 func main() {
-	flag.Var(&proxiesFlags, "P", "Tcp proxies to run, e.g -P \"localport1 dest_host1:dest_port1\" -P \"[::1]:0 www.google.com:443\" ...")
+	flag.Var(&proxiesFlags, "P",
+		"Tcp proxies to run, e.g -P \"localport1 dest_host1:dest_port1\" -P \"[::1]:0 www.google.com:443\" ...")
 	flag.Var(&httpMultiFlags, "M", "Http multi proxy to run, e.g -M \"localport1 baseDestURL1 baseDestURL2\" -M ...")
 	bincommon.SharedMain(usage)
 	if len(os.Args) < 2 {
@@ -253,19 +256,23 @@ func main() {
 		if confDir == "" {
 			log.Infof("Note: not using dynamic flag watching (use -config to set watch directory)")
 		}
-		// To get a start time log/timestamp in the logs
-		log.Infof("All fortio %s servers started!", version.Long())
-		d := *syncIntervalFlag
-		if sync != "" && d > 0 {
-			log.Infof("Will re-sync data dir every %s", d)
-			ticker := time.NewTicker(d)
-			defer ticker.Stop()
-			for range ticker.C {
-				ui.Sync(os.Stdout, sync, *dataDirFlag)
-			}
-		} else {
-			select {}
+		serverLoop(sync)
+	}
+}
+
+func serverLoop(sync string) {
+	// To get a start time log/timestamp in the logs
+	log.Infof("All fortio %s servers started!", version.Long())
+	d := *syncIntervalFlag
+	if sync != "" && d > 0 {
+		log.Infof("Will re-sync data dir every %s", d)
+		ticker := time.NewTicker(d)
+		defer ticker.Stop()
+		for range ticker.C {
+			ui.Sync(os.Stdout, sync, *dataDirFlag)
 		}
+	} else {
+		select {}
 	}
 }
 
diff --git a/version/version.go b/version/version.go
index 3c69a47..da0000b 100644
--- a/version/version.go
+++ b/version/version.go
@@ -24,7 +24,7 @@ import (
 const (
 	major = 1
 	minor = 9
-	patch = 0
+	patch = 1
 
 	debug = false // turn on to debug init()
 )


Test patch:
diff --git a/fhttp/http_forwarder_test.go b/fhttp/http_forwarder_test.go
index 3b11655..71420c2 100644
--- a/fhttp/http_forwarder_test.go
+++ b/fhttp/http_forwarder_test.go
@@ -29,9 +29,9 @@ func init() {
 
 func TestMultiProxy(t *testing.T) {
 	_, debugAddr := ServeTCP("0", "/debug")
-	urlBase := fmt.Sprintf("http://localhost:%d", debugAddr.Port)
+	urlBase := fmt.Sprintf("localhost:%d/", debugAddr.Port)
 	mcfg := MultiServerConfig{}
-	mcfg.Targets = []TargetConf{{Destination: urlBase, MirrorOrigin: true}, {Destination: urlBase + "/echo?status=555"}}
+	mcfg.Targets = []TargetConf{{Destination: urlBase, MirrorOrigin: true}, {Destination: urlBase + "echo?status=555"}}
 	_, multiAddr := MultiServer("0", &mcfg)
 	url := fmt.Sprintf("http://%s/debug", multiAddr)
 	payload := "A test payload"
@@ -56,7 +56,11 @@ func TestMultiProxy(t *testing.T) {
 func TestMultiProxyErrors(t *testing.T) {
 	mcfg := MultiServerConfig{}
 	// No scheme in url to cause error
-	mcfg.Targets = []TargetConf{{Destination: "\001doesntexist.fortio.org:2435/foo"}, {Destination: "\001doesntexist.fortio.org:2435/foo", MirrorOrigin: true},  {Destination: "doesntexist.fortio.org:2435/foo"}}
+	mcfg.Targets = []TargetConf{
+		{Destination: "\001doesntexist.fortio.org:2435/foo"},
+		{Destination: "\001doesntexist.fortio.org:2435/foo", MirrorOrigin: true},
+		{Destination: "doesntexist.fortio.org:2435/foo"},
+	}
 	_, multiAddr := MultiServer("0", &mcfg)
 	url := fmt.Sprintf("http://%s/debug", multiAddr)
 	opts := HTTPOptions{URL: url}