fix: avoid NPE is given spec is null (#1899)

Also optimizes case where resource is a CustomResource. Fixes #1897
operator-framework · May 11, 2023 · 21c8dac · 21c8dac
1 parent 55629a0
commit 21c8dac
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 22 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
@@ -24,6 +24,10 @@ public class ReconcilerUtils {
 
   private static final String FINALIZER_NAME_SUFFIX = "/finalizer";
   protected static final String MISSING_GROUP_SUFFIX = ".javaoperatorsdk.io";
+  private static final String GET_SPEC = "getSpec";
+  private static final String SET_SPEC = "setSpec";
+  private static final Pattern API_URI_PATTERN =
+      Pattern.compile(".*http(s?)://[^/]*/api(s?)/(\\S*).*"); // NOSONAR: input is controlled
 
   // prevent instantiation of util class
   private ReconcilerUtils() {}
@@ -94,25 +98,55 @@ public static boolean specsEqual(HasMetadata r1, HasMetadata r2) {
 
   // will be replaced with: https://github.com/fabric8io/kubernetes-client/issues/3816
   public static Object getSpec(HasMetadata resource) {
+    // optimize CustomResource case
+    if (resource instanceof CustomResource) {
+      CustomResource cr = (CustomResource) resource;
+      return cr.getSpec();
+    }
+
     try {
-      Method getSpecMethod = resource.getClass().getMethod("getSpec");
+      Method getSpecMethod = resource.getClass().getMethod(GET_SPEC);
       return getSpecMethod.invoke(resource);
     } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
-      throw new IllegalStateException("No spec found on resource", e);
+      throw noSpecException(resource, e);
     }
   }
 
+  @SuppressWarnings("unchecked")
   public static Object setSpec(HasMetadata resource, Object spec) {
+    // optimize CustomResource case
+    if (resource instanceof CustomResource) {
+      CustomResource cr = (CustomResource) resource;
+      cr.setSpec(spec);
+      return null;
+    }
+
     try {
       Class<? extends HasMetadata> resourceClass = resource.getClass();
-      Method setSpecMethod =
-          resource.getClass().getMethod("setSpec", getSpecClass(resourceClass, spec));
+
+      // if given spec is null, find the method just using its name
+      Method setSpecMethod;
+      if (spec != null) {
+        setSpecMethod = resourceClass.getMethod(SET_SPEC, spec.getClass());
+      } else {
+        setSpecMethod = Arrays.stream(resourceClass.getMethods())
+            .filter(method -> SET_SPEC.equals(method.getName()))
+            .findFirst()
+            .orElseThrow(() -> noSpecException(resource, null));
+      }
+
       return setSpecMethod.invoke(resource, spec);
     } catch (NoSuchMethodException | InvocationTargetException | IllegalAccessException e) {
-      throw new IllegalStateException("No spec found on resource", e);
+      throw noSpecException(resource, e);
     }
   }
 
+  private static IllegalStateException noSpecException(HasMetadata resource,
+      ReflectiveOperationException e) {
+    return new IllegalStateException("No spec found on resource " + resource.getClass().getName(),
+        e);
+  }
+
   public static <T> T loadYaml(Class<T> clazz, Class loader, String yaml) {
     try (InputStream is = loader.getResourceAsStream(yaml)) {
       if (Builder.class.isAssignableFrom(clazz)) {
@@ -149,9 +183,7 @@ private static boolean matchesResourceType(String resourceTypeName,
     } else {
       // extract matching information from URI in the message if available
       final var message = exception.getMessage();
-      final var regex = Pattern
-          .compile(".*http(s?)://[^/]*/api(s?)/(\\S*).*") // NOSONAR: input is controlled
-          .matcher(message);
+      final var regex = API_URI_PATTERN.matcher(message);
       if (regex.matches()) {
         var group = regex.group(3);
         if (group.endsWith(".")) {
@@ -168,14 +200,4 @@ private static boolean matchesResourceType(String resourceTypeName,
     }
     return false;
   }
-
-  // CustomResouce has a parameterized parameter type
-  private static Class getSpecClass(Class<? extends HasMetadata> resourceClass, Object spec) {
-    if (CustomResource.class.isAssignableFrom(resourceClass)) {
-      return Object.class;
-    } else {
-      return spec.getClass();
-    }
-  }
-
 }
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
@@ -6,6 +6,7 @@
 
 import io.fabric8.kubernetes.api.model.ContainerBuilder;
 import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.Namespace;
 import io.fabric8.kubernetes.api.model.Namespaced;
 import io.fabric8.kubernetes.api.model.Pod;
 import io.fabric8.kubernetes.api.model.PodSpec;
@@ -85,6 +86,16 @@ void getsSpecWithReflection() {
     assertThat(spec.getReplicas()).isEqualTo(5);
   }
 
+  @Test
+  void properlyHandlesNullSpec() {
+    Namespace ns = new Namespace();
+
+    final var spec = ReconcilerUtils.getSpec(ns);
+    assertThat(spec).isNull();
+
+    ReconcilerUtils.setSpec(ns, null);
+  }
+
   @Test
   void setsSpecWithReflection() {
     Deployment deployment = new Deployment();

diff --git a/...orsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java b/...orsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java
@@ -1,10 +1,14 @@
 package io.javaoperatorsdk.operator.processing.dependent.kubernetes;
 
 import java.util.HashMap;
+import java.util.List;
 
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
+import io.fabric8.kubernetes.api.model.Namespace;
+import io.fabric8.kubernetes.api.model.NamespaceBuilder;
+import io.fabric8.kubernetes.api.model.NamespaceSpec;
 import io.fabric8.kubernetes.api.model.apps.Deployment;
 import io.javaoperatorsdk.operator.ReconcilerUtils;
 import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
@@ -25,12 +29,9 @@ static void setUp() {
     when(context.getControllerConfiguration()).thenReturn(controllerConfiguration);
   }
 
-  ResourceUpdatePreProcessor<Deployment> processor =
-      GenericResourceUpdatePreProcessor.processorFor(Deployment.class);
-
-
   @Test
   void preservesValues() {
+    var processor = GenericResourceUpdatePreProcessor.processorFor(Deployment.class);
     var desired = createDeployment();
     var actual = createDeployment();
     actual.getMetadata().setLabels(new HashMap<>());
@@ -45,6 +46,34 @@ void preservesValues() {
     assertThat(result.getSpec().getRevisionHistoryLimit()).isEqualTo(10);
   }
 
+  @Test
+  void checkNamespaces() {
+    var processor = GenericResourceUpdatePreProcessor.processorFor(Namespace.class);
+    var desired = new NamespaceBuilder().withNewMetadata().withName("foo").endMetadata().build();
+    var actual = new NamespaceBuilder().withNewMetadata().withName("foo").endMetadata().build();
+    actual.getMetadata().setLabels(new HashMap<>());
+    actual.getMetadata().getLabels().put("additionalActualKey", "value");
+    actual.getMetadata().setResourceVersion("1234");
+
+    var result = processor.replaceSpecOnActual(actual, desired, context);
+    assertThat(result.getMetadata().getLabels().get("additionalActualKey")).isEqualTo("value");
+    assertThat(result.getMetadata().getResourceVersion()).isEqualTo("1234");
+
+    desired.setSpec(new NamespaceSpec(List.of("halkyon.io/finalizer")));
+
+    result = processor.replaceSpecOnActual(actual, desired, context);
+    assertThat(result.getMetadata().getLabels().get("additionalActualKey")).isEqualTo("value");
+    assertThat(result.getMetadata().getResourceVersion()).isEqualTo("1234");
+    assertThat(result.getSpec().getFinalizers()).containsExactly("halkyon.io/finalizer");
+
+    desired = new NamespaceBuilder().withNewMetadata().withName("foo").endMetadata().build();
+
+    result = processor.replaceSpecOnActual(actual, desired, context);
+    assertThat(result.getMetadata().getLabels().get("additionalActualKey")).isEqualTo("value");
+    assertThat(result.getMetadata().getResourceVersion()).isEqualTo("1234");
+    assertThat(result.getSpec()).isNull();
+  }
+
   Deployment createDeployment() {
     return ReconcilerUtils.loadYaml(
         Deployment.class, GenericResourceUpdatePreProcessorTest.class, "nginx-deployment.yaml");