From 2523a54b0f42f5db039771b71f9ce1ed2b28cb6f Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Wed, 1 Nov 2023 11:17:31 -0400 Subject: [PATCH] [BWC and API enforcement] Introduce checks for enforcing the API restrictions Signed-off-by: Andriy Redko --- .../processor/ApiAnnotationProcessor.java | 341 ++++++++++++++++++ .../annotation/processor/package-info.java | 15 + .../javax.annotation.processing.Processor | 12 + .../ApiAnnotationProcessorTests.java | 52 +++ .../annotation/processor/CompilerSupport.java | 128 +++++++ .../PublicApiMethodArgumentNotAnnotated.java | 19 + server/build.gradle | 4 +- .../bootstrap/test-framework.policy | 6 + 8 files changed, 576 insertions(+), 1 deletion(-) create mode 100644 libs/common/src/main/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessor.java create mode 100644 libs/common/src/main/java/org/opensearch/common/annotation/processor/package-info.java create mode 100644 libs/common/src/main/resources/META-INF/services/javax.annotation.processing.Processor create mode 100644 libs/common/src/test/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessorTests.java create mode 100644 libs/common/src/test/java/org/opensearch/common/annotation/processor/CompilerSupport.java create mode 100644 libs/common/src/test/resources/org/opensearch/common/annotation/processor/PublicApiMethodArgumentNotAnnotated.java diff --git a/libs/common/src/main/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessor.java b/libs/common/src/main/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessor.java new file mode 100644 index 0000000000000..1cea54a221c0b --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessor.java @@ -0,0 +1,341 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.annotation.processor; + +import org.opensearch.common.Nullable; +import org.opensearch.common.annotation.DeprecatedApi; +import org.opensearch.common.annotation.ExperimentalApi; +import org.opensearch.common.annotation.InternalApi; +import org.opensearch.common.annotation.PublicApi; + +import javax.annotation.processing.AbstractProcessor; +import javax.annotation.processing.RoundEnvironment; +import javax.annotation.processing.SupportedAnnotationTypes; +import javax.lang.model.AnnotatedConstruct; +import javax.lang.model.SourceVersion; +import javax.lang.model.element.AnnotationMirror; +import javax.lang.model.element.Element; +import javax.lang.model.element.ElementKind; +import javax.lang.model.element.ExecutableElement; +import javax.lang.model.element.Modifier; +import javax.lang.model.element.PackageElement; +import javax.lang.model.element.TypeElement; +import javax.lang.model.element.TypeParameterElement; +import javax.lang.model.element.VariableElement; +import javax.lang.model.type.ArrayType; +import javax.lang.model.type.DeclaredType; +import javax.lang.model.type.ReferenceType; +import javax.lang.model.type.TypeMirror; +import javax.lang.model.type.TypeVariable; +import javax.lang.model.type.WildcardType; +import javax.tools.Diagnostic.Kind; + +import java.util.HashSet; +import java.util.Set; + +/** + * The annotation processor for API related annotations: {@link DeprecatedApi}, {@link ExperimentalApi}, + * {@link InternalApi} and {@link PublicApi}. + *

+ * The checks are built on top of the following rules: + *

+ */ +@InternalApi +@SupportedAnnotationTypes("org.opensearch.common.annotation.*") +public class ApiAnnotationProcessor extends AbstractProcessor { + private static final String OPENSEARCH_PACKAGE = "org.opensearch"; + + private final Set reported = new HashSet<>(); + private final Set processed = new HashSet<>(); + + @Override + public SourceVersion getSupportedSourceVersion() { + return SourceVersion.latest(); + } + + @Override + public boolean process(Set annotations, RoundEnvironment round) { + processingEnv.getMessager().printMessage(Kind.NOTE, "Processing OpenSearch Api annotations"); + + final Set elements = round.getElementsAnnotatedWithAny( + Set.of(PublicApi.class, ExperimentalApi.class, DeprecatedApi.class) + ); + + for (var element : elements) { + if (!checkPackage(element)) { + continue; + } + + // Skip all not-public elements + if (!element.getModifiers().contains(Modifier.PUBLIC)) { + continue; + } + + if (element instanceof TypeElement) { + process((TypeElement) element); + } + } + + return false; + } + + /** + * Check top level executable element + * @param executable top level executable element + */ + private void process(ExecutableElement executable) { + if (!inspectable(executable)) { + return; + } + + // The executable element should not be internal (unless constructor for injectable core component) + checkNotInternal(null, executable); + + // Process method return types + final TypeMirror returnType = executable.getReturnType(); + if (returnType instanceof ReferenceType) { + process(executable, (ReferenceType) returnType); + } + + // Process method thrown types + for (final TypeMirror thrownType : executable.getThrownTypes()) { + if (thrownType instanceof ReferenceType) { + process(executable, (ReferenceType) thrownType); + } + } + + // Process method type parameters + for (final TypeParameterElement typeParameter : executable.getTypeParameters()) { + for (final TypeMirror boundType : typeParameter.getBounds()) { + if (boundType instanceof ReferenceType) { + process(executable, (ReferenceType) boundType); + } + } + } + + // Process method arguments + for (final VariableElement parameter : executable.getParameters()) { + final TypeMirror parameterType = parameter.asType(); + if (parameterType instanceof ReferenceType) { + process(executable, (ReferenceType) parameterType); + } + } + } + + /** + * Check wildcard type bounds referred by an element + * @param executable element + * @param type wildcard type + */ + private void process(ExecutableElement executable, WildcardType type) { + if (type.getExtendsBound() instanceof ReferenceType) { + process(executable, (ReferenceType) type.getExtendsBound()); + } + + if (type.getSuperBound() instanceof ReferenceType) { + process(executable, (ReferenceType) type.getSuperBound()); + } + } + + /** + * Check reference type bounds referred by an executable element + * @param executable executable element + * @param ref reference type + */ + private void process(ExecutableElement executable, ReferenceType ref) { + // The element has been processed already + if (processed.add(ref) == false) { + return; + } + + if (ref instanceof DeclaredType) { + final DeclaredType declaredType = (DeclaredType) ref; + + final Element element = declaredType.asElement(); + if (inspectable(element)) { + checkNotInternal(executable.getEnclosingElement(), element); + checkPublic(executable.getEnclosingElement(), element); + } + + for (final TypeMirror type : declaredType.getTypeArguments()) { + if (type instanceof ReferenceType) { + process(executable, (ReferenceType) type); + } else if (type instanceof WildcardType) { + process(executable, (WildcardType) type); + } + } + } else if (ref instanceof ArrayType) { + final TypeMirror componentType = ((ArrayType) ref).getComponentType(); + if (componentType instanceof ReferenceType) { + process(executable, (ReferenceType) componentType); + } + } else if (ref instanceof TypeVariable) { + final TypeVariable typeVariable = (TypeVariable) ref; + if (typeVariable.getUpperBound() instanceof ReferenceType) { + process(executable, (ReferenceType) typeVariable.getUpperBound()); + } + if (typeVariable.getLowerBound() instanceof ReferenceType) { + process(executable, (ReferenceType) typeVariable.getLowerBound()); + } + } + + // Check this elements annotations + for (final AnnotationMirror annotation : ref.getAnnotationMirrors()) { + final Element element = annotation.getAnnotationType().asElement(); + if (inspectable(element)) { + checkNotInternal(executable.getEnclosingElement(), element); + checkPublic(executable.getEnclosingElement(), element); + } + } + } + + /** + * Check if a particular executable element should be inspected or not + * @param executable executable element to inspect + * @return {@code true} if a particular executable element should be inspected, {@code false} otherwise + */ + private boolean inspectable(ExecutableElement executable) { + // The constructors for public APIs could use non-public APIs when those are supposed to be only + // consumed (not instantiated) by external consumers. + return executable.getKind() != ElementKind.CONSTRUCTOR && executable.getModifiers().contains(Modifier.PUBLIC); + } + + /** + * Check if a particular element should be inspected or not + * @param element element to inspect + * @return {@code true} if a particular element should be inspected, {@code false} otherwise + */ + private boolean inspectable(Element element) { + final PackageElement pckg = processingEnv.getElementUtils().getPackageOf(element); + return pckg.getQualifiedName().toString().startsWith(OPENSEARCH_PACKAGE); + } + + /** + * Check if a particular element belongs to OpenSeach managed packages + * @param element element to inspect + * @return {@code true} if a particular element belongs to OpenSeach managed packages, {@code false} otherwise + */ + private boolean checkPackage(Element element) { + // The element was reported already + if (reported.contains(element)) { + return false; + } + + final PackageElement pckg = processingEnv.getElementUtils().getPackageOf(element); + final boolean belongsToOpenSearch = pckg.getQualifiedName().toString().startsWith(OPENSEARCH_PACKAGE); + + if (!belongsToOpenSearch) { + reported.add(element); + + processingEnv.getMessager() + .printMessage( + Kind.ERROR, + "The type " + + element + + " is not residing in " + + OPENSEARCH_PACKAGE + + ".* package " + + "and should not be annotated as OpenSearch APIs." + ); + } + + return belongsToOpenSearch; + } + + /** + * Check the fields, methods, constructors, and member types that are directly + * declared in this class or interface. + * @param type class or interface + */ + private void process(Element type) { + // Check the fields, methods, constructors, and member types that are directly + // declared in this class or interface. + for (final Element element : type.getEnclosedElements()) { + // Skip all not-public elements + if (!type.getModifiers().contains(Modifier.PUBLIC)) { + continue; + } + + if (element instanceof ExecutableElement) { + process((ExecutableElement) element); + } + } + } + + /** + * Check if element is public and annotated with {@link PublicApi}, {@link DeprecatedApi} or {@link ExperimentalApi} + * @param referencedBy the referrer for the element + * @param element element to check + */ + private void checkPublic(@Nullable Element referencedBy, final Element element) { + // The element was reported already + if (reported.contains(element)) { + return; + } + + if (!element.getModifiers().contains(Modifier.PUBLIC)) { + reported.add(element); + + processingEnv.getMessager() + .printMessage( + Kind.ERROR, + "The element " + + element + + " is part of the public APIs but does not have public visibility" + + ((referencedBy != null) ? " (referenced by " + referencedBy + ") " : "") + ); + } + + if (element.getAnnotation(PublicApi.class) == null + && element.getAnnotation(ExperimentalApi.class) == null + && element.getAnnotation(DeprecatedApi.class) == null) { + reported.add(element); + + processingEnv.getMessager() + .printMessage( + Kind.ERROR, + "The element " + + element + + " is part of the public APIs but is not maked as @PublicApi, @ExperimentalApi or @DeprecatedApi" + + ((referencedBy != null) ? " (referenced by " + referencedBy + ") " : "") + ); + } + } + + /** + * Check if element is not annotated with {@link InternalApi} + * @param referencedBy the referrer for the element + * @param element element to check + */ + private void checkNotInternal(@Nullable Element referencedBy, final Element element) { + // The element was reported already + if (reported.contains(element)) { + return; + } + + if (element.getAnnotation(InternalApi.class) != null) { + reported.add(element); + + processingEnv.getMessager() + .printMessage( + Kind.ERROR, + "The element " + + element + + " is part of the public APIs but is marked as @InternalApi" + + ((referencedBy != null) ? " (referenced by " + referencedBy + ") " : "") + ); + } + } +} diff --git a/libs/common/src/main/java/org/opensearch/common/annotation/processor/package-info.java b/libs/common/src/main/java/org/opensearch/common/annotation/processor/package-info.java new file mode 100644 index 0000000000000..fa23e4a7addce --- /dev/null +++ b/libs/common/src/main/java/org/opensearch/common/annotation/processor/package-info.java @@ -0,0 +1,15 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +/** + * Classes related yo OpenSearch API annotation processing + * + * @opensearch.internal + */ +@org.opensearch.common.annotation.InternalApi +package org.opensearch.common.annotation.processor; diff --git a/libs/common/src/main/resources/META-INF/services/javax.annotation.processing.Processor b/libs/common/src/main/resources/META-INF/services/javax.annotation.processing.Processor new file mode 100644 index 0000000000000..c4e4dfed864f2 --- /dev/null +++ b/libs/common/src/main/resources/META-INF/services/javax.annotation.processing.Processor @@ -0,0 +1,12 @@ +# +# SPDX-License-Identifier: Apache-2.0 +# +# The OpenSearch Contributors require contributions made to +# this file be licensed under the Apache-2.0 license or a +# compatible open source license. +# +# Modifications Copyright OpenSearch Contributors. See +# GitHub history for details. +# + +org.opensearch.common.annotation.processor.ApiAnnotationProcessor \ No newline at end of file diff --git a/libs/common/src/test/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessorTests.java b/libs/common/src/test/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessorTests.java new file mode 100644 index 0000000000000..74d62c0fe35c0 --- /dev/null +++ b/libs/common/src/test/java/org/opensearch/common/annotation/processor/ApiAnnotationProcessorTests.java @@ -0,0 +1,52 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.annotation.processor; + +import org.opensearch.test.OpenSearchTestCase; + +import javax.tools.Diagnostic; + +import java.io.IOException; +import java.net.URISyntaxException; + +import static org.opensearch.common.annotation.processor.CompilerSupport.HasDiagnostic.matching; +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.CoreMatchers.hasItem; +import static org.hamcrest.CoreMatchers.instanceOf; + +public class ApiAnnotationProcessorTests extends OpenSearchTestCase implements CompilerSupport { + public void testPublicApiMethodArgumentNotAnnotated() throws IOException, URISyntaxException { + final CompilerResult result = compile("PublicApiMethodArgumentNotAnnotated.java"); + assertThat(result, instanceOf(Failure.class)); + + assertThat( + ((Failure) result).diagnotics(), + hasItem( + matching( + Diagnostic.Kind.ERROR, + containsString( + "The element org.opensearch.common.annotation.processor.NotAnnotatedPackagePrivate is part of the public APIs but does not have public visibility (referenced by org.opensearch.common.annotation.processor.PublicApiMethodArgumentNotAnnotated)" + ) + ) + ) + ); + + assertThat( + ((Failure) result).diagnotics(), + hasItem( + matching( + Diagnostic.Kind.ERROR, + containsString( + "The element org.opensearch.common.annotation.processor.NotAnnotatedPackagePrivate is part of the public APIs but is not maked as @PublicApi, @ExperimentalApi or @DeprecatedApi (referenced by org.opensearch.common.annotation.processor.PublicApiMethodArgumentNotAnnotated)" + ) + ) + ) + ); + } +} diff --git a/libs/common/src/test/java/org/opensearch/common/annotation/processor/CompilerSupport.java b/libs/common/src/test/java/org/opensearch/common/annotation/processor/CompilerSupport.java new file mode 100644 index 0000000000000..d78910b16d879 --- /dev/null +++ b/libs/common/src/test/java/org/opensearch/common/annotation/processor/CompilerSupport.java @@ -0,0 +1,128 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.annotation.processor; + +import org.hamcrest.Description; +import org.hamcrest.Matcher; +import org.hamcrest.TypeSafeMatcher; + +import javax.tools.Diagnostic; +import javax.tools.DiagnosticCollector; +import javax.tools.JavaCompiler; +import javax.tools.JavaCompiler.CompilationTask; +import javax.tools.JavaFileObject; +import javax.tools.JavaFileObject.Kind; +import javax.tools.SimpleJavaFileObject; +import javax.tools.StandardJavaFileManager; +import javax.tools.ToolProvider; + +import java.io.IOException; +import java.io.InputStream; +import java.io.StringWriter; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.URL; +import java.nio.charset.StandardCharsets; +import java.security.AccessController; +import java.security.PrivilegedAction; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.Locale; +import java.util.stream.Collectors; + +interface CompilerSupport { + default CompilerResult compile(String name) throws IOException, URISyntaxException { + return compile(Collections.singletonList(name)); + } + + default CompilerResult compile(Collection names) throws IOException, URISyntaxException { + final JavaCompiler compiler = ToolProvider.getSystemJavaCompiler(); + final DiagnosticCollector collector = new DiagnosticCollector<>(); + + try (StringWriter out = new StringWriter()) { + final StandardJavaFileManager fileManager = compiler.getStandardFileManager(collector, null, null); + final List files = names.stream().map(CompilerSupport::asSource).collect(Collectors.toList()); + + final CompilationTask task = compiler.getTask(out, fileManager, collector, null, null, files); + task.setProcessors(Collections.singleton(new ApiAnnotationProcessor())); + + if (AccessController.doPrivileged((PrivilegedAction) () -> task.call())) { + return new Success(); + } else { + return new Failure(collector.getDiagnostics()); + } + } + } + + private static JavaFileObject asSource(String name) { + final String resource = "/" + ApiAnnotationProcessorTests.class.getPackageName().replaceAll("[.]", "/") + "/" + name; + final URL source = ApiAnnotationProcessorTests.class.getResource(resource); + + return new SimpleJavaFileObject(URI.create(source.toExternalForm()), Kind.SOURCE) { + @Override + public CharSequence getCharContent(boolean ignoreEncodingErrors) throws IOException { + try (final InputStream in = ApiAnnotationProcessorTests.class.getResourceAsStream(resource)) { + return new String(in.readAllBytes(), StandardCharsets.UTF_8); + } + } + }; + } + + class CompilerResult {} + + class Success extends CompilerResult { + + } + + class Failure extends CompilerResult { + private final List> diagnotics; + + Failure(List> diagnotics) { + this.diagnotics = diagnotics; + } + + List> diagnotics() { + return diagnotics; + } + } + + class HasDiagnostic extends TypeSafeMatcher> { + private final Diagnostic.Kind kind; + private final Matcher matcher; + + HasDiagnostic(final Diagnostic.Kind kind, final Matcher matcher) { + this.kind = kind; + this.matcher = matcher; + } + + @Override + public void describeTo(Description description) { + description.appendText("diagnostic with kind ").appendValue(kind).appendText(" "); + + if (matcher != null) { + description.appendText(" and message "); + matcher.describeTo(description); + } + } + + @Override + protected boolean matchesSafely(Diagnostic item) { + if (!kind.equals(item.getKind())) { + return false; + } else { + return matcher.matches(item.getMessage(Locale.ROOT)); + } + } + + public static HasDiagnostic matching(final Diagnostic.Kind kind, final Matcher matcher) { + return new HasDiagnostic(kind, matcher); + } + } +} diff --git a/libs/common/src/test/resources/org/opensearch/common/annotation/processor/PublicApiMethodArgumentNotAnnotated.java b/libs/common/src/test/resources/org/opensearch/common/annotation/processor/PublicApiMethodArgumentNotAnnotated.java new file mode 100644 index 0000000000000..7383fb5ff83bc --- /dev/null +++ b/libs/common/src/test/resources/org/opensearch/common/annotation/processor/PublicApiMethodArgumentNotAnnotated.java @@ -0,0 +1,19 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.common.annotation.processor; + +import org.opensearch.common.annotation.PublicApi; + +@PublicApi(since = "1.0.0") +public class PublicApiMethodArgumentNotAnnotated { + public void method(NotAnnotatedPackagePrivate arg) {} +} + +// The public API exposes this class through public method argument, it should be annotated and be public +class NotAnnotatedPackagePrivate {} diff --git a/server/build.gradle b/server/build.gradle index fa8a44ef6fc94..0aed227fe5b1e 100644 --- a/server/build.gradle +++ b/server/build.gradle @@ -147,6 +147,7 @@ dependencies { api "org.apache.logging.log4j:log4j-jul:${versions.log4j}" api "org.apache.logging.log4j:log4j-core:${versions.log4j}", optional annotationProcessor "org.apache.logging.log4j:log4j-core:${versions.log4j}" + annotationProcessor project(':libs:opensearch-common') // jna api "net.java.dev.jna:jna:${versions.jna}" @@ -178,7 +179,8 @@ tasks.withType(JavaCompile).configureEach { } compileJava { - options.compilerArgs += ['-processor', 'org.apache.logging.log4j.core.config.plugins.processor.PluginProcessor'] + options.compilerArgs += ['-processor', 'org.apache.logging.log4j.core.config.plugins.processor.PluginProcessor'/*, + '-processor', 'org.opensearch.common.annotation.processor.ApiAnnotationProcessor'*/] } tasks.named("internalClusterTest").configure { diff --git a/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy b/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy index 0abfd7ef22ae7..7bb7d62c4c5df 100644 --- a/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy +++ b/server/src/main/resources/org/opensearch/bootstrap/test-framework.policy @@ -151,6 +151,12 @@ grant codeBase "file:${gradle.worker.jar}" { }; grant { + // allow to use JVM tooling (Java Compiler for example) in tests + permission java.io.FilePermission "${java.home}/lib/*", "read"; + permission java.io.FilePermission "${java.home}/lib/modules/*", "read"; + permission java.lang.RuntimePermission "accessSystemModules"; + permission java.lang.RuntimePermission "accessDeclaredMembers"; + // since the gradle test worker jar is on the test classpath, our tests should be able to read it permission java.io.FilePermission "${gradle.worker.jar}", "read"; permission java.lang.RuntimePermission "accessDeclaredMembers";