optim redirector code based on comments (#885)

* optim redirector code based on comments * Update doc/reference/filters.md Co-authored-by: Bomin Zhang <[email protected]> Co-authored-by: Bomin Zhang <[email protected]>
easegress-io · Dec 28, 2022 · 9b92b56 · 9b92b56
1 parent 234d91b
commit 9b92b56
Show file tree

Hide file tree

Showing 3 changed files with 131 additions and 70 deletions.
diff --git a/doc/reference/filters.md b/doc/reference/filters.md
@@ -1286,8 +1286,9 @@ output: https://example.com/api/user/123
 | replacement | string | Replacement when the match succeeds. Placeholders like `$1`, `$2` can be used to represent the sub-matches in `regexp` | Yes | 
 | statusCode | int | Status code of response. Supported values: 301, 302, 303, 304, 307, 308. Default: 301. | No | 
 ### Results
-
-`Redirector` has no results. 
+| Value | Description |
+| ----- | ----------- |
+| redirected | The request has been redirected |
 
 ## Common Types
 

diff --git a/pkg/filters/redirector/redirector.go b/pkg/filters/redirector/redirector.go
@@ -18,12 +18,12 @@
 package redirector
 
 import (
+ "errors"
  "regexp"
  "strings"
 
  "github.com/megaease/easegress/pkg/context"
  "github.com/megaease/easegress/pkg/filters"
- "github.com/megaease/easegress/pkg/logger"
  "github.com/megaease/easegress/pkg/protocols/httpprot"
  "github.com/megaease/easegress/pkg/util/stringtool"
 )
@@ -32,7 +32,7 @@ const (
  // Kind is the kind of Redirector.
  Kind = "Redirector"
 
- resultMismatch = "mismatch"
+ resultRedirected = "redirected"
 )
 
 const (
@@ -53,9 +53,12 @@ var statusCodeMap = map[int]string{
 var kind = &filters.Kind{
  Name: Kind,
  Description: "Redirector redirect HTTP requests.",
- Results: []string{resultMismatch},
+ Results: []string{resultRedirected},
  DefaultSpec: func() filters.Spec {
- return &Spec{}
+ return &Spec{
+ MatchPart: matchPartURI,
+ StatusCode: 301,
+ }
  },
  CreateInstance: func(spec filters.Spec) filters.Filter {
  return &Redirector{spec: spec.(*Spec)}
@@ -78,12 +81,30 @@ type (
  filters.BaseSpec `json:",inline"`
 
  Match string `json:"match" jsonschema:"required"`
- MatchPart string `json:"matchPart" jsonschema:"required,enum=uri|full|path"` // default uri
+ MatchPart string `json:"matchPart,omitempty" jsonschema:"omitempty,enum=uri,enum=path,enum=full"` // default uri
  Replacement string `json:"replacement" jsonschema:"required"`
- StatusCode int `json:"statusCode" jsonschema:"required,enum=300|301|302|303|304|307|308"` // default 301
+ StatusCode int `json:"statusCode,omitempty" jsonschema:"omitempty"` // default 301
  }
 )
 
+func (s *Spec) Validate() error {
+ if _, ok := statusCodeMap[s.StatusCode]; !ok {
+ return errors.New("invalid status code of Redirector, support 300, 301, 302, 303, 304, 307, 308")
+ }
+ s.MatchPart = strings.ToLower(s.MatchPart)
+ if !stringtool.StrInSlice(s.MatchPart, []string{matchPartURI, matchPartFull, matchPartPath}) {
+ return errors.New("invalid match part of Redirector, only uri, full and path are supported")
+ }
+ if s.Match == "" || s.Replacement == "" {
+ return errors.New("match and replacement of Redirector can't be empty")
+ }
+ _, err := regexp.Compile(s.Match)
+ if err != nil {
+ return err
+ }
+ return nil
+}
+
 // Name returns the name of the Redirector filter instance.
 func (r *Redirector) Name() string {
  return r.spec.Name()
@@ -110,22 +131,6 @@ func (r *Redirector) Inherit(previousGeneration filters.Filter) {
 }
 
 func (r *Redirector) reload() {
- if r.spec.StatusCode == 0 {
- r.spec.StatusCode = 301
- }
- if _, ok := statusCodeMap[r.spec.StatusCode]; !ok {
- logger.Errorf("invalid status code of Redirector, support 300, 301, 302, 303, 304, 307, 308, use 301 instead")
- r.spec.StatusCode = 301
- }
-
- if r.spec.MatchPart == "" {
- r.spec.MatchPart = matchPartURI
- }
- r.spec.MatchPart = strings.ToLower(r.spec.MatchPart)
- if !stringtool.StrInSlice(r.spec.MatchPart, []string{matchPartURI, matchPartFull, matchPartPath}) {
- logger.Errorf("invalid match part of Redirector, only uri, full and path are supported, use uri instead")
- r.spec.MatchPart = matchPartURI
- }
  r.re = regexp.MustCompile(r.spec.Match)
 }
 
@@ -141,27 +146,31 @@ func (r *Redirector) getMatchInput(req *httpprot.Request) string {
  }
 }
 
-func (r *Redirector) updateResponse(resp *httpprot.Response, matchResult string) {
+func (r *Redirector) updateResponse(resp *httpprot.Response, newLocation string) {
  resp.SetStatusCode(r.spec.StatusCode)
- val, ok := statusCodeMap[r.spec.StatusCode]
- if !ok {
- resp.SetPayload([]byte(statusCodeMap[301]))
- } else {
- resp.SetPayload([]byte(val))
- }
- resp.Header().Add("Location", matchResult)
+ resp.SetPayload([]byte(statusCodeMap[r.spec.StatusCode]))
+ resp.Header().Add("Location", newLocation)
 }
 
 // Handle Redirector Context.
 func (r *Redirector) Handle(ctx *context.Context) string {
  req := ctx.GetInputRequest().(*httpprot.Request)
  matchInput := r.getMatchInput(req)
- matchResult := r.re.ReplaceAllString(matchInput, r.spec.Replacement)
+ newLocation := r.re.ReplaceAllString(matchInput, r.spec.Replacement)
+
+ // if matchInput is not matched, newLocation will be the same as matchInput
+ // consider we have multiple Redirector filters, we should not redirect the request
+ // if the request is not matched by the current Redirector filter
+ // so we return "" to indicate the request is not matched by the current Redirector filter
+ // and the request will be handled by the next filter.
+ if newLocation == matchInput {
+ return ""
+ }
 
  resp, _ := httpprot.NewResponse(nil)
- r.updateResponse(resp, matchResult)
+ r.updateResponse(resp, newLocation)
  ctx.SetOutputResponse(resp)
- return ""
+ return resultRedirected
 }
 
 // Status returns status.

diff --git a/pkg/filters/redirector/redirector_test.go b/pkg/filters/redirector/redirector_test.go
@@ -23,8 +23,10 @@ import (
  "testing"
 
  "github.com/megaease/easegress/pkg/context"
+ "github.com/megaease/easegress/pkg/filters"
  "github.com/megaease/easegress/pkg/logger"
  "github.com/megaease/easegress/pkg/protocols/httpprot"
+ "github.com/megaease/easegress/pkg/util/codectool"
  "github.com/stretchr/testify/assert"
 )
 
@@ -72,45 +74,39 @@ func TestRedirector(t *testing.T) {
  // test different spec configurations
  for i, t := range []testCase{
  {
- spec: getSpec("(.*)", "", "$1", 0), // use default, uri, 301
+ spec: getSpec("(.*)", "uri", "prefix${1}", 301), // uri, 301
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar?baz=qux", 301, "Moved Permanently"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "prefix/foo/bar?baz=qux", 301, "Moved Permanently"),
  },
  },
  {
- spec: getSpec("(.*)", "uri", "$1", 301), // uri, 301
+ spec: getSpec("(.*)", "full", "$1/123", 302), // full, 302
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar?baz=qux", 301, "Moved Permanently"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "http:https://a.com:8080/foo/bar?baz=qux/123", 302, "Found"),
  },
  },
  {
- spec: getSpec("(.*)", "full", "$1", 302), // full, 302
+ spec: getSpec("(.*)", "path", "prefix${1}", 303), // path, 303
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "http:https://a.com:8080/foo/bar?baz=qux", 302, "Found"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "prefix/foo/bar", 303, "See Other"),
  },
  },
  {
- spec: getSpec("(.*)", "path", "$1", 303), // path, 303
+ spec: getSpec("(.*)", "path", "prefix${1}", 304), // path, 304
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar", 303, "See Other"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "prefix/foo/bar", 304, "Not Modified"),
  },
  },
  {
- spec: getSpec("(.*)", "path", "$1", 304), // path, 304
+ spec: getSpec("(.*)", "path", "prefix$1", 307), // path, 307
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar", 304, "Not Modified"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "prefix/foo/bar", 307, "Temporary Redirect"),
  },
  },
  {
- spec: getSpec("(.*)", "path", "$1", 307), // path, 307
+ spec: getSpec("(.*)", "path", "prefix$1", 308), // path, 308
  matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar", 307, "Temporary Redirect"),
- },
- },
- {
- spec: getSpec("(.*)", "path", "$1", 308), // path, 308
- matches: []match{
- getMatch("http:https://a.com:8080/foo/bar?baz=qux", "/foo/bar", 308, "Permanent Redirect"),
+ getMatch("http:https://a.com:8080/foo/bar?baz=qux", "prefix/foo/bar", 308, "Permanent Redirect"),
  },
  },
  } {
@@ -134,21 +130,6 @@ func TestRedirector(t *testing.T) {
  }
  }
 
