Skip to content

feat(): Add alertmanagerconfig_matcher block to mimir.alerts.kubernetes component to change the matcher strategy#5072

Merged
clayton-cornell merged 15 commits intografana:mainfrom
timonegk:add-alertmanagerconfigmatcher
Jan 8, 2026
Merged

feat(): Add alertmanagerconfig_matcher block to mimir.alerts.kubernetes component to change the matcher strategy#5072
clayton-cornell merged 15 commits intografana:mainfrom
timonegk:add-alertmanagerconfigmatcher

Conversation

@timonegk
Copy link
Contributor

@timonegk timonegk commented Dec 12, 2025 •
edited by jharvey10
Loading

PR Description

Which issue(s) this PR fixes

Fixes #5038

Notes to the Reviewer

I also had to update one of the existing tests because the second namespace had not been added to the namespace indexer. The tests was therefore testing wrong behavior.

PR Checklist

  • CHANGELOG.md updated
  • Documentation added
  • Tests updated

BEGIN_COMMIT_OVERRIDE
feat(mimir.alerts.kubernetes): Add alertmanagerconfig_matcher block to change the matcher strategy
END_COMMIT_OVERRIDE

@timonegk timonegk requested review from a team and clayton-cornell as code owners December 12, 2025 11:14
@timonegk timonegk mentioned this pull request Dec 12, 2025
Closed
@timonegk timonegk force-pushed the add-alertmanagerconfigmatcher branch from a2afd08 to 7402388 Compare December 12, 2025 11:46
@artemlive
Copy link

artemlive commented Dec 12, 2025 •
edited
Loading

Thanks for implementing this.
How about adding the "alertmanager namespace" synthetic option and passing it to the Alertmanager instance to support the OnNamespaceExceptAlertmanagerNamespace?

alloy/internal/component/mimir/alerts/kubernetes/events.go

Line 180 in 6142108

cfgBuilder = alertmanager.NewConfigBuilder(slog.New(logging.NewSlogGoKitHandler(c.logger)), *version, store, &monitoringv1.Alertmanager{})

So we can simulate the namespace matching, and it would work in the same way if we watched the Alertmanager config?
UPD:
This can be an overhead, though, since we already have access to set the type to None, which will do the same thing basically.

@artemlive
Copy link

artemlive commented Dec 12, 2025 •
edited
Loading

Maybe it makes sense to add the OnNamespaceExceptAlertmanagerNamespace only for full compatibility, because setting the alertmanager_matching_strategy to None will do the same thing as if we set the alertmanager_namespace explicitly to the same value as the AlertmanagerConfig's namespace.

      remote.kubernetes.configmap "default" {
        namespace = "testing"
        name = "alertmgr-global"
      }

      mimir.alerts.kubernetes "default" {
        address = "http://mimir-nginx.mimir-test.svc:80"
        global_config = remote.kubernetes.configmap.default.data["glbl"]
        alertmanager_namespace = "testing"
        alertmanager_matching_strategy = "OnNamespaceExceptAlertmanagerNamespace"
      }

So when we set the:

...
        alertmanager_namespace = "testing"
        alertmanager_matching_strategy = "OnNamespaceExceptAlertmanagerNamespace"
...

If the AlertmanagerConfig manifest is located in the "testing" namespace, it will generate the config without matchers by the namespace for this instance. But it would be the same if we set the alertmanager_matching_strategy to None.
UPD:
But maybe it's worth adding, since this is a good "fuse" if we have multiple configs like:

      remote.kubernetes.configmap "default" {
        namespace = "testing"
        name = "alertmgr-global"
      }

      # this one is the global config
      mimir.alerts.kubernetes "global" {
        address = "http://mimir-nginx.mimir-test.svc:80"
        global_config = remote.kubernetes.configmap.default.data["glbl"]
        alertmanager_namespace = "testing"
        alertmanager_matching_strategy = "OnNamespaceExceptAlertmanagerNamespace"
      }

      # and this one is a "tenant's" config
      mimir.alerts.kubernetes "tenant1" {
        address = "http://mimir-nginx.mimir-test.svc:80"
        global_config = remote.kubernetes.configmap.default.data["glbl"]
        http_headers = {"X-Scope-OrgID" = ["tenant1"]}
        alertmanagerconfig_namespace_selector {
            match_labels = {
               tenant1  = "yes",
               namespace = "tenant1"  # <<<<<<< just to show that it's not in the same ns as the alertmanager_namespace.
            }
        }
        alertmanagerconfig_selector {
            match_labels = {
                tenant1 = "yes",
            }
        }
      }

In this case, we won't accidentally load the tenant's configuration into the global matcher scope. We could probably do the same thing using the alertmanagerconfig_namespace_selector and alertmanagerconfig_selector in the "global" config, but this is just an example of "missconfig" that can be covered by the alertmanager_namespace option.

@timonegk timonegk force-pushed the add-alertmanagerconfigmatcher branch from 08178e8 to 7cb46a3 Compare December 12, 2025 15:28
@timonegk
Copy link
Contributor Author

timonegk commented Dec 12, 2025

I have implemented the strategy, but I have had some problems with adding a test. I will continue on Monday.

@timonegk
Copy link
Contributor Author

timonegk commented Dec 15, 2025

I added the test for the last strategy. I also had to fix an error in the existing tests where a second namespace was not correctly discovered.
@ptodev I would say this PR is now ready to review.

