feat(mark): removing mark of deleted resources, comments (#123)

swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AbstractResourceTypeHandler.kt

@@ -226,6 +226,30 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
     restoreResources(listOf(markedResource), workConfiguration)
   }
 
+  /**
+   * @param candidates: a list of resources that exist in the account/location defined in the work configuration
+   * @param workConfiguration: defines the account/location to work with
+   *
+   * Fetches already marked resources, filters by work configuration namespace, and un-marks any resource whos id
+   * is not present in candidates.
+   */
+  private fun removeDeletedResourcesFromMarkedResources(
+    candidates: List<T>?,
+    workConfiguration: WorkConfiguration
+  ) {
+    resourceRepository
+      .getMarkedResources()
+      .filter { it.namespace == workConfiguration.namespace }
+      .let { markedResourcesToCheck ->
+        val existingResources = candidates?.map { it.resourceId }?.toHashSet() ?: emptySet<String>()
+        for (mr in markedResourcesToCheck) {
+          if (!existingResources.contains(mr.resourceId)) {
+            ensureResourceUnmarked(mr, workConfiguration, "Resource no longer exists")
+          }
+        }
+      }
+  }
+
   private fun doMark(workConfiguration: WorkConfiguration, postMark: () -> Unit) {
     // initialize counters
     exclusionCounters[Action.MARK] = AtomicInteger(0)
@@ -234,6 +258,7 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
     val violationCounter = AtomicInteger(0)
     val totalResourcesVisitedCounter = AtomicInteger(0)
     val markedResources = mutableListOf<MarkedResource>()
+
     try {
       log.info("${javaClass.simpleName} running. Configuration: ", workConfiguration.toLog())
       val candidates: List<T>? = getCandidates(workConfiguration)
@@ -243,6 +268,11 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
       log.info("Fetched {} resources. Configuration: {}", candidates.size, workConfiguration.toLog())
       totalResourcesVisitedCounter.set(candidates.size)
 
+      removeDeletedResourcesFromMarkedResources(candidates, workConfiguration)
+
+      val markedCandidates: List<MarkedResource> = resourceRepository.getMarkedResources()
+        .filter { it.namespace == workConfiguration.namespace }
+
       val optedOutResourceStates: List<ResourceState> = resourceStateRepository.getAll()
         .filter { it.markedResource.namespace == workConfiguration.namespace && it.optedOut }
 
@@ -254,12 +284,6 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
         .let { filteredCandidates ->
           val maxItemsToProcess = Math.min(filteredCandidates.size, workConfiguration.maxItemsProcessedPerCycle)
 
-          // list of currently marked & stored resources
-          val markedCandidates: List<MarkedResource> = resourceRepository.getMarkedResources()
-            .filter {
-              it.namespace == workConfiguration.namespace
-            }
-
           for (candidate in filteredCandidates) {
             if (candidateCounter.get() > maxItemsToProcess) {
               log.info("Max items ({}) to process reached, short-circuiting", maxItemsToProcess)
@@ -272,7 +296,11 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
                 it.resourceId == candidate.resourceId
               }.let { alreadyMarkedCandidate ->
                 when {
-                  violations.isEmpty() -> ensureResourceUnmarked(alreadyMarkedCandidate, workConfiguration)
+                  violations.isEmpty() -> {
+                    ensureResourceUnmarked(alreadyMarkedCandidate,
+                                           workConfiguration,
+                                           "Resource no longer qualifies for deletion")
+                  }
                   alreadyMarkedCandidate == null -> {
                     val newMarkedResource = MarkedResource(
                       resource = candidate,
@@ -361,12 +389,12 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
 
   private fun ensureResourceUnmarked(
     markedResource: MarkedResource?,
-    workConfiguration: WorkConfiguration
+    workConfiguration: WorkConfiguration,
+    reason: String
   ) {
     if (markedResource != null && !workConfiguration.dryRun) {
       try {
-        // TODO(rz): Log reason
-        log.info("{} is no longer a candidate.", markedResource)
+        log.info("{} is no longer a candidate because: $reason.", markedResource)
         resourceRepository.remove(markedResource)
         applicationEventPublisher.publishEvent(UnMarkResourceEvent(markedResource, workConfiguration))
       } catch (e: Exception) {
@@ -428,7 +456,6 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
     }
 
     optedOutResourceStates.find { it.markedResource.resourceId == resource.resourceId }?.let {
-      // TODO: opting out should expire based on configured time
       log.debug("Skipping Opted out resource {}", resource)
       return true
     }
@@ -460,16 +487,14 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
       }
       log.debug("Found ${resourcesToSoftDelete.size} resource(s) to soft delete")
 
-      //todo eb: seems when someone opts out we should remove them from the list of deletable,
-      // instead of filtering them out every time every where.
       val optedOutResourceStates: List<ResourceState> = resourceStateRepository.getAll()
         .filter { it.markedResource.namespace == workConfiguration.namespace && it.optedOut }
 
       // figure out if they still qualify to be any sort of deleted, and deal with it if they don't
       val candidates: List<T>? = getCandidates(workConfiguration)
       if (candidates == null || candidates.isEmpty()) {
         resourcesToSoftDelete
-          .forEach { ensureResourceUnmarked(it, workConfiguration) }
+          .forEach { ensureResourceUnmarked(it, workConfiguration, "No current candidates for deletion") }
         log.info("Nothing to delete. No upstream resources, Configuration: {}", workConfiguration.toLog())
         return
       }
@@ -485,7 +510,7 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
           if ((candidate == null || candidate.getViolations().isEmpty()) ||
             shouldExcludeResource(candidate, workConfiguration, optedOutResourceStates, Action.SOFTDELETE)) {
             shouldSkip = true
-            ensureResourceUnmarked(resource, workConfiguration)
+            ensureResourceUnmarked(resource, workConfiguration, "Resource no longer qualifies for deletion")
           }
           !shouldSkip
         }
@@ -538,7 +563,7 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
           var candidates: List<T>? = getCandidates(workConfiguration)
           if (candidates == null) {
             toDelete
-              .forEach { ensureResourceUnmarked(it, workConfiguration) }
+              .forEach { ensureResourceUnmarked(it, workConfiguration, "No current candidates for deletion") }
             log.info("Nothing to delete. No upstream resources. Configuration: {}", workConfiguration.toLog())
             return
           }
@@ -556,7 +581,7 @@ abstract class AbstractResourceTypeHandler<T : Resource>(
             if ((candidate == null || candidate.getViolations().isEmpty()) ||
               shouldExcludeResource(candidate, workConfiguration, optedOutResourceStates, Action.DELETE)) {
               shouldSkip = true
-              ensureResourceUnmarked(r, workConfiguration)
+              ensureResourceUnmarked(r, workConfiguration, "Resource no longer qualifies for deletion")
             }
             !shouldSkip
           }.let { filteredCandidateList ->

swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicy.kt

@@ -35,6 +35,11 @@ class AllowListExclusionPolicy(
   )
 
   override fun getType(): ExclusionType = ExclusionType.Allowlist
+
+  /**
+   * Takes a resource that can be excluded (an excludable), and a list of the configured exclusions, and
+   * determines if the resource can be excluded based on those rules.
+   */
   override fun apply(excludable: Excludable, exclusions: List<Exclusion>): String? {
     keysAndValues(exclusions, ExclusionType.Allowlist).let { kv ->
       if (kv.isEmpty()) {
@@ -54,11 +59,13 @@ class AllowListExclusionPolicy(
           }
         } else {
           findProperty(excludable, key, kv[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())
     }
   }

swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/exclusions/ExclusionPolicy.kt

@@ -105,6 +105,11 @@ interface ExclusionPolicy {
     return instance.javaClass.kotlin.memberProperties.first { it.name == propertyName }.get(instance) as R
   }
 
+  /**
+   * Takes a list of config-defined exlusions.
+   * For each exclusion that matches the type we're considering,
+   * transform all information into a key,values that make up this policy
+   */
   fun keysAndValues(exclusions: List<Exclusion>, type: ExclusionType): Map<String, List<String>> {
     val map = mutableMapOf<String, List<String>>()
     exclusions.filter {
@@ -146,6 +151,10 @@ data class ExclusionResult(
 )
 
 interface Excludable : Identifiable {
+  /**
+   * @param exclusionPolicies: all possible policies defined in code
+   * @param exclusions: actual configured policies based on swabbie.yml
+   */
   fun shouldBeExcluded(exclusionPolicies: List<ExclusionPolicy>, exclusions: List<Exclusion>): ExclusionResult {
     exclusionPolicies.mapNotNull { it.apply(this, exclusions) }.let { reasons ->
       return ExclusionResult(!reasons.isEmpty(), reasons.toSet())

swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/exclusions/AllowListExclusionPolicyTest.kt

@@ -30,6 +30,7 @@ import org.junit.jupiter.api.Assertions.*
 import org.junit.jupiter.api.Test
 object AllowListExclusionPolicyTest {
   private val front50ApplicationCache: InMemoryCache<Application> = mock()
+
   @Test
   fun `should exclude based on composite key if not in Allow List`() {
     val exclusions = listOf(
@@ -67,6 +68,7 @@ object AllowListExclusionPolicyTest {
       filteredResources.first().resourceId shouldMatch equalTo("testapp-v001")
     }
   }
+
   @Test
   fun `should exclude if not Allow List`() {
     val exclusions = listOf(
@@ -104,4 +106,50 @@ object AllowListExclusionPolicyTest {
       filteredResources.first().resourceId shouldMatch equalTo("testapp-v001")
     }
   }
+
+  @Test
+  fun `should include if in one of the allow lists`() {
+    val exclusions = listOf(
+      Exclusion()
+        .withType(ExclusionType.Allowlist.toString())
+        .withAttributes(
+          listOf(
+            Attribute()
+              .withKey("swabbieResourceOwner")
+              .withValue(
+                listOf("[email protected]", "[email protected]")
+              ),
+            Attribute()
+              .withKey("name")
+              .withValue(
+                listOf("pattern:^grpc.*\$")
+              )
+          )
+        )
+    )
+    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")
+    }
+  }
 }