Fix #2802: Kubernetes Mock Server Crud Mode should reject invalid Kub…

…ernetes resources

Right now when a resource with no metadata is created, instead of
throwing KubernetesClientException, we're throwing a
NullPointerException here:
  KubernetesAttributesExtractor.extractMetadataAttributes:L154

KubernetesMockServer should validate whether object sent in request body
contains metadata or not

Note that this doesn't solve your issue when your using
KubernetesMockServer in expectations mode since you set what you'll be
getting as response by yourself

CHANGELOG.md

@@ -3,6 +3,7 @@
 ### 5.2-SNAPSHOT
 
 #### Bugs
+* Fix #2802: NullPointerException in HasMetadataOperation patch/replace when using KubernetesMockServer
 * Fix #2828: Remove automatic instantiation of CustomResource spec and status as this feature was causing more issues than it was solving
 * Fix #2857: Fix the log of an unexpected error from an Informer's EventHandler
 * Fix #2853: Cannot change the type of the Service from ClusterIP to ExternalName with PATCH

kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesAttributesExtractor.java

@@ -266,7 +266,7 @@ private static Attribute parseLabel(String label) {
     return null;
   }
 
-  private static HasMetadata toKubernetesResource(String s) {
+  static HasMetadata toKubernetesResource(String s) {
     try (InputStream stream = new ByteArrayInputStream(s.getBytes(StandardCharsets.UTF_8.name()))) {
       return Serialization.unmarshal(stream);
     } catch (Exception e) {

kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java

@@ -17,8 +17,14 @@
 
 import com.fasterxml.jackson.core.JsonProcessingException;
 import com.fasterxml.jackson.databind.JsonNode;
+import io.fabric8.kubernetes.api.model.HasMetadata;
+import io.fabric8.kubernetes.api.model.Status;
+import io.fabric8.kubernetes.api.model.StatusBuilder;
+import io.fabric8.kubernetes.api.model.StatusCause;
+import io.fabric8.kubernetes.api.model.StatusCauseBuilder;
 import io.fabric8.kubernetes.client.dsl.base.CustomResourceDefinitionContext;
 import io.fabric8.kubernetes.client.utils.Serialization;
+import io.fabric8.kubernetes.client.utils.Utils;
 import io.fabric8.mockwebserver.Context;
 import io.fabric8.mockwebserver.crud.Attribute;
 import io.fabric8.mockwebserver.crud.AttributeSet;
@@ -27,6 +33,7 @@
 import io.fabric8.zjsonpatch.JsonPatch;
 import okhttp3.mockwebserver.MockResponse;
 
+import java.io.IOException;
 import java.net.HttpURLConnection;
 import java.net.URI;
 import java.net.URISyntaxException;
@@ -36,12 +43,15 @@
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArraySet;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Supplier;
 
 import okhttp3.mockwebserver.RecordedRequest;
 import okhttp3.mockwebserver.SocketPolicy;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import static io.fabric8.kubernetes.client.server.mock.KubernetesAttributesExtractor.toKubernetesResource;
+
 public class KubernetesCrudDispatcher extends CrudDispatcher {
 
   private static final String POST = "POST";
@@ -51,6 +61,7 @@ public class KubernetesCrudDispatcher extends CrudDispatcher {
   private static final String DELETE = "DELETE";
 
   private static final Logger LOGGER = LoggerFactory.getLogger(KubernetesCrudDispatcher.class);
+  public static final int HTTP_UNPROCESSABLE_ENTITY = 422;
   private final Set<WatchEventsListener> watchEventListeners = new CopyOnWriteArraySet<>();
 
   public KubernetesCrudDispatcher() {
@@ -94,7 +105,7 @@ public synchronized MockResponse dispatch(RecordedRequest request) {
    */
   @Override
   public MockResponse handleCreate(String path, String s) {
-    return new MockResponse().setResponseCode(doCreate(path, s, "ADDED")).setBody(s);
+    return validateRequestBodyAndHandleRequest(s, () -> new MockResponse().setResponseCode(doCreate(path, s, "ADDED")).setBody(s));
   }
 
   /**
@@ -107,7 +118,7 @@ public MockResponse handleReplace(String path, String s) {
     if (doDelete(path, null) == 404) {
       return new MockResponse().setResponseCode(404);
     }
-    return new MockResponse().setResponseCode(doCreate(path, s, "MODIFIED")).setBody(s);
+    return validateRequestBodyAndHandleRequest(s, () -> new MockResponse().setResponseCode(doCreate(path, s, "MODIFIED")).setBody(s));
   }
 
   /**
@@ -316,4 +327,61 @@ private int doCreate(String path, String s, String event) {
     }
     return HttpURLConnection.HTTP_OK;
   }
+
+  private MockResponse validateRequestBodyAndHandleRequest(String s, Supplier<MockResponse> mockResponseSupplier) {
+    HasMetadata h = toKubernetesResource(s);
+    if (h != null) {
+      try {
+        validateResource(h);
+      } catch (IllegalArgumentException illegalArgumentException) {
+        return getUnprocessableEntityMockResponse(s, h, illegalArgumentException);
+      }
+    }
+    return mockResponseSupplier.get();
+  }
+
+  private MockResponse getUnprocessableEntityMockResponse(String s, HasMetadata h, IllegalArgumentException illegalArgumentException) {
+    String statusBody = getStatusBody(h, HTTP_UNPROCESSABLE_ENTITY, illegalArgumentException);
+    if (statusBody == null) {
+      statusBody = s;
+    }
+    return new MockResponse().setResponseCode(HTTP_UNPROCESSABLE_ENTITY).setBody(statusBody);
+  }
+
+  private String getStatusBody(HasMetadata h, int code, IllegalArgumentException illegalArgumentException) {
+    String kind = Utils.getNonNullOrElse(h.getKind(), "Unknown");
+    Status status = new StatusBuilder().withStatus("Failure")
+      .withReason("Invalid")
+      .withMessage(kind + " is invalid")
+      .withNewDetails()
+      .withKind(h.getKind())
+      .withCauses(getFailureStatusCause(illegalArgumentException))
+      .endDetails()
+      .withCode(code)
+      .build();
+    try {
+      return Serialization.jsonMapper().writeValueAsString(status);
+    } catch (IOException ioException) {
+      return null;
+    }
+  }
+
+  private StatusCause getFailureStatusCause(IllegalArgumentException illegalArgumentException) {
+    return new StatusCauseBuilder()
+      .withMessage(illegalArgumentException.getMessage())
+      .withReason("ValueRequired")
+      .build();
+  }
+
+  private void validateResource(HasMetadata item) {
+    if (item == null) {
+      throw new IllegalArgumentException("No item provided");
+    }
+    if (item.getMetadata() == null) {
+      throw new IllegalArgumentException("Required value: metadata is required");
+    }
+    if (Utils.isNullOrEmpty(item.getMetadata().getName()) && Utils.isNullOrEmpty(item.getMetadata().getGenerateName())) {
+      throw new IllegalArgumentException("Required value: name or generateName is required");
+    }
+  }
 }

kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/SecretCrudTest.java

@@ -20,13 +20,18 @@
 import io.fabric8.kubernetes.api.model.SecretBuilder;
 import io.fabric8.kubernetes.api.model.SecretList;
 import io.fabric8.kubernetes.client.KubernetesClient;
+import io.fabric8.kubernetes.client.KubernetesClientException;
+import io.fabric8.kubernetes.client.dsl.NonNamespaceOperation;
+import io.fabric8.kubernetes.client.dsl.Resource;
 import io.fabric8.kubernetes.client.server.mock.KubernetesServer;
 import org.junit.Rule;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.migrationsupport.rules.EnableRuleMigrationSupport;
 
 import java.util.Collections;
 
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -79,6 +84,24 @@ public void testCrud() {
 
     secret2 = client.secrets().inNamespace("ns2").withName("secret2").edit(s -> new SecretBuilder(s).removeFromData("one").build());
     assertNotNull(secret2);
-    assertEquals(null, secret2.getData());
+    assertNull(secret2.getData());
+  }
+
+  @Test
+  void testSecretWithNoMetadataCreateFails() {
+    // Given
+    Secret invalidSecret = new SecretBuilder()
+      .addToData("key.json", "{\"foo\":\"bar\"}")
+      .build();
+    KubernetesClient client = server.getClient();
+
+    // When + Then
+    NonNamespaceOperation<Secret, SecretList, Resource<Secret>> secretOp = client.secrets().inNamespace("foo");
+    KubernetesClientException kubernetesClientException = assertThrows(KubernetesClientException.class, () -> secretOp.create(invalidSecret));
+    assertEquals(422, kubernetesClientException.getCode());
+    assertEquals("Secret is invalid", kubernetesClientException.getStatus().getMessage());
+    assertEquals(1, kubernetesClientException.getStatus().getDetails().getCauses().size());
+    assertEquals("ValueRequired", kubernetesClientException.getStatus().getDetails().getCauses().get(0).getReason());
+    assertEquals("Required value: metadata is required", kubernetesClientException.getStatus().getDetails().getCauses().get(0).getMessage());
   }
 }