Merge pull request kubernetes-client#1804 from yue9944882/bugfix/crd-props-yaml-codec

k8s-ci-robot · web-flow · commit f5efa4748522 · 2021-07-29T13:21:18.000-07:00
Bugfix: CRD's openapi schema extension cannot be properly serialized
diff --git a/util/src/main/java/io/kubernetes/client/util/Yaml.java b/util/src/main/java/io/kubernetes/client/util/Yaml.java
@@ -12,27 +12,36 @@
 */
 package io.kubernetes.client.util;
 
+import com.google.gson.annotations.SerializedName;
 import io.kubernetes.client.common.KubernetesType;
 import io.kubernetes.client.custom.IntOrString;
 import io.kubernetes.client.custom.Quantity;
+import io.kubernetes.client.openapi.models.V1JSONSchemaProps;
+import io.kubernetes.client.openapi.models.V1beta1JSONSchemaProps;
 import java.io.File;
 import java.io.FileReader;
 import java.io.IOException;
 import java.io.Reader;
 import java.io.StringReader;
 import java.io.Writer;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.time.OffsetDateTime;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import okio.ByteString;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.yaml.snakeyaml.DumperOptions;
+import org.yaml.snakeyaml.TypeDescription;
+import org.yaml.snakeyaml.constructor.BaseConstructor;
 import org.yaml.snakeyaml.constructor.Constructor;
 import org.yaml.snakeyaml.constructor.SafeConstructor;
 import org.yaml.snakeyaml.introspector.Property;
@@ -44,6 +53,7 @@
 import org.yaml.snakeyaml.representer.Represent;
 import org.yaml.snakeyaml.representer.Representer;
 
+/** The type Yaml. */
 public class Yaml {
 
   static final Logger logger = LoggerFactory.getLogger(Yaml.class);
@@ -359,17 +369,137 @@ protected NodeTuple representJavaBeanProperty(
     }
   }
 
-  /** @return An instantiated SnakeYaml Object. */
+  /**
+   * Instantiate a snake yaml with a default {@link SafeConstructor}.
+   *
+   * <p>DEPRECATED: Use the parameterized "getSnakeYaml" constructing method below to get rid of
+   * vulnerability from dynamic type serialization.
+   *
+   * @return the snake yaml
+   */
   @Deprecated
   public static org.yaml.snakeyaml.Yaml getSnakeYaml() {
     return getSnakeYaml(null);
   }
 
