Expose current Prometheus config via /status/config #2711

mxinden · 2017-05-11T15:31:37Z

This PR adds the /status/config endpoint which exposes the currently
loaded Prometheus config. This is the same config that is displayed on
/config in the UI. The response payload looks like such:

{
  "status": "success",
  "data": {
    "yaml": "<CONFIG>"
  }
}

Related to PR #2600 and issue #2467

fabxc · 2017-05-15T09:13:07Z

web/api/v1/api.go

@@ -103,17 +103,21 @@ type API struct {
 	targetRetriever       targetRetriever
 	alertmanagerRetriever alertmanagerRetriever

-	now func() model.Time
+	now                  func() model.Time
+	retrieveConfigString configStringFunc


Just call it config and make it a plain func() string. Not everything needs a dedicated type, especially if it's such a one-off thing.

mxinden · 2017-05-15T09:35:29Z

@fabxc Updated PR, renamed to configString like in web/web.go as config is already taken by API endpoint function. This exposes the same string as the UI. Does this need any further security considerations?

fabxc · 2017-05-15T17:51:43Z

#2722 might make us want to reconsider this and switch to properly marshalled configs.

mxinden · 2017-05-29T16:49:56Z

@fabxc As #2775 and #2722 are merged and closed. Can you re-review this PR? Rebased with current master.

grobie · 2017-05-30T21:18:34Z

web/api/v1/api.go

@@ -436,6 +440,17 @@ func (api *API) alertmanagers(r *http.Request) (interface{}, *apiError) {
 	return ams, nil
 }

+type prometheusConfig struct {
+	RawFormat string `json:"rawFormat"`


I wonder if rawFormat is the best name here. I might be wrong, but isn't this the representation without comments, secrets and reformatted by our printer?

agreed, as this is parsed and marshaled again to yaml, it makes most sense to call this maybe yaml instead

Renamed to YAML and yaml. Thanks for the review!

brancz · 2017-06-22T12:22:00Z

lgtm

fabxc · 2017-06-22T12:33:28Z

I thought we changed our mind about this since we are no longer preserving the actual text input but actually re-marshal the parsed YAML. Without the original comments there's no real value in providing embedded YAML over JSON here.

mxinden · 2017-06-22T15:02:26Z

@fabxc You are right. Sorry about that. Do you want to provide both, or just JSON?

fabxc · 2017-06-22T15:34:22Z

No worries. I think any client that wants to show YAML can just implement the conversion themselves, so just JSON seems fine. I'm wondering though, since we have quite a few more things in our status tab in the UI, whether we really want to have a distinct endpoint for each or whether we should just have a single one that has an object with all pieces of data we show on those pages.

The endpoint should be hit very rarely (for health checks we want to implement a dedicated endpoint), so I don't see any performance concerns around that.

@brian-brazil @beorn7

brian-brazil · 2017-06-22T16:19:33Z

My instinct would be one endpoint for each type. Some of the things we're going on have on the status pages as we grow them will get pretty heavy, and not as dumb as the config file.

fabxc · 2017-06-23T09:11:55Z

Okay, fine too.

mxinden · 2017-06-23T17:18:49Z

@fabxc @brian-brazil I have added the following changes:

Expose JSON instead of YAML in api/v1/status/config
Clean up Handler struct. Removing configString and externalLabels
Elide secret in JSON marsheling
Add test for secret hiding in JSON marsheling
Add JSON format string to Config struct and all sub structs

Let me know what you think. Make sure to also review config/config.go even though it is hidden due to the size of the diff.

brian-brazil · 2017-06-26T08:05:27Z

Elide secret in JSON marsheling

Duplicating this logic is risky security wise, especially as features created here will be copied by other repos. I think we should stick with one format.

mxinden · 2017-06-26T08:33:06Z

@brian-brazil You mean either changing the front-end to JSON or exposing YAML via the API endpoint?

brian-brazil · 2017-06-26T09:31:30Z

I'd go for the latter. Mixing JSON and YAML would cause confusion I expect.

mxinden · 2017-06-26T12:03:00Z

@brian-brazil I think exposing YAML in a pure JSON api is not a great user experience.
I do see the risk of having the elide secret logic twice in our code.

One workaround would be to first marshal the config as YAML, which would elide all secrets, and then convert YAML to JSON which is then exposed via the API. It turns out to be quite costly to implement that from scratch, so I would prefer using something like https://github.com/ghodss/yaml.

Second alternative would be to switch the Prometheus config.go entirely to JSON and use the library above to convert incoming YAML to JSON. This sounds a lot cleaner to me.

What do you think?

fabxc · 2017-06-26T12:07:32Z

@brian-brazil more precisely, the YAML unmarshaller creates map[interface{}]interface{} for nested objects where encoding/json only can deal with map[string]interface{}. The conversion to work around this is here.

brian-brazil · 2017-06-26T12:15:08Z

I think exposing YAML in a pure JSON api is not a great user experience.

I don't think there's a perfect solution here. I'd view it more as an opaque config string (which happens to be in YAML), being returned via an RPC mechanism (which happens to be JSON).

fabxc · 2017-06-26T13:44:19Z

The config is structured data. What's the point of exposing it as an opaque string?

brian-brazil · 2017-06-26T14:20:28Z

If the goal is to view the current configuration (as we currently have on the status page), then that's sufficient. I'm not imagining too much processing of the config beyond checking that the right config is loaded.

mxinden · 2017-07-14T16:09:15Z

@fabxc @brian-brazil I still find it inconsistent to (only) expose YAML on a JSON API. E.g. for the Prometheus Operator we would like to process the returned config.

Would one of these two options work:

Adding more tests to make sure no secrets are leaked. (Does not cover copies of other repos)
Using https://github.com/ghodss/yaml to convert the YAML with elided secrets to JSON.

brian-brazil · 2017-07-14T16:26:02Z

I still find it inconsistent to (only) expose YAML on a JSON API.

That's kinda like arguing that if you're returning HTML over a JSON API you should convert its XML structure to JSON first. I don't see an inconsistency here, what serialisation protocol you're using for RPCs shouldn't automatically affect what data you're transporting.

fabxc · 2017-07-14T21:23:15Z

Returning HTML over a JSON API is an equally bad idea :)

…

On Fri, Jul 14, 2017, 6:26 PM Brian Brazil ***@***.***> wrote: I still find it inconsistent to (only) expose YAML on a JSON API. That's kinda like arguing that if you're returning HTML over a JSON API you should convert its XML structure to JSON first. I don't see an inconsistency here, what serialisation protocol you're using for RPCs shouldn't automatically affect what data you're transporting. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEuA8gQQ2Q6jIPOgJWiQsZn7kpnnTTnKks5sN5algaJpZM4NYLOC> .

fabxc · 2017-07-14T21:24:32Z

The config is structured data. Showing it as YAML again in a UI is one possibility. But it shouldn't dictate the API On Fri, Jul 14, 2017, 11:23 PM Fabian Reinartz <fab.reinartz@gmail.com> wrote:

…

Returning HTML over a JSON API is an equally bad idea :) On Fri, Jul 14, 2017, 6:26 PM Brian Brazil ***@***.***> wrote: > I still find it inconsistent to (only) expose YAML on a JSON API. > > That's kinda like arguing that if you're returning HTML over a JSON API > you should convert its XML structure to JSON first. I don't see an > inconsistency here, what serialisation protocol you're using for RPCs > shouldn't automatically affect what data you're transporting. > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#2711 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEuA8gQQ2Q6jIPOgJWiQsZn7kpnnTTnKks5sN5algaJpZM4NYLOC> > . >

