feat(aws): Allow self-referencing security group clone ops (#2371)

spinnaker · Mar 1, 2018 · 1426ce8 · 1426ce8
1 parent 5fd252a
commit 1426ce8
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 20 deletions.
diff --git a/...x/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupIngressConverter.groovy b/...x/spinnaker/clouddriver/aws/deploy/ops/securitygroup/SecurityGroupIngressConverter.groovy
@@ -33,7 +33,26 @@ class SecurityGroupIngressConverter {
   @Immutable
   static class ConvertedIngress {
     List<IpPermission> converted
-    List<SecurityGroupIngress> missingSecurityGroups
+    MissingSecurityGroups missingSecurityGroups
+  }
+
+  @Immutable
+  static class MissingSecurityGroups {
+    List<SecurityGroupIngress> all
+    List<SecurityGroupIngress> selfReferencing
+
+    boolean anyMissing(boolean ignoreSelfReferencing) {
+      if (all.isEmpty()) {
+        return false;
+      } else if (ignoreSelfReferencing) {
+        return all.size() > selfReferencing.size()
+      }
+      return true
+    }
+
+    boolean hasMissingNonSelfReferencingGroups() {
+      return !all.isEmpty() && all.size() > selfReferencing.size()
+    }
   }
 
   static ConvertedIngress convertIngressToIpPermissions(SecurityGroupLookup securityGroupLookup,
@@ -75,7 +94,10 @@ class SecurityGroupIngressConverter {
         ipPermissions.add(newIpPermission)
       }
     }
-    new ConvertedIngress(ipPermissions, missing)
+    new ConvertedIngress(ipPermissions, new MissingSecurityGroups(
+      all: missing,
+      selfReferencing: missing.findAll { it.name == description.name && it.accountName == description.credentialAccount }
+    ))
   }
 
   static List<IpPermission> flattenPermissions(SecurityGroup securityGroup) {

diff --git a/...nnaker/clouddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperation.groovy b/...nnaker/clouddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperation.groovy
@@ -18,10 +18,12 @@ package com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup
 
 import com.amazonaws.AmazonServiceException
 import com.amazonaws.services.ec2.model.IpPermission
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription
+import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.SecurityGroupIngressConverter.ConvertedIngress
+import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.SecurityGroupLookupFactory.SecurityGroupLookup
 import com.netflix.spinnaker.clouddriver.data.task.Task
 import com.netflix.spinnaker.clouddriver.data.task.TaskRepository
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
-import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription
 import groovy.util.logging.Slf4j
 import org.springframework.beans.factory.annotation.Autowired
 
@@ -45,23 +47,20 @@ class UpsertSecurityGroupAtomicOperation implements AtomicOperation<Void> {
   @Override
   Void operate(List priorOutputs) {
     final securityGroupLookup = securityGroupLookupFactory.getInstance(description.region)
-    final ipPermissionsFromDescription = SecurityGroupIngressConverter.
-      convertIngressToIpPermissions(securityGroupLookup, description)
-    if (ipPermissionsFromDescription.missingSecurityGroups) {
-      def missingSecurityGroupDescriptions = ipPermissionsFromDescription.missingSecurityGroups.collect {
-        "'${it.name ?: it.id}' in '${it.accountName ?: description.credentialAccount}' ${it.vpcId ?: description.vpcId ?: 'EC2-classic'}"
-      }
-      def securityGroupsDoNotExistErrorMessage = "The following security groups do not exist: ${missingSecurityGroupDescriptions.join(", ")}"
-      task.updateStatus BASE_PHASE, securityGroupsDoNotExistErrorMessage
-      throw new IllegalStateException(securityGroupsDoNotExistErrorMessage)
-    }
+
+    // Get all of the ingress rules from the description. Will fail the operation if any upstream security groups
+    // are missing, unless they're self-referential to the security group being upserted. In the case of a
+    // self-referential security group, we don't want to fail the upsert if the security group doesn't exist, as we'll
+    // be creating it next.
+    ConvertedIngress ipPermissionsFromDescription = convertDescriptionToIngress(securityGroupLookup, description, true)
 
     def securityGroupUpdater = securityGroupLookup.getSecurityGroupByName(
       description.credentialAccount,
       description.name,
       description.vpcId
     )
 
+    // Create or update the security group itself. If the security group exists, also get the security group rules.
     List<IpPermission> existingIpPermissions
     if (securityGroupUpdater.present) {
       securityGroupUpdater = securityGroupUpdater.get()
@@ -88,9 +87,16 @@ class UpsertSecurityGroupAtomicOperation implements AtomicOperation<Void> {
       }
     }
 
+    // Second conversion of desired security group rules. If any upstream groups (including self-referential) are
+    // missing, the operation will fail.
+    if (!ipPermissionsFromDescription.missingSecurityGroups.selfReferencing.isEmpty()) {
+      ipPermissionsFromDescription = convertDescriptionToIngress(securityGroupLookup, description, false)
+    }
+
     List<IpPermission> ipPermissionsToAdd = ipPermissionsFromDescription.converted - existingIpPermissions
     List<IpPermission> ipPermissionsToRemove = existingIpPermissions - ipPermissionsFromDescription.converted
 
+    // Converge on the desired final set of security group rules
     if (ipPermissionsToAdd) {
       try {
         securityGroupUpdater.addIngress(ipPermissionsToAdd)
@@ -112,4 +118,19 @@ class UpsertSecurityGroupAtomicOperation implements AtomicOperation<Void> {
     null
   }
 
+  private ConvertedIngress convertDescriptionToIngress(SecurityGroupLookup securityGroupLookup, UpsertSecurityGroupDescription description, boolean ignoreSelfReferencingRules) {
+    ConvertedIngress ipPermissionsFromDescription = SecurityGroupIngressConverter.
+      convertIngressToIpPermissions(securityGroupLookup, description)
+
+    if (ipPermissionsFromDescription.missingSecurityGroups.anyMissing(ignoreSelfReferencingRules)) {
+      def missingSecurityGroupDescriptions = ipPermissionsFromDescription.missingSecurityGroups.all.collect {
+        "'${it.name ?: it.id}' in '${it.accountName ?: description.credentialAccount}' ${it.vpcId ?: description.vpcId ?: 'EC2-classic'}"
+      }
+      def securityGroupsDoNotExistErrorMessage = "The following security groups do not exist: ${missingSecurityGroupDescriptions.join(", ")}"
+      task.updateStatus BASE_PHASE, securityGroupsDoNotExistErrorMessage
+      throw new IllegalStateException(securityGroupsDoNotExistErrorMessage)
+    }
+
+    return ipPermissionsFromDescription
+  }
 }
diff --git a/...louddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperationUnitSpec.groovy b/...louddriver/aws/deploy/ops/securitygroup/UpsertSecurityGroupAtomicOperationUnitSpec.groovy
@@ -20,20 +20,16 @@ import com.amazonaws.AmazonServiceException
 import com.amazonaws.services.ec2.model.IpPermission
 import com.amazonaws.services.ec2.model.SecurityGroup
 import com.amazonaws.services.ec2.model.UserIdGroupPair
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription.IpIngress
+import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription.SecurityGroupIngress
 import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.SecurityGroupLookupFactory.SecurityGroupUpdater
-import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.UpsertSecurityGroupAtomicOperation
-import com.netflix.spinnaker.clouddriver.aws.deploy.ops.securitygroup.SecurityGroupLookupFactory
 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.aws.deploy.description.UpsertSecurityGroupDescription
-import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription.SecurityGroupIngress
-import com.netflix.spinnaker.clouddriver.aws.deploy.description.UpsertSecurityGroupDescription.IpIngress
 import spock.lang.Specification
 import spock.lang.Subject
 
-import javax.swing.text.html.Option
-
 class UpsertSecurityGroupAtomicOperationUnitSpec extends Specification {
   def setupSpec() {
     TaskRepository.threadLocalTask.set(Mock(Task))
@@ -351,6 +347,33 @@ class UpsertSecurityGroupAtomicOperationUnitSpec extends Specification {
     ex.message == "The following security groups do not exist: 'bar' in 'test' vpc-123"
   }
 
+  void "should two-phase create self-referential security group in vpc"() {
+    final createdSecurityGroup = Mock(SecurityGroupUpdater)
+    description.securityGroupIngress = [
+      new SecurityGroupIngress(name: "foo", accountName: "test", startPort: 111, endPort: 112, ipProtocol: "tcp")
+    ]
+
+    when:
+    op.operate([])
+
+    then:
+    1 * securityGroupLookup.getAccountIdForName("test") >> "accountId1"
+    2 * securityGroupLookup.getSecurityGroupByName("test", "foo", "vpc-123") >> Optional.empty()
+    1 * securityGroupLookup.createSecurityGroup(description) >> createdSecurityGroup
+
+    and:
+    1 * securityGroupLookup.getAccountIdForName("test") >> "accountId1"
+    1 * securityGroupLookup.getSecurityGroupByName("test", "foo", "vpc-123") >> Optional.of(createdSecurityGroup)
+    2 * createdSecurityGroup.getSecurityGroup() >> new SecurityGroup(groupId: "id-foo")
+
+    and:
+    1 * createdSecurityGroup.addIngress([
+      new IpPermission(ipProtocol: "tcp", fromPort: 111, toPort: 112, userIdGroupPairs: [
+        new UserIdGroupPair(userId: "accountId1", groupId: "id-foo")
+      ])
+    ])
+  }
+
   void "should add ingress by name for missing ingress security group in EC2 classic"() {
     final existingSecurityGroup = Mock(SecurityGroupUpdater)
     description.securityGroupIngress = [