diff --git a/api/v1beta3/alert_types.go b/api/v1beta3/alert_types.go index 722d98999..660f836b3 100644 --- a/api/v1beta3/alert_types.go +++ b/api/v1beta3/alert_types.go @@ -64,8 +64,11 @@ type AlertSpec struct { ExclusionList []string `json:"exclusionList,omitempty"` // Summary holds a short description of the impact and affected cluster. + // Deprecated: Use EventMetadata instead. + // // +kubebuilder:validation:MaxLength:=255 // +optional + // +deprecated Summary string `json:"summary,omitempty"` // Suspend tells the controller to suspend subsequent diff --git a/config/crd/bases/notification.toolkit.fluxcd.io_alerts.yaml b/config/crd/bases/notification.toolkit.fluxcd.io_alerts.yaml index 0de250303..0ff9c2196 100644 --- a/config/crd/bases/notification.toolkit.fluxcd.io_alerts.yaml +++ b/config/crd/bases/notification.toolkit.fluxcd.io_alerts.yaml @@ -557,8 +557,9 @@ spec: - name type: object summary: - description: Summary holds a short description of the impact and affected - cluster. + description: |- + Summary holds a short description of the impact and affected cluster. + Deprecated: Use EventMetadata instead. maxLength: 255 type: string suspend: diff --git a/docs/api/v1beta3/notification.md b/docs/api/v1beta3/notification.md index cb972e603..34410f85d 100644 --- a/docs/api/v1beta3/notification.md +++ b/docs/api/v1beta3/notification.md @@ -161,7 +161,8 @@ string (Optional) -

Summary holds a short description of the impact and affected cluster.

+

Summary holds a short description of the impact and affected cluster. +Deprecated: Use EventMetadata instead.

@@ -492,7 +493,8 @@ string (Optional) -

Summary holds a short description of the impact and affected cluster.

+

Summary holds a short description of the impact and affected cluster. +Deprecated: Use EventMetadata instead.

diff --git a/docs/spec/v1beta3/alerts.md b/docs/spec/v1beta3/alerts.md index 6932e195d..90a9d9abd 100644 --- a/docs/spec/v1beta3/alerts.md +++ b/docs/spec/v1beta3/alerts.md @@ -28,9 +28,13 @@ metadata: name: slack namespace: flux-system spec: - summary: "Cluster addons impacted in us-east-2" providerRef: name: slack-bot + eventMetadata: + summary: Cluster addons impacted + env: prod + cluster: my-cluster + region: us-east-2 eventSeverity: error eventSources: - kind: GitRepository @@ -51,7 +55,7 @@ In the above example: all GitRepositories and Kustomizations in the `flux-system` namespace. - When an event with severity `error` is received, the controller posts a message on Slack channel from `.spec.channel`, - containing the `summary` text and the reconciliation error. + containing the `summary` text, metadata and the reconciliation error. You can run this example by saving the manifests into `slack-alerts.yaml`. @@ -78,10 +82,15 @@ An Alert also needs a ### Summary -`.spec.summary` is an optional field to specify a short description of the -impact and affected cluster. +`.spec.summary` is an optional field to specify a short description of the impact. + +The summary max length can't be greater than 255 characters. -The summary max length can't be greater than 255 characters. +**Warning:** Support for `.spec.summary` has been deprecated and will be removed in +Alert API v1 GA. If you have any Alerts using this field, the controller will log a +deprecation warning. Please use [`.spec.eventMetadata.summary`](#event-metadata) or +[object annotations](#event-metadata-from-object-annotations) for defining alert +summary instead. ### Provider reference @@ -146,10 +155,11 @@ preventing tenants from subscribing to another tenant's events. ### Event metadata `.spec.eventMetadata` is an optional field for adding metadata to events dispatched by -the controller. This can be used for enhancing the context of the event. If a field -would override one already present on the original event as generated by the emitter, -then the override doesn't happen, i.e. the original value is preserved, and an info -log is printed. +the controller. This can be used for enhancing the context of the event, e.g. with +cluster-level information. + +For all the event metadata sources and their precedence order, please refer to +[Event metadata from object annotations](#event-metadata-from-object-annotations). #### Example @@ -168,9 +178,68 @@ spec: inclusionList: - ".*succeeded.*" eventMetadata: - app.kubernetes.io/env: "production" - app.kubernetes.io/cluster: "my-cluster" - app.kubernetes.io/region: "us-east-1" + env: production + cluster: my-cluster + region: us-east-1 +``` + +### Event metadata from object annotations + +Event metadata has four sources. They are listed below in order of precedence, +from lowest to highest: + +1. User-defined metadata on Flux objects, set with the `event.toolkit.fluxcd.io/` +prefix in the keys of the object's `.metadata.annotations`. +2. User-defined metadata on the Alert object, set with [`.spec.eventMetadata`](#event-metadata). +3. User-defined summary on the Alert object, set with [`.spec.summary`](#summary) (deprecated, see docs). +4. Controller-defined metadata, set with the `.toolkit.fluxcd.io/` +prefix in the metadata keys of the event payload. + +If there are any metadata key conflicts between the sources, the higher +precedence source will override the lower precedence source, and a warning +log and Kubernetes event will be emitted. + +#### Example + +```yaml +--- +apiVersion: notification.toolkit.fluxcd.io/v1beta3 +kind: Alert +metadata: + name: +spec: + eventSources: + - kind: HelmRelease + name: '*' + eventMetadata: + env: production + cluster: my-cluster + region: us-east-1 +--- +apiVersion: helm.toolkit.fluxcd.io/v2 +kind: HelmRelease +metadata: + name: my-webapp + annotations: + event.toolkit.fluxcd.io/summary: "my-webapp impacted. Playbook: " + event.toolkit.fluxcd.io/deploymentID: e076e315-5a48-41c3-81c8-8d8bdee7d74d +spec: + ... # fields omitted for brevity +``` + +In the above example, the event payload dispatched by the controller will look like this +(most fields omitted for highlighting the metadata): + +```json +{ + "metadata": { + "env": "production", + "cluster": "my-cluster", + "region": "us-east-1", + "summary": "my-webapp impacted. Playbook: ", + "deploymentID": "e076e315-5a48-41c3-81c8-8d8bdee7d74d" + } +} ``` ### Event severity diff --git a/internal/server/event_handlers.go b/internal/server/event_handlers.go index 685948fd0..921c43650 100644 --- a/internal/server/event_handlers.go +++ b/internal/server/event_handlers.go @@ -24,6 +24,7 @@ import ( "net/http" "net/url" "regexp" + "slices" "strings" "time" @@ -256,7 +257,7 @@ func (s *EventServer) getNotificationParams(ctx context.Context, event *eventv1. } notification := *event.DeepCopy() - s.enhanceEventWithAlertMetadata(ctx, ¬ification, alert) + s.combineEventMetadata(ctx, ¬ification, alert) return sender, ¬ification, token, provider.GetTimeout(), nil } @@ -418,30 +419,90 @@ func (s *EventServer) eventMatchesAlertSource(ctx context.Context, event *eventv return sel.Matches(labels.Set(obj.GetLabels())) } -// enhanceEventWithAlertMetadata enhances the event with Alert metadata. -func (s *EventServer) enhanceEventWithAlertMetadata(ctx context.Context, event *eventv1.Event, alert *apiv1beta3.Alert) { - meta := event.Metadata - if meta == nil { - meta = make(map[string]string) +// combineEventMetadata combines all the sources of metadata for the event +// according to the precedence order defined in RFC 0008. From lowest to +// highest precedence, the sources are: +// +// 1) Event metadata keys prefixed with the Event API Group stripped of the prefix. +// +// 2) Alert .spec.eventMetadata with the keys as they are. +// +// 3) Alert .spec.summary with the key "summary". +// +// 4) Event metadata keys prefixed with the involved object's API Group stripped of the prefix. +// +// At the end of the process key conflicts are detected and a single +// info-level log is emitted to warn users about all the conflicts, +// but only if at least one conflict is found. +func (s *EventServer) combineEventMetadata(ctx context.Context, event *eventv1.Event, alert *apiv1beta3.Alert) { + const ( + sourceEventGroup = "involved object annotations" + sourceAlertEventMetadata = "Alert object .spec.eventMetadata" + sourceAlertSummary = "Alert object .spec.summary" + sourceObjectGroup = "involved object controller metadata" + + summaryKey = "summary" + ) + + l := log.FromContext(ctx) + metadata := make(map[string]string) + metadataSources := make(map[string][]string) + + // 1) Event metadata keys prefixed with the Event API Group stripped of the prefix. + eventGroupPrefix := "event.toolkit.fluxcd.io/" // TODO: use constant from github.com/fluxcd/pkg/apis/event when available + for k, v := range event.Metadata { + if strings.HasPrefix(k, eventGroupPrefix) { + key := strings.TrimPrefix(k, eventGroupPrefix) + metadata[key] = v + metadataSources[key] = append(metadataSources[key], sourceEventGroup) + } } - for key, value := range alert.Spec.EventMetadata { - if _, alreadyPresent := meta[key]; !alreadyPresent { - meta[key] = value - } else { - log.FromContext(ctx). - Info("metadata key found in the existing set of metadata", "key", key) - s.Eventf(alert, corev1.EventTypeWarning, "MetadataAppendFailed", - "metadata key found in the existing set of metadata for '%s' in %s", key, involvedObjectString(event.InvolvedObject)) - } + // 2) Alert .spec.eventMetadata with the keys as they are. + for k, v := range alert.Spec.EventMetadata { + metadata[k] = v + metadataSources[k] = append(metadataSources[k], sourceAlertEventMetadata) } + // 3) Alert .spec.summary with the key "summary". if alert.Spec.Summary != "" { - meta["summary"] = alert.Spec.Summary + metadata[summaryKey] = alert.Spec.Summary + metadataSources[summaryKey] = append(metadataSources[summaryKey], sourceAlertSummary) + l.Info("warning: specifying alert summary cert via '.spec.summary' is deprecated, please use '.spec.eventMetadata.summary' instead") + } + + // 4) Event metadata keys prefixed with the involved object's API Group stripped of the prefix. + objectGroupPrefix := event.InvolvedObject.GroupVersionKind().Group + "/" + for k, v := range event.Metadata { + if strings.HasPrefix(k, objectGroupPrefix) { + key := strings.TrimPrefix(k, objectGroupPrefix) + metadata[key] = v + metadataSources[key] = append(metadataSources[key], sourceObjectGroup) + } + } + + // Detect key conflicts and emit warnings if any. + type keyConflict struct { + Key string `json:"key"` + Sources []string `json:"sources"` + } + var conflictingKeys []*keyConflict + conflictEventAnnotations := make(map[string]string) + for key, sources := range metadataSources { + if len(sources) > 1 { + conflictingKeys = append(conflictingKeys, &keyConflict{key, sources}) + conflictEventAnnotations[key] = strings.Join(sources, ", ") + } + } + if len(conflictingKeys) > 0 { + const msg = "metadata key conflicts detected (please refer to the Alert API docs and Flux RFC 0008 for more information)" + slices.SortFunc(conflictingKeys, func(a, b *keyConflict) int { return strings.Compare(a.Key, b.Key) }) + l.Info("warning: "+msg, "conflictingKeys", conflictingKeys) + s.AnnotatedEventf(alert, conflictEventAnnotations, corev1.EventTypeWarning, "MetadataAppendFailed", "%s", msg) } - if len(meta) > 0 { - event.Metadata = meta + if len(metadata) > 0 { + event.Metadata = metadata } } @@ -450,7 +511,9 @@ func excludeInternalMetadata(event *eventv1.Event) { if len(event.Metadata) == 0 { return } - excludeList := []string{eventv1.MetaTokenKey} + objectGroup := event.InvolvedObject.GetObjectKind().GroupVersionKind().Group + tokenKey := fmt.Sprintf("%s/%s", objectGroup, eventv1.MetaTokenKey) + excludeList := []string{tokenKey} for _, key := range excludeList { delete(event.Metadata, key) } diff --git a/internal/server/event_handlers_test.go b/internal/server/event_handlers_test.go index 0b9f7aef6..9820ddf37 100644 --- a/internal/server/event_handlers_test.go +++ b/internal/server/event_handlers_test.go @@ -515,7 +515,7 @@ func TestGetNotificationParams(t *testing.T) { g.Expect(n.Metadata["summary"]).To(Equal(tt.alertSummary)) } // NOTE: This is performing simple check. Thorough test for event - // metadata is performed in TestEnhanceEventWithAlertMetadata. + // metadata is performed in TestCombineEventMetadata. if tt.alertEventMetadata != nil { for k, v := range tt.alertEventMetadata { g.Expect(n.Metadata).To(HaveKeyWithValue(k, v)) @@ -977,84 +977,143 @@ func TestEventMatchesAlert(t *testing.T) { } } -func TestEnhanceEventWithAlertMetadata(t *testing.T) { - s := &EventServer{ - logger: log.Log, - EventRecorder: record.NewFakeRecorder(32), - } - +func TestCombineEventMetadata(t *testing.T) { for name, tt := range map[string]struct { event eventv1.Event alert apiv1beta3.Alert expectedMetadata map[string]string + conflictEvent string }{ "empty metadata": { event: eventv1.Event{}, alert: apiv1beta3.Alert{}, expectedMetadata: nil, }, - "enhanced with summary": { - event: eventv1.Event{}, + "all metadata sources work": { + event: eventv1.Event{ + Metadata: map[string]string{ + "kustomize.toolkit.fluxcd.io/controllerMetadata1": "controllerMetadataValue1", + "kustomize.toolkit.fluxcd.io/controllerMetadata2": "controllerMetadataValue2", + "event.toolkit.fluxcd.io/objectMetadata1": "objectMetadataValue1", + "event.toolkit.fluxcd.io/objectMetadata2": "objectMetadataValue2", + }, + }, alert: apiv1beta3.Alert{ Spec: apiv1beta3.AlertSpec{ - Summary: "summary", + Summary: "summaryValue", + EventMetadata: map[string]string{ + "foo": "bar", + "baz": "qux", + }, }, }, expectedMetadata: map[string]string{ - "summary": "summary", + "foo": "bar", + "baz": "qux", + "controllerMetadata1": "controllerMetadataValue1", + "controllerMetadata2": "controllerMetadataValue2", + "summary": "summaryValue", + "objectMetadata1": "objectMetadataValue1", + "objectMetadata2": "objectMetadataValue2", }, }, - "overriden with summary": { + "object metadata is overriden by summary": { event: eventv1.Event{ Metadata: map[string]string{ - "summary": "original summary", + "event.toolkit.fluxcd.io/summary": "objectSummary", }, }, alert: apiv1beta3.Alert{ Spec: apiv1beta3.AlertSpec{ - Summary: "summary", + Summary: "alertSummary", }, }, expectedMetadata: map[string]string{ - "summary": "summary", + "summary": "alertSummary", }, + conflictEvent: "Warning MetadataAppendFailed metadata key conflicts detected (please refer to the Alert API docs and Flux RFC 0008 for more information) map[summary:involved object annotations, Alert object .spec.summary]", }, - "enhanced with metadata": { + "alert event metadata is overriden by summary": { event: eventv1.Event{}, alert: apiv1beta3.Alert{ Spec: apiv1beta3.AlertSpec{ + Summary: "alertSummary", EventMetadata: map[string]string{ - "foo": "bar", + "summary": "eventMetadataSummary", }, }, }, expectedMetadata: map[string]string{ - "foo": "bar", + "summary": "alertSummary", + }, + conflictEvent: "Warning MetadataAppendFailed metadata key conflicts detected (please refer to the Alert API docs and Flux RFC 0008 for more information) map[summary:Alert object .spec.eventMetadata, Alert object .spec.summary]", + }, + "summary is overriden by controller metadata": { + event: eventv1.Event{ + Metadata: map[string]string{ + "kustomize.toolkit.fluxcd.io/summary": "controllerSummary", + }, + }, + alert: apiv1beta3.Alert{ + Spec: apiv1beta3.AlertSpec{ + Summary: "alertSummary", + }, + }, + expectedMetadata: map[string]string{ + "summary": "controllerSummary", }, + conflictEvent: "Warning MetadataAppendFailed metadata key conflicts detected (please refer to the Alert API docs and Flux RFC 0008 for more information) map[summary:Alert object .spec.summary, involved object controller metadata]", }, - "skipped override with metadata": { + "precedence order in RFC 0008 is honered": { event: eventv1.Event{ Metadata: map[string]string{ - "foo": "baz", + "kustomize.toolkit.fluxcd.io/objectMetadataOverridenByController": "controllerMetadataValue1", + "kustomize.toolkit.fluxcd.io/alertMetadataOverridenByController": "controllerMetadataValue2", + "kustomize.toolkit.fluxcd.io/controllerMetadata": "controllerMetadataValue3", + "event.toolkit.fluxcd.io/objectMetadata": "objectMetadataValue1", + "event.toolkit.fluxcd.io/objectMetadataOverridenByAlert": "objectMetadataValue2", + "event.toolkit.fluxcd.io/objectMetadataOverridenByController": "objectMetadataValue3", }, }, alert: apiv1beta3.Alert{ Spec: apiv1beta3.AlertSpec{ EventMetadata: map[string]string{ - "foo": "bar", + "objectMetadataOverridenByAlert": "alertMetadataValue1", + "alertMetadata": "alertMetadataValue2", + "alertMetadataOverridenByController": "alertMetadataValue3", }, }, }, expectedMetadata: map[string]string{ - "foo": "baz", - }, + "objectMetadata": "objectMetadataValue1", + "objectMetadataOverridenByAlert": "alertMetadataValue1", + "objectMetadataOverridenByController": "controllerMetadataValue1", + "alertMetadata": "alertMetadataValue2", + "alertMetadataOverridenByController": "controllerMetadataValue2", + "controllerMetadata": "controllerMetadataValue3", + }, + conflictEvent: "Warning MetadataAppendFailed metadata key conflicts detected (please refer to the Alert API docs and Flux RFC 0008 for more information) map[alertMetadataOverridenByController:Alert object .spec.eventMetadata, involved object controller metadata objectMetadataOverridenByAlert:involved object annotations, Alert object .spec.eventMetadata objectMetadataOverridenByController:involved object annotations, involved object controller metadata]", }, } { t.Run(name, func(t *testing.T) { g := NewGomegaWithT(t) - s.enhanceEventWithAlertMetadata(context.Background(), &tt.event, &tt.alert) + eventRecorder := record.NewFakeRecorder(1) + s := &EventServer{ + logger: log.Log, + EventRecorder: eventRecorder, + } + + tt.event.InvolvedObject.APIVersion = "kustomize.toolkit.fluxcd.io/v1" + s.combineEventMetadata(context.Background(), &tt.event, &tt.alert) g.Expect(tt.event.Metadata).To(BeEquivalentTo(tt.expectedMetadata)) + + var event string + select { + case event = <-eventRecorder.Events: + default: + } + g.Expect(event).To(Equal(tt.conflictEvent)) }) } } @@ -1071,13 +1130,16 @@ func Test_excludeInternalMetadata(t *testing.T) { { name: "internal metadata", event: eventv1.Event{ + InvolvedObject: corev1.ObjectReference{ + APIVersion: "kustomize.toolkit.fluxcd.io/v1", + }, Metadata: map[string]string{ - eventv1.MetaTokenKey: "aaaa", - eventv1.MetaRevisionKey: "bbbb", + "kustomize.toolkit.fluxcd.io/" + eventv1.MetaTokenKey: "aaaa", + "kustomize.toolkit.fluxcd.io/" + eventv1.MetaRevisionKey: "bbbb", }, }, wantMetadata: map[string]string{ - eventv1.MetaRevisionKey: "bbbb", + "kustomize.toolkit.fluxcd.io/" + eventv1.MetaRevisionKey: "bbbb", }, }, } diff --git a/internal/server/event_server.go b/internal/server/event_server.go index 9aa5eb110..6cee75502 100644 --- a/internal/server/event_server.go +++ b/internal/server/event_server.go @@ -156,21 +156,23 @@ func (s *EventServer) eventMiddleware(h http.Handler) http.Handler { } // cleanupMetadata removes metadata entries which are not used for alerting. +// In particular, it removes the checksum and digest metadata entries and +// keeps only the metadata entries that are prefixed with either the event +// group prefix or the involved object's group prefix. func cleanupMetadata(event *eventv1.Event) { - group := event.InvolvedObject.GetObjectKind().GroupVersionKind().Group + eventGroupPrefix := "event.toolkit.fluxcd.io/" // TODO: use constant from github.com/fluxcd/pkg/apis/event when available + objectGroupPrefix := event.InvolvedObject.GetObjectKind().GroupVersionKind().Group + "/" excludeList := []string{ - fmt.Sprintf("%s/%s", group, eventv1.MetaChecksumKey), - fmt.Sprintf("%s/%s", group, eventv1.MetaDigestKey), + fmt.Sprintf("%s%s", objectGroupPrefix, eventv1.MetaChecksumKey), + fmt.Sprintf("%s%s", objectGroupPrefix, eventv1.MetaDigestKey), } + // Filter other meta based on group prefix, while filtering out excludes meta := make(map[string]string) - if event.Metadata != nil && len(event.Metadata) > 0 { - // Filter other meta based on group prefix, while filtering out excludes - for key, val := range event.Metadata { - if strings.HasPrefix(key, group) && !inList(excludeList, key) { - newKey := strings.TrimPrefix(key, fmt.Sprintf("%s/", group)) - meta[newKey] = val - } + for key, val := range event.Metadata { + if !inList(excludeList, key) && + (strings.HasPrefix(key, eventGroupPrefix) || strings.HasPrefix(key, objectGroupPrefix)) { + meta[key] = val } } @@ -229,12 +231,16 @@ func eventKeyFunc(r *http.Request) (string, error) { "message=" + event.Message, } - revision, ok := event.Metadata[eventv1.MetaRevisionKey] + objectGroup := event.InvolvedObject.GetObjectKind().GroupVersionKind().Group + + revisionKey := fmt.Sprintf("%s/%s", objectGroup, eventv1.MetaRevisionKey) + revision, ok := event.Metadata[revisionKey] if ok { comps = append(comps, "revision="+revision) } - token, ok := event.Metadata[eventv1.MetaTokenKey] + tokenKey := fmt.Sprintf("%s/%s", objectGroup, eventv1.MetaTokenKey) + token, ok := event.Metadata[tokenKey] if ok { comps = append(comps, "token="+token) } diff --git a/internal/server/event_server_test.go b/internal/server/event_server_test.go index 1b47a3269..ac1e327cc 100644 --- a/internal/server/event_server_test.go +++ b/internal/server/event_server_test.go @@ -562,13 +562,17 @@ func TestCleanupMetadata(t *testing.T) { "source.toolkit.fluxcd.io/baz": "bazval", group + "/zzz": "zzzz", group + "/aa/bb": "cc", + "event.toolkit.fluxcd.io/baz": "foo", + "event.toolkit.fluxcd.io/bazfoo": "foobaz", }, }, wantMeta: map[string]string{ - "foo": "fooval", - "bar": "barval", - "zzz": "zzzz", - "aa/bb": "cc", + "kustomize.toolkit.fluxcd.io/foo": "fooval", + "kustomize.toolkit.fluxcd.io/bar": "barval", + "kustomize.toolkit.fluxcd.io/zzz": "zzzz", + "kustomize.toolkit.fluxcd.io/aa/bb": "cc", + "event.toolkit.fluxcd.io/baz": "foo", + "event.toolkit.fluxcd.io/bazfoo": "foobaz", }, }, }