brian-brazil · 2017-07-14T21:45:27Z

When we've the proto/GRPC variant of this API, would you have that also return JSON or should it be a proto version of the config?

fabxc · 2017-07-14T22:25:23Z

The latter

…

On Fri, Jul 14, 2017, 11:45 PM Brian Brazil ***@***.***> wrote: When we've the proto variant of this API, would you have that also return JSON or should it be a proto version of the config? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2711 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEuA8t1dZ9GCbfu3VSFaKwRH3Evr6FMJks5sN-F9gaJpZM4NYLOC> .

brian-brazil · 2017-07-14T22:41:55Z

You're internally consistent anyway, but that'd be a silly amount of maintenance effort as you'd have to keep a separate copy of the config structure in proto to get all the field numbers right - and that'd also not be forwards compatible for any new fields added.

I don't think changing the packet format to match that of the frame is at all sane.

brian-brazil · 2017-07-18T13:38:56Z

Looking at this from another direction, what would be the objection to expecting users of this to parse the YAML on their end? What are the use cases for doing so?

Keep in mind that it is our goal ultimately to have comments back in the marshalled config, we just can't do that currently due to secrets.

mxinden · 2017-08-07T21:24:34Z

I have updated the PR to expose the config in YAML format.
@fabxc @brian-brazil @brancz Let me know if this looks good to you. Very happy for any further feedback.

Just in case we ever want to expose the config as JSON, instead of writing the JSON format strings all over again, they can be found here: https://github.com/mxinden/prometheus/tree/api-config-json-backup

brian-brazil · 2017-08-07T21:40:50Z

👍

You have conflicts.

mxinden · 2017-08-07T22:01:18Z

@brian-brazil Thanks for the hint. Rebased.

brancz · 2017-08-08T07:49:37Z

web/federate.go

@@ -74,7 +74,7 @@ func (h *Handler) federation(w http.ResponseWriter, req *http.Request) {
 	}
 	sort.Sort(byName(vector))

-	externalLabels := h.externalLabels.Clone()
+	externalLabels := h.config.GlobalConfig.ExternalLabels.Clone()


I don't think this is intended to be part of this PR right? I think something may have gone wrong when rebasing.

@brancz Instead of having configString and externalLabels in the Handler struct, I propose with this PR just having the config itself in there. Whenever we need externalLabels and configString we can get it from Handler.config.String() and Handler.config.GlobalConfig.ExternalLabels.

