From aa651fad8850793637cbdc831003c06c02726eed Mon Sep 17 00:00:00 2001 From: manusa Date: Tue, 1 Sep 2020 16:49:12 +0200 Subject: [PATCH] fix: ConfigMap and other resources are replaced - Reverted **behavior** of BaseOperation#createOrReplace method (and similar) to the previous behavior. API will always be hit, POST to try to create, and PUT in case POST fails. This reverts the behavior trying to mimic kubectl and not performing the second PUT request in case the resource is identical to the server version. --- CHANGELOG.md | 1 + .../client/dsl/base/BaseOperation.java | 180 ++++++------------ ...WatchDeleteRecreateWaitApplicableImpl.java | 10 - ...hDeleteRecreateWaitApplicableListImpl.java | 6 - .../mock/PersistentVolumeClaimTest.java | 123 ++++++++---- .../client/mock/ResourceListTest.java | 82 ++++---- .../kubernetes/client/mock/ResourceTest.java | 14 +- 7 files changed, 187 insertions(+), 229 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a37aeca523..7719e9dbe99 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### 4.11-SNAPSHOT #### Bugs +* Fix #2445: ConfigMap and other resources are replaced #### Improvements diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java index e860e0b0f41..92d5e6a5ab8 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/base/BaseOperation.java @@ -16,7 +16,6 @@ package io.fabric8.kubernetes.client.dsl.base; import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; -import io.fabric8.kubernetes.client.utils.ResourceCompare; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -131,26 +130,8 @@ protected BaseOperation(OperationContext ctx) { this.watchRetryBackoffMultiplier = ctx.getWatchRetryBackoffMultiplier(); } - /** - * Returns the name and falls back to the item name. - * @param item The item. - * @param name The name to check. - * @param - * @return - */ - private static String name(T item, String name) { - if (name != null && !name.isEmpty()) { - return name; - } else if (item instanceof HasMetadata) { - HasMetadata h = (HasMetadata) item; - return h.getMetadata() != null ? h.getMetadata().getName() : null; - } - return null; - } - - public BaseOperation newInstance(OperationContext context) { - return new BaseOperation(context); + return new BaseOperation<>(context); } /** @@ -188,14 +169,8 @@ private void addQueryStringParam(HttpUrl.Builder requestUrlBuilder, String name, @Override public T get() { try { - T answer = getMandatory(); - if (answer instanceof HasMetadata) { - HasMetadata hasMetadata = (HasMetadata) answer; - updateApiVersion(hasMetadata); - } else if (answer instanceof KubernetesResourceList) { - KubernetesResourceList list = (KubernetesResourceList) answer; - updateApiVersion(list); - } + final T answer = getMandatory(); + updateApiVersion(answer); return answer; } catch (KubernetesClientException e) { if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) { @@ -206,19 +181,13 @@ public T get() { } @Override - public T require() throws ResourceNotFoundException { + public T require() { try { T answer = getMandatory(); if (answer == null) { throw new ResourceNotFoundException("The resource you request doesn't exist or couldn't be fetched."); } - if (answer instanceof HasMetadata) { - HasMetadata hasMetadata = (HasMetadata) answer; - updateApiVersion(hasMetadata); - } else if (answer instanceof KubernetesResourceList) { - KubernetesResourceList list = (KubernetesResourceList) answer; - updateApiVersion(list); - } + updateApiVersion(answer); return answer; } catch (KubernetesClientException e) { if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) { @@ -265,7 +234,7 @@ public RootPaths getRootPaths() { } @Override - public D edit() throws KubernetesClientException { + public D edit() { throw new KubernetesClientException("Cannot edit read-only resources"); } @@ -332,8 +301,9 @@ public Gettable fromServer() { return newInstance(context.withReloadingFromServer(true)); } + @SafeVarargs @Override - public T create(T... resources) throws KubernetesClientException { + public final T create(T... resources) { try { if (resources.length > 1) { throw new IllegalArgumentException("Too many items to create."); @@ -363,7 +333,7 @@ public T create(T resource) { } @Override - public D createNew() throws KubernetesClientException { + public D createNew() { final Function visitor = resource -> { try { return create(resource); @@ -381,7 +351,7 @@ public D createNew() throws KubernetesClientException { @Override - public D createOrReplaceWithNew() throws KubernetesClientException { + public D createOrReplaceWithNew() { final Function visitor = resource -> { try { return createOrReplace(resource); @@ -397,8 +367,9 @@ public D createOrReplaceWithNew() throws KubernetesClientException { } } + @SafeVarargs @Override - public T createOrReplace(T... items) { + public final T createOrReplace(T... items) { T itemToCreateOrReplace = getItem(); if (items.length > 1) { throw new IllegalArgumentException("Too many items to create."); @@ -423,16 +394,10 @@ public T createOrReplace(T... items) { if (exception.getCode() != HttpURLConnection.HTTP_CONFLICT) { throw exception; } - // Conflict; Do Replace - T itemFromServer = fromServer().get(); - if (ResourceCompare.equals(itemFromServer, itemToCreateOrReplace)) { - // Nothing changed, ignore - return itemToCreateOrReplace; - } else { - KubernetesResourceUtil.setResourceVersion(itemToCreateOrReplace, KubernetesResourceUtil.getResourceVersion(itemFromServer)); - return replace(itemToCreateOrReplace); - } + final T itemFromServer = fromServer().get(); + KubernetesResourceUtil.setResourceVersion(itemToCreateOrReplace, KubernetesResourceUtil.getResourceVersion(itemFromServer)); + return replace(itemToCreateOrReplace); } } @@ -474,29 +439,28 @@ public FilterWatchListDeletable> withLabelSelec return this; } - // Deprecated as the underlying implementation does not align with the arguments anymore. - // It is possible to negate multiple values with the same key, e.g.: - // foo != bar , foo != baz - // To support this a multi-value map is needed, as a regular map would override the key with the new value. + /** + * @deprecated as the underlying implementation does not align with the arguments anymore. + * It is possible to negate multiple values with the same key, e.g.: + * foo != bar , foo != baz + * To support this a multi-value map is needed, as a regular map would override the key with the new value. + */ @Override @Deprecated - public FilterWatchListDeletable> withoutLabels(Map labels) throws - KubernetesClientException { + public FilterWatchListDeletable> withoutLabels(Map labels) { // Re-use "withoutLabel" to convert values from String to String[] labels.forEach(this::withoutLabel); return this; } @Override - public FilterWatchListDeletable> withLabelIn(String key, String... values) throws - KubernetesClientException { + public FilterWatchListDeletable> withLabelIn(String key, String... values) { labelsIn.put(key, values); return this; } @Override - public FilterWatchListDeletable> withLabelNotIn(String key, String... values) throws - KubernetesClientException { + public FilterWatchListDeletable> withLabelNotIn(String key, String... values) { labelsNotIn.put(key, values); return this; } @@ -540,16 +504,17 @@ public FilterWatchListDeletable> withField(Stri return this; } - // Deprecated as the underlying implementation does not align with the arguments fully. - // Method is created to have a similar API as `withoutLabels`, but should eventually be replaced with something - // better for the same reasons. - // It is possible to negate multiple values with the same key, e.g.: - // foo != bar , foo != baz - // To support this a multi-value map is needed, as a regular map would override the key with the new value. + /** + * @deprecated as the underlying implementation does not align with the arguments fully. + * Method is created to have a similar API as `withoutLabels`, but should eventually be replaced + * with something better for the same reasons. + * It is possible to negate multiple values with the same key, e.g.: + * foo != bar , foo != baz + * To support this a multi-value map is needed, as a regular map would override the key with the new value. + */ @Override @Deprecated - public FilterWatchListDeletable> withoutFields(Map fields) throws - KubernetesClientException { + public FilterWatchListDeletable> withoutFields(Map fields) { // Re-use "withoutField" to convert values from String to String[] labels.forEach(this::withoutField); return this; @@ -655,7 +620,7 @@ public String getFieldQueryParam() { return sb.toString(); } - public L list() throws KubernetesClientException { + public L list() { try { return listRequestHelper(getResourceUrl(namespace, name)); } catch (IOException e) { @@ -700,9 +665,9 @@ public Boolean delete() { } } - + @SafeVarargs @Override - public Boolean delete(T... items) { + public final Boolean delete(T... items) { return delete(Arrays.asList(items)); } @@ -710,25 +675,20 @@ public Boolean delete(T... items) { public Boolean delete(List items) { boolean deleted = true; if (items != null) { - for (T item : items) { - if (item == null) { + for (T toDelete : items) { + if (toDelete == null) { continue; } - updateApiVersionResource(item); + updateApiVersion(toDelete); try { - R op; - - if (item instanceof HasMetadata - && ((HasMetadata) item).getMetadata() != null - && ((HasMetadata) item).getMetadata().getName() != null - && !((HasMetadata) item).getMetadata().getName().isEmpty()) { - op = (R) inNamespace(checkNamespace(item)).withName(((HasMetadata) item).getMetadata().getName()); + if (toDelete.getMetadata() != null + && toDelete.getMetadata().getName() != null + && !toDelete.getMetadata().getName().isEmpty()) { + deleted &= inNamespace(checkNamespace(toDelete)).withName(toDelete.getMetadata().getName()).delete(); } else { - op = (R) withItem(item); + deleted &= withItem(toDelete).delete(); } - - deleted &= op.delete(); } catch (KubernetesClientException e) { if (e.getCode() != HttpURLConnection.HTTP_NOT_FOUND) { throw e; @@ -756,7 +716,7 @@ public BaseOperation withItem(T item) { void deleteThis() { try { if (item != null) { - updateApiVersionResource(item); + updateApiVersion(item); handleDelete(item, gracePeriodSeconds, propagationPolicy, cascading); } else { handleDelete(getResourceUrl(), gracePeriodSeconds, propagationPolicy, cascading); @@ -858,27 +818,27 @@ public boolean isResourceNamespaced() { return Utils.isResourceNamespaced(getType()); } - protected T handleResponse(Request.Builder requestBuilder) throws ExecutionException, InterruptedException, KubernetesClientException, IOException { + protected T handleResponse(Request.Builder requestBuilder) throws ExecutionException, InterruptedException, IOException { return handleResponse(requestBuilder, getType()); } - protected T handleCreate(T resource) throws ExecutionException, InterruptedException, KubernetesClientException, IOException { - updateApiVersionResource(resource); + protected T handleCreate(T resource) throws ExecutionException, InterruptedException, IOException { + updateApiVersion(resource); return handleCreate(resource, getType()); } - protected T handleReplace(T updated) throws ExecutionException, InterruptedException, KubernetesClientException, IOException { - updateApiVersionResource(updated); + protected T handleReplace(T updated) throws ExecutionException, InterruptedException, IOException { + updateApiVersion(updated); return handleReplace(updated, getType()); } - protected T handlePatch(T current, T updated) throws ExecutionException, InterruptedException, KubernetesClientException, IOException { - updateApiVersionResource(updated); + protected T handlePatch(T current, T updated) throws ExecutionException, InterruptedException, IOException { + updateApiVersion(updated); return handlePatch(current, updated, getType()); } protected T handlePatch(T current, Map patchedUpdate) throws ExecutionException, InterruptedException, IOException { - updateApiVersionResource(current); + updateApiVersion(current); return handlePatch(current, patchedUpdate, getType()); } @@ -911,7 +871,7 @@ protected Status handleDeploymentRollback(DeploymentRollback deploymentRollback) protected T handleGet(URL resourceUrl) throws InterruptedException, ExecutionException, IOException { T answer = handleGet(resourceUrl, getType()); - updateApiVersionResource(answer); + updateApiVersion(answer); return answer; } @@ -1033,42 +993,19 @@ protected Class getConfigType() { return Config.class; } - /** - * Updates the list or single item if it has a missing or incorrect apiGroupVersion - * - * @param resource resource object - */ - protected void updateApiVersionResource(Object resource) { - if (resource instanceof HasMetadata) { - HasMetadata hasMetadata = (HasMetadata) resource; - updateApiVersion(hasMetadata); - } else if (resource instanceof KubernetesResourceList) { - KubernetesResourceList list = (KubernetesResourceList) resource; - updateApiVersion(list); - } - } - /** * Updates the list items if they have missing or default apiGroupVersion values and the resource is currently * using API Groups with custom version strings * * @param list Kubernetes resource list */ - protected void updateApiVersion(KubernetesResourceList list) { + protected void updateApiVersion(KubernetesResourceList list) { String version = getApiVersion(); - if (list != null && version != null && version.length() > 0) { - List items = list.getItems(); - if (items != null) { - for (Object item : items) { - if (item instanceof HasMetadata) { - updateApiVersion((HasMetadata) item); - } - } - } + if (list != null && version != null && version.length() > 0 && list.getItems() != null) { + list.getItems().forEach(this::updateApiVersion); } } - /** * Updates the resource if it has missing or default apiGroupVersion values and the resource is currently * using API Groups with custom version strings @@ -1102,8 +1039,7 @@ public boolean isApiGroup() { @Override public Boolean isReady() { - T i = get(); - return i instanceof HasMetadata && Readiness.isReady((HasMetadata) i); + return Readiness.isReady(get()); } @Override diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java index 6aca6b72a38..2012bfef257 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.java @@ -23,9 +23,6 @@ import java.net.HttpURLConnection; import java.util.function.Predicate; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -58,7 +55,6 @@ import io.fabric8.kubernetes.client.dsl.base.OperationSupport; import io.fabric8.kubernetes.client.handlers.KubernetesListHandler; import io.fabric8.kubernetes.client.internal.readiness.Readiness; -import io.fabric8.kubernetes.client.utils.ResourceCompare; import okhttp3.OkHttpClient; public class NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl extends OperationSupport implements @@ -66,8 +62,6 @@ public class NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl ex Waitable, Readiable { - private static final Logger LOGGER = LoggerFactory.getLogger(NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl.class); - private final String fallbackNamespace; private final String explicitNamespace; @@ -153,16 +147,12 @@ public HasMetadata createOrReplace() { } // Conflict; check deleteExisting flag otherwise replace - HasMetadata r = h.reload(client, config, meta.getMetadata().getNamespace(), meta); if (Boolean.TRUE.equals(deletingExisting)) { Boolean deleted = h.delete(client, config, namespaceToUse, propagationPolicy, meta); if (Boolean.FALSE.equals(deleted)) { throw new KubernetesClientException("Failed to delete existing item:" + meta); } return h.create(client, config, namespaceToUse, meta); - } else if (ResourceCompare.equals(r, meta)) { - LOGGER.debug("Item has not changed. Skipping"); - return meta; } else { KubernetesResourceUtil.setResourceVersion(meta, resourceVersion); return h.replace(client, config, namespaceToUse, meta); diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java index 5b5bc951e7e..51406545368 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/dsl/internal/NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl.java @@ -37,14 +37,12 @@ import io.fabric8.kubernetes.client.handlers.KubernetesListHandler; import io.fabric8.kubernetes.client.internal.readiness.Readiness; import io.fabric8.kubernetes.client.utils.KubernetesResourceUtil; -import io.fabric8.kubernetes.client.utils.ResourceCompare; import io.fabric8.kubernetes.client.utils.Serialization; import io.fabric8.kubernetes.client.utils.Utils; import io.fabric8.openshift.api.model.Parameter; import io.fabric8.openshift.api.model.Template; import java.net.HttpURLConnection; -import java.util.concurrent.RejectedExecutionException; import java.util.function.Predicate; import okhttp3.OkHttpClient; import org.slf4j.Logger; @@ -276,15 +274,12 @@ public List createOrReplace() { } // Conflict; check deleteExisting flag otherwise replace - HasMetadata r = h.reload(client, config, meta.getMetadata().getNamespace(), meta); if (Boolean.TRUE.equals(deletingExisting)) { Boolean deleted = h.delete(client, config, namespaceToUse, propagationPolicy, meta); if (Boolean.FALSE.equals(deleted)) { throw new KubernetesClientException("Failed to delete existing item:" + meta); } result.add(h.create(client, config, namespaceToUse, meta)); - } else if (ResourceCompare.equals(r, meta)) { - LOGGER.debug("Item has not changed. Skipping"); } else { KubernetesResourceUtil.setResourceVersion(meta, resourceVersion); result.add(h.replace(client, config, namespaceToUse, meta)); @@ -299,7 +294,6 @@ public Waitable, HasMetadata> createOrReplaceAnd() { return new NamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableListImpl(client, config, fallbackNamespace, explicitNamespace, fromServer, deletingExisting, visitors, createOrReplace(), inputStream, null, gracePeriodSeconds, propagationPolicy, cascading, watchRetryInitialBackoffMillis, watchRetryBackoffMultiplier); } - @Override public Boolean delete() { //First pass check before deleting diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/PersistentVolumeClaimTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/PersistentVolumeClaimTest.java index ed55179b4c6..1b063229d81 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/PersistentVolumeClaimTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/PersistentVolumeClaimTest.java @@ -30,12 +30,15 @@ import org.junit.Rule; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.junit.jupiter.migrationsupport.rules.EnableRuleMigrationSupport; import java.net.HttpURLConnection; import java.util.Collections; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -44,9 +47,17 @@ @EnableRuleMigrationSupport class PersistentVolumeClaimTest { + @Rule public KubernetesServer server = new KubernetesServer(); + private KubernetesClient client; + + @BeforeEach + void setUp() { + client = server.getClient(); + } + @Test void testList() { server.expect().withPath("/api/v1/namespaces/test/persistentvolumeclaims").andReturn(200, new PersistentVolumeClaimListBuilder().build()).once(); @@ -56,7 +67,6 @@ void testList() { .addNewItem().and() .build()).once(); - KubernetesClient client = server.getClient(); PersistentVolumeClaimList persistentVolumeClaimList = client.persistentVolumeClaims().inNamespace("test").list(); assertNotNull(persistentVolumeClaimList); assertEquals(0, persistentVolumeClaimList.getItems().size()); @@ -76,7 +86,6 @@ void testListWithlabels() { .addNewItem().and() .build()).once(); - KubernetesClient client = server.getClient(); PersistentVolumeClaimList persistentVolumeClaimList = client.persistentVolumeClaims().inNamespace("test") .withLabel("key1", "value1") .withLabel("key2","value2") @@ -98,7 +107,6 @@ void testGet() { server.expect().withPath("/api/v1/namespaces/test/persistentvolumeclaims/persistentvolumeclaim1").andReturn(200, new PersistentVolumeClaimBuilder().build()).once(); server.expect().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims/persistentvolumeclaim2").andReturn(200, new PersistentVolumeClaimBuilder().build()).once(); - KubernetesClient client = server.getClient(); PersistentVolumeClaim persistentVolumeClaim = client.persistentVolumeClaims().inNamespace("test").withName("persistentvolumeclaim1").get(); assertNotNull(persistentVolumeClaim); @@ -122,7 +130,6 @@ void testDelete() { server.expect().withPath("/api/v1/namespaces/test/persistentvolumeclaims/persistentvolumeclaim1").andReturn(200, new PersistentVolumeClaimBuilder().build()).once(); server.expect().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims/persistentvolumeclaim2").andReturn(200, new PersistentVolumeClaimBuilder().build()).once(); - KubernetesClient client = server.getClient(); Boolean deleted = client.persistentVolumeClaims().inNamespace("test").withName("persistentvolumeclaim1").delete(); assertTrue(deleted); @@ -142,7 +149,6 @@ void testDeleteMulti() { server.expect().withPath("/api/v1/namespaces/test/persistentvolumeclaims/persistentvolumeclaim1").andReturn(200, persistentVolumeClaim1).once(); server.expect().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims/persistentvolumeclaim2").andReturn(200, persistentVolumeClaim2).once(); - KubernetesClient client = server.getClient(); Boolean deleted = client.persistentVolumeClaims().inAnyNamespace().delete(persistentVolumeClaim1, persistentVolumeClaim2); assertTrue(deleted); @@ -152,7 +158,6 @@ void testDeleteMulti() { @Test void testLoadFromFile() { - KubernetesClient client = server.getClient(); PersistentVolumeClaim persistentVolumeClaim = client.persistentVolumeClaims().load(getClass().getResourceAsStream("/test-persistentvolumeclaim.yml")).get(); assertEquals("task-pv-claim", persistentVolumeClaim.getMetadata().getName()); } @@ -172,54 +177,88 @@ void testBuild() { server.expect().withPath("/api/v1/namespaces/test/persistentvolumeclaims/test-pv-claim").andReturn(200, persistentVolumeClaim).once(); - KubernetesClient client = server.getClient(); persistentVolumeClaim = client.persistentVolumeClaims().inNamespace("test").withName("test-pv-claim").get(); assertNotNull(persistentVolumeClaim); } @Test - void testCreateOrReplaceIgnoreWhenNoChange() { + @DisplayName("createOrReplace, resource is new, should successfully POST to server") + void testCreateOrReplaceWhenNotExists() { // Given - PersistentVolumeClaim pvcOrignal = new PersistentVolumeClaimBuilder() - .withNewMetadata().withName("my-pvc").endMetadata() - .withNewSpec() - .withAccessModes("ReadWriteOnce") - .withNewResources() - .withRequests(Collections.singletonMap("storage", new Quantity("20Gi"))) - .endResources() - .endSpec() - .build(); - PersistentVolumeClaim pvcFromServer = new PersistentVolumeClaimBuilder() - .withNewMetadata() - .withName("my-pvc") - .withCreationTimestamp("2020-07-27T11:02:15Z") - .addToFinalizers("kubernetes.io/pvc-protection") - .withNamespace("default") - .withResourceVersion("203697") - .withSelfLink("/api/v1/namespaces/default/persistentvolumeclaims/my-pvc") - .withUid("60817eaa-19d8-41ba-adb4-3ea75860e1f8") + server.expect().post().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims") + .andReturn(HttpURLConnection.HTTP_CREATED, mockPVCBuilder().build()).once(); + // @formatter:off + final PersistentVolumeClaim toCreate = mockPVCBuilder() + .editMetadata() + .withResourceVersion(null) + .withCreationTimestamp(null) .endMetadata() - .withNewSpec() - .withAccessModes("ReadWriteOnce") - .withNewResources() - .withRequests(Collections.singletonMap("storage", new Quantity("20Gi"))) - .endResources() - .withStorageClassName("standard") - .withVolumeMode("Filesystem") - .withVolumeName("pvc-60817eaa-19d8-41ba-adb4-3ea75860e1f8") - .endSpec() .build(); + // @formatter:on + // When + final PersistentVolumeClaim result = client.persistentVolumeClaims().inNamespace("ns1") + .createOrReplace(toCreate); + // Then + assertThat(result) + .isNotNull() + .extracting("metadata") + .hasFieldOrPropertyWithValue("resourceVersion", "1") + .extracting("creationTimestamp").asString().isNotBlank(); + } + + @Test + @DisplayName("createOrReplace, resource exists on server, should PUT updated resource") + void testCreateOrReplaceWhenExists() { + // Given + final PersistentVolumeClaim existent = mockPVCBuilder().build(); server.expect().post().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims") - .andReturn(HttpURLConnection.HTTP_CONFLICT, pvcFromServer).once(); + .andReturn(HttpURLConnection.HTTP_CONFLICT, "").once(); server.expect().get().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims/my-pvc") - .andReturn(HttpURLConnection.HTTP_OK, pvcFromServer).once(); - KubernetesClient client = server.getClient(); - + .andReturn(HttpURLConnection.HTTP_OK, existent).times(2); + server.expect().put().withPath("/api/v1/namespaces/ns1/persistentvolumeclaims/my-pvc") + .andReturn(HttpURLConnection.HTTP_OK, mockPVCBuilder() + .editMetadata().withResourceVersion("2").endMetadata().build()).once(); + // @formatter:off + final PersistentVolumeClaim toReplace = mockPVCBuilder() + .editMetadata() + .withResourceVersion(null) + .withCreationTimestamp(null) + .endMetadata() + .build(); + // @formatter:on // When - PersistentVolumeClaim pvcResult = client.persistentVolumeClaims().inNamespace("ns1").createOrReplace(pvcOrignal); - + final PersistentVolumeClaim result = client.persistentVolumeClaims().inNamespace("ns1") + .createOrReplace(toReplace); // Then - assertNotNull(pvcResult); + assertThat(result) + .isNotNull() + .extracting("metadata") + .hasFieldOrPropertyWithValue("resourceVersion", "2") + .hasFieldOrPropertyWithValue("creationTimestamp", existent.getMetadata().getCreationTimestamp()); + } + + private static PersistentVolumeClaimBuilder mockPVCBuilder() { + // @formatter:off + return new PersistentVolumeClaimBuilder() + .withNewMetadata() + .withName("my-pvc") + .withCreationTimestamp("2020-07-27T11:02:15Z") + .addToFinalizers("kubernetes.io/pvc-protection") + .withNamespace("ns1") + .withResourceVersion("1") + .withSelfLink("/api/v1/namespaces/default/persistentvolumeclaims/my-pvc") + .withUid("60817eaa-19d8-41ba-adb4-3ea75860e1f8") + .endMetadata() + .withNewSpec() + .withAccessModes("ReadWriteOnce") + .withNewResources() + .withRequests(Collections.singletonMap("storage", new Quantity("20Gi"))) + .endResources() + .withStorageClassName("standard") + .withVolumeMode("Filesystem") + .withVolumeName("pvc-60817eaa-19d8-41ba-adb4-3ea75860e1f8") + .endSpec(); + // @formatter:on } } diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceListTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceListTest.java index c11a55845a1..820be4ef498 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceListTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceListTest.java @@ -34,6 +34,7 @@ import okhttp3.mockwebserver.RecordedRequest; import org.junit.Rule; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.migrationsupport.rules.EnableRuleMigrationSupport; @@ -52,29 +53,25 @@ public class ResourceListTest { @Rule public KubernetesServer server = new KubernetesServer(); - Service service = new ServiceBuilder() - .withNewMetadata().withName("my-service").endMetadata() - .withNewSpec() - .addToSelector("app", "Myapp") - .addNewPort().withProtocol("TCP").withPort(80).withTargetPort(new IntOrString(9376)).endPort() - .endSpec() - .build(); - Service updatedService = new ServiceBuilder(service) - .withNewSpec() - .addToSelector("app", "Myapp") - .addNewPort().withProtocol("TCP").withPort(80).withTargetPort(new IntOrString(9999)).endPort() - .endSpec() - .build(); - - - ConfigMap configMap = new ConfigMapBuilder() - .withNewMetadata().withName("my-configmap").endMetadata() - .addToData(Collections.singletonMap("app", "Myapp")) - .build(); - ConfigMap updatedConfigMap = new ConfigMapBuilder(configMap) - .addToData(Collections.singletonMap("foo", "bar")) - .build(); - + private KubernetesClient client; + private Service service; + private Service updatedService; + private ConfigMap configMap; + private ConfigMap updatedConfigMap; + private KubernetesList resourcesToUpdate; + + @BeforeEach + void setUp() { + client = server.getClient(); + service = mockService().build(); + configMap = mockConfigMap().build(); + updatedService = mockService().editSpec().editFirstPort() + .withTargetPort(new IntOrString(9999)).endPort().endSpec() + .build(); + updatedConfigMap = mockConfigMap().addToData(Collections.singletonMap("foo", "bar")).build(); + resourcesToUpdate = new KubernetesListBuilder() + .withItems(updatedService, updatedConfigMap).build(); + } @Test void testCreateOrReplace() { @@ -82,7 +79,6 @@ void testCreateOrReplace() { server.expect().post().withPath("/api/v1/namespaces/test/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); - KubernetesClient client = server.getClient(); List response = client.resourceList(new PodListBuilder().addToItems(pod1).build()).createOrReplace(); assertTrue(response.contains(pod1)); } @@ -92,7 +88,6 @@ void testCreateOrReplaceFailedCreate() { // Given Pod pod1 = new PodBuilder().withNewMetadata().withName("pod1").withNamespace("test").and().build(); server.expect().post().withPath("/api/v1/namespaces/test/pods").andReturn(HttpURLConnection.HTTP_UNAVAILABLE, pod1).once(); - KubernetesClient client = server.getClient(); NamespaceListVisitFromServerGetDeleteRecreateWaitApplicable listOp = client.resourceList(new PodListBuilder().addToItems(pod1).build()); // When @@ -105,7 +100,6 @@ void testCreateWithExplicitNamespace() { server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); - KubernetesClient client = server.getClient(); List response = client.resourceList(new PodListBuilder().addToItems(pod1).build()).inNamespace("ns1").apply(); assertTrue(response.contains(pod1)); } @@ -121,8 +115,6 @@ void testDelete() { server.expect().withPath("/api/v1/namespaces/ns1/pods/pod2").andReturn(HttpURLConnection.HTTP_OK, pod2).times(2); server.expect().withPath("/api/v1/namespaces/any/pods/pod3").andReturn(HttpURLConnection.HTTP_OK, pod3).times(1); - KubernetesClient client = server.getClient(); - //First time all items should be deleted. Boolean deleted = client.resourceList(new PodListBuilder().withItems(pod1, pod2, pod3).build()).delete(); assertTrue(deleted); @@ -136,16 +128,14 @@ void testDelete() { void testCreateOrReplaceWithoutDeleteExisting() throws Exception { server.expect().post().withPath("/api/v1/namespaces/ns1/services").andReturn(HttpURLConnection.HTTP_CONFLICT, service).once(); server.expect().post().withPath("/api/v1/namespaces/ns1/configmaps").andReturn(HttpURLConnection.HTTP_CONFLICT, configMap).once(); - server.expect().get().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HttpURLConnection.HTTP_OK , service).times(2); - server.expect().get().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HttpURLConnection.HTTP_OK, configMap).times(2); + server.expect().get().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HttpURLConnection.HTTP_OK , service).once(); + server.expect().get().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HttpURLConnection.HTTP_OK, configMap).once(); server.expect().put().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HttpURLConnection.HTTP_OK, updatedService).once(); server.expect().put().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HttpURLConnection.HTTP_OK, updatedConfigMap).once(); - KubernetesClient client = server.getClient(); - KubernetesList list = new KubernetesListBuilder().withItems(updatedService, updatedConfigMap).build(); - client.resourceList(list).inNamespace("ns1").createOrReplace(); + client.resourceList(resourcesToUpdate).inNamespace("ns1").createOrReplace(); - assertEquals(8, server.getMockServer().getRequestCount()); + assertEquals(6, server.getMockServer().getRequestCount()); RecordedRequest request = server.getLastRequest(); assertEquals("/api/v1/namespaces/ns1/configmaps/my-configmap", request.getPath()); assertEquals("PUT", request.getMethod()); @@ -155,21 +145,31 @@ void testCreateOrReplaceWithoutDeleteExisting() throws Exception { void testCreateOrReplaceWithDeleteExisting() throws Exception { server.expect().post().withPath("/api/v1/namespaces/ns1/services").andReturn(HttpURLConnection.HTTP_CONFLICT, service).once(); server.expect().post().withPath("/api/v1/namespaces/ns1/configmaps").andReturn(HttpURLConnection.HTTP_CONFLICT, configMap).once(); - server.expect().get().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HttpURLConnection.HTTP_OK , service).once(); - server.expect().get().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HttpURLConnection.HTTP_OK, configMap).once(); server.expect().delete().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HttpURLConnection.HTTP_OK , service).once(); server.expect().delete().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HttpURLConnection.HTTP_OK, configMap).once(); server.expect().post().withPath("/api/v1/namespaces/ns1/services").andReturn(HttpURLConnection.HTTP_OK, updatedService).once(); server.expect().post().withPath("/api/v1/namespaces/ns1/configmaps").andReturn(HttpURLConnection.HTTP_OK, updatedConfigMap).once(); - KubernetesClient client = server.getClient(); - KubernetesList list = new KubernetesListBuilder().withItems(updatedService, updatedConfigMap).build(); - client.resourceList(list).inNamespace("ns1").deletingExisting().createOrReplace(); - + client.resourceList(resourcesToUpdate).inNamespace("ns1").deletingExisting().createOrReplace(); - assertEquals(8, server.getMockServer().getRequestCount()); + assertEquals(6, server.getMockServer().getRequestCount()); RecordedRequest request = server.getLastRequest(); assertEquals("/api/v1/namespaces/ns1/configmaps", request.getPath()); assertEquals("POST", request.getMethod()); } + + private static ServiceBuilder mockService() { + return new ServiceBuilder() + .withNewMetadata().withName("my-service").endMetadata() + .withNewSpec() + .addToSelector("app", "my-app") + .addNewPort().withProtocol("TCP").withPort(80).withTargetPort(new IntOrString(9376)).endPort() + .endSpec(); + } + + private static ConfigMapBuilder mockConfigMap() { + return new ConfigMapBuilder() + .withNewMetadata().withName("my-configmap").endMetadata() + .addToData(Collections.singletonMap("app", "my-app")); + } } diff --git a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java index 3996d628d98..b3944881bac 100644 --- a/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java +++ b/kubernetes-tests/src/test/java/io/fabric8/kubernetes/client/mock/ResourceTest.java @@ -62,7 +62,6 @@ public class ResourceTest { @Rule public KubernetesServer server = new KubernetesServer(); - @Test void testCreateOrReplace() { // Given @@ -91,13 +90,13 @@ void testCreateOrReplaceWhenCreateFails() { @Test void testCreateWithExplicitNamespace() { - Pod pod1 = new PodBuilder().withNewMetadata().withName("pod1").withNamespace("test").and().build(); + Pod pod1 = new PodBuilder().withNewMetadata().withName("pod1").withNamespace("test").and().build(); - server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); + server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); - KubernetesClient client = server.getClient(); - HasMetadata response = client.resource(pod1).inNamespace("ns1").createOrReplace(); - assertEquals(pod1, response); + KubernetesClient client = server.getClient(); + HasMetadata response = client.resource(pod1).inNamespace("ns1").createOrReplace(); + assertEquals(pod1, response); } @Test @@ -105,7 +104,6 @@ void testCreateOrReplaceWithDeleteExisting() throws Exception { Pod pod1 = new PodBuilder().withNewMetadata().withName("pod1").withNamespace("test").and().build(); server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CONFLICT, pod1).once(); - server.expect().get().withPath("/api/v1/namespaces/ns1/pods/pod1").andReturn(HttpURLConnection.HTTP_OK, pod1).once(); server.expect().delete().withPath("/api/v1/namespaces/ns1/pods/pod1").andReturn(HttpURLConnection.HTTP_OK, pod1).once(); server.expect().post().withPath("/api/v1/namespaces/ns1/pods").andReturn(HttpURLConnection.HTTP_CREATED, pod1).once(); @@ -114,7 +112,7 @@ void testCreateOrReplaceWithDeleteExisting() throws Exception { assertEquals(pod1, response); RecordedRequest request = server.getLastRequest(); - assertEquals(4, server.getMockServer().getRequestCount()); + assertEquals(3, server.getMockServer().getRequestCount()); assertEquals("/api/v1/namespaces/ns1/pods", request.getPath()); assertEquals("POST", request.getMethod()); }