From b81ae9ad3a0abd4d9696163524cb9c4a3c0a039d Mon Sep 17 00:00:00 2001 From: f41gh7 Date: Fri, 23 Aug 2024 10:39:31 +0200 Subject: [PATCH] alertmanagerconfig: fixes email tls_config build Previously config builder had incorrect assumption, that tlsConfig for email notifications must be build only if requireTLS is set and true. But alermanager has requireTLS as true by default and it shouldn't be set to true at configuration. Also this param could be managed via global config section. This commit removes unneeded if requireTLS is true check before building tls config for email notifications. Related issue https://github.com/VictoriaMetrics/operator/issues/1080 Signed-off-by: f41gh7 --- docs/CHANGELOG.md | 3 +- .../operator/factory/alertmanager/config.go | 5 +- .../factory/alertmanager/config_test.go | 49 +++++++++++++++++-- 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index e05659dc..854512c3 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -11,6 +11,7 @@ aliases: - /operator/changelog/index.html --- +- [vmalertmanagerconfig](https://docs.victoriametrics.com/operator/resources/vmalertmanagerconfig): properly construct `tls_config` for `emails` notifications. See this [issue](https://github.com/VictoriaMetrics/operator/issues/1080) for details. - fixed Prometheus scrape config metricsPath conversion. See [this issue](https://github.com/VictoriaMetrics/operator/issues/1073) - [config-reloader](https://docs.victoriametrics.com/operator/): Added `reload` prefix to all config-reloader `tls*` flags to avoid collision with flags from external package. See [this issue](https://github.com/VictoriaMetrics/operator/issues/1072) @@ -1148,7 +1149,7 @@ aliases: - Major API update for `VMServiceScrape`, `VMPodScrape`, `VMProbe`, `VMStaticScrape` and `VMNodeScrape`: - adds missing config params (sampleLimit and etc) - Adds new config options `vm_scrape_params` -- Adds proxyAuth, that allows to authenticate [proxy requests](https://docs.victoriametrics.com/vmagent#scraping-targets-via-a-proxy +- Adds proxyAuth, that allows to authenticate [proxy requests]( - Adds OAuth2 support. - Adds `apiextensions.k8s.io/v1` `CRD` generation, `v1beta1` is now legacy - Adds new `CRD` `VMAlertmanagerConfig`, it supports only v0.22 `alertmanager` version or above diff --git a/internal/controller/operator/factory/alertmanager/config.go b/internal/controller/operator/factory/alertmanager/config.go index a673998e..a2b090d4 100644 --- a/internal/controller/operator/factory/alertmanager/config.go +++ b/internal/controller/operator/factory/alertmanager/config.go @@ -1112,8 +1112,9 @@ func (cb *configBuilder) buildEmail(email vmv1beta1.EmailConfig) error { return fmt.Errorf("required email from is not set at local and global alertmanager config") } - // skip tls_config if require_tls is false - if email.RequireTLS != nil && *email.RequireTLS && email.TLSConfig != nil { + // add tls config in any case + // require_tls is true by default and it could be managed via global configuration + if email.TLSConfig != nil { s, err := cb.BuildTLSConfig(email.TLSConfig, tlsAssetsDir) if err != nil { return err diff --git a/internal/controller/operator/factory/alertmanager/config_test.go b/internal/controller/operator/factory/alertmanager/config_test.go index cf7f4cea..cc2bb9e3 100644 --- a/internal/controller/operator/factory/alertmanager/config_test.go +++ b/internal/controller/operator/factory/alertmanager/config_test.go @@ -35,7 +35,7 @@ func TestBuildConfig(t *testing.T) { wantErr bool }{ { - name: "simple ok", + name: "email section", args: args{ ctx: context.Background(), baseCfg: []byte(`global: @@ -56,19 +56,42 @@ func TestBuildConfig(t *testing.T) { { SendResolved: ptr.To(true), From: "some-sender", - To: "some-dst", + To: "some-dst-1", + Text: "some-text", + Smarthost: "some:443", + TLSConfig: &vmv1beta1.TLSConfig{ + CertFile: "some_cert_path", + }, + }, + { + SendResolved: ptr.To(true), + From: "some-sender", + To: "some-dst-2", + Text: "some-text", + Smarthost: "some:443", + RequireTLS: ptr.To(false), + TLSConfig: &vmv1beta1.TLSConfig{ + CertFile: "some_cert_path", + }, + }, + { + SendResolved: ptr.To(true), + From: "some-sender", + To: "some-dst-3", Text: "some-text", - RequireTLS: ptr.To(true), Smarthost: "some:443", + RequireTLS: ptr.To(true), TLSConfig: &vmv1beta1.TLSConfig{ CertFile: "some_cert_path", }, }, + { SendResolved: ptr.To(true), From: "other-sender", To: "other-dst", Text: "other-text", + RequireTLS: ptr.To(false), }, }, }, @@ -96,15 +119,31 @@ receivers: - name: blackhole - name: default-base-email email_configs: + - tls_config: + cert_file: some_cert_path + from: some-sender + text: some-text + to: some-dst-1 + smarthost: some:443 + send_resolved: true + - require_tls: false + tls_config: + cert_file: some_cert_path + from: some-sender + text: some-text + to: some-dst-2 + smarthost: some:443 + send_resolved: true - require_tls: true tls_config: cert_file: some_cert_path from: some-sender text: some-text - to: some-dst + to: some-dst-3 smarthost: some:443 send_resolved: true - - from: other-sender + - require_tls: false + from: other-sender text: other-text to: other-dst send_resolved: true