fix(aws): Update diffing logic for AWS upsertSecurityGroupOperation (#…

…4300) * fix(aws): Update diffing logic while upsertSecurityGroupOperation * adding UT and address PR comments * fixup based on PR feedback * fixup for matching logic Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
spinnaker · Feb 5, 2020 · 9f2c141 · 9f2c141
1 parent 16f3d5d
commit 9f2c141
Show file tree

Hide file tree

Showing 4 changed files with 240 additions and 14 deletions.
diff --git a/...x/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupIngressConverter.groovy b/...x/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupIngressConverter.groovy
@@ -75,17 +75,17 @@ class SecurityGroupIngressConverter {
       if (ingress.id) {
         newUserIdGroupPair = new UserIdGroupPair(userId: accountId, groupId: ingress.id, vpcId: ingress.vpcId)
       } else {
-          final ingressSecurityGroup = securityGroupLookup.getSecurityGroupByName(accountName, ingress.name, vpcId)
-          if (ingressSecurityGroup.present) {
-            final groupId = ingressSecurityGroup.get().getSecurityGroup().groupId
-            newUserIdGroupPair = new UserIdGroupPair(userId: accountId, groupId: groupId, vpcId: ingress.vpcId)
+        final ingressSecurityGroup = securityGroupLookup.getSecurityGroupByName(accountName, ingress.name, vpcId)
+        if (ingressSecurityGroup.present) {
+          final groupId = ingressSecurityGroup.get().getSecurityGroup().groupId
+          newUserIdGroupPair = new UserIdGroupPair(userId: accountId, groupId: groupId, vpcId: ingress.vpcId)
+        } else {
+          if (description.vpcId) {
+            missing.add(ingress)
           } else {
-            if (description.vpcId) {
-              missing.add(ingress)
-            } else {
-              newUserIdGroupPair = new UserIdGroupPair(userId: accountId, groupName: ingress.name)
-            }
+            newUserIdGroupPair = new UserIdGroupPair(userId: accountId, groupName: ingress.name)
           }
+        }
       }
 
       if (newUserIdGroupPair) {
@@ -107,21 +107,18 @@ class SecurityGroupIngressConverter {
         it.groupName = null
         it.peeringStatus = null
         it.vpcPeeringConnectionId = null
-        it.description = null // not passed in via the UI
         new IpPermission()
           .withFromPort(ipPermission.fromPort)
           .withToPort(ipPermission.toPort)
           .withIpProtocol(ipPermission.ipProtocol)
           .withUserIdGroupPairs(it)
       } + ipPermission.ipv4Ranges.collect {
-        it.description = null // not passed in via the UI
         new IpPermission()
           .withFromPort(ipPermission.fromPort)
           .withToPort(ipPermission.toPort)
           .withIpProtocol(ipPermission.ipProtocol)
           .withIpv4Ranges(it)
       } + ipPermission.ipv6Ranges.collect {
-        it.description = null // not passed in via the UI
         new IpPermission()
           .withFromPort(ipPermission.fromPort)
           .withToPort(ipPermission.toPort)
@@ -130,4 +127,65 @@ class SecurityGroupIngressConverter {
       }
     }.flatten().unique()
   }
+
+  /**
+   *
+   * @param newList from description
+   * @param existingRules
+   * @return Map of rules that needs to be added , removed and updated
+   * Computes the delta between the existing rules and new rule
+   * Any rule present in description and not in the existing rule gets added to addition list.
+   * Any rule in description but present in existing rule get added to the remove list.
+   * Any rule with a change in description only gets added to the update list based on the following,
+   * - If a new rule has description value add it to update list to make it consistent.
+   * - If new rule has no description value set, ignore.
+   */
+  static IpRuleDelta computeIpRuleDelta(List<IpPermission> newList, List<IpPermission> existingRules) {
+    List<IpPermission> tobeAdded = new ArrayList<>()
+    List<IpPermission> tobeRemoved = new ArrayList<>()
+    List<IpPermission> tobeUpdated = new ArrayList<>()
+    List<IpPermission> filteredNewList = newList.findAll { ipPermission -> ipPermission.userIdGroupPairs.isEmpty() }
+    List<IpPermission> filteredExistingRuleList = existingRules.findAll { existingRule -> existingRule.userIdGroupPairs.isEmpty()}
+    filteredNewList.forEach({ newListEntry ->
+      IpPermission match = findIpPermission(filteredExistingRuleList, newListEntry)
+      if (match) {
+        if (newListEntry.ipv4Ranges.collect { it.description }.any()
+          || newListEntry.ipv6Ranges.collect { it.description }.any()) {
+          tobeUpdated.add(newListEntry) // matches old rule , needs an update for description
+        }
+        filteredExistingRuleList.remove(match) // remove from future processing
+      } else {
+        tobeAdded.add(newListEntry) //no match in old rule so must be added
+      }
+    })
+    tobeRemoved = filteredExistingRuleList // rules that needs to be removed
+    return new IpRuleDelta(tobeAdded, tobeRemoved , tobeUpdated)
+  }
+
+  static IpPermission findIpPermission(List<IpPermission> existingList, IpPermission ipPermission) {
+    existingList.find { it ->
+      (((it.ipv4Ranges.collect { it.cidrIp }.sort() == ipPermission.ipv4Ranges.collect { it.cidrIp }.sort()
+        && it.fromPort == ipPermission.fromPort
+        && it.toPort == ipPermission.toPort
+        && it.ipProtocol == ipPermission.ipProtocol) && !ipPermission.ipv4Ranges.isEmpty())
+        || ((it.ipv6Ranges.collect { it.cidrIpv6 }.sort() == ipPermission.ipv6Ranges.collect { it.cidrIpv6 }.sort()
+        && it.fromPort == ipPermission.fromPort
+        && it.toPort == ipPermission.toPort
+        && it.ipProtocol == ipPermission.ipProtocol) && !ipPermission.ipv6Ranges.isEmpty()))
+    }
+  }
+
+  static List<IpPermission> userIdGroupPairsDiff(List<IpPermission> converted, List<IpPermission> existingIpPermissions) {
+    List<IpPermission> convertedFromDesc = converted.findAll { elements -> elements.userIdGroupPairs.size() != 0 }
+    List<IpPermission> existing = existingIpPermissions.findAll { elements -> elements.userIdGroupPairs.size() != 0 }
+    return convertedFromDesc - existing
+  }
+
+  @Canonical
+  static class IpRuleDelta {
+    List<IpPermission> toAdd
+    List<IpPermission> toRemove
+    List<IpPermission> toUpdate
+  }
+
 }
diff --git a/...flix/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupLookupFactory.groovy b/...flix/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupLookupFactory.groovy
@@ -30,6 +30,7 @@ import com.amazonaws.services.ec2.model.SecurityGroup
 import com.amazonaws.services.ec2.model.Tag
 import com.amazonaws.services.ec2.model.DescribeTagsResult
 import com.amazonaws.services.ec2.model.TagDescription
+import com.amazonaws.services.ec2.model.UpdateSecurityGroupRuleDescriptionsIngressRequest
 import com.google.common.collect.ImmutableSet
 import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription
 import com.netflix.spinnaker.clouddriver.aws.security.AmazonClientProvider
@@ -249,6 +250,13 @@ class SecurityGroupLookupFactory {
       this.amazonEC2 = amazonEC2
     }
 
+    void updateIngress(List<IpPermission> ipPermissionsToUpdate) {
+      amazonEC2.updateSecurityGroupRuleDescriptionsIngress(new UpdateSecurityGroupRuleDescriptionsIngressRequest(
+        groupId: securityGroup.groupId,
+        ipPermissions: ipPermissionsToUpdate
+      ))
+    }
+
     void addIngress(List<IpPermission> ipPermissionsToAdd) {
       amazonEC2.authorizeSecurityGroupIngress(new AuthorizeSecurityGroupIngressRequest(
         groupId: securityGroup.groupId,

diff --git a/...nnaker/clouddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperation.groovy b/...nnaker/clouddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperation.groovy
@@ -101,8 +101,36 @@ class UpsertSecurityGroupAtomicOperation implements AtomicOperation<Void> {
       ipPermissionsFromDescription = convertDescriptionToIngress(securityGroupLookup, description, false)
     }
 
-    List<IpPermission> ipPermissionsToAdd = ipPermissionsFromDescription.converted - existingIpPermissions
-    List<IpPermission> ipPermissionsToRemove = existingIpPermissions - ipPermissionsFromDescription.converted
+    SecurityGroupIngressConverter.IpRuleDelta ipRuleDelta = SecurityGroupIngressConverter.computeIpRuleDelta(ipPermissionsFromDescription.converted, existingIpPermissions)
+
+    List<IpPermission> ipPermissionsToAdd = ipRuleDelta.toAdd
+    List<IpPermission> userIdGroupPermissions = SecurityGroupIngressConverter.userIdGroupPairsDiff(ipPermissionsFromDescription.converted,existingIpPermissions)
+    ipPermissionsToAdd = ipPermissionsToAdd + userIdGroupPermissions
+
+    List<IpPermission> ipPermissionsToRemove = ipRuleDelta.toRemove
+    List<IpPermission> userIdGroupPermissionsToRemove = SecurityGroupIngressConverter.userIdGroupPairsDiff(existingIpPermissions,ipPermissionsFromDescription.converted)
+    ipPermissionsToRemove = ipPermissionsToRemove + userIdGroupPermissionsToRemove
+
+    List<IpPermission> tobeUpdated = ipRuleDelta.toUpdate
+
+    //Update rules that are already present on the security group
+    if(tobeUpdated) {
+      String status = "Permissions updated to '${description.name}'"
+      if (tobeUpdated.size() > MAX_RULES_FOR_STATUS) {
+        status = "$status (${tobeUpdated.size()} rules updated)."
+      } else {
+        status = "$status ($tobeUpdated)."
+      }
+      try {
+        securityGroupUpdater.updateIngress(tobeUpdated)
+        //Update tags to ensure they are consistent with rule changes
+        securityGroupUpdater.updateTags(description)
+        task.updateStatus BASE_PHASE, status
+      } catch (AmazonServiceException e) {
+        task.updateStatus BASE_PHASE, "Error updating ingress to '${description.name}' - ${e.errorMessage}"
+        throw e
+      }
+    }
 
     // Converge on the desired final set of security group rules
     if (ipPermissionsToAdd) {
@@ -123,6 +151,7 @@ class UpsertSecurityGroupAtomicOperation implements AtomicOperation<Void> {
         throw e
       }
     }
+
     if (ipPermissionsToRemove && !description.ingressAppendOnly) {
       String status = "Permissions removed from '${description.name}'"
       if (ipPermissionsToRemove.size() > MAX_RULES_FOR_STATUS) {

diff --git a/...louddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperationUnitSpec.groovy b/...louddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperationUnitSpec.groovy
@@ -18,6 +18,7 @@ package com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup
 
 import com.amazonaws.AmazonServiceException
 import com.amazonaws.services.ec2.model.IpPermission
+import com.amazonaws.services.ec2.model.IpRange
 import com.amazonaws.services.ec2.model.SecurityGroup
 import com.amazonaws.services.ec2.model.UserIdGroupPair
 import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription
@@ -27,6 +28,7 @@ import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.SecurityGr
 import com.netflix.spinnaker.clouddriver.aws.security.NetflixAmazonCredentials
 import com.netflix.spinnaker.clouddriver.data.task.Task
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
+import com.netflix.spinnaker.clouddriver.model.securitygroups.IpRangeRule
 import spock.lang.Specification
 import spock.lang.Subject
 
@@ -428,4 +430,133 @@ class UpsertSecurityGroupAtomicOperationUnitSpec extends Specification {
 
   }
 
+  void "should update ingress and add by name for missing ingress security group in EC2 classic"() {
+    final existingSecurityGroup = Mock(SecurityGroupUpdater)
+    final ingressSecurityGroup = Mock(SecurityGroupUpdater)
+    description.securityGroupIngress = [
+      new SecurityGroupIngress(name: "bar", startPort: 111, endPort: 112, vpcId: "vpc-123", ipProtocol: "tcp", accountName: "test")
+    ]
+    description.vpcId = null
+    description.ipIngress = [
+      new IpIngress(cidr: "123.23.45.6/12", startPort: 7002, endPort: 7004, description: "foo", ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getAccountIdForName("test") >> "accountId1"
+    1 * securityGroupLookup.getSecurityGroupByName("test", "bar", "vpc-123") >> Optional.of(ingressSecurityGroup)
+
+    then:
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", null) >> Optional.of(existingSecurityGroup)
+    1 * ingressSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "bar", groupId: "124", vpcId: "vpc-123")
+    1 * existingSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "foo", groupId: "123", ipPermissions: [
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: "foo", cidrIp:"123.23.45.6/12")])
+    ])
+
+    then:
+    1 * existingSecurityGroup.addIngress(_)
+    1 * existingSecurityGroup.updateIngress(_)
+    2 * existingSecurityGroup.updateTags(description)
+  }
+
+  void "should only update ingress of existing ingress when description is not null in the input"() {
+    final existingSecurityGroup = Mock(SecurityGroupUpdater)
+    final ingressSecurityGroup = Mock(SecurityGroupUpdater)
+    description.vpcId = null
+    description.ipIngress = [
+      new IpIngress(cidr: "123.23.45.6/12", startPort: 7002, endPort: 7004, description: "foo", ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", null) >> Optional.of(existingSecurityGroup)
+    1 * existingSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "foo", groupId: "123", ipPermissions: [
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: "foo", cidrIp:"123.23.45.6/12")])
+    ])
+
+    then:
+    1 * existingSecurityGroup.updateIngress(_)
+    1 * existingSecurityGroup.updateTags(description)
+  }
+
+  void "should update existing ingress with description when description is null for existing rule"() {
+    final existingSecurityGroup = Mock(SecurityGroupUpdater)
+    final ingressSecurityGroup = Mock(SecurityGroupUpdater)
+    description.vpcId = null
+    description.ipIngress = [
+      new IpIngress(cidr: "123.23.45.6/12", startPort: 7002, endPort: 7004, description: "foo", ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", null) >> Optional.of(existingSecurityGroup)
+    1 * existingSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "foo", groupId: "123", ipPermissions: [
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: null, cidrIp:"123.23.45.6/12")])
+    ])
+
+    then:
+    1 * existingSecurityGroup.updateIngress(_)
+    1 * existingSecurityGroup.updateTags(description)
+  }
+
+  void "should not update ingress existing ingress with description for the same rule"() {
+    final existingSecurityGroup = Mock(SecurityGroupUpdater)
+    final ingressSecurityGroup = Mock(SecurityGroupUpdater)
+    description.vpcId = null
+    description.ipIngress = [
+      new IpIngress(cidr: "123.23.45.6/12", startPort: 7002, endPort: 7004, description: null, ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", null) >> Optional.of(existingSecurityGroup)
+    1 * existingSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "foo", groupId: "123", ipPermissions: [
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: "foo", cidrIp:"123.23.45.6/12")])
+    ])
+
+    then:
+    0 * existingSecurityGroup.updateIngress(_)
+    0 * existingSecurityGroup.updateTags(description)
+  }
+  void "should add, remove and update security group ingress rules"() {
+    final existingSecurityGroup = Mock(SecurityGroupUpdater)
+    final ingressSecurityGroup = Mock(SecurityGroupUpdater)
+    description.securityGroupIngress = [
+      new SecurityGroupIngress(name: "bar", startPort: 111, endPort: 112, vpcId: "vpc-123", ipProtocol: "tcp", accountName: "test")
+    ]
+    description.vpcId = null
+    description.ipIngress = [
+      new IpIngress(cidr: "123.23.45.6/12", startPort: 7002, endPort: 7004, description: "foo", ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getAccountIdForName("test") >> "accountId1"
+    1 * securityGroupLookup.getSecurityGroupByName("test", "bar", "vpc-123") >> Optional.of(ingressSecurityGroup)
+
+    then:
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", null) >> Optional.of(existingSecurityGroup)
+    1 * ingressSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "bar", groupId: "124", vpcId: "vpc-123")
+    1 * existingSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupName: "foo", groupId: "123", ipPermissions: [
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: "foo", cidrIp:"123.23.45.6/12")]),
+      new IpPermission(ipProtocol: "tcp", fromPort: 7002, toPort: 7004, ipv4Ranges: [new IpRange(description: "baz", cidrIp:"103.23.45.6/12")])
+    ])
+
+    then:
+    1 * existingSecurityGroup.addIngress(_)
+    1 * existingSecurityGroup.updateIngress(_)
+    3 * existingSecurityGroup.updateTags(description)
+    1 * existingSecurityGroup.removeIngress(_)
+  }
+
 }