From d8c9b99b1aec0aca512bc68ae26b8ce2e09b24cf Mon Sep 17 00:00:00 2001 From: =?utf8?q?S=C3=A9bastien=20Lesaint?= Date: Mon, 11 Sep 2017 15:22:03 +0200 Subject: [PATCH] SONAR-9773 ignore JAVA_TOOL_OPTIONS is defined and log warning --- .../process/command/CommandFactoryImpl.java | 18 ++++- .../command/CommandFactoryImplTest.java | 81 ++++++++++++++++++- .../main/java/org/sonar/application/App.java | 3 +- 3 files changed, 97 insertions(+), 5 deletions(-) diff --git a/server/sonar-process/src/main/java/org/sonar/process/command/CommandFactoryImpl.java b/server/sonar-process/src/main/java/org/sonar/process/command/CommandFactoryImpl.java index ae0ed14b064..abeabc77819 100644 --- a/server/sonar-process/src/main/java/org/sonar/process/command/CommandFactoryImpl.java +++ b/server/sonar-process/src/main/java/org/sonar/process/command/CommandFactoryImpl.java @@ -22,9 +22,11 @@ package org.sonar.process.command; import java.io.File; import java.util.Map; import java.util.Optional; +import org.slf4j.LoggerFactory; import org.sonar.process.ProcessId; import org.sonar.process.ProcessProperties; import org.sonar.process.Props; +import org.sonar.process.System2; import org.sonar.process.es.EsFileSystem; import org.sonar.process.es.EsLogging; import org.sonar.process.es.EsSettings; @@ -40,6 +42,7 @@ import static org.sonar.process.ProcessProperties.HTTP_PROXY_HOST; import static org.sonar.process.ProcessProperties.HTTP_PROXY_PORT; public class CommandFactoryImpl implements CommandFactory { + private static final String ENV_VAR_JAVA_TOOL_OPTIONS = "JAVA_TOOL_OPTIONS"; /** * Properties about proxy that must be set as system properties */ @@ -56,9 +59,15 @@ public class CommandFactoryImpl implements CommandFactory { private final Props props; private final File tempDir; - public CommandFactoryImpl(Props props, File tempDir) { + public CommandFactoryImpl(Props props, File tempDir, System2 system2) { this.props = props; this.tempDir = tempDir; + String javaToolOptions = system2.getenv(ENV_VAR_JAVA_TOOL_OPTIONS); + if (javaToolOptions != null && !javaToolOptions.trim().isEmpty()) { + LoggerFactory.getLogger(CommandFactoryImpl.class) + .warn("JAVA_TOOL_OPTIONS is defined but will be ignored. " + + "Use properties sonar.*.javaOpts and/or sonar.*.javaAdditionalOpts in sonar.properties to change SQ JVM processes options"); + } } @Override @@ -76,13 +85,14 @@ public class CommandFactoryImpl implements CommandFactory { .setClusterName(settingsMap.get("cluster.name")) .setHost(settingsMap.get("network.host")) .setPort(Integer.valueOf(settingsMap.get("transport.tcp.port"))) - .addEsOption("-Epath.conf="+esFileSystem.getConfDirectory().getAbsolutePath()) + .addEsOption("-Epath.conf=" + esFileSystem.getConfDirectory().getAbsolutePath()) .setEsJvmOptions(new EsJvmOptions() .addFromMandatoryProperty(props, ProcessProperties.SEARCH_JAVA_OPTS) .addFromMandatoryProperty(props, ProcessProperties.SEARCH_JAVA_ADDITIONAL_OPTS)) .setEsYmlSettings(new EsYmlSettings(settingsMap)) .setEnvVariable("ES_JVM_OPTIONS", esFileSystem.getJvmOptions().getAbsolutePath()) - .setEnvVariable("JAVA_HOME", System.getProperties().getProperty("java.home")); + .setEnvVariable("JAVA_HOME", System.getProperties().getProperty("java.home")) + .suppressEnvVariable(ENV_VAR_JAVA_TOOL_OPTIONS); } @Override @@ -107,6 +117,7 @@ public class CommandFactoryImpl implements CommandFactory { if (driverPath != null) { command.addClasspath(driverPath); } + command.suppressEnvVariable(ENV_VAR_JAVA_TOOL_OPTIONS); return command; } @@ -130,6 +141,7 @@ public class CommandFactoryImpl implements CommandFactory { if (driverPath != null) { command.addClasspath(driverPath); } + command.suppressEnvVariable(ENV_VAR_JAVA_TOOL_OPTIONS); return command; } diff --git a/server/sonar-process/src/test/java/org/sonar/process/command/CommandFactoryImplTest.java b/server/sonar-process/src/test/java/org/sonar/process/command/CommandFactoryImplTest.java index f2bc87961d9..5c87e00c6b4 100644 --- a/server/sonar-process/src/test/java/org/sonar/process/command/CommandFactoryImplTest.java +++ b/server/sonar-process/src/test/java/org/sonar/process/command/CommandFactoryImplTest.java @@ -19,23 +19,35 @@ */ package org.sonar.process.command; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.AppenderBase; import java.io.File; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.Properties; import org.apache.commons.io.FileUtils; +import org.junit.After; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.junit.rules.TemporaryFolder; +import org.mockito.Mockito; import org.sonar.process.ProcessId; import org.sonar.process.ProcessProperties; import org.sonar.process.Props; +import org.sonar.process.System2; +import org.sonar.process.logging.LogbackHelper; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.entry; +import static org.mockito.Matchers.anyString; +import static org.mockito.Mockito.when; public class CommandFactoryImplTest { + + private static final String MEMORY_APPENDER_NAME = "MEMORY_APPENDER"; @Rule public ExpectedException expectedException = ExpectedException.none(); @Rule @@ -44,6 +56,7 @@ public class CommandFactoryImplTest { private File homeDir; private File tempDir; private File logsDir; + private ListAppender listAppender; @Before public void setUp() throws Exception { @@ -52,6 +65,41 @@ public class CommandFactoryImplTest { logsDir = temp.newFolder(); } + @After + public void tearDown() throws Exception { + if (listAppender != null) { + listAppender.stop(); + new LogbackHelper().getRootContext().getLogger(CommandFactoryImpl.class) + .detachAppender(listAppender); + } + } + + @Test + public void constructor_logs_no_warning_if_env_variable_JAVA_TOOL_OPTIONS_is_not_set() { + System2 system2 = Mockito.mock(System2.class); + when(system2.getenv(anyString())).thenReturn(null); + attachMemoryAppenderToLoggerOf(CommandFactoryImpl.class); + + new CommandFactoryImpl(new Props(new Properties()), tempDir, system2); + + assertThat(listAppender.getLogs()).isEmpty(); + } + + @Test + public void constructor_logs_warning_if_env_variable_JAVA_TOOL_OPTIONS_is_set() { + System2 system2 = Mockito.mock(System2.class); + when(system2.getenv("JAVA_TOOL_OPTIONS")).thenReturn("sds"); + attachMemoryAppenderToLoggerOf(CommandFactoryImpl.class); + + new CommandFactoryImpl(new Props(new Properties()), tempDir, system2); + + assertThat(listAppender.getLogs()) + .extracting(ILoggingEvent::getMessage) + .containsOnly( + "JAVA_TOOL_OPTIONS is defined but will be ignored. " + + "Use properties sonar.*.javaOpts and/or sonar.*.javaAdditionalOpts in sonar.properties to change SQ JVM processes options"); + } + @Test public void createEsCommand_throws_ISE_if_es_binary_is_not_found() throws Exception { expectedException.expect(IllegalStateException.class); @@ -86,6 +134,8 @@ public class CommandFactoryImplTest { assertThat(esCommand.getLog4j2Properties()) .contains(entry("appender.file_es.fileName", new File(logsDir, "es.log").getAbsolutePath())); + + assertThat(esCommand.getSuppressedEnvVariables()).containsOnly("JAVA_TOOL_OPTIONS"); } @Test @@ -132,6 +182,8 @@ public class CommandFactoryImplTest { // default settings .contains(entry("sonar.web.javaOpts", "-Xmx512m -Xms128m -XX:+HeapDumpOnOutOfMemoryError")) .contains(entry("sonar.cluster.enabled", "false")); + + assertThat(command.getSuppressedEnvVariables()).containsOnly("JAVA_TOOL_OPTIONS"); } @Test @@ -153,6 +205,8 @@ public class CommandFactoryImplTest { // default settings .contains(entry("sonar.web.javaOpts", "-Xmx10G")) .contains(entry("sonar.cluster.enabled", "false")); + + assertThat(command.getSuppressedEnvVariables()).containsOnly("JAVA_TOOL_OPTIONS"); } @Test @@ -181,6 +235,31 @@ public class CommandFactoryImplTest { Props props = new Props(p); ProcessProperties.completeDefaults(props); - return new CommandFactoryImpl(props, tempDir); + return new CommandFactoryImpl(props, tempDir, System2.INSTANCE); + } + + private void attachMemoryAppenderToLoggerOf(Class loggerClass) { + this.listAppender = new ListAppender(); + new LogbackHelper().getRootContext().getLogger(loggerClass) + .addAppender(listAppender); + listAppender.start(); + } + + private static final class ListAppender extends AppenderBase { + private final List logs = new ArrayList<>(); + + @Override + public String getName() { + return MEMORY_APPENDER_NAME; + } + + @Override + protected void append(ILoggingEvent eventObject) { + logs.add(eventObject); + } + + public List getLogs() { + return logs; + } } } diff --git a/sonar-application/src/main/java/org/sonar/application/App.java b/sonar-application/src/main/java/org/sonar/application/App.java index 4e25774bc5e..54d233f5947 100644 --- a/sonar-application/src/main/java/org/sonar/application/App.java +++ b/sonar-application/src/main/java/org/sonar/application/App.java @@ -27,6 +27,7 @@ import org.sonar.application.process.ProcessLauncher; import org.sonar.application.process.ProcessLauncherImpl; import org.sonar.application.process.StopRequestWatcher; import org.sonar.application.process.StopRequestWatcherImpl; +import org.sonar.process.System2; import org.sonar.process.SystemExit; import org.sonar.process.command.CommandFactory; import org.sonar.process.command.CommandFactoryImpl; @@ -52,7 +53,7 @@ public class App { appState.registerClusterName(settings.getProps().value(CLUSTER_NAME, "sonarqube")); AppReloader appReloader = new AppReloaderImpl(settingsLoader, fileSystem, appState, logging); fileSystem.reset(); - CommandFactory commandFactory = new CommandFactoryImpl(settings.getProps(), fileSystem.getTempDir()); + CommandFactory commandFactory = new CommandFactoryImpl(settings.getProps(), fileSystem.getTempDir(), System2.INSTANCE); try (ProcessLauncher processLauncher = new ProcessLauncherImpl(fileSystem.getTempDir())) { Scheduler scheduler = new SchedulerImpl(settings, appReloader, commandFactory, processLauncher, appState); -- 2.39.5