Skip to content

Commit

Permalink
Merge pull request #1442 from newrelic/weave-violation-filter
Browse files Browse the repository at this point in the history
Add weave violation filter support. Resolves #1197
  • Loading branch information
jtduffy authored Aug 18, 2023
2 parents 566ce65 + 53dbcfe commit 3f62795
Show file tree
Hide file tree
Showing 6 changed files with 232 additions and 9 deletions.
3 changes: 2 additions & 1 deletion instrumentation/spring-4.3.0/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ dependencies {

jar {
manifest { attributes 'Implementation-Title': 'com.newrelic.instrumentation.spring-4.3.0',
'Implementation-Title-Alias': 'spring_annotations' }
'Implementation-Title-Alias': 'spring_annotations',
'Weave-Violation-Filter': 'METHOD_MISSING_REQUIRED_ANNOTATIONS,CLASS_MISSING_REQUIRED_ANNOTATIONS' }
}

verifyInstrumentation {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package com.newrelic.weave;

import com.google.common.collect.Queues;
import com.newrelic.weave.violation.WeaveViolation;
import com.newrelic.weave.violation.WeaveViolationType;

import java.util.Collection;
import java.util.EnumSet;
import java.util.Queue;

/**
* A class used to filter specific violations from being added to {@link com.newrelic.weave.weavepackage.PackageValidationResult} instances.
* Instances of this class are created and stored with each {@link com.newrelic.weave.weavepackage.WeavePackageConfig} object (if
* filters are configured for the instrumentation package).
*/
public class WeaveViolationFilter {
private final Collection<WeaveViolationType> typesToFilter = EnumSet.noneOf(WeaveViolationType.class);

private final String weavePackage;

public WeaveViolationFilter(String weavePackage, Collection<WeaveViolationType> types) {
this.typesToFilter.addAll(types);
this.weavePackage = weavePackage;
}

/**
* Remove ignorable {@link WeaveViolation} instances from the supplied collection of violations.
*
* @param violations the collection of WeaveViolations to apply the filter to
* @return a filtered collection of WeaveViolations
*/
public Collection<WeaveViolation> filterViolationCollection(Collection<WeaveViolation> violations) {
Queue<WeaveViolation> newViolationCollection = Queues.newConcurrentLinkedQueue();
for (WeaveViolation violation : violations) {
if (!shouldIgnoreViolation(violation)) {
newViolationCollection.add(violation);
}
}

return newViolationCollection;
}

/**
* Return true if the supplied {@link WeaveViolation} instance should be ignored based on the filter
*
* @param violation the WeaveViolation to check
* @return true if this WeaveViolation should be filtered
*/
public boolean shouldIgnoreViolation(WeaveViolation violation) {
return typesToFilter.contains(violation.getType());
}

/**
* Return the weave package this filter belongs to
*
* @return the weave package name
*/
public String getWeavePackage() {
return this.weavePackage;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import com.newrelic.weave.MethodProcessors;
import com.newrelic.weave.PreparedExtension;
import com.newrelic.weave.PreparedMatch;
import com.newrelic.weave.WeaveViolationFilter;
import com.newrelic.weave.utils.ClassCache;
import com.newrelic.weave.utils.ClassInformation;
import com.newrelic.weave.utils.SynchronizedClassNode;
Expand Down Expand Up @@ -57,6 +58,7 @@ public class PackageValidationResult {
private final Map<String, PreparedMatch> exactMatches = new ConcurrentHashMap<>();
private final Map<String, PreparedMatch> baseMatches = new ConcurrentHashMap<>();
private final WeavePackage weavePackage;
private final WeaveViolationFilter weaveViolationFilter;

private ClassNode errorHandler;

Expand All @@ -76,6 +78,7 @@ public PackageValidationResult(WeavePackage weavePackage, ClassCache cache, Coll
this.allAnnotationClasses.putAll(allAnnotationClasses);
this.baseAnnotationClasses.putAll(baseAnnotationClasses);
this.allMethodAnnotationClasses.putAll(allMethodAnnotationClasses);
this.weaveViolationFilter = this.weavePackage.getConfig().getWeaveViolationFilter();

this.skipIfPresent(skipIfPresentNames, cache);

Expand Down Expand Up @@ -113,6 +116,7 @@ public PackageValidationResult(WeavePackage weavePackage, ClassCache cache, Coll
public PackageValidationResult(WeavePackage weavePackage, ClassCache cache, Set<String> requiredClasses,
Set<String> illegalClasses) {
this.weavePackage = weavePackage;
this.weaveViolationFilter = this.weavePackage.getConfig().getWeaveViolationFilter();
for (String requiredClass : requiredClasses) {
if (!cache.hasClassResource(requiredClass)) {
violations.add(new WeaveViolation(WeaveViolationType.MISSING_ORIGINAL_BYTECODE, requiredClass));
Expand All @@ -135,6 +139,7 @@ public PackageValidationResult(WeavePackage weavePackage, ClassCache cache, Set<
*/
PackageValidationResult(WeavePackage weavePackage, Queue<WeaveViolation> packageViolations) {
this.weavePackage = weavePackage;
this.weaveViolationFilter = this.weavePackage.getConfig().getWeaveViolationFilter();
this.violations.addAll(packageViolations);
}

Expand Down Expand Up @@ -186,7 +191,7 @@ private void processMatches(ClassCache classCache, Map<String, ClassNode> classN
}
}

private void buildResults(ClassCache classCache, ClassNode originalClassNode, String weaveClassName,
private boolean buildResults(ClassCache classCache, ClassNode originalClassNode, String weaveClassName,
ClassNode weaveNode, Map<String, PreparedMatch> results, boolean isBaseMatch,
Set<String> requiredClassAnnotations, Set<String> requiredMethodAnnotations, ClassNode errorHandler,
Map<String, byte[]> annotationProxyClasses) throws IOException {
Expand All @@ -197,7 +202,6 @@ private void buildResults(ClassCache classCache, ClassNode originalClassNode, St
// exit sooner to prevent further processing (avoids NPE when no default constructor exists)
if (match.isFatalWeaveViolation()) {
violations.addAll(match.getViolations());
return;
}

PreparedMatch prepared = PreparedMatch.prepare(match, errorHandler,
Expand All @@ -221,7 +225,11 @@ private void buildResults(ClassCache classCache, ClassNode originalClassNode, St
annotationProxyClasses.put(annotationProxyClass.getKey(),
WeaveUtils.convertToClassBytes(annotationProxyClass.getValue(), classCache));
}
violations.addAll(match.getViolations());

violations.addAll((this.weaveViolationFilter == null ? match.getViolations() : this.weaveViolationFilter.filterViolationCollection(match.getViolations())));

//Return true if this match had violations, even if they were filtered out of the PackageValidationResult violation list
return match.getViolations().size() > 0;
}

/**
Expand Down Expand Up @@ -456,12 +464,12 @@ private ClassNode getAnnotationMatchComposite(ClassNode targetNode,
try {
boolean isInterfaceMatch = WeaveUtils.isWeaveWithAnnotationInterfaceMatch(weaveNode);
Map<String, PreparedMatch> results = new HashMap<>();
buildResults(cache, targetNode, weaveNode.name, weaveNode, results, isInterfaceMatch,
boolean targetNodeContainsViolations = buildResults(cache, targetNode, weaveNode.name, weaveNode, results, isInterfaceMatch,
weavePackage.getRequiredAnnotationClassesForAnnotationWeave(weaveNode.name),
weavePackage.getRequiredAnnotationClassesForMethodAnnotationWeave(weaveNode.name),
errorHandler, annotationProxyClasses);

if (!violations.isEmpty()) {
if (targetNodeContainsViolations) {
return composite;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,14 @@

package com.newrelic.weave.weavepackage;

import com.newrelic.weave.WeaveViolationFilter;
import com.newrelic.weave.violation.WeaveViolationType;
import org.objectweb.asm.tree.ClassNode;

import java.io.IOException;
import java.lang.instrument.Instrumentation;
import java.net.URL;
import java.util.EnumSet;
import java.util.jar.Attributes;
import java.util.jar.JarInputStream;
import java.util.regex.Matcher;
Expand All @@ -37,6 +40,7 @@ public static class Builder {
private boolean enabled = true;
private long priority = 0L;
private boolean custom = false;
private WeaveViolationFilter weaveViolationFilter = null;
private ClassNode errorHandleClassNode = ErrorTrapHandler.NO_ERROR_TRAP_HANDLER;
private WeavePreprocessor preprocessor = WeavePreprocessor.NO_PREPROCESSOR;
private WeavePostprocessor postprocessor = WeavePostprocessor.NO_POSTPROCESSOR;
Expand Down Expand Up @@ -152,6 +156,41 @@ public Builder priority(long priority) {
return this;
}

/**
* Using the supplied {@code violationFilterToken}, create a {@link WeaveViolationFilter} instance to be stored
* as part of the config.
* <p>
* The filter is configured by adding a manifest attribute of {@code Weave-Violation-Filter} with a value
* of {@link WeaveViolationType} strings, seperated by a comma. For example:
* <pre>
* Weave-Violation-Filter: METHOD_MISSING_REQUIRED_ANNOTATIONS,CLASS_MISSING_REQUIRED_ANNOTATIONS
* </pre>
*
* Weave violations that match any of the configured types will be ignored and not added to the
* {@link PackageValidationResult}'s violation list. If no types are configured in the manifest,
* no filter will be created or applied.
*
* @param violationFilterToken the comma seperated String of WeaveViolationTypes to ignore
* @return builder with updated state
*/
public Builder weaveViolationFilters(String violationFilterToken) {
if (violationFilterToken != null) {
String [] filterTokens = violationFilterToken.split(",");
EnumSet<WeaveViolationType> types = EnumSet.noneOf(WeaveViolationType.class);
for (String type : filterTokens) {
try {
types.add(WeaveViolationType.valueOf(type));
} catch (IllegalArgumentException ignored) {}
}

if (types.size() > 0 ) {
this.weaveViolationFilter = new WeaveViolationFilter(this.name, types);
}
}

return this;
}

/**
* Set whether the package is "custom".
*
Expand Down Expand Up @@ -276,7 +315,10 @@ public Builder jarInputStream(JarInputStream jarStream) throws Exception {
String priorityS = mainAttributes.getValue("Priority");
long priority = priorityS == null ? 0 : Long.parseLong(priorityS);

return this.name(name).alias(alias).vendorId(vendorId).version(version).enabled(enabled).priority(priority);
String violationFilterToken = mainAttributes.getValue("Weave-Violation-Filter");

return this.name(name).alias(alias).vendorId(vendorId).version(version).enabled(enabled).priority(priority)
.weaveViolationFilters(violationFilterToken);
}

/**
Expand All @@ -290,7 +332,7 @@ public WeavePackageConfig build() {
throw new RuntimeException("WeavePackageConfig must have an Implementation-Name");
}
return new WeavePackageConfig(name, alias, vendorId, version, enabled, priority, source, custom, instrumentation,
errorHandleClassNode, preprocessor, postprocessor, extensionClassTemplate);
errorHandleClassNode, preprocessor, postprocessor, extensionClassTemplate, weaveViolationFilter);
}
}

Expand Down Expand Up @@ -321,10 +363,12 @@ public static Builder builder() {
private final WeavePostprocessor postprocessor;
private final ClassNode extensionClassTemplate;
private final long priority;
private final WeaveViolationFilter weaveViolationFilter;

private WeavePackageConfig(String name, String alias, String vendorId, float version, boolean enabled,
long priority, String source, boolean custom, Instrumentation instrumentation, ClassNode errorTrapClassNode,
WeavePreprocessor preprocessor, WeavePostprocessor postprocessor, ClassNode extensionClassTemplate) {
WeavePreprocessor preprocessor, WeavePostprocessor postprocessor, ClassNode extensionClassTemplate,
WeaveViolationFilter weaveViolationFilter) {
this.name = name;
this.alias = alias;
this.vendorId = vendorId;
Expand All @@ -339,6 +383,7 @@ private WeavePackageConfig(String name, String alias, String vendorId, float ver
this.preprocessor = preprocessor;
this.postprocessor = postprocessor;
this.extensionClassTemplate = extensionClassTemplate;
this.weaveViolationFilter = weaveViolationFilter;
}

/**
Expand Down Expand Up @@ -399,6 +444,13 @@ public boolean isCustom() {
return custom;
}

/**
* The {@link WeaveViolationFilter} instance, if violations to filter are configured in the module manifest; null otherwise
*/
public WeaveViolationFilter getWeaveViolationFilter() {
return this.weaveViolationFilter;
}

/**
* The instance of {@link Instrumentation} to be used for managing the bootstrap, or <code>null</code> if not
* available.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package com.newrelic.weave;

import com.newrelic.weave.violation.WeaveViolation;
import com.newrelic.weave.violation.WeaveViolationType;
import org.junit.Before;
import org.junit.Test;

import java.util.ArrayList;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

public class WeaveViolationFilterTest {
EnumSet<WeaveViolationType> violationTypes;

@Before
public void setup() {
violationTypes = EnumSet.of(WeaveViolationType.METHOD_MISSING_REQUIRED_ANNOTATIONS, WeaveViolationType.CLASS_MISSING_REQUIRED_ANNOTATIONS);
}


@Test
public void shouldIgnoreViolation_withIgnorableViolation_returnsTrue() {
WeaveViolationFilter filter = new WeaveViolationFilter("com/nr/agent/instrumentation/SpringController_Instrumentation", violationTypes);

WeaveViolation violation = new WeaveViolation(WeaveViolationType.METHOD_MISSING_REQUIRED_ANNOTATIONS, "com/nr/agent/instrumentation/SpringController_Instrumentation");
assertTrue(filter.shouldIgnoreViolation(violation));

violation = new WeaveViolation(WeaveViolationType.CLASS_MISSING_REQUIRED_ANNOTATIONS, "com/nr/agent/instrumentation/SpringController_Instrumentation");
assertTrue(filter.shouldIgnoreViolation(violation));
}

@Test
public void shouldIgnoreViolation_withNonIgnorableViolation_returnsFalse() {
WeaveViolationFilter filter = new WeaveViolationFilter("com/nr/agent/instrumentation/SpringController_Instrumentation", violationTypes);

WeaveViolation violation = new WeaveViolation(WeaveViolationType.FIELD_TYPE_MISMATCH, "com/nr/agent/instrumentation/SpringController_Instrumentation");
assertFalse(filter.shouldIgnoreViolation(violation));
}

@Test
public void filterViolationCollection_filtersIgnorableViolations() {
WeaveViolationFilter filter = new WeaveViolationFilter("com/nr/agent/instrumentation/SpringController_Instrumentation", violationTypes);

Collection<WeaveViolation> violations = new ArrayList<>();
violations.add(new WeaveViolation(WeaveViolationType.METHOD_MISSING_REQUIRED_ANNOTATIONS, "com/nr/agent/instrumentation/SpringController_Instrumentation"));
violations.add(new WeaveViolation(WeaveViolationType.CLASS_MISSING_REQUIRED_ANNOTATIONS, "com/nr/agent/instrumentation/SpringController_Instrumentation"));
violations.add(new WeaveViolation(WeaveViolationType.EXPECTED_NEW_FIELD_ANNOTATION, "com/nr/agent/instrumentation/SpringController_Instrumentation"));
violations.add(new WeaveViolation(WeaveViolationType.CLASS_ACCESS_MISMATCH, "com/nr/agent/instrumentation/SpringController_Instrumentation"));

assertEquals(4, violations.size());

Collection<WeaveViolation> filteredViolations = filter.filterViolationCollection(violations);
assertEquals(2, filteredViolations.size());
assertTrue(filteredViolations.contains(new WeaveViolation(WeaveViolationType.EXPECTED_NEW_FIELD_ANNOTATION, "com/nr/agent/instrumentation/SpringController_Instrumentation")));
assertTrue(filteredViolations.contains(new WeaveViolation(WeaveViolationType.CLASS_ACCESS_MISMATCH, "com/nr/agent/instrumentation/SpringController_Instrumentation")));
}

@Test
public void getWeavePackage_returnsCorrectValue() {
WeaveViolationFilter filter = new WeaveViolationFilter("com/nr/agent/instrumentation/SpringController_Instrumentation", violationTypes);
assertEquals("com/nr/agent/instrumentation/SpringController_Instrumentation", filter.getWeavePackage());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package com.newrelic.weave.weavepackage;

import com.newrelic.weave.WeaveViolationFilter;
import com.newrelic.weave.violation.WeaveViolation;
import com.newrelic.weave.violation.WeaveViolationType;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

public class WeavePackageConfigTest {
@Test
public void ConfigBuilder_withConfiguredFilters_createsWeaveViolationFilter() {
WeavePackageConfig.Builder builder = new WeavePackageConfig.Builder();
WeavePackageConfig config = builder.name("test")
.weaveViolationFilters("METHOD_MISSING_REQUIRED_ANNOTATIONS,CLASS_MISSING_REQUIRED_ANNOTATIONS")
.build();

WeaveViolationFilter filter = config.getWeaveViolationFilter();
assertEquals("test", filter.getWeavePackage());
assertTrue(filter.shouldIgnoreViolation(new WeaveViolation(WeaveViolationType.CLASS_MISSING_REQUIRED_ANNOTATIONS, "clazz")));
assertTrue(filter.shouldIgnoreViolation(new WeaveViolation(WeaveViolationType.METHOD_MISSING_REQUIRED_ANNOTATIONS, "clazz")));
}

@Test
public void ConfigBuilder_withoutConfiguredFilters_doesNotCreatesWeaveViolationFilter() {
WeavePackageConfig.Builder builder = new WeavePackageConfig.Builder();
WeavePackageConfig config = builder.name("test").build();

assertNull(config.getWeaveViolationFilter());
}
}

0 comments on commit 3f62795

Please sign in to comment.