improve: event recorder correctness

docs/content/en/docs/documentation/operations/configuration.md

@@ -379,7 +379,7 @@ All operator-level keys are prefixed with `josdk.`.
 
 | Key | Type | Description |
 |---|---|---|
-| `josdk.events.cluster-scoped-namespace` | `String` | Namespace to record events about cluster scoped resources in (defaults to `default`) |
+| `josdk.events.cluster-scoped-namespace` | `String` | Namespace to record events about cluster scoped resources in (defaults to `default`); the API server only accepts `default` and `kube-system` |
 
 Recording events requires the operator's service account to be allowed `get`, `create` and `patch` on
 `events` in the core (`""`) API group. All three are used: an event that does not exist yet is
@@ -395,6 +395,11 @@ therefore has to grant the permission in every namespace events are recorded in
 `default`, or whatever `josdk.events.cluster-scoped-namespace` is set to, if the operator reconciles
 cluster scoped resources. A `ClusterRole` covers all of them at once.
 
+The API server only accepts events about cluster scoped objects in the `default` and `kube-system`
+namespaces, so those are the only values `josdk.events.cluster-scoped-namespace` can usefully be set
+to. In particular, the namespace the operator itself runs in is rejected, and the event dropped with
+a warning in the operator log.
+
 #### Leader Election
 
 Leader election is activated when at least one `josdk.leader-election.*` key is present.

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -389,9 +389,11 @@ default Optional<LeaderElectionConfiguration> getLeaderElectionConfiguration() {
    * <p>Defaults to the {@code default} namespace, following the Kubernetes convention, which is
    * also what the Go client does, so that events recorded by an operator end up alongside the ones
    * recorded by the built-in controllers. Operators whose RBAC does not allow creating events in
-   * the {@code default} namespace should override this, typically with the namespace the operator
-   * itself runs in. Note that recording an event is best effort, so a missing permission results in
-   * the event being dropped with a warning in the log rather than in an error.
+   * the {@code default} namespace can override this with {@code kube-system}. The API server
+   * rejects events about cluster scoped resources in any other namespace, such as the namespace the
+   * operator itself runs in. Note that recording an event is best effort, so a missing permission
+   * or a rejected namespace results in the event being dropped with a warning in the log rather
+   * than in an error.
    *
    * @return the namespace to record events about cluster scoped resources in
    */

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -201,8 +201,8 @@ public ConfigurationServiceOverrider withLeaderElectionConfiguration(
 
   /**
    * Sets the namespace in which Kubernetes events about cluster scoped resources are recorded. Use
-   * this when the operator is not allowed to create events in the {@code default} namespace,
-   * passing for example the namespace the operator itself runs in.
+   * this when the operator is not allowed to create events in the {@code default} namespace. The
+   * API server only accepts {@code default} and {@code kube-system} for such events.
    *
    * @param namespace the namespace to record events about cluster scoped resources in
    * @return this {@link ConfigurationServiceOverrider} for chained customization

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;
@@ -47,7 +50,8 @@
  * <p>Events for cluster scoped objects have to live in some namespace: by default the {@value
  * #CLUSTER_SCOPED_EVENT_NAMESPACE} namespace is used, following the Kubernetes convention, but it
  * can be overridden, see {@link
- * io.javaoperatorsdk.operator.api.config.ConfigurationService#clusterScopedEventNamespace()}.
+ * io.javaoperatorsdk.operator.api.config.ConfigurationService#clusterScopedEventNamespace()}. The
+ * API server only accepts such events in the {@code default} and {@code kube-system} namespaces.
  *
  * <p>By default, events are named deterministically, after the object they are about plus a hash of
  * everything that identifies the event, so that recording the same event again resolves to the
@@ -61,6 +65,8 @@ public class DefaultEventRecorder implements EventRecorder {
 
   public static final String CLUSTER_SCOPED_EVENT_NAMESPACE = "default";
 
+  private static final String KUBE_SYSTEM_NAMESPACE = "kube-system";
+
   /**
    * Kubernetes limits object names to 253 characters, as they have to be valid RFC 1123 DNS
    * subdomains.
@@ -75,6 +81,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 +223,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()
@@ -255,10 +268,28 @@ protected Event toEvent(Context<?> context, EventRecord record) {
           .endOwnerReference()
           .endMetadata();
     }
-    record.action().ifPresent(builder::withAction);
+    var action = record.action().filter(a -> !a.isBlank());
+    action.ifPresent(builder::withAction);
+    if (requiresEventTime(regarding, namespace)) {
+      builder.withEventTime(new MicroTime(EVENT_TIME_FORMAT.format(Instant.now())));
+      if (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. An event that carries an {@code eventTime}
+   * (and with it an {@code action}) is also accepted in the {@code kube-system} namespace. No other
+   * namespace is accepted for such an event, whether it has an {@code eventTime} or not.
+   */
+  private static boolean requiresEventTime(HasMetadata regarding, String eventNamespace) {
+    return regarding.getMetadata().getNamespace() == null
+        && KUBE_SYSTEM_NAMESPACE.equals(eventNamespace);
+  }
+
   private String eventNamespace(HasMetadata regarding, Context<?> context) {
     var namespace = regarding.getMetadata().getNamespace();
     if (namespace != null) {
@@ -295,14 +326,20 @@ 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 shortened = truncate(name, MAX_NAME_LENGTH - suffix.length()) + suffix;
+    log.warn(
+        "Shortened the name of event {} to {} to stay within the Kubernetes name limit",
+        name,
+        shortened);
+    return shortened;
+  }
+
+  private static String truncate(String name, int maxLength) {
+    var truncated = name.substring(0, Math.min(name.length(), maxLength));
     while (truncated.endsWith("-") || truncated.endsWith(".")) {
       truncated = truncated.substring(0, truncated.length() - 1);
     }
-    log.warn(
-        "Truncated the name of event {} to {} to stay within the Kubernetes name limit",
-        name,
-        truncated);
     return truncated;
   }
 
@@ -337,12 +374,7 @@ private String identityHashName(
                 .orElseGet(() -> requireNonNullElse(record.message(), "")));
 
     var suffix = "." + identityDigest(identity);
-    var prefix = metadata.getName();
-    var maxPrefixLength = MAX_NAME_LENGTH - suffix.length();
-    if (prefix.length() > maxPrefixLength) {
-      prefix = prefix.substring(0, maxPrefixLength);
-    }
-    return prefix + suffix;
+    return truncate(metadata.getName(), MAX_NAME_LENGTH - suffix.length()) + suffix;
   }
 
   /**

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

@@ -30,8 +30,13 @@
  * <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: they describe the latest
+ * occurrence. The API server only makes them immutable for {@code events.k8s.io/v1} requests, not
+ * for the core group this sink writes to. An event that carries an {@code eventTime} needs an
+ * {@code action}, so the reason stands in for it when the occurrence has none. 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,11 +60,18 @@ public void emit(Event event, Context<?> context) {
     if (existing == null) {
       events.resource(event).create();
     } else {
+      var action = event.getAction();
+      if (action == null && existing.getEventTime() != null) {
+        action = event.getReason();
+      }
       var aggregated =
           new EventBuilder(existing)
               .withCount(requireNonNullElse(existing.getCount(), 1) + 1)
               .withLastTimestamp(event.getLastTimestamp())
               .withMessage(event.getMessage())
+              .withType(event.getType())
+              .withReason(event.getReason())
+              .withAction(action)
               .build();
       events.resource(existing).patch(aggregated);
     }

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

@@ -30,8 +30,9 @@
  *
  * <p>A name must be a valid RFC 1123 DNS subdomain: at most 253 lowercase alphanumeric characters,
  * {@code -} or {@code .}, starting and ending with an alphanumeric character. Names longer than the
- * limit are truncated. A name derived from the object and a fixed lowercase suffix (such as {@code
- * <object>-status-report}) satisfies all of this by construction.
+ * limit are shortened, keeping a hash of the full name so distinct names stay distinct. A name
+ * derived from the object and a fixed lowercase suffix (such as {@code <object>-status-report})
+ * satisfies all of this by construction.
  *
  * <p>An empty result or an invalid name falls back to the default {@code <object>.<identity hash>}
  * name, rather than the event being lost to the API server rejecting the name.