Change filtering rules behavior (#48)

Filtering rules now mimic the behavior of iptables firewall rules. The last applicable rule wins.
Made the test GHA workflow trigger on PRs.

.github/workflows/ci.yaml

@@ -1,6 +1,6 @@
 name: Testing
 
-on: [push]
+on: [push, pull_request]
 
 jobs:
   build:

src/main/java/com/datastax/mcac/FilteringRule.java

@@ -2,6 +2,7 @@
 
 import java.util.Collection;
 import java.util.Objects;
+import java.util.Optional;
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
 
@@ -10,18 +11,19 @@
 
 import org.apache.flink.shaded.jackson2.com.fasterxml.jackson.annotation.JsonIgnore;
 import org.apache.flink.shaded.jackson2.com.fasterxml.jackson.annotation.JsonProperty;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 public class FilteringRule
 {
+    private static final Logger logger = LoggerFactory.getLogger(FilteringRule.class);
     public static final String ALLOW = "allow";
     public static final String DENY = "deny";
     public static final String GLOBAL = "global";
     public static final String DATALOG_ONLY = "datalog";
 
-    public static final FilteringRule FILTERED_GLOBALLY = new FilteringRule(DENY, ".*", GLOBAL);
-    public static final FilteringRule FILTERED_INSIGHTS = new FilteringRule(DENY, ".*", DATALOG_ONLY);
     public static final FilteringRule ALLOWED_GLOBALLY = new FilteringRule(ALLOW, ".*", GLOBAL);
-    public static final FilteringRule ALLOWED_INSIGHTS = new FilteringRule(ALLOW, ".*", DATALOG_ONLY);
+
 
     @JsonProperty("policy")
     public final String policy;
@@ -82,60 +84,30 @@ public void init()
         });
     }
 
-    public boolean isAllowed(String name)
+    public boolean matches(String name)
     {
-        boolean match = patternRegex.get().matcher(name).find();
-        return isAllowRule ? match : !match;
+        return patternRegex.get().matcher(name).find();
     }
 
     /**
-     * Returns the most applicable filtering rule for this name
-     * taking into account global vs insights level scope.
+     * Returns the most applicable filtering rule for this name.
      *
-     * global taking precedent over insights and deny rules taking precedent over allow rules
+     * The last rule in the list of applicable rules wins.
      * @param name
      * @return The rule that applied for this name.
      */
     public static FilteringRule applyFilters(String name, Collection<FilteringRule> rules)
     {
-        FilteringRule allowRule = null;
-        FilteringRule denyRule = null;
-        boolean hasDenyRule = false;
-        boolean hasAllowRule = false;
+        Optional<FilteringRule> lastRule = rules.stream().filter(rule -> rule.matches(name)).reduce((first, second) -> second);
 
-        for (FilteringRule rule : rules)
-        {
-            if (rule.isAllowRule)
-            {
-                hasAllowRule = true;
-                if ((allowRule == null || !allowRule.isGlobal) && rule.isAllowed(name))
-                {
-                    allowRule = rule;
-                }
-            }
-            else
-            {
-                hasDenyRule = true;
-                if ((denyRule == null || !denyRule.isGlobal) && !rule.isAllowed(name))
-                {
-                    denyRule = rule;
-                }
-            }
-        }
-
-        if (rules.isEmpty())
-            return FilteringRule.ALLOWED_GLOBALLY;
-
-        if (denyRule != null)
-            return denyRule;
-
-        if (allowRule != null)
-            return allowRule;
-
-        if (hasDenyRule && !hasAllowRule)
+        if (!lastRule.isPresent())
             return FilteringRule.ALLOWED_GLOBALLY;
 
-        return FilteringRule.FILTERED_GLOBALLY;
+        if (logger.isDebugEnabled()) {
+            logger.debug("Metric {}", name);
+            logger.debug(">>>>>> Applying rule {}", lastRule.get());
+        }
+        return lastRule.get();
     }
 
 

src/main/resources/build_version

