Skip to content

Commit

Permalink
fix(veto): Required load balancer veto needs to match account (#1179)
Browse files Browse the repository at this point in the history 
Fixes #1177
  • Loading branch information
@robfletcher
robfletcher authored May 15, 2020
1 parent 0055435 commit ff52fbc
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 27 deletions.
3 changes: 2 additions & 1 deletion ...2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/api/ec2/ClusterDependencyContainer.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ interface ClusterDependencyContainer {
*/
internal data class RegionalDependency(
val name: String,
val account: String,
val regions: Set<String>
)

Expand Down Expand Up @@ -61,7 +62,7 @@ private fun OverrideableClusterDependencyContainer<*>.dependencyByRegion(fn: (Cl
return listOf(defaults, overrides)
.merge()
.map { (k, v) ->
RegionalDependency(k, v)
RegionalDependency(k, locations.account, v)
}
}

Expand Down
30 changes: 19 additions & 11 deletions ...2-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/vetos/RequiredLoadBalancerVeto.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import com.netflix.spinnaker.keel.api.Resource
import com.netflix.spinnaker.keel.api.ec2.OverrideableClusterDependencyContainer
import com.netflix.spinnaker.keel.api.ec2.loadBalancersByRegion
import com.netflix.spinnaker.keel.api.ec2.targetGroupsByRegion
import com.netflix.spinnaker.keel.clouddriver.CloudDriverCache
import com.netflix.spinnaker.keel.clouddriver.CloudDriverService
import com.netflix.spinnaker.keel.clouddriver.model.AmazonLoadBalancer
import com.netflix.spinnaker.keel.clouddriver.model.ApplicationLoadBalancerModel
Expand All @@ -18,7 +19,8 @@ import org.springframework.stereotype.Component

@Component
class RequiredLoadBalancerVeto(
private val cloudDriver: CloudDriverService
private val cloudDriver: CloudDriverService,
private val cloudDriverCache: CloudDriverCache
) : Veto {
private val log by lazy { LoggerFactory.getLogger(javaClass) }

Expand Down Expand Up @@ -57,25 +59,30 @@ class RequiredLoadBalancerVeto(

private suspend fun List<AmazonLoadBalancer>.findMissingLoadBalancers(spec: OverrideableClusterDependencyContainer<*>): Collection<MissingDependency> {
val missing = mutableListOf<MissingDependency>()
spec.loadBalancersByRegion.forEach { (name, regions) ->
spec.loadBalancersByRegion.forEach { (name, account, regions) ->
val missingRegions = regions.filter { region ->
none { lb -> lb is ClassicLoadBalancerModel && lb.region == region && lb.loadBalancerName == name }
none { lb ->
lb is ClassicLoadBalancerModel && lb.account == account && lb.region == region && lb.loadBalancerName == name
}
}
if (missingRegions.isNotEmpty()) {
missing.add(MissingLoadBalancer(name, missingRegions.toSet()))
missing.add(MissingLoadBalancer(name, account, missingRegions.toSet()))
}
}
spec.targetGroupsByRegion.forEach { (name, regions) ->
spec.targetGroupsByRegion.forEach { (name, account, regions) ->
val missingRegions = regions.filter { region ->
none { lb -> lb is ApplicationLoadBalancerModel && lb.region == region && lb.targetGroups.any { it.targetGroupName == name } }
none { lb -> lb is ApplicationLoadBalancerModel && lb.account == account && lb.region == region && lb.targetGroups.any { it.targetGroupName == name } }
}
if (missingRegions.isNotEmpty()) {
missing.add(MissingTargetGroup(name, missingRegions.toSet()))
missing.add(MissingTargetGroup(name, account, missingRegions.toSet()))
}
}
return missing
}

private val AmazonLoadBalancer.account: String
get() = cloudDriverCache.networkBy(vpcId).account

/**
* I'd like to add a `region` property to [AmazonLoadBalancer] but CloudDriver doesn't
* consistently return it from all the endpoints we use.
Expand All @@ -86,17 +93,18 @@ class RequiredLoadBalancerVeto(

sealed class MissingDependency(
val name: String,
val account: String,
val regions: Set<String>
) {
abstract val message: String
}

class MissingLoadBalancer(name: String, regions: Set<String>) : MissingDependency(name, regions) {
class MissingLoadBalancer(name: String, account: String, regions: Set<String>) : MissingDependency(name, account, regions) {
override val message: String
get() = "Load balancer $name is not found in ${regions.joinToString()}"
get() = "Load balancer $name is not found in $account / ${regions.joinToString()}"
}

class MissingTargetGroup(name: String, regions: Set<String>) : MissingDependency(name, regions) {
class MissingTargetGroup(name: String, account: String, regions: Set<String>) : MissingDependency(name, account, regions) {
override val message: String
get() = "Target group $name is not found in ${regions.joinToString()}"
get() = "Target group $name is not found in $account / ${regions.joinToString()}"
}
6 changes: 3 additions & 3 deletions ...-plugin/src/main/kotlin/com/netflix/spinnaker/keel/ec2/vetos/RequiredSecurityGroupVeto.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ class RequiredSecurityGroupVeto(
val jobs = mutableListOf<Job>()
val securityGroupMissingRegions = mutableMapOf<String, MutableList<String>>()
supervisorScope {
spec.securityGroupsInRegions.forEach { (securityGroupName, regions) ->
spec.securityGroupsInRegions.forEach { (securityGroupName, account, regions) ->
regions.forEach { region ->
launch {
if (!securityGroupExists(spec.account, spec.subnet, region, securityGroupName)) {
if (!securityGroupExists(account, spec.subnet, region, securityGroupName)) {
with(securityGroupMissingRegions) {
putIfAbsent(securityGroupName, mutableListOf())
getValue(securityGroupName).add(region)
Expand Down Expand Up @@ -100,7 +100,7 @@ class RequiredSecurityGroupVeto(
is LoadBalancerSpec ->
SecurityGroupDependencies(
securityGroupsInRegions = dependencies.securityGroupNames.map {
RegionalDependency(it, locations.regions.map(SubnetAwareRegionSpec::name).toSet())
RegionalDependency(it, locations.account, locations.regions.map(SubnetAwareRegionSpec::name).toSet())
},
account = locations.account,
subnet = locations.subnet
Expand Down
74 changes: 62 additions & 12 deletions ...gin/src/test/kotlin/com/netflix/spinnaker/keel/ec2/vetos/RequiredLoadBalancerVetoTests.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@ import com.netflix.spinnaker.keel.api.SubnetAwareRegionSpec
import com.netflix.spinnaker.keel.api.ec2.ClusterSpec
import com.netflix.spinnaker.keel.api.ec2.ClusterSpec.ServerGroupSpec
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.clouddriver.model.ApplicationLoadBalancerModel
import com.netflix.spinnaker.keel.clouddriver.model.ApplicationLoadBalancerModel.TargetGroup
import com.netflix.spinnaker.keel.clouddriver.model.ApplicationLoadBalancerModel.TargetGroupAttributes
import com.netflix.spinnaker.keel.clouddriver.model.ApplicationLoadBalancerModel.TargetGroupMatcher
import com.netflix.spinnaker.keel.clouddriver.model.ClassicLoadBalancerModel
import com.netflix.spinnaker.keel.clouddriver.model.ClassicLoadBalancerModel.ClassicLoadBalancerHealthCheck
import com.netflix.spinnaker.keel.clouddriver.model.Network
import com.netflix.spinnaker.keel.core.api.ClusterDependencies
import com.netflix.spinnaker.keel.core.api.DEFAULT_SERVICE_ACCOUNT
import com.netflix.spinnaker.keel.ec2.EC2_SECURITY_GROUP_V1
Expand All @@ -42,14 +44,20 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
data class Fixture<out SPEC : ResourceSpec>(
val resourceKind: ResourceKind,
val resourceSpec: SPEC,
val cloudDriver: CloudDriverService = mockk()
val cloudDriver: CloudDriverService = mockk(),
val cloudDriverCache: CloudDriverCache = mockk()
) {
val resource: Resource<SPEC>
get() = resource(resourceKind, resourceSpec)

val vpcId = "vpc-5318008"
val vpcIds = mapOf(
("prod" to "ap-south-1") to "vpc-1111111",
("prod" to "af-south-1") to "vpc-2222222",
("test" to "ap-south-1") to "vpc-3333333",
("test" to "af-south-1") to "vpc-4444444"
)

private val veto = RequiredLoadBalancerVeto(cloudDriver)
private val veto = RequiredLoadBalancerVeto(cloudDriver, cloudDriverCache)

private lateinit var vetoResponse: VetoResponse

Expand Down Expand Up @@ -141,10 +149,20 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
)
)
),
cloudDriver = cloudDriver
// mocks need to be copied over otherwise any stubbing is lost
cloudDriver = cloudDriver,
cloudDriverCache = cloudDriverCache
)
}

before {
every { cloudDriverCache.networkBy(any()) } answers {
val vpcId = firstArg<String>()
val (account, region) = vpcIds.filterValues { it == vpcId }.keys.first()
Network("aws", vpcId, null, account, region)
}
}

context("the dependencies exist") {
before {
stubLoadBalancers(resourceSpec.allLoadBalancerNames, resourceSpec.allTargetGroupNames)
Expand Down Expand Up @@ -178,10 +196,10 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
.isNotNull()
.and {
lbNames.forEach { lb ->
contains("Load balancer $lb is not found in ${missingRegions.joinToString()}")
contains("Load balancer $lb is not found in ${resourceSpec.locations.account} / ${missingRegions.joinToString()}")
}
tgNames.forEach { tg ->
contains("Target group $tg is not found in ${missingRegions.joinToString()}")
contains("Target group $tg is not found in ${resourceSpec.locations.account} / ${missingRegions.joinToString()}")
}
}
}
Expand All @@ -192,7 +210,7 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
stubLoadBalancers(
resourceSpec.allLoadBalancerNames,
resourceSpec.allTargetGroupNames,
resourceSpec.locations.regions.take(1).map(SubnetAwareRegionSpec::name)
regions = resourceSpec.locations.regions.take(1).map(SubnetAwareRegionSpec::name)
)
check()
}
Expand All @@ -213,10 +231,41 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
.isNotNull()
.and {
lbNames.forEach { lb ->
contains("Load balancer $lb is not found in ${missingRegions.joinToString()}")
contains("Load balancer $lb is not found in ${resourceSpec.locations.account} / ${missingRegions.joinToString()}")
}
tgNames.forEach { tg ->
contains("Target group $tg is not found in ${resourceSpec.locations.account} / ${missingRegions.joinToString()}")
}
}
}
}

context("the dependencies exist but in a different account") {
before {
stubLoadBalancers(
resourceSpec.allLoadBalancerNames,
resourceSpec.allTargetGroupNames,
account = "test"
)
check()
}

test("the resource is vetoed") {
response.isDenied()
}

test("the veto message specifies the missing resources") {
val lbNames = resourceSpec.allLoadBalancerNames
val tgNames = resourceSpec.allTargetGroupNames
val allRegions = resourceSpec.locations.regions.map(SubnetAwareRegionSpec::name)
response.message
.isNotNull()
.and {
lbNames.forEach { lb ->
contains("Load balancer $lb is not found in ${resourceSpec.locations.account} / ${allRegions.joinToString()}")
}
tgNames.forEach { tg ->
contains("Target group $tg is not found in ${missingRegions.joinToString()}")
contains("Target group $tg is not found in ${resourceSpec.locations.account} / ${allRegions.joinToString()}")
}
}
}
Expand All @@ -232,6 +281,7 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
private fun Fixture<Locatable<SubnetAwareLocations>>.stubLoadBalancers(
loadBalancerNames: Collection<String>,
targetGroupNames: Collection<String>,
account: String = this.resourceSpec.locations.account,
regions: Collection<String> = this.resourceSpec.locations.regions.map(SubnetAwareRegionSpec::name)
) {
every {
Expand All @@ -246,7 +296,7 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
moniker = Moniker("fnord", "elb"), // TODO: parse from name
loadBalancerName = loadBalancerName,
availabilityZones = setOf("a", "b", "c").map { "$region$it" }.toSet(),
vpcId = vpcId,
vpcId = vpcIds.getValue(account to region),
subnets = setOf(),
scheme = "internal",
idleTimeout = 0,
Expand Down Expand Up @@ -281,12 +331,12 @@ internal class RequiredLoadBalancerVetoTests : JUnit5Minutests {
healthCheckIntervalSeconds = 60,
healthyThresholdCount = 10,
unhealthyThresholdCount = 5,
vpcId = vpcId,
vpcId = vpcIds.getValue(account to region),
attributes = TargetGroupAttributes()
)
},
availabilityZones = setOf("a", "b", "c").map { "$region$it" }.toSet(),
vpcId = vpcId,
vpcId = vpcIds.getValue(account to region),
subnets = emptySet(),
scheme = "https",
idleTimeout = 60,
Expand Down

0 comments on commit ff52fbc

Please sign in to comment.