SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP#4215
SOLR-18123: Reduce test reliance on ALLOW_PATHS_SYSPROP#4215openworld-maker wants to merge 4 commits intoapache:mainfrom
Conversation
github-actions
bot
added
documentation
openworld-maker
force-pushed
the
SOLR-18123-test-allow-paths-cleanup
branch
from
c6b55d8 to
9073006
Compare
github-actions
bot
removed
the
documentation
|
Quick update: I rebased/restacked this branch on top of the latest No functional change intended versus the prior version; this is just to keep the diff focused and easier to review. |
|
Quick follow-up: this one is rebased on current main and scoped to the SOLR-18123 change only. If any part should be split further, I can update it. |
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
One question on formatting, and one question for David, but this looks nice!
| EnvUtils.setProperty( | ||
| ALLOW_PATHS_SYSPROP, ExternalPaths.SERVER_HOME.toAbsolutePath().toString()); | ||
| solrTestRule.startSolr(createTempDir()); | ||
| public static void beforeTest() throws Exception { solrTestRule.startSolr(createTempDir()); |
There was a problem hiding this comment.
this formatting line looks werid, really surpirse tidy didn't wrap it!
| ExternalPaths.DEFAULT_CONFIGSET); | ||
| } | ||
|
|
||
| final String allowPaths = EnvUtils.getProperty(CoreContainer.ALLOW_PATHS_SYSPROP); |
There was a problem hiding this comment.
I think this makes sense, but would love @dsmiley to weigh in!
dsmiley
left a comment
dsmiley
left a comment
There was a problem hiding this comment.
TBH I would rather we not touch SolrTestCase. My instinct is to simply not check such things in tests by default... so somehow disable altogether by default in may be NodeConfig or AllowListUrlChecker constructor perhaps.
Albeit... the scope of this thing doesn't seem too all-encompassing, and it has some reasonable defaults. I'd like to understand better why so many tests need to configure it.
|
Updated |
|
Thanks @epugh for the formatting callout— now has its own statement so tidy is happy. The automatic solr.security.allow.paths helper is back inside SolrTestCase.beforeSolrTestCase(), so the test no longer needs to reach for EnvUtils.setProperty. |
|
Thanks @epugh for the formatting callout— |
|
Checking precheck, it looks like one more formatting error: https://github.com/apache/solr/actions/runs/23120891467/job/67158091809?pr=4215 |
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
I've fixed up the merge conflicts and a tidy... ran precommit... going to rerun tests and then merge.
|
@dsmiley can you look at the |
|
Can you please address my question first? It's not yet clear what the right path is. |
|
Okay, I looked some more at the use of the
|
|
My suspicion is that a very small number of tests require any allow-paths manipulation. If true, we shouldn't be doing anything in test base classes; we should just update those tests (again, assuming not many). I suspect security.policy isn't really related. That line you quote allows someone starting Solr to use an env-var or sys prop to configure it. Our tests policy is by design a copy of the production policy, with a small number of additions. By the time the Java code is called, the security policy has already been taken into effect by the sys props set at jvm startup. Thus a test can't possibly influence it. |
4 participants
Description
This PR addresses SOLR-18123 by reducing the need for tests to explicitly set
ALLOW_PATHS_SYSPROP(solr.security.allow.paths).Changes:
solr.security.allow.pathsinSolrTestCasewhen not already defined, using the test framework derived server home pathTestFrameworkAllowPathsTestto verify the default is applied and includes the expected test pathALLOW_PATHS_SYSPROPsetup from tests that only needed it to read standard test-owned pathsNo production security defaults are changed.
Testing
Passed targeted tests for all touched classes:
./gradlew test --continue --tests org.apache.solr.TestFrameworkAllowPathsTest --tests org.apache.solr.client.solrj.apache.BasicHttpSolrClientTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientBadInputTest --tests org.apache.solr.client.solrj.apache.ConcurrentUpdateSolrClientTest --tests org.apache.solr.client.solrj.apache.HttpSolrClientConPoolTest --tests org.apache.solr.handler.admin.ShowFileRequestHandlerTest --tests org.apache.solr.handler.admin.api.RenameCoreAPITest --tests org.apache.solr.handler.component.DistributedDebugComponentTest --tests org.apache.solr.response.TestPrometheusResponseWriter --tests org.apache.solr.client.solrj.jetty.ConcurrentUpdateJettySolrClientBadInputTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientCompatibilityTest --tests org.apache.solr.client.solrj.jetty.HttpJettySolrClientProxyTest --tests org.apache.solr.client.solrj.SolrExampleTests --tests org.apache.solr.client.solrj.TestBatchUpdate --tests org.apache.solr.client.solrj.TestSolrJErrorHandling --tests org.apache.solr.client.solrj.impl.ConcurrentUpdateSolrClientTestBase --tests org.apache.solr.client.solrj.impl.HttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.impl.HttpSolrClientTestBase --tests org.apache.solr.client.solrj.impl.LBHttpSolrClientBadInputTest --tests org.apache.solr.client.solrj.response.InputStreamResponseParserTest --tests org.apache.solr.client.solrj.response.TestSuggesterResponseAlso ran:
./gradlew tidy./gradlew check