- // test invalid spec change to default or case-insensitive match part
- for i, t := range []*Spec{
- getSpec("(.*)", "all", "$1", 800), // invalid match part
- getSpec("(.*)", "other", "$1", 200), // invalid status code
- getSpec("(.*)", "URI", "$1", 200), // invalid status code
- getSpec("(.*)", "uRi", "$1", 200), // invalid status code
- getSpec("(.*)", "urI", "$1", 200), // invalid status code
- } {
- msg := fmt.Sprintf("case %d failed.", i)
- r := &Redirector{spec: t}
- r.Init()
- assert.Equal("uri", r.spec.MatchPart, msg)
- assert.Equal(301, r.spec.StatusCode, msg)
- }
-
  // test complicated regex
  for i, t := range []testCase{
  {
@@ -157,8 +138,6 @@ func TestRedirector(t *testing.T) {
  getMatch("http:https://a.com:8080/users/123", "display?user=123", 301, "Moved Permanently"),
  getMatch("http:https://a.com:8080/users/9", "display?user=9", 301, "Moved Permanently"),
  getMatch("http:https://a.com:8080/users/34", "display?user=34", 301, "Moved Permanently"),
- getMatch("http:https://a.com:8080/users/a123", "/users/a123", 301, "Moved Permanently"),
- getMatch("http:https://a.com:8080/profile/users/a123", "/profile/users/a123", 301, "Moved Permanently"),
  },
  },
  {
@@ -230,3 +209,75 @@ func TestRedirector(t *testing.T) {
  }
  }
 }
