improve: event recorder correctness

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java

@@ -21,6 +21,8 @@
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.time.Instant;
+import java.time.ZoneOffset;
+import java.time.format.DateTimeFormatter;
 import java.time.temporal.ChronoUnit;
 import java.util.HexFormat;
 import java.util.Objects;
@@ -32,6 +34,7 @@
 import io.fabric8.kubernetes.api.model.Event;
 import io.fabric8.kubernetes.api.model.EventBuilder;
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.MicroTime;
 import io.fabric8.kubernetes.api.model.ObjectReference;
 import io.fabric8.kubernetes.api.model.ObjectReferenceBuilder;
 import io.javaoperatorsdk.operator.api.config.LeaderElectionConfiguration;
@@ -75,6 +78,12 @@ public class DefaultEventRecorder implements EventRecorder {
 
   private static final int IDENTITY_HASH_LENGTH = 32;
 
+  /**
+   * The fixed width format of {@code MicroTime}, which the API server refuses to read otherwise.
+   */
+  private static final DateTimeFormatter EVENT_TIME_FORMAT =
+      DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSS'Z'").withZone(ZoneOffset.UTC);
+
   /** What the API server accepts as an object name, see RFC 1123 on DNS subdomains. */
   private static final Pattern RFC_1123_SUBDOMAIN =
       Pattern.compile("[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*");
@@ -211,11 +220,12 @@ protected Event toEvent(Context<?> context, EventRecord record) {
     var regarding = context.getPrimaryResource();
     var now = Instant.now().truncatedTo(ChronoUnit.SECONDS).toString();
     var involvedObject = objectReferenceFor(regarding);
+    var namespace = eventNamespace(regarding, context);
     var builder =
         new EventBuilder()
             .withNewMetadata()
             .withName(eventName(regarding, record, controllerName))
-            .withNamespace(eventNamespace(regarding, context))
+            .withNamespace(namespace)
             .withLabels(record.labels())
             .withAnnotations(record.annotations())
             .endMetadata()
@@ -256,9 +266,26 @@ protected Event toEvent(Context<?> context, EventRecord record) {
           .endMetadata();
     }
     record.action().ifPresent(builder::withAction);
+    if (requiresEventTime(regarding, namespace)) {
+      builder.withEventTime(new MicroTime(EVENT_TIME_FORMAT.format(Instant.now())));
+      if (record.action().isEmpty()) {
+        builder.withAction(record.reason());
+      }
+    }
     return builder.build();
   }
 
+  /**
+   * The API server only accepts an event without {@code eventTime} for a cluster scoped object if
+   * the event lives in the {@code default} namespace. Events that carry an {@code eventTime} are
+   * not subject to that restriction, but then have to set an {@code action} too.
+   */
+  private static boolean requiresEventTime(HasMetadata regarding, String eventNamespace) {
+    return regarding.getMetadata().getNamespace() == null
+        && !CLUSTER_SCOPED_EVENT_NAMESPACE.equals(eventNamespace)
+        && !eventNamespace.isEmpty();
+  }
+
   private String eventNamespace(HasMetadata regarding, Context<?> context) {
     var namespace = regarding.getMetadata().getNamespace();
     if (namespace != null) {
@@ -295,15 +322,17 @@ private static String truncateToMaxNameLength(String name) {
     if (name.length() <= MAX_NAME_LENGTH) {
       return name;
     }
-    var truncated = name.substring(0, MAX_NAME_LENGTH);
+    var suffix = "." + identityDigest(name);
+    var truncated = name.substring(0, MAX_NAME_LENGTH - suffix.length());
     while (truncated.endsWith("-") || truncated.endsWith(".")) {
       truncated = truncated.substring(0, truncated.length() - 1);
     }
+    var shortened = truncated + suffix;
     log.warn(
-        "Truncated the name of event {} to {} to stay within the Kubernetes name limit",
+        "Shortened the name of event {} to {} to stay within the Kubernetes name limit",
         name,
-        truncated);
-    return truncated;
+        shortened);
+    return shortened;
   }
 
   /**

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventSink.java

@@ -15,6 +15,8 @@
  */
 package io.javaoperatorsdk.operator.api.event;
 
+import java.util.Objects;
+
 import io.fabric8.kubernetes.api.model.Event;
 import io.fabric8.kubernetes.api.model.EventBuilder;
 import io.fabric8.kubernetes.client.KubernetesClient;
@@ -30,8 +32,12 @@
  * <p>An event that does not exist yet is created. An event that does is a repeat of one already
  * recorded, since the name of an event is derived from everything that identifies it (see {@link
  * DefaultEventRecorder}), and is aggregated onto the recorded one by patching its {@code count} and
- * {@code lastTimestamp}. That is what makes {@code kubectl describe} report a repeating event once,
- * as {@code (x12 over 3m)}, rather than filling the event list with copies of it.
+ * {@code lastTimestamp}, along with its {@code message}, {@code type}, {@code reason} and {@code
+ * action}, which events sharing a name explicitly may differ in, unless the event carries an {@code
+ * eventTime} (see {@link DefaultEventRecorder}), in which case the API server treats them as
+ * immutable and recording an event that differs in them is rejected. That is what makes {@code
+ * kubectl describe} report a repeating event once, as {@code (x12 over 3m)}, rather than filling
+ * the event list with copies of it.
  *
  * <p>Recording therefore needs {@code get}, {@code create} and {@code patch} on {@code events} in
  * the core API group. The patch is made on the resource version the event was read at, so two
@@ -55,13 +61,31 @@ public void emit(Event event, Context<?> context) {
     if (existing == null) {
       events.resource(event).create();
     } else {
-      var aggregated =
+      if (existing.getEventTime() != null && differsInImmutableFields(existing, event)) {
+        throw new IllegalStateException(
+            "Event "
+                + name
+                + " carries an eventTime, so its type, reason and action cannot change, but the"
+                + " event to record differs in them: use distinct event names for them");
+      }
+      var builder =
           new EventBuilder(existing)
               .withCount(requireNonNullElse(existing.getCount(), 1) + 1)
               .withLastTimestamp(event.getLastTimestamp())
               .withMessage(event.getMessage())
-              .build();
+              .withType(event.getType())
+              .withReason(event.getReason());
+      if (event.getAction() != null) {
+        builder.withAction(event.getAction());
+      }
+      var aggregated = builder.build();
       events.resource(existing).patch(aggregated);
     }
   }
+
+  private static boolean differsInImmutableFields(Event existing, Event event) {
+    return !Objects.equals(existing.getType(), event.getType())
+        || !Objects.equals(existing.getReason(), event.getReason())
+        || (event.getAction() != null && !Objects.equals(existing.getAction(), event.getAction()));
+  }
 }

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java

@@ -436,19 +436,41 @@ void stripsTrailingSeparatorsFromTruncatedNames() {
         EventRecord.builder()
             .reason("Created")
             .message("created")
-            .name("a".repeat(252) + "." + "b".repeat(47))
+            .name("a".repeat(219) + "." + "b".repeat(47))
             .build(),
         context(configMap()));
     recorder.record(
         EventRecord.builder()
             .reason("Created")
             .message("created")
-            .name("b".repeat(252) + "-" + "c".repeat(47))
+            .name("b".repeat(219) + "-" + "c".repeat(47))
             .build(),
         context(configMap()));
 
-    assertThat(emitted.get(0).getMetadata().getName()).isEqualTo("a".repeat(252));
-    assertThat(emitted.get(1).getMetadata().getName()).isEqualTo("b".repeat(252));
+    assertThat(emitted.get(0).getMetadata().getName()).matches("a{219}\\.[0-9a-f]{32}");
+    assertThat(emitted.get(1).getMetadata().getName()).matches("b{219}\\.[0-9a-f]{32}");
+  }
+
+  @Test
+  void keepsLongSuppliedNamesSharingTheirPrefixDistinct() {
+    recorder.record(
+        EventRecord.builder()
+            .reason("Created")
+            .message("created")
+            .name("a".repeat(300) + "-one")
+            .build(),
+        context(configMap()));
+    recorder.record(
+        EventRecord.builder()
+            .reason("Created")
+            .message("created")
+            .name("a".repeat(300) + "-two")
+            .build(),
+        context(configMap()));
+
+    assertThat(emitted.get(0).getMetadata().getName()).hasSize(253);
+    assertThat(emitted.get(0).getMetadata().getName())
+        .isNotEqualTo(emitted.get(1).getMetadata().getName());
   }
 
   @Test