Skip to content

SOLR-15950: Fix auto-creation of filestore in constructor. #653

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
HoustonPutman merged 3 commits into from
Feb 21, 2022
Merged

SOLR-15950: Fix auto-creation of filestore in constructor. #653

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5f127d
SOLR-15950: Fix auto-creation of filestore in constructor.
HoustonPutman Feb 18, 2022
c4258db
Remove need for solr core in managedResource test
HoustonPutman Feb 18, 2022
9e2412e
Remove unrelated changes
HoustonPutman Feb 18, 2022
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
17 changes: 1 addition & 16 deletions solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ public class DistribPackageStore implements PackageStore {
public DistribPackageStore(CoreContainer coreContainer) {
this.coreContainer = coreContainer;
this.solrhome = Paths.get(this.coreContainer.getSolrHome());
ensurePackageStoreDir(Paths.get(coreContainer.getSolrHome()));
}

@Override
Expand Down Expand Up @@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}

private void ensurePackageStoreDir(Path solrHome) {
final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
if (!packageStoreDir.exists()) {
try {
final boolean created = packageStoreDir.mkdirs();
if (!created) {
log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
}
} catch (Exception e) {
log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
}
}
}

public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Copy link
Copy Markdown
Contributor

@madrob madrob Feb 18, 2022

Choose a reason for hiding this comment

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

Should we catch AccessControlException here too?

Copy link
Copy Markdown
Contributor Author

@HoustonPutman HoustonPutman Feb 18, 2022

Choose a reason for hiding this comment

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

I'm ambivalent, it'll be thrown anyways and honestly shouldn't happen in a real deployment. Feel free to add it.

Copy link
Copy Markdown
Contributor

@janhoy janhoy Feb 21, 2022

Choose a reason for hiding this comment

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

It is thrown by Security Manager, right? Which is default enabled in 9.0 (but deprecated in JDK17). The path that was denied was

java.security.AccessControlException: access denied ("java.io.FilePermission" "/home/jenkins/jenkins-slave/workspace/Solr/Solr-Check-main/solr/core/build/resources/test/solr/filestore" "write")

So obviously our code tried to create that folder in the "build/resources/test/solr/" folder, which must have been disallowed by our solr-tests.policy

If we need to check for AccessControlException here, we'd likely need the same in 100 other locations across our code base?

Copy link
Copy Markdown
Contributor

@janhoy janhoy Feb 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The policy already have this line

permission java.io.FilePermission "${solr.solr.home}", "read,write,delete,readlink";

So that should be allowed -- if sysProp solr.solr.home is set to the same location as solrHome in the test that failed (TestManagedResourceStorage) that is, and I guess it is not...

Anyway, this PR avoids that now.

throw new SolrException(SERVER_ERROR, "Faild creating 'filestore' folder in SOLR_HOME", e);
throw new SolrException(SERVER_ERROR, "Failed creating 'filestore' folder in SOLR_HOME", e);
}
}
return path;
Expand Down