Skip to content
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.

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

[Feature] Rule: Add authentication for alertmanagers #606

Closed
FUSAKLA opened this issue Nov 2, 2018 · 8 comments · Fixed by #1838
Closed

[Feature] Rule: Add authentication for alertmanagers #606

FUSAKLA opened this issue Nov 2, 2018 · 8 comments · Fixed by #1838

Comments

@FUSAKLA
Copy link
Member

FUSAKLA commented Nov 2, 2018

It would be great to add possibility to authenticate to Alertmanagers from Rule node when sending alerts the same way as Prometheus allows.

With current configuration using --alertmanagers.url it would have to be common authentication configuration for all those AM I suppose.

Prometheus now supports authentication using client certificate, BasicAuth and bearer token.
I'm not saying there should be all of them but at least one of them would be great.

@FUSAKLA
Copy link
Member Author

FUSAKLA commented Nov 26, 2018

@bwplotka
Do you think this is out of Thanos scope or is there any chance to add this?

I'm willing to work on it but would need to get some guidance on how to do this (configuration, ways of auth and so on).

Only workaround I can think of is adding proxy which would provide header with bearer token to all outgoing requests which sounds like a hack :/

@bwplotka
Copy link
Member

I think, since Prometheus have it we should stick to this to be close possible with Prometheus expierience, as we internally reuse same code usually (if not, we should - we sometimes don't due to package config leaking).

So essentially:
AC:

  • Ruler alertmanagers can be configured with different auth options.
  • Reuse code as much possible from Prometheus (even if this means some contributions upstream)

Now with configuration, I am not sure. As you said with flags we would be limited to common options for all AM - this might be limited. Maybe we need to switch to something like objstore.config but for AM and take it from file that looks similar to AM section in Prometheus. Thoughts?

@FUSAKLA
Copy link
Member Author

FUSAKLA commented Jun 12, 2019

Yeah, config file with the structure of

# Alerting specifies settings related to the Alertmanager.
alerting:
  alert_relabel_configs:
    [ - <relabel_config> ... ]
  alertmanagers:
    [ - <alertmanager_config> ... ]

With https://prometheus.io/docs/prometheus/latest/configuration/configuration/#alertmanager_config
same as Prometheus has could be a good way. The question is how far do we want to take this.
Possibly we could then load even external labels, evaluation interval etc but that is probably for another discussion.
So +1 from me to the config reuse. It will be even easier to share the config with vanilla Prometheus instances.
Just to be clear we will stick just to the DNS service discovery for AM right?

@FUSAKLA
Copy link
Member Author

FUSAKLA commented Jun 12, 2019

Also you mentioned rotation of certs.
I'm not sure if Prometheus supports this and if we want to? I remember it was discussed in some other issue but I'm not sure how that ended up.

@bwplotka
Copy link
Member

Let's get back to this (:

I think I would vote for having alertmanager.config<-file> flag which would control this per endpoint basis. I would vote for static and DNS service discovery only indeed. We could have nice reload based on this: https://github.com/prometheus/common/tree/master/config

Which is exactly as AC stated above

cc @simonpasquier

@simonpasquier
Copy link
Contributor

Thanks @bwplotka, I'm planning to work on this and will be sending a PR soon.

@bwplotka
Copy link
Member

I was thinking about something like this:

       // A set of query parameters with which the target is scraped.
       Params url.Values `yaml:"params,omitempty"`
       // The HTTP resource path on with Alertmanager API endpoint.
       Path string `yaml:"path,omitempty"`
       // The URL scheme with which to fetch metrics from targets.
       Scheme string `yaml:"scheme,omitempty"`
       // List of addresses with dns prefixes.
       StaticAddresses []string `yaml:"file_sd_configs,omitempty"`
       // List of file  configurations (our FileSD supports different dns lookup)
       FileSDConfigs []*file.SDConfig `yaml:"file_sd_configs,omitempty"`
       HTTPClientConfig       config_util.HTTPClientConfig     `yaml:",inline"`

@FUSAKLA
Copy link
Member Author

FUSAKLA commented Jan 13, 2020

Great! 🎉 Thanks @simonpasquier 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants