From 84644734dc6226783faf09fcacc88434dd10c261 Mon Sep 17 00:00:00 2001
From: Rob Fletcher <robfletcher@users.noreply.github.com>
Date: Tue, 23 Feb 2021 11:04:09 -0800
Subject: [PATCH] feat(securitygroups): merge override inbound rules instead of
 replacing (#1787)

---
 .../keel/ec2/resource/SecurityGroupHandler.kt |  2 +-
 .../resource/SecurityGroupOverrideTests.kt    | 85 +++++++++++++++++++
 2 files changed, 86 insertions(+), 1 deletion(-)
 create mode 100644 keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupOverrideTests.kt

diff --git a/keel-ec2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupHandler.kt b/keel-ec2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupHandler.kt
index 44e00131ef..fa0546777e 100644
--- a/keel-ec2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupHandler.kt
+++ b/keel-ec2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupHandler.kt
@@ -73,7 +73,7 @@ open class SecurityGroupHandler(
             region = region.name
           ),
           description = overrides[region.name]?.description ?: description,
-          inboundRules = overrides[region.name]?.inboundRules ?: inboundRules
+          inboundRules = (overrides[region.name]?.inboundRules ?: emptySet()) + inboundRules
         )
       }.toMap()
     }
diff --git a/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupOverrideTests.kt b/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupOverrideTests.kt
new file mode 100644
index 0000000000..f26637df5c
--- /dev/null
+++ b/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/ec2/resource/SecurityGroupOverrideTests.kt
@@ -0,0 +1,85 @@
+package com.netflix.spinnaker.keel.ec2.resource
+
+import com.netflix.spinnaker.keel.api.Moniker
+import com.netflix.spinnaker.keel.api.SimpleLocations
+import com.netflix.spinnaker.keel.api.SimpleRegionSpec
+import com.netflix.spinnaker.keel.api.actuation.TaskLauncher
+import com.netflix.spinnaker.keel.api.ec2.AllPorts
+import com.netflix.spinnaker.keel.api.ec2.EC2_SECURITY_GROUP_V1
+import com.netflix.spinnaker.keel.api.ec2.ReferenceRule
+import com.netflix.spinnaker.keel.api.ec2.SecurityGroupOverride
+import com.netflix.spinnaker.keel.api.ec2.SecurityGroupRule.Protocol.TCP
+import com.netflix.spinnaker.keel.api.ec2.SecurityGroupSpec
+import com.netflix.spinnaker.keel.clouddriver.CloudDriverCache
+import com.netflix.spinnaker.keel.clouddriver.CloudDriverService
+import com.netflix.spinnaker.keel.orca.OrcaService
+import com.netflix.spinnaker.keel.test.resource
+import io.mockk.mockk
+import kotlinx.coroutines.runBlocking
+import org.junit.jupiter.api.Test
+import strikt.api.expect
+import strikt.assertions.hasSize
+import strikt.assertions.isNotNull
+
+class SecurityGroupOverrideTests {
+  val cloudDriverService = mockk<CloudDriverService>()
+  val cloudDriverCache = mockk<CloudDriverCache>()
+  val orcaService = mockk<OrcaService>()
+  val taskLauncher = mockk<TaskLauncher>()
+
+  val securityGroupHandler = SecurityGroupHandler(
+    cloudDriverService = cloudDriverService,
+    cloudDriverCache = cloudDriverCache,
+    orcaService = orcaService,
+    taskLauncher = taskLauncher,
+    resolvers = emptyList()
+  )
+
+  @Test
+  fun `can merge ingress rules specified in overrides`() {
+    val spec = SecurityGroupSpec(
+      moniker = Moniker(
+        app = "fnord"
+      ),
+      locations = SimpleLocations(
+        account = "test",
+        regions = setOf(
+          SimpleRegionSpec(
+            name = "uk-east-17"
+          ),
+          SimpleRegionSpec(
+            name = "ap-south-1"
+          )
+        )
+      ),
+      description = "catflap rubberplant marzipan",
+      inboundRules = setOf(
+        ReferenceRule(
+          protocol = TCP,
+          name = "common",
+          portRange = AllPorts
+        )
+      ),
+      overrides = mapOf(
+        "uk-east-17" to SecurityGroupOverride(
+          inboundRules = setOf(
+            ReferenceRule(
+              protocol = TCP,
+              name = "one-region-only",
+              portRange = AllPorts
+            )
+          )
+        )
+      )
+    )
+
+    val materialized = runBlocking {
+      securityGroupHandler.desired(resource(kind = EC2_SECURITY_GROUP_V1.kind, spec = spec))
+    }
+
+    expect {
+      that(materialized["uk-east-17"]?.inboundRules).isNotNull().hasSize(2)
+      that(materialized["ap-south-1"]?.inboundRules).isNotNull().hasSize(1)
+    }
+  }
+}