Skip to content

Commit

Permalink
Fix SkipUrlClean by not using DefaultServeMux
Browse files Browse the repository at this point in the history
The issue itself happen because we wrap `gorilla.Mux` router through
`http.ServeMux`, which is kinda wrong, because `Mux` is already a
`ServeMux` implementation. So we processing routes twice.

Default `ServeMux` implementation have own route sanitization logic, and
can’t be configured with `SkipClean` option, like gorilla
implementation.

The solution here is to stop using ServeMux and use Gorilla directly.

Problem with this approach is that we need to reload, eg. replace, the
router inside `doReload` function. Neither gorilla or ServeMux support
replacing the routes, so replacing the whole router object is the only
solution. When we pass raw Gorilla Router object to the
`Server.Handler`, and replace `mainRouter` in `doReload`, server do not
see the changes: thats why we had issues when API reloads was not
working.

There are 2 ways to solve this issue: expose server object as a global
variable, and dynamically replace `Handler` attribute, or write simple
`http.Handler` wrapper around `mainRouter`, which always calls the
current router.

I’m not sure about safety of this approach but since previously we
replaced `DefaultServeMux` directly, and had no any issues, I may assume
that it should be thread safe.

Should fix #1093
  • Loading branch information
buger committed Sep 12, 2017
1 parent 288a31a commit 36231f7
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 27 deletions.
53 changes: 53 additions & 0 deletions gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/gorilla/mux"
"github.com/justinas/alice"

"github.com/TykTechnologies/tyk/apidef"
"github.com/TykTechnologies/tyk/config"
)

Expand Down Expand Up @@ -1029,3 +1030,55 @@ func TestProxyUserAgent(t *testing.T) {
}
}
}

func buildAndLoadAPI(apiGens ...func(spec *APISpec)) {
globalConf.AppPath, _ = ioutil.TempDir("", "apps")
defer func() {
os.RemoveAll(globalConf.AppPath)
globalConf.AppPath = "apps/"
}()

for i, gen := range apiGens {
spec := &APISpec{APIDefinition: &apidef.APIDefinition{}}
json.Unmarshal([]byte(sampleAPI), spec.APIDefinition)
gen(spec)
specBytes, _ := json.Marshal(spec)
specFilePath := filepath.Join(globalConf.AppPath, spec.APIID+strconv.Itoa(i)+".json")
if err := ioutil.WriteFile(specFilePath, specBytes, 0644); err != nil {
panic(err)
}
}

doReload()
}

func TestSkipUrlCleaning(t *testing.T) {
globalConf.HttpServerOptions.OverrideDefaults = true
globalConf.HttpServerOptions.SkipURLCleaning = true

ln, _ := generateListener(0)
baseURL := "http://" + ln.Addr().String()
listen(ln, nil, nil)

defer func() {
globalConf.HttpServerOptions.OverrideDefaults = false
globalConf.HttpServerOptions.SkipURLCleaning = false
ln.Close()
}()

s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Write([]byte(r.URL.Path))
}))

buildAndLoadAPI(func(spec *APISpec) {
spec.Proxy.ListenPath = "/"
spec.Proxy.TargetURL = s.URL
})

resp, _ := http.Get(baseURL + "/http://example.com")
body, _ := ioutil.ReadAll(resp.Body)

if string(body) != "/http://example.com" {
t.Error("Should not strip URL", string(body))
}
}
46 changes: 19 additions & 27 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -563,18 +563,15 @@ func doReload() {
log.WithFields(logrus.Fields{
"prefix": "main",
}).Info("Preparing new router")
newRouter := mux.NewRouter()
mainRouter = newRouter
mainRouter = mux.NewRouter()
if globalConf.HttpServerOptions.OverrideDefaults {
mainRouter.SkipClean(globalConf.HttpServerOptions.SkipURLCleaning)
}

if globalConf.ControlAPIPort == 0 {
loadAPIEndpoints(newRouter)
loadAPIEndpoints(mainRouter)
}
loadApps(specs, newRouter)

newServeMux := http.NewServeMux()
newServeMux.Handle("/", mainRouter)

http.DefaultServeMux = newServeMux
loadApps(specs, mainRouter)

log.WithFields(logrus.Fields{
"prefix": "main",
Expand Down Expand Up @@ -1190,6 +1187,13 @@ func startDRL() {
startRateLimitNotifications()
}

// mainHandler's only purpose is to allow mainRouter to be dynamically replaced
type mainHandler struct{}

func (_ mainHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
mainRouter.ServeHTTP(w, r)
}

func listen(l, controlListener net.Listener, err error) {
readTimeout := 120
writeTimeout := 120
Expand Down Expand Up @@ -1240,12 +1244,9 @@ func listen(l, controlListener net.Listener, err error) {
Addr: ":" + targetPort,
ReadTimeout: time.Duration(readTimeout) * time.Second,
WriteTimeout: time.Duration(writeTimeout) * time.Second,
Handler: mainHandler{},
}

newServeMux := http.NewServeMux()
newServeMux.Handle("/", defaultRouter)
http.DefaultServeMux = newServeMux

// Accept connections in a new goroutine.
go s.Serve(l)

Expand All @@ -1262,13 +1263,9 @@ func listen(l, controlListener net.Listener, err error) {
"prefix": "main",
}).Printf("Gateway started (%s)", VERSION)

go http.Serve(l, nil)
go http.Serve(l, mainHandler{})

if !rpcEmergencyMode {
newServeMux := http.NewServeMux()
newServeMux.Handle("/", mainRouter)
http.DefaultServeMux = newServeMux

if controlListener != nil {
go http.Serve(controlListener, controlRouter)
}
Expand Down Expand Up @@ -1312,19 +1309,18 @@ func listen(l, controlListener net.Listener, err error) {
}

if globalConf.HttpServerOptions.OverrideDefaults {
defaultRouter.SkipClean(globalConf.HttpServerOptions.SkipURLCleaning)

log.WithFields(logrus.Fields{
"prefix": "main",
}).Warning("HTTP Server Overrides detected, this could destabilise long-running http-requests")
s := &http.Server{
Addr: ":" + targetPort,
ReadTimeout: time.Duration(readTimeout) * time.Second,
WriteTimeout: time.Duration(writeTimeout) * time.Second,
Handler: mainHandler{},
}

newServeMux := http.NewServeMux()
newServeMux.Handle("/", defaultRouter)
http.DefaultServeMux = newServeMux

log.WithFields(logrus.Fields{
"prefix": "main",
}).Info("Custom gateway started")
Expand All @@ -1343,13 +1339,9 @@ func listen(l, controlListener net.Listener, err error) {
"prefix": "main",
}).Printf("Gateway resumed (%s)", VERSION)

go http.Serve(l, nil)
go http.Serve(l, mainHandler{})

if !rpcEmergencyMode {
newServeMux := http.NewServeMux()
newServeMux.Handle("/", mainRouter)
http.DefaultServeMux = newServeMux

if controlListener != nil {
go http.Serve(controlListener, controlRouter)
}
Expand Down

0 comments on commit 36231f7

Please sign in to comment.