From 75231ea61c33744bce912997662254bc7e6097ec Mon Sep 17 00:00:00 2001 From: Luis Pollo <1323478+luispollo@users.noreply.github.com> Date: Sun, 24 Jan 2021 17:41:28 -0800 Subject: [PATCH] fix(diff): Workaround for diff library's SecurityGroup.inboundRules (#1759) --- .../keel/api/ec2/SecurityGroupRule.kt | 20 ++- .../keel/api/ec2/SecurityGroupDiffTests.kt | 145 ++++++++++++++++++ 2 files changed, 164 insertions(+), 1 deletion(-) create mode 100644 keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupDiffTests.kt diff --git a/keel-ec2-api/src/main/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupRule.kt b/keel-ec2-api/src/main/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupRule.kt index 1410689679..4a160a25f1 100644 --- a/keel-ec2-api/src/main/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupRule.kt +++ b/keel-ec2-api/src/main/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupRule.kt @@ -49,7 +49,25 @@ data class CidrRule( val blockRange: String, @get:ExcludedFromDiff val description: String? = null -) : SecurityGroupRule() +) : SecurityGroupRule() { + // DO NOT REMOVE! Required due to https://github.com/SQiShER/java-object-diff/issues/216 + override fun equals(other: Any?): Boolean { + if (this === other) return true + + return other is CidrRule + && other.protocol == this.protocol + && other.portRange == this.portRange + && other.blockRange == this.blockRange + } + + // DO NOT REMOVE! Required due to https://github.com/SQiShER/java-object-diff/issues/216 + override fun hashCode(): Int { + var result = protocol.hashCode() + result = 31 * result + portRange.hashCode() + result = 31 * result + blockRange.hashCode() + return result + } +} sealed class IngressPorts diff --git a/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupDiffTests.kt b/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupDiffTests.kt new file mode 100644 index 0000000000..8f12e82037 --- /dev/null +++ b/keel-ec2-plugin/src/test/kotlin/com/netflix/spinnaker/keel/api/ec2/SecurityGroupDiffTests.kt @@ -0,0 +1,145 @@ +package com.netflix.spinnaker.keel.api.ec2 + +import com.netflix.spinnaker.keel.api.Moniker +import com.netflix.spinnaker.keel.api.ec2.SecurityGroupRule.Protocol.TCP +import com.netflix.spinnaker.keel.diff.DefaultResourceDiff +import de.danielbechler.diff.node.DiffNode +import de.danielbechler.diff.node.DiffNode.State.ADDED +import de.danielbechler.diff.node.DiffNode.State.UNTOUCHED +import dev.minutest.junit.JUnit5Minutests +import dev.minutest.rootContext +import strikt.api.expectThat +import strikt.assertions.isEqualTo +import com.netflix.spinnaker.keel.api.ec2.SecurityGroup.Location as SecurityGroupLocation + +internal class SecurityGroupDiffTests : JUnit5Minutests { + + data class Fixture( + val desired: Map, + val current: Map? = null + ) + + private val referenceRule: ReferenceRule = ReferenceRule( + protocol = TCP, + name = "some-other-sg", + portRange = PortRange( + startPort = 443, + endPort = 443 + ) + ) + + private val crossAccountReferenceRule: CrossAccountReferenceRule = CrossAccountReferenceRule( + protocol = TCP, + name = "some-other-sg", + account = "some-other-account", + vpc = "vpc0", + portRange = PortRange( + startPort = 443, + endPort = 443 + ) + ) + + private val cidrRule: CidrRule = CidrRule( + protocol = TCP, + blockRange = "10.0.0.0/8", + portRange = PortRange( + startPort = 443, + endPort = 443 + ) + ) + + private val Fixture.diff: DefaultResourceDiff> + get() = DefaultResourceDiff(desired, current) + + private val Fixture.state: DiffNode.State + get() = diff.diff.state + + private fun Fixture.withMatchingCurrentState(): Fixture = + Fixture( + desired = desired, + current = desired + ) + + private fun Fixture.withIgnorableDifference(): Fixture = + Fixture( + desired = desired, + current = desired.mapValues { (_, securityGroup) -> + securityGroup.copy(description = "whatever") + } + ) + + private fun Fixture.withIgnorableDifferenceInCidrRule(): Fixture = + Fixture( + desired = desired, + current = desired.mapValues { (_, securityGroup) -> + securityGroup.copy( + inboundRules = securityGroup.inboundRules + .toMutableSet() + .apply { removeIf { it is CidrRule } } + .apply { add(cidrRule.copy(description = "whatever")) } + .toSet() + ) + } + ) + + fun tests() = rootContext { + context("desire security group with all types of inbound rules") { + fixture { + Fixture( + desired = mapOf( + "us-west-2" to + SecurityGroup( + description = "test", + moniker = Moniker( + app = "fnord", + stack = "test" + ), + location = SecurityGroupLocation( + account = "test", + vpc = "vpc0", + region = "us-west-2" + ), + inboundRules = setOf(referenceRule, crossAccountReferenceRule, cidrRule) + ) + ) + ) + } + + context("currently none exist") { + test("there is a diff") { + expectThat(state).isEqualTo(ADDED) + } + } + + context("current security group matches") { + deriveFixture { + withMatchingCurrentState() + } + + test("there is no diff") { + expectThat(state).isEqualTo(UNTOUCHED) + } + } + + context("ignorable difference in current security group") { + deriveFixture { + withIgnorableDifference() + } + + test("there is no diff") { + expectThat(state).isEqualTo(UNTOUCHED) + } + } + + context("ignorable difference in current security group's CIDR rule") { + deriveFixture { + withIgnorableDifferenceInCidrRule() + } + + test("there is no diff") { + expectThat(state).isEqualTo(UNTOUCHED) + } + } + } + } +}