From 67993de79856850092931b6a081c0ea7cdf700ff Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Fri, 10 Jan 2025 11:48:20 +0100 Subject: [PATCH] fix: ensure manually defined CRDs are considered before generated ones (#2662) * fix: ensure manually defined CRDs are considered before generated ones Fixes #2658 Signed-off-by: Chris Laprun * fix: output more details in logs on errors / failures Signed-off-by: Chris Laprun * wip: add more logging Signed-off-by: Chris Laprun * fix: add missing file Signed-off-by: Chris Laprun * fix: make sure generated CRDs are applied Signed-off-by: Chris Laprun --------- Signed-off-by: Chris Laprun --- .../junit/LocallyRunOperatorExtension.java | 142 ++++++++++-------- .../src/test/crd/test.crd | 21 +++ .../LocallyRunOperatorExtensionTest.java | 26 ++++ .../src/test/resources/crd/test.crd | 19 +++ .../src/test/resources/log4j2.xml | 16 ++ operator-framework/src/test/crd/test.crd | 19 +++ .../operator/CRDMappingInTestExtensionIT.java | 14 +- .../src/test/resources/crd/test.crd | 4 +- pom.xml | 8 + 9 files changed, 201 insertions(+), 68 deletions(-) create mode 100644 operator-framework-junit5/src/test/crd/test.crd create mode 100644 operator-framework-junit5/src/test/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtensionTest.java create mode 100644 operator-framework-junit5/src/test/resources/crd/test.crd create mode 100644 operator-framework-junit5/src/test/resources/log4j2.xml create mode 100644 operator-framework/src/test/crd/test.crd diff --git a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java index c36a16cd38..a2c92bf48b 100644 --- a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java +++ b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java @@ -2,10 +2,11 @@ import java.io.ByteArrayInputStream; import java.io.FileInputStream; -import java.io.FileNotFoundException; import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; import java.time.Duration; import java.util.ArrayList; import java.util.HashMap; @@ -13,7 +14,6 @@ import java.util.Map; import java.util.function.Consumer; import java.util.function.Function; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.junit.jupiter.api.extension.ExtensionContext; @@ -47,7 +47,7 @@ public class LocallyRunOperatorExtension extends AbstractOperatorExtension { private final List localPortForwards; private final List> additionalCustomResourceDefinitions; private final Map registeredControllers; - private final List additionalCrds; + private final Map crdMappings; private LocallyRunOperatorExtension( List reconcilers, @@ -82,7 +82,24 @@ private LocallyRunOperatorExtension( : overrider -> overrider.withKubernetesClient(kubernetesClient); this.operator = new Operator(configurationServiceOverrider); this.registeredControllers = new HashMap<>(); - this.additionalCrds = additionalCrds; + crdMappings = getAdditionalCRDsFromFiles(additionalCrds, getKubernetesClient()); + } + + static Map getAdditionalCRDsFromFiles(Iterable additionalCrds, + KubernetesClient client) { + Map crdMappings = new HashMap<>(); + additionalCrds.forEach(p -> { + try (InputStream is = new FileInputStream(p)) { + client.load(is).items().stream() + // only consider CRDs to avoid applying random resources to the cluster + .filter(CustomResourceDefinition.class::isInstance) + .map(CustomResourceDefinition.class::cast) + .forEach(crd -> crdMappings.put(crd.getMetadata().getName(), p)); + } catch (Exception e) { + throw new RuntimeException("Couldn't load CRD at " + p, e); + } + }); + return crdMappings; } /** @@ -112,25 +129,18 @@ public static void applyCrd(Class resourceClass, Kubernet public static void applyCrd(String resourceTypeName, KubernetesClient client) { String path = "/META-INF/fabric8/" + resourceTypeName + "-v1.yml"; try (InputStream is = LocallyRunOperatorExtension.class.getResourceAsStream(path)) { - applyCrd(is, path, client); - } catch (IllegalStateException e) { - // rethrow directly - throw e; + if (is == null) { + throw new IllegalStateException("Cannot find CRD at " + path); + } + var crdString = new String(is.readAllBytes(), StandardCharsets.UTF_8); + applyCrd(crdString, path, client); } catch (IOException e) { throw new IllegalStateException("Cannot apply CRD yaml: " + path, e); } } - public static void applyCrd(CustomResourceDefinition crd, KubernetesClient client) { - client.resource(crd).serverSideApply(); - } - - private static void applyCrd(InputStream is, String path, KubernetesClient client) { + private static void applyCrd(String crdString, String path, KubernetesClient client) { try { - if (is == null) { - throw new IllegalStateException("Cannot find CRD at " + path); - } - var crdString = new String(is.readAllBytes(), StandardCharsets.UTF_8); LOGGER.debug("Applying CRD: {}", crdString); final var crd = client.load(new ByteArrayInputStream(crdString.getBytes())); crd.serverSideApply(); @@ -144,14 +154,42 @@ private static void applyCrd(InputStream is, String path, KubernetesClient clien } } - public static List parseCrds(String path, KubernetesClient client) { - try (InputStream is = new FileInputStream(path)) { - return client.load(new ByteArrayInputStream(is.readAllBytes())) - .items().stream().map(i -> (CustomResourceDefinition) i).collect(Collectors.toList()); - } catch (FileNotFoundException e) { - throw new RuntimeException(e); - } catch (IOException e) { - throw new RuntimeException(e); + /** + * Applies the CRD associated with the specified custom resource, first checking if a CRD has been + * manually specified using {@link Builder#withAdditionalCRD}, otherwise assuming that its CRD + * should be found in the standard location as explained in + * {@link LocallyRunOperatorExtension#applyCrd(String, KubernetesClient)} + * + * @param crClass the custom resource class for which we want to apply the CRD + */ + public void applyCrd(Class crClass) { + applyCrd(ReconcilerUtils.getResourceTypeName(crClass)); + } + + /** + * Applies the CRD associated with the specified resource type name, first checking if a CRD has + * been manually specified using {@link Builder#withAdditionalCRD}, otherwise assuming that its + * CRD should be found in the standard location as explained in + * {@link LocallyRunOperatorExtension#applyCrd(String, KubernetesClient)} + * + * @param resourceTypeName the resource type name associated with the CRD to be applied, + * typically, given a resource type, its name would be obtained using + * {@link ReconcilerUtils#getResourceTypeName(Class)} + */ + public void applyCrd(String resourceTypeName) { + // first attempt to use a manually defined CRD + final var pathAsString = crdMappings.get(resourceTypeName); + if (pathAsString != null) { + final var path = Path.of(pathAsString); + try { + applyCrd(Files.readString(path), pathAsString, getKubernetesClient()); + } catch (IOException e) { + throw new IllegalStateException("Cannot open CRD file at " + path.toAbsolutePath(), e); + } + crdMappings.remove(resourceTypeName); + } else { + // if no manually defined CRD matches the resource type, apply the generated one + applyCrd(resourceTypeName, getKubernetesClient()); } } @@ -160,7 +198,7 @@ private Stream reconcilers() { } public List getReconcilers() { - return reconcilers().collect(Collectors.toUnmodifiableList()); + return reconcilers().toList(); } public Reconciler getFirstReconciler() { @@ -207,7 +245,6 @@ protected void before(ExtensionContext context) { } additionalCustomResourceDefinitions.forEach(this::applyCrd); - Map unappliedCRDs = getAdditionalCRDsFromFiles(); for (var ref : reconcilers) { final var config = operator.getConfigurationService().getConfigurationFor(ref.reconciler); final var oconfig = override(config); @@ -227,49 +264,28 @@ protected void before(ExtensionContext context) { final var resourceTypeName = ReconcilerUtils.getResourceTypeName(resourceClass); // only try to apply a CRD for the reconciler if it is associated to a CR if (CustomResource.class.isAssignableFrom(resourceClass)) { - if (unappliedCRDs.get(resourceTypeName) != null) { - applyCrd(resourceTypeName); - unappliedCRDs.remove(resourceTypeName); - } else { - applyCrd(resourceClass); - } + applyCrd(resourceTypeName); } // apply yet unapplied CRDs var registeredController = this.operator.register(ref.reconciler, oconfig.build()); registeredControllers.put(ref.reconciler, registeredController); } - unappliedCRDs.keySet().forEach(this::applyCrd); + crdMappings.forEach((crdName, path) -> { + final String crdString; + try { + crdString = Files.readString(Path.of(path)); + } catch (IOException e) { + throw new IllegalArgumentException("Couldn't read CRD located at " + path, e); + } + applyCrd(crdString, path, getKubernetesClient()); + }); + crdMappings.clear(); LOGGER.debug("Starting the operator locally"); this.operator.start(); } - private Map getAdditionalCRDsFromFiles() { - Map crdMappings = new HashMap<>(); - additionalCrds.forEach(p -> { - var crds = parseCrds(p, getKubernetesClient()); - crds.forEach(c -> crdMappings.put(c.getMetadata().getName(), c)); - }); - return crdMappings; - } - - /** - * Applies the CRD associated with the specified custom resource, first checking if a CRD has been - * manually specified using {@link Builder#withAdditionalCRD(String)}, otherwise assuming that its - * CRD should be found in the standard location as explained in - * {@link LocallyRunOperatorExtension#applyCrd(String, KubernetesClient)} - * - * @param crClass the custom resource class for which we want to apply the CRD - */ - public void applyCrd(Class crClass) { - applyCrd(ReconcilerUtils.getResourceTypeName(crClass)); - } - - public void applyCrd(String resourceTypeName) { - applyCrd(resourceTypeName, getKubernetesClient()); - } - @Override protected void after(ExtensionContext context) { super.after(context); @@ -295,7 +311,6 @@ public static class Builder extends AbstractBuilder { private final List reconcilers; private final List portForwards; private final List> additionalCustomResourceDefinitions; - private final Map crdMappings; private final List additionalCRDs = new ArrayList<>(); private KubernetesClient kubernetesClient; @@ -304,7 +319,6 @@ protected Builder() { this.reconcilers = new ArrayList<>(); this.portForwards = new ArrayList<>(); this.additionalCustomResourceDefinitions = new ArrayList<>(); - this.crdMappings = new HashMap<>(); } public Builder withReconciler( @@ -359,8 +373,10 @@ public Builder withAdditionalCustomResourceDefinition( return this; } - public Builder withAdditionalCRD(String path) { - additionalCRDs.add(path); + public Builder withAdditionalCRD(String... paths) { + if (paths != null) { + additionalCRDs.addAll(List.of(paths)); + } return this; } diff --git a/operator-framework-junit5/src/test/crd/test.crd b/operator-framework-junit5/src/test/crd/test.crd new file mode 100644 index 0000000000..0d509f04ee --- /dev/null +++ b/operator-framework-junit5/src/test/crd/test.crd @@ -0,0 +1,21 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: externals.crd.example +spec: + group: crd.example + names: + kind: External + singular: external + plural: externals + scope: Namespaced + versions: + - name: v1 + schema: + openAPIV3Schema: + properties: + foo: + type: "string" + type: "object" + served: true + storage: true diff --git a/operator-framework-junit5/src/test/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtensionTest.java b/operator-framework-junit5/src/test/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtensionTest.java new file mode 100644 index 0000000000..e5b57fa173 --- /dev/null +++ b/operator-framework-junit5/src/test/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtensionTest.java @@ -0,0 +1,26 @@ +package io.javaoperatorsdk.operator.junit; + +import java.nio.file.Path; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import io.fabric8.kubernetes.client.KubernetesClientBuilder; + +import static org.junit.jupiter.api.Assertions.*; + +class LocallyRunOperatorExtensionTest { + + @Test + void getAdditionalCRDsFromFiles() { + System.out.println(Path.of("").toAbsolutePath()); + System.out.println(Path.of("src/test/crd/test.crd").toAbsolutePath()); + final var crds = LocallyRunOperatorExtension.getAdditionalCRDsFromFiles( + List.of("src/test/resources/crd/test.crd", "src/test/crd/test.crd"), + new KubernetesClientBuilder().build()); + assertNotNull(crds); + assertEquals(2, crds.size()); + assertEquals("src/test/crd/test.crd", crds.get("externals.crd.example")); + assertEquals("src/test/resources/crd/test.crd", crds.get("tests.crd.example")); + } +} diff --git a/operator-framework-junit5/src/test/resources/crd/test.crd b/operator-framework-junit5/src/test/resources/crd/test.crd new file mode 100644 index 0000000000..f0891454fe --- /dev/null +++ b/operator-framework-junit5/src/test/resources/crd/test.crd @@ -0,0 +1,19 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: tests.crd.example +spec: + group: crd.example + names: + kind: Test + singular: test + plural: tests + scope: Namespaced + versions: + - name: v1 + schema: + openAPIV3Schema: + properties: + type: "object" + served: true + storage: true \ No newline at end of file diff --git a/operator-framework-junit5/src/test/resources/log4j2.xml b/operator-framework-junit5/src/test/resources/log4j2.xml new file mode 100644 index 0000000000..82d8fa2cb1 --- /dev/null +++ b/operator-framework-junit5/src/test/resources/log4j2.xml @@ -0,0 +1,16 @@ + + + + + + + + + + + + + + + + diff --git a/operator-framework/src/test/crd/test.crd b/operator-framework/src/test/crd/test.crd new file mode 100644 index 0000000000..ac2f5d31b1 --- /dev/null +++ b/operator-framework/src/test/crd/test.crd @@ -0,0 +1,19 @@ +apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: externals.crd.example +spec: + group: crd.example + names: + kind: External + singular: external + plural: externals + scope: Namespaced + versions: + - name: v1 + schema: + openAPIV3Schema: + properties: + type: "object" + served: true + storage: true diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDMappingInTestExtensionIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDMappingInTestExtensionIT.java index 09b52f1078..e0535381fa 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDMappingInTestExtensionIT.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/CRDMappingInTestExtensionIT.java @@ -28,16 +28,22 @@ public class CRDMappingInTestExtensionIT { LocallyRunOperatorExtension operator = LocallyRunOperatorExtension.builder() .withReconciler(new TestReconciler()) - .withAdditionalCRD("src/test/resources/crd/test.crd") + .withAdditionalCRD("src/test/resources/crd/test.crd", "src/test/crd/test.crd") .build(); @Test void correctlyAppliesManuallySpecifiedCRD() { - operator.applyCrd(TestCR.class); - final var crdClient = client.apiextensions().v1().customResourceDefinitions(); await().pollDelay(Duration.ofMillis(150)) - .untilAsserted(() -> assertThat(crdClient.withName("tests.crd.example").get()).isNotNull()); + .untilAsserted(() -> { + final var actual = crdClient.withName("tests.crd.example").get(); + assertThat(actual).isNotNull(); + assertThat(actual.getSpec().getVersions().get(0).getSchema().getOpenAPIV3Schema() + .getProperties().containsKey("foo")).isTrue(); + }); + await().pollDelay(Duration.ofMillis(150)) + .untilAsserted( + () -> assertThat(crdClient.withName("externals.crd.example").get()).isNotNull()); } @Group("crd.example") diff --git a/operator-framework/src/test/resources/crd/test.crd b/operator-framework/src/test/resources/crd/test.crd index f0891454fe..f10e5b3225 100644 --- a/operator-framework/src/test/resources/crd/test.crd +++ b/operator-framework/src/test/resources/crd/test.crd @@ -14,6 +14,8 @@ spec: schema: openAPIV3Schema: properties: + foo: + type: "string" type: "object" served: true - storage: true \ No newline at end of file + storage: true diff --git a/pom.xml b/pom.xml index dc9cd2ed9f..7363be62ed 100644 --- a/pom.xml +++ b/pom.xml @@ -272,6 +272,14 @@ UNICODE + true + true + true + true + false + true + true + false