-  private static org.yaml.snakeyaml.Yaml getSnakeYaml(Class<?> type) {
+  /**
+   * Instantiate a snake yaml with the target model type specified..
+   *
+   * @param type the target model type
+   * @param typeDescriptions additional type descriptions for customizing the serializer
+   * @return the new snake yaml instance
+   */
+  public static org.yaml.snakeyaml.Yaml getSnakeYaml(
+      Class<?> type, TypeDescription... typeDescriptions) {
+    BaseConstructor constructor = new SafeConstructor();
+    Representer representer = new CustomRepresenter();
     if (type != null) {
-      return new org.yaml.snakeyaml.Yaml(new CustomConstructor(type), new CustomRepresenter());
+      constructor = new CustomConstructor(type);
+    }
+    // register builtin type descriptions
+    registerBuiltinGsonCompatibles(constructor, representer);
+    for (TypeDescription desc : typeDescriptions) {
+      registerCustomTypeDescriptions(constructor, representer, desc);
+    }
+    return new org.yaml.snakeyaml.Yaml(constructor, representer);
+  }
+
+  /**
+   * Instantiate a new {@link TypeDescription} which will load the {@link SerializedName} via
+   * reflection so that yaml serialization can work for the custom gson serialized name.
+   *
+   * @param modelClass the kubenretes api model class
+   * @param gsonTaggedFields the custom serialized names tagged by gson
+   * @return the type description
+   */
+  public static TypeDescription newGsonCompatibleTypeDescription(
+      Class modelClass, String... gsonTaggedFields) {
+    TypeDescription desc = new TypeDescription(modelClass);
+    List<String> excluding = new ArrayList<>();
+    for (String targetGsonAnnotation : gsonTaggedFields) {
+      Field field =
+          Arrays.stream(modelClass.getDeclaredFields())
+              .filter(f -> f.getAnnotation(SerializedName.class) != null)
+              .filter(
+                  f -> targetGsonAnnotation.equals(f.getAnnotation(SerializedName.class).value()))
+              .findAny()
+              .orElseThrow(
+                  () ->
+                      new IllegalArgumentException(
+                          "Api model class "
+                              + modelClass.getSimpleName()
+                              + " doesn't have field with Gson @SerializedName with value "
+                              + targetGsonAnnotation));
+      Method getterMethod =
+          tryFindGetterMethod(modelClass, field)
+              .orElseThrow(
+                  () ->
+                      new IllegalArgumentException(
+                          "Cannot find getter method for "
+                              + targetGsonAnnotation
+                              + " on api model class "
+                              + modelClass.getSimpleName()));
+      Method setterMethod =
+          tryFindSetterMethod(modelClass, field)
+              .orElseThrow(
+                  () ->
+                      new IllegalArgumentException(
+                          "Cannot find setter method for "
+                              + targetGsonAnnotation
+                              + " on api model class "
+                              + modelClass.getSimpleName()));
+
+      desc.substituteProperty(
+          targetGsonAnnotation, field.getType(), getterMethod.getName(), setterMethod.getName());
+      excluding.add(field.getName());
     }
-    return new org.yaml.snakeyaml.Yaml(new SafeConstructor(), new CustomRepresenter());
+    desc.setExcludes(excluding.toArray(new String[0]));
+    return desc;
+  }
+
+  private static void registerBuiltinGsonCompatibles(
+      BaseConstructor constructor, Representer representer) {
+    // TODO: Are there more builtin api classes need these explicit substitution below
+    String[] crdOpenApiExtensions =
+        new String[] {
+          "x-kubernetes-embedded-resource",
+          "x-kubernetes-int-or-string",
+          "x-kubernetes-list-map-keys",
+          "x-kubernetes-list-type",
+          "x-kubernetes-map-type",
+          "x-kubernetes-preserve-unknown-fields",
+        };
+    registerCustomTypeDescriptions(
+        constructor,
+        representer,
+        newGsonCompatibleTypeDescription(V1JSONSchemaProps.class, crdOpenApiExtensions),
+        newGsonCompatibleTypeDescription(V1beta1JSONSchemaProps.class, crdOpenApiExtensions));
+  }
+
+  private static void registerCustomTypeDescriptions(
+      BaseConstructor constructor,
+      Representer representer,
+      TypeDescription... addtionalTypeDescriptions) {
+    Arrays.stream(addtionalTypeDescriptions)
+        .forEach(
+            desc -> {
+              constructor.addTypeDescription(desc);
+              representer.addTypeDescription(desc);
+            });
+  }
+
+  private static Optional<Method> tryFindGetterMethod(Class modelClass, Field targetField) {
+    return Arrays.stream(modelClass.getDeclaredMethods())
+        .filter(f -> f.getName().startsWith("get"))
+        .filter(f -> f.getName().equalsIgnoreCase("get" + targetField.getName()))
+        .findAny();
+  }
+
+  private static Optional<Method> tryFindSetterMethod(Class modelClass, Field targetField) {
+    return Arrays.stream(modelClass.getDeclaredMethods())
+        .filter(f -> f.getName().startsWith("set"))
+        .filter(f -> f.getName().equalsIgnoreCase("set" + targetField.getName()))
+        .findAny();
   }
 
   /**
diff --git a/util/src/test/java/io/kubernetes/client/util/YamlTest.java b/util/src/test/java/io/kubernetes/client/util/YamlTest.java
@@ -16,12 +16,14 @@
 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
 import io.kubernetes.client.Resources;
 import io.kubernetes.client.common.KubernetesType;
+import io.kubernetes.client.openapi.models.V1CustomResourceDefinition;
 import io.kubernetes.client.openapi.models.V1Deployment;
 import io.kubernetes.client.openapi.models.V1ObjectMeta;
 import io.kubernetes.client.openapi.models.V1Pod;
@@ -49,6 +51,8 @@ public class YamlTest {
 
   private static final URL TAGGED_FILE = Resources.getResource("pod-tag.yaml");
 
+  private static final URL CRD_INT_OR_STRING_FILE = Resources.getResource("crd-int-or-string.yaml");
+
   private static final String[] kinds =
       new String[] {
         "Pod",
@@ -280,4 +284,22 @@ public void testLoadAsYamlCantConstructObjects() {
     }
     assertFalse("Object should not be constructed!", TestPoJ.hasBeenConstructed());
   }
+
+  @Test
+  public void testLoadDumpCRDWithIntOrStringExtension() {
+    String data = Resources.toString(CRD_INT_OR_STRING_FILE, UTF_8);
+    V1CustomResourceDefinition crd = Yaml.loadAs(data, V1CustomResourceDefinition.class);
+    assertNotNull(crd);
+    assertTrue(
+        crd.getSpec()
+            .getVersions()
+            .get(0)
+            .getSchema()
+            .getOpenAPIV3Schema()
+            .getProperties()
+            .get("foo")
+            .getxKubernetesIntOrString());
+    String dumped = Yaml.dump(crd);
+    assertEquals(data, dumped);
+  }
 }
diff --git a/util/src/test/resources/crd-int-or-string.yaml b/util/src/test/resources/crd-int-or-string.yaml
@@ -0,0 +1,36 @@
+apiVersion: apiextensions.k8s.io/v1
+kind: CustomResourceDefinition
+metadata:
+  name: crontabs.stable.example.com
+spec:
+  group: stable.example.com
+  names:
+    kind: CronTab
+    plural: crontabs
+    shortNames:
+    - ct
+    singular: crontab
+  scope: Namespaced
+  versions:
+  - name: v1
+    schema:
+      openAPIV3Schema:
+        type: object
+        properties:
+          foo:
+            anyOf:
+            - type: integer
+            - type: string
+            pattern: ^.*
+            x-kubernetes-int-or-string: true
+          spec:
+            type: object
+            properties:
+              cronSpec:
+                type: string
+              image:
+                type: string
+              replicas:
+                type: integer
+    served: true
+    storage: true
