Skip to content

Commit

Permalink
alertmanagerconfig: fixes email tls_config build
Browse files Browse the repository at this point in the history
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 #1080

Signed-off-by: f41gh7 <[email protected]>
  • Loading branch information
f41gh7 committed Aug 23, 2024
1 parent 5fd7d6e commit b81ae9a
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 8 deletions.
3 changes: 2 additions & 1 deletion docs/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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` <https://github.com/VictoriaMetrics/operator/issues/303>
- 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](<https://docs.victoriametrics.com/vmagent#scraping-targets-via-a-proxy>
- Adds OAuth2 support.
- Adds `apiextensions.k8s.io/v1` `CRD` generation, `v1beta1` is now legacy <https://github.com/VictoriaMetrics/operator/issues/291>
- Adds new `CRD` `VMAlertmanagerConfig`, it supports only v0.22 `alertmanager` version or above <https://github.com/VictoriaMetrics/operator/issues/188>
Expand Down
5 changes: 3 additions & 2 deletions internal/controller/operator/factory/alertmanager/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
49 changes: 44 additions & 5 deletions internal/controller/operator/factory/alertmanager/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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),
},
},
},
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit b81ae9a

Please sign in to comment.