HoustonPutman commented on a change in pull request #1861:
URL: https://github.com/apache/lucene-solr/pull/1861#discussion_r488913454
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest
req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream,
StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length()
-1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
Review comment:
We might want to clear data that the znode has if it exists and has data.
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest
req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
Review comment:
If you are overwriting an untrusted configSet with a trusted configSet,
I din't think that will get updated here. I guess you would only want it if
CLEANUP is true, since only then are you sure that all of the content is
trusted.
So doing it in that if statement would probably work.
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest
req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream,
StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length()
-1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
} else {
createZkNodeIfNotExistsAndSetData(zkClient, filePathInZk,
IOUtils.toByteArray(zis));
}
}
zis.close();
+ deleteUnusedFiles(zkClient, filesToDelete);
+ }
+
+ private void deleteUnusedFiles(SolrZkClient zkClient, Set<String>
filesToDelete) throws InterruptedException, KeeperException {
+ if (!filesToDelete.isEmpty()) {
+ if (log.isInfoEnabled()) {
+ log.info("Cleaning up {} unused files", filesToDelete.size());
+ }
+ if (log.isDebugEnabled()) {
+ log.debug("Cleaning up unused files: {}", filesToDelete);
+ }
+ for (String f:filesToDelete) {
+ try {
+ zkClient.delete(f, -1, true);
+ } catch (KeeperException.NoNodeException nne) {
+ }
+ }
+ }
+ }
+
+ private Set<String> getAllConfigsetFiles(SolrZkClient zkClient, String
configPathInZk) throws KeeperException, InterruptedException {
+ final Set<String> files = new HashSet<>();
+ if (!configPathInZk.startsWith(ZkConfigManager.CONFIGS_ZKNODE + "/")) {
+ throw new IllegalArgumentException("\"" + configPathInZk + "\" not
recognized as a configset path");
+ }
+ ZkMaintenanceUtils.traverseZkTree(zkClient, configPathInZk,
ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, new ZkMaintenanceUtils.ZkVisitor() {
+ @Override
+ public void visit(String path) throws InterruptedException,
KeeperException {
+ files.add(path);
+ }
+ });
+ files.remove(configPathInZk);
+ return files;
+ }
+
+ /*
+ * Fail if an untrusted request tries to update a trusted ConfigSet
+ */
+ private void ensureOverwritingUntrustedConfigSet(SolrZkClient zkClient,
String configSetZkPath) {
+ byte[] configSetNodeContent;
+ try {
+ configSetNodeContent = zkClient.getData(configSetZkPath, null, null,
true);
+ } catch (KeeperException e) {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Exception while
fetching current configSet at " + configSetZkPath, e);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Interrupted while
fetching current configSet at " + configSetZkPath, e);
+ }
+ @SuppressWarnings("unchecked")
+ Map<Object, Object> contentMap = (Map<Object, Object>)
Utils.fromJSON(configSetNodeContent);
+ Boolean isCurrentlyTrusted = (Boolean) contentMap.get("trusted");
Review comment:
using `getOrDefault("trusted", false)` would remove the need for the
null check.
##########
File path:
solr/core/src/java/org/apache/solr/handler/admin/ConfigSetsHandler.java
##########
@@ -170,21 +176,90 @@ private void handleConfigUploadRequest(SolrQueryRequest
req, SolrQueryResponse r
// Create a node for the configuration in zookeeper
boolean trusted = getTrusted(req);
- zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
- getBytes(StandardCharsets.UTF_8), true);
+ Set<String> filesToDelete = Collections.emptySet();
+ if (overwritesExisting) {
+ if (!trusted) {
+ ensureOverwritingUntrustedConfigSet(zkClient, configPathInZk);
+ }
+ if (req.getParams().getBool(ConfigSetParams.CLEANUP, false)) {
+ filesToDelete = getAllConfigsetFiles(zkClient, configPathInZk);
+ }
+ } else {
+ zkClient.makePath(configPathInZk, ("{\"trusted\": " +
Boolean.toString(trusted) + "}").
+ getBytes(StandardCharsets.UTF_8), true);
+ }
ZipInputStream zis = new ZipInputStream(inputStream,
StandardCharsets.UTF_8);
ZipEntry zipEntry = null;
while ((zipEntry = zis.getNextEntry()) != null) {
String filePathInZk = configPathInZk + "/" + zipEntry.getName();
+ if (filePathInZk.endsWith("/")) {
+ filesToDelete.remove(filePathInZk.substring(0, filePathInZk.length()
-1));
+ } else {
+ filesToDelete.remove(filePathInZk);
+ }
if (zipEntry.isDirectory()) {
- zkClient.makePath(filePathInZk, true);
+ zkClient.makePath(filePathInZk, false, true);
} else {
createZkNodeIfNotExistsAndSetData(zkClient, filePathInZk,
IOUtils.toByteArray(zis));
}
}
zis.close();
+ deleteUnusedFiles(zkClient, filesToDelete);
+ }
+
+ private void deleteUnusedFiles(SolrZkClient zkClient, Set<String>
filesToDelete) throws InterruptedException, KeeperException {
+ if (!filesToDelete.isEmpty()) {
+ if (log.isInfoEnabled()) {
+ log.info("Cleaning up {} unused files", filesToDelete.size());
+ }
+ if (log.isDebugEnabled()) {
+ log.debug("Cleaning up unused files: {}", filesToDelete);
+ }
+ for (String f:filesToDelete) {
+ try {
+ zkClient.delete(f, -1, true);
+ } catch (KeeperException.NoNodeException nne) {
+ }
+ }
+ }
+ }
+
+ private Set<String> getAllConfigsetFiles(SolrZkClient zkClient, String
configPathInZk) throws KeeperException, InterruptedException {
+ final Set<String> files = new HashSet<>();
+ if (!configPathInZk.startsWith(ZkConfigManager.CONFIGS_ZKNODE + "/")) {
+ throw new IllegalArgumentException("\"" + configPathInZk + "\" not
recognized as a configset path");
+ }
+ ZkMaintenanceUtils.traverseZkTree(zkClient, configPathInZk,
ZkMaintenanceUtils.VISIT_ORDER.VISIT_POST, new ZkMaintenanceUtils.ZkVisitor() {
+ @Override
+ public void visit(String path) throws InterruptedException,
KeeperException {
+ files.add(path);
+ }
+ });
Review comment:
the `new ZKMaintenanceUtils.ZKVisitory() { ... }` can be replaced with
`files::add`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
