From 76462597a81e6cc31ef8aa113e6cdd7b18293085 Mon Sep 17 00:00:00 2001 From: hideki narimiya Date: Wed, 6 Jul 2022 19:57:30 +0900 Subject: [PATCH] reflect review --- .../kubernetes/KubernetesClientConfig.java | 52 ++++++++----------- .../KubernetesClientConfigTest.java | 40 +++++++++++++- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/digdag-standards/src/main/java/io/digdag/standards/command/kubernetes/KubernetesClientConfig.java b/digdag-standards/src/main/java/io/digdag/standards/command/kubernetes/KubernetesClientConfig.java index 4e926d170b..bfe108bc96 100644 --- a/digdag-standards/src/main/java/io/digdag/standards/command/kubernetes/KubernetesClientConfig.java +++ b/digdag-standards/src/main/java/io/digdag/standards/command/kubernetes/KubernetesClientConfig.java @@ -4,6 +4,7 @@ import com.google.common.base.Optional; import io.digdag.client.config.Config; import io.digdag.client.config.ConfigException; +import io.digdag.client.config.ConfigFactory; import io.digdag.core.storage.StorageManager; import java.nio.charset.Charset; @@ -11,6 +12,8 @@ import java.nio.file.Path; import java.nio.file.Paths; +import static io.digdag.client.DigdagClient.objectMapper; + public class KubernetesClientConfig { private static final String KUBERNETES_CLIENT_PARAMS_PREFIX = "agent.command_executor.kubernetes."; @@ -19,47 +22,38 @@ public static KubernetesClientConfig create(final Optional name, final Config systemConfig, final Config requestConfig) { - - String clusterName = null; - if (name.isPresent()) { - clusterName = name.get(); + if (!systemConfig.get("agent.command_executor.type", String.class, "").equals("kubernetes")) { + throw new ConfigException("agent.command_executor.type: is not 'kubernetes'"); } - - Config extractedSystemConfig = null; - if (systemConfig != null && systemConfig.get("agent.command_executor.type", String.class, "").equals("kubernetes")) { - if (clusterName == null) { - clusterName = systemConfig.get(KUBERNETES_CLIENT_PARAMS_PREFIX + "name", String.class); // ConfigException - } - final String keyPrefix = KUBERNETES_CLIENT_PARAMS_PREFIX + clusterName + "."; - extractedSystemConfig = StorageManager.extractKeyPrefix(systemConfig, keyPrefix); + String clusterName; + if (!name.isPresent()) { + clusterName = systemConfig.get(KUBERNETES_CLIENT_PARAMS_PREFIX + "name", String.class); // ConfigException + } else { + clusterName = name.get(); } - - Config extractedRequestConfig = null; - if (requestConfig != null && requestConfig.has("kubernetes")) { + final String keyPrefix = KUBERNETES_CLIENT_PARAMS_PREFIX + clusterName + "."; + Config extractedSystemConfig = StorageManager.extractKeyPrefix(systemConfig, keyPrefix);; + Config extractedRequestConfig; + if (systemConfig.get(KUBERNETES_CLIENT_PARAMS_PREFIX + "allow_configure_workflow_definition", Boolean.class, false) + && requestConfig != null + && requestConfig.has("kubernetes")) { if (clusterName == null) { clusterName = requestConfig.get("name", String.class); // ConfigException } extractedRequestConfig = requestConfig.getNested("kubernetes"); - } - - // Create a config that merges RequestConfig with SystemConfig - final Config config; - if (extractedSystemConfig != null && extractedRequestConfig != null) { - config = extractedSystemConfig.merge(extractedRequestConfig); - - } else if (extractedRequestConfig != null) { - config = extractedRequestConfig; - - } else if (extractedSystemConfig != null) { - config = extractedSystemConfig; - } else { - throw new ConfigException("systemConfig and requestConfig does not exist"); + extractedRequestConfig = newEmptyConfig(); } + // Create a config that merges RequestConfig with SystemConfig + final Config config = extractedSystemConfig.merge(extractedRequestConfig); return KubernetesClientConfig.createKubeConfig(clusterName, config); } + private static Config newEmptyConfig() { + return new ConfigFactory(objectMapper()).create(); + } + private static KubernetesClientConfig createKubeConfig(final String clusterName, final Config config){ if (config.has("kube_config_path")) { String kubeConfigPath = config.get("kube_config_path", String.class); diff --git a/digdag-standards/src/test/java/io/digdag/standards/command/kubernetes/KubernetesClientConfigTest.java b/digdag-standards/src/test/java/io/digdag/standards/command/kubernetes/KubernetesClientConfigTest.java index 38918a14a0..9b17fddd19 100644 --- a/digdag-standards/src/test/java/io/digdag/standards/command/kubernetes/KubernetesClientConfigTest.java +++ b/digdag-standards/src/test/java/io/digdag/standards/command/kubernetes/KubernetesClientConfigTest.java @@ -104,6 +104,10 @@ public void testCreateFromSystemConfigWithKubeConfig() public void testCreateFromRequestConfig() throws Exception { + final Config systemConfig = cf.create() + .set("agent.command_executor.type", "kubernetes") + .set("agent.command_executor.kubernetes.allow_configure_workflow_definition", true); + final Config kubernetesConfig = cf.create() .set("master", "https://127.0.0.1") .set("certs_ca_data", "test=") @@ -113,7 +117,7 @@ public void testCreateFromRequestConfig() final Config requestConfig = cf.create() .setNested("kubernetes", kubernetesConfig); - KubernetesClientConfig kubernetesClientConfig = KubernetesClientConfig.create(clusterName, null, requestConfig); + KubernetesClientConfig kubernetesClientConfig = KubernetesClientConfig.create(clusterName, systemConfig, requestConfig); String masterUrl = "https://127.0.0.1"; String namespace = "default"; @@ -135,7 +139,8 @@ public void testCreateFromRequestConfigAndSystemConfigMerge() .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.master", "https://127.0.0.1") .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.certs_ca_data", "test=") .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.oauth_token", "test=") - .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.namespace", "default"); + .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.namespace", "default") + .set("agent.command_executor.kubernetes.allow_configure_workflow_definition", true); final Config kubernetesConfig = cf.create() .set("master", "https://localhost") @@ -158,4 +163,35 @@ public void testCreateFromRequestConfigAndSystemConfigMerge() assertThat(namespace, is(kubernetesClientConfig.getNamespace())); } + @Test + public void testCreateFromRequestConfigAndSystemConfigNotMerge() + throws Exception + { + + final Config systemConfig = cf.create() + .set("agent.command_executor.type", "kubernetes") + .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.master", "https://127.0.0.1") + .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.certs_ca_data", "test=") + .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.oauth_token", "test=") + .set(KUBERNETES_CLIENT_PARAMS_PREFIX+"test.namespace", "default"); + + final Config kubernetesConfig = cf.create() + .set("master", "https://localhost") + .set("namespace", "request"); + + final Config requestConfig = cf.create() + .setNested("kubernetes", kubernetesConfig); + + KubernetesClientConfig kubernetesClientConfig = KubernetesClientConfig.create(clusterName, systemConfig, requestConfig); + + String masterUrl = "https://127.0.0.1"; + String namespace = "default"; + String caCertData = "test="; + String oauthToken = "test="; + assertThat(masterUrl, is(kubernetesClientConfig.getMaster())); + assertThat(caCertData, is(kubernetesClientConfig.getCertsCaData())); + assertThat(oauthToken, is(kubernetesClientConfig.getOauthToken())); + assertThat(namespace, is(kubernetesClientConfig.getNamespace())); + } + }