Repository: istio/fortio
Pull request #254 (2018-06-02 16:39:15)

Move some uihandler methods to another files and add their test.
Two uihandler methods are moved and their tests are added.

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>

olimpias: Hi @ldemailly . I moved AddHttps to http_utils file and SetHostAndPort method to network file. Also added their test methods. I think there is a bug on SetHostPort. You have asked it in closed PR. The problem, even I add ip address to its input, the result returns with localhost. For example, I input 8080, 192.168.31.50:8080 to SetHostAndPort method. It returns localhost:8080.  Please let me know, which methods should I move to http_utils in the uihandler.go file. Thanks 
codecov[bot]: # [Codecov](https://codecov.io/gh/istio/fortio/pull/254?src=pr&el=h1) Report
> Merging [#254](https://codecov.io/gh/istio/fortio/pull/254?src=pr&el=desc) into [master](https://codecov.io/gh/istio/fortio/commit/cbbe3831bfa2aa0e0943b9d64c3774732717288a?src=pr&el=desc) will **increase** coverage by `0.3%`.
> The diff coverage is `100%`.

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

```diff
@@           Coverage Diff            @@
##           master    #254     +/-   ##
========================================
+ Coverage    90.4%   90.8%   +0.3%     
========================================
  Files          10      10             
  Lines        1894    1910     +16     
========================================
+ Hits         1713    1734     +21     
+ Misses        116     113      -3     
+ Partials       65      63      -2
```


| [Impacted Files](https://codecov.io/gh/istio/fortio/pull/254?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [fnet/network.go](https://codecov.io/gh/istio/fortio/pull/254/diff?src=pr&el=tree#diff-Zm5ldC9uZXR3b3JrLmdv) | `94.9% <100%> (+0.6%)` | :arrow_up: |
| [fhttp/http\_utils.go](https://codecov.io/gh/istio/fortio/pull/254/diff?src=pr&el=tree#diff-Zmh0dHAvaHR0cF91dGlscy5nbw==) | `100% <100%> (ø)` | :arrow_up: |
| [log/logger.go](https://codecov.io/gh/istio/fortio/pull/254/diff?src=pr&el=tree#diff-bG9nL2xvZ2dlci5nbw==) | `100% <0%> (ø)` | :arrow_up: |
| [fgrpc/pingsrv.go](https://codecov.io/gh/istio/fortio/pull/254/diff?src=pr&el=tree#diff-ZmdycGMvcGluZ3Nydi5nbw==) | `89.4% <0%> (+1.3%)` | :arrow_up: |

------

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

ldemailly: linters work now:

fhttp/http_utils.go:458:1:warning: comment on exported function AddHTTPS should be of the form "AddHTTPS ..." (golint)



Patch:
diff --git a/fhttp/http_utils.go b/fhttp/http_utils.go
index 774d41a..c2bd166 100644
--- a/fhttp/http_utils.go
+++ b/fhttp/http_utils.go
@@ -449,3 +449,27 @@ func NewHTMLEscapeWriter(w io.Writer) io.Writer {
 func OnBehalfOf(o *HTTPOptions, r *http.Request) {
 	_ = o.AddAndValidateExtraHeader("X-On-Behalf-Of: " + r.RemoteAddr)
 }
+
+const (
+	httpPrefix  = "http://"
+	httpsPrefix = "https://"
+)
+
+// AddHTTPS replaces "http://" in url with "https://" or prepends "https://"
+// if url does not contain prefix "http://".
+func AddHTTPS(url string) string {
+	if len(url) > len(httpPrefix) {
+		if strings.EqualFold(url[:len(httpPrefix)], httpPrefix) {
+			log.Infof("Replacing http scheme with https for url: %s", url)
+			return httpsPrefix + url[len(httpPrefix):]
+		}
+		// returns url with normalized lowercase https prefix
+		if strings.EqualFold(url[:len(httpsPrefix)], httpsPrefix) {
+			return httpsPrefix + url[len(httpsPrefix):]
+		}
+	}
+	// url must not contain any prefix, so add https prefix
+	log.Infof("Prepending https:// to url: %s", url)
+	return httpsPrefix + url
+
+}
diff --git a/fnet/network.go b/fnet/network.go
index 792c31f..d0ff951 100644
--- a/fnet/network.go
+++ b/fnet/network.go
@@ -160,3 +160,13 @@ func Proxy(port string, dest *net.TCPAddr) *net.TCPAddr {
 func ProxyToDestination(listenPort string, destination string) *net.TCPAddr {
 	return Proxy(listenPort, ResolveDestination(destination))
 }
+
+// NormalizeHostPort generates host:port string for the address or uses localhost instead of [::]
+// when the original port binding input didn't specify an address
+func NormalizeHostPort(inputPort string, addr *net.TCPAddr) string {
+	urlHostPort := addr.String()
+	if strings.HasPrefix(inputPort, ":") || !strings.Contains(inputPort, ":") {
+		urlHostPort = fmt.Sprintf("localhost:%d", addr.Port)
+	}
+	return urlHostPort
+}
diff --git a/ui/uihandler.go b/ui/uihandler.go
index 3bb52e7..ddeacf6 100644
--- a/ui/uihandler.go
+++ b/ui/uihandler.go
@@ -17,7 +17,6 @@ package ui // import "istio.io/fortio/ui"
 
 import (
 	"bytes"
-	"net"
 	// md5 is mandated, not our choice
 	"crypto/md5" // nolint: gas
 	"encoding/base64"
@@ -40,6 +39,7 @@ import (
 
 	"istio.io/fortio/fgrpc"
 	"istio.io/fortio/fhttp"
+	"istio.io/fortio/fnet"
 	"istio.io/fortio/log"
 	"istio.io/fortio/periodic"
 	"istio.io/fortio/stats"
@@ -297,7 +297,7 @@ func Handler(w http.ResponseWriter, r *http.Request) {
 				Delay:         grpcPingDelay,
 			}
 			if grpcSecure {
-				o.Destination = addHTTPS(url)
+				o.Destination = fhttp.AddHTTPS(url)
 			}
 			res, err = fgrpc.RunGRPCTest(&o)
 		} else {
@@ -914,7 +914,7 @@ func Serve(baseurl, port, debugpath, uipath, staticRsrcDir string, datadir strin
 		fs := http.FileServer(http.Dir(dataDir))
 		mux.Handle(uiPath+"data/", LogAndFilterDataRequest(http.StripPrefix(uiPath+"data", fs)))
 	}
-	setHostAndPort(port, addr)
+	urlHostPort = fnet.NormalizeHostPort(port, addr)
 	uiMsg := fmt.Sprintf("UI started - visit:\nhttp://%s%s", urlHostPort, uiPath)
 	if !strings.Contains(port, ":") {
 		uiMsg += "   (or any host/ip reachable on this server)"
@@ -935,7 +935,7 @@ func Report(baseurl, port, staticRsrcDir string, datadir string) bool {
 	if addr == nil {
 		return false
 	}
-	setHostAndPort(port, addr)
+	urlHostPort = fnet.NormalizeHostPort(port, addr)
 	uiMsg := fmt.Sprintf("Browse only UI started - visit:\nhttp://%s/", urlHostPort)
 	if !strings.Contains(port, ":") {
 		uiMsg += "   (or any host/ip reachable on this server)"
@@ -961,29 +961,3 @@ func Report(baseurl, port, staticRsrcDir string, datadir string) bool {
 	mux.Handle(uiPath+"data/", LogAndFilterDataRequest(http.StripPrefix(uiPath+"data", fsd)))
 	return true
 }
-
-// setHostAndPort takes the bind address and sets urlHostPort as bind-address:port
-// unless the original input port didn't specify an address (starts with : or is just a port,
-// no colon) in which case it uses localhost:port
-func setHostAndPort(inputPort string, addr *net.TCPAddr) {
-	urlHostPort = addr.String()
-	if strings.HasPrefix(inputPort, ":") || !strings.Contains(inputPort, ":") {
-		urlHostPort = fmt.Sprintf("localhost:%d", addr.Port)
-	}
-}
-
-// addHTTPS replaces "http://" in url with "https://" or prepends "https://"
-// if url does not contain prefix "http://".
-func addHTTPS(url string) string {
-	if strings.HasPrefix(url, "http://") {
-		log.Infof("Replacing http scheme with https for url: %s", url)
-		return "https://" + strings.TrimPrefix(url, "http://")
-	}
-	// return url unchanged since it already has "https://"
-	if strings.HasPrefix(url, "https://") {
-		return url
-	}
-	// url must not contain any prefix, so add https prefix
-	log.Infof("Prepending https:// to url: %s", url)
-	return "https://" + url
-}


Test patch:
diff --git a/fhttp/http_test.go b/fhttp/http_test.go
index 35bebba..760440c 100644
--- a/fhttp/http_test.go
+++ b/fhttp/http_test.go
@@ -941,6 +941,33 @@ func TestChangeMaxPayloadSize(t *testing.T) {
 	}
 }
 
+func TestAddHTTPS(t *testing.T) {
+	var tests = []struct {
+		input    string
+		expected string
+	}{
+		{"foo", "https://foo"},
+		{"foo.com", "https://foo.com"},
+		{"http://foo.com", "https://foo.com"},
+		{"https://foo.com", "https://foo.com"},
+		{"hTTps://foo.com", "https://foo.com"},
+		{"hTTp://foo.com", "https://foo.com"},
+		{"http://fOO.com", "https://fOO.com"},
+		{"hTTp://foo.com/about.html", "https://foo.com/about.html"},
+		{"hTTp://foo.com/ABOUT.html", "https://foo.com/ABOUT.html"},
+		{"hTTp://foo.com/BaR", "https://foo.com/BaR"},
+		{"https://foo.com/BaR", "https://foo.com/BaR"},
+		{"http", "https://http"},
+	}
+
+	for _, test := range tests {
+		output := AddHTTPS(test.input)
+		if output != test.expected {
+			t.Errorf("%s is received but %s was expected", output, test.expected)
+		}
+	}
+}
+
 // --- for bench mark/comparison
 
 func asciiFold0(str string) []byte {
diff --git a/fnet/network_test.go b/fnet/network_test.go
index 710ad9c..cdfd2a4 100644
--- a/fnet/network_test.go
+++ b/fnet/network_test.go
@@ -169,6 +169,50 @@ func TestResolveIpV6(t *testing.T) {
 	}
 }
 
+func TestJoinHostAndPort(t *testing.T) {
+	var tests = []struct {
+		inputPort string
+		addr      *net.TCPAddr
+		expected  string
+	}{
+		{"8080", &net.TCPAddr{
+			IP:   []byte{192, 168, 2, 3},
+			Port: 8081,
+		}, "localhost:8081"},
+		{"192.168.30.14:8081", &net.TCPAddr{
+			IP:   []byte{192, 168, 30, 15},
+			Port: 8080,
+		}, "192.168.30.15:8080"},
+		{":8080",
+			&net.TCPAddr{
+				IP:   []byte{0, 0, 0, 1},
+				Port: 8080,
+			},
+			"localhost:8080"},
+		{"",
+			&net.TCPAddr{
+				IP:   []byte{192, 168, 30, 14},
+				Port: 9090,
+			}, "localhost:9090"},
+		{"http",
+			&net.TCPAddr{
+				IP:   []byte{192, 168, 30, 14},
+				Port: 9090,
+			}, "localhost:9090"},
+		{"192.168.30.14:9090",
+			&net.TCPAddr{
+				IP:   []byte{192, 168, 30, 14},
+				Port: 9090,
+			}, "192.168.30.14:9090"},
+	}
+	for _, test := range tests {
+		urlHostPort := NormalizeHostPort(test.inputPort, test.addr)
+		if urlHostPort != test.expected {
+			t.Errorf("%s is received  but %s was expected", urlHostPort, test.expected)
+		}
+	}
+}
+
 // --- max logging for tests
 
 func init() {