From 76ad9ba3bc2daf046c77c153533788db130e1dc5 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 11 May 2023 11:26:19 +0200 Subject: [PATCH 1/4] fix: avoid NPE is given spec is null Also optimizes case where resource is a CustomResource. Fixes #1897 --- .../operator/ReconcilerUtils.java | 57 +++++++++++++------ .../operator/ReconcilerUtilsTest.java | 11 ++++ 2 files changed, 51 insertions(+), 17 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 53e9122fb2..e5ccc30395 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 @@ -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*).*"); // 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 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 loadYaml(Class clazz, Class loader, String yaml) { try (InputStream is = loader.getResourceAsStream(yaml)) { if (Builder.class.isAssignableFrom(clazz)) { @@ -149,8 +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 + final var regex = API_URI_PATTERN // NOSONAR: input is controlled .matcher(message); if (regex.matches()) { var group = regex.group(3); @@ -168,14 +201,4 @@ private static boolean matchesResourceType(String resourceTypeName, } return false; } - - // CustomResouce has a parameterized parameter type - private static Class getSpecClass(Class 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 index 25bdcf205f..2b4d89ae06 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 @@ -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(); From b7af97001c18ead3eb5c0919ba4d294a225de1e0 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 11 May 2023 13:57:51 +0200 Subject: [PATCH 2/4] fix: avoid Sonar error --- .../java/io/javaoperatorsdk/operator/ReconcilerUtils.java | 5 ++--- 1 file changed, 2 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 e5ccc30395..a30c335fb0 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 @@ -27,7 +27,7 @@ public class ReconcilerUtils { 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*).*"); + Pattern.compile(".*http(s?)://[^/]*/api(s?)/(\\S*).*"); // NOSONAR: input is controlled // prevent instantiation of util class private ReconcilerUtils() {} @@ -183,8 +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 = API_URI_PATTERN // 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(".")) { From 63e2e78b386dcc6310f2049bf805dc3a00742420 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 11 May 2023 13:58:30 +0200 Subject: [PATCH 3/4] chore: add higher-level test --- ...GenericResourceUpdatePreProcessorTest.java | 39 ++++++++++++++++--- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java index e67d579adc..24e1d640cf 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/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 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,9 +46,37 @@ 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"); } -} +} \ No newline at end of file From 198945b10b4b4d547691a1f4dadc134fbbb28180 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Thu, 11 May 2023 14:17:52 +0200 Subject: [PATCH 4/4] fix: format --- .../kubernetes/GenericResourceUpdatePreProcessorTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java index 24e1d640cf..9f1856edbc 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericResourceUpdatePreProcessorTest.java @@ -79,4 +79,4 @@ Deployment createDeployment() { Deployment.class, GenericResourceUpdatePreProcessorTest.class, "nginx-deployment.yaml"); } -} \ No newline at end of file +}