+
+func TestSpecValidate(t *testing.T) {
+ assert := assert.New(t)
+ {
+ // check default values
+ yamlStr := `
+name: filter
+kind: Redirector
+match: ".*"
+replacement: "123"
+`
+ rawSpec := map[string]interface{}{}
+ codectool.MustUnmarshal([]byte(yamlStr), &rawSpec)
+ s, err := filters.NewSpec(nil, "pipeline1", rawSpec)
+ assert.Nil(err)
+ spec := s.(*Spec)
+ assert.Equal("uri", spec.MatchPart)
+ assert.Equal(301, spec.StatusCode)
+ }
+
+ {
+ // check invalid spec
+
+ // invalid matchPart
+ yaml1 := `
+name: filter
+kind: Redirector
+match: ".*"
+replacement: "123"
+matchPart: "invalid"
+`
+
+ // invalid statusCode
+ yaml2 := `
+name: filter
+kind: Redirector
+match: ".*"
+replacement: "123"
+statusCode: 999
+`
+
+ // invalid match
+ yaml3 := `
+name: filter
+kind: Redirector
+match: ""
+`
+
+ // invalid replacement
+ yaml4 := `
+name: filter
+kind: Redirector
+match: ".*"
+replacement: ""
+`
+
+ // invalid match
+ yaml5 := `
+name: filter
+kind: Redirector
+match: "++"
+replacement: "123"
+`
+
+ for _, y := range []string{yaml1, yaml2, yaml3, yaml4, yaml5} {
+ rawSpec := map[string]interface{}{}
+ codectool.MustUnmarshal([]byte(y), &rawSpec)
+ _, err := filters.NewSpec(nil, "pipeline1", rawSpec)
+ assert.NotNil(err, y)
+ }
+ }
+}