@@ -1 +1 @@
-0.2.0
+0.3.0

src/test/java/com/datastax/mcac/FilteringTest.java

@@ -2,6 +2,7 @@
 
 import java.net.MalformedURLException;
 import java.net.URISyntaxException;
+import java.util.Arrays;
 
 import com.google.common.io.Resources;
 import org.junit.Assert;
@@ -14,6 +15,33 @@ public void testConfig() throws URISyntaxException, MalformedURLException
     {
         Configuration c = ConfigurationLoader.loadConfig(Resources.getResource("metric-collector.yaml").toURI().toURL());
 
-        Assert.assertEquals(1, c.filtering_rules.size());
+        Assert.assertEquals(3, c.filtering_rules.size());
+    }
+
+    @Test
+    public void testAllowLastWins() {
+        // Check that allow takes precedence over deny if set last
+        FilteringRule firstRule = new FilteringRule("deny", "org.apache.cassandra.metrics", FilteringRule.GLOBAL);
+        FilteringRule secondRule = new FilteringRule("allow", "org.apache.cassandra.metrics.metric1", FilteringRule.GLOBAL);
+        Assert.assertTrue(FilteringRule.applyFilters("org.apache.cassandra.metrics.metric1", Arrays.asList(firstRule, secondRule)).isAllowRule);
+    }
+
+    @Test
+    public void testAllowFirstLoses() {
+        // Check that deny takes precedence over allow if set last
+        FilteringRule firstRule = new FilteringRule("allow", "org.apache.cassandra.metrics.metric1", FilteringRule.GLOBAL);
+        FilteringRule secondRule = new FilteringRule("deny", "org.apache.cassandra.metrics", FilteringRule.GLOBAL);
+        Assert.assertFalse(FilteringRule.applyFilters("org.apache.cassandra.metrics.metric1", Arrays.asList(firstRule, secondRule)).isAllowRule);
+    }
+
+    @Test
+    public void testMixedConditions() {
+        FilteringRule unaffectedByDenyRule = new FilteringRule("allow", "org.apache.cassandra.whatever.metric3", FilteringRule.GLOBAL);
+        FilteringRule firstRule = new FilteringRule("allow", "org.apache.cassandra.metrics.metric1", FilteringRule.GLOBAL);
+        FilteringRule secondRule = new FilteringRule("deny", "org.apache.cassandra.metrics", FilteringRule.GLOBAL);
+        FilteringRule thirdRule = new FilteringRule("allow", "org.apache.cassandra.metrics.metric2", FilteringRule.GLOBAL);
+        Assert.assertFalse(FilteringRule.applyFilters("org.apache.cassandra.metrics.metric1", Arrays.asList(unaffectedByDenyRule, firstRule, secondRule, thirdRule)).isAllowRule);
+        Assert.assertTrue(FilteringRule.applyFilters("org.apache.cassandra.metrics.metric2", Arrays.asList(unaffectedByDenyRule, firstRule, secondRule, thirdRule)).isAllowRule);
+        Assert.assertTrue(FilteringRule.applyFilters("org.apache.cassandra.whatever.metric3", Arrays.asList(unaffectedByDenyRule, firstRule, secondRule, thirdRule)).isAllowRule);
     }
 }

src/test/resources/metric-collector.yaml

@@ -8,10 +8,18 @@ write_to_disk_enabled: true
 # Default: 5000
 #data_dir_max_size_in_mb: 5000
 
+# The last applicable rule for a metric wins.
+# This allows to deny all metrics for a specific metric group and then allow them individually.
 filtering_rules:
   - policy: deny
     pattern: jvm.fd.usage
     scope: global
+  - policy: deny
+    pattern: org.apache.cassandra.metrics.Table
+    scope: global
+  - policy: allow
+    pattern: org.apache.cassandra.metrics.Table.LiveSSTableCount
+    scope: global
 
 #To output tombstone counter metrics for integration tests
 #metric_sampling_interval - The frequency that metrics are reported to Cassandra Metrics Collector.