ptodev
ptodev reviewed Dec 15, 2025
View reviewed changes
@ptodev ptodev self-assigned this Dec 15, 2025
@timonegk timonegk force-pushed the add-alertmanagerconfigmatcher branch 2 times, most recently from 90b0eee to f43e87d Compare December 15, 2025 14:35
ptodev
ptodev reviewed Dec 15, 2025
View reviewed changes
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thank you! Looks good from a code point of view. The docs need just a bit more polishing.

@timonegk timonegk force-pushed the add-alertmanagerconfigmatcher branch from 2298890 to 8ed28a8 Compare December 16, 2025 10:07
@timonegk
Copy link
Contributor Author

timonegk commented Dec 16, 2025

@ptodev I updated the docs according to your suggestions. I also added a section to the example, what do you think?

clayton-cornell
clayton-cornell reviewed Dec 16, 2025
View reviewed changes
@clayton-cornell clayton-cornell added the type/docs Docs Squad label across all Grafana Labs repos label Dec 16, 2025
ptodev
ptodev reviewed Dec 18, 2025
View reviewed changes
ptodev
ptodev approved these changes Dec 18, 2025
View reviewed changes
Copy link
Contributor

@ptodev ptodev left a comment

Choose a reason for hiding this comment

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

Thank you! We can merge it once @clayton-cornell approves on the docs side

docs/sources/reference/components/mimir/mimir.alerts.kubernetes.md
| Name | Type | Description | Default | Required |
| ------------------------ | -------- | -------------------------------------------------------------------------------------------------------------------- | --------------- | --------------------------------------------------------------------- |
| `strategy` | `string` | Strategy for adding matchers to AlertmanagerConfig CRDs. | `"OnNamespace"` | no |
| `alertmanager_namespace` | `string` | Namespace to use when `alertmanagerconfig_matcher_strategy` is set to `"OnNamespaceExceptForAlertmanagerNamespace"`. | | only when `strategy` is `"OnNamespaceExceptForAlertmanagerNamespace"` |
Copy link
Contributor

@ptodev ptodev Dec 18, 2025

Choose a reason for hiding this comment

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

@clayton-cornell is it ok if the "Required" column is populated like this? Is it too verbose? I couldn't run make docs locally due to a docker error so I can't visualise it.

Copy link
Contributor

@clayton-cornell clayton-cornell Dec 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I wondered about that too. Ultimately we decide for ourselves what we want in each column and what limits we want to place... the site generator doesn't care.

The doc for otelcol.receiver.awss3 has similar conditional elements

Arguments table:

  • start_time - shows "Required if fetching by time."
  • end_time - shows "Required if fetching by time."

Blocks table:

  • sqs block - shows "Required if fetching by SQS notification."

Since strategy can have 3 different values, and alertmanager_namespace is only required for one of the three strategy values... I think this might be the least ambiguous way of presenting this conditional requirement. It does make the table column wider, but... it doesn't leave the discovery of the requirement to chance.

I'd say... leave it for this PR and we can change it if needed in a future PR.

@ptodev ptodev changed the title feat(mimir.alerts.kubernetes): add support for matcher strategy feat(): add support for matcher strategy Dec 18, 2025
@ptodev ptodev changed the title feat(): add support for matcher strategy feat(): add alertmanagerconfig_matcher block to mimir.alerts.kubernetes component to change the matcher strategy Dec 18, 2025
timonegk and others added 9 commits January 7, 2026 10:02
@timonegk @ptodev
Update docs/sources/reference/components/mimir/mimir.alerts.kubernete…
638cf94 
…s.md

Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
@timonegk @ptodev
Update docs/sources/reference/components/mimir/mimir.alerts.kubernete…
b4c321d 
…s.md

Co-authored-by: Paulin Todev <paulin.todev@gmail.com>
@timonegk
docs: update alertmanagerconfig_matcher block
3c4460d
@timonegk
docs: add example for alertmanagerconfig_matcher block
358d009
@timonegk @clayton-cornell
Update docs/sources/reference/components/mimir/mimir.alerts.kubernete…
880c0eb 
…s.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@timonegk @clayton-cornell
Update docs/sources/reference/components/mimir/mimir.alerts.kubernete…
8f39ffd 
…s.md

Co-authored-by: Clayton Cornell <131809008+clayton-cornell@users.noreply.github.com>
@timonegk
docs: updates from review
5e92613
@timonegk
docs: revert changes to table
e99fe98
@ptodev @timonegk
Update internal/component/mimir/alerts/kubernetes/alerts.go
9515633
@timonegk timonegk force-pushed the add-alertmanagerconfigmatcher branch from 66bdf4c to 9515633 Compare January 7, 2026 09:03
@timonegk
Copy link
Contributor Author

timonegk commented Jan 7, 2026 •
edited
Loading

@ptodev I rebased on the main branch. Can we get this merged?
(also Happy New Year 🎉)

@clayton-cornell clayton-cornell changed the title feat(): add alertmanagerconfig_matcher block to mimir.alerts.kubernetes component to change the matcher strategy feat(): Add alertmanagerconfig_matcher block to mimir.alerts.kubernetes component to change the matcher strategy Jan 8, 2026
@clayton-cornell clayton-cornell enabled auto-merge (squash) January 8, 2026 05:28
@clayton-cornell clayton-cornell merged commit f2b9671 into grafana:main Jan 8, 2026
42 of 43 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 23, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ptodev ptodev ptodev approved these changes

@clayton-cornell clayton-cornell clayton-cornell approved these changes

Assignees

@ptodev ptodev

Labels

frozen-due-to-age type/docs Docs Squad label across all Grafana Labs repos

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Alertmanager's alertmanagerConfigMatcherStrategy issue

4 participants

@timonegk @artemlive @ptodev @clayton-cornell