Repository: go-kratos/kratos
Pull request #2072 (2022-06-01 12:43:22)

feat: make secure url to grpcs://127.0.0.1/
<!--
🎉 Thanks for sending a pull request to Kratos! Here are some tips for you:

1. If this is your first time contributing to Kratos, please read our contribution guide: https://go-kratos.dev/en/docs/community/contribution/
2. Ensure you have added or ran the appropriate tests and lint for your PR, please use `make lint` and `make test` before filing your PR, use `make clean` to tidy your go mod.
3. If the PR is unfinished, you may need to mark it as a WIP(Work In Progress) PR or draft PR
4. Please use a semantic commits format title, such as `<type>[optional scope]: <description>`, see: https://go-kratos.dev/docs/community/contribution#type
-->

#### Description (what this PR does / why we need it):
<!--
* The description should include the motivation for this PR or contrast this with previous behavior
-->


#### Which issue(s) this PR fixes (resolves / be part of):
<!--
* Automatically closes linked issue when PR is merged.
* If your PR is not fully resolved the issue, please use `part of #<issue number>` instead.

Usage: `fixes/resolves #<issue number>`, or `fixes/resolves (paste link of issue)`.
-->


#### Other special notes for the reviewers:
<!--
* Somethings that need extra attention for the reviewers
* Some additional notes, TODO list, etc.
-->


feat: #2071

