From 21e9959ca46e8a469626ff0f0e737f1c155fd5ef Mon Sep 17 00:00:00 2001 From: Rohan Kumar Date: Fri, 31 Jul 2020 17:35:46 +0530 Subject: [PATCH] Fix #2308: kubeconfig with external authentication command doesn't work --- CHANGELOG.md | 1 + .../io/fabric8/kubernetes/client/Config.java | 93 ++++++++++++------- .../kubernetes/client/utils/Utils.java | 26 +++++- .../fabric8/kubernetes/client/ConfigTest.java | 36 ++++++- .../kubernetes/client/internal/UtilsTest.java | 18 +++- 5 files changed, 130 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bbd9422fa50..342e204c8f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ### 4.10-SNAPSHOT #### Bugs * Fix #2373: Unable to create a Template on OCP3 +* Fix #2308: Fix kubernetes client `Config` loading KUBECONFIG with external authentication command * Fix #2316: Cannot load resource from stream without apiVersion #### Improvements diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/Config.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/Config.java index be92f82c74d..85fdb2ac68a 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/Config.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/Config.java @@ -32,6 +32,7 @@ import java.util.Locale; import java.util.Map; +import org.apache.commons.lang.SystemUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -600,41 +601,11 @@ private static boolean loadFromKubeconfig(Config config, String context, String } else if (config.getOauthTokenProvider() == null) { // https://kubernetes.io/docs/reference/access-authn-authz/authentication/#client-go-credential-plugins ExecConfig exec = currentAuthInfo.getExec(); if (exec != null) { - String apiVersion = exec.getApiVersion(); - if ("client.authentication.k8s.io/v1alpha1".equals(apiVersion) || "client.authentication.k8s.io/v1beta1".equals(apiVersion)) { - List argv = new ArrayList(); - String command = exec.getCommand(); - if (command.contains("/") && !command.startsWith("/") && kubeconfigPath != null && !kubeconfigPath.isEmpty()) { - // Appears to be a relative path; normalize. Spec is vague about how to detect this situation. - command = Paths.get(kubeconfigPath).resolveSibling(command).normalize().toString(); - } - argv.add(command); - List args = exec.getArgs(); - if (args != null) { - argv.addAll(args); - } - ProcessBuilder pb = new ProcessBuilder(argv); - List env = exec.getEnv(); - if (env != null) { - Map environment = pb.environment(); - env.forEach(var -> environment.put(var.getName(), var.getValue())); - } - // TODO check behavior of tty & stdin - Process p = pb.start(); - if (p.waitFor() != 0) { - LOGGER.warn(IOHelpers.readFully(p.getErrorStream())); - } - ExecCredential ec = Serialization.unmarshal(p.getInputStream(), ExecCredential.class); - if (!apiVersion.equals(ec.apiVersion)) { - LOGGER.warn("Wrong apiVersion {} vs. {}", ec.apiVersion, apiVersion); - } - if (ec.status != null && ec.status.token != null) { - config.setOauthToken(ec.status.token); - } else { - LOGGER.warn("No token returned"); - } - } else { // TODO v1beta1? - LOGGER.warn("Unsupported apiVersion: {}", apiVersion); + ExecCredential ec = getExecCredentialFromExecConfig(exec, kubeconfigPath); + if (ec != null && ec.status != null && ec.status.token != null) { + config.setOauthToken(ec.status.token); + } else { + LOGGER.warn("No token returned"); } } } @@ -651,6 +622,58 @@ private static boolean loadFromKubeconfig(Config config, String context, String return false; } + protected static ExecCredential getExecCredentialFromExecConfig(ExecConfig exec, String kubeconfigPath) throws IOException, InterruptedException { + String apiVersion = exec.getApiVersion(); + if ("client.authentication.k8s.io/v1alpha1".equals(apiVersion) || "client.authentication.k8s.io/v1beta1".equals(apiVersion)) { + List env = exec.getEnv(); + // TODO check behavior of tty & stdin + ProcessBuilder pb = new ProcessBuilder(getAuthenticatorCommandFromExecConfig(exec, kubeconfigPath, Utils.getSystemPathVariable())); + if (env != null) { + Map environment = pb.environment(); + env.forEach(var -> environment.put(var.getName(), var.getValue())); + } + Process p = pb.start(); + if (p.waitFor() != 0) { + LOGGER.warn(IOHelpers.readFully(p.getErrorStream())); + } + ExecCredential ec = Serialization.unmarshal(p.getInputStream(), ExecCredential.class); + if (!apiVersion.equals(ec.apiVersion)) { + LOGGER.warn("Wrong apiVersion {} vs. {}", ec.apiVersion, apiVersion); + } else { + return ec; + } + } else { // TODO v1beta1? + LOGGER.warn("Unsupported apiVersion: {}", apiVersion); + } + return null; + } + + protected static List getAuthenticatorCommandFromExecConfig(ExecConfig exec, String kubeconfigPath, String systemPathValue) { + List argv = new ArrayList<>(); + String command = exec.getCommand(); + if (command.contains("/") && !command.startsWith("/") && kubeconfigPath != null && !kubeconfigPath.isEmpty()) { + // Appears to be a relative path; normalize. Spec is vague about how to detect this situation. + command = Paths.get(kubeconfigPath).resolveSibling(command).normalize().toString(); + } + argv.add(getCommandWithFullyQualifiedPath(command, systemPathValue)); + List args = exec.getArgs(); + if (args != null) { + argv.addAll(args); + } + return argv; + } + + protected static String getCommandWithFullyQualifiedPath(String command, String pathValue) { + String[] pathParts = pathValue.split(File.pathSeparator); + for (String pathPart : pathParts) { + File commandFile = new File(pathPart + File.separator + command); + if (commandFile.exists()) { + return commandFile.getAbsolutePath(); + } + } + return command; + } + private static Context setCurrentContext(String context, Config config, io.fabric8.kubernetes.api.model.Config kubeConfig) { if (context != null) { kubeConfig.setCurrentContext(context); diff --git a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java index 6496df32f12..3d144503d53 100644 --- a/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java +++ b/kubernetes-client/src/main/java/io/fabric8/kubernetes/client/utils/Utils.java @@ -53,6 +53,10 @@ public class Utils { private static final Logger LOGGER = LoggerFactory.getLogger(Utils.class); private static final String ALL_CHARS = "abcdefghijklmnopqrstuvwxyz0123456789"; + public static final String WINDOWS = "win"; + public static final String OS_NAME = "os.name"; + public static final String PATH_WINDOWS = "Path"; + public static final String PATH_UNIX = "PATH"; private Utils() { } @@ -101,7 +105,7 @@ public static boolean getSystemPropertyOrEnvVar(String systemPropertyName, Boole } public static int getSystemPropertyOrEnvVar(String systemPropertyName, int defaultValue) { - String result = getSystemPropertyOrEnvVar(systemPropertyName, new Integer(defaultValue).toString()); + String result = getSystemPropertyOrEnvVar(systemPropertyName, Integer.toString(defaultValue)); return Integer.parseInt(result); } @@ -186,7 +190,7 @@ public static boolean shutdownExecutorService(ExecutorService executorService) { if (LOGGER.isDebugEnabled()) { List tasks = executorService.shutdownNow(); if (!tasks.isEmpty()) { - LOGGER.debug("ExecutorService was not cleanly shutdown, after waiting for 10 seconds. Number of remaining tasks:" + tasks.size()); + LOGGER.debug("ExecutorService was not cleanly shutdown, after waiting for 10 seconds. Number of remaining tasks: {}", tasks.size()); } } } catch (InterruptedException e) { @@ -213,7 +217,7 @@ public static void closeQuietly(Iterable closeables) { c.close(); } } catch (IOException e) { - LOGGER.debug("Error closing:" + c); + LOGGER.debug("Error closing: {}", c); } } } @@ -331,7 +335,7 @@ public static String getProperty(Map properties, String property * @param str Url as string * @return returns encoded string */ - public static final String toUrlEncoded(String str) { + public static String toUrlEncoded(String str) { try { return URLEncoder.encode(str, "UTF-8"); } catch (UnsupportedEncodingException exception) { @@ -341,7 +345,7 @@ public static final String toUrlEncoded(String str) { } public static String getPluralFromKind(String kind) { - StringBuffer pluralBuffer = new StringBuffer(kind.toLowerCase(Locale.ROOT)); + StringBuilder pluralBuffer = new StringBuilder(kind.toLowerCase(Locale.ROOT)); switch (kind) { case "ComponentStatus": case "Ingress": @@ -425,4 +429,16 @@ public static String interpolateString(String templateInput, Map .reduce(Function.identity(), Function::andThen) .apply(Objects.requireNonNull(templateInput, "templateInput is required")); } + + public static boolean isWindowsOperatingSystem() { + return getOperatingSystemFromSystemProperty().toLowerCase().contains(WINDOWS); + } + + public static String getSystemPathVariable() { + return System.getenv(isWindowsOperatingSystem() ? PATH_WINDOWS : PATH_UNIX); + } + + private static String getOperatingSystemFromSystemProperty() { + return System.getProperty(OS_NAME); + } } diff --git a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java index 9b1943036dc..5030925443f 100644 --- a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java +++ b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/ConfigTest.java @@ -16,6 +16,8 @@ package io.fabric8.kubernetes.client; +import io.fabric8.kubernetes.api.model.ExecConfig; +import io.fabric8.kubernetes.api.model.ExecConfigBuilder; import io.fabric8.kubernetes.client.utils.Serialization; import io.fabric8.kubernetes.client.utils.Utils; import okhttp3.OkHttpClient; @@ -28,13 +30,12 @@ import java.io.File; import java.io.FileInputStream; +import java.io.IOException; import java.io.InputStream; -import java.net.MalformedURLException; import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.Paths; import java.nio.file.attribute.PosixFilePermissions; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -577,4 +578,35 @@ private void assertConfig(Config config) { assertEquals("/path/to/keystore", config.getKeyStoreFile()); assertEquals("keystorePassphrase", config.getKeyStorePassphrase()); } + + @Test + void testGetAuthenticatorCommandFromExecConfig() throws IOException { + // Given + File commandFolder = Files.createTempDirectory("test").toFile(); + File commandFile = new File(commandFolder, "aws"); + boolean isNewFileCreated = commandFile.createNewFile(); + String systemPathValue = "/usr/java/jdk-14.0.1/bin" + File.pathSeparator + + commandFolder.getAbsolutePath() + File.pathSeparator + + "/opt/apache-maven/bin"; + ExecConfig execConfig = new ExecConfigBuilder() + .withApiVersion("client.authentication.k8s.io/v1alpha1") + .addToArgs("--region", "us-west2", "eks", "get-token", "--cluster-name", "api-eks.example.com") + .withCommand("aws") + .build(); + + // When + List processBuilderArgs = Config.getAuthenticatorCommandFromExecConfig(execConfig, "~/.kube/config", systemPathValue); + + // Then + assertTrue(isNewFileCreated); + assertNotNull(processBuilderArgs); + assertEquals(7, processBuilderArgs.size()); + assertEquals(commandFile.getAbsolutePath(), processBuilderArgs.get(0)); + assertEquals("--region", processBuilderArgs.get(1)); + assertEquals("us-west2", processBuilderArgs.get(2)); + assertEquals("eks", processBuilderArgs.get(3)); + assertEquals("get-token", processBuilderArgs.get(4)); + assertEquals("--cluster-name", processBuilderArgs.get(5)); + assertEquals("api-eks.example.com", processBuilderArgs.get(6)); + } } diff --git a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/UtilsTest.java b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/UtilsTest.java index ac41bf725a1..9adabc161a4 100644 --- a/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/UtilsTest.java +++ b/kubernetes-client/src/test/java/io/fabric8/kubernetes/client/internal/UtilsTest.java @@ -65,9 +65,11 @@ import io.fabric8.kubernetes.api.model.storage.v1beta1.CSIDriver; import io.fabric8.kubernetes.api.model.storage.v1beta1.CSINode; import io.fabric8.kubernetes.client.utils.Utils; +import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; +import java.io.File; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -75,6 +77,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; class UtilsTest { @@ -103,12 +106,12 @@ void existingEnvVarShouldReturnValueFromConvertedSysPropName() { @Test void existingEnvVarShouldReturnBooleanValueFromConvertedSysPropName() { - assertEquals(true, Utils.getSystemPropertyOrEnvVar("env.var.exists.boolean", false)); + Assertions.assertTrue(Utils.getSystemPropertyOrEnvVar("env.var.exists.boolean", false)); } @Test void missingEnvVarShouldReturnDefaultValue() { - assertEquals(true, Utils.getSystemPropertyOrEnvVar("DONT_EXIST", true)); + Assertions.assertTrue(Utils.getSystemPropertyOrEnvVar("DONT_EXIST", true)); } @Test @@ -339,4 +342,15 @@ void isNotNullNoneAreNullTest() { // Then assertTrue(result); } + + @Test + @DisplayName("test getting system path") + void testGetSystemPathVariable() { + // When + String pathVariable = Utils.getSystemPathVariable(); + + // Then + assertNotNull(pathVariable); + assertTrue(pathVariable.contains(File.pathSeparator)); + } }