Alerting: Use mux router to match hooks, add support for path variables and methods (#79345)

* Use a router inside hooks rather than plain string matching

* Add test for mismatched method

pkg/services/ngalert/api/hooks.go

@@ -1,8 +1,10 @@
 package api
 
 import (
+	"net/http"
 	"net/url"
 
+	"github.com/gorilla/mux"
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/infra/log"
 	contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model"
@@ -11,31 +13,40 @@ import (
 type RequestHandlerFunc func(*contextmodel.ReqContext) response.Response
 
 type Hooks struct {
-	logger log.Logger
-	hooks  map[string]RequestHandlerFunc
+	logger     log.Logger
+	router     *mux.Router
+	routeHooks map[*mux.Route]RequestHandlerFunc
 }
 
 // NewHooks creates an empty set of request handler hooks. Hooks can be used
 // to replace handlers for specific paths.
 func NewHooks(logger log.Logger) *Hooks {
 	return &Hooks{
-		logger: logger,
-		hooks:  make(map[string]RequestHandlerFunc),
+		logger:     logger,
+		router:     mux.NewRouter(),
+		routeHooks: make(map[*mux.Route]RequestHandlerFunc),
 	}
 }
 
 // Add creates a new request hook for a path, causing requests to the path to
 // be handled by the hook function, and not the original handler.
-func (h *Hooks) Set(path string, hook RequestHandlerFunc) {
+func (h *Hooks) Set(method string, path string, hook RequestHandlerFunc) {
 	h.logger.Info("Setting hook override for the specified route", "path", path)
-	h.hooks[path] = hook
+	route := h.router.NewRoute().Path(path).Methods(method)
+	h.routeHooks[route] = hook
 }
 
 // Get returns a hook if one is defined for the matching URL.
 // Get also returns a bool indicating whether or not a matching hook exists.
-func (h *Hooks) Get(url *url.URL) (RequestHandlerFunc, bool) {
-	hook, ok := h.hooks[url.Path]
-	return hook, ok
+func (h *Hooks) Get(method string, url *url.URL) (RequestHandlerFunc, bool) {
+	req := http.Request{Method: method, URL: url}
+
+	match := mux.RouteMatch{}
+	if ok := h.router.Match(&req, &match); ok {
+		return h.routeHooks[match.Route], ok
+	}
+
+	return nil, false
 }
 
 // Wrap returns a new handler which will intercept paths with hooks configured,
@@ -43,7 +54,7 @@ func (h *Hooks) Get(url *url.URL) (RequestHandlerFunc, bool) {
 // then the given handler is invoked.
 func (h *Hooks) Wrap(next RequestHandlerFunc) RequestHandlerFunc {
 	return func(req *contextmodel.ReqContext) response.Response {
-		if hook, ok := h.Get(req.Context.Req.URL); ok {
+		if hook, ok := h.Get(req.Context.Req.Method, req.Context.Req.URL); ok {
 			h.logger.Debug("Hook defined - invoking new handler", "path", req.Context.Req.URL.Path)
 			return hook(req)
 		}

pkg/services/ngalert/api/hooks_test.go

@@ -19,9 +19,9 @@ func TestHooks(t *testing.T) {
 			invoked := false
 			hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil }
 
-			hooks.Set("/some/path", hook)
+			hooks.Set("GET", "/some/path", hook)
 			reqURL, _ := url.Parse("http://domain.test/some/path")
-			handler, ok := hooks.Get(reqURL)
+			handler, ok := hooks.Get("GET", reqURL)
 
 			require.True(t, ok, "hooks did not contain a matching hook for path")
 			require.False(t, invoked, "hook was invoked earlier than expected")
@@ -33,9 +33,9 @@ func TestHooks(t *testing.T) {
 			hooks := NewHooks(log.NewNopLogger())
 			hook := func(*contextmodel.ReqContext) response.Response { return nil }
 
-			hooks.Set("/some/path", hook)
+			hooks.Set("GET", "/some/path", hook)
 			reqURL, _ := url.Parse("http://domain.test/does/not/match")
-			handler, ok := hooks.Get(reqURL)
+			handler, ok := hooks.Get("GET", reqURL)
 
 			require.False(t, ok, "hooks returned a hook when we expected it not to")
 			require.Nil(t, handler)
@@ -45,9 +45,21 @@ func TestHooks(t *testing.T) {
 			hooks := NewHooks(log.NewNopLogger())
 			hook := func(*contextmodel.ReqContext) response.Response { return nil }
 
-			hooks.Set("/some/path", hook)
+			hooks.Set("GET", "/some/path", hook)
 			reqURL, _ := url.Parse("http://domain.test/some/path/with/more")
-			handler, ok := hooks.Get(reqURL)
+			handler, ok := hooks.Get("GET", reqURL)
+
+			require.False(t, ok, "hooks returned a hook when we expected it not to")
+			require.Nil(t, handler)
+		})
+
+		t.Run("hooks do not match requests with the wrong HTTP method", func(t *testing.T) {
+			hooks := NewHooks(log.NewNopLogger())
+			hook := func(*contextmodel.ReqContext) response.Response { return nil }
+
+			hooks.Set("POST", "/some/path", hook)
+			reqURL, _ := url.Parse("http://domain.test/some/path")
+			handler, ok := hooks.Get("GET", reqURL)
 
 			require.False(t, ok, "hooks returned a hook when we expected it not to")
 			require.Nil(t, handler)
@@ -58,9 +70,9 @@ func TestHooks(t *testing.T) {
 			invoked := false
 			hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil }
 
-			hooks.Set("/some/path", hook)
+			hooks.Set("GET", "/some/path", hook)
 			reqURL, _ := url.Parse("http://domain.test/some/path?query=param")
-			handler, ok := hooks.Get(reqURL)
+			handler, ok := hooks.Get("GET", reqURL)
 
 			require.True(t, ok, "hooks did not contain a matching hook for path")
 			require.False(t, invoked, "hook was invoked earlier than expected")
@@ -68,16 +80,19 @@ func TestHooks(t *testing.T) {
 			require.True(t, invoked, "the hook returned by get() was not invoked as expected")
 		})
 
-		t.Run("hooks do not match routes with path variables", func(t *testing.T) {
+		t.Run("hooks match routes with path variables", func(t *testing.T) {
 			hooks := NewHooks(log.NewNopLogger())
-			hook := func(*contextmodel.ReqContext) response.Response { return nil }
+			invoked := false
+			hook := func(*contextmodel.ReqContext) response.Response { invoked = true; return nil }
 
-			hooks.Set("/some/{value}", hook)
+			hooks.Set("GET", "/some/{value}", hook)
 			reqURL, _ := url.Parse("http://domain.test/some/123")
-			handler, ok := hooks.Get(reqURL)
+			handler, ok := hooks.Get("GET", reqURL)
 
-			require.False(t, ok, "hooks returned a hook when we expected it not to")
-			require.Nil(t, handler)
+			require.True(t, ok, "hooks did not contain a matching hook for path")
+			require.False(t, invoked, "hook was invoked earlier than expected")
+			handler(nil)
+			require.True(t, invoked, "the hook returned by get() was not invoked as expected")
 		})
 	})
 
@@ -88,10 +103,10 @@ func TestHooks(t *testing.T) {
 			hookInvoked := false
 			hookHandler := func(*contextmodel.ReqContext) response.Response { hookInvoked = true; return nil }
 			hooks := NewHooks(log.NewNopLogger())
-			hooks.Set("/some/path", hookHandler)
+			hooks.Set("GET", "/some/path", hookHandler)
 
 			composed := hooks.Wrap(defaultHandler)
-			req := createReqWithURL("http://domain.test/some/path")
+			req := createReqForTests("GET", "http://domain.test/some/path")
 			composed(req)
 
 			require.True(t, hookInvoked, "hook was expected to be invoked, but it was not")
@@ -104,10 +119,10 @@ func TestHooks(t *testing.T) {
 			hookInvoked := false
 			hookHandler := func(*contextmodel.ReqContext) response.Response { hookInvoked = true; return nil }
 			hooks := NewHooks(log.NewNopLogger())
-			hooks.Set("/some/path", hookHandler)
+			hooks.Set("GET", "/some/path", hookHandler)
 
 			composed := hooks.Wrap(defaultHandler)
-			req := createReqWithURL("http://domain.test/does/not/match")
+			req := createReqForTests("GET", "http://domain.test/does/not/match")
 			composed(req)
 
 			require.False(t, hookInvoked, "hook was invoked, but it should not have been")
@@ -116,12 +131,13 @@ func TestHooks(t *testing.T) {
 	})
 }
 
-func createReqWithURL(setupURL string) *contextmodel.ReqContext {
+func createReqForTests(method, setupURL string) *contextmodel.ReqContext {
 	reqURL, _ := url.Parse(setupURL)
 	return &contextmodel.ReqContext{
 		Context: &web.Context{
 			Req: &http.Request{
-				URL: reqURL,
+				Method: method,
+				URL:    reqURL,
 			},
 		},
 	}