Skip to content

Commit

Permalink
Making classname optional in Transport protocol (#99702)
Browse files Browse the repository at this point in the history 
Making classname optional in Transport protocol since it is not needed with Stable plugins.
  • Loading branch information
@ldematte
ldematte authored Oct 6, 2023
1 parent 002d941 commit fd37628
Show file tree
Hide file tree
Showing 9 changed files with 87 additions and 21 deletions.
6 changes: 6 additions & 0 deletions docs/changelog/99702.yaml
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 99702
summary: Making classname optional in Transport protocol
area: Infra/Plugins
type: bug
issues:
- 98584
2 changes: 1 addition & 1 deletion server/src/main/java/org/elasticsearch/TransportVersions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ static TransportVersion def(int id) {
public static final TransportVersion NODE_INFO_REQUEST_SIMPLIFIED = def(8_510_00_0);
public static final TransportVersion NESTED_KNN_VECTOR_QUERY_V = def(8_511_00_0);
public static final TransportVersion ML_PACKAGE_LOADER_PLATFORM_ADDED = def(8_512_00_0);

public static final TransportVersion PLUGIN_DESCRIPTOR_OPTIONAL_CLASSNAME = def(8_513_00_0);
/*
* STOP! READ THIS FIRST! No, really,
* ____ _____ ___ ____ _ ____ _____ _ ____ _____ _ _ ___ ____ _____ ___ ____ ____ _____ _
Expand Down
32 changes: 30 additions & 2 deletions server/src/main/java/org/elasticsearch/plugins/PluginDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public class PluginDescriptor implements Writeable, ToXContentObject {
* @param hasNativeController whether or not the plugin has a native controller
* @param isLicensed whether is this a licensed plugin
* @param isModular whether this plugin should be loaded in a module layer
* @param isStable whether this plugin is implemented using the stable plugin API
*/
public PluginDescriptor(
String name,
Expand Down Expand Up @@ -105,6 +106,8 @@ public PluginDescriptor(
this.isLicensed = isLicensed;
this.isModular = isModular;
this.isStable = isStable;

ensureCorrectArgumentsForPluginType();
}

/**
Expand All @@ -119,7 +122,11 @@ public PluginDescriptor(final StreamInput in) throws IOException {
this.version = in.readString();
elasticsearchVersion = Version.readVersion(in);
javaVersion = in.readString();
this.classname = in.readString();
if (in.getTransportVersion().onOrAfter(TransportVersions.PLUGIN_DESCRIPTOR_OPTIONAL_CLASSNAME)) {
this.classname = in.readOptionalString();
} else {
this.classname = in.readString();
}
if (in.getTransportVersion().onOrAfter(MODULE_NAME_SUPPORT)) {
this.moduleName = in.readOptionalString();
} else {
Expand All @@ -145,6 +152,8 @@ public PluginDescriptor(final StreamInput in) throws IOException {
isModular = moduleName != null;
isStable = false;
}

ensureCorrectArgumentsForPluginType();
}

@Override
Expand All @@ -154,7 +163,11 @@ public void writeTo(final StreamOutput out) throws IOException {
out.writeString(version);
Version.writeVersion(elasticsearchVersion, out);
out.writeString(javaVersion);
out.writeString(classname);
if (out.getTransportVersion().onOrAfter(TransportVersions.PLUGIN_DESCRIPTOR_OPTIONAL_CLASSNAME)) {
out.writeOptionalString(classname);
} else {
out.writeString(classname);
}
if (out.getTransportVersion().onOrAfter(MODULE_NAME_SUPPORT)) {
out.writeOptionalString(moduleName);
}
Expand All @@ -174,6 +187,18 @@ public void writeTo(final StreamOutput out) throws IOException {
}
}

private void ensureCorrectArgumentsForPluginType() {
if (classname == null && isStable == false) {
throw new IllegalArgumentException("Classname must be provided for classic plugins");
}
if (classname != null && isStable) {
throw new IllegalArgumentException("Classname is not needed for stable plugins");
}
if (moduleName != null && isStable) {
throw new IllegalArgumentException("ModuleName is not needed for stable plugins");
}
}

/**
* Reads the descriptor file for a plugin.
*
Expand Down Expand Up @@ -329,6 +354,9 @@ public String getDescription() {
* @return the entry point to the plugin
*/
public String getClassname() {
if (isStable) {
throw new IllegalStateException("Stable plugins do not have an explicit entry point");
}
return classname;
}

Expand Down
16 changes: 10 additions & 6 deletions server/src/test/java/org/elasticsearch/nodesinfo/NodeInfoStreamingTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,40 +156,44 @@ private static NodeInfo createNodeInfo() {
int numPlugins = randomIntBetween(0, 5);
List<PluginDescriptor> plugins = new ArrayList<>();
for (int i = 0; i < numPlugins; i++) {
var isStable = randomBoolean();
var hasModuleName = randomBoolean();
plugins.add(
new PluginDescriptor(
randomAlphaOfLengthBetween(3, 10),
randomAlphaOfLengthBetween(3, 10),
randomAlphaOfLengthBetween(3, 10),
VersionUtils.randomVersion(random()),
"1.8",
randomAlphaOfLengthBetween(3, 10),
randomBoolean() ? null : randomAlphaOfLengthBetween(3, 10),
isStable ? null : randomAlphaOfLengthBetween(3, 10),
isStable || hasModuleName == false ? null : randomAlphaOfLengthBetween(3, 10),
Collections.emptyList(),
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
isStable
)
);
}
int numModules = randomIntBetween(0, 5);
List<PluginDescriptor> modules = new ArrayList<>();
for (int i = 0; i < numModules; i++) {
var isStable = randomBoolean();
var hasModuleName = randomBoolean();
modules.add(
new PluginDescriptor(
randomAlphaOfLengthBetween(3, 10),
randomAlphaOfLengthBetween(3, 10),
randomAlphaOfLengthBetween(3, 10),
VersionUtils.randomVersion(random()),
"1.8",
randomAlphaOfLengthBetween(3, 10),
randomBoolean() ? null : randomAlphaOfLengthBetween(3, 10),
isStable ? null : randomAlphaOfLengthBetween(3, 10),
isStable || hasModuleName == false ? null : randomAlphaOfLengthBetween(3, 10),
Collections.emptyList(),
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
isStable
)
);
}
Expand Down
28 changes: 20 additions & 8 deletions server/src/test/java/org/elasticsearch/plugins/PluginDescriptorTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ public void testSerialize() throws Exception {
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
false
);
BytesStreamOutput output = new BytesStreamOutput();
info.writeTo(output);
Expand All @@ -258,7 +258,7 @@ public void testSerializeWithModuleName() throws Exception {
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
false
);
BytesStreamOutput output = new BytesStreamOutput();
info.writeTo(output);
Expand All @@ -268,6 +268,16 @@ public void testSerializeWithModuleName() throws Exception {
assertThat(info2.toString(), equalTo(info.toString()));
}

public void testSerializeStablePluginDescriptor() throws Exception {
PluginDescriptor info = mockStableDescriptor();
BytesStreamOutput output = new BytesStreamOutput();
info.writeTo(output);
ByteBuffer buffer = ByteBuffer.wrap(output.bytes().toBytesRef().bytes);
ByteBufferStreamInput input = new ByteBufferStreamInput(buffer);
PluginDescriptor info2 = new PluginDescriptor(input);
assertThat(info2.toString(), equalTo(info.toString()));
}

PluginDescriptor newMockDescriptor(String name) {
return new PluginDescriptor(
name,
Expand All @@ -281,7 +291,7 @@ PluginDescriptor newMockDescriptor(String name) {
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
false
);
}

Expand Down Expand Up @@ -311,19 +321,21 @@ public void testUnknownProperties() throws Exception {
* use the hashcode to catch duplicate names
*/
public void testPluginEqualityAndHash() {
var isStable = randomBoolean();
var classname = isStable ? null : "dummyclass";
PluginDescriptor descriptor1 = new PluginDescriptor(
"c",
"foo",
"dummy",
Version.CURRENT,
"1.8",
"dummyclass",
classname,
null,
Collections.singletonList("foo"),
randomBoolean(),
randomBoolean(),
randomBoolean(),
randomBoolean()
isStable
);
// everything but name is different from descriptor1
PluginDescriptor descriptor2 = new PluginDescriptor(
Expand All @@ -332,8 +344,8 @@ public void testPluginEqualityAndHash() {
randomValueOtherThan(descriptor1.getVersion(), () -> randomAlphaOfLengthBetween(4, 12)),
descriptor1.getElasticsearchVersion().previousMajor(),
randomValueOtherThan(descriptor1.getJavaVersion(), () -> randomAlphaOfLengthBetween(4, 12)),
randomValueOtherThan(descriptor1.getClassname(), () -> randomAlphaOfLengthBetween(4, 12)),
randomAlphaOfLength(6),
descriptor1.isStable() ? randomAlphaOfLengthBetween(4, 12) : null,
descriptor1.isStable() ? randomAlphaOfLength(6) : null,
Collections.singletonList(
randomValueOtherThanMany(v -> descriptor1.getExtendedPlugins().contains(v), () -> randomAlphaOfLengthBetween(4, 12))
),
Expand All @@ -349,7 +361,7 @@ public void testPluginEqualityAndHash() {
descriptor1.getVersion(),
descriptor1.getElasticsearchVersion(),
descriptor1.getJavaVersion(),
descriptor1.getClassname(),
classname,
descriptor1.getModuleName().orElse(null),
descriptor1.getExtendedPlugins(),
descriptor1.hasNativeController(),
Expand Down
20 changes: 17 additions & 3 deletions server/src/test/java/org/elasticsearch/plugins/PluginsServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -460,10 +460,11 @@ public void testPluginFromParentClassLoader() throws IOException {

public void testExtensiblePlugin() {
TestExtensiblePlugin extensiblePlugin = new TestExtensiblePlugin();
var classname = "FakePlugin";
PluginsService.loadExtensions(
List.of(
new PluginsService.LoadedPlugin(
new PluginDescriptor("extensible", null, null, null, null, null, null, List.of(), false, false, false, false),
new PluginDescriptor("extensible", null, null, null, null, classname, null, List.of(), false, false, false, false),
extensiblePlugin
)
)
Expand All @@ -477,11 +478,24 @@ public void testExtensiblePlugin() {
PluginsService.loadExtensions(
List.of(
new PluginsService.LoadedPlugin(
new PluginDescriptor("extensible", null, null, null, null, null, null, List.of(), false, false, false, false),
new PluginDescriptor("extensible", null, null, null, null, classname, null, List.of(), false, false, false, false),
extensiblePlugin
),
new PluginsService.LoadedPlugin(
new PluginDescriptor("test", null, null, null, null, null, null, List.of("extensible"), false, false, false, false),
new PluginDescriptor(
"test",
null,
null,
null,
null,
classname,
null,
List.of("extensible"),
false,
false,
false,
false
),
testPlugin
)
)
Expand Down
2 changes: 1 addition & 1 deletion server/src/test/java/org/elasticsearch/plugins/PluginsUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ private static PluginDescriptor getPluginDescriptorForVersion(Version id, String
"1.0",
id,
javaVersion,
"FakePlugin",
isStable ? null : "FakePlugin",
null,
Collections.emptyList(),
false,
Expand Down
1 change: 1 addition & 0 deletions ...urity/src/internalClusterTest/java/org/elasticsearch/test/SecuritySingleNodeTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ private void doAssertXPackIsInstalled() {
Collection<String> pluginNames = nodeInfo.getInfo(PluginsAndModules.class)
.getPluginInfos()
.stream()
.filter(p -> p.descriptor().isStable() == false)
.map(p -> p.descriptor().getClassname())
.collect(Collectors.toList());
assertThat(
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecurityIntegTestCase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ protected void doAssertXPackIsInstalled() {
Collection<String> pluginNames = nodeInfo.getInfo(PluginsAndModules.class)
.getPluginInfos()
.stream()
.filter(p -> p.descriptor().isStable() == false)
.map(p -> p.descriptor().getClassname())
.collect(Collectors.toList());
assertThat(
Expand Down

0 comments on commit fd37628

Please sign in to comment.