From d8e15bbea2ea403c78a02171315a24b4bc988117 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 3 Apr 2018 16:40:25 -0400 Subject: [PATCH 1/2] Remove silent batch mode from install plugin Today we have a silent batch mode in the install plugin command when standard input is closed or there is no tty. It appears that historically this was useful when running tests where we want to accept plugin permissions without having to acknowledge them. Now that we have an explicit batch mode flag, this use-case is removed. The motivation for removing this now is that there is another place where silent batch mode arises and that is when a user attempts to install a plugin inside a Docker container without keeping standard input open and attaching a tty. In this case, the install plugin command will treat the situation as a silent batch mode and therefore the user will never have the chance to acknowledge the additional permissions required by a plugin. This commit removes this silent batch mode in favor of using the --batch flag when running tests and requiring the user to take explicit action to acknowledge the additional permissions (either by leaving standard input open and attaching a tty, or by passing the --batch flags themselves). Note that with this change the user will now see a null pointer exception when they try to install a plugin in a Docker container without keeping standard input open and attaching a tty. This will be addressed in an immediate follow-up, but because the implications of that change are larger, they should be handled separately from this one. --- .../elasticsearch/gradle/test/ClusterFormationTasks.groovy | 2 +- .../java/org/elasticsearch/plugins/InstallPluginCommand.java | 3 +-- .../packaging/tests/module_and_plugin_test_cases.bash | 4 ++-- qa/vagrant/src/test/resources/packaging/utils/plugins.bash | 4 ++-- 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy index 4d6b54fa3bbee..8e97ee352ead2 100644 --- a/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy +++ b/buildSrc/src/main/groovy/org/elasticsearch/gradle/test/ClusterFormationTasks.groovy @@ -494,7 +494,7 @@ class ClusterFormationTasks { * the short name requiring the path to already exist. */ final Object esPluginUtil = "${-> node.binPath().resolve('elasticsearch-plugin').toString()}" - final Object[] args = [esPluginUtil, 'install', file] + final Object[] args = [esPluginUtil, 'install', '--batch', file] return configureExecTask(name, project, setup, node, args) } diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 84f3764880243..8d024a580113a 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -208,8 +208,7 @@ protected void printAdditionalHelp(Terminal terminal) { @Override protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { String pluginId = arguments.value(options); - boolean isBatch = options.has(batchOption) || System.console() == null; - execute(terminal, pluginId, isBatch, env); + execute(terminal, pluginId, options.has(batchOption), env); } // pkg private for testing diff --git a/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash b/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash index 190f70e9bad0b..a62d690897e37 100644 --- a/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash +++ b/qa/vagrant/src/test/resources/packaging/tests/module_and_plugin_test_cases.bash @@ -416,7 +416,7 @@ fi @test "[$GROUP] install a sample plugin with different logging modes and check output" { local relativePath=${1:-$(readlink -m custom-settings-*.zip)} - sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/elasticsearch-plugin" install "file://$relativePath" > /tmp/plugin-cli-output + sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/elasticsearch-plugin" install --batch "file://$relativePath" > /tmp/plugin-cli-output # exclude progress line local loglines=$(cat /tmp/plugin-cli-output | grep -v "^[[:cntrl:]]" | wc -l) [ "$loglines" -eq "2" ] || { @@ -427,7 +427,7 @@ fi remove_plugin_example local relativePath=${1:-$(readlink -m custom-settings-*.zip)} - sudo -E -u $ESPLUGIN_COMMAND_USER ES_JAVA_OPTS="-Des.logger.level=DEBUG" "$ESHOME/bin/elasticsearch-plugin" install "file://$relativePath" > /tmp/plugin-cli-output + sudo -E -u $ESPLUGIN_COMMAND_USER ES_JAVA_OPTS="-Des.logger.level=DEBUG" "$ESHOME/bin/elasticsearch-plugin" install --batch "file://$relativePath" > /tmp/plugin-cli-output local loglines=$(cat /tmp/plugin-cli-output | grep -v "^[[:cntrl:]]" | wc -l) [ "$loglines" -gt "2" ] || { echo "Expected more than 2 lines excluding progress bar but the output had $loglines lines and was:" diff --git a/qa/vagrant/src/test/resources/packaging/utils/plugins.bash b/qa/vagrant/src/test/resources/packaging/utils/plugins.bash index 403d89b30ecad..f9110b3066295 100644 --- a/qa/vagrant/src/test/resources/packaging/utils/plugins.bash +++ b/qa/vagrant/src/test/resources/packaging/utils/plugins.bash @@ -47,9 +47,9 @@ install_plugin() { fi if [ -z "$umask" ]; then - sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/elasticsearch-plugin" install -batch "file://$path" + sudo -E -u $ESPLUGIN_COMMAND_USER "$ESHOME/bin/elasticsearch-plugin" install --batch "file://$path" else - sudo -E -u $ESPLUGIN_COMMAND_USER bash -c "umask $umask && \"$ESHOME/bin/elasticsearch-plugin\" install -batch \"file://$path\"" + sudo -E -u $ESPLUGIN_COMMAND_USER bash -c "umask $umask && \"$ESHOME/bin/elasticsearch-plugin\" install --batch \"file://$path\"" fi #check we did not accidentially create a log file as root as /usr/share/elasticsearch From f5ae6a79e6d0044a4032cbe8719f8cf1d1773265 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 3 Apr 2018 16:49:20 -0400 Subject: [PATCH 2/2] Smaller diff --- .../java/org/elasticsearch/plugins/InstallPluginCommand.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java index 8d024a580113a..5a14d041c763b 100644 --- a/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java +++ b/distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/InstallPluginCommand.java @@ -208,7 +208,8 @@ protected void printAdditionalHelp(Terminal terminal) { @Override protected void execute(Terminal terminal, OptionSet options, Environment env) throws Exception { String pluginId = arguments.value(options); - execute(terminal, pluginId, options.has(batchOption), env); + final boolean isBatch = options.has(batchOption); + execute(terminal, pluginId, isBatch, env); } // pkg private for testing