From 4ea79fc88e010f236fbb3afb1288671ae155a1c0 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 11 Jan 2022 22:27:35 +0100 Subject: [PATCH 1/3] fix: generate proper resource name & finalizer for group-less resources Fixes #812 --- .../io/javaoperatorsdk/operator/ReconcilerUtils.java | 10 ++++++++-- .../operator/api/config/ControllerConfiguration.java | 10 +++++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java index dbf3d1a59e..e789375ad5 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java @@ -10,9 +10,15 @@ public class ReconcilerUtils { private static final String FINALIZER_NAME_SUFFIX = "/finalizer"; + protected static final String MISSING_GROUP_SUFFIX = ".javaoperatorsdk.io"; - public static String getDefaultFinalizerName(String crdName) { - return crdName + FINALIZER_NAME_SUFFIX; + public static String getDefaultFinalizerName(String resourceName) { + // resource names for historic resources such as Pods are missing periods and therefore do not + // constitute valid domain names as mandated by Kubernetes so generate one that does + if (resourceName.indexOf('.') < 0) { + resourceName = resourceName + MISSING_GROUP_SUFFIX; + } + return resourceName + FINALIZER_NAME_SUFFIX; } public static String getNameFor(Class reconcilerClass) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 1353e6ea38..0c990f6d83 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -17,7 +17,15 @@ default String getName() { } default String getResourceTypeName() { - return HasMetadata.getFullResourceName(getResourceClass()); + return getResourceTypeName(getResourceClass()); + } + + static String getResourceTypeName(Class resourceClass) { + final var group = HasMetadata.getGroup(resourceClass); + final var plural = HasMetadata.getPlural(resourceClass); + return (group == null || group.isEmpty()) ? plural : plural + "." + group; + // use when fabric8 5.12 is released + // return HasMetadata.getFullResourceName(getResourceClass()); } default String getFinalizer() { From daa820f8b06a711dbb191d81c1715fe05a1b2f2c Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 12 Jan 2022 11:25:03 +0100 Subject: [PATCH 2/3] refactor: clean things up, add tests --- .../operator/ReconcilerUtils.java | 39 ++++++++++++++++++- .../api/config/ControllerConfiguration.java | 12 +----- .../operator/ReconcilerUtilsTest.java | 24 ++++++++++-- .../runtime/AnnotationConfiguration.java | 12 ++++-- .../DefaultConfigurationServiceTest.java | 10 ++--- .../EventSourceTestCustomReconciler.java | 4 +- .../retry/RetryTestCustomReconciler.java | 4 +- .../sample/simple/TestReconciler.java | 3 +- .../SubResourceTestCustomReconciler.java | 4 +- 9 files changed, 79 insertions(+), 33 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java index e789375ad5..bc0d3e921e 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/ReconcilerUtils.java @@ -2,6 +2,8 @@ import java.util.Locale; +import io.fabric8.kubernetes.api.model.HasMetadata; +import io.fabric8.kubernetes.api.model.ObjectMeta; import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; @@ -12,7 +14,42 @@ public class ReconcilerUtils { private static final String FINALIZER_NAME_SUFFIX = "/finalizer"; protected static final String MISSING_GROUP_SUFFIX = ".javaoperatorsdk.io"; - public static String getDefaultFinalizerName(String resourceName) { + // prevent instantiation of util class + private ReconcilerUtils() {} + + public static boolean isFinalizerValid(String finalizer) { + // todo: use fabric8 method when 5.12 is released + // return HasMetadata.validateFinalizer(finalizer); + final var validator = new HasMetadata() { + + @Override + public ObjectMeta getMetadata() { + throw new UnsupportedOperationException(); + } + + @Override + public void setMetadata(ObjectMeta objectMeta) { + throw new UnsupportedOperationException(); + } + + @Override + public void setApiVersion(String s) { + throw new UnsupportedOperationException(); + } + }; + return validator.isFinalizerValid(finalizer); + } + + public static String getResourceTypeName(Class resourceClass) { + // todo: use fabric8 method when 5.12 is released + // return HasMetadata.getFullResourceName(resourceClass); + final var group = HasMetadata.getGroup(resourceClass); + final var plural = HasMetadata.getPlural(resourceClass); + return (group == null || group.isEmpty()) ? plural : plural + "." + group; + } + + public static String getDefaultFinalizerName(Class resourceClass) { + var resourceName = getResourceTypeName(resourceClass); // resource names for historic resources such as Pods are missing periods and therefore do not // constitute valid domain names as mandated by Kubernetes so generate one that does if (resourceName.indexOf('.') < 0) { diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java index 0c990f6d83..b85df49eba 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java @@ -17,19 +17,11 @@ default String getName() { } default String getResourceTypeName() { - return getResourceTypeName(getResourceClass()); - } - - static String getResourceTypeName(Class resourceClass) { - final var group = HasMetadata.getGroup(resourceClass); - final var plural = HasMetadata.getPlural(resourceClass); - return (group == null || group.isEmpty()) ? plural : plural + "." + group; - // use when fabric8 5.12 is released - // return HasMetadata.getFullResourceName(getResourceClass()); + return ReconcilerUtils.getResourceTypeName(getResourceClass()); } default String getFinalizer() { - return ReconcilerUtils.getDefaultFinalizerName(getResourceTypeName()); + return ReconcilerUtils.getDefaultFinalizerName(getResourceClass()); } /** diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsTest.java index 47870e4f93..cf97be1d2d 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/ReconcilerUtilsTest.java @@ -2,17 +2,35 @@ import org.junit.jupiter.api.Test; +import io.fabric8.kubernetes.api.model.Pod; import io.javaoperatorsdk.operator.sample.simple.TestCustomReconciler; +import io.javaoperatorsdk.operator.sample.simple.TestCustomResource; +import static io.javaoperatorsdk.operator.ReconcilerUtils.getDefaultFinalizerName; +import static io.javaoperatorsdk.operator.ReconcilerUtils.getDefaultNameFor; +import static io.javaoperatorsdk.operator.ReconcilerUtils.getDefaultReconcilerName; +import static io.javaoperatorsdk.operator.ReconcilerUtils.isFinalizerValid; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; class ReconcilerUtilsTest { @Test - void getDefaultResourceControllerName() { + void defaultReconcilerNameShouldWork() { assertEquals( "testcustomreconciler", - ReconcilerUtils.getDefaultReconcilerName( - TestCustomReconciler.class.getCanonicalName())); + getDefaultReconcilerName(TestCustomReconciler.class.getCanonicalName())); + assertEquals( + getDefaultNameFor(TestCustomReconciler.class), + getDefaultReconcilerName(TestCustomReconciler.class.getCanonicalName())); + assertEquals( + getDefaultNameFor(TestCustomReconciler.class), + getDefaultReconcilerName(TestCustomReconciler.class.getSimpleName())); + } + + @Test + void defaultFinalizerShouldWork() { + assertTrue(isFinalizerValid(getDefaultFinalizerName(Pod.class))); + assertTrue(isFinalizerValid(getDefaultFinalizerName(TestCustomResource.class))); } } diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java index 7bda61874c..1000cb61c4 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java @@ -31,9 +31,15 @@ public String getName() { @Override public String getFinalizer() { if (annotation == null || annotation.finalizerName().isBlank()) { - return ReconcilerUtils.getDefaultFinalizerName(getResourceTypeName()); + return ReconcilerUtils.getDefaultFinalizerName(getResourceClass()); } else { - return annotation.finalizerName(); + final var finalizer = annotation.finalizerName(); + if (ReconcilerUtils.isFinalizerValid(finalizer)) { + return finalizer; + } else { + throw new IllegalArgumentException(finalizer + + " is not a valid finalizer. See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers for details"); + } } } @@ -93,7 +99,7 @@ public ResourceEventFilter getEventFilter() { answer = answer.and(filter); } } catch (Exception e) { - throw new RuntimeException(e); + throw new IllegalArgumentException(e); } } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationServiceTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationServiceTest.java index 9c4bf8c9ab..bce957a399 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationServiceTest.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/DefaultConfigurationServiceTest.java @@ -22,7 +22,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -public class DefaultConfigurationServiceTest { +class DefaultConfigurationServiceTest { public static final String CUSTOM_FINALIZER_NAME = "a.custom/finalizer"; @@ -73,21 +73,21 @@ void attemptingToRetrieveAnUnknownControllerShouldLogWarning() { } @Test - public void returnsValuesFromControllerAnnotationFinalizer() { + void returnsValuesFromControllerAnnotationFinalizer() { final var reconciler = new TestCustomReconciler(); final var configuration = DefaultConfigurationService.instance().getConfigurationFor(reconciler); assertEquals(CustomResource.getCRDName(TestCustomResource.class), configuration.getResourceTypeName()); assertEquals( - ReconcilerUtils.getDefaultFinalizerName(configuration.getResourceTypeName()), + ReconcilerUtils.getDefaultFinalizerName(TestCustomResource.class), configuration.getFinalizer()); assertEquals(TestCustomResource.class, configuration.getResourceClass()); assertFalse(configuration.isGenerationAware()); } @Test - public void returnCustomerFinalizerNameIfSet() { + void returnCustomerFinalizerNameIfSet() { final var reconciler = new TestCustomFinalizerReconciler(); final var configuration = DefaultConfigurationService.instance().getConfigurationFor(reconciler); @@ -95,7 +95,7 @@ public void returnCustomerFinalizerNameIfSet() { } @Test - public void supportsInnerClassCustomResources() { + void supportsInnerClassCustomResources() { final var reconciler = new TestCustomFinalizerReconciler(); assertDoesNotThrow( () -> { diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomReconciler.java index 3319ecc86c..55eb61634f 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomReconciler.java @@ -2,7 +2,6 @@ import java.util.concurrent.atomic.AtomicInteger; -import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.*; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; @@ -14,8 +13,7 @@ public class EventSourceTestCustomReconciler TestExecutionInfoProvider { public static final String FINALIZER_NAME = - ReconcilerUtils.getDefaultFinalizerName( - CustomResource.getCRDName(EventSourceTestCustomResource.class)); + ReconcilerUtils.getDefaultFinalizerName(EventSourceTestCustomResource.class); public static final int TIMER_PERIOD = 500; private final AtomicInteger numberOfExecutions = new AtomicInteger(0); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java index df26745def..a76b5b0113 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/retry/RetryTestCustomReconciler.java @@ -5,7 +5,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; @@ -18,8 +17,7 @@ public class RetryTestCustomReconciler implements Reconciler, TestExecutionInfoProvider { public static final String FINALIZER_NAME = - ReconcilerUtils.getDefaultFinalizerName( - CustomResource.getCRDName(RetryTestCustomResource.class)); + ReconcilerUtils.getDefaultFinalizerName(RetryTestCustomResource.class); private static final Logger log = LoggerFactory.getLogger(RetryTestCustomReconciler.class); private final AtomicInteger numberOfExecutions = new AtomicInteger(0); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java index bd32ad2bd3..94a7d36868 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/simple/TestReconciler.java @@ -10,7 +10,6 @@ import io.fabric8.kubernetes.api.model.ConfigMap; import io.fabric8.kubernetes.api.model.ConfigMapBuilder; import io.fabric8.kubernetes.api.model.ObjectMetaBuilder; -import io.fabric8.kubernetes.client.CustomResource; import io.fabric8.kubernetes.client.KubernetesClient; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.*; @@ -26,7 +25,7 @@ public class TestReconciler private static final Logger log = LoggerFactory.getLogger(TestReconciler.class); public static final String FINALIZER_NAME = - ReconcilerUtils.getDefaultFinalizerName(CustomResource.getCRDName(TestCustomResource.class)); + ReconcilerUtils.getDefaultFinalizerName(TestCustomResource.class); private final AtomicInteger numberOfExecutions = new AtomicInteger(0); private KubernetesClient kubernetesClient; diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java index 431374392f..081100beb3 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/subresource/SubResourceTestCustomReconciler.java @@ -5,7 +5,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import io.fabric8.kubernetes.client.CustomResource; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.reconciler.Context; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; @@ -18,8 +17,7 @@ public class SubResourceTestCustomReconciler implements Reconciler, TestExecutionInfoProvider { public static final String FINALIZER_NAME = - ReconcilerUtils.getDefaultFinalizerName( - CustomResource.getCRDName(SubResourceTestCustomResource.class)); + ReconcilerUtils.getDefaultFinalizerName(SubResourceTestCustomResource.class); private static final Logger log = LoggerFactory.getLogger(SubResourceTestCustomReconciler.class); private final AtomicInteger numberOfExecutions = new AtomicInteger(0); From 578282ea314d12e5b309ddecb6e9fd6ba6482b48 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Wed, 12 Jan 2022 14:11:10 +0100 Subject: [PATCH 3/3] fix: NO_FINALIZER constant is a valid finalizer for our purpose --- .../operator/config/runtime/AnnotationConfiguration.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java index 1000cb61c4..a02db55e22 100644 --- a/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java +++ b/operator-framework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java @@ -6,6 +6,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; import io.javaoperatorsdk.operator.ReconcilerUtils; import io.javaoperatorsdk.operator.api.config.ConfigurationService; +import io.javaoperatorsdk.operator.api.reconciler.Constants; import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter; @@ -34,7 +35,7 @@ public String getFinalizer() { return ReconcilerUtils.getDefaultFinalizerName(getResourceClass()); } else { final var finalizer = annotation.finalizerName(); - if (ReconcilerUtils.isFinalizerValid(finalizer)) { + if (Constants.NO_FINALIZER.equals(finalizer) || ReconcilerUtils.isFinalizerValid(finalizer)) { return finalizer; } else { throw new IllegalArgumentException(finalizer