fix: refactor: bump kubernetes-client to 6.12.0 (redhat-developer#713) (

redhat-developer#730) * bump kubernetes-client to 6.10 (redhat-developer#713) * remove watches even if closing failed * dont dead lock when switching ns/refreshing cluster * bumped kubernetes-client to 6.12.0 (redhat-developer#713) Signed-off-by: Andre Dietisheim <[email protected]>
adietish · Apr 16, 2024 · c996c6c · c996c6c
1 parent 9f97baa
commit c996c6c
Show file tree

Hide file tree

Showing 11 changed files with 63 additions and 60 deletions.
diff --git a/build.gradle b/build.gradle
@@ -1,5 +1,4 @@
 buildscript {
-
     ext.ideaVersion = ideaVersion
     ext.kotlinVersion = kotlinVersion
     ext.java_version = "17"
@@ -42,17 +41,17 @@ dependencies {
             "io.fabric8:kubernetes-model-common:${kubernetesClientVersion}",
             "io.fabric8:openshift-client:${kubernetesClientVersion}",
             "io.fabric8:kubernetes-httpclient-okhttp:${kubernetesClientVersion}",
+            "com.fasterxml.jackson.core:jackson-core:2.17.0", /* IC-2022.3 ships 2.16.0 */
             "org.apache.commons:commons-lang3:3.12.0"
     )
     testImplementation(
             "org.assertj:assertj-core:3.22.0",
             "org.mockito:mockito-inline:4.5.1",
             "com.nhaarman.mockitokotlin2:mockito-kotlin:2.2.0",
-            "org.jetbrains.kotlin:kotlin-test-junit:${kotlinVersion}",
-            "org.yaml:snakeyaml:1.33" /* IC-2023.2 provides incompatible 2.0 */
+            "org.jetbrains.kotlin:kotlin-test-junit:${kotlinVersion}"
     )
     integrationTestImplementation(
-            "com.redhat.devtools.intellij:intellij-common:1.1.0",
+            "com.redhat.devtools.intellij:intellij-common:${intellijCommonVersion}",
             "com.redhat.devtools.intellij:intellij-common-ui-test-library:0.2.0",
             "org.junit.jupiter:junit-jupiter-engine:5.8.2",
             "org.junit.jupiter:junit-jupiter-api:5.8.2",

diff --git a/gradle.properties b/gradle.properties
@@ -2,14 +2,14 @@ ideaVersion=IC-2024.1
 # build number ranges
 # https://www.jetbrains.org/intellij/sdk/docs/basics/getting_started/build_number_ranges.html
 sinceIdeaBuild=223
-projectVersion=1.2.3
+projectVersion=1.2.4-SNAPSHOT
 jetBrainsToken=invalid
 jetBrainsChannel=stable
 intellijPluginVersion=1.16.1
 kotlinJvmPluginVersion=1.8.0
-intellijCommonVersion=1.9.4-SNAPSHOT
+intellijCommonVersion=1.9.4
 telemetryPluginVersion=1.1.0.52
 kotlin.stdlib.default.dependency = false
 kotlinVersion=1.6.21
-kubernetesClientVersion=6.4.0
+kubernetesClientVersion=6.12.0
 fixturesVersion=1.1.18
diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/AllContexts.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/AllContexts.kt
@@ -22,7 +22,6 @@ import com.redhat.devtools.intellij.kubernetes.model.context.IActiveContext
 import com.redhat.devtools.intellij.kubernetes.model.context.IContext
 import com.redhat.devtools.intellij.kubernetes.model.resource.ResourceKind
 import com.redhat.devtools.intellij.kubernetes.model.util.ResettableLazyProperty
-import com.redhat.devtools.intellij.kubernetes.model.util.ResourceException
 import com.redhat.devtools.intellij.kubernetes.telemetry.TelemetryService
 import com.redhat.devtools.intellij.kubernetes.telemetry.TelemetryService.NAME_PREFIX_CONTEXT
 import com.redhat.devtools.intellij.kubernetes.telemetry.TelemetryService.PROP_IS_OPENSHIFT
@@ -31,9 +30,11 @@ import com.redhat.devtools.intellij.kubernetes.telemetry.TelemetryService.PROP_O
 import io.fabric8.kubernetes.api.model.HasMetadata
 import io.fabric8.kubernetes.client.Config
 import io.fabric8.kubernetes.client.KubernetesClient
-import io.fabric8.kubernetes.client.KubernetesClientException
 import java.nio.file.Paths
 import java.util.concurrent.CompletionException
+import java.util.concurrent.locks.ReentrantReadWriteLock
+import kotlin.concurrent.read
+import kotlin.concurrent.write
 
 interface IAllContexts {
 	/**
@@ -89,25 +90,29 @@ open class AllContexts(
 		watchKubeConfig()
 	}
 
+	private val lock = ReentrantReadWriteLock()
+
 	private val client = ResettableLazyProperty {
-		clientFactory.invoke(null,null)
+		lock.write {
+			clientFactory.invoke(null, null)
+		}
 	}
 
 	override val current: IActiveContext<out HasMetadata, out KubernetesClient>?
 		get() {
-			synchronized(this) {
-				return findActive(all)
-			}
+			return findActive(all)
 		}
 
-	override val all: MutableList<IContext> = mutableListOf()
+	private val _all: MutableList<IContext> = mutableListOf()
+
+	override val all: List<IContext>
 		get() {
-			synchronized(this) {
-				if (field.isEmpty()) {
+			lock.write {
+				if (_all.isEmpty()) {
 					val all = createContexts(client.get(), client.get()?.config)
-					field.addAll(all)
+						_all.addAll(all)
 				}
-				return field
+				return _all
 			}
 		}
 
@@ -137,36 +142,32 @@ open class AllContexts(
 		newClient: ClientAdapter<out KubernetesClient>,
 		toWatch: Collection<ResourceKind<out HasMetadata>>?,
 	) : IActiveContext<out HasMetadata, out KubernetesClient>? {
-		try {
-			synchronized(this) {
+		lock.write {
+			try {
 				replaceClient(newClient, this.client.get())
 				newClient.config.save().join()
 				current?.close()
-				all.clear() // causes reload of all contexts when accessed afterwards
+				clearAllContexts() // causes reload of all contexts when accessed afterwards
 				val newCurrent = current // gets new current from all
 				if (toWatch != null) {
 					newCurrent?.watchAll(toWatch)
 				}
 				return newCurrent
+			} catch (e: CompletionException) {
+				val cause = e.cause ?: throw e
+				throw cause
 			}
-		} catch (e: CompletionException) {
-			val cause = e.cause ?: throw e
-			throw cause
 		}
 	}
 
-	private fun throwIfNotAccessible(namespace: String, client: KubernetesClient) {
-		try {
-			client.namespaces()?.withName(namespace)?.isReady
-		} catch(e: KubernetesClientException) {
-			throw ResourceException("Namespace $namespace is not accessible", e)
-		}
+	private fun clearAllContexts() {
+		_all.clear()
 	}
 
 	override fun refresh() {
-		synchronized(this) {
+		lock.write {
 			this.current?.close()
-			all.clear() // latter access will cause reload
+			clearAllContexts() // latter access will cause reload
 		}
 		modelChange.fireAllContextsChanged()
 	}
@@ -187,14 +188,16 @@ open class AllContexts(
 		) {
 			return emptyList()
 		}
-		return config.allContexts
-			.map {
-				if (config.isCurrent(it)) {
-					createActiveContext(client) ?: Context(it)
-				} else {
-					Context(it)
+		lock.read {
+			return config.allContexts
+				.map {
+					if (config.isCurrent(it)) {
+						createActiveContext(client) ?: Context(it)
+					} else {
+						Context(it)
+					}
 				}
-			}
+		}
 	}
 
 	private fun replaceClient(new: ClientAdapter<out KubernetesClient>, old: ClientAdapter<out KubernetesClient>?)
@@ -249,7 +252,7 @@ open class AllContexts(
 	}
 
 	protected open fun onKubeConfigChanged(fileConfig: io.fabric8.kubernetes.api.model.Config?) {
-		synchronized(this) {
+		lock.read {
 			fileConfig ?: return
 			val client = client.get() ?: return
 			val clientConfig = client.config.configuration
@@ -258,8 +261,8 @@ open class AllContexts(
 			}
 			this.client.reset() // create new client when accessed
 			client.close()
-			refresh()
 		}
+		refresh()
 	}
 
 	/** for testing purposes */

diff --git a/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/ResourceWatch.kt b/src/main/kotlin/com/redhat/devtools/intellij/kubernetes/model/ResourceWatch.kt
@@ -113,7 +113,8 @@ open class ResourceWatch<T>(
             true
         } catch (e: Exception) {
             logger<ResourceWatch<*>>().warn("Error when closing watch for $type resources.", e)
-            false
+            // do as if close() worked so that watch gets removed
+            true
         }
     }
 

diff --git a/...tlin/com/redhat/devtools/intellij/kubernetes/model/resource/NamespacedResourceOperator.kt b/...tlin/com/redhat/devtools/intellij/kubernetes/model/resource/NamespacedResourceOperator.kt
@@ -117,7 +117,7 @@ abstract class NamespacedResourceOperator<R : HasMetadata, C: Client>(
             client.adapt(KubernetesClient::class.java)
                 .resource(toReplace)
                 .inNamespace(inNamespace)
-                .replace()
+                .patch()
         }
     }
 

diff --git a/...n/com/redhat/devtools/intellij/kubernetes/model/resource/NonNamespacedResourceOperator.kt b/...n/com/redhat/devtools/intellij/kubernetes/model/resource/NonNamespacedResourceOperator.kt
@@ -87,7 +87,7 @@ abstract class NonNamespacedResourceOperator<R : HasMetadata, C : Client>(
         return runWithoutServerSetProperties(toReplace) {
             client.adapt(KubernetesClient::class.java)
                 .resource(toReplace)
-                .replace()
+                .patch()
         }
     }
 

diff --git a/.../intellij/kubernetes/model/resource/kubernetes/custom/NamespacedCustomResourceOperator.kt b/.../intellij/kubernetes/model/resource/kubernetes/custom/NamespacedCustomResourceOperator.kt
@@ -86,7 +86,7 @@ open class NamespacedCustomResourceOperator(
 			getOperation()
 				?.inNamespace(inNamespace)
 				?.resource(toReplace)
-				?.createOrReplace()
+				?.patch()
 		}
 	}
 

diff --git a/...tellij/kubernetes/model/resource/kubernetes/custom/NonNamespacedCustomResourceOperator.kt b/...tellij/kubernetes/model/resource/kubernetes/custom/NonNamespacedCustomResourceOperator.kt
@@ -72,7 +72,10 @@ class NonNamespacedCustomResourceOperator(
         return runWithoutServerSetProperties(toReplace) {
             getOperation()
                 ?.resource(resource)
-                ?.createOrReplace()
+                /**
+                 * See: https://github.com/fabric8io/kubernetes-client/blob/main/doc/FAQ.md#alternatives-to-createOrReplace-and-replace
+                 */
+                ?.patch()
         }
     }
 

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
@@ -16,9 +16,6 @@ import io.fabric8.kubernetes.api.model.apiextensions.v1.CustomResourceDefinition
 import io.fabric8.kubernetes.client.utils.ApiVersionUtil
 import io.fabric8.kubernetes.client.utils.KubernetesVersionPriority
 import io.fabric8.kubernetes.client.utils.Serialization
-import io.fabric8.kubernetes.model.annotation.Group
-import io.fabric8.kubernetes.model.annotation.Version
-import io.fabric8.kubernetes.model.util.Helper
 import java.util.stream.Collectors
 
 const val MARKER_WILL_BE_DELETED = "willBeDeleted"
@@ -128,13 +125,13 @@ fun String.isGreaterIntThan(other: String?): Boolean {
  * @see io.fabric8.kubernetes.model.annotation.Group (annotation)
  */
 fun getApiVersion(clazz: Class<out HasMetadata>): String {
-	val apiVersion = Helper.getAnnotationValue(clazz, Version::class.java)
-	return if (!apiVersion.isNullOrBlank()) {
-		val apiGroup = Helper.getAnnotationValue(clazz, Group::class.java)
-		if (!apiGroup.isNullOrBlank()) {
-			getApiVersion(apiGroup, apiVersion)
+	val version = HasMetadata.getVersion(clazz)
+	return if (!version.isNullOrBlank()) {
+		val group = HasMetadata.getGroup(clazz)
+		if (!group.isNullOrBlank()) {
+			getApiVersion(group, version)
 		} else {
-			apiVersion
+			version
 		}
 	} else {
 		clazz.simpleName

diff --git a/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/mocks/ClientMocks.kt b/src/test/kotlin/com/redhat/devtools/intellij/kubernetes/model/mocks/ClientMocks.kt
@@ -130,15 +130,15 @@ object ClientMocks {
     ) {
         val inNamespaceOp: Resource<HasMetadata> = mock {
             on { delete() } doReturn statusDetails
-            on { replace() } doReturn resource
+            on { patch() } doReturn resource
             on { create() } doReturn resource
             on { get() } doReturn resource
         }
         /** [KubernetesClient.resource] */
         val resourceOperation: NamespaceableResource<HasMetadata> = mock {
             on { inNamespace(any()) } doReturn inNamespaceOp
             on { delete() } doReturn statusDetails
-            on { replace() } doReturn resource
+            on { patch() } doReturn resource
         }
 
         doReturn(resourceOperation)

diff --git a/...dhat/devtools/intellij/kubernetes/model/resource/kubernetes/NamespacedPodsOperatorTest.kt b/...dhat/devtools/intellij/kubernetes/model/resource/kubernetes/NamespacedPodsOperatorTest.kt
@@ -381,7 +381,7 @@ class NamespacedPodsOperatorTest {
         verify(client.get().adapt(KubernetesClient::class.java)
             .resource(toReplace)
             .inNamespace(toReplace.metadata.namespace))
-            .replace()
+            .patch()
     }
 
     @Test
@@ -424,7 +424,7 @@ class NamespacedPodsOperatorTest {
         verify(client.get().adapt(KubernetesClient::class.java)
             .resource(toReplace)
             .inNamespace(toReplace.metadata.namespace))
-            .replace()
+            .patch()
     }
 
     @Test
@@ -436,7 +436,7 @@ class NamespacedPodsOperatorTest {
         whenever(client.get().adapt(KubernetesClient::class.java)
             .resource(toReplace)
             .inNamespace(toReplace.metadata.namespace)
-            .replace())
+            .patch())
             .thenReturn(POD3)
         // when
         val newPod = operator.replace(toReplace)
-Original file line number
+Diff line change
@@ Expand Up / @@ -113,7 +113,8 @@ open class ResourceWatch<T>( @@
                 true
             } catch (e: Exception) {
                 logger<ResourceWatch<*>>().warn("Error when closing watch for $type resources.", e)
-                false
+                // do as if close() worked so that watch gets removed
+                true
             }
         }
@@ Expand Down @@