Skip to content

SOLR-17685: Remove script creation of solr url based on SOLR_TOOL_HOST in favour of java code in CLI tools #3223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
epugh merged 15 commits into from
Mar 13, 2025
Merged

SOLR-17685: Remove script creation of solr url based on SOLR_TOOL_HOST in favour of java code in CLI tools #3223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6698bd9
Test to demonstrate conflict with -z, and the fix.
epugh Feb 26, 2025
b496ea6
Restore SOLR_TOOL_HOST, it is used by CLIUtils.getDefaultSolrUrl
epugh Feb 26, 2025
fd42ecd
Remove hard coded url from cli input params
epugh Feb 26, 2025
69ace75
Nicer test name
epugh Feb 27, 2025
10c3290
Speed test up with less heavy operation
epugh Feb 28, 2025
372814f
Update solr/bin/solr
epugh Mar 10, 2025
1da1047
Remove solr.tool.host and SOLR_TOOL_HOST
epugh Mar 10, 2025
e947498
Test on CI if we need explicit disable of auth
epugh Mar 11, 2025
6027fdf
to make it a bit faster we are testing using cluster end point, not c…
epugh Mar 11, 2025
dafcc01
check api response
epugh Mar 11, 2025
92d33ab
Preserve AUTH_PORT usage, with a bats test
epugh Mar 11, 2025
0401177
Revert "Preserve AUTH_PORT usage, with a bats test"
epugh Mar 11, 2025
64988ef
Preserve legacy AUTH_PORT usage
epugh Mar 11, 2025
d8577a6
Merge remote-tracking branch 'upstream/main' into SOLR-17685
epugh Mar 13, 2025
4ba077c
Track change
epugh Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions solr/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,8 @@ Improvements
* SOLR-17607: SolrJ CloudSolrClient configured with HTTP URLs will no longer eagerly connect to
anything. (David Smiley)

* SOLR-17685: Simplify bin/solr scripts by no longer SOLR_TOOL_HOST variable in favour of existing SOLR_HOST ENV variable. (Eric Pugh, Houston Putnam)

Optimizations
---------------------
* SOLR-17578: Remove ZkController internal core supplier, for slightly faster reconnection after Zookeeper session loss. (Pierre Salagnac)
Expand Down
23 changes: 6 additions & 17 deletions solr/bin/solr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,10 +356,6 @@ fi
# This looks strange, but it is to avoid extra spaces when we have only one of the values set
AUTHC_OPTS="${AUTHC_OPTS:-}${SOLR_AUTHENTICATION_OPTS:+ $SOLR_AUTHENTICATION_OPTS}"

# Set the SOLR_TOOL_HOST variable for use when connecting to a running Solr instance
SOLR_TOOL_HOST="${SOLR_HOST:-localhost}"
export SOLR_TOOL_HOST

