fix: use #create/replace not #patch when resource has managed fields (#…

…755) Signed-off-by: Andre Dietisheim <[email protected]>
redhat-developer · Apr 29, 2024 · ea92dba · ea92dba
1 parent a0c4512
commit ea92dba
Show file tree

Hide file tree

Showing 8 changed files with 136 additions and 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
@@ -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
@@ -266,6 +266,10 @@ abstract class ActiveContext<N : HasMetadata, C : KubernetesClient>(
         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
@@ -142,12 +142,21 @@ interface IActiveContext<N: HasMetadata, C: KubernetesClient>: 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/...om/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperator.kt b/...om/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<GenericKubernetesResource, GenericKubernetesResourceList, Resource<GenericKubernetesResource>>
+        op: NonNamespaceOperation<GenericKubernetesResource, GenericKubernetesResourceList, Resource<GenericKubernetesResource>>,
+        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
@@ -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
@@ -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/...edhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt b/...edhat/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<PatchContext> { 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<PatchContext> { 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
@@ -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()
+	}
+
 }