From e760ffab0a8e42d597e04b786e0cf88bb58f5f5a Mon Sep 17 00:00:00 2001 From: "Grot (@grafanabot)" <43478413+grafanabot@users.noreply.github.com> Date: Fri, 10 Jun 2022 02:15:46 -0600 Subject: [PATCH] Alerting: Invalid setting of enabled for unified alerting should return error (#49876) (#50569) (cherry picked from commit bda47df4adc00aa22808e4ab325b87cb696af21d) Co-authored-by: George Robinson --- pkg/setting/setting_test.go | 118 ++++++++++++++++++++++-- pkg/setting/setting_unified_alerting.go | 20 ++-- 2 files changed, 124 insertions(+), 14 deletions(-) diff --git a/pkg/setting/setting_test.go b/pkg/setting/setting_test.go index 826e6ad4e3b..92f2a42a54d 100644 --- a/pkg/setting/setting_test.go +++ b/pkg/setting/setting_test.go @@ -489,7 +489,7 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when legacy alerting is enabled and unified is invalid (or not defined) [OSS]", + desc: "when legacy alerting is enabled and unified is not defined [OSS]", legacyAlertingEnabled: "true", unifiedAlertingEnabled: "", isEnterprise: false, @@ -507,7 +507,24 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when legacy alerting is enabled and unified is invalid (or not defined) [Enterprise]", + desc: "when legacy alerting is enabled and unified is invalid [OSS]", + legacyAlertingEnabled: "true", + unifiedAlertingEnabled: "invalid", + isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, + { + desc: "when legacy alerting is enabled and unified is not defined [Enterprise]", legacyAlertingEnabled: "true", unifiedAlertingEnabled: "", isEnterprise: true, @@ -525,10 +542,27 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when legacy alerting is disabled and unified is invalid (or not defined) [OSS]", - legacyAlertingEnabled: "false", + desc: "when legacy alerting is enabled and unified is invalid [Enterprise]", + legacyAlertingEnabled: "true", unifiedAlertingEnabled: "invalid", isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, + { + desc: "when legacy alerting is disabled and unified is not defined [OSS]", + legacyAlertingEnabled: "false", + unifiedAlertingEnabled: "", + isEnterprise: false, verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { err := readAlertingSettings(f) require.NoError(t, err) @@ -543,9 +577,26 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when legacy alerting is disabled and unified is invalid (or not defined) [Enterprise]", + desc: "when legacy alerting is disabled and unified is invalid [OSS]", legacyAlertingEnabled: "false", unifiedAlertingEnabled: "invalid", + isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, + { + desc: "when legacy alerting is disabled and unified is not defined [Enterprise]", + legacyAlertingEnabled: "false", + unifiedAlertingEnabled: "", isEnterprise: true, verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { err := readAlertingSettings(f) @@ -561,10 +612,27 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when both are invalid (or not defined) [OSS]", - legacyAlertingEnabled: "invalid", + desc: "when legacy alerting is disabled and unified is invalid [Enterprise]", + legacyAlertingEnabled: "false", unifiedAlertingEnabled: "invalid", isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, + { + desc: "when both are not defined [OSS]", + legacyAlertingEnabled: "", + unifiedAlertingEnabled: "", + isEnterprise: false, verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { err := readAlertingSettings(f) require.NoError(t, err) @@ -578,9 +646,26 @@ func TestAlertingEnabled(t *testing.T) { }, }, { - desc: "when both are invalid (or not defined) [Enterprise]", + desc: "when both are not invalid [OSS]", legacyAlertingEnabled: "invalid", unifiedAlertingEnabled: "invalid", + isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, + { + desc: "when both are not defined [Enterprise]", + legacyAlertingEnabled: "", + unifiedAlertingEnabled: "", isEnterprise: true, verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { err := readAlertingSettings(f) @@ -594,6 +679,23 @@ func TestAlertingEnabled(t *testing.T) { assert.Nil(t, AlertingEnabled) }, }, + { + desc: "when both are not invalid [Enterprise]", + legacyAlertingEnabled: "invalid", + unifiedAlertingEnabled: "invalid", + isEnterprise: false, + verifyCfg: func(t *testing.T, cfg Cfg, f *ini.File) { + err := readAlertingSettings(f) + require.NoError(t, err) + err = cfg.readFeatureToggles(f) + require.NoError(t, err) + err = cfg.ReadUnifiedAlertingSettings(f) + assert.EqualError(t, err, "failed to read unified alerting enabled setting: invalid value invalid, should be either true or false") + assert.Nil(t, cfg.UnifiedAlerting.Enabled) + assert.NotNil(t, AlertingEnabled) + assert.Equal(t, false, *AlertingEnabled) + }, + }, { desc: "when both are false", legacyAlertingEnabled: "false", diff --git a/pkg/setting/setting_unified_alerting.go b/pkg/setting/setting_unified_alerting.go index e0bcc3da0e3..5762760c38d 100644 --- a/pkg/setting/setting_unified_alerting.go +++ b/pkg/setting/setting_unified_alerting.go @@ -103,31 +103,39 @@ func (cfg *Cfg) readUnifiedAlertingEnabledSetting(section *ini.Section) (*bool, // At present an invalid value is considered the same as no value. This means that a // spelling mistake in the string "false" could enable unified alerting rather // than disable it. This issue can be found here - unifiedAlerting, err := section.Key("enabled").Bool() - if err != nil { + hasEnabled := section.Key("enabled").Value() != "" + if !hasEnabled { // TODO: Remove in Grafana v9 if cfg.IsFeatureToggleEnabled("ngalert") { cfg.Logger.Warn("ngalert feature flag is deprecated: use unified alerting enabled setting instead") // feature flag overrides the legacy alerting setting legacyAlerting := false AlertingEnabled = &legacyAlerting - unifiedAlerting = true + unifiedAlerting := true return &unifiedAlerting, nil } // if legacy alerting has not been configured then enable unified alerting if AlertingEnabled == nil { - unifiedAlerting = true + unifiedAlerting := true return &unifiedAlerting, nil } // enable unified alerting and disable legacy alerting legacyAlerting := false AlertingEnabled = &legacyAlerting - unifiedAlerting = true + unifiedAlerting := true return &unifiedAlerting, nil } + unifiedAlerting, err := section.Key("enabled").Bool() + if err != nil { + // the value for unified alerting is invalid so disable all alerting + legacyAlerting := false + AlertingEnabled = &legacyAlerting + return nil, fmt.Errorf("invalid value %s, should be either true or false", section.Key("enabled")) + } + // If both legacy and unified alerting are enabled then return an error if AlertingEnabled != nil && *AlertingEnabled && unifiedAlerting { return nil, errors.New("legacy and unified alerting cannot both be enabled at the same time, please disable one of them and restart Grafana") @@ -149,7 +157,7 @@ func (cfg *Cfg) ReadUnifiedAlertingSettings(iniFile *ini.File) error { ua := iniFile.Section("unified_alerting") uaCfg.Enabled, err = cfg.readUnifiedAlertingEnabledSetting(ua) if err != nil { - return err + return fmt.Errorf("failed to read unified alerting enabled setting: %w", err) } uaCfg.DisabledOrgs = make(map[int64]struct{})