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

…755)

Signed-off-by: Andre Dietisheim <adietish@redhat.com>

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) {

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)
     }

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?
 
     /**

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
@@ -35,13 +36,17 @@ import io.fabric8.kubernetes.client.utils.Serialization
 import java.net.HttpURLConnection
 
 /**
- * Offers remoting operations like [get], [replace], [watch] to
+ * Offers remoting operations like [get], [create],  [replace], [watch] to
  * retrieve, create, replace or watch a resource on the current cluster.
  * API discovery is executed and a [KubernetesClientException] is thrown if resource kind and version are not supported.
  */
 class NonCachingSingleResourceOperator(
     private val client: ClientAdapter<out KubernetesClient>,
-    private val api: APIResources = APIResources(client)
+    private val api: APIResources = APIResources(client),
+    private val genericResourceFactory: (HasMetadata) -> GenericKubernetesResource = { resource ->
+        val yaml = Serialization.asYaml(resource)
+        Serialization.unmarshal(yaml, GenericKubernetesResource::class.java)
+    }
 ) {
 
     /**
@@ -84,26 +89,51 @@ 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.STRATEGIC_MERGE)
+            } else {
+                patch(genericKubernetesResource, op, PatchType.SERVER_SIDE_APPLY)
+            }
         } else if (hasGenerateName(genericKubernetesResource)) {
-            op.resource(genericKubernetesResource)
-                .create()
+            create(genericKubernetesResource, op)
         } else {
             throw ResourceException("Could not replace ${resource.kind ?: "resource"}: has neither name nor generateName.")
         }
     }
 
-    private fun patch(
+    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 {
+            create(genericKubernetesResource, op)
+        }
+    }
+
+    private fun create(
         genericKubernetesResource: GenericKubernetesResource,
         op: NonNamespaceOperation<GenericKubernetesResource, GenericKubernetesResourceList, Resource<GenericKubernetesResource>>
+    ): GenericKubernetesResource? =
+        runWithoutServerSetProperties(genericKubernetesResource) {
+            op.resource(genericKubernetesResource).create()
+        }
+
+    private fun patch(
+        genericKubernetesResource: 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()
                 )
         }
@@ -181,8 +211,7 @@ class NonCachingSingleResourceOperator(
             && !clone) {
             resource
         } else {
-            val yaml = Serialization.asYaml(resource)
-            Serialization.unmarshal(yaml, GenericKubernetesResource::class.java)
+            genericResourceFactory.invoke(resource)
         }
     }
 

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)
 }

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

src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/resource/NonCachingSingleResourceOperatorTest.kt

@@ -18,6 +18,7 @@ import com.nhaarman.mockitokotlin2.doReturn
 import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.never
 import com.nhaarman.mockitokotlin2.spy
+import com.nhaarman.mockitokotlin2.times
 import com.nhaarman.mockitokotlin2.verify
 import com.nhaarman.mockitokotlin2.whenever
 import com.redhat.devtools.intellij.kubernetes.model.client.KubeClientAdapter
@@ -28,6 +29,8 @@ 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.ObjectMeta
 import io.fabric8.kubernetes.api.model.ObjectMetaBuilder
 import io.fabric8.kubernetes.api.model.PodBuilder
 import io.fabric8.kubernetes.client.KubernetesClientException
@@ -185,6 +188,84 @@ class NonCachingSingleResourceOperatorTest {
         // then
     }
 
+    @Test
+    fun `#create should call #patch(SERVER_SIDE_APPLY) if resource has a name and NO managed fields`() {
+        // given
+        val metadata = ObjectMetaBuilder().build().apply {
+            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.create(hasName)
+        // then
+        verify(resourceOp)
+            .patch(argThat(ArgumentMatcher<PatchContext> { context ->
+                context.patchType == PatchType.SERVER_SIDE_APPLY
+            }))
+    }
+
+    @Test
+    fun `#create should call #create if resource has no name`() {
+        // given
+        val hasNoName = PodBuilder(namespacedCoreResource)
+            .withNewMetadata()
+                .withManagedFields(ManagedFieldsEntry())
+            .endMetadata()
+            .build()
+        val apiResource = namespacedApiResource(namespacedCoreResource)
+        val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
+        // when
+        operator.create(hasNoName)
+        // then
+        verify(resourceOp)
+            .create()
+    }
+
+    @Test
+    fun `#create should remove resourceVersion and uid before calling #create`() {
+        // given
+        val metadata = mock<ObjectMeta>()
+        val genericResource = mock<GenericKubernetesResource> {
+            on { getMetadata() } doReturn metadata
+        }
+        val hasNoName = PodBuilder(namespacedCoreResource)
+            .withMetadata(metadata)
+            .build()
+        val apiResource = namespacedApiResource(namespacedCoreResource)
+        val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
+            { resource -> genericResource }
+        // when
+        operator.create(hasNoName)
+        // then
+        verify(resourceOp).create() // make sure #create was called as this only applies when #create is called
+        verify(metadata, times(2)).setResourceVersion(null) //
+        verify(metadata, times(2)).setUid(null)
+    }
+
+    @Test
+    fun `#create should call #create if resource has a name but managed fields`() {
+        // given
+        val hasNameAndManagedFields = PodBuilder(namespacedCoreResource)
+            .withNewMetadata()
+                .withManagedFields(ManagedFieldsEntry())
+                .withName("obiwan")
+            .endMetadata()
+            .build()
+        val apiResource = namespacedApiResource(namespacedCoreResource)
+        val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
+        // when
+        operator.create(hasNameAndManagedFields)
+        // then
+        verify(resourceOp)
+            .create()
+    }
+
     @Test
     fun `#replace should call #inNamespace for namespaced resource`() {
         // given
@@ -210,11 +291,40 @@ class NonCachingSingleResourceOperatorTest {
     }
 
     @Test
-    fun `#replace should call #patch() if resource has a name`() {
+    fun `#replace should call #patch(STRATEGIC_MERGE) if resource has a name and managed fields`() {
         // given
-        val hasName = PodBuilder(namespacedCoreResource).build()
-        hasName.metadata.name = "yoda"
-        hasName.metadata.generateName = null
+        val metadata = ObjectMetaBuilder()
+            .withManagedFields(ManagedFieldsEntry())
+            .build().apply {
+                name = "yoda"
+                generateName = null
+            }
+        val hasName = PodBuilder(namespacedCoreResource)
+            .withMetadata(metadata)
+            .build()
+        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.STRATEGIC_MERGE
+            }))
+    }
+
+    @Test
+    fun `#replace should call #patch(SERVER_SIDE_APPLY) if resource has a name and NO managed fields`() {
+        // given
+        val metadata = ObjectMetaBuilder()
+            .build().apply {
+                name = "yoda"
+                generateName = null
+                managedFields = null
+            }
+        val hasName = PodBuilder(namespacedCoreResource)
+            .withMetadata(metadata)
+            .build()
         val apiResource = namespacedApiResource(namespacedCoreResource)
         val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
         // when
@@ -229,9 +339,10 @@ class NonCachingSingleResourceOperatorTest {
     @Test
     fun `#replace should call #create() if resource has NO name but has generateName`() {
         // given
-        val hasGeneratedName = PodBuilder(namespacedCoreResource).build()
-        hasGeneratedName.metadata.name = null
-        hasGeneratedName.metadata.generateName = "storm trooper clone"
+        val hasGeneratedName = PodBuilder(namespacedCoreResource).build().apply {
+            metadata.name = null
+            metadata.generateName = "storm trooper clone"
+        }
         val operator = NonCachingSingleResourceOperator(
             clientAdapter,
             createAPIResources(namespacedApiResource(hasGeneratedName))
@@ -246,9 +357,10 @@ class NonCachingSingleResourceOperatorTest {
     @Test(expected = ResourceException::class)
     fun `#replace should throw if resource has NO name NOR generateName`() {
         // given
-        val generatedName = PodBuilder(namespacedCoreResource).build()
-        generatedName.metadata.name = null
-        generatedName.metadata.generateName = null
+        val generatedName = PodBuilder(namespacedCoreResource).build().apply {
+            metadata.name = null
+            metadata.generateName = null
+        }
         val apiResource = namespacedApiResource(namespacedCustomResource)
         val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
         // when
@@ -306,8 +418,10 @@ class NonCachingSingleResourceOperatorTest {
     @Test
     fun `#watch should return NULL if resource has NO name`() {
         // given
-        val unnamed = PodBuilder(namespacedCoreResource).build()
-        unnamed.metadata.name = null
+        val unnamed = PodBuilder(namespacedCoreResource).build().apply {
+            metadata.name = null
+        }
+
         val apiResource = namespacedApiResource(unnamed)
         val operator = NonCachingSingleResourceOperator(clientAdapter, createAPIResources(apiResource))
         // when