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.

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

Remove patch version check for plugin compatibility #6837

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
import org.opensearch.env.TestEnvironment;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.PosixPermissionsResetter;
import org.opensearch.test.VersionUtils;
import org.junit.After;
import org.junit.Before;

Expand Down Expand Up @@ -262,6 +263,10 @@ static String createPluginUrl(String name, Path structure, String... additionalP
}

static void writePlugin(String name, Path structure, String... additionalProps) throws IOException {
writePlugin(name, structure, Version.CURRENT, additionalProps);
}

static void writePlugin(String name, Path structure, Version coreVersion, String... additionalProps) throws IOException {
String[] properties = Stream.concat(
Stream.of(
"description",
Expand All @@ -271,7 +276,7 @@ static void writePlugin(String name, Path structure, String... additionalProps)
"version",
"1.0",
"opensearch.version",
Version.CURRENT.toString(),
coreVersion.toString(),
"java.version",
System.getProperty("java.specification.version"),
"classname",
Expand All @@ -284,6 +289,11 @@ static void writePlugin(String name, Path structure, String... additionalProps)
writeJar(structure.resolve("plugin.jar"), className);
}

static Path createPlugin(String name, Path structure, Version coreVersion, String... additionalProps) throws IOException {
writePlugin(name, structure, coreVersion, additionalProps);
return writeZip(structure, null);
}

static void writePluginSecurityPolicy(Path pluginDir, String... permissions) throws IOException {
StringBuilder securityPolicyContent = new StringBuilder("grant {\n ");
for (String permission : permissions) {
Expand Down Expand Up @@ -867,6 +877,45 @@ public void testInstallMisspelledOfficialPlugins() throws Exception {
assertThat(e.getMessage(), containsString("Unknown plugin unknown_plugin"));
}

public void testInstallPluginWithDifferentPatchVersionIfPluginOptsIn() throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp);
Version coreVersion = Version.CURRENT;
Version pluginVersion = VersionUtils.getVersion(coreVersion.major, coreVersion.minor, (byte) (coreVersion.revision + 1));
// Plugin explicitly specifies compatibility across patch versions
String pluginZip = createPlugin("fake", pluginDir, pluginVersion, "compatible.across.patch.versions", "true").toUri()
.toURL()
.toString();
skipJarHellCommand.execute(terminal, Collections.singletonList(pluginZip), false, env.v2());
assertThat(terminal.getOutput(), containsString("100%"));
}

public void testInstallPluginWithDifferentPatchVersionNotAllowedByDefault() throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp);
Version coreVersion = Version.CURRENT;
Version pluginVersion = VersionUtils.getVersion(coreVersion.major, coreVersion.minor, (byte) (coreVersion.revision + 1));
String pluginZip = createPlugin("fake", pluginDir, pluginVersion).toUri().toURL().toString();
IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> skipJarHellCommand.execute(terminal, Collections.singletonList(pluginZip), false, env.v2())
);
assertThat(e.getMessage(), containsString("Plugin [fake] was built for OpenSearch version"));
}

public void testInstallPluginDifferingInMinorVersionNotAllowed() throws Exception {
Tuple<Path, Environment> env = createEnv(fs, temp);
Path pluginDir = createPluginDir(temp);
Version coreVersion = Version.CURRENT;
Version pluginVersion = VersionUtils.getVersion(coreVersion.major, (byte) (coreVersion.minor + 1), coreVersion.revision);
String pluginZip = createPlugin("fake", pluginDir, pluginVersion).toUri().toURL().toString();
IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> skipJarHellCommand.execute(terminal, Collections.singletonList(pluginZip), false, env.v2())
);
assertThat(e.getMessage(), containsString("Plugin [fake] was built for OpenSearch version"));
}

