chore(allowlist): Clean out unused code (#321)

- remove unused logic for composite matching
spinnaker · Dec 28, 2019 · 2f65ce9 · 2f65ce9
1 parent a7375c0
commit 2f65ce9
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 134 deletions.
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
@@ -18,22 +18,10 @@ package com.netflix.spinnaker.swabbie.exclusions
 
 import com.netflix.spinnaker.config.Exclusion
 import com.netflix.spinnaker.config.ExclusionType
-import com.netflix.spinnaker.moniker.frigga.FriggaReflectiveNamer
-import com.netflix.spinnaker.swabbie.AccountProvider
-import com.netflix.spinnaker.swabbie.InMemoryCache
-import com.netflix.spinnaker.swabbie.model.Application
 import org.springframework.stereotype.Component
 
 @Component
-class AllowListExclusionPolicy(
-  front50ApplicationCache: InMemoryCache<Application>,
-  accountProvider: AccountProvider
-) : ResourceExclusionPolicy {
-  private val compositeTypeMapping = mapOf(
-    "account" to accountProvider.getAccounts(),
-    "application" to front50ApplicationCache.get()
-  )
-
+class AllowListExclusionPolicy : ResourceExclusionPolicy {
   override fun getType(): ExclusionType = ExclusionType.Allowlist
 
   /**
@@ -47,38 +35,14 @@ class AllowListExclusionPolicy(
       }
 
       kv.keys.forEach { key ->
-        val parts = key.split(".")
-        val identifier = getIdentifierForType(excludable, parts[0])
-        if (identifier != null && parts.size > 1) {
-          compositeTypeMapping[parts[0]]?.filter { it.resourceId == identifier }?.forEach { target ->
-            target.findMatchingAttribute(parts[1], kv.getValue(key))?.let {
-              if (identifier.equals(it, ignoreCase = true)) {
-                return null
-              }
-            }
-          }
-        } else {
-          excludable.findMatchingAttribute(key, kv.getValue(key))?.let {
-            // since a matching value is returned for the key, we know it is in the allowlist
-            return null
-          }
+        excludable.findMatchingAttribute(key, kv.getValue(key))?.let {
+          // since a matching value is returned for the key, we know it is in the allowlist
+          return null
         }
       }
 
       // if none of the keys have a qualifying value, the resource is not in the allowlist
-      return notAllowlistedMessage(getIdentifierForType(excludable), kv.values.flatten().toSet())
-    }
-  }
-
-  private fun getIdentifierForType(excludable: Excludable, type: String? = null): String? {
-    if (type == "application") {
-      return FriggaReflectiveNamer().deriveMoniker(excludable).app?.toLowerCase() ?: ""
+      return notAllowlistedMessage(excludable.name, kv.values.flatten().toSet())
     }
-
-    if (type == "account") {
-      return excludable.name
-    }
-
-    return excludable.resourceId
   }
 }
diff --git a/.../src/test/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicyTest.kt b/.../src/test/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicyTest.kt
@@ -14,67 +14,24 @@
  * limitations under the License.
  */
 package com.netflix.spinnaker.swabbie.exclusions
-import com.natpryce.hamkrest.equalTo
-import com.natpryce.hamkrest.should.shouldMatch
 import com.netflix.spinnaker.config.Attribute
 import com.netflix.spinnaker.config.Exclusion
 import com.netflix.spinnaker.config.ExclusionType
-import com.netflix.spinnaker.swabbie.InMemoryCache
-import com.netflix.spinnaker.swabbie.model.Application
 import com.netflix.spinnaker.swabbie.test.TestResource
-import com.nhaarman.mockito_kotlin.doReturn
-import com.nhaarman.mockito_kotlin.mock
-import com.nhaarman.mockito_kotlin.whenever
-import org.junit.jupiter.api.Assertions
-import org.junit.jupiter.api.Assertions.assertTrue
 import org.junit.jupiter.api.Test
+import strikt.api.expect
+import strikt.assertions.containsExactly
+import strikt.assertions.isEqualTo
 
 object AllowListExclusionPolicyTest {
-  private val front50ApplicationCache: InMemoryCache<Application> = mock()
-
-  @Test
-  fun `should exclude based on composite key if not in Allow List`() {
-    val exclusions = listOf(
-      Exclusion()
-        .withType(ExclusionType.Allowlist.toString())
-        .withAttributes(
-          setOf(
-            Attribute()
-              .withKey("application.name")
-              .withValue(
-                listOf("testapp", "pattern:^important")
-              )
-          )
-        )
-    )
-    whenever(front50ApplicationCache.get()) doReturn
-      setOf(
-        Application(name = "testapp", email = "[email protected]"),
-        Application(name = "important", email = "[email protected]"),
-        Application(name = "random", email = "[email protected]")
-      )
-    val resources = listOf(
-      TestResource("testapp-v001"),
-      TestResource("important-v001"),
-      TestResource("test-v001")
-    )
-    resources.filter {
-      AllowListExclusionPolicy(front50ApplicationCache, mock()).apply(it, exclusions) == null
-    }.let { filteredResources ->
-      filteredResources.size shouldMatch equalTo(2)
-      filteredResources.map { it.resourceId }.let {
-        assertTrue(it.contains("important-v001"), "Allow List by pattern")
-        assertTrue(it.contains("testapp-v001"), "Allow List by name")
-      }
-      filteredResources.first().resourceId shouldMatch equalTo("testapp-v001")
-    }
-  }
+  private val subject = AllowListExclusionPolicy()
+  private val allowListType = ExclusionType.Allowlist.name
 
   @Test
   fun `should exclude if not Allow List`() {
     val exclusions = listOf(
       Exclusion()
-        .withType(ExclusionType.Allowlist.toString())
+        .withType(allowListType)
         .withAttributes(
           setOf(
             Attribute()
@@ -85,38 +42,39 @@ object AllowListExclusionPolicyTest {
           )
         )
     )
-    whenever(front50ApplicationCache.get()) doReturn
-      setOf(
-        Application(name = "testapp", email = "[email protected]"),
-        Application(name = "important", email = "[email protected]"),
-        Application(name = "random", email = "[email protected]")
-      )
-    val resources = listOf(
-      TestResource("testapp-v001"),
-      TestResource("important-v001"),
-      TestResource("test-v001")
-    )
-    resources.filter {
-      AllowListExclusionPolicy(front50ApplicationCache, mock()).apply(it, exclusions) == null
-    }.let { filteredResources ->
-      filteredResources.size shouldMatch equalTo(2)
-      filteredResources.map { it.resourceId }.let {
-        Assertions.assertTrue(it.contains("important-v001"), "Allow List by pattern")
-        Assertions.assertTrue(it.contains("testapp-v001"), "Allow List by name")
+
+    val resource1 = TestResource("testapp-v001")
+    val resource2 = TestResource("important-v001")
+    val resource3 = TestResource("test-v001")
+
+    val resources = listOf(resource1, resource2, resource3)
+
+    val result = resources
+      .filter {
+        subject.apply(it, exclusions) == null
       }
-      filteredResources.first().resourceId shouldMatch equalTo("testapp-v001")
+
+    expect {
+      that(result.size).isEqualTo(2)
+      that(result).containsExactly(resource1, resource2)
     }
   }
 
   @Test
   fun `should include if in one of the allow lists`() {
+    val ownerField = "swabbieResourceOwner"
+    val resource1 = TestResource("testapp-v001").withDetail(ownerField, "[email protected]")
+    val resource2 = TestResource("grpclab-v001").withDetail(ownerField, "[email protected]")
+    val resource3 = TestResource("test-v001").withDetail(ownerField, "[email protected]")
+    val resources = listOf(resource1, resource2, resource3)
+
     val exclusions = listOf(
       Exclusion()
-        .withType(ExclusionType.Allowlist.toString())
+        .withType(allowListType)
         .withAttributes(
           setOf(
             Attribute()
-              .withKey("swabbieResourceOwner")
+              .withKey(ownerField)
               .withValue(
                 listOf("[email protected]", "[email protected]")
               ),
@@ -128,29 +86,14 @@ object AllowListExclusionPolicyTest {
           )
         )
     )
-    whenever(front50ApplicationCache.get()) doReturn
-      setOf(
-        Application(name = "testapp", email = "[email protected]"),
-        Application(name = "important", email = "[email protected]"),
-        Application(name = "random", email = "[email protected]")
-      )
-    val resources = listOf(
-      TestResource("testapp-v001")
-        .withDetail("swabbieResourceOwner", "[email protected]"),
-      TestResource("grpclab-v001")
-        .withDetail("swabbieResourceOwner", "[email protected]"),
-      TestResource("test-v001")
-        .withDetail("swabbieResourceOwner", "[email protected]")
-    )
-    resources.filter {
-      AllowListExclusionPolicy(front50ApplicationCache, mock()).apply(it, exclusions) == null
-    }.let { filteredResources ->
-      filteredResources.size shouldMatch equalTo(2)
-      filteredResources.map { it.resourceId }.let {
-        assertTrue(it.contains("grpclab-v001"), "Allow List by pattern")
-        assertTrue(it.contains("testapp-v001"), "Allow List by owner")
-      }
-      filteredResources.first().resourceId shouldMatch equalTo("testapp-v001")
+
+    val result = resources.filter {
+      subject.apply(it, exclusions) == null
+    }
+
+    expect {
+      that(result.size).isEqualTo(2)
+      that(result).containsExactly(resource1, resource2)
     }
   }
 }