API Implementation #2372

jtlisi · 2020-03-31T23:07:33Z

What this PR does:
This is the implementation associated with the design document in #2327.

In this implementation the API struct mainly just handles routing for Cortex. In the future this package will centralize non-component based handlers and create a set of functions and structs that can be reused by Cortex handlers to make the API consistent.

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Good job @jtlisi. It quite a tricky PR to review to accurately find if breaking changes have been introduced. I've left some comments and questions, but overall the design looks good to me.

I also have few more comments:

Could you update docs/apis.md accordingly, please?
What's the impact of these changes on Loki?
Can you also reflect the changes in the CHANGELOG? Ie. I noticed these changes (but there are probably more):
- Alertmanager: /status is available only when running in microservices mode and the target is the alertmanager
- Ruler: /ruler_ring > /ruler/ring

pkg/cortex/modules.go

pkg/api/api.go

pracucci · 2020-04-06T08:08:37Z

pkg/cortex/modules.go

@@ -98,6 +90,22 @@ func (m *ModuleName) UnmarshalYAML(unmarshal func(interface{}) error) error {
 	return m.Set(s)
 }

+func (t *Cortex) initAPI(cfg *Config) (services.Service, error) {
+	cfg.API.ServerPrefix = cfg.Server.PathPrefix
+	cfg.API.LegacyHTTPPrefix = cfg.HTTPPrefix


This is potentially a breaking change. If an user has customised HTTPPrefix, the behaviour will be different, because for most of the routes before the /api/prom was hard-coded while now it's always based on HTTPPrefix. Should be at least mentioned in the CHANGELOG as CHANGE, but not sure if it breaks the 1.0.0 stability guarantees.

I'm not sure either. I always felt it was an issue if not a bug that services did not respect the configured prefix. I will add a line in the changelog explaining the change in behaviour. However, I don't think this goes against any of our 1.0 guarantees:

Additional API endpoints for management of Cortex itself, such as the ring. These APIs are not part of the any compatibility guarantees.

pkg/api/api.go

pkg/api/handlers.go

Signed-off-by: Jacob Lisi <[email protected]>

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: Jacob Lisi <[email protected]>

Signed-off-by: Jacob Lisi <[email protected]>

pracucci · 2020-04-07T16:02:21Z

pkg/api/api.go

+		handler = a.authMiddleware.Wrap(handler)
+	}
+	if len(methods) == 0 {
+		a.server.HTTP.Path(path).Handler(handler)


a.server.HTTP.Path() > a.server.HTTP.PathPrefix()

Sorry about that, I was just looking into the failed integration tests and that looked to be the issue. Also the primary reason for not explicitly registering the alertmanager routes is because the Alertmanager UI routing is relatively complex and I didn't want to handle it in this PR. I also think it is fine to serve the Alertmanager directly if it is routed under the /alertmanager/ prefix.

pracucci · 2020-04-07T16:04:12Z

pkg/api/api.go

+// 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, d.Push), true)
+	a.registerRoute("/distributor/all_user_stats", http.HandlerFunc(d.AllUserStatsHandler), false)


Then I think all new routes should use underscores (I also prefer dashes, but my personal preference doesn't matter when it breaks consistency, so agree on underscores).

pracucci · 2020-04-07T16:07:45Z

pkg/api/api.go

+	a.server.HTTP.Path(path).Methods(methods...).Handler(handler)
+}
+
+func (a *API) registerRoutesWithPrefix(path string, handler http.Handler, auth bool, methods ...string) {


I would rename path to prefix to make it clearer

pracucci · 2020-04-07T16:08:23Z

docs/configuration/config-file-reference.md

@@ -62,6 +62,15 @@ Where default_value is the value to use if the environment variable is undefined
 # CLI flag: -http.prefix
 [http_prefix: <string> | default = "/api/prom"]

+api:
+  # Base path for data storage.


Remember to address this.

Signed-off-by: Jacob Lisi <[email protected]>

pracucci

Thanks @jtlisi for addressing my feedback. I've few more minor comments. May you also look at the failing lint and integration tests, please?

pkg/api/api.go

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi · 2020-04-07T16:43:37Z

I just wanted to circle back and address:

Alertmanager: /status is available only when running in microservices mode and the target is the alertmanager

I don't know if that is worth addressing since there is no current way to run the alertmanager in the single binary. Even after this PR that functionality has not been enabled.

Signed-off-by: Jacob Lisi <[email protected]>

pracucci · 2020-04-07T16:50:03Z

I just wanted to circle back and address:

Alertmanager: /status is available only when running in microservices mode and the target is the alertmanager

I don't know if that is worth addressing since there is no current way to run the alertmanager in the single binary. Even after this PR that functionality has not been enabled.

🤦‍♂ You're definitely right. All good on this 👍

Signed-off-by: Jacob Lisi <[email protected]>

pracucci

Good job @jtlisi! LGTM. I just left few minor nits. Would be great to update the docs/apis.md (not necessarily in this PR, we can do in a subsequent one, but would be great doing it).

pracucci · 2020-04-07T16:51:04Z

CHANGELOG.md

@@ -2,6 +2,9 @@

 ## master / unreleased

+* [CHANGE] Added v1 API routes documented in #2327. #2372
+  * Added `-http.alertmanager-http-prefix` and `http.prometheus-http-prefix` flags.


Suggested change

* Added `-http.alertmanager-http-prefix` and `http.prometheus-http-prefix` flags.

* Added `-http.alertmanager-http-prefix` and `-http.prometheus-http-prefix` flags.

pracucci · 2020-04-07T16:51:42Z

CHANGELOG.md

@@ -2,6 +2,9 @@

 ## master / unreleased

+* [CHANGE] Added v1 API routes documented in #2327. #2372
+  * Added `-http.alertmanager-http-prefix` and `http.prometheus-http-prefix` flags.
+  * Updated the index hosted at the root prefix to point to the updated routes.


Could you try to make it more clear? I think it's a bit difficult to understand for the final user.

pracucci · 2020-04-07T17:01:13Z

CHANGELOG.md

+* [CHANGE] Added v1 API routes documented in #2327. #2372
+  * Added `-http.alertmanager-http-prefix` and `http.prometheus-http-prefix` flags.
+  * Updated the index hosted at the root prefix to point to the updated routes.
+  * Legacy routes hardcoded with the `/api/prom` prefix now respect the `http.prefix` flag.


Suggested change

* Legacy routes hardcoded with the `/api/prom` prefix now respect the `http.prefix` flag.

* Legacy routes hardcoded with the `/api/prom` prefix now respect the `-http.prefix` flag.

Signed-off-by: Jacob Lisi <[email protected]>

pull-request-size bot added the size/XL label Mar 31, 2020

jtlisi marked this pull request as ready for review April 2, 2020 14:04

pracucci reviewed Apr 6, 2020

View reviewed changes

jtlisi force-pushed the 20200326_api_spike branch from 0fb552c to 0295435 Compare April 6, 2020 22:53

jtlisi and others added 21 commits April 6, 2020 19:27

initial v1 api routes except AM

c725dcc

Signed-off-by: Jacob Lisi <[email protected]>

all modules registered using API

480e927

Signed-off-by: Jacob Lisi <[email protected]>

update vendor directory and remove unused handlers

424b596

Signed-off-by: Jacob Lisi <[email protected]>

disable single binary alertmanager & ruler

701dafd

Signed-off-by: Jacob Lisi <[email protected]>

add logging to api package

4c8fb40

Signed-off-by: Jacob Lisi <[email protected]>

remove unused auth setup in initAPI

92fce99

Signed-off-by: Jacob Lisi <[email protected]>

revert gorilla mux to current version

bd0306e

Signed-off-by: Jacob Lisi <[email protected]>

remove unneeded vendored import

b859d39

Signed-off-by: Jacob Lisi <[email protected]>

revert go.sum

2627e70

Signed-off-by: Jacob Lisi <[email protected]>

remove unused field

322942a

Signed-off-by: Jacob Lisi <[email protected]>

update docs

cd2d2e5

Signed-off-by: Jacob Lisi <[email protected]>

ensure legacy prefix is propagated to API module

e34b4c3

Signed-off-by: Jacob Lisi <[email protected]>

goimports for handlers.go

2da693d

Signed-off-by: Jacob Lisi <[email protected]>

ensure prom wrapped with fakeAddr

27dfd7d

Signed-off-by: Jacob Lisi <[email protected]>

fix prefix for promrouter

6b913e5

Signed-off-by: Jacob Lisi <[email protected]>

use single util function for route registration

817b88b

Signed-off-by: Jacob Lisi <[email protected]>

ensure correct methods are used for prometheus API calls

c6fe850

Signed-off-by: Jacob Lisi <[email protected]>

remove unused prometheuRouter

1da249e

Signed-off-by: Jacob Lisi <[email protected]>

Update pkg/api/api.go

4579066

Co-Authored-By: Marco Pracucci <[email protected]> Signed-off-by: Jacob Lisi <[email protected]>

address straight forward PR feedback

1edcecb

Signed-off-by: Jacob Lisi <[email protected]>

remove ingester specific health check

4dabad2

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi force-pushed the 20200326_api_spike branch from eec97a3 to 4dabad2 Compare April 6, 2020 23:35

fix import error introduced in rebase

42c0be8

Signed-off-by: Jacob Lisi <[email protected]>

pracucci reviewed Apr 7, 2020

View reviewed changes

fix alertmanager routing and update docs

b4752da

Signed-off-by: Jacob Lisi <[email protected]>

pracucci reviewed Apr 7, 2020

View reviewed changes

pkg/api/api.go Outdated Show resolved Hide resolved

fix typo

26a5126

Signed-off-by: Jacob Lisi <[email protected]>

update docs to fix typo

941d2e3

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi linked an issue Apr 7, 2020 that may be closed by this pull request

Server path prefix flag is harmful #2411

Closed

jtlisi added 2 commits April 7, 2020 12:57

add bugfix to changelog

6ea96bb

Signed-off-by: Jacob Lisi <[email protected]>

add http prefix config to changelog

f09a2af

Signed-off-by: Jacob Lisi <[email protected]>

pracucci approved these changes Apr 7, 2020

View reviewed changes

clarify changelog

b3d9c2e

Signed-off-by: Jacob Lisi <[email protected]>

jtlisi merged commit 1f600cb into cortexproject:master Apr 8, 2020

jtlisi deleted the 20200326_api_spike branch April 8, 2020 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Implementation #2372

API Implementation #2372

jtlisi commented Mar 31, 2020 •

edited

Loading

pracucci left a comment •

edited

Loading

pracucci Apr 6, 2020

jtlisi Apr 6, 2020

pracucci Apr 7, 2020 •

edited

Loading

jtlisi Apr 7, 2020

pracucci Apr 7, 2020

pracucci Apr 7, 2020

jtlisi Apr 7, 2020

pracucci Apr 7, 2020

pracucci left a comment

jtlisi commented Apr 7, 2020

pracucci commented Apr 7, 2020

pracucci left a comment

pracucci Apr 7, 2020

pracucci Apr 7, 2020

pracucci Apr 7, 2020

	* Added `-http.alertmanager-http-prefix` and `http.prometheus-http-prefix` flags.
	* Added `-http.alertmanager-http-prefix` and `-http.prometheus-http-prefix` flags.

	* Legacy routes hardcoded with the `/api/prom` prefix now respect the `http.prefix` flag.
	* Legacy routes hardcoded with the `/api/prom` prefix now respect the `-http.prefix` flag.

API Implementation #2372

API Implementation #2372

Conversation

jtlisi commented Mar 31, 2020 • edited Loading

pracucci left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

jtlisi commented Apr 7, 2020

pracucci commented Apr 7, 2020

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtlisi commented Mar 31, 2020 •

edited

Loading

pracucci left a comment •

edited

Loading

pracucci Apr 7, 2020 •

edited

Loading