Alerting: Invalid setting of enabled for unified alerting should return error (#49876)

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",

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{})