[RFC-0008] Custom Event Metadata from Annotations

Signed-off-by: Matheus Pimenta <[email protected]>

docs/spec/v1beta3/alerts.md

@@ -28,9 +28,13 @@ metadata:
   name: slack
   namespace: flux-system
 spec:
-  summary: "Cluster addons impacted in us-east-2"
+  summary: Cluster addons impacted
   providerRef:
     name: slack-bot
+  eventMetadata:
+    env: prod
+    cluster: prod-us-east-2
+    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,12 @@ 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. 
+Please refer to [`.spec.eventMetadata`](#event-metadata) for a better method of defining metadata
+in alerts. `.spec.summary` may be deprecated in a future release.
 
 ### Provider reference
 
@@ -151,6 +157,8 @@ would override one already present on the original event as generated by the emi
 then the override doesn't happen, i.e. the original value is preserved, and an info
 log is printed.
 
+TODO: update the docs according to RFC 0008 (and link the RFC)
+
 #### Example
 
 Add metadata fields to successful `HelmRelease` events:

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, &notification, alert)
+	s.combineEventMetadata(ctx, &notification, alert)
 
 	return sender, &notification, token, provider.GetTimeout(), nil
 }
@@ -418,30 +419,88 @@ 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"
+	)
+
+	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)
+	}
+
+	// 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) })
+		log.FromContext(ctx).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 +509,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)
 	}

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