fix: generate proper resource name & finalizer for group-less resources (#814)

metacosm · web-flow · commit fdefd56ceb6d · 2022-01-12T16:34:26.000+01:00
Fixes #812
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
@@ -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;
@@ -10,9 +12,50 @@
 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;
+  // 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<? extends HasMetadata> 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<? extends HasMetadata> 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) {
+      resourceName = resourceName + MISSING_GROUP_SUFFIX;
+    }
+    return resourceName + FINALIZER_NAME_SUFFIX;
   }
 
   public static String getNameFor(Class<? extends Reconciler> 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
@@ -17,11 +17,11 @@ default String getName() {
   }
 
   default String getResourceTypeName() {
-    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
@@ -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
@@ -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;
@@ -31,9 +32,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 (Constants.NO_FINALIZER.equals(finalizer) || 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 +100,7 @@ public ResourceEventFilter<R> 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
@@ -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,29 +73,29 @@ 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);
     assertEquals(CUSTOM_FINALIZER_NAME, configuration.getFinalizer());
   }
 
   @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
@@ -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
@@ -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<RetryTestCustomResource>, 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
@@ -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
@@ -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<SubResourceTestCustomResource>, 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);

Original file line number	Diff line number	Diff line change
`@@ -17,11 +17,11 @@ default String getName() {`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`default String getResourceTypeName() {`
`20`		`- return HasMetadata.getFullResourceName(getResourceClass());`
	`20`	`+ return ReconcilerUtils.getResourceTypeName(getResourceClass());`
`21`	`21`	`}`
`22`	`22`
`23`	`23`	`default String getFinalizer() {`
`24`		`- return ReconcilerUtils.getDefaultFinalizerName(getResourceTypeName());`
	`24`	`+ return ReconcilerUtils.getDefaultFinalizerName(getResourceClass());`
`25`	`25`	`}`
`26`	`26`
`27`	`27`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -6,6 +6,7 @@`
`6`	`6`	`import io.fabric8.kubernetes.api.model.HasMetadata;`
`7`	`7`	`import io.javaoperatorsdk.operator.ReconcilerUtils;`
`8`	`8`	`import io.javaoperatorsdk.operator.api.config.ConfigurationService;`
	`9`	`+import io.javaoperatorsdk.operator.api.reconciler.Constants;`
`9`	`10`	`import io.javaoperatorsdk.operator.api.reconciler.ControllerConfiguration;`
`10`	`11`	`import io.javaoperatorsdk.operator.api.reconciler.Reconciler;`
`11`	`12`	`import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;`
`@@ -31,9 +32,15 @@ public String getName() {`
`31`	`32`	`@Override`
`32`	`33`	`public String getFinalizer() {`
`33`	`34`	`if (annotation == null \|\| annotation.finalizerName().isBlank()) {`
`34`		`- return ReconcilerUtils.getDefaultFinalizerName(getResourceTypeName());`
	`35`	`+ return ReconcilerUtils.getDefaultFinalizerName(getResourceClass());`
`35`	`36`	`} else {`
`36`		`- return annotation.finalizerName();`
	`37`	`+ final var finalizer = annotation.finalizerName();`
	`38`	`+ if (Constants.NO_FINALIZER.equals(finalizer) \|\| ReconcilerUtils.isFinalizerValid(finalizer)) {`
	`39`	`+ return finalizer;`
	`40`	`+ } else {`
	`41`	`+ throw new IllegalArgumentException(finalizer`
	`42`	`+ + " is not a valid finalizer. See https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#finalizers for details");`
	`43`	`+ }`
`37`	`44`	`}`
`38`	`45`	`}`
`39`	`46`
`@@ -93,7 +100,7 @@ public ResourceEventFilter<R> getEventFilter() {`
`93`	`100`	`answer = answer.and(filter);`
`94`	`101`	`}`
`95`	`102`	`} catch (Exception e) {`
`96`		`- throw new RuntimeException(e);`
	`103`	`+ throw new IllegalArgumentException(e);`
`97`	`104`	`}`
`98`	`105`	`}`
`99`	`106`	`}`