fix(ecr): Two credentials with same accountId confuses EcrImageProvid…

…er with different regions (#5885) (#5888) * fix(ecr) Add test for credential search bug, and a suggested fix * fix(test) Change regions to something reasonable for eu-west-1 * fix(test): Make the regions reasonable here too --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> (cherry picked from commit 07b1d87) Co-authored-by: David Watt <[email protected]> Co-authored-by: Jason <[email protected]>
spinnaker · Mar 28, 2023 · e998b05 · e998b05
1 parent 08bcf69
commit e998b05
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 6 deletions.
diff --git a/...s/src/main/java/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcrImageProvider.java b/...s/src/main/java/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcrImageProvider.java
@@ -89,7 +89,7 @@ public List<EcsDockerImage> findImage(String url) {
         !(identifier.startsWith("sha256:") && identifier.length() == ("sha256:".length() + 64));
     String region = extractAwsRegion(url);
 
-    NetflixAmazonCredentials credentials = getCredentials(accountId);
+    NetflixAmazonCredentials credentials = getCredentials(accountId, region);
 
     if (!isValidRegion(credentials, region)) {
       throw new IllegalArgumentException(
@@ -144,15 +144,20 @@ private boolean imageFilter(ImageIdentifier imageIdentifier, String identifier,
         : imageIdentifier.getImageDigest().equals(identifier);
   }
 
-  private NetflixAmazonCredentials getCredentials(String accountId) {
+  private NetflixAmazonCredentials getCredentials(String accountId, String region) {
+
     for (NetflixECSCredentials credentials : credentialsRepository.getAll()) {
-      if (credentials.getAccountId().equals(accountId)) {
+      if (credentials.getAccountId().equals(accountId)
+          && (credentials.getRegions().isEmpty()
+              || credentials.getRegions().stream()
+                  .anyMatch(oneRegion -> oneRegion.getName().equals(region)))) {
         return credentials;
       }
     }
     throw new NotFoundException(
         String.format(
-            "AWS account %s was not found.  Please specify a valid account name", accountId));
+            "AWS account %s with region %s was not found.  Please specify a valid account name and region",
+            accountId, region));
   }
 
   private List<ImageIdentifier> getImageIdentifiers(

diff --git a/...st/groovy/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcrImageProviderSpec.groovy b/...st/groovy/com/netflix/spinnaker/clouddriver/ecs/provider/view/EcrImageProviderSpec.groovy
@@ -136,6 +136,65 @@ class EcrImageProviderSpec extends Specification {
     retrievedListOfImages == expectedListOfImages
   }
 
+  def 'should find second credential when two share account ids'() {
+    given:
+    def region = 'us-east-1'
+    def repoName = 'repositoryname'
+    def accountId = '123456789012'
+    def tag = 'arbitrary-tag'
+    def digest = 'sha256:deadbeef785192c146085da66a4261e25e79a6210103433464eb7f79deadbeef'
+    def creationDate = new Date()
+    def url = accountId + '.dkr.ecr.' + region + '.amazonaws.com/' + repoName + ':' + tag// + '@' + digest
+    def imageDetail = new ImageDetail(
+      repositoryName: repoName,
+      registryId: accountId,
+      imageDigest: digest,
+      imageTags: List.of(tag),
+      imagePushedAt: creationDate
+    )
+
+    Map<String, Object> region1 = Map.of(
+      'name', 'eu-west-1',
+      'availabilityZones', Arrays.asList('eu-west-1a', 'eu-west-1b', 'eu-west-1c')
+    )
+    Map<String, Object> overrides1 = Map.of(
+      'accountId', accountId,
+      'regions', Arrays.asList(region1)
+    )
+
+    Map<String, Object> region2 = Map.of(
+      'name', region,
+      'availabilityZones', Arrays.asList('us-east-1a', 'us-east-1b', 'us-east-1c')
+    )
+    Map<String, Object> overrides2 = Map.of(
+      'accountId', accountId,
+      'regions', Arrays.asList(region2)
+    )
+
+    credentialsRepository.getAll() >> [
+      new NetflixECSCredentials(TestCredential.named('incorrect-region', overrides1)),
+      new NetflixECSCredentials(TestCredential.named('correct-region', overrides2))]
+
+    def amazonECR = Mock(AmazonECR)
+
+    amazonClientProvider.getAmazonEcr(_, _, _) >> amazonECR
+    amazonECR.listImages(_) >> new ListImagesResult().withImageIds(Collections.emptyList())
+    amazonECR.describeImages(_) >> new DescribeImagesResult().withImageDetails(imageDetail)
+
+    def expectedListOfImages = [new EcsDockerImage(
+      region: region,
+      imageName: accountId + '.dkr.ecr.' + region + '.amazonaws.com/' + repoName + '@' + digest,
+      amis: ['us-east-1': Collections.singletonList(digest)],
+      attributes: [creationDate: creationDate]
+    )]
+
+    when:
+    def retrievedListOfImages = provider.findImage(url)
+
+    then:
+    retrievedListOfImages == expectedListOfImages
+  }
+
   def 'should throw exception due to malformed account'() {
     given:
     def region = 'us-west-1'
@@ -198,8 +257,8 @@ class EcrImageProviderSpec extends Specification {
     provider.findImage(url)
 
     then:
-    final IllegalArgumentException error = thrown()
-    error.message == "The repository URI provided does not belong to a region that the credentials have access to or the region is not valid."
+    final com.netflix.spinnaker.kork.web.exceptions.NotFoundException error = thrown()
+    error.message == String.format("AWS account %s with region %s was not found.  Please specify a valid account name and region", accountId, region)
   }
 
   def 'should find the image in a repository with a large number of images'() {