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

Ruler: Add support for arbitrary extra URL parameters to Alertmanager client's OAuth config #10030

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
8 changes: 8 additions & 0 deletions cmd/mimir/config-descriptor.json
Original file line number Diff line number Diff line change
Expand Up @@ -12393,6 +12393,14 @@
"fieldDefaultValue": "",
"fieldFlag": "ruler.alertmanager-client.oauth.scopes",
"fieldType": "string"
},
{
"kind": "block",
"name": "endpoint_params",
"required": false,
"desc": "",
"fieldValue": null,
"fieldDefaultValue": null
}
],
"fieldValue": null,
Expand Down
2 changes: 2 additions & 0 deletions cmd/mimir/help-all.txt.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -2825,6 +2825,8 @@ Usage of ./cmd/mimir/mimir:
OAuth2 client ID. Enables the use of OAuth2 for authenticating with Alertmanager.
-ruler.alertmanager-client.oauth.client_secret string
OAuth2 client secret.
-ruler.alertmanager-client.oauth.endpoint-params value
Optional additional URL parameters to send to the token URL. (default {})
-ruler.alertmanager-client.oauth.scopes comma-separated-list-of-strings
Optional scopes to include with the token request.
-ruler.alertmanager-client.oauth.token_url string
Expand Down
2 changes: 2 additions & 0 deletions cmd/mimir/help.txt.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,8 @@ Usage of ./cmd/mimir/mimir:
OAuth2 client ID. Enables the use of OAuth2 for authenticating with Alertmanager.
-ruler.alertmanager-client.oauth.client_secret string
OAuth2 client secret.
-ruler.alertmanager-client.oauth.endpoint-params value
Optional additional URL parameters to send to the token URL. (default {})
-ruler.alertmanager-client.oauth.scopes comma-separated-list-of-strings
Optional scopes to include with the token request.
-ruler.alertmanager-client.oauth.token_url string
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1969,6 +1969,8 @@ alertmanager_client:
# CLI flag: -ruler.alertmanager-client.oauth.scopes
[scopes: <string> | default = ""]

endpoint_params:

# (advanced) Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route
# requests through. Applies to all requests, including auxiliary traffic, such
# as OAuth token requests.
Expand Down
26 changes: 22 additions & 4 deletions pkg/ruler/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@ import (
"github.com/prometheus/prometheus/notifier"

"github.com/grafana/mimir/pkg/util"
"github.com/grafana/mimir/pkg/util/validation"
)

var (
errRulerNotifierStopped = cancellation.NewErrorf("rulerNotifier stopped")
errRulerSimultaneousBasicAuthAndOAuth = errors.New("cannot use both Basic Auth and OAuth2 simultaneously")
errEmptyEndpointParamValue = errors.New("endpoint params value cannot be empty")
)

type NotifierConfig struct {
Expand All @@ -49,18 +51,30 @@ func (cfg *NotifierConfig) RegisterFlags(f *flag.FlagSet) {
f.StringVar(&cfg.ProxyURL, "ruler.alertmanager-client.proxy-url", "", "Optional HTTP, HTTPS via CONNECT, or SOCKS5 proxy URL to route requests through. Applies to all requests, including auxiliary traffic, such as OAuth token requests.")
}

func validateOAuth2EndpointParam(k string, v string) error {
if v == "" {
return errEmptyEndpointParamValue
}
return nil
}

type OAuth2Config struct {
ClientID string `yaml:"client_id"`
ClientSecret flagext.Secret `yaml:"client_secret"`
TokenURL string `yaml:"token_url"`
Scopes flagext.StringSliceCSV `yaml:"scopes,omitempty"`
ClientID string `yaml:"client_id"`
ClientSecret flagext.Secret `yaml:"client_secret"`
TokenURL string `yaml:"token_url"`
Scopes flagext.StringSliceCSV `yaml:"scopes,omitempty"`
EndpointParams validation.LimitsMap[string] `yaml:"endpoint_params" category:"advanced"`
}

func (cfg *OAuth2Config) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
f.StringVar(&cfg.ClientID, prefix+"client_id", "", "OAuth2 client ID. Enables the use of OAuth2 for authenticating with Alertmanager.")
f.Var(&cfg.ClientSecret, prefix+"client_secret", "OAuth2 client secret.")
f.StringVar(&cfg.TokenURL, prefix+"token_url", "", "Endpoint used to fetch access token.")
f.Var(&cfg.Scopes, prefix+"scopes", "Optional scopes to include with the token request.")
if !cfg.EndpointParams.IsInitialized() {
cfg.EndpointParams = validation.NewLimitsMap(validateOAuth2EndpointParam)
}
f.Var(&cfg.EndpointParams, prefix+"endpoint-params", "Optional additional URL parameters to send to the token URL.")
}

