feat: enhance about ip revise logic

cmd/register.go

@@ -34,8 +34,9 @@ goc register [flags]
 `,
 	Run: func(cmd *cobra.Command, args []string) {
 		s := cover.ServiceUnderTest{
-			Name:    name,
-			Address: address,
+			Name:     name,
+			Address:  address,
+			IPRevise: ipRevise,
 		}
 		res, err := cover.NewWorker(center).RegisterService(s)
 		if err != nil {
@@ -46,14 +47,16 @@ goc register [flags]
 }
 
 var (
-	name    string
-	address string
+	name     string
+	address  string
+	ipRevise string
 )
 
 func init() {
 	registerCmd.Flags().StringVarP(&center, "center", "", "http://127.0.0.1:7777", "cover profile host center")
 	registerCmd.Flags().StringVarP(&name, "name", "n", "", "service name")
 	registerCmd.Flags().StringVarP(&address, "address", "a", "", "service address")
+	registerCmd.Flags().StringVarP(&ipRevise, "ip_revise", "", "true", "whether to do ip revise during registering")
 	registerCmd.MarkFlagRequired("name")
 	registerCmd.MarkFlagRequired("address")
 	rootCmd.AddCommand(registerCmd)

cmd/server.go

@@ -42,17 +42,17 @@ goc server --port=localhost:8080
 		if err != nil {
 			log.Fatalf("New file based server failed, err: %v", err)
 		}
-		server.IPRevise = iprevise
+		server.IPRevise = IPRevise
 		server.Run(port)
 	},
 }
 
 var port, localPersistence string
-var iprevise bool
+var IPRevise bool
 
 func init() {
 	serverCmd.Flags().StringVarP(&port, "port", "", ":7777", "listen port to start a coverage host center")
 	serverCmd.Flags().StringVarP(&localPersistence, "local-persistence", "", "_svrs_address.txt", "the file to save services address information")
-	serverCmd.Flags().BoolVarP(&iprevise, "ip_revise", "", true, "setting the network type(default:regist server use proxy or under nat、same network ect,direct:use register request parm)")
+	serverCmd.Flags().BoolVarP(&IPRevise, "ip_revise", "", true, "whether to do ip revise during registering. Recommend to set this as false if under NAT or Proxy environment")
 	rootCmd.AddCommand(serverCmd)
 }

pkg/cover/client.go

@@ -79,7 +79,7 @@ func (c *client) RegisterService(srv ServiceUnderTest) ([]byte, error) {
 	if strings.TrimSpace(srv.Name) == "" {
 		return nil, fmt.Errorf("invalid service name")
 	}
-	u := fmt.Sprintf("%s%s?name=%s&address=%s", c.Host, CoverRegisterServiceAPI, srv.Name, srv.Address)
+	u := fmt.Sprintf("%s%s?name=%s&address=%s&ip_revise=%s", c.Host, CoverRegisterServiceAPI, srv.Name, srv.Address, srv.IPRevise)
 	_, res, err := c.do("POST", u, "", nil)
 	return res, err
 }

pkg/cover/server.go

@@ -27,6 +27,7 @@ import (
 	"os"
 	"regexp"
 	"strconv"
+	"strings"
 
 	"github.com/gin-gonic/gin"
 	log "github.com/sirupsen/logrus"
@@ -102,7 +103,7 @@ func (s *server) Route(w io.Writer) *gin.Engine {
 type ServiceUnderTest struct {
 	Name     string `form:"name" json:"name" binding:"required"`
 	Address  string `form:"address" json:"address" binding:"required"`
-	IPRevise string `form:"iprevise" json:"-"`
+	IPRevise string `form:"ip_revise" json:"ip_revise" binding:"-"` // whether to do ip revise during registering
 }
 
 // ProfileParam is param of profile API
@@ -126,59 +127,58 @@ func (s *server) registerService(c *gin.Context) {
 		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
 		return
 	}
-	if service.IPRevise == "" {
-		service.IPRevise = strconv.FormatBool(s.IPRevise)
-	}
-	isrevise, err := strconv.ParseBool(service.IPRevise)
-	if err != nil {
-		isrevise = s.IPRevise
-	}
+
 	u, err := url.Parse(service.Address)
 	if err != nil {
-		c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+		c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("url.Parse %s failed: %s", service.Address, err.Error())})
 		return
 	}
 	if u.Scheme != "https" && u.Scheme != "http" {
 		c.JSON(http.StatusBadRequest, gin.H{"error": "unsupport schema"})
 		return
-	} else if u.Path != "" {
-		c.JSON(http.StatusBadRequest, gin.H{"error": "uri path must empty"})
+	}
+	if u.Host == "" {
+		c.JSON(http.StatusBadRequest, gin.H{"error": "empty host"})
 		return
 	}
-	if !isrevise {
-		if u.Host == "" {
-			c.JSON(http.StatusBadRequest, gin.H{"error": "address is empty"})
-			return
-		}
-		if u.Hostname() == "" {
-			c.JSON(http.StatusBadRequest, gin.H{"error": "empty host name"})
+	host, port, err := net.SplitHostPort(u.Host)
+	if err != nil {
+		if strings.Contains(err.Error(), "missing port in address") {
+			// valid scenario, keep going
+			host = u.Host
+		} else {
+			c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("net.SplitHostPort %s failed: %s", u.Host, err.Error())})
 			return
 		}
-	} else {
-		host := u.Hostname()
-		port := u.Port()
-		if host == "" {
-			host = c.ClientIP()
-		}
-		if port == "" {
-			port = "80"
-		}
-		u.Host = fmt.Sprintf("%s:%s", host, port)
-		host, port, err := net.SplitHostPort(u.Host)
+	}
+
+	var doIPRevise bool
+	// Prefer user's decision first.
+	if service.IPRevise != "" {
+		doIPRevise, err = strconv.ParseBool(service.IPRevise)
 		if err != nil {
-			c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
+			c.JSON(http.StatusBadRequest, gin.H{"error": fmt.Sprintf("strconv.ParseBool %s failed: %s", service.IPRevise, err.Error())})
 			return
 		}
+	} else {
+		doIPRevise = s.IPRevise
+	}
 
+	if doIPRevise {
 		realIP := c.ClientIP()
 		// only for IPV4
 		// refer: https://github.com/qiniu/goc/issues/177
 		if net.ParseIP(realIP).To4() != nil && host != realIP {
 			log.Printf("the registered host %s of service %s is different with the real one %s, here we choose the real one", service.Name, host, realIP)
-			service.Address = fmt.Sprintf("%s://%s:%s", u.Scheme, realIP, port)
+			host = realIP
 		}
 	}
 
+	service.Address = fmt.Sprintf("%s://%s", u.Scheme, host)
+	if port != "" {
+		service.Address = fmt.Sprintf("%s:%s", service.Address, port)
+	}
+
 	address := s.Store.Get(service.Name)
 	if !contains(address, service.Address) {
 		if err := s.Store.Add(service); err != nil && err != ErrServiceAlreadyRegistered {

pkg/cover/server_test.go

@@ -9,7 +9,6 @@ import (
 	"net/url"
 	"os"
 	"reflect"
-	"strconv"
 	"strings"
 	"testing"
 
@@ -166,13 +165,12 @@ func TestRegisterService(t *testing.T) {
 	// regist with unempty path
 	data = url.Values{}
 	data.Set("name", "aaa")
-	data.Set("address", "http://127.0.0.1:21/")
+	data.Set("address", "http://127.0.0.1:21/") // valid scenario, the final stored address will be http://127.0.0.0.1:21
 	w = httptest.NewRecorder()
 	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	router.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "uri path must empty")
+	assert.Equal(t, http.StatusOK, w.Code)
 
 	// regist with empty host(in direct mode,empty must fail,default is success)
 	// in default,use request realhost as host,use 80 as port
@@ -185,32 +183,20 @@ func TestRegisterService(t *testing.T) {
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	router.ServeHTTP(w, req)
 	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "address is empty")
-
-	data = url.Values{}
-	data.Set("name", "aaa")
-	data.Set("address", "http://:8080")
-	w = httptest.NewRecorder()
-	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
-	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
-	router.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusBadRequest, w.Code)
-	assert.Contains(t, w.Body.String(), "empty host name")
+	assert.Contains(t, w.Body.String(), "empty host")
 
 	data = url.Values{}
 	data.Set("name", "aaa")
-	data.Set("address", "http://127.0.0.1")
+	data.Set("address", "http://:8080") //valid scenario, be consistent with curl
 	w = httptest.NewRecorder()
 	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	router.ServeHTTP(w, req)
 	assert.Equal(t, http.StatusOK, w.Code)
 
-	//change network type
 	data = url.Values{}
 	data.Set("name", "aaa")
-	data.Set("address", "http://")
-	data.Set("iprevise", "true")
+	data.Set("address", "http://127.0.0.1")
 	w = httptest.NewRecorder()
 	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
@@ -225,7 +211,7 @@ func TestRegisterService(t *testing.T) {
 	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	router.ServeHTTP(w, req)
-	assert.Equal(t, http.StatusOK, w.Code)
+	assert.Equal(t, http.StatusBadRequest, w.Code)
 
 	data = url.Values{}
 	data.Set("name", "aaa")
@@ -248,8 +234,8 @@ func TestRegisterService(t *testing.T) {
 	// register with store failure
 	expectedS := ServiceUnderTest{
 		Name:     "foo",
-		Address:  "http://:64444", // the real IP is empty in unittest, so server will get a empty one
-		IPRevise: strconv.FormatBool(server.IPRevise),
+		Address:  "http://:8080",
+		IPRevise: "true",
 	}
 	testObj := new(MockStore)
 	testObj.On("Get", "foo").Return([]string{"http://127.0.0.1:66666"})
@@ -260,7 +246,7 @@ func TestRegisterService(t *testing.T) {
 	w = httptest.NewRecorder()
 	data.Set("name", expectedS.Name)
 	data.Set("address", expectedS.Address)
-	data.Set("network", "")
+	data.Set("ip_revise", expectedS.IPRevise)
 	req, _ = http.NewRequest("POST", "/v1/cover/register", strings.NewReader(data.Encode()))
 	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
 	router.ServeHTTP(w, req)