diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java index 67764b7146..1d070d7e4e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorder.java +++ b/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; } /** diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventSink.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventSink.java index c2f5461ae8..8269759e95 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/event/DefaultEventSink.java +++ b/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 @@ *

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. * *

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())); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java index 388217b9be..3cedc6cc9f 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/DefaultEventRecorderTest.java +++ b/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 diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/EventRecorderRegressionTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/EventRecorderRegressionTest.java new file mode 100644 index 0000000000..4a1020e23f --- /dev/null +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/event/EventRecorderRegressionTest.java @@ -0,0 +1,244 @@ +/* + * Copyright Java Operator SDK Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.javaoperatorsdk.operator.api.event; + +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicReference; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.api.model.ConfigMap; +import io.fabric8.kubernetes.api.model.ConfigMapBuilder; +import io.fabric8.kubernetes.api.model.Event; +import io.fabric8.kubernetes.api.model.EventList; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.NamespaceBuilder; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.dsl.MixedOperation; +import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation; +import io.fabric8.kubernetes.client.dsl.Resource; +import io.fabric8.kubernetes.client.dsl.V1APIGroupDSL; +import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; +import io.javaoperatorsdk.operator.api.reconciler.Context; + +import static io.javaoperatorsdk.operator.api.config.LeaderElectionConfigurationBuilder.aLeaderElectionConfiguration; +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** Regression tests for the aggregation, naming and namespacing of recorded events. */ +class EventRecorderRegressionTest { + + private final List emitted = new ArrayList<>(); + + /** + * The core/v1 API server (legacyValidateEvent in pkg/apis/core/validation/events.go) rejects an + * event without eventTime whose involvedObject.namespace is empty unless the event itself lives + * in "" or "default". Overriding the cluster scoped event namespace, as + * ConfigurationService#clusterScopedEventNamespace recommends, therefore requires the event to + * carry an eventTime, and with it an action. + */ + @Test + void overriddenClusterScopedEventNamespaceProducesEventsTheCoreApiAccepts() { + var recorder = new DefaultEventRecorder((event, context) -> emitted.add(event)); + var namespace = + new NamespaceBuilder() + .withNewMetadata() + .withName("ns1") + .withUid("uid-2") + .endMetadata() + .build(); + + recorder.record(EventRecord.normal("Created", "created"), context(namespace, "operator-ns")); + + var event = emitted.get(0); + assertThat(event.getInvolvedObject().getNamespace()).isNull(); + assertThat(event.getMetadata().getNamespace()).isEqualTo("operator-ns"); + assertThat(event.getEventTime().getTime()) + .matches("\\d{4}-\\d{2}-\\d{2}T\\d{2}:\\d{2}:\\d{2}\\.\\d{6}Z"); + assertThat(event.getAction()).isEqualTo("Created"); + } + + @Test + void clusterScopedEventsInTheDefaultNamespaceDoNotCarryAnEventTime() { + var recorder = new DefaultEventRecorder((event, context) -> emitted.add(event)); + var namespace = + new NamespaceBuilder() + .withNewMetadata() + .withName("ns1") + .withUid("uid-2") + .endMetadata() + .build(); + + recorder.record(EventRecord.normal("Created", "created"), context(namespace, "default")); + + assertThat(emitted.get(0).getEventTime()).isNull(); + assertThat(emitted.get(0).getMetadata().getNamespace()).isEqualTo("default"); + } + + /** + * A naming strategy (here the one EventNamingStrategy's javadoc suggests, a fixed suffix per + * object) makes the name the aggregation identity on its own, so records with different type and + * reason resolve to the same event. DefaultEventSink then only patches count, lastTimestamp and + * message: the Warning is published under the type and reason of the earlier Normal event. + */ + @Test + void aggregatingOntoANamedEventKeepsTheTypeAndReasonOfTheFirstOccurrence() { + var stored = new AtomicReference(); + var patched = new AtomicReference(); + var sink = new DefaultEventSink(mockClient(stored, patched)); + var recorder = + DefaultEventRecorder.builder(sink) + .namingStrategy( + (regarding, record) -> + Optional.of(regarding.getMetadata().getName() + "-status-report")) + .build(); + var context = context(configMap("test1"), "default"); + + recorder.record(EventRecord.normal("Ready", "all good"), context); + recorder.record(EventRecord.warning("Degraded", "replica down"), context); + + assertThat(patched.get()).isNotNull(); + assertThat(patched.get().getCount()).isEqualTo(2); + assertThat(patched.get().getMessage()).isEqualTo("replica down"); + assertThat(patched.get().getType()).isEqualTo("Warning"); + assertThat(patched.get().getReason()).isEqualTo("Degraded"); + } + + @Test + void aggregatingOntoAnEventWithAnEventTimeRejectsChangedImmutableFields() { + var stored = new AtomicReference(); + var patched = new AtomicReference(); + var sink = new DefaultEventSink(mockClient(stored, patched)); + var recorder = + DefaultEventRecorder.builder(sink) + .namingStrategy((regarding, record) -> Optional.of("fixed-name")) + .build(); + var context = context(clusterScoped("ns1"), "operator-ns"); + + recorder.record(EventRecord.normal("Ready", "all good"), context); + recorder.record(EventRecord.warning("Degraded", "replica down"), context); + + assertThat(patched.get()).isNull(); + assertThat(stored.get().getType()).isEqualTo("Normal"); + + recorder.record(EventRecord.normal("Ready", "still good"), context); + + assertThat(patched.get().getCount()).isEqualTo(2); + } + + /** + * Supplied names are truncated to 253 characters, which silently maps distinct names onto the + * same event, so two different events of a long-named object are aggregated as one. + */ + @Test + void truncatingSuppliedNamesMergesDistinctEvents() { + var recorder = + DefaultEventRecorder.builder((event, context) -> emitted.add(event)) + .namingStrategy( + (regarding, record) -> + Optional.of( + regarding.getMetadata().getName() + "-" + record.reason().toLowerCase())) + .build(); + var context = context(configMap("a".repeat(245)), "default"); + + recorder.record(EventRecord.warning("BackoffLimitExceeded", "job failed"), context); + recorder.record(EventRecord.warning("BackoffStarted", "job retrying"), context); + + assertThat(emitted.get(0).getMetadata().getName()) + .isNotEqualTo(emitted.get(1).getMetadata().getName()); + } + + @SuppressWarnings({"unchecked", "rawtypes"}) + private KubernetesClient mockClient( + AtomicReference stored, AtomicReference patched) { + var client = mock(KubernetesClient.class); + var v1 = mock(V1APIGroupDSL.class); + MixedOperation> events = mock(MixedOperation.class); + NonNamespaceOperation> inNs = + mock(NonNamespaceOperation.class); + when(client.v1()).thenReturn(v1); + when(v1.events()).thenReturn(events); + when(events.inNamespace(anyString())).thenReturn(inNs); + Resource byName = mock(Resource.class); + when(inNs.withName(anyString())).thenReturn(byName); + when(byName.get()).thenAnswer(i -> stored.get()); + when(inNs.resource(any(Event.class))) + .thenAnswer( + i -> { + Event target = i.getArgument(0); + Resource r = mock(Resource.class); + when(r.create()) + .thenAnswer( + c -> { + stored.set(target); + return target; + }); + when(r.patch(any(Event.class))) + .thenAnswer( + p -> { + Event e = p.getArgument(0); + patched.set(e); + stored.set(e); + return e; + }); + return r; + }); + return client; + } + + @SuppressWarnings("rawtypes") + private Context context(HasMetadata primaryResource, String clusterScopedEventNamespace) { + var configurationService = mock(ConfigurationService.class); + when(configurationService.getLeaderElectionConfiguration()) + .thenReturn( + Optional.of(aLeaderElectionConfiguration("lease").withIdentity("pod-1").build())); + when(configurationService.clusterScopedEventNamespace()) + .thenReturn(clusterScopedEventNamespace); + ControllerConfiguration controllerConfiguration = mock(ControllerConfiguration.class); + when(controllerConfiguration.getName()).thenReturn("testcontroller"); + when(controllerConfiguration.getConfigurationService()).thenReturn(configurationService); + Context context = mock(Context.class); + when(context.getPrimaryResource()).thenReturn(primaryResource); + when(context.getControllerConfiguration()).thenReturn(controllerConfiguration); + return context; + } + + private HasMetadata clusterScoped(String name) { + return new NamespaceBuilder() + .withNewMetadata() + .withName(name) + .withUid("uid-3") + .endMetadata() + .build(); + } + + private ConfigMap configMap(String name) { + return new ConfigMapBuilder() + .withNewMetadata() + .withName(name) + .withNamespace("ns1") + .withUid("uid-1") + .withResourceVersion("42") + .endMetadata() + .build(); + } +}