Skip to content

Commit 9be18df

Browse files
robzienertrobzienert
authored
fix(intent): Refactor security group to only map to a single region (#49)
1 parent d8e756f commit 9be18df

File tree

7 files changed

+234
-192
lines changed

7 files changed

+234
-192
lines changed

‎keel-intent/src/main/kotlin/com/netflix/spinnaker/keel/intent/SecurityGroupIntent.kt

+3-6
Original file line numberDiff line numberDiff line change
@@ -35,17 +35,15 @@ class SecurityGroupIntent
3535
schema = CURRENT_SCHEMA,
3636
spec = spec
3737
) {
38-
// TODO rz - Region needs to be included in the id, but we also need a way to supercede other intents if a region is
39-
// added or removed.
40-
@JsonIgnore override val defaultId = "$KIND:${spec.cloudProvider}:${spec.accountName}:${spec.name}"
38+
@JsonIgnore override val defaultId = "$KIND:${spec.cloudProvider}:${spec.accountName}:${spec.region}:${spec.name}"
4139
}
4240

4341
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, include = JsonTypeInfo.As.PROPERTY, property = "kind")
4442
abstract class SecurityGroupSpec : ApplicationAwareIntentSpec() {
4543
abstract val name: String
4644
abstract val cloudProvider: String
4745
abstract val accountName: String
48-
abstract val regions: Set<String>
46+
abstract val region: String
4947
abstract val inboundRules: Set<SecurityGroupRule>
5048
}
5149

@@ -55,7 +53,7 @@ data class AmazonSecurityGroupSpec(
5553
override val name: String,
5654
override val cloudProvider: String,
5755
override val accountName: String,
58-
override val regions: Set<String>,
56+
override val region: String,
5957
override val inboundRules: Set<SecurityGroupRule>,
6058
val outboundRules: Set<SecurityGroupRule>,
6159
// We don't care to support EC2 Classic, but for some reason clouddriver returns nulls (and isn't "default" vpcs)
@@ -110,7 +108,6 @@ data class CrossAccountReferenceSecurityGroupRule(
110108
override val protocol: String,
111109
override val name: String,
112110
val account: String,
113-
val region: String, // TODO rz - remove; can only x-account to same region
114111
val vpcName: String
115112
) : SecurityGroupRule(), PortRangeSupport
116113

‎keel-intent/src/main/kotlin/com/netflix/spinnaker/keel/intent/processor/SecurityGroupIntentProcessor.kt

+44-66
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ class SecurityGroupIntentProcessor
7878
return ConvergeResult(listOf(), changeSummary)
7979
}
8080

81-
changeSummary.type = if (currentState.isEmpty()) ChangeType.CREATE else ChangeType.UPDATE
81+
changeSummary.type = if (currentState == null) ChangeType.CREATE else ChangeType.UPDATE
8282

