gddo-server: redirect based on config shouldRedirectRequest now uses data from the DynamicConfig to decide whether a request should be redirected. Change-Id: I69a3d4df275248003b170c0e48ca6959cad93603 Reviewed-on: https://go-review.googlesource.com/c/gddo/+/285933 Trust: Julie Qiu <julie@golang.org> Reviewed-by: Jonathan Amsterdam <jba@google.com>
diff --git a/gddo-server/dynconfig/dynconfig.go b/gddo-server/dynconfig/dynconfig.go index d75f8bc..5207164 100644 --- a/gddo-server/dynconfig/dynconfig.go +++ b/gddo-server/dynconfig/dynconfig.go
@@ -26,6 +26,10 @@ type DynamicConfig struct { // Fields can be added at any time, but removing or changing a field // requires careful coordination with the config file contents. + RedirectBadges bool + RedirectHomepage bool + RedirectSearch bool + RedirectPaths []string } // Read reads dynamic configuration from the given location.
diff --git a/gddo-server/pkgsite.go b/gddo-server/pkgsite.go index 5c883f8..162d6e1 100644 --- a/gddo-server/pkgsite.go +++ b/gddo-server/pkgsite.go
@@ -17,6 +17,8 @@ "strings" "time" + "github.com/golang/gddo/gddo-server/dynconfig" + "github.com/golang/gddo/gddo-server/poller" "golang.org/x/mod/module" "golang.org/x/net/context/ctxhttp" ) @@ -185,11 +187,14 @@ // pkgGoDevRedirectHandler redirects requests from godoc.org to pkg.go.dev, // based on whether a cookie is set for pkggodev-redirect. The cookie // can be turned on/off using a query param. -func pkgGoDevRedirectHandler(f func(http.ResponseWriter, *http.Request) error) func(http.ResponseWriter, *http.Request) error { +func pkgGoDevRedirectHandler(configPoller *poller.Poller, f func(http.ResponseWriter, *http.Request) error) func(http.ResponseWriter, *http.Request) error { return func(w http.ResponseWriter, r *http.Request) error { - if shouldRedirectRequest(r) { - http.Redirect(w, r, pkgGoDevURL(r.URL).String(), http.StatusFound) - return nil + redirectURL := pkgGoDevURL(r.URL).String() + if configPoller != nil { + if shouldRedirectURL(r, configPoller) { + http.Redirect(w, r, redirectURL, http.StatusFound) + return nil + } } if userReturningFromPkgGoDev(r) { @@ -207,32 +212,50 @@ if !shouldRedirectToPkgGoDev(r) { return f(w, r) } - http.Redirect(w, r, pkgGoDevURL(r.URL).String(), http.StatusFound) + http.Redirect(w, r, redirectURL, http.StatusFound) return nil } } -// alwaysRedirectPaths is a set of paths that are always redirected to -// pkg.go.dev. -var alwaysRedirectPaths = map[string]bool{ - "/-/about": true, +// shouldRedirectURL reports whether a request to the given URL should be +// redirected to pkg.go.dev. +func shouldRedirectURL(r *http.Request, poller *poller.Poller) bool { + cfg := poller.Current().(*dynconfig.DynamicConfig) + return shouldRedirectURLForSnapshot(r, cfg) } -func shouldRedirectRequest(r *http.Request) bool { +func shouldRedirectURLForSnapshot(r *http.Request, cfg *dynconfig.DynamicConfig) bool { + if cfg == nil { + log.Printf("shouldRedirectURLForSnapshot(%q): cfg is nil", r.URL.Path) + return false + } + // Requests to api.godoc.org and talks.godoc.org are not redirected. if strings.HasPrefix(r.URL.Host, "api") || strings.HasPrefix(r.URL.Host, "talks") { return false } - // Badge SVGs will be redirected last. + _, isSVG := r.URL.Query()["status.svg"] _, isPNG := r.URL.Query()["status.png"] if isSVG || isPNG { + return cfg.RedirectBadges + } + for _, p := range cfg.RedirectPaths { + if r.URL.Path == p { + return true + } + } + q := strings.TrimSpace(r.Form.Get("q")) + if r.URL.Path == "/" || r.URL.Path == "" { + if q == "" && cfg.RedirectHomepage { + return true + } + if q != "" && cfg.RedirectSearch { + return true + } return false } - if alwaysRedirectPaths[r.URL.Path] { - return true - } // TODO: redirect based on rollout percentage. return false }
diff --git a/gddo-server/pkgsite_test.go b/gddo-server/pkgsite_test.go index 18ffe08..34ad51b 100644 --- a/gddo-server/pkgsite_test.go +++ b/gddo-server/pkgsite_test.go
@@ -14,6 +14,7 @@ "strings" "testing" + "github.com/golang/gddo/gddo-server/dynconfig" "github.com/google/go-cmp/cmp" ) @@ -66,19 +67,13 @@ cookie: &http.Cookie{Name: "pkggodev-redirect", Value: "on"}, wantStatusCode: http.StatusOK, }, - { - name: "always redirect /-/about", - url: "http://godoc.org/-/about", - wantLocationHeader: "https://pkg.go.dev/about?utm_source=godoc", - wantStatusCode: http.StatusFound, - }, } { t.Run(test.name, func(t *testing.T) { req := httptest.NewRequest("GET", test.url, nil) if test.cookie != nil { req.AddCookie(test.cookie) } - handler := pkgGoDevRedirectHandler(func(w http.ResponseWriter, r *http.Request) error { + handler := pkgGoDevRedirectHandler(nil, func(w http.ResponseWriter, r *http.Request) error { return nil }) @@ -498,3 +493,74 @@ } } } + +func TestShouldRedirectURLForSnapshot(t *testing.T) { + var testRequests []*http.Request + for _, tu := range []string{ + "https://api.godoc.org/-/about", + "https://api.godoc.org/?q=http", + "https://api.godoc.org/-/net/http", + "https://godoc.org", + "https://godoc.org/-/about", + "https://godoc.org/-/bot", + "https://godoc.org/-/go", + "https://godoc.org/-/subrepo", + "https://godoc.org/?q=http", + "https://godoc.org/net/http", + "https://godoc.org/cloud.google.com/go", + } { + u, err := url.Parse(tu) + if err != nil { + t.Fatal(err) + } + req := httptest.NewRequest("GET", u.String(), nil) + req.URL = u + req.Form = u.Query() + testRequests = append(testRequests, req) + } + + for _, test := range []struct { + name string + snapshot *dynconfig.DynamicConfig + redirect map[string]bool + }{ + { + name: "redirect homepage", + snapshot: &dynconfig.DynamicConfig{ + RedirectHomepage: true, + }, + redirect: map[string]bool{"https://godoc.org": true}, + }, + { + name: "redirect search page", + snapshot: &dynconfig.DynamicConfig{ + RedirectSearch: true, + }, + redirect: map[string]bool{"https://godoc.org/?q=http": true}, + }, + { + name: "redirect static page", + snapshot: &dynconfig.DynamicConfig{ + RedirectPaths: []string{"/-/about", "/-/subrepo", "/-/go"}, + }, + redirect: map[string]bool{ + "https://godoc.org/-/about": true, + "https://godoc.org/-/go": true, + "https://godoc.org/-/subrepo": true, + }, + }, + } { + t.Run(test.name, func(t *testing.T) { + for _, req := range testRequests { + u := req.URL.String() + got := shouldRedirectURLForSnapshot(req, test.snapshot) + if test.redirect[u] && !got { + t.Errorf("%q should redirect but didn't", u) + } + if !test.redirect[u] && got { + t.Errorf("%q should not redirect and did", u) + } + } + }) + } +}
diff --git a/gddo-server/server.go b/gddo-server/server.go index fdf5ce4..40c1954 100644 --- a/gddo-server/server.go +++ b/gddo-server/server.go
@@ -557,11 +557,9 @@ if err != nil { return err } - return s.templates.execute(resp, "home"+templateExt(req), http.StatusOK, nil, map[string]interface{}{ - "Popular": pkgs, - + "Popular": pkgs, "showPkgGoDevRedirectToast": userReturningFromPkgGoDev(req), }) } @@ -607,8 +605,7 @@ func (s *server) serveAbout(resp http.ResponseWriter, req *http.Request) error { return s.templates.execute(resp, "about.html", http.StatusOK, nil, map[string]interface{}{ - "Host": req.Host, - + "Host": req.Host, "showPkgGoDevRedirectToast": userReturningFromPkgGoDev(req), }) } @@ -886,7 +883,7 @@ httpClient: newHTTPClient(v), importGraphSem: make(chan struct{}, 10), } - cfg, err := newConfigPoller(ctx, 1*time.Millisecond) + cfg, err := newConfigPoller(ctx, 1*time.Minute) if err != nil { log.Printf("newConfigPoller: %q", err) } @@ -965,10 +962,10 @@ } } - mux.Handle("/-/about", handler(pkgGoDevRedirectHandler(s.serveAbout))) - mux.Handle("/-/bot", handler(pkgGoDevRedirectHandler(s.serveBot))) - mux.Handle("/-/go", handler(pkgGoDevRedirectHandler(s.serveGoIndex))) - mux.Handle("/-/subrepo", handler(pkgGoDevRedirectHandler(s.serveGoSubrepoIndex))) + mux.Handle("/-/about", handler(pkgGoDevRedirectHandler(s.configPoller, s.serveAbout))) + mux.Handle("/-/bot", handler(pkgGoDevRedirectHandler(s.configPoller, s.serveBot))) + mux.Handle("/-/go", handler(pkgGoDevRedirectHandler(s.configPoller, s.serveGoIndex))) + mux.Handle("/-/subrepo", handler(pkgGoDevRedirectHandler(s.configPoller, s.serveGoSubrepoIndex))) mux.Handle("/-/refresh", handler(s.serveRefresh)) mux.Handle("/about", http.RedirectHandler("/-/about", http.StatusMovedPermanently)) mux.Handle("/favicon.ico", staticServer.FileHandler("favicon.ico")) @@ -978,7 +975,7 @@ mux.Handle("/BingSiteAuth.xml", staticServer.FileHandler("BingSiteAuth.xml")) mux.Handle("/C", http.RedirectHandler("http://golang.org/doc/articles/c_go_cgo.html", http.StatusMovedPermanently)) mux.Handle("/code.jquery.com/", http.NotFoundHandler()) - mux.Handle("/", handler(pkgGoDevRedirectHandler(s.serveHome))) + mux.Handle("/", handler(pkgGoDevRedirectHandler(s.configPoller, s.serveHome))) ahMux := http.NewServeMux() ready := new(health.Handler) @@ -1080,7 +1077,7 @@ } func (s *server) teeRequestToPkgGoDev(r *http.Request, latency time.Duration, status int) { - if shouldRedirectRequest(r) { + if shouldRedirectURL(r, s.configPoller) { log.Printf("shouldRedirectToPkgGoDev(%q, %q)= true: not teeing request because it is redirected to pkg.go.dev", r.URL.Host, r.URL.Path) return }