function print_usage() {
CMD="${1:-}"
ERROR_MSG="${2:-}"
Expand Down Expand Up @@ -601,6 +597,10 @@ if [[ "$SCRIPT_CMD" == "auth" ]]; then
exit 1
fi

if [ -n "${AUTH_PORT}" ]; then
export SOLR_PORT="${AUTH_PORT}"
fi

if [ -z "${SOLR_HOME:-}" ]; then
SOLR_HOME="$SOLR_SERVER_DIR/solr"
elif [[ $SOLR_HOME != /* ]]; then
Expand All @@ -612,22 +612,11 @@ if [[ "$SCRIPT_CMD" == "auth" ]]; then
fi
fi

if [ -z "${AUTH_PORT:-}" ]; then
for ID in $(ps auxww | grep java | grep start\.jar | awk '{print $2}' | sort -r)
do
port=$(jetty_port "$ID")
if [ "$port" != "" ]; then
AUTH_PORT=$port
break
fi
done
fi

run_tool auth $@ --solr-url "$SOLR_URL_SCHEME://$SOLR_TOOL_HOST:${AUTH_PORT:-8983}" --auth-conf-dir "$SOLR_HOME" "--solr-include-file" "$SOLR_INCLUDE"
Comment on lines -615 to -626

@malliaridis malliaridis Mar 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eliminates the AUTH_PORT and SOLR_TOOL_HOST environment variables, right? I believe it is fine, as the behavior is checking the jetty port and falls back to the default Solr port, so no need to distinguish it in the scripts.

Still, this is a breaking change.

EDIT: I've noticed this eliminates the use of these only in the scripts, and SOLR_TOOL_HOST is still used in java classes via solr.tool.host (but not auth.port?).

run_tool auth $@ --auth-conf-dir "$SOLR_HOME" "--solr-include-file" "$SOLR_INCLUDE"
exit $?
fi

# at this point all tools that have a custom run process, like "status" and "auth" have been run and exited.
# at this point the only tool that has a custom run process, "auth" has been run and exited.
# Unless a command is one of the ones in the if clause below, we will just run it with the default run_tool function and then exit.
if [ "$SCRIPT_CMD" != "start" ] && [ "$SCRIPT_CMD" != "stop" ] && [ "$SCRIPT_CMD" != "restart" ]; then
# hand off the command to the SolrCLI and let it handle the option parsing and validation
Expand Down
24 changes: 5 additions & 19 deletions solr/bin/solr.cmd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,12 +240,6 @@ IF DEFINED SOLR_AUTHENTICATION_CLIENT_BUILDER (
)
set "AUTHC_OPTS=%AUTHC_CLIENT_BUILDER_ARG% %SOLR_AUTHENTICATION_OPTS%"

REM Set the SOLR_TOOL_HOST variable for use when connecting to a running Solr instance
IF NOT "%SOLR_HOST%"=="" (
set "SOLR_TOOL_HOST=%SOLR_HOST%"
) ELSE (
set "SOLR_TOOL_HOST=localhost"
)
IF "%SOLR_JETTY_HOST%"=="" (
set "SOLR_JETTY_HOST=127.0.0.1"
)
Expand Down Expand Up @@ -1175,7 +1169,7 @@ IF "%FG%"=="1" (
"%JAVA%" %SOLR_SSL_OPTS% %AUTHC_OPTS% %SOLR_ZK_CREDS_AND_ACLS% %SOLR_TOOL_OPTS% -Dsolr.install.dir="%SOLR_TIP%" -Dsolr.default.confdir="%DEFAULT_CONFDIR%"^
-Dlog4j.configurationFile="file:///%DEFAULT_SERVER_DIR%\resources\log4j2-console.xml" ^
-classpath "%DEFAULT_SERVER_DIR%\solr-webapp\webapp\WEB-INF\lib\*;%DEFAULT_SERVER_DIR%\lib\ext\*" ^
org.apache.solr.cli.SolrCLI status --max-wait-secs !SOLR_START_WAIT! --solr-url !SOLR_URL_SCHEME!://%SOLR_TOOL_HOST%:%SOLR_PORT%
org.apache.solr.cli.SolrCLI status --max-wait-secs !SOLR_START_WAIT!
IF NOT "!ERRORLEVEL!"=="0" (
set "SCRIPT_ERROR=Solr did not start or was not reachable. Check the logs for errors."
goto err
Expand Down Expand Up @@ -1220,22 +1214,14 @@ IF NOT EXIST "%SOLR_HOME%\" (
)
)

if "!AUTH_PORT!"=="" (
for /f "usebackq" %%i in (`dir /b "%SOLR_TIP%\bin" ^| findstr /i "^solr-.*\.port$"`) do (
set SOME_SOLR_PORT=
For /F "Delims=" %%J In ('type "%SOLR_TIP%\bin\%%i"') do set SOME_SOLR_PORT=%%~J
if NOT "!SOME_SOLR_PORT!"=="" (
for /f "tokens=2,5" %%j in ('netstat -aon ^| find "TCP " ^| find ":0 " ^| find ":!SOME_SOLR_PORT! "') do (
IF NOT "%%k"=="0" set AUTH_PORT=!SOME_SOLR_PORT!
)
)
)
IF DEFINED AUTH_PORT (
set "SOLR_PORT=%AUTH_PORT%"
)

"%JAVA%" %SOLR_SSL_OPTS% %AUTHC_OPTS% %SOLR_ZK_CREDS_AND_ACLS% %SOLR_TOOL_OPTS% -Dsolr.install.dir="%SOLR_TIP%" ^
-Dlog4j.configurationFile="file:///%DEFAULT_SERVER_DIR%\resources\log4j2-console.xml" ^
-classpath "%DEFAULT_SERVER_DIR%\solr-webapp\webapp\WEB-INF\lib\*;%DEFAULT_SERVER_DIR%\lib\ext\*" ^
org.apache.solr.cli.SolrCLI auth %AUTH_PARAMS% --solr-include-file "%SOLR_INCLUDE%" --auth-conf-dir "%SOLR_HOME%" ^
--solr-url !SOLR_URL_SCHEME!://%SOLR_TOOL_HOST%:!AUTH_PORT!
org.apache.solr.cli.SolrCLI auth %AUTH_PARAMS% --solr-include-file "%SOLR_INCLUDE%" --auth-conf-dir "%SOLR_HOME%"
goto done

:err
Expand Down
6 changes: 4 additions & 2 deletions solr/core/src/java/org/apache/solr/cli/AuthTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,10 @@ public class AuthTool extends ToolBase {
.longOpt("solr-include-file")
.hasArg()
.argName("FILE")
.required()
Comment thread
malliaridis marked this conversation as resolved.
.desc(
"The Solr include file which contains overridable environment variables for configuring Solr configurations.")
"The Solr include file which contains overridable environment variables for configuring Solr configurations. Defaults to solr.in."
+ (CLIUtils.isWindows() ? ".cmd" : ".sh"))
.build();

private static final Option UPDATE_INCLUDE_FILE_OPTION =
Expand All @@ -96,7 +98,7 @@ public class AuthTool extends ToolBase {
.argName("FILE")
.required()
.desc(
"This is where any authentication related configuration files, if any, would be placed.")
"This is where any authentication related configuration files, if any, would be placed. Defaults to $SOLR_HOME.")
Comment thread
malliaridis marked this conversation as resolved.
.build();

public AuthTool() {
Expand Down
7 changes: 6 additions & 1 deletion solr/core/src/java/org/apache/solr/cli/CLIUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.Set;
import java.util.concurrent.TimeUnit;
import org.apache.commons.cli.CommandLine;
import org.apache.commons.exec.OS;
import org.apache.solr.client.solrj.SolrClient;
import org.apache.solr.client.solrj.SolrRequest;
import org.apache.solr.client.solrj.SolrServerException;
Expand Down Expand Up @@ -70,7 +71,7 @@ private CLIUtils() {}
public static String getDefaultSolrUrl() {
// note that ENV_VAR syntax (and the env vars too) are mapped to env.var sys props
String scheme = EnvUtils.getProperty("solr.url.scheme", "http");
String host = EnvUtils.getProperty("solr.tool.host", "localhost");
String host = EnvUtils.getProperty("solr.host", "localhost");
String port = EnvUtils.getProperty("jetty.port", "8983"); // from SOLR_PORT env
return String.format(Locale.ROOT, "%s://%s:%s", scheme.toLowerCase(Locale.ROOT), host, port);
}
Expand Down Expand Up @@ -346,4 +347,8 @@ public static Path getConfigSetsDir(Path solrInstallDir) {
Path configSetsPath = Paths.get("server/solr/configsets/");
return solrInstallDir.resolve(configSetsPath);
}

public static boolean isWindows() {
return (OS.isFamilyDOS() || OS.isFamilyWin9x() || OS.isFamilyWindows());
}
}
3 changes: 1 addition & 2 deletions solr/core/src/java/org/apache/solr/cli/RunExampleTool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
import org.apache.commons.exec.DefaultExecutor;
import org.apache.commons.exec.ExecuteException;
import org.apache.commons.exec.Executor;
import org.apache.commons.exec.OS;
import org.apache.commons.exec.environment.EnvironmentUtils;
import org.apache.commons.io.file.PathUtils;
import org.apache.solr.client.solrj.SolrClient;
Expand Down Expand Up @@ -695,7 +694,7 @@ protected Map<String, Object> startSolr(
Path cwd = Path.of(System.getProperty("user.dir"));
Path binDir = Path.of(script).getParent();

boolean isWindows = (OS.isFamilyDOS() || OS.isFamilyWin9x() || OS.isFamilyWindows());
boolean isWindows = CLIUtils.isWindows();
String callScript = (!isWindows && cwd.equals(binDir.getParent())) ? "bin/solr" : script;

String cwdPath = cwd.toAbsolutePath().toString();
Expand Down
4 changes: 2 additions & 2 deletions solr/core/src/test/org/apache/solr/cli/CLIUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class CLIUtilsTest extends SolrCloudTestCase {
@Test
public void testDefaultSolrUrlWithNoProperties() {
System.clearProperty("solr.url.scheme");
System.clearProperty("solr.tool.host");
System.clearProperty("solr.host");
System.clearProperty("jetty.port");
assertEquals(
"Default Solr URL should match with no properties set.",
Expand All @@ -39,7 +39,7 @@ public void testDefaultSolrUrlWithNoProperties() {
@Test
public void testDefaultSolrUrlWithProperties() {
System.setProperty("solr.url.scheme", "https");
System.setProperty("solr.tool.host", "other.local");
System.setProperty("solr.host", "other.local");
System.setProperty("jetty.port", "1234");
assertEquals(
"Default Solr URL should match with custom properties set.",
Expand Down
34 changes: 34 additions & 0 deletions solr/packaging/test/test_auth.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,37 @@ setup() {
assert_output --partial '"numFound":0'
solr stop --all
}

@test "enable auth connects via zookeeper" {
solr start
run solr auth enable --type basicAuth --credentials name:password -z localhost:${ZK_PORT}
assert_output --partial 'Successfully enabled basic auth'
run curl -u name:password --basic "http://localhost:${SOLR_PORT}/api/cluster"
assert_output --partial '"status":0'

solr auth disable -z localhost:${ZK_PORT}
run curl "http://localhost:${SOLR_PORT}/api/cluster"
assert_output --partial '"status":0'
solr stop --all
}

@test "AUTH_PORT delegates to SOLR_PORT" {
solr start -p ${SOLR2_PORT}

export AUTH_PORT=${SOLR2_PORT}

unset SOLR2_PORT
assert [ -z "${SOLR2_PORT}" ]

solr auth enable --type basicAuth --credentials name:password
solr assert --started http://localhost:${AUTH_PORT} --timeout 5000

run curl -u name:password --basic "http://localhost:${AUTH_PORT}/api/cluster"
assert_output --partial '"status":0'

solr auth disable
run curl "http://localhost:${AUTH_PORT}/api/cluster"
assert_output --partial '"status":0'
solr stop --all
export SOLR2_PORT=${AUTH_PORT}
}