8383
return ConvergeResult(listOf(
8484
OrchestrationRequest(
@@ -93,65 +93,45 @@ class SecurityGroupIntentProcessor
9393
)
9494
}
9595

96-
private fun getSecurityGroups(spec: SecurityGroupSpec): Set<SecurityGroup> {
96+
private fun getSecurityGroups(spec: SecurityGroupSpec): SecurityGroup? {
9797
if (spec is AmazonSecurityGroupSpec) {
98-
return spec.regions
99-
.map { region ->
100-
try {
101-
return@map if (spec.vpcName == null) {
102-
cloudDriverService.getSecurityGroup(spec.accountName, "aws", spec.name, region)
103-
} else {
104-
cloudDriverService.getSecurityGroup(spec.accountName, "aws", spec.name, region, clouddriverCache.networkBy(
105-
spec.vpcName,
106-
spec.accountName,
107-
region
108-
).id)
109-
}
110-
} catch (e: RetrofitError) {
111-
if (e.notFound()) {
112-
return@map null
113-
}
114-
throw e
115-
}
98+
try {
99+
return if (spec.vpcName == null) {
100+
cloudDriverService.getSecurityGroup(spec.accountName, "aws", spec.name, spec.region)
101+
} else {
102+
cloudDriverService.getSecurityGroup(spec.accountName, "aws", spec.name, spec.region, clouddriverCache.networkBy(
103+
spec.vpcName,
104+
spec.accountName,
105+
spec.region
106+
).id)
116107
}
117-
.filterNotNull()
118-
.toSet()
108+
} catch (e: RetrofitError) {
109+
if (e.notFound()) {
110+
return null
111+
}
112+
throw e
113+
}
119114
}
120115
throw NotImplementedError("Only amazon security groups are supported at the moment")
121116
}
122117

123118
private fun currentStateUpToDate(intentId: String,
124-
currentState: Set<SecurityGroup>,
119+
currentState: SecurityGroup?,
125120
desiredState: SecurityGroupSpec,
126121
changeSummary: ChangeSummary): Boolean {
127122

128-
if (currentState.size != desiredState.regions.size) return false
129-
130-
val desired = securityGroupConverter.convertToState(desiredState)
131-
132-
val statePairs = mutableListOf<Pair<SecurityGroup, SecurityGroup>>()
133-
desired.forEach { d ->
134-
val key = "${d.accountName}/${d.region}/${d.name}"
135-
currentState.forEach { c ->
136-
if ("${c.accountName}/${c.region}/${c.name}" == key) {
137-
statePairs.add(Pair(c, d))
138-
}
139-
}
123+
if (currentState == null) {
124+
return false
140125
}
141126

142-
if (statePairs.size != desiredState.regions.size) return false
143-
144-
val stateInspector = StateInspector(objectMapper)
145-
val diff = statePairs.flatMap {
146-
stateInspector.getDiff(
147-
intentId = intentId,
148-
currentState = it.first,
149-
desiredState = it.second,
150-
modelClass = SecurityGroup::class,
151-
specClass = SecurityGroupSpec::class,
152-
ignoreKeys = setOf("type", "id", "moniker", "summary", "description")
153-
)
154-
}.toSet()
127+
val diff = StateInspector(objectMapper).getDiff(
128+
intentId = intentId,
129+
currentState = currentState,
130+
desiredState = securityGroupConverter.convertToState(desiredState),
131+
modelClass = SecurityGroup::class,
132+
specClass = SecurityGroupSpec::class,
133+
ignoreKeys = setOf("type", "id", "moniker", "summary", "description")
134+
)
155135
changeSummary.diff = diff
156136
return diff.isEmpty()
157137
}
@@ -162,27 +142,25 @@ class SecurityGroupIntentProcessor
162142
.filter {
163143
spec.name != it.name
164144
}
165-
.flatMap {
166-
spec.regions.map { region ->
167-
if (spec is AmazonSecurityGroupSpec) {
168-
try {
169-
cloudDriverService.getSecurityGroup(
170-
spec.accountName,
171-
"aws",
172-
it.name,
173-
region,
174-
clouddriverCache.networkBy(spec.vpcName!!, spec.accountName, region).id
175-
)
176-
} catch (e: RetrofitError) {
177-
if (e.notFound()) {
178-
return@map it.name
179-
}
145+
.map {
146+
if (spec is AmazonSecurityGroupSpec) {
147+
try {
148+
cloudDriverService.getSecurityGroup(
149+
spec.accountName,
150+
"aws",
151+
it.name,
152+
spec.region,
153+
clouddriverCache.networkBy(spec.vpcName!!, spec.accountName, spec.region).id
154+
)
155+
} catch (e: RetrofitError) {
156+
if (e.notFound()) {
157+
return@map it.name
180158
}
181-
} else {
182-
log.error("${spec.javaClass.simpleName} is not supported yet")
183159
}
184-
return@map null
160+
} else {
161+
log.error("${spec.javaClass.simpleName} is not supported yet")
185162
}
163+
return@map null
186164
}
187165
.filterNotNull()
188166
.distinct()

‎keel-intent/src/main/kotlin/com/netflix/spinnaker/keel/intent/processor/converter/SecurityGroupConverter.kt

+51-60
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ import javax.annotation.PostConstruct
3636
class SecurityGroupConverter(
3737
private val clouddriverCache: CloudDriverCache,
3838
private val objectMapper: ObjectMapper
39-
) : SpecConverter<SecurityGroupSpec, Set<SecurityGroup>> {
39+
) : SpecConverter<SecurityGroupSpec, SecurityGroup> {
4040

4141
private val log = LoggerFactory.getLogger(javaClass)
4242

@@ -46,70 +46,61 @@ class SecurityGroupConverter(
4646
)
4747
}
4848

49-
override fun convertToState(spec: SecurityGroupSpec): Set<SecurityGroup> {
49+
override fun convertToState(spec: SecurityGroupSpec): SecurityGroup {
5050
if (spec is AmazonSecurityGroupSpec) {
51-
return spec.regions.map { region ->
52-
SecurityGroup(
53-
type = "aws",
54-
name = spec.name,
55-
description = spec.description,
56-
accountName = spec.accountName,
57-
region = region,
58-
// TODO rz - do we even want to mess with EC2-classic support?
59-
vpcId = clouddriverCache.networkBy(spec.vpcName!!, spec.accountName, region).id,
60-
inboundRules = spec.inboundRules.map {
61-
when (it) {
62-
is ReferenceSecurityGroupRule -> SecurityGroup.SecurityGroupRule(
63-
protocol = it.protocol,
64-
portRanges = it.portRanges.map { SecurityGroup.SecurityGroupRulePortRange(it.startPort, it.endPort) },
65-
securityGroup = SecurityGroup.SecurityGroupRuleReference(
66-
name = it.name,
67-
accountName = spec.accountName,
68-
region = region
69-
)
51+
return SecurityGroup(
52+
type = "aws",
53+
name = spec.name,
54+
description = spec.description,
55+
accountName = spec.accountName,
56+
region = spec.region,
57+
// TODO rz - do we even want to mess with EC2-classic support?
58+
vpcId = clouddriverCache.networkBy(spec.vpcName!!, spec.accountName, spec.region).id,
59+
inboundRules = spec.inboundRules.map {
60+
when (it) {
61+
is ReferenceSecurityGroupRule -> SecurityGroup.SecurityGroupRule(
62+
protocol = it.protocol,
63+
portRanges = it.portRanges.map { SecurityGroup.SecurityGroupRulePortRange(it.startPort, it.endPort) },
64+
securityGroup = SecurityGroup.SecurityGroupRuleReference(
65+
name = it.name,
66+
accountName = spec.accountName,
67+
region = spec.region
7068
)
71-
is CrossAccountReferenceSecurityGroupRule -> SecurityGroup.SecurityGroupRule(
72-
protocol = it.protocol,
73-
portRanges = it.portRanges.map { SecurityGroup.SecurityGroupRulePortRange(it.startPort, it.endPort) },
74-
securityGroup = SecurityGroup.SecurityGroupRuleReference(
75-
name = it.name,
76-
accountName = it.account,
77-
region = it.region
78-
)
69+
)
70+
is CrossAccountReferenceSecurityGroupRule -> SecurityGroup.SecurityGroupRule(
71+
protocol = it.protocol,
72+
portRanges = it.portRanges.map { SecurityGroup.SecurityGroupRulePortRange(it.startPort, it.endPort) },
73+
securityGroup = SecurityGroup.SecurityGroupRuleReference(
74+
name = it.name,
75+
accountName = it.account,
76+
region = spec.region
7977
)
80-
else -> TODO(reason = "${it.javaClass.simpleName} has not been implemented yet")
81-
}
82-
}.toSet(),
83-
id = null,
84-
// TODO rz - fix so not bad
85-
moniker = Moniker(spec.name)
86-
)
87-
}.toSet()
78+
)
79+
else -> TODO(reason = "${it.javaClass.simpleName} has not been implemented yet")
80+
}
81+
}.toSet(),
82+
id = null,
83+
moniker = Moniker(spec.name)
84+
)
8885
}
8986
throw NotImplementedError("Only AWS security groups are supported at the moment")
9087
}
9188

92-
override fun convertFromState(state: Set<SecurityGroup>): SecurityGroupSpec? {
93-
if (state.isEmpty()) {
94-
return null
95-
}
96-
97-
state.first().let {
98-
if (it.type == "aws") {
99-
return AmazonSecurityGroupSpec(
100-
cloudProvider = "aws",
101-
application = it.moniker.app,
102-
name = it.name,
103-
description = it.description!!,
104-
accountName = it.accountName,
105-
regions = state.map { s -> s.region }.toSet(),
106-
vpcName = clouddriverCache.networkBy(it.vpcId!!).name,
107-
inboundRules = it.inboundRules.map {
108-
objectMapper.convertValue(it, SecurityGroupRule::class.java)
109-
}.toSet(),
110-
outboundRules = setOf()
111-
)
112-
}
89+
override fun convertFromState(state: SecurityGroup): SecurityGroupSpec? {
90+
if (state.type == "aws") {
91+
return AmazonSecurityGroupSpec(
92+
cloudProvider = "aws",
93+
application = state.moniker.app,
94+
name = state.name,
95+
description = state.description!!,
96+
accountName = state.accountName,
97+
region = state.region,
98+
vpcName = clouddriverCache.networkBy(state.vpcId!!).name,
99+
inboundRules = state.inboundRules.map {
100+
objectMapper.convertValue(state, SecurityGroupRule::class.java)
101+
}.toSet(),
102+
outboundRules = setOf()
103+
)
113104
}
114105
throw NotImplementedError("Only AWS security groups are supported at the moment")
115106
}
@@ -125,7 +116,7 @@ class SecurityGroupConverter(
125116
"credentials" to spec.accountName,
126117
"cloudProvider" to "aws",
127118
"name" to spec.name,
128-
"regions" to spec.regions,
119+
"regions" to listOf(spec.region),
129120
"vpcId" to spec.vpcName,
130121
"description" to spec.description,
131122
"securityGroupIngress" to spec.inboundRules.flatMap {
@@ -142,7 +133,7 @@ class SecurityGroupConverter(
142133
changeSummary.addMessage("Adding cross account reference support account ${it.account}")
143134
m["accountName"] = it.account
144135
m["crossAccountEnabled"] = true
145-
m["vpcId"] = clouddriverCache.networkBy(it.vpcName, spec.accountName, it.region)
136+
m["vpcId"] = clouddriverCache.networkBy(it.vpcName, spec.accountName, spec.region)
146137
}
147138
m
148139
}

‎keel-intent/src/main/kotlin/com/netflix/spinnaker/keel/intent/processor/converter/SpecConverter.kt

-2
Original file line numberDiff line numberDiff line change
@@ -25,5 +25,3 @@ interface SpecConverter<I : IntentSpec, S : Any> {
2525
fun convertFromState(state: S): I?
2626
fun convertToJob(spec: I, changeSummary: ChangeSummary): List<Job>
2727
}
28-
29-
const val COMPUTED_VALUE = "<computed>"

0 commit comments

Comments
 (0)