From 04ee6ec42c7f3b81cf161d87c31aa3060d8374be Mon Sep 17 00:00:00 2001 From: Steve Hawkins Date: Thu, 24 Feb 2022 15:54:32 -0500 Subject: [PATCH] fix #3859: refinements to how a deserialization class is chosen --- CHANGELOG.md | 1 + doc/MIGRATION-v6.md | 7 +- .../kubernetes/client/utils/Utils.java | 32 ++-- .../test/resources/test-podsecuritypolicy.yml | 2 +- .../internal/KubernetesDeserializer.java | 151 ++++++++++++------ .../internal/KubernetesDeserializerTest.java | 36 +++-- .../fabric8/openshift/api/model/Template.java | 2 +- .../internal/core/TemplateOperationsImpl.java | 2 +- 8 files changed, 144 insertions(+), 89 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e91fdbe1568..1ffcbf2ad36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ * Fix #3848: Supports Queue (cluster) API for Volcano extension * Fix #3582: SSL truststore can be loaded in FIPS enabled environments * Fix #3818: adding missing throws to launderThrowable +* Fix #3859: refined how a deserialization class is chosen to not confuse types with the same kind #### Improvements * Fix #3811: Reintroduce `Replaceable` interface in `NonNamespaceOperation` diff --git a/doc/MIGRATION-v6.md b/doc/MIGRATION-v6.md index a9d5eb485a7..c2c7fb77d41 100644 --- a/doc/MIGRATION-v6.md +++ b/doc/MIGRATION-v6.md @@ -2,6 +2,7 @@ ## Contents: - [API/Impl split](#api-impl-split) +- [Deserialization Resolution](#deserialization-resolution) - [Deprecation Removals](#deprecation-removals) - [IntOrString changes](#intorstring-changes) @@ -29,7 +30,11 @@ To use it, exclude the kubernetes-httpclient-okhttp dependency and add the kuber - client.utils classes including Base64, CreateOrReplaceHelper, DeleteOrCreateHelper, OptionalDendencyWrapper, etc. are not in the -api jar, they are still in the -client jar under utils.internal. - Some other effectively internal classes in dsl.base and other packages were moved to corresponding internal packages - it is unlikely this will affect you unless you developed a custom extension. -### Deprecation Removals +## Deserialization Resolution + +The group on the object being deserialized is not required to match the prospective class - even for built-in types. This prevents the unintentional parsing of custom types without a registered class as a built-in type of the same name. This also means that you must ensure the apiVersion values are correct on the objects you are deserializing as they will no longer resolve to built-in type of the same name when there is a mistake. + +## Deprecation Removals - Removed KubernetesClient.customResource / RawCustomResourceOperationsImpl, please use the generic resource api instead - Removed HttpClientUtils.createHttpClient(final Config config, final Consumer additionalConfig), please use the OkHttpClientFactory instead diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java index 9b933f7b9e2..7cebe4334e9 100644 --- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java +++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java @@ -20,6 +20,9 @@ import io.fabric8.kubernetes.client.KubernetesClientException; import io.fabric8.kubernetes.model.annotation.Group; import io.fabric8.kubernetes.model.annotation.Version; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import java.io.Closeable; import java.io.Flushable; import java.io.IOException; @@ -51,8 +54,6 @@ import java.util.concurrent.TimeoutException; import java.util.function.Function; import java.util.stream.Stream; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; public class Utils { @@ -63,7 +64,7 @@ public class Utils { public static final String PATH_WINDOWS = "Path"; public static final String PATH_UNIX = "PATH"; private static final Random random = new Random(); - + private static final ExecutorService SHARED_POOL = Executors.newCachedThreadPool(); private static final CachedSingleThreadScheduler SHARED_SCHEDULER = new CachedSingleThreadScheduler(); @@ -166,12 +167,12 @@ public static boolean waitUntilReady(Future future, long amount, TimeUnit tim t = e.getCause(); } t.addSuppressed(new Throwable("waiting here")); - throw KubernetesClientException.launderThrowable(t); + throw KubernetesClientException.launderThrowable(t); } catch (Exception e) { throw KubernetesClientException.launderThrowable(e); } } - + /** * Similar to {@link #waitUntilReady(Future, long, TimeUnit)}, but will always throw an exception if not ready */ @@ -228,15 +229,6 @@ public static String randomString(int length) { return sb.toString(); } - public static String randomString(String prefix, int length) { - StringBuilder sb = new StringBuilder(); - for (int i = 0; i < length - prefix.length(); i++) { - int index = random.nextInt(ALL_CHARS.length()); - sb.append(ALL_CHARS.charAt(index)); - } - return sb.toString(); - } - public static String filePath(URL path) { try { return Paths.get(path.toURI()).toString(); @@ -431,7 +423,7 @@ public static List getCommandPlatformPrefix() { private static String getOperatingSystemFromSystemProperty() { return System.getProperty(OS_NAME); } - + /** * Create a {@link ThreadFactory} with daemon threads and a thread * name based upon the object passed in. @@ -444,17 +436,17 @@ public static ThreadFactory daemonThreadFactory(Object forObject) { static ThreadFactory daemonThreadFactory(String name) { return new ThreadFactory() { ThreadFactory threadFactory = Executors.defaultThreadFactory(); - + @Override public Thread newThread(Runnable r) { - Thread ret = threadFactory.newThread(r); + Thread ret = threadFactory.newThread(r); ret.setName(name + "-" + ret.getName()); ret.setDaemon(true); return ret; } }; } - + /** * Schedule a task to run in the given {@link Executor} - which should run the task in a different thread as to not * hold the scheduling thread @@ -471,12 +463,12 @@ public static ScheduledFuture scheduleAtFixedRate(Executor executor, Runnable // because of the hand-off to the other executor, there's no difference between rate and delay return SHARED_SCHEDULER.scheduleWithFixedDelay(() -> executor.execute(command), initialDelay, delay, unit); } - + /** * Get the common executor service - callers should not shutdown this service */ public static ExecutorService getCommonExecutorSerive() { return SHARED_POOL; } - + } diff --git a/kubernetes-itests/src/test/resources/test-podsecuritypolicy.yml b/kubernetes-itests/src/test/resources/test-podsecuritypolicy.yml index 4521eeda569..deb45d3abbc 100644 --- a/kubernetes-itests/src/test/resources/test-podsecuritypolicy.yml +++ b/kubernetes-itests/src/test/resources/test-podsecuritypolicy.yml @@ -14,7 +14,7 @@ # limitations under the License. # -apiVersion: extensions/v1beta1 +apiVersion: policy/v1beta1 kind: PodSecurityPolicy metadata: name: example diff --git a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java index 4ffa54e32f3..512da49295b 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java @@ -20,12 +20,12 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.ServiceLoader; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; import java.util.function.Predicate; -import java.util.stream.Collectors; import java.util.stream.Stream; import java.util.stream.StreamSupport; @@ -44,6 +44,37 @@ public class KubernetesDeserializer extends JsonDeserializer { + static class TypeKey { + final String kind; + final String apiGroup; + final String version; + + TypeKey(String kind, String apiGroup, String version) { + this.kind = kind; + this.apiGroup = apiGroup; + this.version = version; + } + + @Override + public int hashCode() { + return Objects.hash(kind, apiGroup, version); + } + + @Override + public boolean equals(Object obj) { + if (this == obj) { + return true; + } + if (!(obj instanceof TypeKey)) { + return false; + } + TypeKey o = (TypeKey) obj; + return Objects.equals(kind, o.kind) && Objects.equals(apiGroup, o.apiGroup) + && Objects.equals(version, o.version); + } + + } + private static final String TEMPLATE_CLASS_NAME = "io.fabric8.openshift.api.model.Template"; private static final String KIND = "kind"; private static final String API_VERSION = "apiVersion"; @@ -84,7 +115,7 @@ private KubernetesResource fromArrayNode(JsonParser jp, JsonNode node) throws IO } private static KubernetesResource fromObjectNode(JsonParser jp, JsonNode node) throws IOException { - String key = getKey(node); + TypeKey key = getKey(node); if (key != null) { Class resourceType = mapping.getForKey(key); if (resourceType == null) { @@ -110,7 +141,7 @@ private static KubernetesResource fromObjectNode(JsonParser jp, JsonNode node) t /** * Return a string representation of the key of the type: #. */ - private static String getKey(JsonNode node) { + private static TypeKey getKey(JsonNode node) { JsonNode apiVersion = node.get(API_VERSION); JsonNode kind = node.get(KIND); @@ -216,25 +247,54 @@ static class Mapping { "io.fabric8.kubernetes.api.model.extensions." }; - private Map> mappings = new ConcurrentHashMap<>(); + private Map> mappings = new ConcurrentHashMap<>(); + private Map> internalMappings = new ConcurrentHashMap<>(); Mapping() { registerAllProviders(); } - public Class getForKey(String key) { + public Class getForKey(TypeKey key) { if (key == null) { return null; } + // check for an exact match Class clazz = mappings.get(key); if (clazz != null) { return clazz; } - clazz = getInternalTypeForName(key); - if (clazz != null) { - mappings.put(key, clazz); + // check if it's a lazily-loaded internal type + List defaults = internalMappings.get(key.kind); + if (defaults == null) { + defaults = loadInternalTypes(key.kind); + clazz = mappings.get(key); // check again after load for an exact match + if (clazz != null) { + return clazz; + } + } + // if there are internal types matching kind, look for matching groups and versions + // but use first version seen (in PACKAGES order) + TypeKey bestMatch = null; + for (TypeKey typeKey : defaults) { + if (key.apiGroup != null) { + if (!key.apiGroup.equals(typeKey.apiGroup)) { + continue; + } + bestMatch = typeKey; + break; + } + if (key.version != null && key.version.equals(typeKey.apiGroup)) { + bestMatch = typeKey; + break; + } + if (bestMatch == null) { + bestMatch = typeKey; + } + } + if (bestMatch != null) { + return mappings.get(bestMatch); } - return clazz; + return null; } public void registerKind(String apiVersion, String kind, Class clazz) { @@ -245,27 +305,39 @@ public void registerProvider(KubernetesResourceMappingProvider provider) { if (provider == null) { return; } - Map> providerMappings = provider.getMappings().entrySet().stream() + provider.getMappings().entrySet().stream() //If the model is shaded (which is as part of kubernetes-client uberjar) this is going to cause conflicts. //This is why we NEED TO filter out incompatible resources. .filter(entry -> KubernetesResource.class.isAssignableFrom(entry.getValue())) - .collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); - mappings.putAll(providerMappings); + .forEach(e -> mappings.put(createKey(e.getKey()), e.getValue())); } /** * Returns a composite key for apiVersion and kind. */ - String createKey(String apiVersion, String kind) { + TypeKey createKey(String apiVersion, String kind) { if (kind == null) { return null; } else if (apiVersion == null) { - return kind; + return new TypeKey(kind, null, null); } else { - return apiVersion + KEY_SEPARATOR + kind; + String[] versionParts = new String[] {null, apiVersion}; + if (apiVersion != null && apiVersion.contains("/")) { + versionParts = apiVersion.split("/", 2); + } + return new TypeKey(kind, versionParts[0], versionParts[1]); } } + TypeKey createKey(String key) { + // null is not allowed + if (key.contains(KEY_SEPARATOR)) { + String[] parts = key.split(KEY_SEPARATOR, 2); + return createKey(parts[0], parts[1]); + } + return createKey(null, key); + } + private void registerAllProviders() { getAllMappingProviders().forEach(this::registerProvider); } @@ -284,52 +356,31 @@ Stream getAllMappingProviders() { .filter(distinctByClassName(KubernetesResourceMappingProvider::getClass)); } - private String getClassName(String key) { - if (key != null && key.contains(KEY_SEPARATOR)) { - return key.substring(key.indexOf(KEY_SEPARATOR) + 1); - } else { - return key; - } - } - - private Class getInternalTypeForName(String key) { - String name = getClassName(key); - List> possibleResults = new ArrayList<>(); - - // First pass, check if there are more than one class of same name - for (String aPackage : PACKAGES) { - Class result = loadClassIfExists(aPackage + name); - if (result != null) { - possibleResults.add(result); + private List loadInternalTypes(String kind) { + List ordering = new ArrayList<>(); + for (int i = 0; i < PACKAGES.length; i++) { + Class result = loadClassIfExists(PACKAGES[i] + kind); + if (result == null) { + continue; } + TypeKey defaultKeyFromClass = getKeyFromClass(result); + mappings.put(defaultKeyFromClass, result); + ordering.add(defaultKeyFromClass); } - // If only one class found, return it - if (possibleResults.size() == 1) { - return possibleResults.get(0); - } else if (possibleResults.size() > 1) { - // Compare with apiVersions being compared for set of classes found - for (Class result : possibleResults) { - String defaultKeyFromClass = getKeyFromClass(result); - if (key.equals(defaultKeyFromClass)) { - return result; - } - } - return possibleResults.get(0); - } else { - return null; - } + internalMappings.put(kind, ordering); + return ordering; } - private String getKeyFromClass(Class clazz) { + private TypeKey getKeyFromClass(Class clazz) { String apiGroup = Helper.getAnnotationValue(clazz, Group.class); String apiVersion = Helper.getAnnotationValue(clazz, Version.class); if (apiGroup != null && !apiGroup.isEmpty() && apiVersion != null && !apiVersion.isEmpty()) { - return createKey(apiGroup + "/" + apiVersion, clazz.getSimpleName()); + return new TypeKey(clazz.getSimpleName(), apiGroup, apiVersion); } else if (apiVersion != null && !apiVersion.isEmpty()) { return createKey(apiVersion, clazz.getSimpleName()); } - return clazz.getSimpleName(); + return new TypeKey(clazz.getSimpleName(), null, null); } private Class loadClassIfExists(String className) { diff --git a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/internal/KubernetesDeserializerTest.java b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/internal/KubernetesDeserializerTest.java index 8e63c6b18cf..05376f7d2f5 100644 --- a/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/internal/KubernetesDeserializerTest.java +++ b/kubernetes-model-generator/kubernetes-model-core/src/test/java/io/fabric8/kubernetes/internal/KubernetesDeserializerTest.java @@ -19,6 +19,7 @@ import io.fabric8.kubernetes.api.model.KubernetesResource; import io.fabric8.kubernetes.api.model.Pod; import io.fabric8.kubernetes.api.model.Quantity; +import io.fabric8.kubernetes.internal.KubernetesDeserializer.TypeKey; import org.apache.commons.lang3.tuple.Pair; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -43,7 +44,7 @@ void shouldRegisterKind() { // given String version = "version1"; String kind = "kind1"; - String key = mapping.createKey(version, kind); + TypeKey key = mapping.createKey(version, kind); assertThat(mapping.getForKey(key)).isNull(); // when mapping.registerKind(version, kind, SmurfResource.class); @@ -67,7 +68,7 @@ void shouldRegisterKindWithoutVersionIfNullVersion() { // given String version = null; String kind = "kind1"; - String key = mapping.createKey(version, kind); + TypeKey key = mapping.createKey(version, kind); // when mapping.registerKind(version, kind, SmurfResource.class); // then @@ -78,10 +79,10 @@ void shouldRegisterKindWithoutVersionIfNullVersion() { @Test void shouldRegisterProvider() { // given - String key = mapping.createKey("42", "Hitchhiker"); + TypeKey key = mapping.createKey("42", "Hitchhiker"); assertThat(mapping.getForKey(key)).isNull(); KubernetesResourceMappingProvider provider = createProvider( - Pair.of(key, SmurfResource.class)); + Pair.of("42#Hitchhiker", SmurfResource.class)); // when mapping.registerProvider(provider); // then @@ -94,8 +95,8 @@ void shouldReturnMappedClass() { // given String version = "version1"; String kind = SmurfResource.class.getSimpleName(); - String key = mapping.createKey(version, kind); - assertThat(mapping.getForKey(key)).isNull();; + TypeKey key = mapping.createKey(version, kind); + assertThat(mapping.getForKey(key)).isNull(); mapping.registerKind(version, kind, SmurfResource.class); // when Class clazz = mapping.getForKey(key); @@ -115,27 +116,37 @@ void shouldReturnNullIfKeyIsNull() { @Test void shouldLoadClassInPackage() { // given - String key = mapping.createKey("42", Pod.class.getSimpleName()); + TypeKey key = mapping.createKey("42", Pod.class.getSimpleName()); // when Class clazz = mapping.getForKey(key); // then assertThat(clazz).isEqualTo(Pod.class); } + @Test + void shouldNotLoadClassInPackage() { + // given + TypeKey key = mapping.createKey("other/v1", Pod.class.getSimpleName()); + // when + Class clazz = mapping.getForKey(key); + // then + assertThat(clazz).isNull(); + } + @Test void shouldNotLoadClassInPackageIfNotKubernetesResource() { // given Quantity is not a KubernetesResource - String key = mapping.createKey("42", Quantity.class.getSimpleName()); + TypeKey key = mapping.createKey("42", Quantity.class.getSimpleName()); // when Class clazz = mapping.getForKey(key); // then - assertThat(clazz).isNull();; + assertThat(clazz).isNull(); } @Test void shouldLoadClassIfKeyOnlyHasKind() { // given Quantity is not a KubernetesResource - String key = mapping.createKey(null, Pod.class.getSimpleName()); + TypeKey key = mapping.createKey(null, Pod.class.getSimpleName()); // when Class clazz = mapping.getForKey(key); // then @@ -160,11 +171,6 @@ protected Stream getAllMappingProviders() { return Stream.of(provider); } - - @Override - protected String createKey(String apiVersion, String kind) { - return super.createKey(apiVersion, kind); - } } private static final class SmurfResource implements KubernetesResource { diff --git a/kubernetes-model-generator/openshift-model/src/main/java/io/fabric8/openshift/api/model/Template.java b/kubernetes-model-generator/openshift-model/src/main/java/io/fabric8/openshift/api/model/Template.java index e78847dd615..ea647c99123 100644 --- a/kubernetes-model-generator/openshift-model/src/main/java/io/fabric8/openshift/api/model/Template.java +++ b/kubernetes-model-generator/openshift-model/src/main/java/io/fabric8/openshift/api/model/Template.java @@ -69,7 +69,7 @@ }) @Buildable(editableEnabled = false, validationEnabled = false, generateBuilderPackage=false, builderPackage = "io.fabric8.kubernetes.api.builder") @Version("v1") -@Group("") +@Group("template.openshift.io") @TemplateTransformations({ @TemplateTransformation(value = "/manifest.vm", outputPath = "openshift.properties", gather = true) }) diff --git a/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/core/TemplateOperationsImpl.java b/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/core/TemplateOperationsImpl.java index ac762594fb6..24caabb3ab3 100644 --- a/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/core/TemplateOperationsImpl.java +++ b/openshift-client/src/main/java/io/fabric8/openshift/client/dsl/internal/core/TemplateOperationsImpl.java @@ -233,7 +233,7 @@ private URL getProcessUrl() throws MalformedURLException { @Override public TemplateResource load(InputStream is) { - String generatedName = Utils.randomString("template-", 10); + String generatedName = "template-" + Utils.randomString(5); Template template = null; Object item = Serialization.unmarshal(is, parameters); if (item instanceof Template) {