func (cfg *OAuth2Config) IsEnabled() bool {
Expand Down Expand Up @@ -224,6 +238,10 @@ func amConfigWithSD(rulerConfig *Config, url *url.URL, sdConfig discovery.Config
Scopes: rulerConfig.Notifier.OAuth2.Scopes,
}

if rulerConfig.Notifier.OAuth2.EndpointParams.IsInitialized() {
amConfig.HTTPClientConfig.OAuth2.EndpointParams = rulerConfig.Notifier.OAuth2.EndpointParams.Read()
}

if rulerConfig.Notifier.ProxyURL != "" {
url, err := url.Parse(rulerConfig.Notifier.ProxyURL)
if err != nil {
Expand Down
92 changes: 92 additions & 0 deletions pkg/ruler/notifier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
package ruler

import (
"bytes"
"errors"
"flag"
"maps"
"net/url"
"testing"
"time"
Expand All @@ -17,9 +20,11 @@ import (
"github.com/prometheus/common/model"
"github.com/prometheus/prometheus/config"
"github.com/prometheus/prometheus/discovery"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/grafana/mimir/pkg/util"
"github.com/grafana/mimir/pkg/util/validation"
)

func TestBuildNotifierConfig(t *testing.T) {
Expand Down Expand Up @@ -373,6 +378,51 @@ func TestBuildNotifierConfig(t *testing.T) {
},
},
},
{
name: "with OAuth2 and optional endpoint params",
cfg: &Config{
AlertmanagerURL: "dnssrv+https://_http._tcp.alertmanager-0.default.svc.cluster.local/alertmanager",
Notifier: NotifierConfig{
OAuth2: OAuth2Config{
ClientID: "oauth2-client-id",
ClientSecret: flagext.SecretWithValue("test"),
TokenURL: "https://oauth2-token-endpoint.local/token",
EndpointParams: validation.NewLimitsMapWithData[string](
map[string]string{
"param1": "value1",
"param2": "value2",
},
validateOAuth2EndpointParam,
),
},
},
},
ncfg: &config.Config{
AlertingConfig: config.AlertingConfig{
AlertmanagerConfigs: []*config.AlertmanagerConfig{
{
HTTPClientConfig: config_util.HTTPClientConfig{
OAuth2: &config_util.OAuth2{
ClientID: "oauth2-client-id",
ClientSecret: "test",
TokenURL: "https://oauth2-token-endpoint.local/token",
EndpointParams: map[string]string{"param1": "value1", "param2": "value2"},
},
},
APIVersion: "v2",
Scheme: "https",
PathPrefix: "/alertmanager",
ServiceDiscoveryConfigs: discovery.Configs{
dnsServiceDiscovery{
Host: "_http._tcp.alertmanager-0.default.svc.cluster.local",
QType: dns.SRV,
},
},
},
},
},
},
},
{
name: "with OAuth2 and proxy_url simultaneously, inheriting proxy",
cfg: &Config{
Expand Down Expand Up @@ -498,6 +548,48 @@ func TestBuildNotifierConfig(t *testing.T) {
}
}

func TestOAuth2Config_ValidateEndpointParams(t *testing.T) {
for name, tc := range map[string]struct {
args []string
expected validation.LimitsMap[string]
error string
}{
"basic test": {
args: []string{"-map-flag", "{\"param1\": \"value1\" }"},
expected: validation.NewLimitsMapWithData(map[string]string{
"param1": "value1",
}, validateOAuth2EndpointParam),
},

"empty value": {
args: []string{"-map-flag", "{\"param1\": \"\" }"},
error: "invalid value \"{\\\"param1\\\": \\\"\\\" }\" for flag -map-flag: endpoint params value cannot be empty",
},

"parsing error": {
args: []string{"-map-flag", "{\"hello\": ..."},
error: "invalid value \"{\\\"hello\\\": ...\" for flag -map-flag: invalid character '.' looking for beginning of value",
},
} {
t.Run(name, func(t *testing.T) {
v := validation.NewLimitsMap(validateOAuth2EndpointParam)

fs := flag.NewFlagSet("test", flag.ContinueOnError)
fs.SetOutput(&bytes.Buffer{}) // otherwise errors would go to stderr.
fs.Var(v, "map-flag", "Map flag, you can pass JSON into this")
err := fs.Parse(tc.args)

if tc.error != "" {
require.NotNil(t, err)
assert.Equal(t, tc.error, err.Error())
} else {
assert.NoError(t, err)
assert.True(t, maps.Equal(tc.expected.Read(), v.Read()))
}
})
}
}

func urlMustParse(t *testing.T, raw string) *url.URL {
t.Helper()
u, err := url.Parse(raw)
Expand Down
6 changes: 3 additions & 3 deletions pkg/util/validation/limits.go
Original file line number Diff line number Diff line change
Expand Up @@ -890,7 +890,7 @@ func (o *Overrides) RulerTenantShardSize(userID string) int {
func (o *Overrides) RulerMaxRulesPerRuleGroup(userID, namespace string) int {
u := o.getOverridesForUser(userID)

if namespaceLimit, ok := u.RulerMaxRulesPerRuleGroupByNamespace.data[namespace]; ok {
if namespaceLimit, ok := u.RulerMaxRulesPerRuleGroupByNamespace.Read()[namespace]; ok {
return namespaceLimit
}

Expand All @@ -906,7 +906,7 @@ func (o *Overrides) RulerMaxRulesPerRuleGroup(userID, namespace string) int {
func (o *Overrides) RulerMaxRuleGroupsPerTenant(userID, namespace string) int {
u := o.getOverridesForUser(userID)

if namespaceLimit, ok := u.RulerMaxRuleGroupsPerTenantByNamespace.data[namespace]; ok {
if namespaceLimit, ok := u.RulerMaxRuleGroupsPerTenantByNamespace.Read()[namespace]; ok {
return namespaceLimit
}

Expand Down Expand Up @@ -982,7 +982,7 @@ func (o *Overrides) AlertmanagerReceiversBlockPrivateAddresses(user string) bool
// 4. default limits
func (o *Overrides) getNotificationLimitForUser(user, integration string) float64 {
u := o.getOverridesForUser(user)
if n, ok := u.NotificationRateLimitPerIntegration.data[integration]; ok {
if n, ok := u.NotificationRateLimitPerIntegration.Read()[integration]; ok {
return n
}

Expand Down
17 changes: 13 additions & 4 deletions pkg/util/validation/limits_map.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,19 @@ import (
"gopkg.in/yaml.v3"
)

// LimitsMap is a generic map that can hold either float64 or int as values.
type LimitsMap[T float64 | int] struct {
// LimitsMap is a flag.Value implementation that looks like a generic map, holding float64s, ints, or strings as values.
type LimitsMap[T float64 | int | string] struct {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, the only place where we really use map flags is in Limits, that's why this was called LimitsMap. But, nothing in this file is really specific to limits.

This new use case isn't really a limit, but I felt it gives a better configuration experience to be consistent with existing flags here, rather than invent a second map format that behaves differently.

I've kept the name LimitsMap and package as i feel it's close enough to being descriptive and would muddy the diff to include renames/moves. I also think there's a valid case for moving this to dskit's flagext package in the future anyway, so it didn't make sense to do a big rename yet.

I'm happy to change the name, or explore moving this to dskit, if reviewers would prefer. Or, this can be left to a subsequent PR.

data map[string]T
validator func(k string, v T) error
}

func NewLimitsMap[T float64 | int](validator func(k string, v T) error) LimitsMap[T] {
func NewLimitsMap[T float64 | int | string](validator func(k string, v T) error) LimitsMap[T] {
return NewLimitsMapWithData(make(map[string]T), validator)
}

func NewLimitsMapWithData[T float64 | int | string](data map[string]T, validator func(k string, v T) error) LimitsMap[T] {
return LimitsMap[T]{
data: make(map[string]T),
data: data,
validator: validator,
}
}
Expand Down Expand Up @@ -45,6 +49,10 @@ func (m LimitsMap[T]) Set(s string) error {
return m.updateMap(newMap)
}

func (m LimitsMap[T]) Read() map[string]T {
return m.data
}

// Clone returns a copy of the LimitsMap.
func (m LimitsMap[T]) Clone() LimitsMap[T] {
newMap := make(map[string]T, len(m.data))
Expand Down Expand Up @@ -73,6 +81,7 @@ func (m LimitsMap[T]) updateMap(newMap map[string]T) error {
}
}

clear(m.data)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

drive-by bugfix i happened to notice. added corresponding test.

for k, v := range newMap {
m.data[k] = v
}
Expand Down
Loading
Loading