I find this cleaner. In addition it enables us to access more than just the external labels and the config string in both web.go and api.go in the future. @brancz What do you think?

Gotcha, fine with it, just wanted to make sure this was intentional as you just rebased.

@brancz Appreciate the mindfulness!

brancz · 2017-08-08T09:57:26Z

lgtm 👍

mxinden · 2017-08-09T21:52:00Z

I am not a member of the Prometheus GitHub organization. Thereby I am not able to merge here.

@brancz @fabxc @brian-brazil In case you have no further objections, could you merge this PR?

brian-brazil · 2017-08-09T21:57:26Z

Hmm, you're on -team so you should be in the org. Invite sent.

mxinden · 2017-08-13T20:07:40Z

@brian-brazil Thanks for the invitation!

@brancz and @brian-brazil I had to do one more fix here: YAML string 'yaml:"yaml"' -> YAML string 'json:"yaml"' . Please merge in case you are fine with all of this.

brian-brazil

👍

brian-brazil · 2017-08-13T20:11:37Z

config/config_test.go

@@ -540,15 +540,27 @@ func TestLoadConfig(t *testing.T) {
 	if !reflect.DeepEqual(c, expectedConf) {
 		t.Fatalf("%s: unexpected config result: \n\n%s\n expected\n\n%s", "testdata/conf.good.yml", bgot, bexp)
 	}
+}
+
+// YAML marsheling must not reveal authentication credentials.


marshalling

This PR adds the `/status/config` endpoint which exposes the currently loaded Prometheus config. This is the same config that is displayed on `/config` in the UI in YAML format. The response payload looks like such: ``` { "status": "success", "data": { "yaml": <CONFIG> } } ```

mxinden force-pushed the api-config branch from 87e5331 to 106ed3c Compare May 11, 2017 15:35

fabxc reviewed May 15, 2017

View reviewed changes

mxinden force-pushed the api-config branch from 106ed3c to b9d5bc2 Compare May 15, 2017 09:44

mxinden mentioned this pull request May 16, 2017

Prometheus doesn't elide all secrets in config #2722

Closed

mxinden force-pushed the api-config branch from b9d5bc2 to b0dbe23 Compare May 29, 2017 16:32

grobie reviewed May 30, 2017

View reviewed changes

mxinden force-pushed the api-config branch from b0dbe23 to e77ace5 Compare June 1, 2017 13:14

mxinden force-pushed the api-config branch from e77ace5 to 837d380 Compare June 22, 2017 12:14

mxinden force-pushed the api-config branch 9 times, most recently from e4f28db to e5bd75c Compare June 23, 2017 17:13

mxinden mentioned this pull request Jul 14, 2017

Data accessible via API #2467

Closed

mxinden force-pushed the api-config branch 2 times, most recently from 732434d to e5d1824 Compare August 7, 2017 21:20

mxinden force-pushed the api-config branch from e5d1824 to be2ec79 Compare August 7, 2017 21:59

brancz reviewed Aug 8, 2017

View reviewed changes

mxinden force-pushed the api-config branch from be2ec79 to c75f8ba Compare August 13, 2017 19:11

brian-brazil reviewed Aug 13, 2017

View reviewed changes

mxinden force-pushed the api-config branch from c75f8ba to 1c96fbb Compare August 13, 2017 20:21

mxinden merged commit 3101606 into prometheus:master Aug 14, 2017

Expose current Prometheus config via /status/config #2711

Expose current Prometheus config via /status/config #2711

Conversation

mxinden commented May 11, 2017 • edited

Choose a reason for hiding this comment

mxinden commented May 15, 2017

fabxc commented May 15, 2017

mxinden commented May 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brancz commented Jun 22, 2017

fabxc commented Jun 22, 2017

mxinden commented Jun 22, 2017

fabxc commented Jun 22, 2017

brian-brazil commented Jun 22, 2017

fabxc commented Jun 23, 2017

mxinden commented Jun 23, 2017

brian-brazil commented Jun 26, 2017

mxinden commented Jun 26, 2017

brian-brazil commented Jun 26, 2017

mxinden commented Jun 26, 2017

fabxc commented Jun 26, 2017

brian-brazil commented Jun 26, 2017

fabxc commented Jun 26, 2017

brian-brazil commented Jun 26, 2017

mxinden commented Jul 14, 2017

brian-brazil commented Jul 14, 2017

fabxc commented Jul 14, 2017 via email

fabxc commented Jul 14, 2017 via email

brian-brazil commented Jul 14, 2017 • edited

fabxc commented Jul 14, 2017 via email

brian-brazil commented Jul 14, 2017

brian-brazil commented Jul 18, 2017

mxinden commented Aug 7, 2017

brian-brazil commented Aug 7, 2017

mxinden commented Aug 7, 2017

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

brancz commented Aug 8, 2017

mxinden commented Aug 9, 2017

brian-brazil commented Aug 9, 2017

mxinden commented Aug 13, 2017

brian-brazil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented May 11, 2017 •

edited

brian-brazil commented Jul 14, 2017 •

edited