From 805dc0aa58e18b2885120bcf5e6baf2e371b8496 Mon Sep 17 00:00:00 2001 From: Chris Laprun Date: Tue, 8 Oct 2024 13:37:21 +0200 Subject: [PATCH] fix: record version with CRD name to properly output multiple versions (#973) Also fixes an issue with improperly recorded reconciler name and handling of generic kubernetes resources. Fixes #886 Signed-off-by: Chris Laprun --- .../operatorsdk/common/Constants.java | 3 +++ .../CustomResourceAugmentedClassInfo.java | 4 +--- .../DependentResourceAugmentedClassInfo.java | 12 +++++++----- .../common/ReconciledAugmentedClassInfo.java | 16 +++++++++++----- .../ReconciledResourceAugmentedClassInfo.java | 19 +++++++++++++++++++ .../common/ReconcilerAugmentedClassInfo.java | 2 +- .../ResourceAssociatedAugmentedClassInfo.java | 9 ++++++++- .../operatorsdk/deployment/CRDGeneration.java | 16 +++++++--------- .../deployment/CRDGenerationBuildStep.java | 13 ++++++------- .../deployment/ResourceControllerMapping.java | 10 +++++----- .../test/AllCRDGenerationTest.java | 4 ++-- 11 files changed, 70 insertions(+), 38 deletions(-) diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/Constants.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/Constants.java index 18fec24f..a0ee10f9 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/Constants.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/Constants.java @@ -10,6 +10,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Ignore; import io.javaoperatorsdk.operator.api.reconciler.Reconciler; import io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource; +import io.javaoperatorsdk.operator.processing.dependent.kubernetes.GenericKubernetesDependentResource; import io.quarkiverse.operatorsdk.annotations.AdditionalRBACRoleRefs; import io.quarkiverse.operatorsdk.annotations.AdditionalRBACRules; import io.quarkiverse.operatorsdk.annotations.RBACCRoleRef; @@ -25,6 +26,8 @@ private Constants() { public static final DotName HAS_METADATA = DotName.createSimple(HasMetadata.class.getName()); public static final DotName CONTROLLER_CONFIGURATION = DotName.createSimple(ControllerConfiguration.class.getName()); public static final DotName DEPENDENT_RESOURCE = DotName.createSimple(DependentResource.class.getName()); + public static final DotName GENERIC_KUBERNETES_DEPENDENT_RESOURCE = DotName + .createSimple(GenericKubernetesDependentResource.class.getName()); public static final DotName CONFIGURED = DotName.createSimple(Configured.class.getName()); public static final DotName ANNOTATION_CONFIGURABLE = DotName.createSimple(AnnotationConfigurable.class.getName()); public static final DotName OBJECT = DotName.createSimple(Object.class.getName()); diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/CustomResourceAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/CustomResourceAugmentedClassInfo.java index f0a31c86..3478f65f 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/CustomResourceAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/CustomResourceAugmentedClassInfo.java @@ -20,14 +20,12 @@ protected CustomResourceAugmentedClassInfo(ClassInfo classInfo, String associate @Override protected boolean doKeep(IndexView index, Logger log, Map context) { - // only keep the information if the associated CRD hasn't already been generated - final var fullName = fullResourceName(); return Optional.ofNullable(context.get(EXISTING_CRDS_KEY)) .map(value -> { @SuppressWarnings("unchecked") Set generated = (Set) value; - return !generated.contains(fullName); + return !generated.contains(id()); }) .orElse(true); } diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/DependentResourceAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/DependentResourceAugmentedClassInfo.java index 9fbf286f..75d771cb 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/DependentResourceAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/DependentResourceAugmentedClassInfo.java @@ -16,24 +16,26 @@ public class DependentResourceAugmentedClassInfo extends ResourceAssociatedAugme private final AnnotationInstance dependentAnnotationFromController; public DependentResourceAugmentedClassInfo(ClassInfo classInfo) { - this(classInfo, null); + this(classInfo, null, null); } - private DependentResourceAugmentedClassInfo(ClassInfo classInfo, AnnotationInstance dependentAnnotationFromController) { + private DependentResourceAugmentedClassInfo(ClassInfo classInfo, AnnotationInstance dependentAnnotationFromController, + String reconcilerName) { super(classInfo, DEPENDENT_RESOURCE, 2, Optional.ofNullable(dependentAnnotationFromController) .map(a -> a.value("name")) .map(AnnotationValue::asString) .filter(Predicate.not(String::isBlank)) // note that this should match DependentResource.getDefaultNameFor implementation) - .orElse(classInfo.name().toString())); + .orElse(classInfo.name().toString()), + reconcilerName); this.dependentAnnotationFromController = dependentAnnotationFromController; } public static DependentResourceAugmentedClassInfo createFor(ClassInfo classInfo, AnnotationInstance dependentAnnotationFromController, IndexView index, Logger log, - Map context) { - final var info = new DependentResourceAugmentedClassInfo(classInfo, dependentAnnotationFromController); + Map context, String reconcilerName) { + final var info = new DependentResourceAugmentedClassInfo(classInfo, dependentAnnotationFromController, reconcilerName); info.augmentIfKept(index, log, context); return info; } diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledAugmentedClassInfo.java index 2416594b..7887bc43 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledAugmentedClassInfo.java @@ -1,9 +1,7 @@ package io.quarkiverse.operatorsdk.common; import static io.quarkiverse.operatorsdk.common.ClassLoadingUtils.loadClass; -import static io.quarkiverse.operatorsdk.common.Constants.CUSTOM_RESOURCE; -import static io.quarkiverse.operatorsdk.common.Constants.HAS_METADATA; -import static io.quarkiverse.operatorsdk.common.Constants.OBJECT; +import static io.quarkiverse.operatorsdk.common.Constants.*; import java.util.Map; import java.util.Optional; @@ -65,14 +63,21 @@ public ReconciledResourceAugmentedClassInfo asResourceTargeting() { } @SuppressWarnings("rawtypes") - public static ReconciledAugmentedClassInfo createFor(ClassInfo resourceCI, String reconcilerName, + public static ReconciledAugmentedClassInfo createFor(ResourceAssociatedAugmentedClassInfo parent, ClassInfo resourceCI, + String reconcilerName, IndexView index, Logger log, Map context) { var isResource = false; var isCR = false; + var isGenericKubernetesResource = false; try { isResource = ClassUtils.isImplementationOf(index, resourceCI, HAS_METADATA); if (isResource) { isCR = JandexUtil.isSubclassOf(index, resourceCI, CUSTOM_RESOURCE); + if (!isCR) { + // check if the target resource is a generic one + isGenericKubernetesResource = JandexUtil.isSubclassOf(index, parent.classInfo(), + GENERIC_KUBERNETES_DEPENDENT_RESOURCE); + } } } catch (BuildException e) { log.errorv( @@ -83,7 +88,8 @@ public static ReconciledAugmentedClassInfo createFor(ClassInfo resourceCI, Strin ReconciledAugmentedClassInfo reconciledInfo; if (isCR) { reconciledInfo = new CustomResourceAugmentedClassInfo(resourceCI, reconcilerName); - } else if (isResource) { + } else if (isResource && !isGenericKubernetesResource) { + // only record detailed information if the target resource is not generic reconciledInfo = new ReconciledResourceAugmentedClassInfo<>(resourceCI, HAS_METADATA, 0, reconcilerName); } else { diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledResourceAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledResourceAugmentedClassInfo.java index 59148464..2754ea06 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledResourceAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconciledResourceAugmentedClassInfo.java @@ -1,6 +1,7 @@ package io.quarkiverse.operatorsdk.common; import java.util.Map; +import java.util.Objects; import org.jboss.jandex.ClassInfo; import org.jboss.jandex.DotName; @@ -13,12 +14,18 @@ public class ReconciledResourceAugmentedClassInfo extends public static final String STATUS = "status"; protected boolean hasStatus; + private final String id; protected ReconciledResourceAugmentedClassInfo(ClassInfo classInfo, DotName extendedOrImplementedClass, int expectedParameterTypesCardinality, String associatedReconcilerName) { super(classInfo, extendedOrImplementedClass, expectedParameterTypesCardinality, associatedReconcilerName); + id = fullResourceName() + version(); + } + + public String id() { + return id; } public String fullResourceName() { @@ -55,4 +62,16 @@ protected boolean hasStatus(IndexView index) { public boolean hasNonVoidStatus() { return hasStatus; } + + @Override + public boolean equals(Object o) { + if (!(o instanceof ReconciledResourceAugmentedClassInfo that)) + return false; + return Objects.equals(id, that.id); + } + + @Override + public int hashCode() { + return Objects.hashCode(id); + } } diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconcilerAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconcilerAugmentedClassInfo.java index 6f29133c..39f31b43 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconcilerAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ReconcilerAugmentedClassInfo.java @@ -61,7 +61,7 @@ protected void doAugment(IndexView index, Logger log, Map contex dependentConfig.value("type"), DependentResource.class, index); final var dependent = DependentResourceAugmentedClassInfo.createFor(dependentType, dependentConfig, index, - log, context); + log, context, nameOrFailIfUnset()); final var dependentName = dependent.nameOrFailIfUnset(); final var dependentTypeName = dependentType.name().toString(); if (dependentResources.containsKey(dependentName)) { diff --git a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ResourceAssociatedAugmentedClassInfo.java b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ResourceAssociatedAugmentedClassInfo.java index 093827df..026a41af 100644 --- a/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ResourceAssociatedAugmentedClassInfo.java +++ b/common-deployment/src/main/java/io/quarkiverse/operatorsdk/common/ResourceAssociatedAugmentedClassInfo.java @@ -13,11 +13,18 @@ public class ResourceAssociatedAugmentedClassInfo extends SelectiveAugmentedClassInfo { private final String name; private ReconciledAugmentedClassInfo resourceInfo; + private final String reconcilerName; protected ResourceAssociatedAugmentedClassInfo(ClassInfo classInfo, DotName extendedOrImplementedClass, int expectedParameterTypesCardinality, String name) { + this(classInfo, extendedOrImplementedClass, expectedParameterTypesCardinality, name, null); + } + + protected ResourceAssociatedAugmentedClassInfo(ClassInfo classInfo, + DotName extendedOrImplementedClass, int expectedParameterTypesCardinality, String name, String reconcilerName) { super(classInfo, extendedOrImplementedClass, expectedParameterTypesCardinality); this.name = name; + this.reconcilerName = reconcilerName != null ? reconcilerName : name; } public DotName resourceTypeName() { @@ -42,7 +49,7 @@ protected void doAugment(IndexView index, Logger log, Map contex + "' has not been found in the Jandex index so it cannot be introspected. Please index your classes with Jandex."); } - resourceInfo = ReconciledAugmentedClassInfo.createFor(primaryCI, name, index, log, context); + resourceInfo = ReconciledAugmentedClassInfo.createFor(this, primaryCI, reconcilerName, index, log, context); } public ReconciledAugmentedClassInfo associatedResourceInfo() { diff --git a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGeneration.java b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGeneration.java index 9cf6fdb7..f8b423e7 100644 --- a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGeneration.java +++ b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGeneration.java @@ -56,7 +56,7 @@ boolean shouldApply() { * Generates the CRD in the location specified by the output target, using the specified CRD * generation configuration only if generation has been requested by call * {@link #scheduleForGenerationIfNeeded(CustomResourceAugmentedClassInfo, Map, Set)} or - * {@link #withCustomResource(Class, String, String)} + * {@link #withCustomResource(Class, String)} * * @param outputTarget the {@link OutputTargetBuildItem} specifying where the CRDs * should be generated @@ -101,7 +101,7 @@ CRDGenerationInfo generate(OutputTargetBuildItem outputTarget, return new CRDGenerationInfo(shouldApply(), validateCustomResources, converted, generated); } - private boolean needsGeneration(Map existingCRDInfos, Set changedClassNames, String targetCRName) { + private boolean needsGeneration(Map existingCRDInfos, Set changedClassNames) { final boolean[] generateCurrent = { true }; // request CRD generation by default crdConfiguration.versions().forEach(v -> { final var crd = existingCRDInfos.get(v); @@ -122,7 +122,7 @@ private boolean needsGeneration(Map existingCRDInfos, Set existingCRDInfos, Set existingCRDInfos, Set changedClasses) { var scheduleCurrent = true; - final String targetCRName = crInfo.asResourceTargeting().fullResourceName(); if (existingCRDInfos != null && !existingCRDInfos.isEmpty()) { - scheduleCurrent = needsGeneration(existingCRDInfos, changedClasses, targetCRName); + scheduleCurrent = needsGeneration(existingCRDInfos, changedClasses); } if (scheduleCurrent) { - withCustomResource(crInfo.loadAssociatedClass(), targetCRName, crInfo.getAssociatedReconcilerName().orElse(null)); + withCustomResource(crInfo.loadAssociatedClass(), crInfo.getAssociatedReconcilerName().orElse(null)); } return scheduleCurrent; } - public void withCustomResource(Class> crClass, String crdName, - String associatedControllerName) { + public void withCustomResource(Class> crClass, String associatedControllerName) { // first check if the CR is not filtered out if (crdConfiguration.excludeResources().map(excluded -> excluded.contains(crClass.getName())).orElse(false)) { log.infov("CRD generation was skipped for ''{0}'' because it was excluded from generation", crClass.getName()); @@ -159,7 +157,7 @@ public void withCustomResource(Class> crClass, St generator = new CRDGenerator().withParallelGenerationEnabled(crdConfiguration.generateInParallel()); } final var info = CustomResourceInfo.fromClass(crClass); - crMappings.add(info, crdName, associatedControllerName); + crMappings.add(info, associatedControllerName); generator.customResources(info); needGeneration = true; } catch (Exception e) { diff --git a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGenerationBuildStep.java b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGenerationBuildStep.java index 1e5082ec..6c3cdd99 100644 --- a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGenerationBuildStep.java +++ b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/CRDGenerationBuildStep.java @@ -52,22 +52,22 @@ GeneratedCRDInfoBuildItem generateCRDs( final ReconciledAugmentedClassInfo associatedResource = raci.associatedResourceInfo(); if (associatedResource.isCR()) { final var crInfo = associatedResource.asResourceTargeting(); - final String targetCRName = crInfo.fullResourceName(); + final String crId = crInfo.id(); // if the primary resource is unowned, mark it as "scheduled" (i.e. already handled) so that it doesn't get considered if all CRDs generation is requested if (!operatorConfiguration.isControllerOwningPrimary(raci.nameOrFailIfUnset())) { - scheduledForGeneration.add(targetCRName); + scheduledForGeneration.add(crId); } else { // When we have a live reload, check if we need to regenerate the associated CRD Map crdInfos = Collections.emptyMap(); if (liveReload.isLiveReload()) { - crdInfos = storedCRDInfos.getCRDInfosFor(targetCRName); + crdInfos = storedCRDInfos.getCRDInfosFor(crId); } // schedule the generation of associated primary resource CRD if required if (crdGeneration.scheduleForGenerationIfNeeded((CustomResourceAugmentedClassInfo) crInfo, crdInfos, changedClasses)) { - scheduledForGeneration.add(targetCRName); + scheduledForGeneration.add(crId); } } } @@ -80,9 +80,8 @@ GeneratedCRDInfoBuildItem generateCRDs( Map.of(CustomResourceAugmentedClassInfo.EXISTING_CRDS_KEY, scheduledForGeneration)) .map(CustomResourceAugmentedClassInfo.class::cast) .forEach(cr -> { - final var targetCRName = cr.fullResourceName(); - crdGeneration.withCustomResource(cr.loadAssociatedClass(), targetCRName, null); - log.infov("Will generate CRD for non-reconciler bound resource: {0}", targetCRName); + crdGeneration.withCustomResource(cr.loadAssociatedClass(), null); + log.infov("Will generate CRD for non-reconciler bound resource: {0}", cr.fullResourceName()); }); } } diff --git a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/ResourceControllerMapping.java b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/ResourceControllerMapping.java index 07106b7d..a9ed3798 100644 --- a/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/ResourceControllerMapping.java +++ b/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/ResourceControllerMapping.java @@ -17,8 +17,9 @@ public Map getResourceInfos(String resourceFullName) { return infos; } - public void add(io.fabric8.crdv2.generator.CustomResourceInfo info, String crdName, String associatedControllerName) { + public void add(io.fabric8.crdv2.generator.CustomResourceInfo info, String associatedControllerName) { final var version = info.version(); + final var crdName = info.crdName(); final var versionsForCR = resourceFullNameToVersionToInfos.computeIfAbsent(crdName, s -> new HashMap<>()); final var cri = versionsForCR.get(version); if (cri != null) { @@ -37,17 +38,16 @@ public void add(io.fabric8.crdv2.generator.CustomResourceInfo info, String crdNa throw new IllegalStateException(msg); } - final var converted = augment(info, crdName, associatedControllerName); + final var converted = augment(info, associatedControllerName); versionsForCR.put(version, converted); } - private static ResourceInfo augment(io.fabric8.crdv2.generator.CustomResourceInfo info, - String crdName, String associatedControllerName) { + private static ResourceInfo augment(io.fabric8.crdv2.generator.CustomResourceInfo info, String associatedControllerName) { return new ResourceInfo( info.group(), info.version(), info.kind(), info.singular(), info.plural(), info.shortNames(), info.storage(), info.served(), info.scope(), info.crClassName(), - info.statusClassName().map(ClassUtils::isStatusNotVoid).orElse(false), crdName, + info.statusClassName().map(ClassUtils::isStatusNotVoid).orElse(false), info.crdName(), associatedControllerName); } } diff --git a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/AllCRDGenerationTest.java b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/AllCRDGenerationTest.java index eb840bc4..191119aa 100644 --- a/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/AllCRDGenerationTest.java +++ b/core/deployment/src/test/java/io/quarkiverse/operatorsdk/test/AllCRDGenerationTest.java @@ -28,8 +28,8 @@ public class AllCRDGenerationTest { static final QuarkusProdModeTest config = new QuarkusProdModeTest() .setApplicationName("test") .withApplicationRoot( - (jar) -> jar.addClasses(SimpleReconciler.class, SimpleCR.class, SimpleSpec.class, SimpleStatus.class, - External.class, ExternalV1.class, SimpleReconcilerV2.class, SimpleCRV2.class)) + (jar) -> jar.addClasses(SimpleCR.class, SimpleSpec.class, SimpleStatus.class, + External.class, ExternalV1.class, SimpleCRV2.class, SimpleReconcilerV2.class)) .overrideConfigKey("quarkus.operator-sdk.crd.generate-all", "true"); @ProdBuildResults