public void testBatchFlag() throws Exception {
MockTerminal terminal = new MockTerminal();
installPlugin(terminal, true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ public void testPluginWithVerbose() throws Exception {
"Native Controller: false",
"Extended Plugins: []",
" * Classname: org.fake",
"Folder name: custom-folder"
"Folder name: custom-folder",
"Compatible across patch versions: false"
),
terminal.getOutput()
);
Expand All @@ -195,7 +196,8 @@ public void testPluginWithNativeController() throws Exception {
"Native Controller: true",
"Extended Plugins: []",
" * Classname: org.fake",
"Folder name: custom-folder"
"Folder name: custom-folder",
"Compatible across patch versions: false"
),
terminal.getOutput()
);
Expand All @@ -220,6 +222,7 @@ public void testPluginWithVerboseMultiplePlugins() throws Exception {
"Extended Plugins: []",
" * Classname: org.fake",
"Folder name: custom-folder",
"Compatible across patch versions: false",
"fake_plugin2",
"- Plugin information:",
"Name: fake_plugin2",
Expand All @@ -230,7 +233,8 @@ public void testPluginWithVerboseMultiplePlugins() throws Exception {
"Native Controller: false",
"Extended Plugins: []",
" * Classname: org.fake2",
"Folder name: custom-folder"
"Folder name: custom-folder",
"Compatible across patch versions: false"
),
terminal.getOutput()
);
Expand Down
120 changes: 75 additions & 45 deletions server/src/main/java/org/opensearch/plugins/PluginInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -75,19 +75,21 @@ public class PluginInfo implements Writeable, ToXContentObject {
private final String customFolderName;
private final List<String> extendedPlugins;
private final boolean hasNativeController;
private final boolean compatibleAcrossPatchVersions;

/**
* Construct plugin info.
*
* @param name the name of the plugin
* @param description a description of the plugin
* @param version an opaque version identifier for the plugin
* @param opensearchVersion the version of OpenSearch the plugin was built for
* @param javaVersion the version of Java the plugin was built with
* @param classname the entry point to the plugin
* @param customFolderName the custom folder name for the plugin
* @param extendedPlugins other plugins this plugin extends through SPI
* @param hasNativeController whether or not the plugin has a native controller
* @param name the name of the plugin
* @param description a description of the plugin
* @param version an opaque version identifier for the plugin
* @param opensearchVersion the version of OpenSearch the plugin was built for
* @param javaVersion the version of Java the plugin was built with
* @param classname the entry point to the plugin
* @param customFolderName the custom folder name for the plugin
* @param extendedPlugins other plugins this plugin extends through SPI
* @param hasNativeController whether or not the plugin has a native controller
* @param compatibleAcrossPatchVersions whether or not the plugin is compatible across patch versions
*/
public PluginInfo(
String name,
Expand All @@ -98,7 +100,8 @@ public PluginInfo(
String classname,
String customFolderName,
List<String> extendedPlugins,
boolean hasNativeController
boolean hasNativeController,
boolean compatibleAcrossPatchVersions
) {
this.name = name;
this.description = description;
Expand All @@ -109,19 +112,21 @@ public PluginInfo(
this.customFolderName = customFolderName;
this.extendedPlugins = Collections.unmodifiableList(extendedPlugins);
this.hasNativeController = hasNativeController;
this.compatibleAcrossPatchVersions = compatibleAcrossPatchVersions;
}

/**
* Construct plugin info.
*
* @param name the name of the plugin
* @param description a description of the plugin
* @param version an opaque version identifier for the plugin
* @param opensearchVersion the version of OpenSearch the plugin was built for
* @param javaVersion the version of Java the plugin was built with
* @param classname the entry point to the plugin
* @param extendedPlugins other plugins this plugin extends through SPI
* @param hasNativeController whether or not the plugin has a native controller
* @param name the name of the plugin
* @param description a description of the plugin
* @param version an opaque version identifier for the plugin
* @param opensearchVersion the version of OpenSearch the plugin was built for
* @param javaVersion the version of Java the plugin was built with
* @param classname the entry point to the plugin
* @param extendedPlugins other plugins this plugin extends through SPI
* @param hasNativeController whether or not the plugin has a native controller
* @param compatibleAcrossPatchVersions whether or not the plugin is compatible with all patch versions
*/
public PluginInfo(
String name,
Expand All @@ -131,7 +136,8 @@ public PluginInfo(
String javaVersion,
String classname,
List<String> extendedPlugins,
boolean hasNativeController
boolean hasNativeController,
boolean compatibleAcrossPatchVersions
) {
this(
name,
Expand All @@ -142,7 +148,8 @@ public PluginInfo(
classname,
null /* customFolderName */,
extendedPlugins,
hasNativeController
hasNativeController,
compatibleAcrossPatchVersions
);
}

Expand All @@ -162,6 +169,7 @@ public PluginInfo(final StreamInput in) throws IOException {
this.customFolderName = in.readString();
this.extendedPlugins = in.readStringList();
this.hasNativeController = in.readBoolean();
this.compatibleAcrossPatchVersions = in.readBoolean();
}

@Override
Expand All @@ -179,6 +187,7 @@ public void writeTo(final StreamOutput out) throws IOException {
}
out.writeStringCollection(extendedPlugins);
out.writeBoolean(hasNativeController);
out.writeBoolean(compatibleAcrossPatchVersions);
}

/**
Expand Down Expand Up @@ -241,29 +250,14 @@ public static PluginInfo readFromProperties(final Path path) throws IOException
}

final String hasNativeControllerValue = propsMap.remove("has.native.controller");
final boolean hasNativeController;
if (hasNativeControllerValue == null) {
hasNativeController = false;
} else {
switch (hasNativeControllerValue) {
case "true":
hasNativeController = true;
break;
case "false":
hasNativeController = false;
break;
default:
final String message = String.format(
Locale.ROOT,
"property [%s] must be [%s], [%s], or unspecified but was [%s]",
"has_native_controller",
"true",
"false",
hasNativeControllerValue
);
throw new IllegalArgumentException(message);
}
}
final boolean hasNativeController = getBooleanProperty("has.native.controller", hasNativeControllerValue, false);

final String compatibleAcrossPatchVersionsValue = propsMap.remove("compatible.across.patch.versions");
final boolean compatibleAcrossPatchVersions = getBooleanProperty(
"compatible.across.patch.versions",
compatibleAcrossPatchVersionsValue,
false
);

if (propsMap.isEmpty() == false) {
throw new IllegalArgumentException("Unknown properties in plugin descriptor: " + propsMap.keySet());
Expand All @@ -278,10 +272,33 @@ public static PluginInfo readFromProperties(final Path path) throws IOException
classname,
customFolderName,
extendedPlugins,
hasNativeController
hasNativeController,
compatibleAcrossPatchVersions
);
}

private static boolean getBooleanProperty(String propertyName, String propertyValue, boolean defaultBool) {
if (propertyValue == null) {
return defaultBool;
}
switch (propertyValue) {
case "true":
return true;
case "false":
return false;
default:
final String message = String.format(
Locale.ROOT,
"property [%s] must be [%s], [%s], or unspecified but was [%s]",
propertyName,
"true",
"false",
propertyValue
);
throw new IllegalArgumentException(message);
}
}

/**
* The name of the plugin.
*
Expand Down Expand Up @@ -363,6 +380,14 @@ public boolean hasNativeController() {
return hasNativeController;
}

/**
* Whether or not the plugin is compatible with all patch versions for given opensearch version
* @return {@code true} if the plugin is compatible with all patch versions, {@code false} otherwise.
*/
public boolean compatibleAcrossPatchVersions() {
return compatibleAcrossPatchVersions;
}

/**
* The target folder name for the plugin.
*
Expand All @@ -385,6 +410,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.field("custom_foldername", customFolderName);
builder.field("extended_plugins", extendedPlugins);
builder.field("has_native_controller", hasNativeController);
builder.field("compatible_across_patch_versions", compatibleAcrossPatchVersions);
}
builder.endObject();

Expand Down Expand Up @@ -452,7 +478,11 @@ public String toString(String prefix) {
.append("\n")
.append(prefix)
.append("Folder name: ")
.append(customFolderName);
.append(customFolderName)
.append("\n")
.append(prefix)
.append("Compatible across patch versions: ")
.append(compatibleAcrossPatchVersions);
return information.toString();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ public PluginsService(
pluginClass.getName(),
null,
Collections.emptyList(),
false,
false
);
if (logger.isTraceEnabled()) {
Expand Down Expand Up @@ -387,7 +388,7 @@ public static List<Path> findPluginDirs(final Path rootPath) throws IOException
* Verify the given plugin is compatible with the current OpenSearch installation.
*/
static void verifyCompatibility(PluginInfo info) {
if (info.getOpenSearchVersion().equals(Version.CURRENT) == false) {
if (!isPluginVersionCompatible(info, Version.CURRENT)) {
throw new IllegalArgumentException(
"Plugin ["
+ info.getName()
Expand All @@ -401,6 +402,21 @@ static void verifyCompatibility(PluginInfo info) {
JarHell.checkJavaVersion(info.getName(), info.getJavaVersion());
}

/**
* This method checks if plugin was built with a version that is compatible with core's version.
* @param pluginInfo {@code PluginInfo} to fetch plugin properties
* @param coreVersion Core version
* @return true if plugin's version is compatible with core, false otherwise
*/
static boolean isPluginVersionCompatible(final PluginInfo pluginInfo, final Version coreVersion) {
final Version pluginVersion = pluginInfo.getOpenSearchVersion();
if (pluginInfo.compatibleAcrossPatchVersions()) {
// Ignore patch version if plugin is compatible across patch versions
return pluginVersion.major == coreVersion.major && pluginVersion.minor == coreVersion.minor;
}
return pluginVersion.equals(coreVersion);
}

static void checkForFailedPluginRemovals(final Path pluginsDirectory) throws IOException {
/*
* Check for the existence of a marker file that indicates any plugins are in a garbage state from a failed attempt to remove the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ private static NodeInfo createNodeInfo() {
randomAlphaOfLengthBetween(3, 10),
name,
Collections.emptyList(),
randomBoolean(),
randomBoolean()
)
);
Expand All @@ -197,6 +198,7 @@ private static NodeInfo createNodeInfo() {
randomAlphaOfLengthBetween(3, 10),
name,
Collections.emptyList(),
randomBoolean(),
randomBoolean()
)
);
Expand Down
Loading
Loading