From 805648848d50b89e4c85d2f35a8de7456c67a694 Mon Sep 17 00:00:00 2001 From: Jason Tedor Date: Tue, 5 Jun 2018 19:56:22 -0400 Subject: [PATCH] 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. --- build.gradle | 11 +- x-pack/plugin/build.gradle | 50 ++- x-pack/test/feature-aware/build.gradle | 16 + .../test/feature_aware/FeatureAwareCheck.java | 180 ++++++++++ .../feature_aware/FeatureAwareCheckTests.java | 323 ++++++++++++++++++ 5 files changed, 574 insertions(+), 6 deletions(-) create mode 100644 x-pack/test/feature-aware/build.gradle create mode 100644 x-pack/test/feature-aware/src/main/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheck.java create mode 100644 x-pack/test/feature-aware/src/test/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheckTests.java diff --git a/build.gradle b/build.gradle index 05ad5479e8d..620e043d1c0 100644 --- a/build.gradle +++ b/build.gradle @@ -226,6 +226,7 @@ subprojects { "org.elasticsearch.distribution.deb:elasticsearch:${version}": ':distribution:packages:deb', "org.elasticsearch.distribution.deb:elasticsearch-oss:${version}": ':distribution:packages:oss-deb', "org.elasticsearch.test:logger-usage:${version}": ':test:logger-usage', + "org.elasticsearch.xpack.test:feature-aware:${version}": ':x-pack:test:feature-aware', // for transport client "org.elasticsearch.plugin:transport-netty4-client:${version}": ':modules:transport-netty4', "org.elasticsearch.plugin:reindex-client:${version}": ':modules:reindex', @@ -311,7 +312,15 @@ gradle.projectsEvaluated { // :test:framework:test cannot run before and after :server:test return } - configurations.all { + configurations.all { Configuration configuration -> + /* + * The featureAwarePlugin configuration has a dependency on x-pack:plugin:core and x-pack:plugin:core has a dependency on the + * featureAwarePlugin configuration. The below task ordering logic would force :x-pack:plugin:core:test + * :x-pack:test:feature-aware:test to depend on each other circularly. We break that cycle here. + */ + if (configuration.name == "featureAwarePlugin") { + return + } dependencies.all { Dependency dep -> Project upstreamProject = dependencyToProject(dep) if (upstreamProject != null) { diff --git a/x-pack/plugin/build.gradle b/x-pack/plugin/build.gradle index 4a0b29c4258..ac423c42811 100644 --- a/x-pack/plugin/build.gradle +++ b/x-pack/plugin/build.gradle @@ -1,12 +1,8 @@ import org.elasticsearch.gradle.LoggedExec -import org.elasticsearch.gradle.MavenFilteringHack +import org.elasticsearch.gradle.plugin.PluginBuildPlugin import org.elasticsearch.gradle.test.NodeInfo import java.nio.charset.StandardCharsets -import java.nio.file.Files -import java.nio.file.Path -import java.nio.file.StandardCopyOption -import org.elasticsearch.gradle.test.RunTask; apply plugin: 'elasticsearch.standalone-rest-test' apply plugin: 'elasticsearch.rest-test' @@ -17,6 +13,50 @@ dependencies { testCompile project(path: xpackModule('core'), configuration: 'testArtifacts') } +subprojects { + afterEvaluate { + if (project.plugins.hasPlugin(PluginBuildPlugin)) { + // see the root Gradle file for additional logic regarding this configuration + project.configurations.create('featureAwarePlugin') + project.dependencies.add('featureAwarePlugin', project.configurations.compileClasspath) + project.dependencies.add( + 'featureAwarePlugin', + "org.elasticsearch.xpack.test:feature-aware:${org.elasticsearch.gradle.VersionProperties.elasticsearch}") + project.dependencies.add('featureAwarePlugin', project.sourceSets.main.output.getClassesDirs()) + + final Task featureAwareTask = project.tasks.create("featureAwareCheck", LoggedExec) { + description = "Runs FeatureAwareCheck on main classes." + dependsOn project.configurations.featureAwarePlugin + + final File successMarker = new File(project.buildDir, 'markers/featureAware') + outputs.file(successMarker) + + executable = new File(project.runtimeJavaHome, 'bin/java') + + // default to main class files if such a source set exists + final List files = [] + if (project.sourceSets.findByName("main")) { + files.add(project.sourceSets.main.output.classesDir) + dependsOn project.tasks.classes + } + // filter out non-existent classes directories from empty source sets + final FileCollection classDirectories = project.files(files).filter { it.exists() } + + doFirst { + args('-cp', project.configurations.featureAwarePlugin.asPath, 'org.elasticsearch.xpack.test.feature_aware.FeatureAwareCheck') + classDirectories.each { args it.getAbsolutePath() } + } + doLast { + successMarker.parentFile.mkdirs() + successMarker.setText("", 'UTF-8') + } + } + + project.precommit.dependsOn featureAwareTask + } + } +} + // https://github.com/elastic/x-plugins/issues/724 configurations { testArtifacts.extendsFrom testRuntime diff --git a/x-pack/test/feature-aware/build.gradle b/x-pack/test/feature-aware/build.gradle new file mode 100644 index 00000000000..217ed25a2d4 --- /dev/null +++ b/x-pack/test/feature-aware/build.gradle @@ -0,0 +1,16 @@ +apply plugin: 'elasticsearch.build' + +dependencies { + compile 'org.ow2.asm:asm:6.2' + compile "org.elasticsearch:elasticsearch:${version}" + compile "org.elasticsearch.plugin:x-pack-core:${version}" + testCompile "org.elasticsearch.test:framework:${version}" +} + +forbiddenApisMain.enabled = true + +dependencyLicenses.enabled = false + +jarHell.enabled = false + +thirdPartyAudit.enabled = false diff --git a/x-pack/test/feature-aware/src/main/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheck.java b/x-pack/test/feature-aware/src/main/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheck.java new file mode 100644 index 00000000000..7746692b408 --- /dev/null +++ b/x-pack/test/feature-aware/src/main/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheck.java @@ -0,0 +1,180 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.test.feature_aware; + +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.common.SuppressForbidden; +import org.elasticsearch.common.io.PathUtils; +import org.elasticsearch.persistent.PersistentTaskParams; +import org.elasticsearch.xpack.core.XPackPlugin; +import org.objectweb.asm.ClassReader; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.io.InputStream; +import java.nio.file.FileVisitResult; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.SimpleFileVisitor; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.Set; +import java.util.TreeSet; +import java.util.function.Consumer; + +/** + * Used in the featureAwareCheck to check for classes in X-Pack that implement customs but do not extend the appropriate marker interface. + */ +public final class FeatureAwareCheck { + + /** + * Check the class directories specified by the arguments for classes in X-Pack that implement customs but do not extend the appropriate + * marker interface that provides a mix-in implementation of {@link ClusterState.FeatureAware#getRequiredFeature()}. + * + * @param args the class directories to check + * @throws IOException if an I/O exception is walking the class directories + */ + public static void main(final String[] args) throws IOException { + systemOutPrintln("checking for custom violations"); + final List violations = new ArrayList<>(); + checkDirectories(violations::add, args); + if (violations.isEmpty()) { + systemOutPrintln("no custom violations found"); + } else { + violations.forEach(violation -> + systemOutPrintln( + "class [" + violation.name + "] implements" + + " [" + violation.interfaceName + " but does not implement" + + " [" + violation.expectedInterfaceName + "]") + ); + throw new IllegalStateException( + "found custom" + (violations.size() == 1 ? "" : "s") + " in X-Pack not extending appropriate X-Pack mix-in"); + } + } + + @SuppressForbidden(reason = "System.out#println") + private static void systemOutPrintln(final String s) { + System.out.println(s); + } + + private static void checkDirectories( + final Consumer callback, + final String... classDirectories) throws IOException { + for (final String classDirectory : classDirectories) { + final Path root = pathsGet(classDirectory); + if (Files.isDirectory(root)) { + Files.walkFileTree(root, new SimpleFileVisitor() { + @Override + public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) throws IOException { + if (Files.isRegularFile(file) && file.getFileName().toString().endsWith(".class")) { + try (InputStream in = Files.newInputStream(file)) { + checkClass(in, callback); + } + } + return super.visitFile(file, attrs); + } + }); + } else { + throw new FileNotFoundException("class directory [" + classDirectory + "] should exist"); + } + } + } + + @SuppressForbidden(reason = "Paths#get") + private static Path pathsGet(final String pathString) { + return Paths.get(pathString); + } + + /** + * Represents a feature-aware violation. + */ + static class FeatureAwareViolation { + + final String name; + final String interfaceName; + final String expectedInterfaceName; + + /** + * Constructs a representation of a feature-aware violation. + * + * @param name the name of the custom class + * @param interfaceName the name of the feature-aware interface + * @param expectedInterfaceName the name of the expected mix-in class + */ + FeatureAwareViolation(final String name, final String interfaceName, final String expectedInterfaceName) { + this.name = name; + this.interfaceName = interfaceName; + this.expectedInterfaceName = expectedInterfaceName; + } + + } + + /** + * Loads a class from the specified input stream and checks that if it implements a feature-aware custom then it extends the appropriate + * mix-in interface from X-Pack. If the class does not, then the specified callback is invoked. + * + * @param in the input stream + * @param callback the callback to invoke + * @throws IOException if an I/O exception occurs loading the class hierarchy + */ + static void checkClass(final InputStream in, final Consumer callback) throws IOException { + // the class format only reports declared interfaces so we have to walk the hierarchy looking for all interfaces + final List interfaces = new ArrayList<>(); + ClassReader cr = new ClassReader(in); + final String name = cr.getClassName(); + do { + interfaces.addAll(Arrays.asList(cr.getInterfaces())); + final String superName = cr.getSuperName(); + if ("java/lang/Object".equals(superName)) { + break; + } + cr = new ClassReader(superName); + } while (true); + checkClass(name, interfaces, callback); + } + + private static void checkClass( + final String name, + final List interfaces, + final Consumer callback) { + checkCustomForClass(ClusterState.Custom.class, XPackPlugin.XPackClusterStateCustom.class, name, interfaces, callback); + checkCustomForClass(MetaData.Custom.class, XPackPlugin.XPackMetaDataCustom.class, name, interfaces, callback); + checkCustomForClass(PersistentTaskParams.class, XPackPlugin.XPackPersistentTaskParams.class, name, interfaces, callback); + } + + private static void checkCustomForClass( + final Class interfaceToCheck, + final Class expectedInterface, + final String name, + final List interfaces, + final Consumer callback) { + final Set interfaceSet = new TreeSet<>(interfaces); + final String interfaceToCheckName = formatClassName(interfaceToCheck); + final String expectedXPackInterfaceName = formatClassName(expectedInterface); + if (interfaceSet.contains(interfaceToCheckName) + && name.equals(expectedXPackInterfaceName) == false + && interfaceSet.contains(expectedXPackInterfaceName) == false) { + assert name.startsWith("org/elasticsearch/license") || name.startsWith("org/elasticsearch/xpack"); + callback.accept(new FeatureAwareViolation(name, interfaceToCheckName, expectedXPackInterfaceName)); + } + } + + /** + * Format the specified class to a name in the ASM format replacing all dots in the class name with forward-slashes. + * + * @param clazz the class whose name to format + * @return the formatted class name + */ + static String formatClassName(final Class clazz) { + return clazz.getName().replace(".", "/"); + } + +} diff --git a/x-pack/test/feature-aware/src/test/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheckTests.java b/x-pack/test/feature-aware/src/test/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheckTests.java new file mode 100644 index 00000000000..2dde9efce42 --- /dev/null +++ b/x-pack/test/feature-aware/src/test/java/org/elasticsearch/xpack/test/feature_aware/FeatureAwareCheckTests.java @@ -0,0 +1,323 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +package org.elasticsearch.xpack.test.feature_aware; + +import org.elasticsearch.Version; +import org.elasticsearch.cluster.ClusterState; +import org.elasticsearch.cluster.Diff; +import org.elasticsearch.cluster.metadata.MetaData; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.persistent.PersistentTaskParams; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.XPackPlugin; + +import java.io.IOException; +import java.util.EnumSet; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.function.Consumer; + +import static org.hamcrest.Matchers.equalTo; + +public class FeatureAwareCheckTests extends ESTestCase { + + public void testClusterStateCustomViolation() throws IOException { + runCustomViolationTest( + ClusterStateCustomViolation.class, + getClass(), + ClusterState.Custom.class, + XPackPlugin.XPackClusterStateCustom.class); + } + + public void testClusterStateCustom() throws IOException { + runCustomTest(XPackClusterStateCustom.class, getClass(), ClusterState.Custom.class, XPackPlugin.XPackClusterStateCustom.class); + } + + public void testClusterStateCustomMarkerInterface() throws IOException { + // marker interfaces do not implement the marker interface but should not fail the feature aware check + runCustomTest( + XPackPlugin.XPackClusterStateCustom.class, + XPackPlugin.class, + ClusterState.Custom.class, + XPackPlugin.XPackClusterStateCustom.class); + } + + public void testMetaDataCustomViolation() throws IOException { + runCustomViolationTest(MetaDataCustomViolation.class, getClass(), MetaData.Custom.class, XPackPlugin.XPackMetaDataCustom.class); + } + + public void testMetaDataCustom() throws IOException { + runCustomTest(XPackMetaDataCustom.class, getClass(), MetaData.Custom.class, XPackPlugin.XPackMetaDataCustom.class); + } + + public void testMetaDataCustomMarkerInterface() throws IOException { + // marker interfaces do not implement the marker interface but should not fail the feature aware check + runCustomTest( + XPackPlugin.XPackMetaDataCustom.class, + XPackPlugin.class, + MetaData.Custom.class, + XPackPlugin.XPackMetaDataCustom.class); + } + + public void testPersistentTaskParamsViolation() throws IOException { + runCustomViolationTest( + PersistentTaskParamsViolation.class, + getClass(), + PersistentTaskParams.class, + XPackPlugin.XPackPersistentTaskParams.class); + } + + public void testPersistentTaskParams() throws IOException { + runCustomTest(XPackPersistentTaskParams.class, getClass(), PersistentTaskParams.class, XPackPlugin.XPackPersistentTaskParams.class); + } + + public void testPersistentTaskParamsMarkerInterface() throws IOException { + // marker interfaces do not implement the marker interface but should not fail the feature aware check + runCustomTest( + XPackPlugin.XPackPersistentTaskParams.class, + XPackPlugin.class, + PersistentTaskParams.class, + XPackPlugin.XPackPersistentTaskParams.class); + } + + abstract class ClusterStateCustomFeatureAware implements ClusterState.Custom { + + private final String writeableName; + + ClusterStateCustomFeatureAware(final String writeableName) { + this.writeableName = writeableName; + } + + @Override + public Diff diff(ClusterState.Custom previousState) { + return null; + } + + @Override + public String getWriteableName() { + return writeableName; + } + + @Override + public Version getMinimalSupportedVersion() { + return Version.CURRENT.minimumCompatibilityVersion(); + } + + @Override + public void writeTo(final StreamOutput out) throws IOException { + + } + + @Override + public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { + return builder; + } + + } + + class ClusterStateCustomViolation extends ClusterStateCustomFeatureAware { + + ClusterStateCustomViolation() { + super("cluster_state_custom_violation"); + } + } + + class XPackClusterStateCustom extends ClusterStateCustomFeatureAware implements XPackPlugin.XPackClusterStateCustom { + + XPackClusterStateCustom() { + super("x_pack_cluster_state_custom"); + } + + } + + abstract class MetaDataCustomFeatureAware implements MetaData.Custom { + + private final String writeableName; + + MetaDataCustomFeatureAware(final String writeableName) { + this.writeableName = writeableName; + } + + @Override + public EnumSet context() { + return MetaData.ALL_CONTEXTS; + } + + @Override + public Diff diff(MetaData.Custom previousState) { + return null; + } + + @Override + public String getWriteableName() { + return writeableName; + } + + @Override + public Version getMinimalSupportedVersion() { + return Version.CURRENT.minimumCompatibilityVersion(); + } + + @Override + public void writeTo(final StreamOutput out) throws IOException { + + } + + @Override + public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { + return builder; + } + + } + + class MetaDataCustomViolation extends MetaDataCustomFeatureAware { + + MetaDataCustomViolation() { + super("meta_data_custom_violation"); + } + + } + + class XPackMetaDataCustom extends MetaDataCustomFeatureAware implements XPackPlugin.XPackMetaDataCustom { + + XPackMetaDataCustom() { + super("x_pack_meta_data_custom"); + } + + } + + abstract class PersistentTaskParamsFeatureAware implements PersistentTaskParams { + + private final String writeableName; + + PersistentTaskParamsFeatureAware(final String writeableName) { + this.writeableName = writeableName; + } + + @Override + public String getWriteableName() { + return writeableName; + } + + @Override + public Version getMinimalSupportedVersion() { + return Version.CURRENT.minimumCompatibilityVersion(); + } + + @Override + public void writeTo(final StreamOutput out) throws IOException { + + } + + @Override + public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException { + return builder; + } + + } + + class PersistentTaskParamsViolation extends PersistentTaskParamsFeatureAware { + + PersistentTaskParamsViolation() { + super("persistent_task_params_violation"); + } + + } + + class XPackPersistentTaskParams extends PersistentTaskParamsFeatureAware implements XPackPlugin.XPackPersistentTaskParams { + + XPackPersistentTaskParams() { + super("x_pack_persistent_task_params"); + } + + } + + private class FeatureAwareViolationConsumer implements Consumer { + + private final AtomicBoolean called = new AtomicBoolean(); + private final String name; + private final String interfaceName; + private final String expectedInterfaceName; + + FeatureAwareViolationConsumer(final String name, final String interfaceName, final String expectedInterfaceName) { + this.name = name; + this.interfaceName = interfaceName; + this.expectedInterfaceName = expectedInterfaceName; + } + + @Override + public void accept(final org.elasticsearch.xpack.test.feature_aware.FeatureAwareCheck.FeatureAwareViolation featureAwareViolation) { + called.set(true); + assertThat(featureAwareViolation.name, equalTo(name)); + assertThat(featureAwareViolation.interfaceName, equalTo(interfaceName)); + assertThat(featureAwareViolation.expectedInterfaceName, equalTo(expectedInterfaceName)); + } + + } + + /** + * Runs a test on an actual class implementing a custom interface and not the expected marker interface. + * + * @param clazz the custom implementation + * @param outerClazz the outer class to load the custom implementation relative to + * @param interfaceClazz the custom + * @param expectedInterfaceClazz the marker interface + * @throws IOException if an I/O error occurs reading the class + */ + private void runCustomViolationTest( + final Class clazz, + final Class outerClazz, + final Class interfaceClazz, + final Class expectedInterfaceClazz) throws IOException { + runTest(clazz, outerClazz, interfaceClazz, expectedInterfaceClazz, true); + } + + /** + * Runs a test on an actual class implementing a custom interface and the expected marker interface. + * + * @param clazz the custom implementation + * @param outerClazz the outer class to load the custom implementation relative to + * @param interfaceClazz the custom + * @param expectedInterfaceClazz the marker interface + * @throws IOException if an I/O error occurs reading the class + */ + private void runCustomTest( + final Class clazz, + final Class outerClazz, + final Class interfaceClazz, + final Class expectedInterfaceClazz) throws IOException { + runTest(clazz, outerClazz, interfaceClazz, expectedInterfaceClazz, false); + } + + /** + * Runs a test on an actual class implementing a custom interface and should implement the expected marker interface if and only if + * the specified violation parameter is false. + * + * @param clazz the custom implementation + * @param outerClazz the outer class to load the custom implementation relative to + * @param interfaceClazz the custom + * @param expectedInterfaceClazz the marker interface + * @param violation whether or not the actual class is expected to fail the feature aware check + * @throws IOException if an I/O error occurs reading the class + */ + private void runTest( + final Class clazz, + final Class outerClazz, + final Class interfaceClazz, + final Class expectedInterfaceClazz, + final boolean violation) throws IOException { + final String name = clazz.getName(); + final FeatureAwareViolationConsumer callback = + new FeatureAwareViolationConsumer( + FeatureAwareCheck.formatClassName(clazz), + FeatureAwareCheck.formatClassName(interfaceClazz), + FeatureAwareCheck.formatClassName(expectedInterfaceClazz)); + FeatureAwareCheck.checkClass(outerClazz.getResourceAsStream(name.substring(1 + name.lastIndexOf(".")) + ".class"), callback); + assertThat(callback.called.get(), equalTo(violation)); + } + +}