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

Conversation

alexweav
Copy link
Contributor

@alexweav alexweav commented Nov 26, 2024

What this PR does

This PR allows you to specify arbitrary URL parameters to send alongside OAuth token requests, when the Alertmanager client is configured in OAuth mode.

Some OAuth providers require extra fields here that are not normally part of the spec - for example, audience is a common one. The Go stdlib followed a similar convention for such fields: golang/oauth2#216 where arbitrary extra URL values are allowed.

Which issue(s) this PR fixes or relates to

n/a

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

// 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.

@@ -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.

@alexweav alexweav marked this pull request as ready for review November 26, 2024 22:38
@alexweav alexweav requested review from a team and tacole02 as code owners November 26, 2024 22:38
Copy link
Contributor

@tacole02 tacole02 left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

2 participants