Skip to content
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

SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored #2202

Merged
merged 3 commits into from
Jan 24, 2024
Merged

SOLR-17119: Exception swallowing in ContainerPluginsApi causes some config validation errors to be ignored #2202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63629d8
Fix an undesired exception swallowing in ContainerPluginsApi.validate…
ycallea Jan 17, 2024
a5b7088
Minor update following review to improve tests readability
ycallea Jan 19, 2024
0246076
Update CHANGES.txt
ycallea Jan 23, 2024
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
3 changes: 2 additions & 1 deletion solr/CHANGES.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ New Features

Improvements
---------------------
(No changes)
* SOLR-17119: When registering or updating a ConfigurablePlugin through the `/cluster/plugin` API,
config validation exceptions are now propagated to the callers. (Yohann Callea)

Optimizations
---------------------
Expand Down
39 changes: 16 additions & 23 deletions solr/core/src/java/org/apache/solr/handler/admin/ContainerPluginsApi.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,12 @@

package org.apache.solr.handler.admin;

import static org.apache.lucene.util.IOUtils.closeWhileHandlingException;

import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import org.apache.solr.api.AnnotatedApi;
import org.apache.solr.api.ClusterPluginsSource;
import org.apache.solr.api.Command;
import org.apache.solr.api.ContainerPluginsRegistry;
Expand Down Expand Up @@ -130,28 +127,24 @@ public void update(PayloadObj<PluginMeta> payload) throws IOException {
}

private void validateConfig(PayloadObj<PluginMeta> payload, PluginMeta info) throws IOException {
if (info.klass.indexOf(':') > 0) {
if (info.version == null) {
payload.addError("Using package. must provide a packageVersion");
return;
}
}
List<String> errs = new ArrayList<>();
ContainerPluginsRegistry.ApiInfo apiInfo =
coreContainer.getContainerPluginsRegistry().createInfo(payload.getDataMap(), errs);
if (!errs.isEmpty()) {
for (String err : errs) payload.addError(err);
if (info.klass.indexOf(':') > 0 && info.version == null) {
payload.addError("Using package. must provide a packageVersion");
return;
}
AnnotatedApi api = null;
try {
apiInfo.init();
} catch (Exception e) {
log.error("Error instantiating plugin ", e);
errs.add(e.getMessage());
return;
} finally {
closeWhileHandlingException(api);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method (missing in the modified version) what was causing the exception to be swallowed?

Copy link
Contributor Author

@ycallea ycallea Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The exception was swallowed here because the error message was mistakenly only added to the local errs list (line 151) but never included in the response's payload.

The removal of the call to closeWhileHandlingException(...) is a minor code clean-up.
In this context, the purpose of this method was to close the AnnotatedApi declared above (line 146), but as it is never instantiated this finally block always resulted in a no-op.

I therefore removed both this unused local variable declaration and the finally block which was dedicated to closing this resource.


final List<String> errs = new ArrayList<>();
final ContainerPluginsRegistry.ApiInfo apiInfo =
coreContainer.getContainerPluginsRegistry().createInfo(payload.getDataMap(), errs);

if (errs.isEmpty()) {
try {
apiInfo.init();
} catch (Exception e) {
log.error("Error instantiating plugin ", e);
errs.add(e.getMessage());
}
}

errs.forEach(payload::addError);
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't change the outcome, but maybe it could be done in an else block? It's difficult to explain why, but I find this statement difficult to parse in my head in this current form.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose to implement it this way so we are only adding errors in the response's payload (payload.addError(...)) at a single place.

We could however also do it as such, to keep the same flow as before my change if it improves readability :

final List<String> errs = new ArrayList<>();
final ContainerPluginsRegistry.ApiInfo apiInfo =
    coreContainer.getContainerPluginsRegistry().createInfo(payload.getDataMap(), errs);

if (errs.isEmpty()) {
  try {
    apiInfo.init();
  } catch (Exception e) {
    log.error("Error instantiating plugin ", e);
    payload.addError(e.getMessage());
  }
} else {
  errs.forEach(payload::addError);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a side note, the way the createInfo(...) method (and by extension the ApiInfo constructor) is currently implemented feels a bit unintuitive to be honest, you have to pass it a reference to a list that will be populated with some error messages, should any validation error occur in here.

I wanted to keep the changes from this pull request small enough to only remediate the identified bug, but I feel it would be worth refactoring as part of another pull request.

In my opinion, we should just throw exceptions instead of relying on this errors' list (see ContainerPluginsRegistry.java#L329-L413), it would simplify the code and ease errors management when constructing ApiInfo objects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't get it that only appending to errs had no effect (which is what you are fixing). I take back my comment then, it's fine as it is!

}
}
46 changes: 46 additions & 0 deletions solr/core/src/test/org/apache/solr/handler/TestContainerPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.apache.solr.client.solrj.response.V2Response;
import org.apache.solr.cloud.ClusterSingleton;
import org.apache.solr.cloud.SolrCloudTestCase;
import org.apache.solr.common.SolrException;
import org.apache.solr.common.annotation.JsonProperty;
import org.apache.solr.common.util.ReflectMapWriter;
import org.apache.solr.core.CoreContainer;
Expand Down Expand Up @@ -377,6 +378,37 @@ public void testApiFromPackage() throws Exception {
assertEquals(1452, C5.classData.limit());
}

@Test
public void testPluginConfigValidation() throws Exception {
final Callable<V2Response> readPluginState = getPlugin("/cluster/plugin");

// Register a plugin with an invalid configuration
final ValidatableConfig config = new ValidatableConfig();
config.willPassValidation = false;

final PluginMeta plugin = new PluginMeta();
plugin.name = "validatableplugin";
plugin.klass = ConfigurablePluginWithValidation.class.getName();
plugin.config = config;

final V2Request addPlugin = postPlugin(singletonMap("add", plugin));

// Verify that the expected error is thrown and the plugin is not registered
expectError(addPlugin, "invalid config");
V2Response response = readPluginState.call();
assertNull(response._getStr("/plugin/validatableplugin/class", null));

// Now register it with a valid configuration
config.willPassValidation = true;
addPlugin.process(cluster.getSolrClient());

// Verify that the plugin is properly registered
response = readPluginState.call();
assertEquals(
ConfigurablePluginWithValidation.class.getName(),
response._getStr("/plugin/validatableplugin/class", null));
}

public static class CC1 extends CC {}

public static class CC2 extends CC1 {}
Expand Down Expand Up @@ -509,6 +541,20 @@ public void m2(SolrQueryRequest req, SolrQueryResponse rsp) {
}
}

public static class ConfigurablePluginWithValidation
implements ConfigurablePlugin<ValidatableConfig> {
@Override
public void configure(ValidatableConfig cfg) {
if (!cfg.willPassValidation) {
throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "invalid config");
}
}
}

public static class ValidatableConfig implements ReflectMapWriter {
@JsonProperty public boolean willPassValidation;
}

private Callable<V2Response> getPlugin(String path) {
V2Request req = new V2Request.Builder(path).forceV2(forceV2).GET().build();
return () -> {
Expand Down