Skip to content

Conversation

@JacobsonMT
Copy link
Contributor

@JacobsonMT JacobsonMT commented Jul 10, 2025

Followup to #338

This PR contains two commits:

  • Move http_config to a common area: This moves http_config into a more central area so it can be easily extended to other notifiers and their tests. http_config is now un-marshalled separate from the notifier-specific configuration.
  • Enable Oauth2 for most http-based notifiers: This does the actual enabling on the common HTTP config options to most other notifiers. A much simpler PR because of the above prep.

Testing of the common HTTP config options (currently just oauth2) is fairly comprehensive and centralized in notify/receivers_test.go instead of spread out among each notifier's package tests. The tests use mock servers to ensure the token is actually being correctly retrieved from the oauth2 server and the Authorization header set on outgoing http calls.

Notifiers not yet supported and why:

  • Email: Choice of implementation isn't clear.
  • Slack: Needs to use standard http client and handle Bot Token required fields if oauth2 is configured without a token.
  • SNS: Needs to integrate with standard http client.
  • MQTT: Implementation non-trivial.
  • Alertmanager: Needs to use standard http client.

Notifiers not yet supported:
- Email
- Slack
- SNS
- MQTT
- Alertmanager
@JacobsonMT JacobsonMT requested a review from a team as a code owner July 10, 2025 22:04
@github-project-automation github-project-automation bot moved this to In review in Alerting Jul 10, 2025
@JacobsonMT JacobsonMT force-pushed the jacobsonmt/oauth2-other-http-notifiers branch from 74628a8 to 854f62a Compare July 11, 2025 03:52
@yuri-tceretian yuri-tceretian self-requested a review July 11, 2025 14:34
Copy link
Collaborator

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM
I am not a fan of the making the model generic for all integrations as there might be situations when particular implementation would require different settings and maintaining aggregated model would make it a nightmare. However, I understand the reason why you do it now, and I think it makes sense.

Instead fix test HTTPClientConfig marshalling by making ProxyConfig a pointer
@JacobsonMT JacobsonMT merged commit 8eef376 into main Jul 11, 2025
6 checks passed
@JacobsonMT JacobsonMT deleted the jacobsonmt/oauth2-other-http-notifiers branch July 11, 2025 18:16
@github-project-automation github-project-automation bot moved this from In review to Done in Alerting Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants