Merge pull request #46 from jeyrschabu/fixnotifications

jeyrschabu · web-flow · commit 395500fe87c0 · 2018-04-11T09:42:16.000-07:00
refactor(notifications): Updating notifications
diff --git a/README.md b/README.md
@@ -10,9 +10,8 @@ It applies a set of rules to mark cleanup candidates. Once marked, a resource is
 ```
 swabbie:
   taggingEnabled: false
-  optOut:
-    url: http://localhost:8088/
-    defaultTtlDays: 14
+  optOutBaseUrl: http://localhost:8088/
+  spinnakerResourceSearchUrl: https://spinnaker/infrastructure?q=
 
   agents:
     mark:
@@ -58,6 +57,12 @@ swabbie:
           dryRun: true
           retentionDays: 14
           notifyOwner: false
+          exclusions:
+            - type: Whitelist
+              attributes:
+                - key: application
+                  value:
+                    - appname
 
 ```
 
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
@@ -22,6 +22,8 @@ import org.springframework.boot.context.properties.ConfigurationProperties
 open class SwabbieProperties {
   var dryRun: Boolean = true
   var providers: List<CloudProviderConfiguration> = mutableListOf()
+  var optOutBaseUrl: String = ""
+  var spinnakerResourceSearchUrl: String = ""
 }
 
 class CloudProviderConfiguration {
@@ -30,9 +32,11 @@ class CloudProviderConfiguration {
   var locations: List<String> = mutableListOf()
   var accounts: List<String> = mutableListOf()
   var resourceTypes: List<ResourceTypeConfiguration> = mutableListOf()
+  var resourcesPerNotification: Int = 10
   override fun toString(): String {
-    return "CloudProviderConfiguration(exclusions=$exclusions, name='$name', locations=$locations, accounts=$accounts, resourceTypes=$resourceTypes)"
+    return "CloudProviderConfiguration(exclusions=$exclusions, name='$name', locations=$locations, accounts=$accounts, resourceTypes=$resourceTypes, resourcesPerNotification=$resourcesPerNotification)"
   }
+
 }
 
 class Exclusion {
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AbstractResourceTypeHandler.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AbstractResourceTypeHandler.kt
@@ -181,24 +181,32 @@ abstract class AbstractResourceTypeHandler<out T : Resource>(
         }
 
       owners.let { ownersToResources ->
-        val optOutUrl = "https://localhost:1000" //TODO: pass in configuration
-        ownersToResources.forEach { ownerToResource ->
+        ownersToResources.forEach { ownerToResources ->
           log.info("DryRun={}, shouldNotify={}, user={}, {} cleanup candidate(s)",
-            workConfiguration.dryRun, workConfiguration.notifyOwner, ownerToResource.key, ownerToResource.value.size)
-          if (workConfiguration.dryRun || !workConfiguration.notifyOwner) {
+            workConfiguration.dryRun, workConfiguration.notificationConfiguration!!.notifyOwner, ownerToResources.key, ownerToResources.value.size)
+          if (workConfiguration.dryRun || !workConfiguration.notificationConfiguration.notifyOwner) {
             log.info("Skipping notifications for {}", workConfiguration)
           } else {
-            ownerToResource.value.let { resources ->
-              val subject = NotificationMessage.subject(MessageType.EMAIL, clock, *resources.toTypedArray())
-              val body = NotificationMessage.body(MessageType.EMAIL, clock, optOutUrl, *resources.toTypedArray())
-              notifier.notify(ownerToResource.key, subject, body, EchoService.Notification.Type.EMAIL.name).let {
-                log.info("Notification sent to {} for {}", ownerToResource.key, resources)
+            ownerToResources.value.let { resources ->
+              notifier.notify(
+                recipient = ownerToResources.key,
+                messageType = EchoService.Notification.Type.EMAIL.name,
+                additionalContext = mapOf(
+                  "resourceOwner" to ownerToResources.key,
+                  "resources" to resources,
+                  "configuration" to workConfiguration,
+                  "resourceType" to workConfiguration.resourceType.formatted(),
+                  "spinnakerLink" to workConfiguration.notificationConfiguration.spinnakerResourceUrl,
+                  "optOutLink" to workConfiguration.notificationConfiguration.optOutUrl
+                )
+              ).let {
+                log.info("Notification sent to {} for {}", ownerToResources.key, resources)
                 resources.forEach { resource ->
                   val offsetStampSinceMarked: Long = ChronoUnit.MILLIS.between(Instant.ofEpochMilli(resource.createdTs!!), Instant.now(clock))
                   resource.apply {
-                    projectedDeletionStamp = offsetStampSinceMarked + projectedDeletionStamp!!
+                    projectedDeletionStamp += offsetStampSinceMarked
                     notificationInfo = NotificationInfo(
-                      recipient = ownerToResource.key,
+                      recipient = ownerToResources.key,
                       notificationStamp = Instant.now(clock).toEpochMilli(),
                       notificationType = EchoService.Notification.Type.EMAIL.name
                     )
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AgentRunner.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/AgentRunner.kt
@@ -39,7 +39,7 @@ class LockEnabledAgentRunner(
       workConfiguration.takeIf {
         lock(agent, workConfiguration, acquire = true)
       }?.let {
-          log.info("{} processing {}", agentName, workConfiguration)
+          log.info("{} running with configuration {}", agentName, workConfiguration)
           agent.process(
             workConfiguration = workConfiguration,
             onCompleteCallback = {
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/WorkConfigurator.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/WorkConfigurator.kt
@@ -21,6 +21,7 @@ import com.netflix.spinnaker.config.mergeExclusions
 import com.netflix.spinnaker.swabbie.exclusions.ExclusionPolicy
 import com.netflix.spinnaker.swabbie.model.Account
 import com.netflix.spinnaker.swabbie.model.EmptyAccount
+import com.netflix.spinnaker.swabbie.model.NotificationConfiguration
 import com.netflix.spinnaker.swabbie.model.WorkConfiguration
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
@@ -71,8 +72,13 @@ class WorkConfigurator(
                     resourceType = resourceTypeConfiguration.name,
                     retentionDays = resourceTypeConfiguration.retentionDays,
                     exclusions = mergeExclusions(cloudProviderConfiguration.exclusions, resourceTypeConfiguration.exclusions),
-                    notifyOwner = resourceTypeConfiguration.notifyOwner,
-                    dryRun = if (swabbieProperties.dryRun) true else resourceTypeConfiguration.dryRun
+                    dryRun = if (swabbieProperties.dryRun) true else resourceTypeConfiguration.dryRun,
+                    notificationConfiguration = NotificationConfiguration(
+                      notifyOwner = resourceTypeConfiguration.notifyOwner,
+                      spinnakerResourceUrl = swabbieProperties.spinnakerResourceSearchUrl,
+                      optOutUrl = swabbieProperties.optOutBaseUrl,
+                      resourcesPerNotification = cloudProviderConfiguration.resourcesPerNotification
+                    )
                   ).let { configuration ->
                     configuration.takeIf {
                       !shouldExclude(account, it)
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/events/ResourceStateManager.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/events/ResourceStateManager.kt
@@ -126,3 +126,4 @@ class ResourceStateManager(
 }
 
 internal fun MarkedResource.typeAndName(): String = this.resourceType.split("(?=[A-Z])".toRegex()).joinToString(" ") + ": " + this.name
+internal fun String.formatted(): String = this.split("(?=[A-Z])".toRegex()).joinToString(" ").toLowerCase()
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/model/WorkConfiguration.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/model/WorkConfiguration.kt
@@ -27,5 +27,14 @@ data class WorkConfiguration(
   val retentionDays: Int,
   val exclusions: List<Exclusion>,
   val dryRun: Boolean = true,
-  val notifyOwner: Boolean
+  val notificationConfiguration: NotificationConfiguration? = EmptyNotificationConfiguration()
 )
+
+open class NotificationConfiguration(
+  val notifyOwner: Boolean,
+  val optOutUrl: String,
+  val resourcesPerNotification: Int,
+  val spinnakerResourceUrl: String
+)
+
+class EmptyNotificationConfiguration : NotificationConfiguration(false, "", 0, "")
diff --git a/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/notificationMessages.kt b/swabbie-core/src/main/kotlin/com/netflix/spinnaker/swabbie/notificationMessages.kt
diff --git a/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/ResourceTypeHandlerTest.kt b/swabbie-core/src/test/kotlin/com/netflix/spinnaker/swabbie/ResourceTypeHandlerTest.kt
@@ -286,8 +286,7 @@ object ResourceTypeHandlerTest {
       cloudProvider = TEST_RESOURCE_PROVIDER_TYPE,
       retentionDays = 14,
       dryRun = dryRun,
-      exclusions = exclusions,
-      notifyOwner = notifyOwner
+      exclusions = exclusions
     )
 
   class TestRule(
diff --git a/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/EchoNotifier.kt b/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/EchoNotifier.kt
@@ -22,18 +22,16 @@ import org.springframework.stereotype.Component
 class EchoNotifier(
   private val echoService: EchoService
 ) : Notifier {
-  override fun notify(recipient: String, subject: String, body: String, messageType: String) {
+  override fun notify(recipient: String, additionalContext: Map<String, Any>, messageType: String) {
     echoService.create(
       EchoService.Notification(
         notificationType = EchoService.Notification.Type.valueOf(messageType),
         to = listOf(recipient),
         cc = listOf(recipient),
         severity = NotificationSeverity.HIGH,
         source = EchoService.Notification.Source("swabbie"),
-        additionalContext = mapOf(
-          "subject" to subject,
-          "body" to body
-        )
+        templateGroup = "swabbie",
+        additionalContext = additionalContext
       )
     )
   }
diff --git a/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/EchoService.kt b/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/EchoService.kt
@@ -32,6 +32,7 @@ interface EchoService {
     val cc: Collection<String> = listOf(),
     val severity: NotificationSeverity,
     val source: Source,
+    val templateGroup: String,
     val additionalContext: Map<String, Any?> = mapOf()
   ) {
 
diff --git a/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/Notifier.kt b/swabbie-echo/src/main/kotlin/com/netflix/spinnaker/swabbie/echo/Notifier.kt
@@ -17,5 +17,5 @@
 package com.netflix.spinnaker.swabbie.echo
 
 interface Notifier {
-  fun notify(recipient: String, subject: String, body: String, messageType: String)
+  fun notify(recipient: String, additionalContext: Map<String, Any>, messageType: String)
 }
diff --git a/swabbie-echo/src/test/kotlin/com/netflix/spinnaker/swabbie/echo/EchoNotifierTest.kt b/swabbie-echo/src/test/kotlin/com/netflix/spinnaker/swabbie/echo/EchoNotifierTest.kt
@@ -29,8 +29,7 @@ object EchoNotifierTest {
   fun `should notify`() {
     subject.notify(
       recipient = "yolo@netflix.com",
-      subject = "subject",
-      body = "body",
+      additionalContext = mapOf(),
       messageType = EchoService.Notification.Type.EMAIL.name
     )
 
diff --git a/swabbie-redis/src/test/kotlin/com/netflix/spinnaker/swabbie/redis/RedisResourceTrackingRepositoryTest.kt b/swabbie-redis/src/test/kotlin/com/netflix/spinnaker/swabbie/redis/RedisResourceTrackingRepositoryTest.kt
@@ -73,8 +73,7 @@ object RedisResourceTrackingRepositoryTest {
       resourceType = "testResourceType",
       cloudProvider = AWS,
       retentionDays = 14,
-      exclusions = emptyList(),
-      notifyOwner = true
+      exclusions = emptyList()
     )
 
     val markedResource = MarkedResource(
@@ -110,8 +109,7 @@ object RedisResourceTrackingRepositoryTest {
       resourceType = "testResourceType",
       cloudProvider = AWS,
       retentionDays = 14,
-      exclusions = emptyList(),
-      notifyOwner = true
+      exclusions = emptyList()
     )
 
     listOf(
diff --git a/swabbie-web/src/main/kotlin/com/netflix/spinnaker/swabbie/tagging/ResourceEntityTagger.kt b/swabbie-web/src/main/kotlin/com/netflix/spinnaker/swabbie/tagging/ResourceEntityTagger.kt
@@ -16,29 +16,23 @@
 
 package com.netflix.spinnaker.swabbie.tagging
 
+import com.netflix.spinnaker.config.SwabbieProperties
 import com.netflix.spinnaker.moniker.frigga.FriggaReflectiveNamer
-import com.netflix.spinnaker.swabbie.MessageType
-import com.netflix.spinnaker.swabbie.NotificationMessage
 import com.netflix.spinnaker.swabbie.ResourceTagger
-import com.netflix.spinnaker.swabbie.model.LOAD_BALANCER
-import com.netflix.spinnaker.swabbie.model.MarkedResource
-import com.netflix.spinnaker.swabbie.model.SECURITY_GROUP
-import com.netflix.spinnaker.swabbie.model.WorkConfiguration
+import com.netflix.spinnaker.swabbie.model.*
 import org.slf4j.Logger
 import org.slf4j.LoggerFactory
-import org.springframework.beans.factory.annotation.Value
 import org.springframework.boot.autoconfigure.condition.ConditionalOnExpression
 import org.springframework.stereotype.Component
 import java.time.Clock
 
 @Component
 @ConditionalOnExpression("\${swabbie.taggingEnabled}")
 class ResourceEntityTagger(
-  private val taggingService: EntityTaggingService,
-  private val clock: Clock
+  private val clock: Clock,
+  private val swabbieProperties: SwabbieProperties,
+  private val taggingService: EntityTaggingService
 ) : ResourceTagger {
-  @Value("\${swabbie.optOut.url}")
-  lateinit var optOutUrl: String
   private val log: Logger = LoggerFactory.getLogger(javaClass)
   override fun tag(markedResource: MarkedResource, workConfiguration: WorkConfiguration, description: String) {
     markedResource
@@ -56,7 +50,7 @@ class ResourceEntityTagger(
           tags = listOf(
             EntityTag(
               namespace = "swabbie:${workConfiguration.namespace.toLowerCase()}",
-              value = TagValue(message = NotificationMessage.body(MessageType.TAG, clock, optOutUrl, markedResource))
+              value = TagValue(message = tagMessage(markedResource))
             )
           ),
           application = FriggaReflectiveNamer().deriveMoniker(markedResource).app,
@@ -67,6 +61,16 @@ class ResourceEntityTagger(
       }
   }
 
+  private fun tagMessage(markedResource: MarkedResource): String =
+    markedResource.summaries.joinToString(", ") {
+      it.description
+    }.let { summary ->
+        val time = markedResource.humanReadableDeletionTime(clock)
+        "Scheduled to be cleaned up on $time<br /> \n " +
+          "* $summary <br /> \n" +
+          "* Click <a href='${swabbieProperties.optOutBaseUrl}' target='_blank'>here</a> to keep the it for 2 additional weeks."
+      }
+
   private fun supportedForResource(markedResource: MarkedResource): Boolean =
     markedResource.resourceType in SUPPORTED_RESOURCE_TYPES
       && FriggaReflectiveNamer().deriveMoniker(markedResource)?.app != null

Original file line number	Diff line number	Diff line change
`@@ -22,6 +22,8 @@ import org.springframework.boot.context.properties.ConfigurationProperties`
`22`	`22`	`open class SwabbieProperties {`
`23`	`23`	`var dryRun: Boolean = true`
`24`	`24`	`var providers: List<CloudProviderConfiguration> = mutableListOf()`
	`25`	`+ var optOutBaseUrl: String = ""`
	`26`	`+ var spinnakerResourceSearchUrl: String = ""`
`25`	`27`	`}`
`26`	`28`
`27`	`29`	`class CloudProviderConfiguration {`
`@@ -30,9 +32,11 @@ class CloudProviderConfiguration {`
`30`	`32`	`var locations: List<String> = mutableListOf()`
`31`	`33`	`var accounts: List<String> = mutableListOf()`
`32`	`34`	`var resourceTypes: List<ResourceTypeConfiguration> = mutableListOf()`
	`35`	`+ var resourcesPerNotification: Int = 10`
`33`	`36`	`override fun toString(): String {`
`34`		`- return "CloudProviderConfiguration(exclusions=$exclusions, name='$name', locations=$locations, accounts=$accounts, resourceTypes=$resourceTypes)"`
	`37`	`+ return "CloudProviderConfiguration(exclusions=$exclusions, name='$name', locations=$locations, accounts=$accounts, resourceTypes=$resourceTypes, resourcesPerNotification=$resourcesPerNotification)"`
`35`	`38`	`}`
	`39`	`+`
`36`	`40`	`}`
`37`	`41`
`38`	`42`	`class Exclusion {`
Original file line number	Diff line number	Diff line change
`@@ -126,3 +126,4 @@ class ResourceStateManager(`
`126`	`126`	`}`
`127`	`127`
`128`	`128`	`internal fun MarkedResource.typeAndName(): String = this.resourceType.split("(?=[A-Z])".toRegex()).joinToString(" ") + ": " + this.name`
	`129`	`+internal fun String.formatted(): String = this.split("(?=[A-Z])".toRegex()).joinToString(" ").toLowerCase()`
Original file line number	Diff line number	Diff line change
`@@ -286,8 +286,7 @@ object ResourceTypeHandlerTest {`
`286`	`286`	`cloudProvider = TEST_RESOURCE_PROVIDER_TYPE,`
`287`	`287`	`retentionDays = 14,`
`288`	`288`	`dryRun = dryRun,`
`289`		`- exclusions = exclusions,`
`290`		`- notifyOwner = notifyOwner`
	`289`	`+ exclusions = exclusions`
`291`	`290`	`)`
`292`	`291`
`293`	`292`	`class TestRule(`