From 9b92b56916da37297be9077d7900496cf11f414c Mon Sep 17 00:00:00 2001 From: su chen Date: Wed, 28 Dec 2022 17:05:41 +0800 Subject: [PATCH] optim redirector code based on comments (#885) * optim redirector code based on comments * Update doc/reference/filters.md Co-authored-by: Bomin Zhang Co-authored-by: Bomin Zhang --- doc/reference/filters.md | 5 +- pkg/filters/redirector/redirector.go | 75 ++++++++------ pkg/filters/redirector/redirector_test.go | 121 +++++++++++++++------- 3 files changed, 131 insertions(+), 70 deletions(-) diff --git a/doc/reference/filters.md b/doc/reference/filters.md index d5aa0055f6..7a8f749c8f 100644 --- 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 index 2669dd2025..ce4a1de355 100644 --- 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 index dddc9ce5c9..dcf058f60c 100644 --- 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://a.com:8080/foo/bar?baz=qux", "/foo/bar?baz=qux", 301, "Moved Permanently"), + getMatch("http://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://a.com:8080/foo/bar?baz=qux", "/foo/bar?baz=qux", 301, "Moved Permanently"), + getMatch("http://a.com:8080/foo/bar?baz=qux", "http://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://a.com:8080/foo/bar?baz=qux", "http://a.com:8080/foo/bar?baz=qux", 302, "Found"), + getMatch("http://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://a.com:8080/foo/bar?baz=qux", "/foo/bar", 303, "See Other"), + getMatch("http://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://a.com:8080/foo/bar?baz=qux", "/foo/bar", 304, "Not Modified"), + getMatch("http://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://a.com:8080/foo/bar?baz=qux", "/foo/bar", 307, "Temporary Redirect"), - }, - }, - { - spec: getSpec("(.*)", "path", "$1", 308), // path, 308 - matches: []match{ - getMatch("http://a.com:8080/foo/bar?baz=qux", "/foo/bar", 308, "Permanent Redirect"), + getMatch("http://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://a.com:8080/users/123", "display?user=123", 301, "Moved Permanently"), getMatch("http://a.com:8080/users/9", "display?user=9", 301, "Moved Permanently"), getMatch("http://a.com:8080/users/34", "display?user=34", 301, "Moved Permanently"), - getMatch("http://a.com:8080/users/a123", "/users/a123", 301, "Moved Permanently"), - getMatch("http://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) + } + } +}