feat: support Service discovery for Alertmanager - #2060
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for discovering Alertmanager targets via Kubernetes Service DNS names in addition to the default Endpoints-based discovery. It adds a new discoveryType field to the AlertmanagerEndpoints specification, updates the validation logic to require a numeric port when using Service discovery, and updates the configuration generation to construct the appropriate Service DNS name. Unit and end-to-end tests have been added to verify this new functionality. There are no review comments, and I have no additional feedback to provide.
|
Thanks! I think this change is reasonable. Do you self-deploy operator or do you use managed GMP? |
|
Thanks! I don't currently run either setup in production. I picked this up from #595 and validated the change against the open-source operator's configuration-generation path and repository tests, but I haven't tested it with managed GMP. The use case I had in mind is self-deployed operators that need to target an ExternalName Service without maintaining an Endpoints object. If managed GMP has additional API or compatibility constraints, I'm happy to adjust the scope. |
|
Cool, that's ok - let me know once it's ready for review! |
|
Thanks! It’s ready for review now. The implementation and relevant automated repository tests are complete and passing. |
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
One top level question first.
Just to understand the expectations again, is this something you need or just helping us with a tech debt?
| require.Len(t, staticConfig[0].Targets, 1) | ||
| require.Equal(t, | ||
| prommodel.LabelValue("external-alertmanager.monitoring.svc:9093"), | ||
| staticConfig[0].Targets[0][prommodel.AddressLabel], |
There was a problem hiding this comment.
It looks like we added a test for Service discovery here, but it might be worth adding a regression test to ensure that the default Endpoints SD behaviour remains backwards compatible too.
| }, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Consider adding a test case to verify that an unknown DiscoveryType safely falls back and returns the expected unknown type validation error.
| func validateAlertManagerEndpoint(alertManagerEndpoint *AlertmanagerEndpoints) error { | ||
| switch alertManagerEndpoint.DiscoveryType { | ||
| case "", AlertmanagerDiscoveryTypeEndpoints: | ||
| case AlertmanagerDiscoveryTypeService: |
There was a problem hiding this comment.
Since Service discovery bypasses standard endpoints validation, an empty Name or Namespace could yield a broken svcDNSName (e.g. ..svc:9093). Consider adding an explicit sanity rejection for empty strings here.
| cfg.ServiceDiscoveryConfigs = discovery.Configs{ | ||
| discovery.StaticConfig{ | ||
| &targetgroup.Group{ | ||
| Targets: []prommodel.LabelSet{{prommodel.AddressLabel: prommodel.LabelValue(svcDNSName)}}, |
There was a problem hiding this comment.
Technically this is a DNS SD not a "service" discovery through Kubernetes API. Why not using Kube API for this (other than more code?)
There was a problem hiding this comment.
Also if we need DNS we could use DNS SD too
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Sorry I didn't meant to approve yet, just comment
|
Thanks for checking. This is a community contribution addressing #595 rather than something I personally require in production. The issue describes a real user need for routing managed rule evaluation to an external Alertmanager through an ExternalName Service. You are right that the current implementation constructs a static DNS target rather than using Kubernetes Service discovery. Since the rule evaluator already has Kubernetes API access and Service permissions, |
2 participants
What this change does
Servicediscovery mode toAlertmanagerEndpointswhile preserving existingEndpointsdiscovery as the default.<name>.<namespace>.svc:<port>), so portlessExternalNameServices work without an Endpoints object.Why
The current Kubernetes discovery path requires an Endpoints object. Services without Endpoints, including the
ExternalNamesetup described in #595, therefore cannot be used as Alertmanager targets without manually maintaining an Endpoints object.Fixes #595.
Validation
go test ./pkg/operator/... -count=1in Go 1.25./hack/presubmit.sh testin the repository's hermetic Go 1.26.5 image./hack/presubmit.sh all diffin the repository's hermetic imageAssistance disclosure
This contribution was prepared with Codex assistance and the resulting code, generated artifacts, and tests were reviewed and validated locally before publication.