Skip to content

Commit

Permalink
fix: validate child routes (#6931)
Browse files Browse the repository at this point in the history 
This change rejects AlertmanagerConfig objects with invalid child routes
definition to cause problems in the configuration generation.

Closes #6277

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
  • Loading branch information
@simonpasquier
simonpasquier committed Sep 18, 2024
1 parent 6105be5 commit 7fc97ea
Show file tree
Hide file tree
Showing 9 changed files with 222 additions and 19 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

## 0.76.1 / 2024-09-03

* [BUGFIX] fix panic when processing an invalid `AlertmanagerConfig` object used for global configuration. #6931
* [BUGFIX] fix bug with Kubernetes service discovery `Selector.Role` field. #6896

## 0.76.0 / 2024-08-08
Expand Down
4 changes: 4 additions & 0 deletions pkg/alertmanager/amcfg.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,10 @@ func (cb *configBuilder) initializeFromAlertmanagerConfig(ctx context.Context, g
Name: amConfig.Name,
}

if err := checkAlertmanagerConfigResource(ctx, amConfig, cb.amVersion, cb.store); err != nil {
return err
}

global, err := cb.convertGlobalConfig(ctx, globalConfig, crKey)
if err != nil {
return err
Expand Down
58 changes: 58 additions & 0 deletions pkg/alertmanager/amcfg_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "null",
},
{
Name: "myreceiver",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Expand Down Expand Up @@ -190,6 +193,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "mynamespace/global-config/null",
},
{
Name: "mynamespace/global-config/myreceiver",
},
},
Route: &route{
Receiver: "mynamespace/global-config/null",
Expand Down Expand Up @@ -225,6 +231,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "null",
},
{
Name: "myreceiver",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Expand All @@ -247,6 +256,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "mynamespace/global-config/null",
},
{
Name: "mynamespace/global-config/myreceiver",
},
},
Route: &route{
Receiver: "mynamespace/global-config/null",
Expand Down Expand Up @@ -354,6 +366,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "null",
},
{
Name: "myreceiver",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Expand All @@ -376,6 +391,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "mynamespace/global-config/null",
},
{
Name: "mynamespace/global-config/myreceiver",
},
},
Route: &route{
Receiver: "mynamespace/global-config/null",
Expand Down Expand Up @@ -483,6 +501,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "null",
},
{
Name: "myreceiver",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Expand All @@ -505,6 +526,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "mynamespace/global-config/null",
},
{
Name: "mynamespace/global-config/myreceiver",
},
},
Route: &route{
Receiver: "mynamespace/global-config/null",
Expand Down Expand Up @@ -571,6 +595,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "null",
},
{
Name: "myreceiver",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Expand All @@ -593,6 +620,9 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
{
Name: "mynamespace/global-config/null",
},
{
Name: "mynamespace/global-config/myreceiver",
},
},
Route: &route{
Receiver: "mynamespace/global-config/null",
Expand Down Expand Up @@ -864,6 +894,34 @@ func TestInitializeFromAlertmanagerConfig(t *testing.T) {
},
wantErr: false,
},
{
name: "invalid alertmanagerConfig with invalid child routes",
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "global-config",
Namespace: "mynamespace",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Receivers: []monitoringv1alpha1.Receiver{
{
Name: "null",
},
},
Route: &monitoringv1alpha1.Route{
Receiver: "null",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"severity":"!=critical$"}]}`),
},
},
},
},
},
matcherStrategy: monitoringingv1.AlertmanagerConfigMatcherStrategy{
Type: "OnNamespace",
},
wantErr: true,
},
}
for _, tt := range tests {
if tt.amVersion == nil {
Expand Down
4 changes: 4 additions & 0 deletions pkg/alertmanager/operator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1021,10 +1021,14 @@ func (c *Operator) selectAlertmanagerConfigs(ctx context.Context, am *monitoring
// checkAlertmanagerConfigResource verifies that an AlertmanagerConfig object is valid
// for the given Alertmanager version and has no missing references to other objects.
func checkAlertmanagerConfigResource(ctx context.Context, amc *monitoringv1alpha1.AlertmanagerConfig, amVersion semver.Version, store *assets.StoreBuilder) error {
// Perform semantic validation irrespective of the Alertmanager version.
if err := validationv1alpha1.ValidateAlertmanagerConfig(amc); err != nil {
return err
}

// Perform more specific validations which depend on the Alertmanager
// version. It also retrieves data from referenced secrets and configmaps
// (and fails in case of missing/invalid references).
if err := checkReceivers(ctx, amc, store, amVersion); err != nil {
return err
}
Expand Down
122 changes: 119 additions & 3 deletions pkg/alertmanager/operator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,9 @@ func TestCreateStatefulSetInputHash(t *testing.T) {

// Test to exercise the function checkAlertmanagerConfigResource
// and validate that semantic validation is in place for all the fields in the
// AlertmanagerConfig CR. The validation is preformed by the operator
// after selecting AlertmanagerConfig resources but before passing them to
// addAlertmanagerConfigs.
// AlertmanagerConfig CR. The validation is performed by the operator
// after selecting AlertmanagerConfig resources and before generating the
// Alertmanager configuration.
func TestCheckAlertmanagerConfig(t *testing.T) {
version, err := semver.ParseTolerant(operator.DefaultAlertmanagerVersion)
require.NoError(t, err)
Expand Down Expand Up @@ -958,15 +958,131 @@ func TestCheckAlertmanagerConfig(t *testing.T) {
},
ok: true,
},
{
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "subroute-with-unknow-field",
Namespace: "ns1",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Route: &monitoringv1alpha1.Route{
Receiver: "recv1",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"severity":"!=critical$"}]}`),
},
},
},
Receivers: []monitoringv1alpha1.Receiver{{
Name: "recv1",
}, {
Name: "recv2",
}},
},
},
},
{
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "subroute-with-invalid-matcher",
Namespace: "ns1",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Route: &monitoringv1alpha1.Route{
Receiver: "recv1",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"name": "severity", "value": "critical", "matchType": "!!"}]}`),
},
},
},
Receivers: []monitoringv1alpha1.Receiver{{
Name: "recv1",
}, {
Name: "recv2",
}},
},
},
},
{
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "subroute-with-empty-matcher-name",
Namespace: "ns1",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Route: &monitoringv1alpha1.Route{
Receiver: "recv1",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"name": "", "value": "critical", "matchType": "!="}]}`),
},
},
},
Receivers: []monitoringv1alpha1.Receiver{{
Name: "recv1",
}, {
Name: "recv2",
}},
},
},
},
{
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "subroute-with-missing-receiver",
Namespace: "ns1",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Route: &monitoringv1alpha1.Route{
Receiver: "recv1",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"name": "severity", "value": "critical", "matchType": "!="}]}`),
},
},
},
Receivers: []monitoringv1alpha1.Receiver{{
Name: "recv1",
}},
},
},
},
{
amConfig: &monitoringv1alpha1.AlertmanagerConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "valid-subroute-definition",
Namespace: "ns1",
},
Spec: monitoringv1alpha1.AlertmanagerConfigSpec{
Route: &monitoringv1alpha1.Route{
Receiver: "recv1",
Routes: []apiextensionsv1.JSON{
{
Raw: []byte(`{"receiver": "recv2", "matchers": [{"name": "severity", "value": "critical", "matchType": "!="}]}`),
},
},
},
Receivers: []monitoringv1alpha1.Receiver{{
Name: "recv1",
}, {
Name: "recv2",
}},
},
},
ok: true,
},
} {
t.Run(tc.amConfig.Name, func(t *testing.T) {
store := assets.NewStoreBuilder(c.CoreV1(), c.CoreV1())

err := checkAlertmanagerConfigResource(context.Background(), tc.amConfig, version, store)
if tc.ok {
require.NoError(t, err)
return
}

t.Logf("err: %s", err)
require.Error(t, err)
})
}
Expand Down
21 changes: 14 additions & 7 deletions pkg/alertmanager/validation/v1alpha1/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func ValidateAlertmanagerConfig(amc *monitoringv1alpha1.AlertmanagerConfig) erro
return err
}

return validateAlertManagerRoutes(amc.Spec.Route, receivers, muteTimeIntervals, true)
return validateRoute(amc.Spec.Route, receivers, muteTimeIntervals, true)
}

func validateReceivers(receivers []monitoringv1alpha1.Receiver) (map[string]struct{}, error) {
Expand Down Expand Up @@ -345,10 +345,11 @@ func validateMSTeamsConfigs(configs []monitoringv1alpha1.MSTeamsConfig) error {
return nil
}

// validateAlertManagerRoutes verifies that the given route and all its children are semantically valid.
// because of the self-referential issues mentioned in https://github.com/kubernetes/kubernetes/issues/62872
// it is not currently possible to apply OpenAPI validation to a v1alpha1.Route.
func validateAlertManagerRoutes(r *monitoringv1alpha1.Route, receivers, muteTimeIntervals map[string]struct{}, topLevelRoute bool) error {
// validateRoute verifies that the given route and all its children are
// semantically valid. because of the self-referential issues mentioned in
// https://github.com/kubernetes/kubernetes/issues/62872 it is not currently
// possible to apply OpenAPI validation to a v1alpha1.Route.
func validateRoute(r *monitoringv1alpha1.Route, receivers, muteTimeIntervals map[string]struct{}, topLevelRoute bool) error {
if r == nil {
return nil
}
Expand Down Expand Up @@ -388,7 +389,6 @@ func validateAlertManagerRoutes(r *monitoringv1alpha1.Route, receivers, muteTime
}
}

// validate that if defaults are set, they match regex
if r.GroupInterval != "" && !durationRe.MatchString(r.GroupInterval) {
return fmt.Errorf("groupInterval %s does not match required regex: %s", r.GroupInterval, durationRe.String())

Expand All @@ -401,13 +401,20 @@ func validateAlertManagerRoutes(r *monitoringv1alpha1.Route, receivers, muteTime
return fmt.Errorf("repeatInterval %s does not match required regex: %s", r.RepeatInterval, durationRe.String())
}

for i, m := range r.Matchers {
if err := m.Validate(); err != nil {
return fmt.Errorf("matcher[%d]: %w", i, err)
}
}

// Unmarshal the child routes and validate them recursively.
children, err := r.ChildRoutes()
if err != nil {
return err
}

for i := range children {
if err := validateAlertManagerRoutes(&children[i], receivers, muteTimeIntervals, false); err != nil {
if err := validateRoute(&children[i], receivers, muteTimeIntervals, false); err != nil {
return fmt.Errorf("route[%d]: %w", i, err)
}
}
Expand Down
Loading

0 comments on commit 7fc97ea

Please sign in to comment.