Skip to content

Enforce HTTP method required by most API endpoints #3228

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 25, 2020
Merged

Enforce HTTP method required by most API endpoints #3228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cbc2389
Enforce HTTP method required by most API endpoints
pracucci Sep 23, 2020
105b2d0
Enforced method to api.RegisterRoute()
pracucci Sep 23, 2020
0f90823
Small fixes
pracucci Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,34 @@
* [CHANGE] Dropped "blank Alertmanager configuration; using fallback" message from Info to Debug level. #3205
* [CHANGE] Zone-awareness replication for time-series now should be explicitly enabled in the distributor via the `-distributor.zone-awareness-enabled` CLI flag (or its respective YAML config option). Before, zone-aware replication was implicitly enabled if a zone was set on ingesters. #3200
* [CHANGE] Removed the deprecated CLI flag `-config-yaml`. You should use `-schema-config-file` instead. #3225
* [CHANGE] Enforced the HTTP method required by some API endpoints which did (incorrectly) allow any method before that. #3228
- `GET /`
- `GET /config`
- `GET /debug/fgprof`
- `GET /distributor/all_user_stats`
- `GET /distributor/ha_tracker`
- `GET /all_user_stats`
- `GET /ha-tracker`
- `GET /api/v1/user_stats`
- `GET /api/v1/chunks`
- `GET <legacy-http-prefix>/user_stats`
- `GET <legacy-http-prefix>/chunks`
- `GET /services`
- `GET /multitenant_alertmanager/status`
- `GET /status` (alertmanager microservice)
- `GET|POST /ingester/ring`
- `GET|POST /ring`
- `GET|POST /store-gateway/ring`
- `GET|POST /compactor/ring`
- `GET|POST /ingester/flush`
Copy link
Contributor

@pstibrany pstibrany Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should make /flush, /shutdown and push POST-only? It would make flush and shutdown links on index page not working from browser, which may be a good thing (no accidental click from browser, or prefetching, or recursive link-following robots).

Copy link
Contributor Author

@pracucci pracucci Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

push POST-only

It is POST only but the CHANGELOG was wrong. Fixed, thanks!

Perhaps we should make /flush, /shutdown

