From 84debd8aa9e8015597390cb65aaf403122202026 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Attila=20M=C3=A9sz=C3=A1ros?= Date: Wed, 27 Nov 2024 13:17:28 +0100 Subject: [PATCH] improve: mapping from annotation depends on type (#2606) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * improve: mapping from annotation depends on type Signed-off-by: Attila Mészáros compiles Signed-off-by: Attila Mészáros * refactor: avoid creating useless strings Signed-off-by: Chris Laprun * refactor: ensure roundtrip works Signed-off-by: Chris Laprun * refactor: rename toSimpleString to toGVKString for greater clarity Signed-off-by: Chris Laprun * refactor: minor improvements Signed-off-by: Chris Laprun --------- Signed-off-by: Attila Mészáros Signed-off-by: Chris Laprun Co-authored-by: Chris Laprun --- .../operator/processing/GroupVersionKind.java | 26 ++++++-- .../KubernetesDependentResource.java | 8 ++- .../event/source/informer/Mappers.java | 64 +++++++++++++------ .../processing/GroupVersionKindTest.java | 12 ++++ ...formerEventSourceTestCustomReconciler.java | 5 +- .../InformerRemoteClusterReconciler.java | 3 +- ...stomMappingConfigMapDependentResource.java | 7 +- 7 files changed, 92 insertions(+), 33 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java index 4b0a607182..f9cf38fa56 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/GroupVersionKind.java @@ -7,6 +7,7 @@ import io.fabric8.kubernetes.api.model.HasMetadata; public class GroupVersionKind { + private static final String SEPARATOR = "/"; private final String group; private final String version; private final String kind; @@ -16,7 +17,7 @@ public class GroupVersionKind { public GroupVersionKind(String apiVersion, String kind) { this.kind = kind; - String[] groupAndVersion = apiVersion.split("/"); + String[] groupAndVersion = apiVersion.split(SEPARATOR); if (groupAndVersion.length == 1) { this.group = null; this.version = groupAndVersion[0]; @@ -40,24 +41,24 @@ public GroupVersionKind(String group, String version, String kind) { this.group = group; this.version = version; this.kind = kind; - this.apiVersion = (group == null || group.isBlank()) ? version : group + "/" + version; + this.apiVersion = (group == null || group.isBlank()) ? version : group + SEPARATOR + version; } /** * Parse GVK from a String representation. Expected format is: [group]/[version]/[kind] - * + * * 
    *   Sample: "apps/v1/Deployment"
    *
- * + * * or: [version]/[kind] - * + * * 
    *     Sample: v1/ConfigMap
    *
**/ public static GroupVersionKind fromString(String gvk) { - String[] parts = gvk.split("/"); + String[] parts = gvk.split(SEPARATOR); if (parts.length == 3) { return new GroupVersionKind(parts[0], parts[1], parts[2]); } else if (parts.length == 2) { @@ -68,6 +69,19 @@ public static GroupVersionKind fromString(String gvk) { } } + /** + * Reverse to {@link #fromString(String)}. + * + * @return gvk encoded in simple string. + */ + public String toGVKString() { + if (group != null) { + return group + SEPARATOR + version + SEPARATOR + kind; + } else { + return version + SEPARATOR + kind; + } + } + public String getGroup() { return group; } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java index 048b2b8f03..a3373e2d6c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java @@ -19,6 +19,7 @@ import io.javaoperatorsdk.operator.api.reconciler.Ignore; import io.javaoperatorsdk.operator.api.reconciler.dependent.GarbageCollected; import io.javaoperatorsdk.operator.api.reconciler.dependent.managed.ConfiguredDependentResource; +import io.javaoperatorsdk.operator.processing.GroupVersionKind; import io.javaoperatorsdk.operator.processing.dependent.AbstractEventSourceHolderDependentResource; import io.javaoperatorsdk.operator.processing.dependent.Matcher.Result; import io.javaoperatorsdk.operator.processing.event.ResourceID; @@ -208,17 +209,18 @@ private boolean useNonOwnerRefBasedSecondaryToPrimaryMapping() { protected void addSecondaryToPrimaryMapperAnnotations(R desired, P primary) { addSecondaryToPrimaryMapperAnnotations(desired, primary, Mappers.DEFAULT_ANNOTATION_FOR_NAME, - Mappers.DEFAULT_ANNOTATION_FOR_NAMESPACE); + Mappers.DEFAULT_ANNOTATION_FOR_NAMESPACE, Mappers.DEFAULT_ANNOTATION_FOR_PRIMARY_TYPE); } protected void addSecondaryToPrimaryMapperAnnotations(R desired, P primary, String nameKey, - String namespaceKey) { + String namespaceKey, String typeKey) { var annotations = desired.getMetadata().getAnnotations(); annotations.put(nameKey, primary.getMetadata().getName()); var primaryNamespaces = primary.getMetadata().getNamespace(); if (primaryNamespaces != null) { annotations.put(namespaceKey, primary.getMetadata().getNamespace()); } + annotations.put(typeKey, GroupVersionKind.gvkFor(primary.getClass()).toGVKString()); } @Override @@ -274,7 +276,7 @@ protected Optional> getSecondaryToPrimaryMapper( return Optional .of(Mappers.fromOwnerReferences(context.getPrimaryResourceClass(), clustered)); } else if (isCreatable()) { - return Optional.of(Mappers.fromDefaultAnnotations()); + return Optional.of(Mappers.fromDefaultAnnotations(context.getPrimaryResourceClass())); } } return Optional.empty(); diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java index 97ab1ef402..db3877376c 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/Mappers.java @@ -5,6 +5,7 @@ import java.util.stream.Collectors; import io.fabric8.kubernetes.api.model.HasMetadata; +import io.javaoperatorsdk.operator.processing.GroupVersionKind; import io.javaoperatorsdk.operator.processing.event.ResourceID; import io.javaoperatorsdk.operator.processing.event.source.SecondaryToPrimaryMapper; @@ -13,33 +14,41 @@ public class Mappers { public static final String DEFAULT_ANNOTATION_FOR_NAME = "io.javaoperatorsdk/primary-name"; public static final String DEFAULT_ANNOTATION_FOR_NAMESPACE = "io.javaoperatorsdk/primary-namespace"; + public static final String DEFAULT_ANNOTATION_FOR_PRIMARY_TYPE = + "io.javaoperatorsdk/primary-type"; private Mappers() {} public static SecondaryToPrimaryMapper fromAnnotation( - String nameKey) { - return fromMetadata(nameKey, null, false); + String nameKey, String typeKey, Class primaryResourceType) { + return fromAnnotation(nameKey, null, typeKey, primaryResourceType); } @SuppressWarnings("unused") public static SecondaryToPrimaryMapper fromAnnotation( - String nameKey, String namespaceKey) { - return fromMetadata(nameKey, namespaceKey, false); + String nameKey, String namespaceKey, String typeKey, + Class primaryResourceType) { + return fromMetadata(nameKey, namespaceKey, typeKey, primaryResourceType, false); } @SuppressWarnings("unused") - public static SecondaryToPrimaryMapper fromLabel(String nameKey) { - return fromMetadata(nameKey, null, true); + public static SecondaryToPrimaryMapper fromLabel(String nameKey, + String typeKey, + Class primaryResourceType) { + return fromLabel(nameKey, null, typeKey, primaryResourceType); } - public static SecondaryToPrimaryMapper fromDefaultAnnotations() { - return fromMetadata(DEFAULT_ANNOTATION_FOR_NAME, DEFAULT_ANNOTATION_FOR_NAMESPACE, false); + public static SecondaryToPrimaryMapper fromDefaultAnnotations( + Class primaryResourceType) { + return fromAnnotation(DEFAULT_ANNOTATION_FOR_NAME, DEFAULT_ANNOTATION_FOR_NAMESPACE, + DEFAULT_ANNOTATION_FOR_PRIMARY_TYPE, primaryResourceType); } @SuppressWarnings("unused") public static SecondaryToPrimaryMapper fromLabel( - String nameKey, String namespaceKey) { - return fromMetadata(nameKey, namespaceKey, true); + String nameKey, String namespaceKey, String typeKey, + Class primaryResourceType) { + return fromMetadata(nameKey, namespaceKey, typeKey, primaryResourceType, true); } public static SecondaryToPrimaryMapper fromOwnerReferences( @@ -78,7 +87,8 @@ public static SecondaryToPrimaryMapper fromOwnerRefer } private static SecondaryToPrimaryMapper fromMetadata( - String nameKey, String namespaceKey, boolean isLabel) { + String nameKey, String namespaceKey, String typeKey, + Class primaryResourceType, boolean isLabel) { return resource -> { final var metadata = resource.getMetadata(); if (metadata == null) { @@ -94,6 +104,15 @@ private static SecondaryToPrimaryMapper fromMetadata( } var namespace = namespaceKey == null ? resource.getMetadata().getNamespace() : map.get(namespaceKey); + + String gvkSimple = map.get(typeKey); + + if (gvkSimple != null && + !GroupVersionKind.fromString(gvkSimple) + .equals(GroupVersionKind.gvkFor(primaryResourceType))) { + return Set.of(); + } + return Set.of(new ResourceID(name, namespace)); } }; @@ -105,14 +124,11 @@ public static ResourceID fromString(String cacheKey) { } final String[] split = cacheKey.split("/"); - switch (split.length) { - case 1: - return new ResourceID(split[0]); - case 2: - return new ResourceID(split[1], split[0]); - default: - throw new IllegalArgumentException("Cannot extract a ResourceID from " + cacheKey); - } + return switch (split.length) { + case 1 -> new ResourceID(split[0]); + case 2 -> new ResourceID(split[1], split[0]); + default -> throw new IllegalArgumentException("Cannot extract a ResourceID from " + cacheKey); + }; } /** @@ -139,9 +155,17 @@ public static SecondaryToPrim public static class SecondaryToPrimaryFromDefaultAnnotation implements SecondaryToPrimaryMapper { + + private final Class primaryResourceType; + + public SecondaryToPrimaryFromDefaultAnnotation( + Class primaryResourceType) { + this.primaryResourceType = primaryResourceType; + } + @Override public Set toPrimaryResourceIDs(HasMetadata resource) { - return Mappers.fromDefaultAnnotations().toPrimaryResourceIDs(resource); + return Mappers.fromDefaultAnnotations(primaryResourceType).toPrimaryResourceIDs(resource); } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/GroupVersionKindTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/GroupVersionKindTest.java index aefbaf6d49..b7aeaae670 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/GroupVersionKindTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/GroupVersionKindTest.java @@ -75,4 +75,16 @@ void pluralShouldOverrideDefaultComputedVersionIfProvided() { "MyPlural"); assertThat(gvk.getPlural()).hasValue("MyPlural"); } + + @Test + void encodesToGVKString() { + final var deploymentGVK = "apps/v1/Deployment"; + var gvk = GroupVersionKind.fromString(deploymentGVK); + assertThat(gvk.toGVKString()).isEqualTo(deploymentGVK); + assertThat(gvk).isEqualTo(GroupVersionKind.fromString(gvk.toGVKString())); + + gvk = GroupVersionKind.fromString("v1/ConfigMap"); + assertThat(gvk.toGVKString()).isEqualTo("v1/ConfigMap"); + assertThat(gvk).isEqualTo(GroupVersionKind.fromString(gvk.toGVKString())); + } } diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informereventsource/InformerEventSourceTestCustomReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informereventsource/InformerEventSourceTestCustomReconciler.java index 59430fd581..5028d6a0f4 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informereventsource/InformerEventSourceTestCustomReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informereventsource/InformerEventSourceTestCustomReconciler.java @@ -26,6 +26,7 @@ public class InformerEventSourceTestCustomReconciler LoggerFactory.getLogger(InformerEventSourceTestCustomReconciler.class); public static final String RELATED_RESOURCE_NAME = "relatedResourceName"; + public static final String RELATED_RESOURCE_TYPE = "relatedResourceType"; public static final String TARGET_CONFIG_MAP_KEY = "targetStatus"; public static final String MISSING_CONFIG_MAP = "Missing Config Map"; @@ -38,7 +39,9 @@ public List> prepareEventS InformerEventSourceConfiguration config = InformerEventSourceConfiguration .from(ConfigMap.class, InformerEventSourceTestCustomResource.class) - .withSecondaryToPrimaryMapper(Mappers.fromAnnotation(RELATED_RESOURCE_NAME)) + .withSecondaryToPrimaryMapper( + Mappers.fromAnnotation(RELATED_RESOURCE_NAME, RELATED_RESOURCE_TYPE, + InformerEventSourceTestCustomResource.class)) .build(); return List.of(new InformerEventSource<>(config, context)); diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informerremotecluster/InformerRemoteClusterReconciler.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informerremotecluster/InformerRemoteClusterReconciler.java index 44d81f5327..9246e0bbd5 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informerremotecluster/InformerRemoteClusterReconciler.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/informerremotecluster/InformerRemoteClusterReconciler.java @@ -53,7 +53,8 @@ public List> prepareEventSou .from(ConfigMap.class, InformerRemoteClusterCustomResource.class) // owner references do not work cross cluster, using // annotations here to reference primary resource - .withSecondaryToPrimaryMapper(Mappers.fromDefaultAnnotations()) + .withSecondaryToPrimaryMapper( + Mappers.fromDefaultAnnotations(InformerRemoteClusterCustomResource.class)) // setting remote client for informer .withKubernetesClient(remoteClient) .withInformerConfiguration( diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/dependentcustommappingannotation/CustomMappingConfigMapDependentResource.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/dependentcustommappingannotation/CustomMappingConfigMapDependentResource.java index 1f05ebfeca..ad8639f6cf 100644 --- a/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/dependentcustommappingannotation/CustomMappingConfigMapDependentResource.java +++ b/operator-framework/src/test/java/io/javaoperatorsdk/operator/dependent/dependentcustommappingannotation/CustomMappingConfigMapDependentResource.java @@ -20,10 +20,12 @@ public class CustomMappingConfigMapDependentResource public static final String CUSTOM_NAME_KEY = "customNameKey"; public static final String CUSTOM_NAMESPACE_KEY = "customNamespaceKey"; + public static final String CUSTOM_TYPE_KEY = "customTypeKey"; public static final String KEY = "key"; private static final SecondaryToPrimaryMapper mapper = - Mappers.fromAnnotation(CUSTOM_NAME_KEY, CUSTOM_NAMESPACE_KEY); + Mappers.fromAnnotation(CUSTOM_NAME_KEY, CUSTOM_NAMESPACE_KEY, CUSTOM_TYPE_KEY, + DependentCustomMappingCustomResource.class); public CustomMappingConfigMapDependentResource() { super(ConfigMap.class); @@ -44,7 +46,8 @@ protected ConfigMap desired(DependentCustomMappingCustomResource primary, @Override protected void addSecondaryToPrimaryMapperAnnotations(ConfigMap desired, DependentCustomMappingCustomResource primary) { - addSecondaryToPrimaryMapperAnnotations(desired, primary, CUSTOM_NAME_KEY, CUSTOM_NAMESPACE_KEY); + addSecondaryToPrimaryMapperAnnotations(desired, primary, CUSTOM_NAME_KEY, CUSTOM_NAMESPACE_KEY, + CUSTOM_TYPE_KEY); }