From ea92dba62b24efe63f2c83908109a354bd4ef0e0 Mon Sep 17 00:00:00 2001 From: Andre Dietisheim Date: Mon, 29 Apr 2024 15:48:19 +0200 Subject: [PATCH] fix: use #create/replace not #patch when resource has managed fields (#755) Signed-off-by: Andre Dietisheim --- .../kubernetes/editor/ClusterResource.kt | 6 ++- .../kubernetes/model/context/ActiveContext.kt | 4 ++ .../model/context/IActiveContext.kt | 13 +++++- .../NonCachingSingleResourceOperator.kt | 31 ++++++++++--- .../kubernetes/model/util/ResourceUtils.kt | 4 ++ .../kubernetes/editor/ClusterResourceTest.kt | 15 ++++++- .../NonCachingSingleResourceOperatorTest.kt | 32 +++++++++++-- .../model/util/ResourceUtilsTest.kt | 45 +++++++++++++++++++ 8 files changed, 136 insertions(+), 14 deletions(-) diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResource.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResource.kt index 9404c7bbf..87d4aa53c 100644 --- a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResource.kt +++ b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResource.kt @@ -140,7 +140,11 @@ open class ClusterResource protected constructor( "Unsupported resource kind ${resource.kind} in version ${resource.apiVersion}." ) } - val updated = context.replace(resource) + val updated = if (exists()) { + context.replace(resource) + } else { + context.create(resource) + } set(updated) return updated } catch (e: KubernetesClientException) { diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/ActiveContext.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/ActiveContext.kt index b189dcfb4..ced25a804 100644 --- a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/ActiveContext.kt +++ b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/ActiveContext.kt @@ -266,6 +266,10 @@ abstract class ActiveContext( return singleResourceOperator.get(resource) } + override fun create(resource: HasMetadata): HasMetadata? { + return singleResourceOperator.create(resource) + } + override fun replace(resource: HasMetadata): HasMetadata? { return singleResourceOperator.replace(resource) } diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/IActiveContext.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/IActiveContext.kt index 44465e082..37abc2621 100644 --- a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/IActiveContext.kt +++ b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/context/IActiveContext.kt @@ -142,12 +142,21 @@ interface IActiveContext: IContext { fun get(resource: HasMetadata): HasMetadata? /** - * Replaces the given resource on the cluster if it exists. Creates a new one if it doesn't. + * Creates the given resource on the cluster if it doesn't exist. Throws if it exists already. * - * @param resource that shall be replaced on the cluster + * @param resource that shall be created on the cluster * * @return the resource that was created */ + fun create(resource: HasMetadata): HasMetadata? + + /** + * Replaces the given resource on the cluster if it exists. Throws if it doesn't. + * + * @param resource that shall be replaced on the cluster + * + * @return the resource that was replaced + */ fun replace(resource: HasMetadata): HasMetadata? /** diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperator.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperator.kt index e26c4399c..57b3cfc63 100644 --- a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperator.kt +++ b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperator.kt @@ -13,6 +13,7 @@ package com.redhat.devtools.intellij.kubernetes.model.resource import com.redhat.devtools.intellij.kubernetes.model.client.ClientAdapter import com.redhat.devtools.intellij.kubernetes.model.util.ResourceException import com.redhat.devtools.intellij.kubernetes.model.util.hasGenerateName +import com.redhat.devtools.intellij.kubernetes.model.util.hasManagedFields import com.redhat.devtools.intellij.kubernetes.model.util.hasName import com.redhat.devtools.intellij.kubernetes.model.util.runWithoutServerSetProperties import io.fabric8.kubernetes.api.model.APIResource @@ -84,26 +85,44 @@ class NonCachingSingleResourceOperator( val genericKubernetesResource = toGenericKubernetesResource(resource, true) val op = createOperation(resource) return if (hasName(genericKubernetesResource)) { - patch(genericKubernetesResource, op) + if (hasManagedFields(genericKubernetesResource)) { + patch(genericKubernetesResource, op, PatchType.JSON_MERGE) + } else { + patch(genericKubernetesResource, op, PatchType.SERVER_SIDE_APPLY) + } } else if (hasGenerateName(genericKubernetesResource)) { - op.resource(genericKubernetesResource) - .create() + op.resource(genericKubernetesResource).create() } else { throw ResourceException("Could not replace ${resource.kind ?: "resource"}: has neither name nor generateName.") } } + fun create(resource: HasMetadata): HasMetadata? { + // force clone, patch changes the given resource + val genericKubernetesResource = toGenericKubernetesResource(resource, true) + val op = createOperation(resource) + return if (hasName(genericKubernetesResource) + && !hasManagedFields(genericKubernetesResource) + ) { + patch(genericKubernetesResource, op, PatchType.SERVER_SIDE_APPLY) + } else { + op.resource(genericKubernetesResource) + .create() + } + } + private fun patch( genericKubernetesResource: GenericKubernetesResource, - op: NonNamespaceOperation> + op: NonNamespaceOperation>, + patchType: PatchType ): HasMetadata? { return runWithoutServerSetProperties(genericKubernetesResource) { op .resource(genericKubernetesResource) .patch( PatchContext.Builder() - .withForce(true) - .withPatchType(PatchType.SERVER_SIDE_APPLY) + //.withForce(true) + .withPatchType(patchType) .build() ) } diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtils.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtils.kt index f0821d74c..87530fe62 100644 --- a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtils.kt +++ b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtils.kt @@ -172,6 +172,10 @@ fun hasDeletionTimestamp(resource: HasMetadata?): Boolean { return null != resource?.metadata?.deletionTimestamp } +fun hasManagedFields(resource: HasMetadata?): Boolean { + return true == resource?.metadata?.managedFields?.isNotEmpty() +} + fun setWillBeDeleted(resource: HasMetadata) { setDeletionTimestamp(MARKER_WILL_BE_DELETED, resource) } diff --git a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResourceTest.kt b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResourceTest.kt index 81ea12d27..7a2f12c5d 100644 --- a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResourceTest.kt +++ b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/editor/ClusterResourceTest.kt @@ -125,10 +125,21 @@ class ClusterResourceTest { } @Test - fun `#push should call operator#replace`() { + fun `#push should call operator#create if resource does NOT exist`() { // given whenever(context.get(any())) - .doReturn(null) + .thenReturn(null) + // when + cluster.push(endorResourceOnCluster) + // then + verify(context).create(endorResourceOnCluster) + } + + @Test + fun `#push should call operator#replace if resource exists`() { + // given + whenever(context.get(any())) + .thenReturn(endorResourceOnCluster) // when cluster.push(endorResourceOnCluster) // then diff --git a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt index dd1172ad6..eb101c0d2 100644 --- a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt +++ b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt @@ -28,6 +28,7 @@ import io.fabric8.kubernetes.api.model.APIResource import io.fabric8.kubernetes.api.model.GenericKubernetesResource import io.fabric8.kubernetes.api.model.GenericKubernetesResourceList import io.fabric8.kubernetes.api.model.HasMetadata +import io.fabric8.kubernetes.api.model.ManagedFieldsEntry import io.fabric8.kubernetes.api.model.ObjectMetaBuilder import io.fabric8.kubernetes.api.model.PodBuilder import io.fabric8.kubernetes.client.KubernetesClientException @@ -210,9 +211,13 @@ class NonCachingSingleResourceOperatorTest { } @Test - fun `#replace should call #patch() if resource has a name`() { + fun `#replace should call #patch(JSON_MERGE) if resource has a name and managed fields`() { // given - val hasName = PodBuilder(namespacedCoreResource).build() + val hasName = PodBuilder(namespacedCoreResource) + .withNewMetadata() + .withManagedFields(ManagedFieldsEntry()) + .endMetadata() + .build() hasName.metadata.name = "yoda" hasName.metadata.generateName = null val apiResource = namespacedApiResource(namespacedCoreResource) @@ -222,10 +227,31 @@ class NonCachingSingleResourceOperatorTest { // then verify(resourceOp) .patch(argThat(ArgumentMatcher { context -> - context.patchType == PatchType.SERVER_SIDE_APPLY + context.patchType == PatchType.JSON_MERGE })) } + @Test + fun `#replace should call #patch(SERVER_SIDE_APPLY) if resource has a name and NO managed fields`() { + // given + val metadata = ObjectMetaBuilder() + .build() + metadata.managedFields = null + val hasName = PodBuilder(namespacedCoreResource) + .withMetadata(metadata) + .build() + hasName.metadata.name = "yoda" + hasName.metadata.generateName = null + val apiResource = namespacedApiResource(namespacedCoreResource) + val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource)) + // when + operator.replace(hasName) + // then + verify(resourceOp) + .patch(argThat(ArgumentMatcher { context -> + context.patchType == PatchType.SERVER_SIDE_APPLY + })) + } @Test fun `#replace should call #create() if resource has NO name but has generateName`() { // given diff --git a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtilsTest.kt b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtilsTest.kt index 7ce0bed16..5f07b4d3d 100644 --- a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtilsTest.kt +++ b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/util/ResourceUtilsTest.kt @@ -12,6 +12,7 @@ package com.redhat.devtools.intellij.kubernetes.model.util import com.redhat.devtools.intellij.kubernetes.model.mocks.ClientMocks.resource import io.fabric8.kubernetes.api.model.HasMetadata +import io.fabric8.kubernetes.api.model.ManagedFieldsEntry import io.fabric8.kubernetes.api.model.ObjectMetaBuilder import io.fabric8.kubernetes.api.model.Pod import io.fabric8.kubernetes.api.model.PodBuilder @@ -525,4 +526,48 @@ class ResourceUtilsTest { assertThat(resource2.metadata.resourceVersion).isEqualTo("21") assertThat(resource2.metadata.uid).isEqualTo("obiwan") } + + @Test + fun `#hasManagedField should return true if resource has managed fields `() { + // given + val meta = ObjectMetaBuilder() + .withManagedFields(ManagedFieldsEntry()) + .build() + val neo = PodBuilder() + .withMetadata(meta) + .build() + // when + val hasManagedFields = hasManagedFields(neo) + // then + assertThat(hasManagedFields).isTrue() + } + + @Test + fun `#hasManagedField should return false if resource has empty list of managed fields `() { + // given + val meta = ObjectMetaBuilder() + .build() + meta.managedFields = emptyList() + val neo = PodBuilder() + .withMetadata(meta) + .build() + // when + val hasManagedFields = hasManagedFields(neo) + // then + assertThat(hasManagedFields).isFalse() + } + + @Test + fun `#hasManagedField should return false if resource has no managed fields `() { + // given + val meta = ObjectMetaBuilder().build() + val neo = PodBuilder() + .withMetadata(meta) + .build() + // when + val hasManagedFields = hasManagedFields(neo) + // then + assertThat(hasManagedFields).isFalse() + } + } \ No newline at end of file