I agree would be safer. If do so, we should consider:

  1. Change tools/migrate-ingester-statefulsets.sh because calls GET /shutdown, but then we would have to use something different than wget (and curl is not available in Cortex images)
  2. We would have to remove these links from the / HTML page because if you click on that they will fail (so there's no point linking them, the https://cortexmetrics.io/docs/api/ will be enough)

Thoughts?

Copy link
Contributor

@pstibrany pstibrany Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Change tools/migrate-ingester-statefulsets.sh because calls GET /shutdown, but then we would have to use something different than wget (and curl is not available in Cortex images)

Valid point. What about we include little tool in the image to call those endpoints? We could fix another problems with included wget too -- it doesn't like 204. I'd suggest separate PR for all that.

  1. We would have to remove these links from the / HTML page because if you click on that they will fail (so there's no point linking them, the https://cortexmetrics.io/docs/api/ will be enough)

I would keep them in the index page (it's useful to know that given Cortex supports it), but add a note that they are POST-only.

Copy link
Contributor Author

@pracucci pracucci Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid point. What about we include little tool in the image to call those endpoints? We could fix another problems with included wget too -- it doesn't like 204. I'd suggest separate PR for all that.

I've the feeling it's overkilled. Installing curl in our Docker images could be just easier.

Copy link
Contributor

@pstibrany pstibrany Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curl also doesn't like 204 and returns non-zero exit code. We could simplify our script if we had this.

Copy link
Contributor

@gouthamve gouthamve Sep 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to making them POST only!
+1 to showing them in index page but mention that they only support POST.

Not too familiar with the script though.

Copy link
Contributor Author

@pracucci pracucci Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you agree if I do it in a separate PR? We need to find a solution for the wget replacement and I would like to avoid blocking this PR because of that.

Copy link
Contributor

@pstibrany pstibrany Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you agree if I do it in a separate PR? We need to find a solution for the wget replacement and I would like to avoid blocking this PR because of that.

Absolutely

Copy link
Contributor Author

@pracucci pracucci Sep 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened an issue for /flush and /shutdown:
#3243

- `GET|POST /ingester/shutdown`
- `GET|POST /flush`
- `GET|POST /shutdown`
- `GET|POST /ruler/ring`
- `POST /api/v1/push`
- `POST <legacy-http-prefix>/push`
- `POST /push`
- `POST /ingester/push`
* [FEATURE] Added support for shuffle-sharding queriers in the query-frontend. When configured (`-frontend.max-queriers-per-user` globally, or using per-user limit `max_queriers_per_user`), each user's requests will be handled by different set of queriers. #3113
* [ENHANCEMENT] Added `cortex_query_frontend_connected_clients` metric to show the number of workers currently connected to the frontend. #3207
* [ENHANCEMENT] Shuffle sharding: improved shuffle sharding in the write path. Shuffle sharding now should be explicitly enabled via `-distributor.sharding-strategy` CLI flag (or its respective YAML config option) and guarantees stability, consistency, shuffling and balanced zone-awareness properties. #3090
Expand Down
23 changes: 23 additions & 0 deletions integration/api_config_test.go → integration/api_endpoints_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,36 @@ import (
"net/http"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/thanos-io/thanos/pkg/runutil"

"github.com/cortexproject/cortex/integration/e2e"
"github.com/cortexproject/cortex/integration/e2ecortex"
)

func TestIndexAPIEndpoint(t *testing.T) {
s, err := e2e.NewScenario(networkName)
require.NoError(t, err)
defer s.Close()

// Start Cortex in single binary mode, reading the config from file.
require.NoError(t, copyFileToSharedDir(s, "docs/configuration/single-process-config.yaml", cortexConfigFile))

cortex1 := e2ecortex.NewSingleBinaryWithConfigFile("cortex-1", cortexConfigFile, nil, "", 9009, 9095)
require.NoError(t, s.StartAndWaitReady(cortex1))

// GET / should succeed
res, err := e2e.GetRequest(fmt.Sprintf("http://%s", cortex1.Endpoint(9009)))
require.NoError(t, err)
assert.Equal(t, 200, res.StatusCode)

// POST / should fail
res, err = e2e.PostRequest(fmt.Sprintf("http://%s", cortex1.Endpoint(9009)))
require.NoError(t, err)
assert.Equal(t, 405, res.StatusCode)
}

func TestConfigAPIEndpoint(t *testing.T) {
s, err := e2e.NewScenario(networkName)
require.NoError(t, err)
Expand Down
8 changes: 8 additions & 0 deletions integration/e2e/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"os"
"os/exec"
"path/filepath"
"strings"
"time"

"github.com/prometheus/common/model"
Expand Down Expand Up @@ -69,6 +70,13 @@ func GetRequest(url string) (*http.Response, error) {
return client.Get(url)
}

func PostRequest(url string) (*http.Response, error) {
const timeout = 1 * time.Second

client := &http.Client{Timeout: timeout}
return client.Post(url, "", strings.NewReader(""))
}

// timeToMilliseconds returns the input time as milliseconds, using the same
// formula used by Prometheus in order to get the same timestamp when asserting
// on query results.
Expand Down
68 changes: 37 additions & 31 deletions pkg/api/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,17 @@ func New(cfg Config, serverCfg server.Config, s *server.Server, logger log.Logge
return api, nil
}

func (a *API) RegisterRoute(path string, handler http.Handler, auth bool, methods ...string) {
a.registerRouteWithRouter(a.server.HTTP, path, handler, auth, methods...)
// RegisterRoute registers a single route enforcing HTTP methods. A single
// route is expected to be specific about which HTTP methods are supported.
func (a *API) RegisterRoute(path string, handler http.Handler, auth bool, method string, methods ...string) {
a.registerRouteWithRouter(a.server.HTTP, path, handler, auth, method, methods...)
}

func (a *API) registerRouteWithRouter(router *mux.Router, path string, handler http.Handler, auth bool, methods ...string) {
// RegisterRoute registers a single route to a router, enforcing HTTP methods. A single
// route is expected to be specific about which HTTP methods are supported.
func (a *API) registerRouteWithRouter(router *mux.Router, path string, handler http.Handler, auth bool, method string, methods ...string) {
methods = append([]string{method}, methods...)

level.Debug(a.logger).Log("msg", "api: registering route", "methods", strings.Join(methods, ","), "path", path, "auth", auth)
if auth {
handler = a.authMiddleware.Wrap(handler)
Expand Down Expand Up @@ -148,7 +154,7 @@ func fakeRemoteAddr(handler http.Handler) http.Handler {
func (a *API) RegisterAlertmanager(am *alertmanager.MultitenantAlertmanager, target, apiEnabled bool) {
a.indexPage.AddLink(SectionAdminEndpoints, "/multitenant_alertmanager/status", "Alertmanager Status")
// Ensure this route is registered before the prefixed AM route
a.RegisterRoute("/multitenant_alertmanager/status", am.GetStatusHandler(), false)
a.RegisterRoute("/multitenant_alertmanager/status", am.GetStatusHandler(), false, "GET")
Copy link
Contributor Author

@pracucci pracucci Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gotjosh Do you see problem with this?


// UI components lead to a large number of routes to support, utilize a path prefix instead
a.RegisterRoutesWithPrefix(a.cfg.AlertmanagerHTTPPrefix, am, true)
Expand All @@ -157,7 +163,7 @@ func (a *API) RegisterAlertmanager(am *alertmanager.MultitenantAlertmanager, tar
// If the target is Alertmanager, enable the legacy behaviour. Otherwise only enable
// the component routed API.
if target {
a.RegisterRoute("/status", am.GetStatusHandler(), false)
a.RegisterRoute("/status", am.GetStatusHandler(), false, "GET")
a.RegisterRoutesWithPrefix(a.cfg.LegacyHTTPPrefix, am, true)
}

Expand All @@ -173,25 +179,25 @@ func (a *API) RegisterAlertmanager(am *alertmanager.MultitenantAlertmanager, tar
func (a *API) RegisterAPI(httpPathPrefix string, cfg interface{}) {
a.indexPage.AddLink(SectionAdminEndpoints, "/config", "Current Config")

a.RegisterRoute("/config", configHandler(cfg), false)
a.RegisterRoute("/", indexHandler(httpPathPrefix, a.indexPage), false)
a.RegisterRoute("/debug/fgprof", fgprof.Handler(), false)
a.RegisterRoute("/config", configHandler(cfg), false, "GET")
a.RegisterRoute("/", indexHandler(httpPathPrefix, a.indexPage), false, "GET")
a.RegisterRoute("/debug/fgprof", fgprof.Handler(), false, "GET")
}

// RegisterDistributor registers the endpoints associated with the distributor.
func (a *API) RegisterDistributor(d *distributor.Distributor, pushConfig distributor.Config) {
a.RegisterRoute("/api/v1/push", push.Handler(pushConfig, a.sourceIPs, d.Push), true)
a.RegisterRoute("/api/v1/push", push.Handler(pushConfig, a.sourceIPs, d.Push), true, "POST")

a.indexPage.AddLink(SectionAdminEndpoints, "/distributor/all_user_stats", "Usage Statistics")
a.indexPage.AddLink(SectionAdminEndpoints, "/distributor/ha_tracker", "HA Tracking Status")

a.RegisterRoute("/distributor/all_user_stats", http.HandlerFunc(d.AllUserStatsHandler), false)
a.RegisterRoute("/distributor/ha_tracker", d.HATracker, false)
a.RegisterRoute("/distributor/all_user_stats", http.HandlerFunc(d.AllUserStatsHandler), false, "GET")
a.RegisterRoute("/distributor/ha_tracker", d.HATracker, false, "GET")

// Legacy Routes
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/push", push.Handler(pushConfig, a.sourceIPs, d.Push), true)
a.RegisterRoute("/all_user_stats", http.HandlerFunc(d.AllUserStatsHandler), false)
a.RegisterRoute("/ha-tracker", d.HATracker, false)
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/push", push.Handler(pushConfig, a.sourceIPs, d.Push), true, "POST")
a.RegisterRoute("/all_user_stats", http.HandlerFunc(d.AllUserStatsHandler), false, "GET")
a.RegisterRoute("/ha-tracker", d.HATracker, false, "GET")
}

// RegisterIngester registers the ingesters HTTP and GRPC service
Expand All @@ -200,14 +206,14 @@ func (a *API) RegisterIngester(i *ingester.Ingester, pushConfig distributor.Conf

a.indexPage.AddLink(SectionDangerous, "/ingester/flush", "Trigger a Flush of data from Ingester to storage")
a.indexPage.AddLink(SectionDangerous, "/ingester/shutdown", "Trigger Ingester Shutdown (Dangerous)")
a.RegisterRoute("/ingester/flush", http.HandlerFunc(i.FlushHandler), false)
a.RegisterRoute("/ingester/shutdown", http.HandlerFunc(i.ShutdownHandler), false)
a.RegisterRoute("/ingester/push", push.Handler(pushConfig, a.sourceIPs, i.Push), true) // For testing and debugging.
a.RegisterRoute("/ingester/flush", http.HandlerFunc(i.FlushHandler), false, "GET", "POST")
a.RegisterRoute("/ingester/shutdown", http.HandlerFunc(i.ShutdownHandler), false, "GET", "POST")
a.RegisterRoute("/ingester/push", push.Handler(pushConfig, a.sourceIPs, i.Push), true, "POST") // For testing and debugging.

// Legacy Routes
a.RegisterRoute("/flush", http.HandlerFunc(i.FlushHandler), false)
a.RegisterRoute("/shutdown", http.HandlerFunc(i.ShutdownHandler), false)
a.RegisterRoute("/push", push.Handler(pushConfig, a.sourceIPs, i.Push), true) // For testing and debugging.
a.RegisterRoute("/flush", http.HandlerFunc(i.FlushHandler), false, "GET", "POST")
a.RegisterRoute("/shutdown", http.HandlerFunc(i.ShutdownHandler), false, "GET", "POST")
a.RegisterRoute("/push", push.Handler(pushConfig, a.sourceIPs, i.Push), true, "POST") // For testing and debugging.
}

// RegisterPurger registers the endpoints associated with the Purger/DeleteStore. They do not exactly
Expand All @@ -230,10 +236,10 @@ func (a *API) RegisterPurger(store *purger.DeleteStore, deleteRequestCancelPerio
// API is not enabled only the ring route is registered.
func (a *API) RegisterRuler(r *ruler.Ruler, apiEnabled bool) {
a.indexPage.AddLink(SectionAdminEndpoints, "/ruler/ring", "Ruler Ring Status")
a.RegisterRoute("/ruler/ring", r, false)
a.RegisterRoute("/ruler/ring", r, false, "GET", "POST")

// Legacy Ring Route
a.RegisterRoute("/ruler_ring", r, false)
a.RegisterRoute("/ruler_ring", r, false, "GET", "POST")

if apiEnabled {
// Prometheus Rule API Routes
Expand Down Expand Up @@ -267,24 +273,24 @@ func (a *API) RegisterRuler(r *ruler.Ruler, apiEnabled bool) {
// RegisterRing registers the ring UI page associated with the distributor for writes.
func (a *API) RegisterRing(r *ring.Ring) {
a.indexPage.AddLink(SectionAdminEndpoints, "/ingester/ring", "Ingester Ring Status")
a.RegisterRoute("/ingester/ring", r, false)
a.RegisterRoute("/ingester/ring", r, false, "GET", "POST")

// Legacy Route
a.RegisterRoute("/ring", r, false)
a.RegisterRoute("/ring", r, false, "GET", "POST")
}

// RegisterStoreGateway registers the ring UI page associated with the store-gateway.
func (a *API) RegisterStoreGateway(s *storegateway.StoreGateway) {
storegatewaypb.RegisterStoreGatewayServer(a.server.GRPC, s)

a.indexPage.AddLink(SectionAdminEndpoints, "/store-gateway/ring", "Store Gateway Ring")
a.RegisterRoute("/store-gateway/ring", http.HandlerFunc(s.RingHandler), false)
a.RegisterRoute("/store-gateway/ring", http.HandlerFunc(s.RingHandler), false, "GET", "POST")
}

// RegisterCompactor registers the ring UI page associated with the compactor.
func (a *API) RegisterCompactor(c *compactor.Compactor) {
a.indexPage.AddLink(SectionAdminEndpoints, "/compactor/ring", "Compactor Ring Status")
a.RegisterRoute("/compactor/ring", http.HandlerFunc(c.RingHandler), false)
a.RegisterRoute("/compactor/ring", http.HandlerFunc(c.RingHandler), false, "GET", "POST")
}

// RegisterQuerier registers the Prometheus routes supported by the
Expand Down Expand Up @@ -322,11 +328,11 @@ func (a *API) RegisterQuerier(
)

// these routes are always registered to the default server
a.RegisterRoute("/api/v1/user_stats", http.HandlerFunc(distributor.UserStatsHandler), true)
a.RegisterRoute("/api/v1/chunks", querier.ChunksHandler(queryable), true)
a.RegisterRoute("/api/v1/user_stats", http.HandlerFunc(distributor.UserStatsHandler), true, "GET")
a.RegisterRoute("/api/v1/chunks", querier.ChunksHandler(queryable), true, "GET")

a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/user_stats", http.HandlerFunc(distributor.UserStatsHandler), true)
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/chunks", querier.ChunksHandler(queryable), true)
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/user_stats", http.HandlerFunc(distributor.UserStatsHandler), true, "GET")
a.RegisterRoute(a.cfg.LegacyHTTPPrefix+"/chunks", querier.ChunksHandler(queryable), true, "GET")

// these routes are either registered the default server OR to an internal mux. The internal mux is
// for use in a single binary mode when both the query frontend and the querier would attempt to claim these routes
Expand Down Expand Up @@ -424,5 +430,5 @@ func (a *API) RegisterQueryFrontend(f *frontend.Frontend) {
// or a future module manager #2291
func (a *API) RegisterServiceMapHandler(handler http.Handler) {
a.indexPage.AddLink(SectionAdminEndpoints, "/services", "Service Status")
a.RegisterRoute("/services", handler, false)
a.RegisterRoute("/services", handler, false, "GET")
}
1 change: 0 additions & 1 deletion pkg/cortex/index.go
View file
Open in desktop

This file was deleted.