From 84145053e8337f20c196388f1412286e194c0ba0 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 02:56:55 +0100 Subject: [PATCH 1/9] build: update the Kubernetes client to 28.0.0 Move io.kubernetes:client-java from 11.0.4 to 28.0.0 (Kubernetes 1.37), with SnakeYAML 2 instead of 1.33, and set the versions of the libraries that the client needs and Spring Boot 2.7 manages lower (OkHttp, Kotlin, Gson, Commons Lang, Commons Codec). Drop the pins of protobuf-java, commons-io, commons-compress and bcprov-ext-jdk15on: the client asks for the same versions, or nothing needs them any more. - Calls in KubernetesClientWrapper and KubernetesObjectsSupplier use the request-builder API; the models use java.time instead of Joda-Time. - KubernetesJson: a Gson that ignores unknown fields, as the client did up to version 19, since its models now refuse objects of newer Kubernetes versions (kubernetes-client/java#4941). - The merge-patch client keeps the "Bearer" prefix of a kubeconfig token, which it dropped, so patches failed outside a cluster. - ServiceInfoService builds its YAML constructor with LoaderOptions (SnakeYAML 2). - A task whose taskmaster job has no creation time gets none, rather than the current time. Co-Authored-By: Claude Opus 5.5 --- pom.xml | 35 +- .../uk/ac/ebi/tsc/tesk/config/GsonConfig.java | 4 +- .../k8s/config/KubernetesClientConfig.java | 19 +- .../tsc/tesk/k8s/config/KubernetesJson.java | 42 ++ .../convert/KubernetesObjectsSupplier.java | 4 +- .../tesk/k8s/convert/TaskStateResolver.java | 7 +- .../k8s/convert/TesKubernetesConverter.java | 21 +- .../k8s/service/KubernetesClientWrapper.java | 79 ++-- .../tesk/tes/service/ServiceInfoService.java | 2 + .../java/uk/ac/ebi/tsc/tesk/TestUtils.java | 34 ++ .../config/KubernetesClientConfigTest.java | 67 +++ .../tesk/k8s/config/KubernetesJsonTest.java | 49 +++ .../convert/IgnoredBackendParametersTest.java | 2 +- .../k8s/convert/TaskStateResolverTest.java | 8 +- .../TesKubernetesConverterMinimalTest.java | 36 +- .../convert/TesKubernetesConverterTest.java | 19 +- .../service/KubernetesClientWrapperTest.java | 394 ++++++------------ .../tes/service/TesServiceImplCreateTest.java | 2 +- 18 files changed, 453 insertions(+), 371 deletions(-) create mode 100644 src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfigTest.java create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java diff --git a/pom.xml b/pom.xml index 81e454c1..fbe25f9d 100644 --- a/pom.xml +++ b/pom.xml @@ -25,7 +25,7 @@ 21 3.0.0 - 11.0.4 + 28.0.0 33.8.0-jre - 1.33 + + 2.7 + 5.5.0 + 2.1.21 + 2.14.0 + 3.21.0 + 1.23.0 3.6.1 3.12.0 4.6.0 @@ -50,21 +56,6 @@ spring-security-oauth2 2.5.2.RELEASE - - com.google.protobuf - protobuf-java - 4.36.2 - - - commons-io - commons-io - 2.22.0 - - - org.apache.commons - commons-compress - 1.28.0 - org.bitbucket.b_c jose4j @@ -75,12 +66,8 @@ bcprov-jdk15on 1.70 - - org.bouncycastle - bcprov-ext-jdk15on - 1.70 - - + org.bouncycastle bcpkix-jdk15on diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/config/GsonConfig.java b/src/main/java/uk/ac/ebi/tsc/tesk/config/GsonConfig.java index 94fc1996..1473e6e7 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/config/GsonConfig.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/config/GsonConfig.java @@ -1,9 +1,9 @@ package uk.ac.ebi.tsc.tesk.config; import com.google.gson.Gson; -import io.kubernetes.client.openapi.JSON; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; +import uk.ac.ebi.tsc.tesk.k8s.config.KubernetesJson; /** @author Ania Niewielska */ @Configuration @@ -11,6 +11,6 @@ public class GsonConfig { @Bean Gson gson() { - return new JSON().getGson(); + return KubernetesJson.GSON; } } diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java index d6f85999..8af1fe3c 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java @@ -15,6 +15,11 @@ @Configuration public class KubernetesClientConfig { + static { + // read objects of Kubernetes versions newer than the client + KubernetesJson.install(); + } + @Bean @Primary public ApiClient kubernetesApiClient() throws IOException { @@ -38,13 +43,13 @@ public CoreV1Api coreApi(ApiClient apiClient) { /** * Workaround to enable merge patching (Kube client does JSON patch only - headers hardcoded) - * Separate instance of ApiClient only for running PATCH calls + * Separate instance of ApiClient only for running PATCH calls, with the connection and + * credentials of the default one */ @Bean(name = "patchApiClient") - public ApiClient patchApiClient() throws IOException { + public ApiClient patchApiClient(ApiClient kubernetesApiClient) { - ApiClient defaultClient = Config.defaultClient(); - return new ApiClientWithMergePatching(defaultClient); + return new ApiClientWithMergePatching(kubernetesApiClient); } /** Separate object to perform merge-patching of Jobs */ @@ -71,7 +76,11 @@ private static class ApiClientWithMergePatching extends ApiClient { ApiClientWithMergePatching(ApiClient apiClient) { this.setBasePath(apiClient.getBasePath()); this.setHttpClient(apiClient.getHttpClient()); - this.setApiKey(((ApiKeyAuth) apiClient.getAuthentication("BearerToken")).getApiKey()); + // the token (from a kubeconfig file; in a cluster, the HTTP client adds it) with its + // prefix ("Bearer"), without which Kubernetes refuses it + ApiKeyAuth auth = (ApiKeyAuth) apiClient.getAuthentication("BearerToken"); + this.setApiKeyPrefix(auth.getApiKeyPrefix()); + this.setApiKey(auth.getApiKey()); } @Override diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java new file mode 100644 index 00000000..7c49de6b --- /dev/null +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java @@ -0,0 +1,42 @@ +package uk.ac.ebi.tsc.tesk.k8s.config; + +import com.google.gson.Gson; +import io.kubernetes.client.openapi.JSON; +import java.time.LocalDate; +import java.time.OffsetDateTime; + +/** + * Gson for the objects of the Kubernetes API that ignores fields it doesn't know, as the Kubernetes + * client did up to version 19 (and as client-go does). + * + *

The models of the Kubernetes client since version 20 refuse JSON with fields that they don't + * know, so every object that a newer Kubernetes version extends (e.g. with a field of a job's or + * pod's status) would make reading it fail (https://github.com/kubernetes-client/java/issues/4941). + * TESK API reads jobs, pods, ConfigMaps and LimitRanges, and only creates objects from its own + * templates and patches them with its own patches, so ignoring unknown fields loses nothing. It + * doesn't check required fields either. Everything else is as in the client's Gson: times (RFC + * 3339), byte arrays (Base64), and the client's handling of Status objects and managed fields. + */ +public final class KubernetesJson { + + /** The client's Gson, for its adapters of times and byte arrays */ + private static final Gson CLIENT_GSON = JSON.getGson(); + + public static final Gson GSON = + JSON.createGson() + .registerTypeAdapter( + OffsetDateTime.class, CLIENT_GSON.getAdapter(OffsetDateTime.class)) + .registerTypeAdapter(LocalDate.class, CLIENT_GSON.getAdapter(LocalDate.class)) + .registerTypeAdapter(byte[].class, CLIENT_GSON.getAdapter(byte[].class)) + .create(); + + private KubernetesJson() {} + + /** + * Lets the Kubernetes client read responses with {@link #GSON}: its Gson is global (static), + * shared by all API clients + */ + public static void install() { + JSON.setGson(GSON); + } +} diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/KubernetesObjectsSupplier.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/KubernetesObjectsSupplier.java index c7089f02..2b53248c 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/KubernetesObjectsSupplier.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/KubernetesObjectsSupplier.java @@ -65,8 +65,8 @@ private static V1PodSecurityContext createSecurityContext( confmap_namespace = System.getenv(envConfigMapNamespace); } V1ConfigMap config = - coreApi.readNamespacedConfigMap( - System.getenv(envConfigMap), confmap_namespace, null, false, false); + coreApi.readNamespacedConfigMap(System.getenv(envConfigMap), confmap_namespace) + .execute(); String config_key = "securityContext"; if (System.getenv(envConfigMapKey) != null) { diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolver.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolver.java index 1bc36af0..28a2376d 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolver.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolver.java @@ -13,6 +13,7 @@ import io.kubernetes.client.openapi.models.V1Pod; import io.kubernetes.client.openapi.models.V1PodCondition; import io.kubernetes.client.openapi.models.V1PodStatus; +import java.time.OffsetDateTime; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; @@ -23,7 +24,6 @@ import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.Stream; -import org.joda.time.DateTime; import uk.ac.ebi.tsc.tesk.k8s.constant.K8sConstants; import uk.ac.ebi.tsc.tesk.k8s.convert.data.Job; import uk.ac.ebi.tsc.tesk.k8s.convert.data.Task; @@ -99,11 +99,12 @@ private static boolean hasEnded(V1Pod pod) { if (pod.getStatus() != null && ENDED_POD_PHASES.contains(pod.getStatus().getPhase())) { return true; } - DateTime deletion = + OffsetDateTime deletion = Optional.ofNullable(pod.getMetadata()) .map(V1ObjectMeta::getDeletionTimestamp) .orElse(null); - return deletion != null && deletion.plusSeconds(DELETION_SLACK_SECONDS).isBeforeNow(); + return deletion != null + && deletion.plusSeconds(DELETION_SLACK_SECONDS).isBefore(OffsetDateTime.now()); } /** diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java index 0c155fa0..9687f7ef 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java @@ -12,13 +12,13 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.time.ZoneOffset; +import java.time.format.DateTimeFormatter; import java.util.*; import java.util.function.Supplier; import java.util.stream.Collectors; import java.util.stream.IntStream; import java.util.zip.GZIPOutputStream; -import org.joda.time.format.DateTimeFormatter; -import org.joda.time.format.ISODateTimeFormat; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Qualifier; @@ -49,8 +49,9 @@ public class TesKubernetesConverter { private final TrsToolClient trsClient; + /** Times in UTC, with milliseconds (e.g. 2018-05-31T09:21:57.000Z) */ private static final DateTimeFormatter DATE_FORMATTER = - ISODateTimeFormat.dateTime().withZoneUTC(); + DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSXXX").withZone(ZoneOffset.UTC); public TesKubernetesConverter( @Qualifier("executor") Supplier executorTemplateSupplier, @@ -512,12 +513,12 @@ public TesExecutorLog extractExecutorLogFromK8sJobAndPod(Job executor) { log.setStartTime( Optional.ofNullable(executorJob.getStatus()) .map(V1JobStatus::getStartTime) - .map(DATE_FORMATTER::print) + .map(DATE_FORMATTER::format) .orElse(null)); log.setEndTime( Optional.ofNullable(executorJob.getStatus()) .map(V1JobStatus::getCompletionTime) - .map(DATE_FORMATTER::print) + .map(DATE_FORMATTER::format) .orElse(null)); // workaround for py-tes compatibility. py-tes breaks for ExecutorLogs that can unmarshal to // TaskLogs. Non-null stdout prevents that. @@ -605,7 +606,11 @@ public TesTask fromK8sJobsToTesTaskBasic( } task.setId(taskMasterJobMetadata.getName()); task.setState(this.extractStateFromK8sJobs(taskmasterWithExecutors)); - task.setCreationTime(DATE_FORMATTER.print(taskMasterJobMetadata.getCreationTimestamp())); + // Kubernetes sets it for every job it stores + task.setCreationTime( + Optional.ofNullable(taskMasterJobMetadata.getCreationTimestamp()) + .map(DATE_FORMATTER::format) + .orElse(null)); TesTaskLog log = new TesTaskLog(); task.addLogsItem(log); // the files that the outputs filer uploaded, as the taskmaster stored them (issue #160) @@ -618,12 +623,12 @@ public TesTask fromK8sJobsToTesTaskBasic( log.setStartTime( Optional.ofNullable(taskMasterJob.getStatus()) .map(V1JobStatus::getStartTime) - .map(DATE_FORMATTER::print) + .map(DATE_FORMATTER::format) .orElse(null)); log.setEndTime( Optional.ofNullable(taskMasterJob.getStatus()) .map(V1JobStatus::getCompletionTime) - .map(DATE_FORMATTER::print) + .map(DATE_FORMATTER::format) .orElse(null)); for (Job executorJob : taskmasterWithExecutors.getExecutors()) { TesExecutorLog executorLog = this.extractExecutorLogFromK8sJobAndPod(executorJob); diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java index b2db4222..7bfd8051 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapper.java @@ -64,7 +64,7 @@ public KubernetesClientWrapper( public V1Job createJob(V1Job job) { try { - return this.batchApi.createNamespacedJob(namespace, job, null, null, null); + return this.batchApi.createNamespacedJob(namespace, job).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -77,8 +77,10 @@ public V1Job createJob(V1Job job) { */ public void deleteJob(String name) { try { - this.batchApi.deleteNamespacedJob( - name, namespace, null, null, null, null, PROPAGATION_POLICY_BACKGROUND, null); + this.batchApi + .deleteNamespacedJob(name, namespace) + .propagationPolicy(PROPAGATION_POLICY_BACKGROUND) + .execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -95,7 +97,7 @@ public void deleteJob(String name) { */ public void checkConfigMap(V1ConfigMap map) { try { - this.coreApi.createNamespacedConfigMap(namespace, map, null, DRY_RUN_ALL, null); + this.coreApi.createNamespacedConfigMap(namespace, map).dryRun(DRY_RUN_ALL).execute(); } catch (ApiException e) { if (isDryRunUnsupported(e)) { logger.warn( @@ -120,7 +122,7 @@ private static boolean isDryRunUnsupported(ApiException e) { public V1ConfigMap createConfigMap(V1ConfigMap map) { try { - return this.coreApi.createNamespacedConfigMap(namespace, map, null, null, null); + return this.coreApi.createNamespacedConfigMap(namespace, map).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -129,7 +131,7 @@ public V1ConfigMap createConfigMap(V1ConfigMap map) { /** Checks the Secret's admission and permissions before creating the taskmaster job. */ public void checkSecret(V1Secret secret) { try { - this.coreApi.createNamespacedSecret(namespace, secret, null, DRY_RUN_ALL, null); + this.coreApi.createNamespacedSecret(namespace, secret).dryRun(DRY_RUN_ALL).execute(); } catch (ApiException e) { if (isDryRunUnsupported(e)) { logger.warn( @@ -143,7 +145,7 @@ public void checkSecret(V1Secret secret) { public V1Secret createSecret(V1Secret secret) { try { - return this.coreApi.createNamespacedSecret(namespace, secret, null, null, null); + return this.coreApi.createNamespacedSecret(namespace, secret).execute(); } catch (ApiException e) { throw KubernetesException.fromSecretApiException(e); } @@ -159,7 +161,7 @@ public V1Secret createSecret(V1Secret secret) { */ public V1Job readTaskmasterJob(String taskId) { try { - V1Job job = this.batchApi.readNamespacedJob(taskId, namespace, null, null, null); + V1Job job = this.batchApi.readNamespacedJob(taskId, namespace).execute(); // a job without metadata or labels isn't a taskmaster's job either V1ObjectMeta metadata = job.getMetadata(); Map labels = metadata == null ? null : metadata.getLabels(); @@ -178,18 +180,12 @@ public V1Job readTaskmasterJob(String taskId) { private V1JobList listJobs(String _continue, String labelSelector, Integer limit) { try { - return this.batchApi.listNamespacedJob( - namespace, - null, - null, - _continue, - null, - labelSelector, - limit, - null, - null, - null, - null); + return this.batchApi + .listNamespacedJob(namespace) + ._continue(_continue) + .labelSelector(labelSelector) + .limit(limit) + .execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -202,18 +198,12 @@ private V1JobList listJobs(String _continue, String labelSelector, Integer limit */ private V1LimitRangeList listLimits(String _continue, String labelSelector, Integer limit) { try { - return this.coreApi.listNamespacedLimitRange( - namespace, - (String) null, - (Boolean) null, - _continue, - (String) null, - labelSelector, - limit, - (String) null, - (String) null, - (Integer) null, - (Boolean) null); + return this.coreApi + .listNamespacedLimitRange(namespace) + ._continue(_continue) + .labelSelector(labelSelector) + .limit(limit) + .execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -349,8 +339,9 @@ public V1JobList listSingleTaskExecutorJobs(String taskId) { public Optional getSingleTaskOutputFilerJob(String taskId) { try { V1Job job = - this.batchApi.readNamespacedJob( - taskId + JOB_NAME_FILER_SUF, namespace, null, null, null); + this.batchApi + .readNamespacedJob(taskId + JOB_NAME_FILER_SUF, namespace) + .execute(); return Optional.of(job); } catch (ApiException e) { if (e.getCode() != HttpStatus.NOT_FOUND.value()) { @@ -380,8 +371,7 @@ public V1PodList listSingleJobPods(V1Job job) { .map(entry -> entry.getKey() + "=" + entry.getValue()) .collect(Collectors.joining(",")); try { - return this.coreApi.listNamespacedPod( - namespace, null, null, null, null, labelSelector, null, null, null, null, null); + return this.coreApi.listNamespacedPod(namespace).labelSelector(labelSelector).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -390,8 +380,7 @@ public V1PodList listSingleJobPods(V1Job job) { public V1PodList listAllJobPods() { String labelSelector = "job-name"; try { - return this.coreApi.listNamespacedPod( - namespace, null, null, null, null, labelSelector, null, null, null, null, null); + return this.coreApi.listNamespacedPod(namespace).labelSelector(labelSelector).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -399,8 +388,7 @@ public V1PodList listAllJobPods() { public String readPodLog(String podName) { try { - return this.coreApi.readNamespacedPodLog( - podName, namespace, null, null, null, null, null, null, null, null, null); + return this.coreApi.readNamespacedPodLog(podName, namespace).execute(); } catch (ApiException e) { logger.info("Getting logs for pod " + podName + " failed.", e); } @@ -409,8 +397,7 @@ public String readPodLog(String podName) { public void labelJobAsCancelled(String taskId) { try { - this.patchBatchApi.patchNamespacedJob( - taskId, namespace, CANCEL_PATCH, null, null, null, null); + this.patchBatchApi.patchNamespacedJob(taskId, namespace, CANCEL_PATCH).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -423,8 +410,9 @@ public void labelJobAsCancelled(String taskId) { */ public void cancelJobBeforeStart(String taskId) { try { - this.patchBatchApi.patchNamespacedJob( - taskId, namespace, CANCEL_BEFORE_START_PATCH, null, null, null, null); + this.patchBatchApi + .patchNamespacedJob(taskId, namespace, CANCEL_BEFORE_START_PATCH) + .execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } @@ -432,8 +420,7 @@ public void cancelJobBeforeStart(String taskId) { public void labelPodAsCancelled(String podName) { try { - this.patchCoreApi.patchNamespacedPod( - podName, namespace, CANCEL_PATCH, null, null, null, null); + this.patchCoreApi.patchNamespacedPod(podName, namespace, CANCEL_PATCH).execute(); } catch (ApiException e) { throw KubernetesException.fromApiException(e); } diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java b/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java index 9baa2146..06e5f8fc 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java @@ -11,6 +11,7 @@ import org.springframework.core.io.Resource; import org.springframework.scheduling.annotation.Scheduled; import org.springframework.stereotype.Service; +import org.yaml.snakeyaml.LoaderOptions; import org.yaml.snakeyaml.TypeDescription; import org.yaml.snakeyaml.Yaml; import org.yaml.snakeyaml.constructor.Construct; @@ -74,6 +75,7 @@ public TesServiceInfo getServiceInfo() { // https://bitbucket.org/asomov/snakeyaml/wiki/Howto#markdown-header-how-to-parse-jodatime class OffsetDateTimePropertyConstructor extends Constructor { public OffsetDateTimePropertyConstructor() { + super(new LoaderOptions()); yamlClassConstructors.put(NodeId.scalar, new TimeStampConstruct()); // keys of the service info schema that aren't Java property names (TES 1.1) TypeDescription serviceInfo = new TypeDescription(TesServiceInfo.class); diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java index 8b136e02..00affc01 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java @@ -1,5 +1,9 @@ package uk.ac.ebi.tsc.tesk; +import com.google.gson.JsonArray; +import com.google.gson.JsonElement; +import com.google.gson.JsonObject; +import com.google.gson.JsonParser; import java.io.IOException; import java.nio.file.Files; import org.slf4j.Logger; @@ -14,4 +18,34 @@ public static String getFileContentFromResources(String path) throws IOException Resource fileFromResources = new ClassPathResource(path); return new String(Files.readAllBytes(fileFromResources.getFile().toPath())); } + + /** + * The JSON without the members of objects (at any depth) that are empty arrays or objects, or + * become empty without them: the Kubernetes client writes the lists and maps of its models also + * if they are empty + */ + public static String withoutEmpty(String json) { + JsonElement element = JsonParser.parseString(json); + removeEmpty(element); + return element.toString(); + } + + /** Removes the empty members, and tells whether the element is (then) empty */ + private static boolean removeEmpty(JsonElement element) { + if (element.isJsonObject()) { + JsonObject object = element.getAsJsonObject(); + for (String key : object.keySet().toArray(new String[0])) { + if (removeEmpty(object.get(key))) { + object.remove(key); + } + } + return object.size() == 0; + } + if (element.isJsonArray()) { + JsonArray array = element.getAsJsonArray(); + array.forEach(TestUtils::removeEmpty); + return array.size() == 0; + } + return false; + } } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfigTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfigTest.java new file mode 100644 index 00000000..bfc0f224 --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfigTest.java @@ -0,0 +1,67 @@ +package uk.ac.ebi.tsc.tesk.k8s.config; + +import static com.github.tomakehurst.wiremock.client.WireMock.containing; +import static com.github.tomakehurst.wiremock.client.WireMock.equalTo; +import static com.github.tomakehurst.wiremock.client.WireMock.okJson; +import static com.github.tomakehurst.wiremock.client.WireMock.patch; +import static com.github.tomakehurst.wiremock.client.WireMock.patchRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; + +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import io.kubernetes.client.custom.V1Patch; +import io.kubernetes.client.openapi.ApiClient; +import io.kubernetes.client.openapi.ApiException; +import io.kubernetes.client.openapi.apis.BatchV1Api; +import io.kubernetes.client.openapi.apis.CoreV1Api; +import io.kubernetes.client.util.ClientBuilder; +import io.kubernetes.client.util.credentials.AccessTokenAuthentication; +import org.junit.Rule; +import org.junit.Test; + +/** The clients that merge-patch jobs and pods (labelling and cancelling tasks) */ +public class KubernetesClientConfigTest { + + @Rule public WireMockRule kubernetes = new WireMockRule(wireMockConfig().dynamicPort()); + + private final KubernetesClientConfig config = new KubernetesClientConfig(); + + /** A client with a bearer token, as from a kubeconfig file */ + private ApiClient client() { + return new ClientBuilder() + .setBasePath(kubernetes.baseUrl()) + .setAuthentication(new AccessTokenAuthentication("token-123")) + .build(); + } + + /** + * Jobs are patched with a JSON merge patch, with the credentials of the client, including the + * "Bearer" prefix of its token + */ + @Test + public void patchBatchApi() throws ApiException { + kubernetes.givenThat( + patch(urlPathEqualTo("/apis/batch/v1/namespaces/default/jobs/task-123")) + .willReturn(okJson("{}"))); + BatchV1Api api = config.patchBatchApi(config.patchApiClient(client())); + api.patchNamespacedJob("task-123", "default", new V1Patch("{}")).execute(); + kubernetes.verify( + patchRequestedFor(urlPathEqualTo("/apis/batch/v1/namespaces/default/jobs/task-123")) + .withHeader("Content-Type", containing("application/merge-patch+json")) + .withHeader("Authorization", equalTo("Bearer token-123"))); + } + + /** Pods are patched in the same way */ + @Test + public void patchCoreApi() throws ApiException { + kubernetes.givenThat( + patch(urlPathEqualTo("/api/v1/namespaces/default/pods/pod-123")) + .willReturn(okJson("{}"))); + CoreV1Api api = config.patchCoreApi(config.patchApiClient(client())); + api.patchNamespacedPod("pod-123", "default", new V1Patch("{}")).execute(); + kubernetes.verify( + patchRequestedFor(urlPathEqualTo("/api/v1/namespaces/default/pods/pod-123")) + .withHeader("Content-Type", containing("application/merge-patch+json")) + .withHeader("Authorization", equalTo("Bearer token-123"))); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java new file mode 100644 index 00000000..988ee9f0 --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java @@ -0,0 +1,49 @@ +package uk.ac.ebi.tsc.tesk.k8s.config; + +import static org.assertj.core.api.Assertions.assertThat; + +import io.kubernetes.client.openapi.JSON; +import io.kubernetes.client.openapi.models.V1Job; +import java.time.OffsetDateTime; +import java.time.ZoneOffset; +import org.junit.Test; + +/** Reading the Kubernetes objects of clusters newer than the client */ +public class KubernetesJsonTest { + + /** + * A job with fields that the client doesn't know (as Kubernetes adds them in new versions, e.g. + * to the status) is read, and the fields it knows are kept + */ + @Test + public void readsUnknownFields() { + V1Job job = + KubernetesJson.GSON.fromJson( + "{\"metadata\":{\"name\":\"task-123\",\"newField\":1}," + + "\"spec\":{\"template\":{},\"newSpecField\":\"x\"}," + + "\"status\":{\"succeeded\":1,\"newStatusField\":{\"a\":[]}}}", + V1Job.class); + assertThat(job.getMetadata().getName()).isEqualTo("task-123"); + assertThat(job.getStatus().getSucceeded()).isEqualTo(1); + } + + /** Times are read and written as Kubernetes writes them (RFC 3339, UTC) */ + @Test + public void times() { + V1Job job = + KubernetesJson.GSON.fromJson( + "{\"metadata\":{\"creationTimestamp\":\"2018-05-31T09:21:57Z\"}}", + V1Job.class); + OffsetDateTime created = job.getMetadata().getCreationTimestamp(); + assertThat(created).isEqualTo(OffsetDateTime.of(2018, 5, 31, 9, 21, 57, 0, ZoneOffset.UTC)); + assertThat(KubernetesJson.GSON.toJson(job)) + .contains("\"creationTimestamp\":\"2018-05-31T09:21:57.000000Z\""); + } + + /** The client deserializes responses with the same Gson */ + @Test + public void installed() { + KubernetesJson.install(); + assertThat(JSON.getGson()).isSameAs(KubernetesJson.GSON); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/IgnoredBackendParametersTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/IgnoredBackendParametersTest.java index 3a283f37..fd7e0a46 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/IgnoredBackendParametersTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/IgnoredBackendParametersTest.java @@ -91,7 +91,7 @@ void noKeysNoAnnotation() { IgnoredBackendParameters.annotate(metadata, Collections.emptyList()); - assertThat(metadata.getAnnotations()).isNull(); + assertThat(metadata.getAnnotations()).isNullOrEmpty(); assertThat(IgnoredBackendParameters.systemLog(metadata)).isEmpty(); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolverTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolverTest.java index dbbf494f..bf0e574e 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolverTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TaskStateResolverTest.java @@ -14,6 +14,7 @@ import io.kubernetes.client.openapi.models.V1PodCondition; import io.kubernetes.client.openapi.models.V1PodSpec; import io.kubernetes.client.openapi.models.V1PodStatus; +import java.time.OffsetDateTime; import java.util.ArrayList; import java.util.Arrays; import java.util.EnumSet; @@ -21,7 +22,6 @@ import java.util.List; import java.util.Set; import java.util.stream.Stream; -import org.joda.time.DateTime; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -597,12 +597,12 @@ void podPastItsDeletionTimeHasEnded() { Task stuck = task(true, JobState.FAILED, JobState.FAILED, JobState.ABSENT, JobState.ABSENT); V1Pod pod = pod(TASK + "-ex-00-pod-2", new V1PodStatus().phase("Running")); int slack = TaskStateResolver.DELETION_SLACK_SECONDS; - pod.getMetadata().deletionTimestamp(DateTime.now().minusSeconds(slack + 10)); + pod.getMetadata().deletionTimestamp(OffsetDateTime.now().minusSeconds(slack + 10)); stuck.getExecutors().get(0).addPod(pod); assertThat(TaskStateResolver.resolve(stuck, () -> 1)).isEqualTo(TesState.CANCELED); - pod.getMetadata().deletionTimestamp(DateTime.now().minusSeconds(slack - 10)); + pod.getMetadata().deletionTimestamp(OffsetDateTime.now().minusSeconds(slack - 10)); assertThat(TaskStateResolver.resolve(stuck, () -> 1)).isEqualTo(TesState.CANCELING); - pod.getMetadata().deletionTimestamp(DateTime.now().plusMinutes(1)); + pod.getMetadata().deletionTimestamp(OffsetDateTime.now().plusMinutes(1)); assertThat(TaskStateResolver.resolve(stuck, () -> 1)).isEqualTo(TesState.CANCELING); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java index 27044ba7..cdabb93e 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java @@ -219,9 +219,14 @@ public void fromTesTaskToK8sConfigMap() throws IOException { .extractingJsonPathArrayValue("executors[*].spec.template.spec.restartPolicy") .containsOnly("Never") .hasSize(1); + // no requests or limits (the Kubernetes client writes empty collections) taskMasterInputJson .extractingJsonPathMapValue( - "executors[0].spec.template.spec.containers[0].resources") + "executors[0].spec.template.spec.containers[0].resources.requests") + .isEmpty(); + taskMasterInputJson + .extractingJsonPathMapValue( + "executors[0].spec.template.spec.containers[0].resources.limits") .isEmpty(); taskMasterInputJson @@ -233,14 +238,20 @@ public void fromTesTaskToK8sConfigMap() throws IOException { "executors[0].spec.template.spec.containers[0].image") .isEqualTo("ubuntu"); - taskMasterInputJson + // without the empty collections that the Kubernetes client writes: the resources (no + // requests or limits, see above) are gone too + JsonContentAssert withoutEmpty = + new JsonContentAssert(this.getClass(), TestUtils.withoutEmpty(jsonString)); + withoutEmpty .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") - .containsOnlyKeys("name", "image", "command", "resources"); + .containsOnlyKeys("name", "image", "command"); taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(0.1); - taskMasterInputJson.isEqualToJson( - new ClassPathResource("fromTesToK8s_minimal/taskmaster_param.json"), + withoutEmpty.isEqualToJson( + TestUtils.withoutEmpty( + TestUtils.getFileContentFromResources( + "fromTesToK8s_minimal/taskmaster_param.json")), JSONCompareMode.NON_EXTENSIBLE); } @@ -288,6 +299,8 @@ public void privateExecutorImageUsesOwnedSecretWithoutExposingCredentials() thro String serialized = input.toString(); org.assertj.core.api.Assertions.assertThat(serialized) .doesNotContain(credentialRepresentations); + // no image pull Secrets for the public image (the Kubernetes client writes the + // empty list) org.assertj.core.api.Assertions.assertThat( input.getAsJsonArray("executors") .get(0) @@ -295,8 +308,8 @@ public void privateExecutorImageUsesOwnedSecretWithoutExposingCredentials() thro .getAsJsonObject("spec") .getAsJsonObject("template") .getAsJsonObject("spec") - .has("imagePullSecrets")) - .isFalse(); + .getAsJsonArray("imagePullSecrets")) + .isEmpty(); org.assertj.core.api.Assertions.assertThat( input.getAsJsonArray("executors") .get(1) @@ -557,6 +570,15 @@ public void fromK8sToTask_queued_new_job() throws IOException { assertThat(outputTask, CoreMatchers.is(expectedTask)); } + /** Without a creation time of the taskmaster's job (Kubernetes sets one), the task has none */ + @Test + public void fromK8sToTask_noCreationTime() throws IOException { + TaskBuilder taskBuilder = this.prepareBaseTaskBuider(false, false); + taskBuilder.getTask().getTaskmaster().getJob().getMetadata().setCreationTimestamp(null); + TesTask outputTask = this.converter.fromK8sJobsToTesTaskBasic(taskBuilder.getTask(), false); + org.assertj.core.api.Assertions.assertThat(outputTask.getCreationTime()).isNull(); + } + /** A taskmaster's job with failed pods counted, none active and no condition: an error */ @Test public void fromK8sToTask_system_error_failed_pods() throws IOException { diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java index faa36c9c..a9326fa6 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java @@ -120,8 +120,11 @@ public void setUpKubernetes() throws ApiException, IOException { configMapKey, "fsGroup: 1000\nrunAsUser: 1001\nnonRoot: true\nrunAsGroup: 1002"); V1ConfigMap testConfigMap = new V1ConfigMap(); testConfigMap.setData(testConfigMapData); - when(coreApi.readNamespacedConfigMap(configMapName, configMapNamespace, null, false, false)) - .thenReturn(testConfigMap); + CoreV1Api.APIreadNamespacedConfigMapRequest readConfigMap = + mock(CoreV1Api.APIreadNamespacedConfigMapRequest.class); + when(coreApi.readNamespacedConfigMap(configMapName, configMapNamespace)) + .thenReturn(readConfigMap); + when(readConfigMap.execute()).thenReturn(testConfigMap); KubernetesObjectsSupplier objectsSupplier = new KubernetesObjectsSupplier( gson, jobNameGenerator, taskmasterEnvProperties, "test-namespace", coreApi); @@ -400,10 +403,12 @@ public void fromTesTaskToK8sConfigMap() throws IOException { .containsOnly("16106127360") .hasSize(2); - taskMasterInputJson + JsonContentAssert withoutEmpty = + new JsonContentAssert(this.getClass(), TestUtils.withoutEmpty(jsonString)); + withoutEmpty .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") .containsOnlyKeys("name", "image", "command", "resources", "volumeMounts"); - taskMasterInputJson + withoutEmpty .extractingJsonPathMapValue("executors[1].spec.template.spec.containers[0]") .containsOnlyKeys( "name", @@ -442,8 +447,10 @@ public void fromTesTaskToK8sConfigMap() throws IOException { taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(100.0); - taskMasterInputJson.isEqualToJson( - new ClassPathResource("fromTesToK8s/taskmaster_param.json"), + withoutEmpty.isEqualToJson( + TestUtils.withoutEmpty( + TestUtils.getFileContentFromResources( + "fromTesToK8s/taskmaster_param.json")), JSONCompareMode.NON_EXTENSIBLE); } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperTest.java index e4d019d3..53ecf738 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperTest.java @@ -8,6 +8,7 @@ import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.*; import static org.mockito.BDDMockito.given; +import static org.mockito.Mockito.mock; import static uk.ac.ebi.tsc.tesk.k8s.constant.Constants.ANN_USERID_KEY; import static uk.ac.ebi.tsc.tesk.k8s.constant.Constants.LABEL_GROUPNAME_KEY; import static uk.ac.ebi.tsc.tesk.k8s.constant.Constants.LABEL_LEGACY_USERID_KEY; @@ -16,12 +17,13 @@ import io.kubernetes.client.openapi.apis.BatchV1Api; import io.kubernetes.client.openapi.apis.CoreV1Api; import io.kubernetes.client.openapi.models.*; +import java.time.OffsetDateTime; import java.util.Arrays; import java.util.Optional; -import org.joda.time.DateTime; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; +import org.mockito.Answers; import org.mockito.ArgumentCaptor; import org.mockito.Mockito; import org.springframework.beans.factory.annotation.Value; @@ -49,12 +51,50 @@ public class KubernetesClientWrapperTest { private KubernetesClientWrapper wrapper; + // The requests of the Kubernetes client: their setters return the request itself, only + // execute() needs to be stubbed + private BatchV1Api.APIcreateNamespacedJobRequest createJob; + + private BatchV1Api.APIdeleteNamespacedJobRequest deleteJob; + + private BatchV1Api.APIreadNamespacedJobRequest readJob; + + private BatchV1Api.APIlistNamespacedJobRequest listJobs; + + private BatchV1Api.APIpatchNamespacedJobRequest patchJob; + + private CoreV1Api.APIcreateNamespacedConfigMapRequest createConfigMap; + + private CoreV1Api.APIcreateNamespacedSecretRequest createSecret; + + private CoreV1Api.APIlistNamespacedPodRequest listPods; + @Before public void setUp() { + createJob = request(BatchV1Api.APIcreateNamespacedJobRequest.class); + given(batchApi.createNamespacedJob(any(), any())).willReturn(createJob); + deleteJob = request(BatchV1Api.APIdeleteNamespacedJobRequest.class); + given(batchApi.deleteNamespacedJob(any(), any())).willReturn(deleteJob); + readJob = request(BatchV1Api.APIreadNamespacedJobRequest.class); + given(batchApi.readNamespacedJob(any(), any())).willReturn(readJob); + listJobs = request(BatchV1Api.APIlistNamespacedJobRequest.class); + given(batchApi.listNamespacedJob(any())).willReturn(listJobs); + patchJob = request(BatchV1Api.APIpatchNamespacedJobRequest.class); + given(batchApi.patchNamespacedJob(any(), any(), any())).willReturn(patchJob); + createConfigMap = request(CoreV1Api.APIcreateNamespacedConfigMapRequest.class); + given(coreApi.createNamespacedConfigMap(any(), any())).willReturn(createConfigMap); + createSecret = request(CoreV1Api.APIcreateNamespacedSecretRequest.class); + given(coreApi.createNamespacedSecret(any(), any())).willReturn(createSecret); + listPods = request(CoreV1Api.APIlistNamespacedPodRequest.class); + given(coreApi.listNamespacedPod(any())).willReturn(listPods); this.wrapper = new KubernetesClientWrapper(batchApi, batchApi, coreApi, coreApi, namespace); } + private static T request(Class type) { + return mock(type, Answers.RETURNS_SELF); + } + /** * A job cancelled before its pod runs is labelled as cancelled and gets parallelism 0 and an * active deadline of 1 second, in one patch, so that Kubernetes removes its pods and fails it @@ -66,14 +106,8 @@ public void cancelJobBeforeStart() throws ApiException { org.mockito.ArgumentCaptor.forClass(io.kubernetes.client.custom.V1Patch.class); wrapper.cancelJobBeforeStart("task-123"); Mockito.verify(batchApi) - .patchNamespacedJob( - eq("task-123"), - eq("test-namespace"), - patch.capture(), - isNull(), - isNull(), - isNull(), - isNull()); + .patchNamespacedJob(eq("task-123"), eq("test-namespace"), patch.capture()); + Mockito.verify(patchJob).execute(); assertEquals( "{\"metadata\":{\"labels\":{\"task-status\":\"Cancelled\"}}," + "\"spec\":{\"parallelism\":0,\"activeDeadlineSeconds\":1}}", @@ -82,8 +116,7 @@ public void cancelJobBeforeStart() throws ApiException { @Test(expected = KubernetesException.class) public void cancelJobBeforeStart_refused() throws ApiException { - given(batchApi.patchNamespacedJob(any(), any(), any(), any(), any(), any(), any())) - .willThrow(new ApiException(403, "forbidden")); + given(patchJob.execute()).willThrow(new ApiException(403, "forbidden")); wrapper.cancelJobBeforeStart("task-123"); } @@ -92,15 +125,14 @@ public void cancelJobBeforeStart_refused() throws ApiException { public void checkConfigMap() throws ApiException { V1ConfigMap map = new V1ConfigMap().metadata(new V1ObjectMeta().name("123")); wrapper.checkConfigMap(map); - Mockito.verify(coreApi) - .createNamespacedConfigMap( - eq("test-namespace"), same(map), isNull(), eq("All"), isNull()); + Mockito.verify(coreApi).createNamespacedConfigMap(eq("test-namespace"), same(map)); + Mockito.verify(createConfigMap).dryRun("All"); + Mockito.verify(createConfigMap).execute(); } @Test(expected = KubernetesException.class) public void checkConfigMap_refused() throws ApiException { - given(coreApi.createNamespacedConfigMap(any(), any(), any(), any(), any())) - .willThrow(new ApiException(422, "too large")); + given(createConfigMap.execute()).willThrow(new ApiException(422, "too large")); wrapper.checkConfigMap(new V1ConfigMap()); } @@ -110,7 +142,7 @@ public void checkConfigMap_refused() throws ApiException { */ @Test public void checkConfigMap_webhookWithoutDryRun() throws ApiException { - given(coreApi.createNamespacedConfigMap(any(), any(), any(), any(), any())) + given(createConfigMap.execute()) .willThrow( new ApiException( "", @@ -125,15 +157,14 @@ public void checkConfigMap_webhookWithoutDryRun() throws ApiException { /** A 400 of the dry run without a Status to tell the cause isn't skipped */ @Test(expected = KubernetesException.class) public void checkConfigMap_badRequestWithoutBody() throws ApiException { - given(coreApi.createNamespacedConfigMap(any(), any(), any(), any(), any())) - .willThrow(new ApiException("", 400, null, null)); + given(createConfigMap.execute()).willThrow(new ApiException("", 400, null, null)); wrapper.checkConfigMap(new V1ConfigMap()); } /** Other 400s of the dry run aren't skipped */ @Test(expected = KubernetesException.class) public void checkConfigMap_otherBadRequest() throws ApiException { - given(coreApi.createNamespacedConfigMap(any(), any(), any(), any(), any())) + given(createConfigMap.execute()) .willThrow(new ApiException("", 400, null, "{\"message\":\"invalid\"}")); wrapper.checkConfigMap(new V1ConfigMap()); } @@ -142,14 +173,14 @@ public void checkConfigMap_otherBadRequest() throws ApiException { public void checkSecret() throws ApiException { V1Secret secret = new V1Secret().metadata(new V1ObjectMeta().name("task-123-ex-00")); wrapper.checkSecret(secret); - Mockito.verify(coreApi) - .createNamespacedSecret( - eq("test-namespace"), same(secret), isNull(), eq("All"), isNull()); + Mockito.verify(coreApi).createNamespacedSecret(eq("test-namespace"), same(secret)); + Mockito.verify(createSecret).dryRun("All"); + Mockito.verify(createSecret).execute(); } @Test public void checkSecret_webhookWithoutDryRun() throws ApiException { - given(coreApi.createNamespacedSecret(any(), any(), any(), any(), any())) + given(createSecret.execute()) .willThrow( new ApiException( "", @@ -161,8 +192,7 @@ public void checkSecret_webhookWithoutDryRun() throws ApiException { @Test public void checkSecret_refused() throws ApiException { - given(coreApi.createNamespacedSecret(any(), any(), any(), any(), any())) - .willThrow(new ApiException(403, "private-token")); + given(createSecret.execute()).willThrow(new ApiException(403, "private-token")); try { wrapper.checkSecret(new V1Secret()); fail("Expected KubernetesException"); @@ -175,15 +205,15 @@ public void checkSecret_refused() throws ApiException { @Test public void createSecret() throws ApiException { V1Secret secret = new V1Secret().metadata(new V1ObjectMeta().name("task-123-ex-00")); - given(coreApi.createNamespacedSecret("test-namespace", secret, null, null, null)) - .willReturn(secret); + given(createSecret.execute()).willReturn(secret); assertThat(wrapper.createSecret(secret), is(secret)); + Mockito.verify(coreApi).createNamespacedSecret("test-namespace", secret); + Mockito.verify(createSecret, Mockito.never()).dryRun(any()); } @Test public void createSecret_refusedDoesNotExposeCredentials() throws ApiException { - given(coreApi.createNamespacedSecret(any(), any(), any(), any(), any())) - .willThrow(new ApiException(422, "private-token")); + given(createSecret.execute()).willThrow(new ApiException(422, "private-token")); try { wrapper.createSecret(new V1Secret()); fail("Expected KubernetesException"); @@ -201,11 +231,11 @@ public void createJob() throws ApiException { .metadata( new V1ObjectMeta() .name("test job") - .creationTimestamp(new DateTime())); - given(batchApi.createNamespacedJob("test-namespace", inJob, null, null, null)) - .willReturn(outJob); + .creationTimestamp(OffsetDateTime.now())); + given(createJob.execute()).willReturn(outJob); V1Job result = wrapper.createJob(inJob); assertThat(result, is(outJob)); + Mockito.verify(batchApi).createNamespacedJob("test-namespace", inJob); } @Test @@ -216,10 +246,10 @@ public void readTaskmasterJob() throws ApiException { new V1ObjectMeta() .name("test job") .putLabelsItem("job-type", "taskmaster")); - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); V1Job result = wrapper.readTaskmasterJob("123"); assertThat(result, is(job)); + Mockito.verify(batchApi).readNamespacedJob("123", "test-namespace"); } @Test(expected = TaskNotFoundException.class) @@ -230,8 +260,7 @@ public void readTaskmasterJob_wrongType() throws Exception { new V1ObjectMeta() .name("test job") .putLabelsItem("job-type", "executor")); - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); wrapper.readTaskmasterJob("123"); } @@ -243,8 +272,7 @@ public void readTaskmasterJob_noType() throws Exception { new V1ObjectMeta() .name("test job") .putLabelsItem("other", "taskmaster")); - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); wrapper.readTaskmasterJob("123"); } @@ -252,22 +280,20 @@ public void readTaskmasterJob_noType() throws Exception { @Test(expected = TaskNotFoundException.class) public void readTaskmasterJob_noLabels() throws Exception { V1Job job = new V1Job().metadata(new V1ObjectMeta().name("123")); - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); wrapper.readTaskmasterJob("123"); } /** A job without metadata isn't a taskmaster's job either (issue #125) */ @Test(expected = TaskNotFoundException.class) public void readTaskmasterJob_noMetadata() throws Exception { - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(new V1Job()); + given(readJob.execute()).willReturn(new V1Job()); wrapper.readTaskmasterJob("123"); } @Test(expected = TaskNotFoundException.class) public void readTaskmasterJob_noJob() throws Exception { - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) + given(readJob.execute()) .willThrow(new ApiException(HttpStatus.NOT_FOUND.value(), "No job")); wrapper.readTaskmasterJob("123"); } @@ -280,10 +306,9 @@ public void readTaskmasterJob_beingDeleted() throws Exception { .metadata( new V1ObjectMeta() .name("123") - .deletionTimestamp(new DateTime()) + .deletionTimestamp(OffsetDateTime.now()) .putLabelsItem("job-type", "taskmaster")); - given(batchApi.readNamespacedJob("123", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); wrapper.readTaskmasterJob("123"); } @@ -296,11 +321,8 @@ public void listAllTaskmasterJobsForUser_skipsJobsBeingDeleted() throws ApiExcep .metadata( new V1ObjectMeta() .name("deleted") - .deletionTimestamp(new DateTime())); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) + .deletionTimestamp(OffsetDateTime.now())); + given(listJobs.execute()) .willReturn( new V1JobList() .metadata(new V1ListMeta()._continue("next")) @@ -315,21 +337,14 @@ public void listAllTaskmasterJobsForUser_skipsJobsBeingDeleted() throws ApiExcep @Test public void deleteJob() throws ApiException { wrapper.deleteJob("123"); - Mockito.verify(batchApi) - .deleteNamespacedJob( - eq("123"), - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - eq("Background"), - isNull()); + Mockito.verify(batchApi).deleteNamespacedJob("123", "test-namespace"); + Mockito.verify(deleteJob).propagationPolicy("Background"); + Mockito.verify(deleteJob).execute(); } @Test(expected = KubernetesException.class) public void deleteJob_fails() throws ApiException { - given(batchApi.deleteNamespacedJob(any(), any(), any(), any(), any(), any(), any(), any())) + given(deleteJob.execute()) .willThrow(new ApiException(HttpStatus.FORBIDDEN.value(), "forbidden")); wrapper.deleteJob("123"); } @@ -339,25 +354,9 @@ public void listAllTaskmasterJobsForUser_any() throws ApiException { User user = User.builder("123").build(); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(this.resultList()); + given(listJobs.execute()).willReturn(this.resultList()); V1JobList result = wrapper.listAllTaskmasterJobsForUser(null, null, user); - Mockito.verify(batchApi) - .listNamespacedJob( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs("test-namespace", argument); assertThat(argument.getValue(), is("job-type=taskmaster")); assertThat(result, is(this.resultList())); } @@ -367,25 +366,9 @@ public void listAllTaskmasterJobsForUser_admin() throws ApiException { User user = User.builder("123").teskAdmin(true).build(); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(this.resultList()); + given(listJobs.execute()).willReturn(this.resultList()); V1JobList result = wrapper.listAllTaskmasterJobsForUser(null, null, user); - Mockito.verify(batchApi) - .listNamespacedJob( - eq(namespace), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs(namespace, argument); assertThat(argument.getValue(), is("job-type=taskmaster")); assertThat(result, is(this.resultList())); } @@ -398,25 +381,9 @@ public void listAllTaskmasterJobsForUser_member() throws ApiException { .teskMemberedGroups(StringUtils.commaDelimitedListToSet("TEST")) .build(); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(this.resultList()); + given(listJobs.execute()).willReturn(this.resultList()); V1JobList result = wrapper.listAllTaskmasterJobsForUser(null, null, user); - Mockito.verify(batchApi) - .listNamespacedJob( - eq(namespace), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs(namespace, argument); assertThat( argument.getValue(), is( @@ -436,25 +403,9 @@ public void listAllTaskmasterJobsForUser_groupAdmin() throws ApiException { .teskManagedGroups(StringUtils.commaDelimitedListToSet("TEST,XYZ")) .build(); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(this.resultList()); + given(listJobs.execute()).willReturn(this.resultList()); V1JobList result = wrapper.listAllTaskmasterJobsForUser(null, null, user); - Mockito.verify(batchApi) - .listNamespacedJob( - eq(namespace), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs(namespace, argument); assertThat(argument.getValue(), is("job-type=taskmaster,creator-group-name in (TEST,XYZ)")); assertThat(result, is(this.resultList())); } @@ -468,25 +419,9 @@ public void listAllTaskmasterJobsForUser_mixed() throws ApiException { .teskMemberedGroups(StringUtils.commaDelimitedListToSet("XYZ")) .build(); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(this.resultList()); + given(listJobs.execute()).willReturn(this.resultList()); V1JobList result = wrapper.listAllTaskmasterJobsForUser(null, null, user); - Mockito.verify(batchApi) - .listNamespacedJob( - eq(namespace), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs(namespace, argument); assertThat(argument.getValue(), is("job-type=taskmaster,creator-group-name in (XYZ,TEST)")); assertThat(result, is(filteredResultList())); } @@ -495,34 +430,22 @@ public void listAllTaskmasterJobsForUser_mixed() throws ApiException { public void listSingleTaskExecutorJobs() throws ApiException { ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); wrapper.listSingleTaskExecutorJobs("123"); - Mockito.verify(batchApi) - .listNamespacedJob( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs("test-namespace", argument); assertThat(argument.getValue(), is("taskmaster-name=123")); } @Test public void getSingleTaskOutputFilerJob() throws ApiException { V1Job job = new V1Job().metadata(new V1ObjectMeta().name("test job")); - given(batchApi.readNamespacedJob("123-outputs-filer", "test-namespace", null, null, null)) - .willReturn(job); + given(readJob.execute()).willReturn(job); Optional result = wrapper.getSingleTaskOutputFilerJob("123"); assertThat(result, is(Optional.of(job))); + Mockito.verify(batchApi).readNamespacedJob("123-outputs-filer", "test-namespace"); } @Test public void getSingleTaskOutputFilerJob_noJob() throws ApiException { - given(batchApi.readNamespacedJob("123-outputs-filer", "test-namespace", null, null, null)) + given(readJob.execute()) .willThrow(new ApiException(HttpStatus.NOT_FOUND.value(), "No Job")); Optional result = wrapper.getSingleTaskOutputFilerJob("123"); assertThat(result, is(Optional.empty())); @@ -532,19 +455,7 @@ public void getSingleTaskOutputFilerJob_noJob() throws ApiException { public void listAllTaskExecutorJobs() throws ApiException { ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); wrapper.listAllTaskExecutorJobs(); - Mockito.verify(batchApi) - .listNamespacedJob( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs("test-namespace", argument); assertThat(argument.getValue(), is("job-type=executor")); } @@ -552,19 +463,7 @@ public void listAllTaskExecutorJobs() throws ApiException { public void listAllFilerJobs() throws ApiException { ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); wrapper.listAllFilerJobs(); - Mockito.verify(batchApi) - .listNamespacedJob( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + verifyListJobs("test-namespace", argument); assertThat(argument.getValue(), is("!job-type")); } @@ -581,38 +480,31 @@ public void listSingleJobPods() throws ApiException { .putMatchLabelsItem("labelA", "valueB"))); ArgumentCaptor argument = ArgumentCaptor.forClass(String.class); wrapper.listSingleJobPods(job); - Mockito.verify(coreApi) - .listNamespacedPod( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + Mockito.verify(coreApi).listNamespacedPod("test-namespace"); + Mockito.verify(listPods).labelSelector(argument.capture()); + Mockito.verify(listPods).execute(); assertThat(argument.getValue(), is("label1=value1,labelA=valueB")); } @Test public void listAllJobPods() throws ApiException { wrapper.listAllJobPods(); - Mockito.verify(coreApi) - .listNamespacedPod( - eq("test-namespace"), - isNull(), - isNull(), - isNull(), - isNull(), - eq("job-name"), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + Mockito.verify(coreApi).listNamespacedPod("test-namespace"); + Mockito.verify(listPods).labelSelector("job-name"); + Mockito.verify(listPods).execute(); + } + + /** + * Verifies that the jobs were listed once, in the namespace, without a page token or limit, and + * captures the label selector + */ + private void verifyListJobs(String namespace, ArgumentCaptor labelSelector) + throws ApiException { + Mockito.verify(batchApi).listNamespacedJob(namespace); + Mockito.verify(listJobs)._continue(isNull()); + Mockito.verify(listJobs).labelSelector(labelSelector.capture()); + Mockito.verify(listJobs).limit(isNull()); + Mockito.verify(listJobs).execute(); } private static V1Job job(String group, String userId) { @@ -655,11 +547,7 @@ private V1JobList filteredResultList() { private V1JobList listJobsReturning(V1Job... jobs) throws ApiException { V1JobList jobList = new V1JobList().items(Arrays.asList(jobs)); - given( - batchApi.listNamespacedJob( - any(), isNull(), isNull(), isNull(), isNull(), any(), isNull(), - isNull(), isNull(), isNull(), isNull())) - .willReturn(jobList); + given(listJobs.execute()).willReturn(jobList); return jobList; } @@ -697,19 +585,8 @@ public void listAllTaskmasterJobsForUser_legacyLabels() throws ApiException { assertThat( wrapper.listAllTaskmasterJobsForUser(null, null, legacyUser).getItems(), is(Arrays.asList(legacyJob("TEST", "a_b"), job("TEST", "a@b")))); - Mockito.verify(batchApi, Mockito.times(2)) - .listNamespacedJob( - eq(namespace), - isNull(), - isNull(), - isNull(), - isNull(), - argument.capture(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull()); + Mockito.verify(batchApi, Mockito.times(2)).listNamespacedJob(namespace); + Mockito.verify(listJobs, Mockito.times(2)).labelSelector(argument.capture()); assertThat(argument.getValue(), is("job-type=taskmaster,creator-group-name in (TEST)")); } @@ -761,7 +638,6 @@ public void listAllTaskmasterJobsForUser_malformedPageToken() throws ApiExceptio private void assertInvalidPageToken(int code) throws ApiException { this.listJobsFails( - "secret-token", code, "{\"kind\":\"Status\",\"message\":\"continue key is not valid: secret-token\"}"); try { @@ -771,12 +647,22 @@ private void assertInvalidPageToken(int code) throws ApiException { assertTrue(e.getMessage().contains("page_token")); assertFalse(e.getMessage().contains("secret-token")); } + Mockito.verify(listJobs)._continue("secret-token"); + } + + /** The page token and the page size are passed on to Kubernetes */ + @Test + public void listAllTaskmasterJobsForUser_page() throws ApiException { + given(listJobs.execute()).willReturn(this.resultList()); + wrapper.listAllTaskmasterJobsForUser("token", 10, User.builder("123").build()); + Mockito.verify(listJobs)._continue("token"); + Mockito.verify(listJobs).limit(10); } /** Without a page token, a 400 of Kubernetes stays a server error */ @Test(expected = KubernetesException.class) public void listAllTaskmasterJobsForUser_badRequestWithoutPageToken() throws ApiException { - this.listJobsFails(null, HttpStatus.BAD_REQUEST.value(), "bad selector"); + this.listJobsFails(HttpStatus.BAD_REQUEST.value(), "bad selector"); wrapper.listAllTaskmasterJobsForUser(null, null, User.builder("123").build()); } @@ -784,7 +670,6 @@ public void listAllTaskmasterJobsForUser_badRequestWithoutPageToken() throws Api @Test(expected = KubernetesException.class) public void listAllTaskmasterJobsForUser_otherBadRequestWithPageToken() throws ApiException { this.listJobsFails( - "token", HttpStatus.BAD_REQUEST.value(), "{\"kind\":\"Status\",\"message\":\"unable to parse requirement\"}"); wrapper.listAllTaskmasterJobsForUser("token", null, User.builder("123").build()); @@ -794,36 +679,21 @@ public void listAllTaskmasterJobsForUser_otherBadRequestWithPageToken() throws A @Test(expected = KubernetesException.class) public void listAllTaskmasterJobsForUser_badRequestWithoutBodyWithPageToken() throws ApiException { - this.listJobsFails("token", HttpStatus.BAD_REQUEST.value(), null); + this.listJobsFails(HttpStatus.BAD_REQUEST.value(), null); wrapper.listAllTaskmasterJobsForUser("token", null, User.builder("123").build()); } /** Other errors of Kubernetes stay server errors, also with a page token */ @Test(expected = KubernetesException.class) public void listAllTaskmasterJobsForUser_serverErrorWithPageToken() throws ApiException { - this.listJobsFails("token", HttpStatus.INTERNAL_SERVER_ERROR.value(), "error"); + this.listJobsFails(HttpStatus.INTERNAL_SERVER_ERROR.value(), "error"); wrapper.listAllTaskmasterJobsForUser("token", null, User.builder("123").build()); } /** - * Lets Kubernetes answer the list request with the page token (or none) with an error and the - * response body (a Status object) + * Lets Kubernetes answer the list request with an error and the response body (a Status object) */ - private void listJobsFails(String pageToken, int code, String responseBody) - throws ApiException { - given( - batchApi.listNamespacedJob( - any(), - isNull(), - isNull(), - pageToken == null ? isNull() : eq(pageToken), - isNull(), - any(), - isNull(), - isNull(), - isNull(), - isNull(), - isNull())) - .willThrow(new ApiException("", code, null, responseBody)); + private void listJobsFails(int code, String responseBody) throws ApiException { + given(listJobs.execute()).willThrow(new ApiException("", code, null, responseBody)); } } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImplCreateTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImplCreateTest.java index db9c513a..8dc11ce1 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImplCreateTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/tes/service/TesServiceImplCreateTest.java @@ -387,7 +387,7 @@ void noBackendParametersNoNote(CapturedOutput output) { ArgumentCaptor job = ArgumentCaptor.forClass(V1Job.class); verify(this.client).createJob(job.capture()); - assertThat(job.getValue().getMetadata().getAnnotations()).isNull(); + assertThat(job.getValue().getMetadata().getAnnotations()).isNullOrEmpty(); assertThat(output.getOut()).doesNotContain("backend_parameters"); } } From 3adad3eb05330d1ca9ddc8d4bf1fc2a06b78d760 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 02:57:14 +0100 Subject: [PATCH 2/9] build: drop the SnakeYAML 1 scan exception The image has SnakeYAML 2 now, so CVE-2022-1471 no longer needs an entry in .trivyignore.yaml. Dependabot may propose major versions of the Kubernetes client again. Co-Authored-By: Claude Opus 5.5 --- .github/dependabot.yml | 2 -- .trivyignore.yaml | 7 +------ 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index e370cc49..18bcdda9 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -16,8 +16,6 @@ updates: # Held back until the move to Spring Boot 3 (#66): the next major # versions of these need Java 11 or 17, or Spring Framework 6 or 7 # (#91). Remove these rules with #66 - - dependency-name: io.kubernetes:client-java - update-types: ["version-update:semver-major"] - dependency-name: org.springframework.security:* update-types: ["version-update:semver-major"] - dependency-name: com.github.tomakehurst:wiremock diff --git a/.trivyignore.yaml b/.trivyignore.yaml index d78fe80f..8e1e9693 100644 --- a/.trivyignore.yaml +++ b/.trivyignore.yaml @@ -5,8 +5,7 @@ # the affected package versions (`purls`), so that other packages with the same # vulnerability are still reported. # -# All of these are fixed only in Spring Boot 3 / Spring Framework 6 and later -# (or, for snakeyaml, in the 2.x line, which the Kubernetes client can't use), +# All of these are fixed only in Spring Boot 3 / Spring Framework 6 and later, # while `pom.xml` has the last open source releases of the Spring Boot 2.7 line. # # See https://trivy.dev/latest/docs/configuration/filtering/#trivyignoreyaml @@ -59,7 +58,3 @@ vulnerabilities: purls: ["pkg:maven/org.springframework/spring-webmvc@5.3.39"] statement: Concerns XsltView; the app has no views expired_at: 2027-01-31 - - id: CVE-2022-1471 - purls: ["pkg:maven/org.yaml/snakeyaml@1.33"] - statement: Needs YAML from an attacker; the app only reads files and ConfigMaps of the deployer - expired_at: 2027-01-31 From c33d709acf6d03738fc063967288fa839e0e892d Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 02:57:25 +0100 Subject: [PATCH 3/9] docs: describe the Kubernetes client update Deployment: the client version, and the Kubernetes versions TESK API works with. Development: the library versions to update with the client, and KubernetesJson. Upgrade notes: what changes. Co-Authored-By: Claude Opus 5.5 --- docs/deployment.md | 11 +++++++++++ docs/development.md | 9 +++++++++ docs/upgrading.md | 9 +++++++++ 3 files changed, 29 insertions(+) diff --git a/docs/deployment.md b/docs/deployment.md index 0ef634b4..7d3489ea 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -56,6 +56,17 @@ Resource | Verbs | For `secrets` | `create`, `delete` | Private executor image credentials, as Secrets owned by the taskmaster Job (`delete` for `OwnerReferencesPermissionEnforcement`) `limitranges` | `list` | Raising requested memory to the namespace's minimum (without the right, it logs an error and goes on) +TESK API has version 28 of the client, made for Kubernetes 1.37. It also +works with older and newer Kubernetes versions: the jobs, ConfigMaps and +Secrets that it creates, and its patches, use only fields that Kubernetes has +had for years, and it ignores the fields that newer versions add to the +objects that it reads (jobs, pods, ConfigMaps and LimitRanges), which the +client itself would refuse +([kubernetes-client/java#4941](https://github.com/kubernetes-client/java/issues/4941)). +The objects that it creates list empty fields, e.g. `args: []`, which +Kubernetes treats as unset; Kubernetes versions that don't know such a field +drop it, and may answer with a warning. + Passing private executor image credentials in TES requests is an [experimental workaround for proof-of-concept use only](api.md#private-executor-images), not a feature for production clients. diff --git a/docs/development.md b/docs/development.md index 66442b83..b40bbdbd 100644 --- a/docs/development.md +++ b/docs/development.md @@ -8,6 +8,15 @@ plugin](https://github.com/swagger-api/swagger-codegen/tree/master/modules/swagg and the [Kubernetes Java client](https://github.com/kubernetes-client/java) talks to the Kubernetes API. +The Kubernetes client needs newer versions of some libraries than Spring +Boot 2.7 manages (SnakeYAML, OkHttp, Kotlin, Gson, Commons Lang and Commons +Codec): `pom.xml` sets them to those of the client's parent POM +(`io.kubernetes:client-java-parent`), so update them together with the +client. The models of the client refuse JSON with fields they don't know; +`KubernetesJson` installs a Gson that ignores them (see +[Deployment](deployment.md#kubernetes)), so test fixtures of Kubernetes +objects may leave out fields that Kubernetes requires. + The wrapper is the `only-script` type of the Apache Maven Wrapper, so there is no binary `maven-wrapper.jar` in the repository. It needs `curl` or `wget` and `unzip` (without `unzip` it would fetch the `.tar.gz`, which does not match the diff --git a/docs/upgrading.md b/docs/upgrading.md index 80937200..97efa7fa 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -51,6 +51,15 @@ notice changed answers: see [Configuration](configuration.md)). An older taskmaster stops at the failure: the task then ends `SYSTEM_ERROR`, unless the failed executor was the last one and the task has no outputs, which completes as asked. +- **Kubernetes client.** TESK API has version 28 of the Kubernetes Java + client instead of 11 (see [Deployment](deployment.md#kubernetes)), and + SnakeYAML 2 instead of 1.33. Nothing changes for deployments: it needs the + same rights, and works with the same Kubernetes versions. The jobs and the + taskmaster's input that it creates list empty fields (e.g. `args: []`), + which Kubernetes treats as unset. Run outside a cluster with a token in the + kubeconfig file (e.g. after `oc login`), TESK API can now cancel tasks: + it sent the token without its `Bearer` prefix in the patches, which + Kubernetes refused. ## To 1.2.0 From 6e742acaaf809890a6de4cdb0ec21e4ba379cab7 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 03:09:04 +0100 Subject: [PATCH 4/9] build: keep one Bouncy Castle on the classpath The Kubernetes client brings bcprov/bcpkix-jdk18on 1.86, and spring-security-jwt the jdk15on jars 1.70, which have the same classes: the image had both. Exclude the jdk15on jars, whose pins go too; spring-security-jwt only uses ASN.1 classes that jdk18on has. Drop the unused apache.commons.lang3.version property. Co-Authored-By: Claude Opus 5.5 --- docs/development.md | 4 +++- pom.xml | 26 ++++++++------------------ 2 files changed, 11 insertions(+), 19 deletions(-) diff --git a/docs/development.md b/docs/development.md index b40bbdbd..30af5e74 100644 --- a/docs/development.md +++ b/docs/development.md @@ -12,7 +12,9 @@ The Kubernetes client needs newer versions of some libraries than Spring Boot 2.7 manages (SnakeYAML, OkHttp, Kotlin, Gson, Commons Lang and Commons Codec): `pom.xml` sets them to those of the client's parent POM (`io.kubernetes:client-java-parent`), so update them together with the -client. The models of the client refuse JSON with fields they don't know; +client. It also keeps the older Bouncy Castle jars (`*-jdk15on`) of +spring-security-jwt out, as the client brings the current ones +(`*-jdk18on`), with the same classes. The models of the client refuse JSON with fields they don't know; `KubernetesJson` installs a Gson that ignores them (see [Deployment](deployment.md#kubernetes)), so test fixtures of Kubernetes objects may leave out fields that Kubernetes requires. diff --git a/pom.xml b/pom.xml index fbe25f9d..956d1fd2 100644 --- a/pom.xml +++ b/pom.xml @@ -44,7 +44,6 @@ 3.21.0 1.23.0 3.6.1 - 3.12.0 4.6.0 @@ -61,23 +60,6 @@ jose4j 0.9.7 - - org.bouncycastle - bcprov-jdk15on - 1.70 - - - - org.bouncycastle - bcpkix-jdk15on - 1.70 - - - org.bouncycastle - bcutil-jdk15on - 1.70 - @@ -104,6 +86,14 @@ org.springframework.security.oauth.boot spring-security-oauth2-autoconfigure 2.6.8 + + + + org.bouncycastle + bcpkix-jdk15on + + From edc5d7a47670961bfb75311103b495227752efdc Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 03:09:16 +0100 Subject: [PATCH 5/9] test: check the Kubernetes requests over HTTP KubernetesClientWrapperHttpTest sends the wrapper's requests through a real client to WireMock: query parameters (labelSelector, continue, limit, dryRun, propagationPolicy), merge-patch bodies, pod logs, LimitRanges and errors. KubernetesClientConfig installs the lenient Gson in its constructor, which a test checks. Converter tests strip empty fields only in the executors' jobs, with a test of the helper. Docs: the creation time, the affected kubeconfigs. Co-Authored-By: Claude Opus 5.5 --- docs/deployment.md | 22 ++- docs/upgrading.md | 10 +- .../k8s/config/KubernetesClientConfig.java | 7 +- .../tesk/tes/service/ServiceInfoService.java | 1 + .../java/uk/ac/ebi/tsc/tesk/TestUtils.java | 24 ++- .../uk/ac/ebi/tsc/tesk/TestUtilsTest.java | 25 +++ .../tesk/k8s/config/KubernetesJsonTest.java | 34 +++- .../TesKubernetesConverterMinimalTest.java | 5 +- .../convert/TesKubernetesConverterTest.java | 5 +- .../KubernetesClientWrapperHttpTest.java | 186 ++++++++++++++++++ 10 files changed, 288 insertions(+), 31 deletions(-) create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java create mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperHttpTest.java diff --git a/docs/deployment.md b/docs/deployment.md index 7d3489ea..5e513ba6 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -56,16 +56,18 @@ Resource | Verbs | For `secrets` | `create`, `delete` | Private executor image credentials, as Secrets owned by the taskmaster Job (`delete` for `OwnerReferencesPermissionEnforcement`) `limitranges` | `list` | Raising requested memory to the namespace's minimum (without the right, it logs an error and goes on) -TESK API has version 28 of the client, made for Kubernetes 1.37. It also -works with older and newer Kubernetes versions: the jobs, ConfigMaps and -Secrets that it creates, and its patches, use only fields that Kubernetes has -had for years, and it ignores the fields that newer versions add to the -objects that it reads (jobs, pods, ConfigMaps and LimitRanges), which the -client itself would refuse -([kubernetes-client/java#4941](https://github.com/kubernetes-client/java/issues/4941)). -The objects that it creates list empty fields, e.g. `args: []`, which -Kubernetes treats as unset; Kubernetes versions that don't know such a field -drop it, and may answer with a warning. +TESK API has version 28 of the client, which supports Kubernetes 1.37. It +also works with older and newer Kubernetes versions: + +- It reads jobs, pods, ConfigMaps and LimitRanges, and ignores the fields + that newer Kubernetes versions add to them. (The client itself would refuse + such objects, see + [kubernetes-client/java#4941](https://github.com/kubernetes-client/java/issues/4941).) +- The jobs, ConfigMaps and Secrets that it creates, and its patches, use + only fields that Kubernetes has had for years. +- The objects that it creates list empty fields, e.g. `args: []`, which + Kubernetes treats as unset. A Kubernetes version that doesn't know such a + field drops it, and may answer with a warning. Passing private executor image credentials in TES requests is an [experimental workaround for proof-of-concept use only](api.md#private-executor-images), diff --git a/docs/upgrading.md b/docs/upgrading.md index 97efa7fa..d4af5fad 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -56,10 +56,12 @@ notice changed answers: SnakeYAML 2 instead of 1.33. Nothing changes for deployments: it needs the same rights, and works with the same Kubernetes versions. The jobs and the taskmaster's input that it creates list empty fields (e.g. `args: []`), - which Kubernetes treats as unset. Run outside a cluster with a token in the - kubeconfig file (e.g. after `oc login`), TESK API can now cancel tasks: - it sent the token without its `Bearer` prefix in the patches, which - Kubernetes refused. + which Kubernetes treats as unset. Run outside a cluster with a token in its + kubeconfig file (e.g. after `oc login`; not with client certificates), + TESK API can now cancel tasks: it sent the token without its `Bearer` + prefix in the patches, which Kubernetes refused. A task whose taskmaster + job has no creation time has no `creation_time`, instead of the current + time; Kubernetes sets it for every job, so this only matters for tests. ## To 1.2.0 diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java index 8af1fe3c..bdd5652f 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesClientConfig.java @@ -15,8 +15,11 @@ @Configuration public class KubernetesClientConfig { - static { - // read objects of Kubernetes versions newer than the client + /** + * Lets the Kubernetes client read objects of Kubernetes versions newer than itself, before any + * of the API clients below is used + */ + public KubernetesClientConfig() { KubernetesJson.install(); } diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java b/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java index 06e5f8fc..5a11b1df 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/tes/service/ServiceInfoService.java @@ -75,6 +75,7 @@ public TesServiceInfo getServiceInfo() { // https://bitbucket.org/asomov/snakeyaml/wiki/Howto#markdown-header-how-to-parse-jodatime class OffsetDateTimePropertyConstructor extends Constructor { public OffsetDateTimePropertyConstructor() { + // SnakeYAML 2 has no constructor without options; these are its defaults super(new LoaderOptions()); yamlClassConstructors.put(NodeId.scalar, new TimeStampConstruct()); // keys of the service info schema that aren't Java property names (TES 1.1) diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java index 00affc01..41d9727e 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java @@ -20,17 +20,25 @@ public static String getFileContentFromResources(String path) throws IOException } /** - * The JSON without the members of objects (at any depth) that are empty arrays or objects, or - * become empty without them: the Kubernetes client writes the lists and maps of its models also - * if they are empty + * The taskmaster's input without the empty arrays and objects in its executors' jobs: the + * Kubernetes client writes the lists and maps of its models also if they are empty. Lossy: an + * empty field that TESK API sets on purpose is gone too, so tests check those separately. The + * rest of the input (e.g. its {@code inputs}, {@code outputs} and {@code volumes}) is kept. */ - public static String withoutEmpty(String json) { - JsonElement element = JsonParser.parseString(json); - removeEmpty(element); - return element.toString(); + public static String withoutEmptyInExecutors(String json) { + JsonObject input = JsonParser.parseString(json).getAsJsonObject(); + JsonElement executors = input.get("executors"); + if (executors != null && executors.isJsonArray()) { + executors.getAsJsonArray().forEach(TestUtils::removeEmpty); + } + return input.toString(); } - /** Removes the empty members, and tells whether the element is (then) empty */ + /** + * Removes the members of objects that are empty arrays or objects, or become empty without + * their own empty members, at any depth, and tells whether the element is (then) empty. + * Elements of arrays are cleaned, but kept even if they become empty. + */ private static boolean removeEmpty(JsonElement element) { if (element.isJsonObject()) { JsonObject object = element.getAsJsonObject(); diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java new file mode 100644 index 00000000..ccc899cf --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java @@ -0,0 +1,25 @@ +package uk.ac.ebi.tsc.tesk; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.junit.Test; + +/** The helpers of the tests */ +public class TestUtilsTest { + + /** + * Empty arrays and objects go from the executors' jobs, also objects that become empty, but not + * elements of arrays; the rest of the input stays as it is + */ + @Test + public void withoutEmptyInExecutors() { + assertThat( + TestUtils.withoutEmptyInExecutors( + "{\"outputs\":[],\"volumes\":{},\"executors\":[{\"a\":[]," + + "\"b\":{\"c\":{},\"d\":[]},\"e\":[{\"f\":[]}]," + + "\"g\":0,\"h\":\"\"}]}")) + .isEqualTo( + "{\"outputs\":[],\"volumes\":{},\"executors\":[{\"e\":[{}]," + + "\"g\":0,\"h\":\"\"}]}"); + } +} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java index 988ee9f0..723a633b 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java @@ -2,8 +2,11 @@ import static org.assertj.core.api.Assertions.assertThat; +import com.google.gson.Gson; import io.kubernetes.client.openapi.JSON; import io.kubernetes.client.openapi.models.V1Job; +import io.kubernetes.client.openapi.models.V1Secret; +import java.nio.charset.StandardCharsets; import java.time.OffsetDateTime; import java.time.ZoneOffset; import org.junit.Test; @@ -40,10 +43,35 @@ public void times() { .contains("\"creationTimestamp\":\"2018-05-31T09:21:57.000000Z\""); } - /** The client deserializes responses with the same Gson */ + /** + * The configuration of the Kubernetes client lets it deserialize responses with the same Gson + * (it is global) + */ @Test public void installed() { - KubernetesJson.install(); - assertThat(JSON.getGson()).isSameAs(KubernetesJson.GSON); + Gson before = JSON.getGson(); + try { + JSON.setGson(new Gson()); + new KubernetesClientConfig(); + assertThat(JSON.getGson()).isSameAs(KubernetesJson.GSON); + } finally { + JSON.setGson(before); + } + } + + /** Required fields may be missing (e.g. in test fixtures) */ + @Test + public void missingRequiredFields() { + V1Job job = KubernetesJson.GSON.fromJson("{\"spec\":{\"parallelism\":1}}", V1Job.class); + assertThat(job.getSpec().getParallelism()).isEqualTo(1); + } + + /** Binary data of Secrets and ConfigMaps (Base64), both ways */ + @Test + public void binaryData() { + V1Secret secret = + KubernetesJson.GSON.fromJson("{\"data\":{\"key\":\"aGVsbG8=\"}}", V1Secret.class); + assertThat(secret.getData().get("key")).isEqualTo("hello".getBytes(StandardCharsets.UTF_8)); + assertThat(KubernetesJson.GSON.toJson(secret)).contains("\"key\":\"aGVsbG8=\""); } } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java index cdabb93e..71b796c6 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java @@ -241,7 +241,8 @@ public void fromTesTaskToK8sConfigMap() throws IOException { // without the empty collections that the Kubernetes client writes: the resources (no // requests or limits, see above) are gone too JsonContentAssert withoutEmpty = - new JsonContentAssert(this.getClass(), TestUtils.withoutEmpty(jsonString)); + new JsonContentAssert( + this.getClass(), TestUtils.withoutEmptyInExecutors(jsonString)); withoutEmpty .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") .containsOnlyKeys("name", "image", "command"); @@ -249,7 +250,7 @@ public void fromTesTaskToK8sConfigMap() throws IOException { taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(0.1); withoutEmpty.isEqualToJson( - TestUtils.withoutEmpty( + TestUtils.withoutEmptyInExecutors( TestUtils.getFileContentFromResources( "fromTesToK8s_minimal/taskmaster_param.json")), JSONCompareMode.NON_EXTENSIBLE); diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java index a9326fa6..7aa5de4f 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java @@ -404,7 +404,8 @@ public void fromTesTaskToK8sConfigMap() throws IOException { .hasSize(2); JsonContentAssert withoutEmpty = - new JsonContentAssert(this.getClass(), TestUtils.withoutEmpty(jsonString)); + new JsonContentAssert( + this.getClass(), TestUtils.withoutEmptyInExecutors(jsonString)); withoutEmpty .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") .containsOnlyKeys("name", "image", "command", "resources", "volumeMounts"); @@ -448,7 +449,7 @@ public void fromTesTaskToK8sConfigMap() throws IOException { taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(100.0); withoutEmpty.isEqualToJson( - TestUtils.withoutEmpty( + TestUtils.withoutEmptyInExecutors( TestUtils.getFileContentFromResources( "fromTesToK8s/taskmaster_param.json")), JSONCompareMode.NON_EXTENSIBLE); diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperHttpTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperHttpTest.java new file mode 100644 index 00000000..dd0e2db4 --- /dev/null +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/service/KubernetesClientWrapperHttpTest.java @@ -0,0 +1,186 @@ +package uk.ac.ebi.tsc.tesk.k8s.service; + +import static com.github.tomakehurst.wiremock.client.WireMock.*; +import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.github.tomakehurst.wiremock.junit.WireMockRule; +import io.kubernetes.client.openapi.ApiClient; +import io.kubernetes.client.openapi.apis.BatchV1Api; +import io.kubernetes.client.openapi.apis.CoreV1Api; +import io.kubernetes.client.openapi.models.V1ConfigMap; +import io.kubernetes.client.openapi.models.V1ObjectMeta; +import io.kubernetes.client.util.ClientBuilder; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import uk.ac.ebi.tsc.tesk.config.security.User; +import uk.ac.ebi.tsc.tesk.k8s.config.KubernetesClientConfig; +import uk.ac.ebi.tsc.tesk.k8s.exception.KubernetesException; +import uk.ac.ebi.tsc.tesk.tes.exception.TaskNotFoundException; + +/** + * The requests that the wrapper sends to Kubernetes through a real client: paths, query parameters, + * bodies and the handling of errors (the other tests mock the client) + */ +public class KubernetesClientWrapperHttpTest { + + private static final String JOBS = "/apis/batch/v1/namespaces/tesk/jobs"; + + private static final String PODS = "/api/v1/namespaces/tesk/pods"; + + @Rule public WireMockRule kubernetes = new WireMockRule(wireMockConfig().dynamicPort()); + + private KubernetesClientWrapper wrapper; + + @Before + public void setUp() { + KubernetesClientConfig config = new KubernetesClientConfig(); + ApiClient client = new ClientBuilder().setBasePath(kubernetes.baseUrl()).build(); + ApiClient patchClient = config.patchApiClient(client); + wrapper = + new KubernetesClientWrapper( + new BatchV1Api(client), + config.patchBatchApi(patchClient), + new CoreV1Api(client), + config.patchCoreApi(patchClient), + "tesk"); + } + + /** A page of taskmasters' jobs: the selector, the page token and the page size */ + @Test + public void listTaskmasterJobs() { + kubernetes.givenThat(get(urlPathEqualTo(JOBS)).willReturn(okJson("{\"items\":[]}"))); + wrapper.listAllTaskmasterJobsForUser("token-1", 10, User.builder("123").build()); + kubernetes.verify( + getRequestedFor(urlPathEqualTo(JOBS)) + .withQueryParam("labelSelector", equalTo("job-type=taskmaster")) + .withQueryParam("continue", equalTo("token-1")) + .withQueryParam("limit", equalTo("10"))); + } + + /** Without a page token or size, the request has neither */ + @Test + public void listExecutorJobs() { + kubernetes.givenThat(get(urlPathEqualTo(JOBS)).willReturn(okJson("{\"items\":[]}"))); + wrapper.listAllTaskExecutorJobs(); + kubernetes.verify( + getRequestedFor(urlPathEqualTo(JOBS)) + .withQueryParam("labelSelector", equalTo("job-type=executor")) + .withQueryParam("continue", absent()) + .withQueryParam("limit", absent())); + } + + /** A ConfigMap is checked in a dry run, and created without one */ + @Test + public void configMaps() { + kubernetes.givenThat( + post(urlPathEqualTo("/api/v1/namespaces/tesk/configmaps")) + .willReturn(okJson("{\"metadata\":{\"name\":\"task-1\"}}"))); + V1ConfigMap map = new V1ConfigMap().metadata(new V1ObjectMeta().name("task-1")); + wrapper.checkConfigMap(map); + wrapper.createConfigMap(map); + kubernetes.verify( + 1, + postRequestedFor(urlPathEqualTo("/api/v1/namespaces/tesk/configmaps")) + .withQueryParam("dryRun", equalTo("All"))); + kubernetes.verify( + 1, + postRequestedFor(urlPathEqualTo("/api/v1/namespaces/tesk/configmaps")) + .withQueryParam("dryRun", absent())); + } + + /** A job is deleted with its pods, in the background */ + @Test + public void deleteJob() { + kubernetes.givenThat(delete(urlPathEqualTo(JOBS + "/task-1")).willReturn(okJson("{}"))); + wrapper.deleteJob("task-1"); + kubernetes.verify( + deleteRequestedFor(urlPathEqualTo(JOBS + "/task-1")) + .withQueryParam("propagationPolicy", equalTo("Background"))); + } + + /** Errors of Kubernetes become KubernetesExceptions with their status code */ + @Test + public void error() { + kubernetes.givenThat( + delete(urlPathEqualTo(JOBS + "/task-1")) + .willReturn(aResponse().withStatus(403).withBody("{}"))); + assertThatThrownBy(() -> wrapper.deleteJob("task-1")) + .isInstanceOfSatisfying( + KubernetesException.class, + e -> assertThat(e.getApiException().getCode()).isEqualTo(403)); + } + + /** A job that doesn't exist (404) is a task that doesn't exist */ + @Test(expected = TaskNotFoundException.class) + public void readMissingJob() { + kubernetes.givenThat( + get(urlPathEqualTo(JOBS + "/task-1")) + .willReturn(aResponse().withStatus(404).withBody("{}"))); + wrapper.readTaskmasterJob("task-1"); + } + + /** The minimum memory of the namespace's LimitRanges, in GB */ + @Test + public void minimumRamGb() { + kubernetes.givenThat( + get(urlPathEqualTo("/api/v1/namespaces/tesk/limitranges")) + .willReturn( + okJson( + "{\"items\":[{\"spec\":{\"limits\":[{\"type\":\"Container\"," + + "\"min\":{\"memory\":\"2Gi\"}}]}}]}"))); + assertThat(wrapper.minimumRamGb()).isEqualTo(2.0); + kubernetes.verify( + getRequestedFor(urlPathEqualTo("/api/v1/namespaces/tesk/limitranges")) + .withQueryParam("labelSelector", absent()) + .withQueryParam("continue", absent()) + .withQueryParam("limit", absent())); + } + + /** A pod's log as Kubernetes sends it (plain text), or null if Kubernetes refuses it */ + @Test + public void readPodLog() { + kubernetes.givenThat( + get(urlPathEqualTo(PODS + "/pod-1/log")) + .willReturn(ok("hello\n").withHeader("Content-Type", "text/plain"))); + kubernetes.givenThat( + get(urlPathEqualTo(PODS + "/pod-2/log")) + .willReturn(aResponse().withStatus(404).withBody("{}"))); + assertThat(wrapper.readPodLog("pod-1")).isEqualTo("hello\n"); + assertThat(wrapper.readPodLog("pod-2")).isNull(); + } + + /** Jobs and pods are labelled as cancelled with merge patches */ + @Test + public void labelAsCancelled() { + kubernetes.givenThat(patch(urlPathEqualTo(JOBS + "/task-1")).willReturn(okJson("{}"))); + kubernetes.givenThat(patch(urlPathEqualTo(PODS + "/pod-1")).willReturn(okJson("{}"))); + wrapper.labelJobAsCancelled("task-1"); + wrapper.labelPodAsCancelled("pod-1"); + String body = "{\"metadata\":{\"labels\":{\"task-status\":\"Cancelled\"}}}"; + kubernetes.verify( + patchRequestedFor(urlPathEqualTo(JOBS + "/task-1")) + .withHeader("Content-Type", containing("application/merge-patch+json")) + .withRequestBody(equalToJson(body))); + kubernetes.verify( + patchRequestedFor(urlPathEqualTo(PODS + "/pod-1")) + .withHeader("Content-Type", containing("application/merge-patch+json")) + .withRequestBody(equalToJson(body))); + } + + /** A job whose taskmaster doesn't run yet is labelled and ended in one patch */ + @Test + public void cancelJobBeforeStart() { + kubernetes.givenThat(patch(urlPathEqualTo(JOBS + "/task-1")).willReturn(okJson("{}"))); + wrapper.cancelJobBeforeStart("task-1"); + kubernetes.verify( + patchRequestedFor(urlPathEqualTo(JOBS + "/task-1")) + .withRequestBody( + equalToJson( + "{\"metadata\":{\"labels\":{\"task-status\":\"Cancelled\"}}," + + "\"spec\":{\"parallelism\":0," + + "\"activeDeadlineSeconds\":1}}"))); + } +} From ebd87bedeb99112a97e32212584be79863cf82d5 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 03:17:45 +0100 Subject: [PATCH 6/9] fix: keep empty fields out of the taskmaster input The models of client 28 start with empty lists and maps, so the executors' jobs in the taskmaster's input had "limits": {} for a task with ram_gb but no cpu_cores. The taskmaster sets the limits to the requests only if there are none, so such executors ran without a memory limit. Write the executors' jobs with a Gson that leaves out empty lists and maps, as client 11 did, and restore the strict assertions of the converter tests (and drop the helper that stripped empty fields in them). Co-Authored-By: Claude Opus 5.5 --- docs/deployment.md | 4 +- docs/upgrading.md | 6 +- .../tsc/tesk/k8s/config/KubernetesJson.java | 47 +++++++++++++ .../k8s/convert/TesKubernetesConverter.java | 9 ++- .../java/uk/ac/ebi/tsc/tesk/TestUtils.java | 42 ------------ .../uk/ac/ebi/tsc/tesk/TestUtilsTest.java | 25 ------- .../tesk/k8s/config/KubernetesJsonTest.java | 31 +++++++++ .../TesKubernetesConverterMinimalTest.java | 68 +++++++++++++------ .../convert/TesKubernetesConverterTest.java | 13 ++-- 9 files changed, 143 insertions(+), 102 deletions(-) delete mode 100644 src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java diff --git a/docs/deployment.md b/docs/deployment.md index 5e513ba6..c6dcb879 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -67,7 +67,9 @@ also works with older and newer Kubernetes versions: only fields that Kubernetes has had for years. - The objects that it creates list empty fields, e.g. `args: []`, which Kubernetes treats as unset. A Kubernetes version that doesn't know such a - field drops it, and may answer with a warning. + field drops it, and may answer with a warning. The executors' jobs in the + taskmaster's input leave them out, as before: the taskmaster tells unset + fields (e.g. no limits, which it sets to the requests) by their absence. Passing private executor image credentials in TES requests is an [experimental workaround for proof-of-concept use only](api.md#private-executor-images), diff --git a/docs/upgrading.md b/docs/upgrading.md index d4af5fad..d4e79bbd 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -54,9 +54,9 @@ notice changed answers: - **Kubernetes client.** TESK API has version 28 of the Kubernetes Java client instead of 11 (see [Deployment](deployment.md#kubernetes)), and SnakeYAML 2 instead of 1.33. Nothing changes for deployments: it needs the - same rights, and works with the same Kubernetes versions. The jobs and the - taskmaster's input that it creates list empty fields (e.g. `args: []`), - which Kubernetes treats as unset. Run outside a cluster with a token in its + same rights, and works with the same Kubernetes versions. The jobs, + ConfigMaps and Secrets that it creates list empty fields (e.g. `args: []`), + which Kubernetes treats as unset; the taskmaster's input stays as it was. Run outside a cluster with a token in its kubeconfig file (e.g. after `oc login`; not with client certificates), TESK API can now cancel tasks: it sent the token without its `Bearer` prefix in the patches, which Kubernetes refused. A task whose taskmaster diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java index 7c49de6b..a129c512 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJson.java @@ -1,9 +1,17 @@ package uk.ac.ebi.tsc.tesk.k8s.config; import com.google.gson.Gson; +import com.google.gson.TypeAdapter; +import com.google.gson.TypeAdapterFactory; +import com.google.gson.reflect.TypeToken; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonWriter; import io.kubernetes.client.openapi.JSON; +import java.io.IOException; import java.time.LocalDate; import java.time.OffsetDateTime; +import java.util.Collection; +import java.util.Map; /** * Gson for the objects of the Kubernetes API that ignores fields it doesn't know, as the Kubernetes @@ -30,8 +38,47 @@ public final class KubernetesJson { .registerTypeAdapter(byte[].class, CLIENT_GSON.getAdapter(byte[].class)) .create(); + /** + * {@link #GSON}, but leaving out lists and maps that are empty, as the Kubernetes client did up + * to version 19: its models now start with empty lists and maps instead of none. For the + * executors' jobs in the taskmaster's input, as the taskmaster tells "no limits" (which it sets + * to the requests) from limits by whether the key is there. Objects without fields (e.g. {@code + * emptyDir: {}}) are kept. + */ + public static final Gson GSON_WITHOUT_EMPTY_COLLECTIONS = + GSON.newBuilder().registerTypeAdapterFactory(new WithoutEmptyCollections()).create(); + private KubernetesJson() {} + /** Writes empty lists and maps as null, which Gson leaves out of objects */ + private static final class WithoutEmptyCollections implements TypeAdapterFactory { + + @Override + public TypeAdapter create(Gson gson, TypeToken type) { + Class raw = type.getRawType(); + if (!Collection.class.isAssignableFrom(raw) && !Map.class.isAssignableFrom(raw)) { + return null; + } + TypeAdapter delegate = gson.getDelegateAdapter(this, type); + return new TypeAdapter() { + @Override + public void write(JsonWriter out, T value) throws IOException { + if (value instanceof Collection && ((Collection) value).isEmpty() + || value instanceof Map && ((Map) value).isEmpty()) { + out.nullValue(); + } else { + delegate.write(out, value); + } + } + + @Override + public T read(JsonReader in) throws IOException { + return delegate.read(in); + } + }; + } + } + /** * Lets the Kubernetes client read responses with {@link #GSON}: its Gson is global (static), * shared by all API clients diff --git a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java index 9687f7ef..c54fead7 100644 --- a/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java +++ b/src/main/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverter.java @@ -24,6 +24,7 @@ import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.stereotype.Component; import uk.ac.ebi.tsc.tesk.config.security.User; +import uk.ac.ebi.tsc.tesk.k8s.config.KubernetesJson; import uk.ac.ebi.tsc.tesk.k8s.convert.data.Job; import uk.ac.ebi.tsc.tesk.k8s.convert.data.Task; import uk.ac.ebi.tsc.tesk.tes.model.*; @@ -180,7 +181,13 @@ public V1ConfigMap fromTesTaskToK8sConfigMap(TesTask task, User user, V1Job job) taskMasterConfigMap.getMetadata().getName()), e); } - taskMasterInput.put(TASKMASTER_INPUT_EXEC_KEY, executorsAsJobs); + // without the empty lists and maps of the client's models, as before (see + // KubernetesJson) + taskMasterInput.put( + TASKMASTER_INPUT_EXEC_KEY, + executorsAsJobs.stream() + .map(KubernetesJson.GSON_WITHOUT_EMPTY_COLLECTIONS::toJsonTree) + .collect(Collectors.toList())); String taskMasterInputAsJSON = this.gson.toJson(taskMasterInput); try { diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java index 41d9727e..8b136e02 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtils.java @@ -1,9 +1,5 @@ package uk.ac.ebi.tsc.tesk; -import com.google.gson.JsonArray; -import com.google.gson.JsonElement; -import com.google.gson.JsonObject; -import com.google.gson.JsonParser; import java.io.IOException; import java.nio.file.Files; import org.slf4j.Logger; @@ -18,42 +14,4 @@ public static String getFileContentFromResources(String path) throws IOException Resource fileFromResources = new ClassPathResource(path); return new String(Files.readAllBytes(fileFromResources.getFile().toPath())); } - - /** - * The taskmaster's input without the empty arrays and objects in its executors' jobs: the - * Kubernetes client writes the lists and maps of its models also if they are empty. Lossy: an - * empty field that TESK API sets on purpose is gone too, so tests check those separately. The - * rest of the input (e.g. its {@code inputs}, {@code outputs} and {@code volumes}) is kept. - */ - public static String withoutEmptyInExecutors(String json) { - JsonObject input = JsonParser.parseString(json).getAsJsonObject(); - JsonElement executors = input.get("executors"); - if (executors != null && executors.isJsonArray()) { - executors.getAsJsonArray().forEach(TestUtils::removeEmpty); - } - return input.toString(); - } - - /** - * Removes the members of objects that are empty arrays or objects, or become empty without - * their own empty members, at any depth, and tells whether the element is (then) empty. - * Elements of arrays are cleaned, but kept even if they become empty. - */ - private static boolean removeEmpty(JsonElement element) { - if (element.isJsonObject()) { - JsonObject object = element.getAsJsonObject(); - for (String key : object.keySet().toArray(new String[0])) { - if (removeEmpty(object.get(key))) { - object.remove(key); - } - } - return object.size() == 0; - } - if (element.isJsonArray()) { - JsonArray array = element.getAsJsonArray(); - array.forEach(TestUtils::removeEmpty); - return array.size() == 0; - } - return false; - } } diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java deleted file mode 100644 index ccc899cf..00000000 --- a/src/test/java/uk/ac/ebi/tsc/tesk/TestUtilsTest.java +++ /dev/null @@ -1,25 +0,0 @@ -package uk.ac.ebi.tsc.tesk; - -import static org.assertj.core.api.Assertions.assertThat; - -import org.junit.Test; - -/** The helpers of the tests */ -public class TestUtilsTest { - - /** - * Empty arrays and objects go from the executors' jobs, also objects that become empty, but not - * elements of arrays; the rest of the input stays as it is - */ - @Test - public void withoutEmptyInExecutors() { - assertThat( - TestUtils.withoutEmptyInExecutors( - "{\"outputs\":[],\"volumes\":{},\"executors\":[{\"a\":[]," - + "\"b\":{\"c\":{},\"d\":[]},\"e\":[{\"f\":[]}]," - + "\"g\":0,\"h\":\"\"}]}")) - .isEqualTo( - "{\"outputs\":[],\"volumes\":{},\"executors\":[{\"e\":[{}]," - + "\"g\":0,\"h\":\"\"}]}"); - } -} diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java index 723a633b..6d64007f 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java @@ -3,9 +3,15 @@ import static org.assertj.core.api.Assertions.assertThat; import com.google.gson.Gson; +import io.kubernetes.client.custom.Quantity; import io.kubernetes.client.openapi.JSON; +import io.kubernetes.client.openapi.models.V1Container; +import io.kubernetes.client.openapi.models.V1EmptyDirVolumeSource; import io.kubernetes.client.openapi.models.V1Job; +import io.kubernetes.client.openapi.models.V1PodSpec; +import io.kubernetes.client.openapi.models.V1ResourceRequirements; import io.kubernetes.client.openapi.models.V1Secret; +import io.kubernetes.client.openapi.models.V1Volume; import java.nio.charset.StandardCharsets; import java.time.OffsetDateTime; import java.time.ZoneOffset; @@ -66,6 +72,31 @@ public void missingRequiredFields() { assertThat(job.getSpec().getParallelism()).isEqualTo(1); } + /** + * Without empty collections: empty lists and maps are left out, objects without fields (e.g. an + * emptyDir volume) and non-empty collections are kept + */ + @Test + public void withoutEmptyCollections() { + V1PodSpec spec = + new V1PodSpec() + .addContainersItem( + new V1Container() + .name("c") + .resources( + new V1ResourceRequirements() + .putRequestsItem( + "memory", new Quantity("1Gi")))) + .addVolumesItem( + new V1Volume().name("v").emptyDir(new V1EmptyDirVolumeSource())); + assertThat(KubernetesJson.GSON.toJson(spec)).contains("\"limits\":{}"); + assertThat(KubernetesJson.GSON_WITHOUT_EMPTY_COLLECTIONS.toJson(spec)) + .isEqualTo( + "{\"containers\":[{\"name\":\"c\",\"resources\":{\"requests\":" + + "{\"memory\":\"1Gi\"}}}]," + + "\"volumes\":[{\"emptyDir\":{},\"name\":\"v\"}]}"); + } + /** Binary data of Secrets and ConfigMaps (Base64), both ways */ @Test public void binaryData() { diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java index 71b796c6..618e03bd 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterMinimalTest.java @@ -56,6 +56,7 @@ import uk.ac.ebi.tsc.tesk.k8s.constant.K8sConstants; import uk.ac.ebi.tsc.tesk.k8s.convert.data.TaskBuilder; import uk.ac.ebi.tsc.tesk.tes.model.TesExecutor; +import uk.ac.ebi.tsc.tesk.tes.model.TesResources; import uk.ac.ebi.tsc.tesk.tes.model.TesState; import uk.ac.ebi.tsc.tesk.tes.model.TesTask; import uk.ac.ebi.tsc.tesk.trs.TrsToolClient; @@ -219,14 +220,9 @@ public void fromTesTaskToK8sConfigMap() throws IOException { .extractingJsonPathArrayValue("executors[*].spec.template.spec.restartPolicy") .containsOnly("Never") .hasSize(1); - // no requests or limits (the Kubernetes client writes empty collections) taskMasterInputJson .extractingJsonPathMapValue( - "executors[0].spec.template.spec.containers[0].resources.requests") - .isEmpty(); - taskMasterInputJson - .extractingJsonPathMapValue( - "executors[0].spec.template.spec.containers[0].resources.limits") + "executors[0].spec.template.spec.containers[0].resources") .isEmpty(); taskMasterInputJson @@ -238,24 +234,56 @@ public void fromTesTaskToK8sConfigMap() throws IOException { "executors[0].spec.template.spec.containers[0].image") .isEqualTo("ubuntu"); - // without the empty collections that the Kubernetes client writes: the resources (no - // requests or limits, see above) are gone too - JsonContentAssert withoutEmpty = - new JsonContentAssert( - this.getClass(), TestUtils.withoutEmptyInExecutors(jsonString)); - withoutEmpty + taskMasterInputJson .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") - .containsOnlyKeys("name", "image", "command"); + .containsOnlyKeys("name", "image", "command", "resources"); taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(0.1); - withoutEmpty.isEqualToJson( - TestUtils.withoutEmptyInExecutors( - TestUtils.getFileContentFromResources( - "fromTesToK8s_minimal/taskmaster_param.json")), + taskMasterInputJson.isEqualToJson( + new ClassPathResource("fromTesToK8s_minimal/taskmaster_param.json"), JSONCompareMode.NON_EXTENSIBLE); } + /** + * An executor with memory but no CPU gets no limits in the taskmaster's input: not even empty + * ones, as the taskmaster sets the limits to the requests only if there are none (a pod without + * limits could use all memory of its node) + */ + @Test + public void ramOnlyExecutorHasNoLimits() throws IOException { + given(this.jobNameGenerator.getTaskMasterName()).willReturn("task-98605447"); + TesTask task = + new TesTask() + .addExecutorsItem(new TesExecutor().image("ubuntu").addCommandItem("true")) + .resources(new TesResources().ramGb(2.0)); + User user = User.builder("test-user-id").build(); + V1Job taskmaster = this.converter.fromTesTaskToK8sJob(task, user); + V1ConfigMap configMap = this.converter.fromTesTaskToK8sConfigMap(task, user, taskmaster); + try (InputStreamReader reader = + new InputStreamReader( + new GZIPInputStream( + new ByteArrayInputStream( + configMap.getBinaryData().get("JSON_INPUT.gz"))), + StandardCharsets.UTF_8)) { + JsonObject resources = + this.gson + .fromJson(reader, JsonObject.class) + .getAsJsonArray("executors") + .get(0) + .getAsJsonObject() + .getAsJsonObject("spec") + .getAsJsonObject("template") + .getAsJsonObject("spec") + .getAsJsonArray("containers") + .get(0) + .getAsJsonObject() + .getAsJsonObject("resources"); + assertThat(resources.has("limits")).isFalse(); + assertThat(resources.getAsJsonObject("requests").has("memory")).isTrue(); + } + } + @Test public void privateExecutorImageUsesOwnedSecretWithoutExposingCredentials() throws IOException { given(this.jobNameGenerator.getTaskMasterName()).willReturn("task-98605447"); @@ -300,8 +328,6 @@ public void privateExecutorImageUsesOwnedSecretWithoutExposingCredentials() thro String serialized = input.toString(); org.assertj.core.api.Assertions.assertThat(serialized) .doesNotContain(credentialRepresentations); - // no image pull Secrets for the public image (the Kubernetes client writes the - // empty list) org.assertj.core.api.Assertions.assertThat( input.getAsJsonArray("executors") .get(0) @@ -309,8 +335,8 @@ public void privateExecutorImageUsesOwnedSecretWithoutExposingCredentials() thro .getAsJsonObject("spec") .getAsJsonObject("template") .getAsJsonObject("spec") - .getAsJsonArray("imagePullSecrets")) - .isEmpty(); + .has("imagePullSecrets")) + .isFalse(); org.assertj.core.api.Assertions.assertThat( input.getAsJsonArray("executors") .get(1) diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java index 7aa5de4f..ce6a4319 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/convert/TesKubernetesConverterTest.java @@ -403,13 +403,10 @@ public void fromTesTaskToK8sConfigMap() throws IOException { .containsOnly("16106127360") .hasSize(2); - JsonContentAssert withoutEmpty = - new JsonContentAssert( - this.getClass(), TestUtils.withoutEmptyInExecutors(jsonString)); - withoutEmpty + taskMasterInputJson .extractingJsonPathMapValue("executors[0].spec.template.spec.containers[0]") .containsOnlyKeys("name", "image", "command", "resources", "volumeMounts"); - withoutEmpty + taskMasterInputJson .extractingJsonPathMapValue("executors[1].spec.template.spec.containers[0]") .containsOnlyKeys( "name", @@ -448,10 +445,8 @@ public void fromTesTaskToK8sConfigMap() throws IOException { taskMasterInputJson.extractingJsonPathNumberValue("resources.disk_gb").isEqualTo(100.0); - withoutEmpty.isEqualToJson( - TestUtils.withoutEmptyInExecutors( - TestUtils.getFileContentFromResources( - "fromTesToK8s/taskmaster_param.json")), + taskMasterInputJson.isEqualToJson( + new ClassPathResource("fromTesToK8s/taskmaster_param.json"), JSONCompareMode.NON_EXTENSIBLE); } From 3fd89b0747e2f0002f62c31477b42a07b1821386 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 03:20:28 +0100 Subject: [PATCH 7/9] docs: rewrap the Kubernetes client notes Co-Authored-By: Claude Opus 5.5 --- docs/development.md | 10 ++++++---- docs/upgrading.md | 9 +++++---- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/docs/development.md b/docs/development.md index 30af5e74..739457f1 100644 --- a/docs/development.md +++ b/docs/development.md @@ -14,10 +14,12 @@ Codec): `pom.xml` sets them to those of the client's parent POM (`io.kubernetes:client-java-parent`), so update them together with the client. It also keeps the older Bouncy Castle jars (`*-jdk15on`) of spring-security-jwt out, as the client brings the current ones -(`*-jdk18on`), with the same classes. The models of the client refuse JSON with fields they don't know; -`KubernetesJson` installs a Gson that ignores them (see -[Deployment](deployment.md#kubernetes)), so test fixtures of Kubernetes -objects may leave out fields that Kubernetes requires. +(`*-jdk18on`), with the same classes. The models of the client refuse JSON +with fields they don't know; `KubernetesJson` installs a Gson that ignores +them (see [Deployment](deployment.md#kubernetes)), so test fixtures of +Kubernetes objects may leave out fields that Kubernetes requires. The +executors' jobs in the taskmaster's input are written without the empty +lists and maps of the client's models (`GSON_WITHOUT_EMPTY_COLLECTIONS`). The wrapper is the `only-script` type of the Apache Maven Wrapper, so there is no binary `maven-wrapper.jar` in the repository. It needs `curl` or `wget` and diff --git a/docs/upgrading.md b/docs/upgrading.md index d4e79bbd..8496dbf2 100644 --- a/docs/upgrading.md +++ b/docs/upgrading.md @@ -56,10 +56,11 @@ notice changed answers: SnakeYAML 2 instead of 1.33. Nothing changes for deployments: it needs the same rights, and works with the same Kubernetes versions. The jobs, ConfigMaps and Secrets that it creates list empty fields (e.g. `args: []`), - which Kubernetes treats as unset; the taskmaster's input stays as it was. Run outside a cluster with a token in its - kubeconfig file (e.g. after `oc login`; not with client certificates), - TESK API can now cancel tasks: it sent the token without its `Bearer` - prefix in the patches, which Kubernetes refused. A task whose taskmaster + which Kubernetes treats as unset; the taskmaster's input stays as it + was. Run outside a cluster with a token in its kubeconfig file (e.g. + after `oc login`; not with client certificates), TESK API can now cancel + tasks: it sent the token without its `Bearer` prefix in the patches, + which Kubernetes refused. A task whose taskmaster job has no creation time has no `creation_time`, instead of the current time; Kubernetes sets it for every job, so this only matters for tests. From 73e07c09b4419bbf73878ec699ad40d3eb0e2792 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 17:50:32 +0100 Subject: [PATCH 8/9] docs: drop the Bouncy Castle note spring-security-jwt left with #175, so there are no older Bouncy Castle jars left to keep out. Co-Authored-By: Claude Opus 5.5 --- docs/development.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/docs/development.md b/docs/development.md index 739457f1..bd8d1293 100644 --- a/docs/development.md +++ b/docs/development.md @@ -12,9 +12,7 @@ The Kubernetes client needs newer versions of some libraries than Spring Boot 2.7 manages (SnakeYAML, OkHttp, Kotlin, Gson, Commons Lang and Commons Codec): `pom.xml` sets them to those of the client's parent POM (`io.kubernetes:client-java-parent`), so update them together with the -client. It also keeps the older Bouncy Castle jars (`*-jdk15on`) of -spring-security-jwt out, as the client brings the current ones -(`*-jdk18on`), with the same classes. The models of the client refuse JSON +client. The models of the client refuse JSON with fields they don't know; `KubernetesJson` installs a Gson that ignores them (see [Deployment](deployment.md#kubernetes)), so test fixtures of Kubernetes objects may leave out fields that Kubernetes requires. The From e43f5800bd88e35a31918f3d8ad1c2c0e41e7423 Mon Sep 17 00:00:00 2001 From: Alex Kanitz Date: Sat, 14 Nov 2026 18:14:11 +0100 Subject: [PATCH 9/9] test: read with the Gson without empty fields Covers the read side of the adapter that leaves out empty collections, the one line of the patch that no test ran. Co-Authored-By: Claude Opus 5.5 --- .../ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java index 6d64007f..728fbe5e 100644 --- a/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java +++ b/src/test/java/uk/ac/ebi/tsc/tesk/k8s/config/KubernetesJsonTest.java @@ -97,6 +97,18 @@ public void withoutEmptyCollections() { + "\"volumes\":[{\"emptyDir\":{},\"name\":\"v\"}]}"); } + /** Without empty collections, reading is as with {@link KubernetesJson#GSON} */ + @Test + public void withoutEmptyCollectionsReads() { + V1PodSpec spec = + KubernetesJson.GSON_WITHOUT_EMPTY_COLLECTIONS.fromJson( + "{\"containers\":[{\"name\":\"c\",\"args\":[]}],\"unknown\":1}", + V1PodSpec.class); + assertThat(spec.getContainers()).hasSize(1); + assertThat(spec.getContainers().get(0).getName()).isEqualTo("c"); + assertThat(spec.getContainers().get(0).getArgs()).isEmpty(); + } + /** Binary data of Secrets and ConfigMaps (Base64), both ways */ @Test public void binaryData() {