From a899a4c1ceaa1d7471c5bc648ee2ff93e934171d Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Sep 2021 16:06:12 +0200 Subject: [PATCH 1/2] fix: only throw MissingCRDException if we get a 404 on the target CRD Fixes #552 --- .../operator/processing/EventDispatcher.java | 2 +- .../processing/event/DefaultEventSourceManager.java | 12 +++++++++--- .../event/DefaultEventSourceManagerTest.java | 4 +++- pom.xml | 2 +- 4 files changed, 14 insertions(+), 6 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java index 7ed0e25aca..79cf21bab9 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java @@ -24,7 +24,7 @@ /** * Dispatches events to the Controller and handles Finalizers for a single type of Custom Resource. */ -class EventDispatcher> { +public class EventDispatcher> { private static final Logger log = LoggerFactory.getLogger(EventDispatcher.class); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index ff7ba37b15..842d89c659 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -35,18 +35,21 @@ public class DefaultEventSourceManager> private final Map eventSources = new ConcurrentHashMap<>(); private final DefaultEventHandler defaultEventHandler; private TimerEventSource retryTimerEventSource; + private final String targetCRDName; - DefaultEventSourceManager(DefaultEventHandler defaultEventHandler, boolean supportRetry) { + DefaultEventSourceManager(DefaultEventHandler defaultEventHandler, boolean supportRetry, + String targetCRDName) { this.defaultEventHandler = defaultEventHandler; defaultEventHandler.setEventSourceManager(this); if (supportRetry) { this.retryTimerEventSource = new TimerEventSource<>(); registerEventSource(RETRY_TIMER_EVENT_SOURCE_NAME, retryTimerEventSource); } + this.targetCRDName = targetCRDName; } public DefaultEventSourceManager(ConfiguredController controller) { - this(new DefaultEventHandler<>(controller), true); + this(new DefaultEventHandler<>(controller), true, controller.getConfiguration().getCRDName()); registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, new CustomResourceEventSource<>(controller)); } @@ -92,7 +95,10 @@ public final void registerEventSource(String name, EventSource eventSource) if (e instanceof KubernetesClientException) { KubernetesClientException ke = (KubernetesClientException) e; if (404 == ke.getCode()) { - throw new MissingCRDException(null, null); + // only throw MissingCRDException if the 404 error occurs on the target CRD + if (targetCRDName.equals(ke.getFullResourceName())) { + throw new MissingCRDException(targetCRDName, null); + } } } throw new OperatorException("Couldn't register event source named '" + name + "'", e); diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java index 8862450d06..1475195030 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java @@ -9,6 +9,7 @@ import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; +import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static org.assertj.core.api.Assertions.assertThat; @@ -24,7 +25,8 @@ class DefaultEventSourceManagerTest { private DefaultEventHandler defaultEventHandlerMock = mock(DefaultEventHandler.class); private DefaultEventSourceManager defaultEventSourceManager = - new DefaultEventSourceManager(defaultEventHandlerMock, false); + new DefaultEventSourceManager(defaultEventHandlerMock, false, + CustomResource.getCRDName(TestCustomResource.class)); @Test public void registersEventSource() { diff --git a/pom.xml b/pom.xml index db32e5c2cc..52edf21105 100644 --- a/pom.xml +++ b/pom.xml @@ -41,7 +41,7 @@ ${java.version} 5.8.1 - 5.7.2 + 5.8.0 1.7.32 2.14.1 3.12.4 From 30190502bb6279f1b0b49a52e91ec21ad9857612 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 23 Sep 2021 22:50:44 +0200 Subject: [PATCH 2/2] refactor: move check for missing CRD to CustomResourceEventSource This indeed makes more sense like this as this should only happen when attempting to register watches for the primary resources. --- .../event/DefaultEventSourceManager.java | 19 ++-------- .../internal/CustomResourceEventSource.java | 38 +++++++++++++------ .../event/DefaultEventSourceManagerTest.java | 4 +- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java index 842d89c659..3aad5131f8 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java @@ -15,7 +15,6 @@ import org.slf4j.LoggerFactory; import io.fabric8.kubernetes.client.CustomResource; -import io.fabric8.kubernetes.client.KubernetesClientException; import io.javaoperatorsdk.operator.MissingCRDException; import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.processing.ConfiguredController; @@ -35,21 +34,18 @@ public class DefaultEventSourceManager> private final Map eventSources = new ConcurrentHashMap<>(); private final DefaultEventHandler defaultEventHandler; private TimerEventSource retryTimerEventSource; - private final String targetCRDName; - DefaultEventSourceManager(DefaultEventHandler defaultEventHandler, boolean supportRetry, - String targetCRDName) { + DefaultEventSourceManager(DefaultEventHandler defaultEventHandler, boolean supportRetry) { this.defaultEventHandler = defaultEventHandler; defaultEventHandler.setEventSourceManager(this); if (supportRetry) { this.retryTimerEventSource = new TimerEventSource<>(); registerEventSource(RETRY_TIMER_EVENT_SOURCE_NAME, retryTimerEventSource); } - this.targetCRDName = targetCRDName; } public DefaultEventSourceManager(ConfiguredController controller) { - this(new DefaultEventHandler<>(controller), true, controller.getConfiguration().getCRDName()); + this(new DefaultEventHandler<>(controller), true); registerEventSource(CUSTOM_RESOURCE_EVENT_SOURCE_NAME, new CustomResourceEventSource<>(controller)); } @@ -88,19 +84,10 @@ public final void registerEventSource(String name, EventSource eventSource) eventSource.setEventHandler(defaultEventHandler); eventSource.start(); } catch (Throwable e) { - if (e instanceof IllegalStateException) { + if (e instanceof IllegalStateException || e instanceof MissingCRDException) { // leave untouched throw e; } - if (e instanceof KubernetesClientException) { - KubernetesClientException ke = (KubernetesClientException) e; - if (404 == ke.getCode()) { - // only throw MissingCRDException if the 404 error occurs on the target CRD - if (targetCRDName.equals(ke.getFullResourceName())) { - throw new MissingCRDException(targetCRDName, null); - } - } - } throw new OperatorException("Couldn't register event source named '" + name + "'", e); } finally { lock.unlock(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java index 5c9f97621d..89f81efdf2 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java @@ -11,10 +11,12 @@ import io.fabric8.kubernetes.api.model.ListOptions; import io.fabric8.kubernetes.client.CustomResource; +import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.client.Watch; import io.fabric8.kubernetes.client.Watcher; import io.fabric8.kubernetes.client.WatcherException; import io.fabric8.kubernetes.client.utils.Utils; +import io.javaoperatorsdk.operator.MissingCRDException; import io.javaoperatorsdk.operator.api.config.ControllerConfiguration; import io.javaoperatorsdk.operator.processing.ConfiguredController; import io.javaoperatorsdk.operator.processing.CustomResourceCache; @@ -54,17 +56,31 @@ public void start() { options.setLabelSelector(labelSelector); } - if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { - var w = client.inAnyNamespace().watch(options, this); - watches.add(w); - log.debug("Registered {} -> {} for any namespace", controller, w); - } else { - targetNamespaces.forEach( - ns -> { - var w = client.inNamespace(ns).watch(options, this); - watches.add(w); - log.debug("Registered {} -> {} for namespace: {}", controller, w, ns); - }); + try { + if (ControllerConfiguration.allNamespacesWatched(targetNamespaces)) { + var w = client.inAnyNamespace().watch(options, this); + watches.add(w); + log.debug("Registered {} -> {} for any namespace", controller, w); + } else { + targetNamespaces.forEach( + ns -> { + var w = client.inNamespace(ns).watch(options, this); + watches.add(w); + log.debug("Registered {} -> {} for namespace: {}", controller, w, ns); + }); + } + } catch (Exception e) { + if (e instanceof KubernetesClientException) { + KubernetesClientException ke = (KubernetesClientException) e; + if (404 == ke.getCode()) { + // only throw MissingCRDException if the 404 error occurs on the target CRD + final var targetCRDName = controller.getConfiguration().getCRDName(); + if (targetCRDName.equals(ke.getFullResourceName())) { + throw new MissingCRDException(targetCRDName, null); + } + } + } + throw e; } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java index 1475195030..8862450d06 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java @@ -9,7 +9,6 @@ import io.javaoperatorsdk.operator.TestUtils; import io.javaoperatorsdk.operator.processing.DefaultEventHandler; import io.javaoperatorsdk.operator.processing.KubernetesResourceUtils; -import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; import static io.javaoperatorsdk.operator.processing.KubernetesResourceUtils.getUID; import static org.assertj.core.api.Assertions.assertThat; @@ -25,8 +24,7 @@ class DefaultEventSourceManagerTest { private DefaultEventHandler defaultEventHandlerMock = mock(DefaultEventHandler.class); private DefaultEventSourceManager defaultEventSourceManager = - new DefaultEventSourceManager(defaultEventHandlerMock, false, - CustomResource.getCRDName(TestCustomResource.class)); + new DefaultEventSourceManager(defaultEventHandlerMock, false); @Test public void registersEventSource() {