From 58b1d22c294f8a3cb5911f0af92dd576d81d50d6 Mon Sep 17 00:00:00 2001 From: Jayapriya Pai Date: Thu, 28 Nov 2024 17:33:25 +0530 Subject: [PATCH] fix: validate smtp smarthost and smtp from fields SMTP smarthost and SMTP from are required fields which needs to be specified at either global or at receiver level. This change will make sure reconciliation fails if there is empty at both levels. Related-to #6003 Signed-off-by: Jayapriya Pai --- pkg/alertmanager/amcfg.go | 19 +++ pkg/alertmanager/amcfg_test.go | 156 ++++++++++++++++++ .../CR_with_EmailConfig_Receiver_Conf.golden | 15 ++ ...onfig_Receiver_Global_Defaults_Conf.golden | 16 ++ test/e2e/alertmanager_test.go | 6 +- 5 files changed, 211 insertions(+), 1 deletion(-) create mode 100644 pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Conf.golden create mode 100644 pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Global_Defaults_Conf.golden diff --git a/pkg/alertmanager/amcfg.go b/pkg/alertmanager/amcfg.go index 48a6f46bd..959bf6354 100644 --- a/pkg/alertmanager/amcfg.go +++ b/pkg/alertmanager/amcfg.go @@ -258,6 +258,13 @@ func (cb *configBuilder) initializeFromAlertmanagerConfig(ctx context.Context, g } globalAlertmanagerConfig.Global = global + // This is need to check required fields are set either at global or receiver level at later step. + if global != nil { + cb.cfg = &alertmanagerConfig{ + Global: global, + } + } + // Add inhibitRules to globalAlertmanagerConfig.InhibitRules without enforce namespace for _, inhibitRule := range amConfig.Spec.InhibitRules { globalAlertmanagerConfig.InhibitRules = append(globalAlertmanagerConfig.InhibitRules, cb.convertInhibitRule(&inhibitRule)) @@ -1048,6 +1055,18 @@ func (cb *configBuilder) convertEmailConfig(ctx context.Context, in monitoringv1 RequireTLS: in.RequireTLS, } + if in.Smarthost == "" { + if cb.cfg.Global == nil || cb.cfg.Global.SMTPSmarthost.Host == "" { + return nil, fmt.Errorf("SMTP smarthost is a mandatory field, it is neither specified at global config nor at receiver level") + } + } + + if in.From == "" { + if cb.cfg.Global == nil || cb.cfg.Global.SMTPFrom == "" { + return nil, fmt.Errorf("SMTP from is a mandatory field, it is neither specified at global config nor at receiver level") + } + } + if in.Smarthost != "" { out.Smarthost.Host, out.Smarthost.Port, _ = net.SplitHostPort(in.Smarthost) } diff --git a/pkg/alertmanager/amcfg_test.go b/pkg/alertmanager/amcfg_test.go index b3c579ee9..550d04bf1 100644 --- a/pkg/alertmanager/amcfg_test.go +++ b/pkg/alertmanager/amcfg_test.go @@ -1019,6 +1019,7 @@ func TestGenerateConfig(t *testing.T) { matcherStrategy monitoringv1.AlertmanagerConfigMatcherStrategy amConfigs map[string]*monitoringv1alpha1.AlertmanagerConfig golden string + expectedError bool } version24, err := semver.ParseTolerant("v0.24.0") require.NoError(t, err) @@ -2364,6 +2365,157 @@ func TestGenerateConfig(t *testing.T) { }, golden: "CR_with_MSTeams_Receiver_Partial_Conf.golden", }, + { + name: "CR with EmailConfig with Required Fields specified at Receiver level", + amVersion: &version26, + kclient: fake.NewSimpleClientset(), + baseConfig: alertmanagerConfig{ + Route: &route{ + Receiver: "null", + }, + Receivers: []*receiver{{Name: "null"}}, + }, + amConfigs: map[string]*monitoringv1alpha1.AlertmanagerConfig{ + "mynamespace": { + ObjectMeta: metav1.ObjectMeta{ + Name: "myamc", + Namespace: "mynamespace", + }, + Spec: monitoringv1alpha1.AlertmanagerConfigSpec{ + Route: &monitoringv1alpha1.Route{ + Receiver: "test", + }, + Receivers: []monitoringv1alpha1.Receiver{ + { + Name: "test", + EmailConfigs: []monitoringv1alpha1.EmailConfig{ + { + Smarthost: "example.com:25", + From: "admin@example.com", + To: "customers@example.com", + }, + }, + }, + }, + }, + }, + }, + golden: "CR_with_EmailConfig_Receiver_Conf.golden", + }, + { + name: "CR with EmailConfig Missing SmartHost Field", + amVersion: &version26, + kclient: fake.NewSimpleClientset(), + baseConfig: alertmanagerConfig{ + Route: &route{ + Receiver: "null", + }, + Receivers: []*receiver{{Name: "null"}}, + }, + amConfigs: map[string]*monitoringv1alpha1.AlertmanagerConfig{ + "mynamespace": { + ObjectMeta: metav1.ObjectMeta{ + Name: "myamc", + Namespace: "mynamespace", + }, + Spec: monitoringv1alpha1.AlertmanagerConfigSpec{ + Route: &monitoringv1alpha1.Route{ + Receiver: "test", + }, + Receivers: []monitoringv1alpha1.Receiver{ + { + Name: "test", + EmailConfigs: []monitoringv1alpha1.EmailConfig{ + { + From: "admin@example.com", + To: "customers@example.com", + }, + }, + }, + }, + }, + }, + }, + expectedError: true, + }, + { + name: "CR with EmailConfig Missing SMTP From Field", + amVersion: &version26, + kclient: fake.NewSimpleClientset(), + baseConfig: alertmanagerConfig{ + Route: &route{ + Receiver: "null", + }, + Receivers: []*receiver{{Name: "null"}}, + }, + amConfigs: map[string]*monitoringv1alpha1.AlertmanagerConfig{ + "mynamespace": { + ObjectMeta: metav1.ObjectMeta{ + Name: "myamc", + Namespace: "mynamespace", + }, + Spec: monitoringv1alpha1.AlertmanagerConfigSpec{ + Route: &monitoringv1alpha1.Route{ + Receiver: "test", + }, + Receivers: []monitoringv1alpha1.Receiver{ + { + Name: "test", + EmailConfigs: []monitoringv1alpha1.EmailConfig{ + { + From: "admin@example.com", + To: "customers@example.com", + }, + }, + }, + }, + }, + }, + }, + expectedError: true, + }, + { + name: "CR with EmailConfig Missing Required Fields from Receiver level but specified at Global level", + amVersion: &version26, + kclient: fake.NewSimpleClientset(), + baseConfig: alertmanagerConfig{ + Global: &globalConfig{ + SMTPSmarthost: config.HostPort{ + Host: "smtp.example.org", + Port: "587", + }, + SMTPFrom: "admin@globaltest.com", + }, + Route: &route{ + Receiver: "null", + }, + Receivers: []*receiver{{Name: "null"}}, + }, + amConfigs: map[string]*monitoringv1alpha1.AlertmanagerConfig{ + "mynamespace": { + ObjectMeta: metav1.ObjectMeta{ + Name: "myamc", + Namespace: "mynamespace", + }, + Spec: monitoringv1alpha1.AlertmanagerConfigSpec{ + Route: &monitoringv1alpha1.Route{ + Receiver: "test", + }, + Receivers: []monitoringv1alpha1.Receiver{ + { + Name: "test", + EmailConfigs: []monitoringv1alpha1.EmailConfig{ + { + To: "customers@example.com", + }, + }, + }, + }, + }, + }, + }, + golden: "CR_with_EmailConfig_Receiver_Global_Defaults_Conf.golden", + }, } logger := newNopLogger(t) @@ -2380,6 +2532,10 @@ func TestGenerateConfig(t *testing.T) { cb := newConfigBuilder(logger, *tc.amVersion, store, tc.matcherStrategy) cb.cfg = &tc.baseConfig + if tc.expectedError { + require.Error(t, cb.addAlertmanagerConfigs(context.Background(), tc.amConfigs)) + return + } require.NoError(t, cb.addAlertmanagerConfigs(context.Background(), tc.amConfigs)) cfgBytes, err := cb.marshalJSON() diff --git a/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Conf.golden b/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Conf.golden new file mode 100644 index 000000000..0fb8a95ca --- /dev/null +++ b/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Conf.golden @@ -0,0 +1,15 @@ +route: + receiver: "null" + routes: + - receiver: mynamespace/myamc/test + matchers: + - namespace="mynamespace" + continue: true +receivers: +- name: "null" +- name: mynamespace/myamc/test + email_configs: + - to: customers@example.com + from: admin@example.com + smarthost: example.com:25 +templates: [] diff --git a/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Global_Defaults_Conf.golden b/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Global_Defaults_Conf.golden new file mode 100644 index 000000000..b5d58dbfd --- /dev/null +++ b/pkg/alertmanager/testdata/CR_with_EmailConfig_Receiver_Global_Defaults_Conf.golden @@ -0,0 +1,16 @@ +global: + smtp_from: admin@globaltest.com + smtp_smarthost: smtp.example.org:587 +route: + receiver: "null" + routes: + - receiver: mynamespace/myamc/test + matchers: + - namespace="mynamespace" + continue: true +receivers: +- name: "null" +- name: mynamespace/myamc/test + email_configs: + - to: customers@example.com +templates: [] diff --git a/test/e2e/alertmanager_test.go b/test/e2e/alertmanager_test.go index 806164e9d..f60d240bc 100644 --- a/test/e2e/alertmanager_test.go +++ b/test/e2e/alertmanager_test.go @@ -1083,7 +1083,9 @@ func testAlertmanagerConfigCRD(t *testing.T) { SendResolved: func(b bool) *bool { return &b }(true), - To: "test@example.com", + Smarthost: "example.com:25", + From: "admin@example.com", + To: "test@example.com", AuthPassword: &v1.SecretKeySelector{ LocalObjectReference: v1.LocalObjectReference{ Name: testingSecret, @@ -1496,6 +1498,8 @@ receivers: email_configs: - send_resolved: true to: test@example.com + from: admin@example.com + smarthost: example.com:25 auth_password: 1234abc auth_secret: 1234abc headers: