Skip to content

Commit cb8648c

Browse files
committed
Add check for feature aware implementations (#31081)
This commit adds a check that any class in X-Pack that is a feature aware custom also implements the appropriate mix-in interface in X-Pack. These interfaces provide a default implementation of FeatureAware#getRequiredFeature that returns that x-pack is the required feature. By implementing this interface, this gives a consistent way for X-Pack feature aware customs to return the appopriate required feature and this check enforces that all such feature aware customs return the appropriate required feature.
1 parent 8fb2f2b commit cb8648c

File tree

5 files changed

+574
-6
lines changed

5 files changed

+574
-6
lines changed

build.gradle

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,7 @@ subprojects {
220220
"org.elasticsearch.distribution.deb:elasticsearch:${version}": ':distribution:packages:deb',
221221
"org.elasticsearch.distribution.deb:elasticsearch-oss:${version}": ':distribution:packages:oss-deb',
222222
"org.elasticsearch.test:logger-usage:${version}": ':test:logger-usage',
223+
"org.elasticsearch.xpack.test:feature-aware:${version}": ':x-pack:test:feature-aware',
223224
// for transport client
224225
"org.elasticsearch.plugin:transport-netty4-client:${version}": ':modules:transport-netty4',
225226
"org.elasticsearch.plugin:reindex-client:${version}": ':modules:reindex',
@@ -305,7 +306,15 @@ gradle.projectsEvaluated {
305306
// :test:framework:test cannot run before and after :server:test
306307
return
307308
}
308-
configurations.all {
309+
configurations.all { Configuration configuration ->
310+
/*
311+
* The featureAwarePlugin configuration has a dependency on x-pack:plugin:core and x-pack:plugin:core has a dependency on the
312+
* featureAwarePlugin configuration. The below task ordering logic would force :x-pack:plugin:core:test
313+
* :x-pack:test:feature-aware:test to depend on each other circularly. We break that cycle here.
314+
*/
315+
if (configuration.name == "featureAwarePlugin") {
316+
return
317+
}
309318
dependencies.all { Dependency dep ->
310319
Project upstreamProject = dependencyToProject(dep)
311320
if (upstreamProject != null) {

x-pack/plugin/build.gradle

Lines changed: 45 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,8 @@
11
import org.elasticsearch.gradle.LoggedExec
2-
import org.elasticsearch.gradle.MavenFilteringHack
2+
import org.elasticsearch.gradle.plugin.PluginBuildPlugin
33
import org.elasticsearch.gradle.test.NodeInfo
44

55
import java.nio.charset.StandardCharsets
6-
import java.nio.file.Files
7-
import java.nio.file.Path
8-
import java.nio.file.StandardCopyOption
9-
import org.elasticsearch.gradle.test.RunTask;
106

117
apply plugin: 'elasticsearch.standalone-rest-test'
128
apply plugin: 'elasticsearch.rest-test'
@@ -17,6 +13,50 @@ dependencies {
1713
testCompile project(path: xpackModule('core'), configuration: 'testArtifacts')
1814
}
1915

16+
subprojects {
17+
afterEvaluate {
18+
if (project.plugins.hasPlugin(PluginBuildPlugin)) {
19+
// see the root Gradle file for additional logic regarding this configuration
20+
project.configurations.create('featureAwarePlugin')
21+
project.dependencies.add('featureAwarePlugin', project.configurations.compileClasspath)
22+
project.dependencies.add(
23+
'featureAwarePlugin',
24+
"org.elasticsearch.xpack.test:feature-aware:${org.elasticsearch.gradle.VersionProperties.elasticsearch}")
25+
project.dependencies.add('featureAwarePlugin', project.sourceSets.main.output.getClassesDirs())
26+
27+
final Task featureAwareTask = project.tasks.create("featureAwareCheck", LoggedExec) {
28+
description = "Runs FeatureAwareCheck on main classes."
29+
dependsOn project.configurations.featureAwarePlugin
30+
31+
final File successMarker = new File(project.buildDir, 'markers/featureAware')
32+
outputs.file(successMarker)
33+
34+
executable = new File(project.runtimeJavaHome, 'bin/java')
35+
36+
// default to main class files if such a source set exists
37+
final List files = []
38+
if (project.sourceSets.findByName("main")) {
39+
files.add(project.sourceSets.main.output.classesDir)
40+
dependsOn project.tasks.classes
41+
}
42+
// filter out non-existent classes directories from empty source sets
43+
final FileCollection classDirectories = project.files(files).filter { it.exists() }
44+
45+
doFirst {
46+
args('-cp', project.configurations.featureAwarePlugin.asPath, 'org.elasticsearch.xpack.test.feature_aware.FeatureAwareCheck')
47+
classDirectories.each { args it.getAbsolutePath() }
48+
}
49+
doLast {
50+
successMarker.parentFile.mkdirs()
51+
successMarker.setText("", 'UTF-8')
52+
}
53+
}
54+
55+
project.precommit.dependsOn featureAwareTask
56+
}
57+
}
58+
}
59+
2060
// https://github.com/elastic/x-plugins/issues/724
2161
configurations {
2262
testArtifacts.extendsFrom testRuntime
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
apply plugin: 'elasticsearch.build'
2+
3+
dependencies {
4+
compile 'org.ow2.asm:asm:6.2'
5+
compile "org.elasticsearch:elasticsearch:${version}"
6+
compile "org.elasticsearch.plugin:x-pack-core:${version}"
7+
testCompile "org.elasticsearch.test:framework:${version}"
8+
}
9+
10+
forbiddenApisMain.enabled = true
11+
12+
dependencyLicenses.enabled = false
13+
14+
jarHell.enabled = false
15+
16+
thirdPartyAudit.enabled = false
Lines changed: 180 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,180 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License;
4+
* you may not use this file except in compliance with the Elastic License.
5+
*/
6+
7+
package org.elasticsearch.xpack.test.feature_aware;
8+
9+
import org.elasticsearch.cluster.ClusterState;
10+
import org.elasticsearch.cluster.metadata.MetaData;
11+
import org.elasticsearch.common.SuppressForbidden;
12+
import org.elasticsearch.common.io.PathUtils;
13+
import org.elasticsearch.persistent.PersistentTaskParams;
14+
import org.elasticsearch.xpack.core.XPackPlugin;
15+
import org.objectweb.asm.ClassReader;
16+
17+
import java.io.FileNotFoundException;
18+
import java.io.IOException;
19+
import java.io.InputStream;
20+
import java.nio.file.FileVisitResult;
21+
import java.nio.file.Files;
22+
import java.nio.file.Path;
23+
import java.nio.file.Paths;
24+
import java.nio.file.SimpleFileVisitor;
25+
import java.nio.file.attribute.BasicFileAttributes;
26+
import java.util.ArrayList;
27+
import java.util.Arrays;
28+
import java.util.List;
29+
import java.util.Set;
30+
import java.util.TreeSet;
31+
import java.util.function.Consumer;
32+
33+
/**
34+
* Used in the featureAwareCheck to check for classes in X-Pack that implement customs but do not extend the appropriate marker interface.
35+
*/
36+
public final class FeatureAwareCheck {
37+
38+
/**
39+
* Check the class directories specified by the arguments for classes in X-Pack that implement customs but do not extend the appropriate
40+
* marker interface that provides a mix-in implementation of {@link ClusterState.FeatureAware#getRequiredFeature()}.
41+
*
42+
* @param args the class directories to check
43+
* @throws IOException if an I/O exception is walking the class directories
44+
*/
45+
public static void main(final String[] args) throws IOException {
46+
systemOutPrintln("checking for custom violations");
47+
final List<FeatureAwareViolation> violations = new ArrayList<>();
48+
checkDirectories(violations::add, args);
49+
if (violations.isEmpty()) {
50+
systemOutPrintln("no custom violations found");
51+
} else {
52+
violations.forEach(violation ->
53+
systemOutPrintln(
54+
"class [" + violation.name + "] implements"
55+
+ " [" + violation.interfaceName + " but does not implement"
56+
+ " [" + violation.expectedInterfaceName + "]")
57+
);
58+
throw new IllegalStateException(
59+
"found custom" + (violations.size() == 1 ? "" : "s") + " in X-Pack not extending appropriate X-Pack mix-in");
60+
}
61+
}
62+
63+
@SuppressForbidden(reason = "System.out#println")
64+
private static void systemOutPrintln(final String s) {
65+
System.out.println(s);
66+
}
67+
68+
private static void checkDirectories(
69+
final Consumer<FeatureAwareViolation> callback,
70+
final String... classDirectories) throws IOException {
71+
for (final String classDirectory : classDirectories) {
72+
final Path root = pathsGet(classDirectory);
73+
if (Files.isDirectory(root)) {
74+
Files.walkFileTree(root, new SimpleFileVisitor<Path>() {
75+
@Override
76+
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException {
77+
if (Files.isRegularFile(file) && file.getFileName().toString().endsWith(".class")) {
78+
try (InputStream in = Files.newInputStream(file)) {
79+
checkClass(in, callback);
80+
}
81+
}
82+
return super.visitFile(file, attrs);
83+
}
84+
});
85+
} else {
86+
throw new FileNotFoundException("class directory [" + classDirectory + "] should exist");
87+
}
88+
}
89+
}
90+
91+
@SuppressForbidden(reason = "Paths#get")
92+
private static Path pathsGet(final String pathString) {
93+
return Paths.get(pathString);
94+
}
95+
96+
/**
97+
* Represents a feature-aware violation.
98+
*/
99+
static class FeatureAwareViolation {
100+
101+
final String name;
102+
final String interfaceName;
103+
final String expectedInterfaceName;
104+
105+
/**
106+
* Constructs a representation of a feature-aware violation.
107+
*
108+
* @param name the name of the custom class
109+
* @param interfaceName the name of the feature-aware interface
110+
* @param expectedInterfaceName the name of the expected mix-in class
111+
*/
112+
FeatureAwareViolation(final String name, final String interfaceName, final String expectedInterfaceName) {
113+
this.name = name;
114+
this.interfaceName = interfaceName;
115+
this.expectedInterfaceName = expectedInterfaceName;
116+
}
117+
118+
}
119+
120+
/**
121+
* Loads a class from the specified input stream and checks that if it implements a feature-aware custom then it extends the appropriate
122+
* mix-in interface from X-Pack. If the class does not, then the specified callback is invoked.
123+
*
124+
* @param in the input stream
125+
* @param callback the callback to invoke
126+
* @throws IOException if an I/O exception occurs loading the class hierarchy
127+
*/
128+
static void checkClass(final InputStream in, final Consumer<FeatureAwareViolation> callback) throws IOException {
129+
// the class format only reports declared interfaces so we have to walk the hierarchy looking for all interfaces
130+
final List<String> interfaces = new ArrayList<>();
131+
ClassReader cr = new ClassReader(in);
132+
final String name = cr.getClassName();
133+
do {
134+
interfaces.addAll(Arrays.asList(cr.getInterfaces()));
135+
final String superName = cr.getSuperName();
136+
if ("java/lang/Object".equals(superName)) {
137+
break;
138+
}
139+
cr = new ClassReader(superName);
140+
} while (true);
141+
checkClass(name, interfaces, callback);
142+
}
143+
144+
private static void checkClass(
145+
final String name,
146+
final List<String> interfaces,
147+
final Consumer<FeatureAwareViolation> callback) {
148+
checkCustomForClass(ClusterState.Custom.class, XPackPlugin.XPackClusterStateCustom.class, name, interfaces, callback);
149+
checkCustomForClass(MetaData.Custom.class, XPackPlugin.XPackMetaDataCustom.class, name, interfaces, callback);
150+
checkCustomForClass(PersistentTaskParams.class, XPackPlugin.XPackPersistentTaskParams.class, name, interfaces, callback);
151+
}
152+
153+
private static void checkCustomForClass(
154+
final Class<? extends ClusterState.FeatureAware> interfaceToCheck,
155+
final Class<? extends ClusterState.FeatureAware> expectedInterface,
156+
final String name,
157+
final List<String> interfaces,
158+
final Consumer<FeatureAwareViolation> callback) {
159+
final Set<String> interfaceSet = new TreeSet<>(interfaces);
160+
final String interfaceToCheckName = formatClassName(interfaceToCheck);
161+
final String expectedXPackInterfaceName = formatClassName(expectedInterface);
162+
if (interfaceSet.contains(interfaceToCheckName)
163+
&& name.equals(expectedXPackInterfaceName) == false
164+
&& interfaceSet.contains(expectedXPackInterfaceName) == false) {
165+
assert name.startsWith("org/elasticsearch/license") || name.startsWith("org/elasticsearch/xpack");
166+
callback.accept(new FeatureAwareViolation(name, interfaceToCheckName, expectedXPackInterfaceName));
167+
}
168+
}
169+
170+
/**
171+
* Format the specified class to a name in the ASM format replacing all dots in the class name with forward-slashes.
172+
*
173+
* @param clazz the class whose name to format
174+
* @return the formatted class name
175+
*/
176+
static String formatClassName(final Class<?> clazz) {
177+
return clazz.getName().replace(".", "/");
178+
}
179+
180+
}

0 commit comments

Comments
 (0)