From b3c2a998cce2396da5672da1f8e70b8a86b8bf38 Mon Sep 17 00:00:00 2001 From: Olivier Levitt Date: Mon, 24 Jun 2024 11:41:36 +0200 Subject: [PATCH] Revert "Refactor and merge test classes for Command class (#441)" (#447) --- .../inseefrlab/helmwrapper/utils/Command.java | 92 ++++---- .../helmwrapper/UtilsCommandTest.java | 47 ++++ .../helmwrapper/utils/CommandTest.java | 207 ------------------ .../api/dao/universe/CatalogRefresher.java | 5 +- 4 files changed, 88 insertions(+), 263 deletions(-) create mode 100644 helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/UtilsCommandTest.java delete mode 100644 helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/utils/CommandTest.java diff --git a/helm-wrapper/src/main/java/io/github/inseefrlab/helmwrapper/utils/Command.java b/helm-wrapper/src/main/java/io/github/inseefrlab/helmwrapper/utils/Command.java index e06699e8..d0519caa 100644 --- a/helm-wrapper/src/main/java/io/github/inseefrlab/helmwrapper/utils/Command.java +++ b/helm-wrapper/src/main/java/io/github/inseefrlab/helmwrapper/utils/Command.java @@ -22,7 +22,7 @@ public class Command { "^[ ]*[a-z0-9]([-a-z0-9 ]*[a-z0-9 ])?(\\.[a-z0-9 ]([-a-z0-9 ]*[a-z0-9 ])?)*[ ]*$"); private static final Logger LOGGER = LoggerFactory.getLogger(Command.class); - static ProcessExecutor getProcessExecutor() { // Changed to package-private + private static ProcessExecutor getProcessExecutor() { ProcessExecutor processExecutor = new ProcessExecutor(); processExecutor.redirectError(System.err); processExecutor.readOutput(true); @@ -40,52 +40,45 @@ public void afterStart(Process process, ProcessExecutor executor) { public static ProcessResult executeAndGetResponseAsJson( HelmConfiguration helmConfiguration, String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return getProcessExecutor() .environment(getEnv(helmConfiguration)) - .commandSplit(buildSecureCommand(command, helmConfiguration) + " --output json") + .commandSplit(addConfigToCommand(command, helmConfiguration) + " --output json") .execute(); } public static ProcessResult executeAndGetResponseAsJson(String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return executeAndGetResponseAsJson(null, command); } public static ProcessResult executeAndGetResponseAsRaw( HelmConfiguration helmConfiguration, String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return getProcessExecutor() .environment(getEnv(helmConfiguration)) - .commandSplit(buildSecureCommand(command, helmConfiguration)) + .commandSplit(addConfigToCommand(command, helmConfiguration)) .execute(); } public static ProcessResult executeAndGetResponseAsRaw(String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return executeAndGetResponseAsRaw(null, command); } public static ProcessResult execute(HelmConfiguration helmConfiguration, String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return getProcessExecutor() .environment(getEnv(helmConfiguration)) - .commandSplit(buildSecureCommand(command, helmConfiguration)) + .commandSplit(addConfigToCommand(command, helmConfiguration)) .execute(); } public static ProcessResult execute(String command) throws InvalidExitValueException, IOException, InterruptedException, TimeoutException { - validateCommand(command); return execute(null, command); } - static Map getEnv( - HelmConfiguration helmConfiguration) { // Changed to package-private + private static Map getEnv(HelmConfiguration helmConfiguration) { Map env = new HashMap<>(); if (System.getProperty("http.proxyHost") != null) { env.put( @@ -112,50 +105,45 @@ static Map getEnv( return env; } - static String buildSecureCommand( - String command, HelmConfiguration helmConfiguration) { // Changed to package-private - StringBuilder newCommand = new StringBuilder(command).append(" "); - if (helmConfiguration != null) { - if (helmConfiguration.getAsKubeUser() != null) { - newCommand - .append(" --kube-as-user ") - .append(escapeArgument(helmConfiguration.getAsKubeUser())) - .append(" "); - } - String kubeConfig = null; - if (StringUtils.isNotEmpty(helmConfiguration.getApiserverUrl())) { - newCommand - .append(" --kube-apiserver=") - .append(escapeArgument(helmConfiguration.getApiserverUrl())) - .append(" "); - kubeConfig = "/dev/null"; - } - if (StringUtils.isNotEmpty(helmConfiguration.getKubeToken())) { - newCommand - .append(" --kube-token=") - .append(escapeArgument(helmConfiguration.getKubeToken())) - .append(" "); - kubeConfig = "/dev/null"; - } - if (StringUtils.isNotEmpty(helmConfiguration.getKubeConfig())) { - kubeConfig = helmConfiguration.getKubeConfig(); - } - if (kubeConfig != null) { - newCommand.append(" --kubeconfig=").append(escapeArgument(kubeConfig)).append(" "); - } + private static String addConfigToCommand(String command, HelmConfiguration helmConfiguration) { + if (helmConfiguration == null) { + return command; + } + String newCommand = command; + newCommand = newCommand.concat(" "); + if (helmConfiguration.getAsKubeUser() != null) { + newCommand = + newCommand.concat(" --kube-as-user " + helmConfiguration.getAsKubeUser() + " "); + } + String kubeConfig = null; + if (StringUtils.isNotEmpty(helmConfiguration.getApiserverUrl())) { + newCommand = + newCommand + .concat(" --kube-apiserver=" + helmConfiguration.getApiserverUrl()) + .concat(" "); + // Kubeconfig should be set to /dev/null to prevent mixing user provided configuration + // with pre-existing local kubeconfig (most likely re-using a cluster certificate from + // another cluster) + kubeConfig = "/dev/null"; } - return newCommand.toString(); - } - static void validateCommand(String command) - throws IllegalArgumentException { // Changed to package-private - if (!safeToConcatenate.matcher(command).matches()) { - throw new IllegalArgumentException("Illegal characters in command"); + if (StringUtils.isNotEmpty(helmConfiguration.getKubeToken())) { + newCommand = + newCommand + .concat(" --kube-token=" + helmConfiguration.getKubeToken()) + .concat(" "); + kubeConfig = "/dev/null"; + } + + if (StringUtils.isNotEmpty(helmConfiguration.getKubeConfig())) { + kubeConfig = helmConfiguration.getKubeConfig(); + } + + if (kubeConfig != null) { + newCommand = newCommand.concat(" --kubeconfig=" + kubeConfig).concat(" "); } - } - static String escapeArgument(String argument) { // Changed to package-private - return argument.replaceAll("([\"\\\\])", "\\\\$1"); + return newCommand; } public static void safeConcat(StringBuilder currentCommand, String toConcat) diff --git a/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/UtilsCommandTest.java b/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/UtilsCommandTest.java new file mode 100644 index 00000000..52f15906 --- /dev/null +++ b/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/UtilsCommandTest.java @@ -0,0 +1,47 @@ +package io.github.inseefrlab.helmwrapper; + +import static org.junit.jupiter.api.Assertions.assertThrows; + +import io.github.inseefrlab.helmwrapper.utils.Command; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.boot.test.context.SpringBootTest; + +@SpringBootTest +public class UtilsCommandTest { + + @ParameterizedTest + @ValueSource( + strings = { + "rstudio", + "hello-world", + " vscode-python-33432 ", + "vscode-python-11 ", + " rstudio" + }) + public void shouldAllowConcatenate(String toConcatenate) { + Command.safeConcat(new StringBuilder("helm ls -a"), toConcatenate); + } + + @ParameterizedTest + @ValueSource( + strings = { + "", + "--post-renderer test", + "-o", + "-o", + "--option", + " --option", + "&", + "&&", + "@", + "|" + }) + public void shouldDisallowConcatenate(String toConcatenate) { + assertThrows( + IllegalArgumentException.class, + () -> { + Command.safeConcat(new StringBuilder("helm ls -a"), toConcatenate); + }); + } +} diff --git a/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/utils/CommandTest.java b/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/utils/CommandTest.java deleted file mode 100644 index a45eea31..00000000 --- a/helm-wrapper/src/test/java/io/github/inseefrlab/helmwrapper/utils/CommandTest.java +++ /dev/null @@ -1,207 +0,0 @@ -package io.github.inseefrlab.helmwrapper.utils; - -import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.*; - -import io.github.inseefrlab.helmwrapper.configuration.HelmConfiguration; -import java.util.Map; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.MockedStatic; -import org.mockito.Mockito; -import org.zeroturnaround.exec.ProcessExecutor; -import org.zeroturnaround.exec.ProcessResult; - -class CommandTest { - - private HelmConfiguration helmConfiguration; - private ProcessExecutor processExecutor; - - @BeforeEach - void setUp() { - helmConfiguration = new HelmConfiguration(); - helmConfiguration.setAsKubeUser("admin-user"); - helmConfiguration.setApiserverUrl("https://k8s-cluster.local"); - helmConfiguration.setKubeToken("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9..."); - helmConfiguration.setKubeConfig("/etc/kubernetes/admin.conf"); - - processExecutor = mock(ProcessExecutor.class); - } - - @Test - void testExecuteAndGetResponseAsJson_withValidCommand() throws Exception { - try (MockedStatic mockedCommand = - Mockito.mockStatic(Command.class, Mockito.CALLS_REAL_METHODS)) { - mockedCommand.when(Command::getProcessExecutor).thenReturn(processExecutor); - - ProcessResult expectedResult = mock(ProcessResult.class); - when(processExecutor.commandSplit(any())).thenReturn(processExecutor); - when(processExecutor.environment(any(Map.class))).thenReturn(processExecutor); - when(processExecutor.execute()).thenReturn(expectedResult); - - ProcessResult result = - Command.executeAndGetResponseAsJson(helmConfiguration, "helm repo list"); - - assertNotNull(result); - verify(processExecutor, times(1)).execute(); - } - } - - @Test - void testExecuteAndGetResponseAsJson_withInvalidCommand() { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.executeAndGetResponseAsJson(helmConfiguration, "invalid;command"); - }); - } - - @Test - void testExecuteAndGetResponseAsRaw_withValidCommand() throws Exception { - try (MockedStatic mockedCommand = - Mockito.mockStatic(Command.class, Mockito.CALLS_REAL_METHODS)) { - mockedCommand.when(Command::getProcessExecutor).thenReturn(processExecutor); - - ProcessResult expectedResult = mock(ProcessResult.class); - when(processExecutor.commandSplit(any())).thenReturn(processExecutor); - when(processExecutor.environment(any(Map.class))).thenReturn(processExecutor); - when(processExecutor.execute()).thenReturn(expectedResult); - - ProcessResult result = - Command.executeAndGetResponseAsRaw(helmConfiguration, "helm repo list"); - - assertNotNull(result); - verify(processExecutor, times(1)).execute(); - } - } - - @Test - void testExecuteAndGetResponseAsRaw_withInvalidCommand() { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.executeAndGetResponseAsRaw(helmConfiguration, "invalid;command"); - }); - } - - @Test - void testExecute_withValidCommand() throws Exception { - try (MockedStatic mockedCommand = - Mockito.mockStatic(Command.class, Mockito.CALLS_REAL_METHODS)) { - mockedCommand.when(Command::getProcessExecutor).thenReturn(processExecutor); - - ProcessResult expectedResult = mock(ProcessResult.class); - when(processExecutor.commandSplit(any())).thenReturn(processExecutor); - when(processExecutor.environment(any(Map.class))).thenReturn(processExecutor); - when(processExecutor.execute()).thenReturn(expectedResult); - - ProcessResult result = Command.execute(helmConfiguration, "helm repo list"); - - assertNotNull(result); - verify(processExecutor, times(1)).execute(); - } - } - - @Test - void testExecute_withNefariousCommand() { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.execute(helmConfiguration, "helm repo list; rm -rf /"); - }); - } - - @Test - void testValidateCommand_withValidCommand() { - assertDoesNotThrow( - () -> { - Command.validateCommand("helm repo list"); - }); - } - - @Test - void testValidateCommand_withInvalidCommand() { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.validateCommand("invalid;command"); - }); - } - - @Test - void testValidateCommand_withNefariousCommand() { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.validateCommand("helm repo list; rm -rf /"); - }); - } - - @Test - void testEscapeArgument() { - String escapedArgument = Command.escapeArgument("\"test\""); - assertEquals("\\\"test\\\"", escapedArgument); - } - - @Test - void testBuildSecureCommand() { - String secureCommand = Command.buildSecureCommand("helm repo list", helmConfiguration); - assertTrue(secureCommand.contains(" --kube-as-user admin-user ")); - assertTrue(secureCommand.contains(" --kube-apiserver=https://k8s-cluster.local ")); - assertTrue( - secureCommand.contains(" --kube-token=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9... ")); - assertTrue(secureCommand.contains(" --kubeconfig=/etc/kubernetes/admin.conf ")); - } - - @Test - void testGetEnv() { - System.setProperty("http.proxyHost", "http://proxy"); - System.setProperty("http.proxyPort", "8080"); - System.setProperty("https.proxyHost", "https://secureproxy"); - System.setProperty("https.proxyPort", "8443"); - System.setProperty("no_proxy", "localhost"); - - Map env = Command.getEnv(helmConfiguration); - - assertEquals("http://proxy:8080", env.get("HTTP_PROXY")); - assertEquals("https://secureproxy:8443", env.get("HTTPS_PROXY")); - assertEquals("localhost", env.get("NO_PROXY")); - } - - @ParameterizedTest - @ValueSource( - strings = { - "rstudio", - "hello-world", - "vscode-python-33432", - "vscode-python-11", - "rstudio" - }) - public void shouldAllowConcatenate(String toConcatenate) { - Command.safeConcat(new StringBuilder("helm ls -a"), toConcatenate); - } - - @ParameterizedTest - @ValueSource( - strings = { - "", - "--post-renderer test", - "-o", - "--option", - " --option", - "&", - "&&", - "@", - "|" - }) - public void shouldDisallowConcatenate(String toConcatenate) { - assertThrows( - IllegalArgumentException.class, - () -> { - Command.safeConcat(new StringBuilder("helm ls -a"), toConcatenate); - }); - } -} diff --git a/onyxia-api/src/main/java/fr/insee/onyxia/api/dao/universe/CatalogRefresher.java b/onyxia-api/src/main/java/fr/insee/onyxia/api/dao/universe/CatalogRefresher.java index d3e8af69..284cee56 100644 --- a/onyxia-api/src/main/java/fr/insee/onyxia/api/dao/universe/CatalogRefresher.java +++ b/onyxia-api/src/main/java/fr/insee/onyxia/api/dao/universe/CatalogRefresher.java @@ -36,7 +36,7 @@ public CatalogRefresher( this.refreshTime = refreshTime; } - private void refreshCatalogs() throws InterruptedException { + private void refreshCatalogs() { catalogs.getCatalogs() .forEach( c -> { @@ -49,9 +49,6 @@ private void refreshCatalogs() throws InterruptedException { c.getCaFile()); LOGGER.info("Updating catalog: {}", c.getId()); catalogLoader.updateCatalog(c); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException(e); // or handle it as needed } catch (Exception e) { LOGGER.warn( "Exception occurred while updating catalog: {}",