fix fabric8io#3859: refinements to how a deserialization class is chosen

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`

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)
 - [Object sorting](#object-sorting)
@@ -30,7 +31,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<OkHttpClient.Builder> additionalConfig), please use the OkHttpClientFactory instead

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<String> 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;
   }
-    
+
 }

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

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<KubernetesResource> {
 
+    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<? extends KubernetesResource> 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: <version>#<kind>.
      */
-    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<String, Class<? extends KubernetesResource>> mappings = new ConcurrentHashMap<>();
+        private Map<TypeKey, Class<? extends KubernetesResource>> mappings = new ConcurrentHashMap<>();
+        private Map<String, List<TypeKey>> internalMappings = new ConcurrentHashMap<>();
 
         Mapping() {
         	registerAllProviders();
         }
 
-        public Class<? extends KubernetesResource> getForKey(String key) {
+        public Class<? extends KubernetesResource> getForKey(TypeKey key) {
             if (key == null) {
                 return null;
             }
+            // check for an exact match
             Class<? extends KubernetesResource> 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<TypeKey> 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<? extends KubernetesResource> clazz) {
@@ -245,27 +305,39 @@ public void registerProvider(KubernetesResourceMappingProvider provider) {
             if (provider == null) {
                 return;
             }
-            Map<String, Class<? extends KubernetesResource>> 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<KubernetesResourceMappingProvider> 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<? extends KubernetesResource> getInternalTypeForName(String key) {
-            String name = getClassName(key);
-            List<Class<? extends KubernetesResource>> possibleResults = new ArrayList<>();
-
-            // First pass, check if there are more than one class of same name
-            for (String aPackage : PACKAGES) {
-                Class<? extends KubernetesResource> result = loadClassIfExists(aPackage + name);
-                if (result != null) {
-                    possibleResults.add(result);
+        private List<TypeKey> loadInternalTypes(String kind) {
+            List<TypeKey> ordering = new ArrayList<>();
+            for (int i = 0; i < PACKAGES.length; i++) {
+                Class<? extends KubernetesResource> 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<? extends KubernetesResource> 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<? extends KubernetesResource> clazz) {
+        private TypeKey getKeyFromClass(Class<? extends KubernetesResource> 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<? extends KubernetesResource> loadClassIfExists(String className) {