feat(titus): Lookup server group names when converting TitusDeployDes…

…cription (#4654) -Additionally, removes requiresAuthorization from `ResourcesNameable` and removes `BasicAmazonDeployDescription` implementation of `ResourcesNameable`. We will instead be requiring authorization on these resources via an implementation of `AllowedAccountsValidator` which runs prior to `DescriptionAuthorizer`. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
spinnaker · Jun 5, 2020 · 0763864 · 0763864
1 parent e7152e0
commit 0763864
Show file tree

Hide file tree

Showing 8 changed files with 52 additions and 63 deletions.
diff --git a/.../netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy b/.../netflix/spinnaker/clouddriver/aws/deploy/description/BasicAmazonDeployDescription.groovy
@@ -21,14 +21,13 @@ import com.netflix.spinnaker.clouddriver.aws.model.AmazonBlockDevice
 import com.netflix.spinnaker.clouddriver.deploy.DeployDescription
 import com.netflix.spinnaker.clouddriver.orchestration.events.OperationEvent
 import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable
-import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable
 import groovy.transform.AutoClone
 import groovy.transform.Canonical
 
 @AutoClone
 @Canonical
 class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription implements
-  DeployDescription, ApplicationNameable, ResourcesNameable {
+  DeployDescription, ApplicationNameable {
   String application
   String amiName
   String stack
@@ -117,16 +116,6 @@ class BasicAmazonDeployDescription extends AbstractAmazonCredentialsDescription
     return [application]
   }
 
-  @Override
-  Collection<String> getNames() {
-    return securityGroupNames ?: []
-  }
-
-  @Override
-  boolean requiresAuthorization() {
-    return false
-  }
-
   @Canonical
   static class Capacity {
     Integer min

diff --git a/...er-core/src/main/groovy/com/netflix/spinnaker/clouddriver/config/CloudDriverConfig.groovy b/...er-core/src/main/groovy/com/netflix/spinnaker/clouddriver/config/CloudDriverConfig.groovy
@@ -346,16 +346,12 @@ class CloudDriverConfig {
 
   @Bean
   DescriptionAuthorizer descriptionAuthorizer(Registry registry,
-                                              ObjectMapper objectMapper,
                                               Optional<FiatPermissionEvaluator> fiatPermissionEvaluator,
-                                              SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps,
-                                              DynamicConfigService dynamicConfigService) {
+                                              SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     return new DescriptionAuthorizer(
       registry,
-      objectMapper,
       fiatPermissionEvaluator,
-      opsSecurityConfigProps,
-      dynamicConfigService
+      opsSecurityConfigProps
     )
   }
 

diff --git a/...-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java b/...-core/src/main/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizer.java
@@ -18,17 +18,14 @@
 
 import static java.lang.String.format;
 
-import com.fasterxml.jackson.databind.ObjectMapper;
 import com.netflix.spectator.api.Id;
 import com.netflix.spectator.api.Registry;
 import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig;
 import com.netflix.spinnaker.clouddriver.security.resources.AccountNameable;
 import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable;
 import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable;
 import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator;
-import com.netflix.spinnaker.kork.dynamicconfig.DynamicConfigService;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
 import java.util.Objects;
@@ -44,26 +41,20 @@ public class DescriptionAuthorizer<T> {
   private final Logger log = LoggerFactory.getLogger(getClass());
 
   private final Registry registry;
-  private final ObjectMapper objectMapper;
   private final FiatPermissionEvaluator fiatPermissionEvaluator;
   private final SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps;
-  private final DynamicConfigService dynamicConfigService;
 
   private final Id skipAuthorizationId;
   private final Id missingApplicationId;
   private final Id authorizationId;
 
   public DescriptionAuthorizer(
       Registry registry,
-      ObjectMapper objectMapper,
       Optional<FiatPermissionEvaluator> fiatPermissionEvaluator,
-      SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps,
-      DynamicConfigService dynamicConfigService) {
+      SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) {
     this.registry = registry;
-    this.objectMapper = objectMapper;
     this.fiatPermissionEvaluator = fiatPermissionEvaluator.orElse(null);
     this.opsSecurityConfigProps = opsSecurityConfigProps;
-    this.dynamicConfigService = dynamicConfigService;
 
     this.skipAuthorizationId = registry.createId("authorization.skipped");
     this.missingApplicationId = registry.createId("authorization.missingApplication");
@@ -114,29 +105,11 @@ public void authorize(T description, Errors errors) {
     if (description instanceof ResourcesNameable) {
       ResourcesNameable resourcesNameable = (ResourcesNameable) description;
 
-      if (!resourcesNameable.requiresAuthorization()
-          || dynamicConfigService.isEnabled("aws.fiat.authorize.resources-nameable", false)) {
-        registry
-            .counter(
-                skipAuthorizationId.withTag(
-                    "descriptionClass", description.getClass().getSimpleName()))
-            .increment();
-
-        Collection<String> resourceNames =
-            Optional.ofNullable(resourcesNameable.getNames()).orElse(Collections.emptyList());
-
-        log.info(
-            "Skipping authorization for operation={}, resource names={}, resource applications={}",
-            description.getClass().getSimpleName(),
-            resourceNames,
-            resourcesNameable.getResourceApplications().toString());
-      } else {
-        applications.addAll(
-            Optional.ofNullable(resourcesNameable.getResourceApplications())
-                .orElse(Collections.emptyList()).stream()
-                .filter(Objects::nonNull)
-                .collect(Collectors.toList()));
-      }
+      applications.addAll(
+          Optional.ofNullable(resourcesNameable.getResourceApplications())
+              .orElse(Collections.emptyList()).stream()
+              .filter(Objects::nonNull)
+              .collect(Collectors.toList()));
     }
 
     boolean hasPermission = true;

diff --git a/...src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy b/...src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerSpec.groovy
@@ -34,14 +34,13 @@ class DescriptionAuthorizerSpec extends Specification {
   def registry = new NoopRegistry()
   def evaluator = Mock(FiatPermissionEvaluator)
   def opsSecurityConfigProps
-  def dynamicConfigService = Mock(DynamicConfigService)
 
   @Subject
   DescriptionAuthorizer authorizer
 
   def setup() {
     opsSecurityConfigProps = new SecurityConfig.OperationsSecurityConfigurationProperties()
-    authorizer = new DescriptionAuthorizer(registry, new ObjectMapper(), Optional.of(evaluator), opsSecurityConfigProps, dynamicConfigService)
+    authorizer = new DescriptionAuthorizer(registry, Optional.of(evaluator), opsSecurityConfigProps)
   }
 
   def "should authorize passed description"() {

diff --git a/...c/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java b/...c/main/groovy/com/netflix/spinnaker/clouddriver/security/resources/ResourcesNameable.java
@@ -36,14 +36,4 @@ default Collection<String> getResourceApplications() {
         .map(name -> Names.parseName(name).getApp())
         .collect(Collectors.toList());
   }
-
-  /**
-   * Determine if the resource should be authz evaluated by the {@link
-   * com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator}.
-   *
-   * @return boolean
-   */
-  default boolean requiresAuthorization() {
-    return true;
-  }
 }
diff --git a/...rc/main/groovy/com/netflix/spinnaker/clouddriver/titus/caching/utils/AwsLookupUtil.groovy b/...rc/main/groovy/com/netflix/spinnaker/clouddriver/titus/caching/utils/AwsLookupUtil.groovy
@@ -25,6 +25,7 @@ import com.netflix.spinnaker.clouddriver.aws.provider.view.AmazonSecurityGroupPr
 import com.netflix.spinnaker.clouddriver.aws.provider.view.AmazonVpcProvider
 import com.netflix.spinnaker.clouddriver.aws.security.AmazonCredentials
 import com.netflix.spinnaker.clouddriver.aws.services.RegionScopedProviderFactory
+import com.netflix.spinnaker.clouddriver.aws.services.SecurityGroupService
 import com.netflix.spinnaker.clouddriver.security.AccountCredentialsProvider
 import com.netflix.spinnaker.clouddriver.titus.client.model.Job
 import com.netflix.spinnaker.clouddriver.titus.credentials.NetflixTitusCredentials
@@ -74,6 +75,26 @@ class AwsLookupUtil {
     awsSecurityGroupProvider.getIdByName(awsDetails.awsAccount, region, providedSecurityGroup, awsDetails.vpcId)
   }
 
+  /**
+   * Converts security groups to security group names.  This handles the case wherein the list of
+   * security groups may include both IDs and names.
+   */
+  List<String> convertSecurityGroupsToNames(String account, String region, List<String> securityGroups) {
+    Map awsDetails = awsAccountLookup.find {
+      it.titusAccount == account && it.region == region
+    }
+
+    RegionScopedProviderFactory.RegionScopedProvider regionScopedProvider = regionScopedProviderFactory.forRegion(accountCredentialsProvider.all.find {
+      it instanceof AmazonCredentials && it.name == awsDetails.awsAccount
+    }, region)
+
+    SecurityGroupService securityGroupService = regionScopedProvider.getSecurityGroupService()
+
+    return securityGroupService.resolveSecurityGroupNamesByStrategy(securityGroups) { List<String> ids ->
+      securityGroupService.getSecurityGroupNamesFromIds(ids)
+    }
+  }
+
   String createSecurityGroupForApplication(account, region, application) {
     Map awsDetails = awsAccountLookup.find {
       it.titusAccount == account && it.region == region

diff --git a/.../spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy b/.../spinnaker/clouddriver/titus/deploy/converters/TitusDeployAtomicOperationConverter.groovy
@@ -21,13 +21,18 @@ import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperation
 import com.netflix.spinnaker.clouddriver.orchestration.AtomicOperations
 import com.netflix.spinnaker.clouddriver.security.AbstractAtomicOperationsCredentialsSupport
 import com.netflix.spinnaker.clouddriver.titus.TitusOperation
+import com.netflix.spinnaker.clouddriver.titus.caching.utils.AwsLookupUtil
 import com.netflix.spinnaker.clouddriver.titus.deploy.description.TitusDeployDescription
+import org.springframework.beans.factory.annotation.Autowired
 import org.springframework.stereotype.Component
 
 @TitusOperation(AtomicOperations.CREATE_SERVER_GROUP)
 @Component
 class TitusDeployAtomicOperationConverter extends AbstractAtomicOperationsCredentialsSupport {
 
+  @Autowired
+  AwsLookupUtil awsLookupUtil
+
   @Override
   AtomicOperation convertOperation(Map input) {
     new DeployAtomicOperation(convertDescription(input))
@@ -49,6 +54,13 @@ class TitusDeployAtomicOperationConverter extends AbstractAtomicOperationsCreden
 
     def converted = objectMapper.convertValue(input, TitusDeployDescription)
     converted.credentials = getCredentialsObject(input.credentials as String)
+
+    if (converted.securityGroups != null && !converted.securityGroups.isEmpty()) {
+      converted.setSecurityGroupNames(
+        awsLookupUtil.convertSecurityGroupsToNames(converted.account, converted.region, converted.securityGroups)
+      )
+    }
+
     converted
   }
 }
diff --git a/...vy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java b/...vy/com/netflix/spinnaker/clouddriver/titus/deploy/description/TitusDeployDescription.java
@@ -30,6 +30,7 @@ public class TitusDeployDescription extends AbstractTitusCredentialsDescription
   private String subnet;
   private List<String> zones = new ArrayList<>();
   private List<String> securityGroups = new ArrayList<>();
+  private List<String> securityGroupNames = new ArrayList<>();
   private List<String> targetGroups = new ArrayList<>();
   private List<String> softConstraints;
   private List<String> hardConstraints;
@@ -239,6 +240,14 @@ public void setSecurityGroups(List<String> securityGroups) {
     this.securityGroups = securityGroups;
   }
 
+  public List<String> getSecurityGroupNames() {
+    return securityGroupNames;
+  }
+
+  public void setSecurityGroupNames(List<String> securityGroupNames) {
+    this.securityGroupNames = securityGroupNames;
+  }
+
   public List<String> getTargetGroups() {
     return targetGroups;
   }