improvement(attributesRule): Handling non strings in attributeRule (#323

) * improvement(attributesRule): Handling non strings in attributeRule - updated attributes matching to handle non strings properties * - parameter rename
spinnaker · Jan 6, 2020 · db01503 · db01503
1 parent 869c188
commit db01503
Show file tree

Hide file tree

Showing 7 changed files with 42 additions and 28 deletions.
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/config/SwabbieProperties.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/config/SwabbieProperties.kt
@@ -146,11 +146,7 @@ class ResourceTypeConfiguration {
             - name: ExpiredResourceRule
             - name: AttributeRule
               parameters:
-                - type: Tag
-                  attributes:
-                  - key: description
-                    value:
-                    - pattern:^packer-build
+                description: pattern:^packer-build
    *----------------
    * @property operator ([RuleConfiguration.OPERATOR.OR], [RuleConfiguration.OPERATOR.AND]) dictate how rules are applied
    * @property rules a list of named rules [com.netflix.spinnaker.swabbie.model.Rule]
@@ -204,7 +200,7 @@ class ResourceTypeConfiguration {
 
   class RuleDefinition {
     var name: String = ""
-    var parameters: Map<String, Any> = mapOf()
+    var parameters: Map<String, Any?> = mapOf()
 
     override fun equals(other: Any?): Boolean {
       if (this === other) {

diff --git a/...core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicy.kt b/...core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicy.kt
@@ -35,7 +35,7 @@ class AllowListExclusionPolicy : ResourceExclusionPolicy {
       }
 
       kv.keys.forEach { key ->
-        excludable.findMatchingAttribute(key, kv.getValue(key))?.let {
+        if (excludable.matchResourceAttributes(key, kv.getValue(key))) {
           // since a matching value is returned for the key, we know it is in the allowlist
           return null
         }

diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/ExclusionPolicy.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/ExclusionPolicy.kt
@@ -67,8 +67,9 @@ interface ExclusionPolicy {
 
     // match on property name
     kv.keys.forEach { key ->
-      excludable.findMatchingAttribute(key, kv.getValue(key))?.let {
-        return patternMatchMessage(key, setOf(it))
+      val matchingValues: List<Any?> = kv.getValue(key)
+      if (excludable.matchResourceAttributes(key, matchingValues)) {
+        return patternMatchMessage(key, matchingValues.map { "$it" }.toSet())
       }
     }
 

diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/model/Resource.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/model/Resource.kt
@@ -174,35 +174,31 @@ interface Identifiable : Named {
     return "${workConfiguration.notificationConfiguration.resourceUrl}/$resourceId"
   }
 
-  fun findMatchingAttribute(key: String, values: List<Any>): String? {
-    try {
-      val fieldValue = getProperty(key) as? String
-      if (propertyMatches(values, fieldValue)) {
-        return fieldValue
-      }
-    } catch (e: IllegalArgumentException) {
-      return null
+  fun matchResourceAttributes(propertyName: String, values: List<Any?>): Boolean {
+    return try {
+      (getProperty(propertyName) as? Any).matchesAny(values)
+    } catch (e: Exception) {
+      false
     }
-
-    return null
   }
 
-  private fun propertyMatches(values: List<Any>, fieldValue: String?): Boolean {
-    if (fieldValue == null) {
-      return false
+  private fun Any?.matchesAny(values: List<Any?>): Boolean {
+    if (this == null || this !is String) {
+      return values.contains(this)
     }
-    val splitFieldValue = fieldValue.split(",").map { it.trim() }
 
-    return values.contains(fieldValue) ||
-      values.any { it is String && fieldValue.patternMatched(it) || splitFieldValue.contains(it) }
+    val splitFieldValue = this.split(",").map { it.trim() }
+
+    return values.contains(this) ||
+      values.any { it is String && this.patternMatched(it) || splitFieldValue.contains(it) }
   }
 
   private fun <R : Any?> getProperty(propertyName: String): R {
     try {
       return readPropery(propertyName)
     } catch (e: NoSuchElementException) {
       val details: Map<String, Any?>? = readPropery("details")
-      if (details != null) {
+      if (details != null && propertyName in details) {
         return details[propertyName] as R
       }
 

diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/rules/Rules.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/rules/Rules.kt
@@ -71,6 +71,6 @@ class AttributeRule : Rule {
   private fun <T : Resource> T.matchAttributes(ruleDefinition: RuleDefinition?): Boolean {
     // params contains key-value pairs to match against the resource's attributes
     val params = ruleDefinition?.parameters ?: return false
-    return params.any { (key, value) -> findMatchingAttribute(key, listOf(value)) != null }
+    return params.any { (key, value) -> matchResourceAttributes(key, listOf(value)) }
   }
 }
diff --git a/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/TestResourceTypeHandler.kt b/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/TestResourceTypeHandler.kt
@@ -138,7 +138,7 @@ class TestResourceTypeHandler(
 }
 
 class TestRule(
-  private val invalidOn: (Resource, Map<String, Any>) -> Boolean,
+  private val invalidOn: (Resource, Map<String, Any?>) -> Boolean,
   private val summary: Summary?,
   private val name: String = ""
 ) : Rule {

diff --git a/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/rules/AttributeRuleTest.kt b/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/rules/AttributeRuleTest.kt
@@ -61,4 +61,25 @@ object AttributeRuleTest {
     expectThat(rule.apply(resource.copy(name = "foo bar"), ruleDefinition).summary).isNull()
     expectThat(rule.apply(resource.copy(name = "bar foo"), ruleDefinition).summary).isNotNull()
   }
+
+  @Test
+  fun `should work with non strings properties`() {
+    expectThat(rule.apply(resource).summary).isNull()
+
+    val ruleDefinition = RuleDefinition()
+      .apply {
+        name = rule.name()
+        parameters = mapOf(
+          "isValid" to false,
+          "version" to 1.0
+        )
+      }
+
+    expectThat(rule.apply(resource, ruleDefinition).summary).isNull()
+    expectThat(rule.apply(resource.withDetail("isValid", false), ruleDefinition).summary).isNotNull()
+    expectThat(rule.apply(resource.withDetail("isValid", true), ruleDefinition).summary).isNull()
+
+    expectThat(rule.apply(resource.withDetail("version", 0), ruleDefinition).summary).isNull()
+    expectThat(rule.apply(resource.withDetail("version", 1.0), ruleDefinition).summary).isNotNull()
+  }
 }