codecov-commenter: # [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) Report
> Merging [#2072](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (c44f659) into [main](https://codecov.io/gh/go-kratos/kratos/commit/97c103a39562edc0b6d9e3909a6c0103a8862f47?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (97c103a) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

> :exclamation: Current head c44f659 differs from pull request most recent head 57b821c. Consider uploading reports for the commit 57b821c to get more accurate results

```diff
@@            Coverage Diff             @@
##             main    #2072      +/-   ##
==========================================
- Coverage   79.10%   79.08%   -0.02%     
==========================================
  Files          82       82              
  Lines        3656     3658       +2     
==========================================
+ Hits         2892     2893       +1     
- Misses        554      555       +1     
  Partials      210      210              
```


| [Impacted Files](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) | Coverage Δ | |
|---|---|---|
| [transport/http/server.go](https://codecov.io/gh/go-kratos/kratos/pull/2072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-dHJhbnNwb3J0L2h0dHAvc2VydmVyLmdv) | `78.22% <ø> (ø)` | |
| [internal/endpoint/endpoint.go](https://codecov.io/gh/go-kratos/kratos/pull/2072/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-aW50ZXJuYWwvZW5kcG9pbnQvZW5kcG9pbnQuZ28=) | `85.00% <100.00%> (-3.89%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Last update [97c103a...57b821c](https://codecov.io/gh/go-kratos/kratos/pull/2072?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).


Patch:
diff --git a/internal/endpoint/endpoint.go b/internal/endpoint/endpoint.go
index a125d924..e4c9ef96 100644
--- a/internal/endpoint/endpoint.go
+++ b/internal/endpoint/endpoint.go
@@ -6,29 +6,42 @@ import (
 )
 
 // NewEndpoint new an Endpoint URL.
-func NewEndpoint(scheme, host string, isSecure bool) *url.URL {
-	var query string
-	if isSecure {
-		query = "isSecure=true"
-	}
-	return &url.URL{Scheme: scheme, Host: host, RawQuery: query}
+func NewEndpoint(scheme, host string) *url.URL {
+	return &url.URL{Scheme: scheme, Host: host}
 }
 
 // ParseEndpoint parses an Endpoint URL.
-func ParseEndpoint(endpoints []string, scheme string, isSecure bool) (string, error) {
+func ParseEndpoint(endpoints []string, scheme string) (string, error) {
 	for _, e := range endpoints {
 		u, err := url.Parse(e)
 		if err != nil {
 			return "", err
 		}
-		if u.Scheme == scheme && IsSecure(u) == isSecure {
+
+		// TODO: Compatibility processing
+		// Function is to convert grpc:/127.0.0.1/?isSecure=true into grpcs:/127.0.0.1/
+		// It will be deleted in about a month
+		u = legacyURLToNew(u)
+		if u.Scheme == scheme {
 			return u.Host, nil
 		}
 	}
 	return "", nil
 }
 
+func legacyURLToNew(u *url.URL) *url.URL {
+	if u.Scheme == "https" || u.Scheme == "grpcs" {
+		return u
+	}
+	if IsSecure(u) {
+		return &url.URL{Scheme: u.Scheme + "s", Host: u.Host}
+	}
+	return u
+}
+
 // IsSecure parses isSecure for Endpoint URL.
+// Note: It will be deleted after some time,
+// unified use grpcs://127.0.0.1:8080 instead of grpc://127.0.0.1:8080?isSecure=true
 func IsSecure(u *url.URL) bool {
 	ok, err := strconv.ParseBool(u.Query().Get("isSecure"))
 	if err != nil {
@@ -36,3 +49,12 @@ func IsSecure(u *url.URL) bool {
 	}
 	return ok
 }
+
+// Scheme is the scheme of endpoint url.
+// examples: scheme="http",isSecure=true get "https"
+func Scheme(scheme string, isSecure bool) string {
+	if isSecure {
+		return scheme + "s"
+	}
+	return scheme
+}
diff --git a/transport/grpc/resolver/discovery/resolver.go b/transport/grpc/resolver/discovery/resolver.go
index 770fa801..6a7f6e7e 100644
--- a/transport/grpc/resolver/discovery/resolver.go
+++ b/transport/grpc/resolver/discovery/resolver.go
@@ -49,7 +49,7 @@ func (r *discoveryResolver) update(ins []*registry.ServiceInstance) {
 	addrs := make([]resolver.Address, 0)
 	endpoints := make(map[string]struct{})
 	for _, in := range ins {
-		endpoint, err := endpoint.ParseEndpoint(in.Endpoints, "grpc", !r.insecure)
+		endpoint, err := endpoint.ParseEndpoint(in.Endpoints, endpoint.Scheme("grpc", !r.insecure))
 		if err != nil {
 			log.Errorf("[resolver] Failed to parse discovery endpoint: %v", err)
 			continue
diff --git a/transport/grpc/server.go b/transport/grpc/server.go
index 8ec76884..31c03f75 100644
--- a/transport/grpc/server.go
+++ b/transport/grpc/server.go
@@ -207,6 +207,6 @@ func (s *Server) listenAndEndpoint() error {
 		_ = s.lis.Close()
 		return err
 	}
-	s.endpoint = endpoint.NewEndpoint("grpc", addr, s.tlsConf != nil)
+	s.endpoint = endpoint.NewEndpoint(endpoint.Scheme("grpc", s.tlsConf != nil), addr)
 	return nil
 }
diff --git a/transport/http/codec.go b/transport/http/codec.go
index 79db5747..176e599a 100644
--- a/transport/http/codec.go
+++ b/transport/http/codec.go
@@ -12,6 +12,12 @@ import (
 // SupportPackageIsVersion1 These constants should not be referenced from any other code.
 const SupportPackageIsVersion1 = true
 
+// Redirector replies to the request with a redirect to url
+// which may be a path relative to the request path.
+type Redirector interface {
+	Redirect() (string, int)
+}
+
 // Request type net/http.
 type Request = http.Request
 
@@ -49,8 +55,12 @@ func DefaultRequestDecoder(r *http.Request, v interface{}) error {
 // DefaultResponseEncoder encodes the object to the HTTP response.
 func DefaultResponseEncoder(w http.ResponseWriter, r *http.Request, v interface{}) error {
 	if v == nil {
-		_, err := w.Write(nil)
-		return err
+		return nil
+	}
+	if rd, ok := v.(Redirector); ok {
+		url, code := rd.Redirect()
+		http.Redirect(w, r, url, code)
+		return nil
 	}
 	codec, _ := CodecForRequest(r, "Accept")
 	data, err := codec.Marshal(v)
diff --git a/transport/http/redirect.go b/transport/http/redirect.go
new file mode 100644
index 00000000..cb98ac09
--- /dev/null
+++ b/transport/http/redirect.go
@@ -0,0 +1,18 @@
+package http
+
+type redirect struct {
+	URL  string
+	Code int
+}
+
+func (r *redirect) Redirect() (string, int) {
+	return r.URL, r.Code
+}
+
+// NewRedirect new a redirect with url, which may be a path relative to the request path.
+// The provided code should be in the 3xx range and is usually StatusMovedPermanently, StatusFound or StatusSeeOther.
+// If the Content-Type header has not been set, Redirect sets it to "text/html; charset=utf-8" and writes a small HTML body.
+// Setting the Content-Type header to any value, including nil, disables that behavior.
+func NewRedirect(url string, code int) Redirector {
+	return &redirect{URL: url, Code: code}
+}
diff --git a/transport/http/resolver.go b/transport/http/resolver.go
index 1a4ba81c..f742c36a 100644
--- a/transport/http/resolver.go
+++ b/transport/http/resolver.go
@@ -112,7 +112,7 @@ func newResolver(ctx context.Context, discovery registry.Discovery, target *Targ
 func (r *resolver) update(services []*registry.ServiceInstance) bool {
 	nodes := make([]selector.Node, 0)
 	for _, ins := range services {
-		ept, err := endpoint.ParseEndpoint(ins.Endpoints, "http", !r.insecure)
+		ept, err := endpoint.ParseEndpoint(ins.Endpoints, endpoint.Scheme("http", !r.insecure))
 		if err != nil {
 			log.Errorf("Failed to parse (%v) discovery endpoint: %v error %v", r.target, ins.Endpoints, err)
 			continue
diff --git a/transport/http/server.go b/transport/http/server.go
index 58f3060d..c45a7269 100644
--- a/transport/http/server.go
+++ b/transport/http/server.go
@@ -224,7 +224,8 @@ func (s *Server) filter() mux.MiddlewareFunc {
 
 // Endpoint return a real address to registry endpoint.
 // examples:
-//   http://127.0.0.1:8000?isSecure=false
+//   https://127.0.0.1:8000
+//   Legacy: http://127.0.0.1:8000?isSecure=false
 func (s *Server) Endpoint() (*url.URL, error) {
 	if s.err != nil {
 		return nil, s.err
@@ -272,6 +273,6 @@ func (s *Server) listenAndEndpoint() error {
 		_ = s.lis.Close()
 		return err
 	}
-	s.endpoint = endpoint.NewEndpoint("http", addr, s.tlsConf != nil)
+	s.endpoint = endpoint.NewEndpoint(endpoint.Scheme("http", s.tlsConf != nil), addr)
 	return nil
 }


Test patch:
diff --git a/internal/endpoint/endpoint_test.go b/internal/endpoint/endpoint_test.go
index db089288..356d8fbc 100644
--- a/internal/endpoint/endpoint_test.go
+++ b/internal/endpoint/endpoint_test.go
@@ -18,17 +18,17 @@ func TestEndPoint(t *testing.T) {
 		// TODO: Add test cases.
 		{
 			name: "grpc://127.0.0.1?isSecure=false",
-			args: args{NewEndpoint("grpc", "127.0.0.1", false)},
+			args: args{&url.URL{Scheme: "grpc", Host: "127.0.0.1", RawQuery: "isSecure=false"}},
 			want: false,
 		},
 		{
 			name: "grpc://127.0.0.1?isSecure=true",
-			args: args{NewEndpoint("http", "127.0.0.1", true)},
+			args: args{&url.URL{Scheme: "grpc", Host: "127.0.0.1", RawQuery: "isSecure=true"}},
 			want: true,
 		},
 		{
 			name: "grpc://127.0.0.1",
-			args: args{NewEndpoint("grpc", "localhost", false)},
+			args: args{&url.URL{Scheme: "grpc", Host: "127.0.0.1"}},
 			want: false,
 		},
 	}
@@ -43,9 +43,8 @@ func TestEndPoint(t *testing.T) {
 
 func TestNewEndpoint(t *testing.T) {
 	type args struct {
-		scheme   string
-		host     string
-		isSecure bool
+		scheme string
+		host   string
 	}
 	tests := []struct {
 		name string
@@ -54,18 +53,23 @@ func TestNewEndpoint(t *testing.T) {
 	}{
 		{
 			name: "https://github.com/go-kratos/kratos/",
-			args: args{"https", "github.com/go-kratos/kratos/", false},
+			args: args{"https", "github.com/go-kratos/kratos/"},
 			want: &url.URL{Scheme: "https", Host: "github.com/go-kratos/kratos/"},
 		},
 		{
 			name: "https://go-kratos.dev/",
-			args: args{"https", "go-kratos.dev/", true},
-			want: &url.URL{Scheme: "https", Host: "go-kratos.dev/", RawQuery: "isSecure=true"},
+			args: args{"https", "go-kratos.dev/"},
+			want: &url.URL{Scheme: "https", Host: "go-kratos.dev/"},
+		},
+		{
+			name: "https://www.google.com/",
+			args: args{"https", "www.google.com/"},
+			want: &url.URL{Scheme: "https", Host: "www.google.com/"},
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := NewEndpoint(tt.args.scheme, tt.args.host, tt.args.isSecure); !reflect.DeepEqual(got, tt.want) {
+			if got := NewEndpoint(tt.args.scheme, tt.args.host); !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("NewEndpoint() = %v, want %v", got, tt.want)
 			}
 		})
@@ -76,7 +80,6 @@ func TestParseEndpoint(t *testing.T) {
 	type args struct {
 		endpoints []string
 		scheme    string
-		isSecure  bool
 	}
 	tests := []struct {
 		name    string
@@ -86,20 +89,46 @@ func TestParseEndpoint(t *testing.T) {
 	}{
 		{
 			name:    "kratos",
-			args:    args{endpoints: []string{"https://github.com/go-kratos/kratos?isSecure=true"}, scheme: "https", isSecure: true},
+			args:    args{endpoints: []string{"https://github.com/go-kratos/kratos"}, scheme: "https"},
 			want:    "github.com",
 			wantErr: false,
 		},
 		{
 			name:    "test",
-			args:    args{endpoints: []string{"https://go-kratos.dev/"}, scheme: "http", isSecure: true},
+			args:    args{endpoints: []string{"http://go-kratos.dev/"}, scheme: "https"},
+			want:    "",
+			wantErr: false,
+		},
+		{
+			name:    "localhost:8080",
+			args:    args{endpoints: []string{"grpcs://localhost:8080/"}, scheme: "grpcs"},
+			want:    "localhost:8080",
+			wantErr: false,
+		},
+		{
+			name:    "localhost:8081",
+			args:    args{endpoints: []string{"grpcs://localhost:8080/"}, scheme: "grpc"},
 			want:    "",
 			wantErr: false,
 		},
+
+		// Legacy
+		{
+			name:    "google",
+			args:    args{endpoints: []string{"grpc://www.google.com/?isSecure=true"}, scheme: "grpcs"},
+			want:    "www.google.com",
+			wantErr: false,
+		},
+		{
+			name:    "baidu",
+			args:    args{endpoints: []string{"http://www.baidu.com/?isSecure=true"}, scheme: "https"},
+			want:    "www.baidu.com",
+			wantErr: false,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := ParseEndpoint(tt.args.endpoints, tt.args.scheme, tt.args.isSecure)
+			got, err := ParseEndpoint(tt.args.endpoints, tt.args.scheme)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("ParseEndpoint() error = %v, wantErr %v", err, tt.wantErr)
 				return
diff --git a/transport/http/redirect_test.go b/transport/http/redirect_test.go
new file mode 100644
index 00000000..b98bf4f0
--- /dev/null
+++ b/transport/http/redirect_test.go
@@ -0,0 +1,23 @@
+package http
+
+import (
+	"net/http/httptest"
+	"testing"
+)
+
+func TestRedirect(t *testing.T) {
+	var (
+		redirectURL  = "/redirect"
+		redirectCode = 302
+	)
+	r := httptest.NewRequest("POST", "/test", nil)
+	w := httptest.NewRecorder()
+	_ = DefaultResponseEncoder(w, r, NewRedirect(redirectURL, redirectCode))
+
+	if w.Code != redirectCode {
+		t.Fatalf("want %d but got %d", redirectCode, w.Code)
+	}
+	if v := w.Header().Get("Location"); v != redirectURL {
+		t.Fatalf("want %s but got %s", redirectURL, v)
+	}
+}