From b3083a9ecdfe1f85445976a39d7820e09a9734d5 Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Thu, 11 Mar 2021 22:54:59 +0530 Subject: [PATCH] Fix #2802: Kubernetes Mock Server Crud Mode should reject invalid Kubernetes 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 | 1 + .../mock/KubernetesAttributesExtractor.java | 2 +- .../server/mock/KubernetesCrudDispatcher.java | 72 ++++++++++++++++++- .../client/mock/SecretCrudTest.java | 25 ++++++- 4 files changed, 96 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 610f51321cf..c1a0ebbafdb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesAttributesExtractor.java b/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesAttributesExtractor.java index 9af6c4ae576..dcc167ad29c 100644 --- a/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesAttributesExtractor.java +++ b/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) { diff --git a/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java b/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java index 645cbd3cbcf..74804b58949 100644 --- a/kubernetes-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java +++ b/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 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 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"); + } + } } diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/SecretCrudTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/SecretCrudTest.java index c3ec51e4492..d82e15cc734 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/SecretCrudTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/SecretCrudTest.java @@ -20,6 +20,9 @@ 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; @@ -27,6 +30,8 @@ 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> 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()); } }