Repository: crossplane/provider-aws (go)

Issue:
ECR Policy interaction with AWS causes update loops
### What happened?
Implementing ECR Repository Policies atop some existing ECR Repo definitions, I was using the `accountId` principal to set permissions for a repo from particular AWS accounts.

I was notified this morning of excess AWS Config charges from calls to `SetRepositoryPolicy`: 700,000 over the course of 10 days per Repo 😄 

Digging around, it looks like there's 2 cases where the ECR Repository Policy does not correctly detect the resource as up to date, and repeatedly triggers an Update call:

1. When using the `awsAccountId` principal, AWS documentation says you can specify either the plain account ID, OR the ARN of the root of the account, i.e. the below, but AWS converts the raw account ID to an ARN string internally and appears to _always_ return the ARN. `SerializeAWSPrincipal` just returns the plain user input as a string, so if the user specifies a plain account ID rather than an ARN, these will never be detected as up to date.
    - `"Principal": { "AWS": "arn:aws:iam::123456789012:root" }` or
    - `"Principal": { "AWS": "123456789012" }` are functionally equal.
2. Principals are not sorted before or during comparison, so if AWS returns the principals in a different order to the user input they will also never be detected as up to date.

### How can we reproduce it?
Create an ECR repo (settings unimportant), and then create a `RepositoryPolicy` referring to it with the following:
```yaml
apiVersion: ecr.aws.crossplane.io/v1alpha1
kind: RepositoryPolicy
metadata:
  name: my-never-uptodate-policy
spec:
  forProvider:
    policy:
      id: AllowAccounts
      statements:
      - action:
        - ecr:BatchCheckLayerAvailability
        - ecr:BatchGetImage
        - ecr:GetDownloadUrlForLayer
        effect: Allow
        principal:
          awsPrincipals:
          - awsAccountId: 123456789012
          - awsAccountId: arn:aws:iam::123456789013:root
        sid: AllowPull
      version: "2008-10-17"
    region: us-east-1
    repositoryName: <name-of-your-ecr-repo>
```

The status object of the resource will look something like this:

```yaml
status:
  atProvider:
    policyText: |-
      {
        "Version" : "2008-10-17",
        "Id" : "AllowAccounts",
        "Statement" : [ {
          "Sid" : "AllowPull",
          "Effect" : "Allow",
          "Principal" : {
            "AWS" : [ "arn:aws:iam::123456789013:root", "arn:aws:iam::123456789012:root" ]
          },
          "Action" : [ "ecr:BatchCheckLayerAvailability", "ecr:BatchGetImage", "ecr:GetDownloadUrlForLayer" ]
        } ]
      }
```

Note that the principals are always an ARN and they're not in the same order as specified by the resource.

### What environment did it happen in?
AWS Provider version: v0.18.1

### Suggested fix
For issue 1, maybe we can convert the raw account ID into an ARN within crossplane (`arn:aws:iam::%s:root`) as I doubt the format will ever change, or if we leave as-is, make sure the behaviour is documented to avoid unintended update behaviour.

For issue 2, we should supply a `cmpopts.SortSlices` function to `IsRepositoryPolicyUpToDate()`, to make sure that principal slices are sorted before comparison - something like the below:

```go
sortSlicesOpt := cmpopts.SortSlices(func(x, y interface{}) bool {
	return x.(string) < y.(string)
})
```

Happy to submit a PR for these if approach makes sense.

Pull request:
Fix ECR Repo policy eternally updating
### Description of your changes
Resolves a couple of issues around detecting changes on the ECR
`RepositoryPolicy` type:
  1. Converts `awsAccountId` principals that were not specified as an
     ARN into the ARN format, as AWS always returns in this format from
     the API.
  2. Makes sure to sort string slices within the policy document during
     comparison, to avoid issues where AWS returns values in a different
     order.

Fixes #700

Signed-off-by: Ben Agricola <bagricola@squiz.co.uk>
I have:

- [x] Read and followed Crossplane's [contribution process].
- [x] Run `make reviewable test` to ensure this PR is ready for review.

### How has this code been tested

Additional test added to compare policies with defined principals as a string slice, that tests both of these cases.
That test fails without the fixes applied.

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/pkg/clients/ecr/repository_policy.go b/pkg/clients/ecr/repository_policy.go
index e509f91ea..cd90d57cd 100644
--- a/pkg/clients/ecr/repository_policy.go
+++ b/pkg/clients/ecr/repository_policy.go
@@ -4,11 +4,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"strconv"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/aws/awserr"
 	"github.com/aws/aws-sdk-go-v2/service/ecr"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 
 	"github.com/crossplane/provider-aws/apis/ecr/v1alpha1"
 	awsclient "github.com/crossplane/provider-aws/pkg/clients"
@@ -146,6 +148,16 @@ func SerializeRepositoryPrincipal(p *v1alpha1.RepositoryPrincipal) (interface{},
 func SerializeAWSPrincipal(p v1alpha1.AWSPrincipal) *string {
 	switch {
 	case p.AWSAccountID != nil:
+		// Note: AWS Docs say you can specify the account ID either
+		// raw or as an ARN, but AWS actually converts internally to
+		// the ARN format, which is problematic for checking if we're
+		// up to date. So here we just do the conversion ourselves if
+		// we were given a string containing a number that looks like an
+		// AWS account ID (looks like a 12-digit integer).
+		if _, err := strconv.ParseInt(*p.AWSAccountID, 10, 64); err == nil {
+			s := fmt.Sprintf("arn:aws:iam::%s:root", *p.AWSAccountID)
+			return &s
+		}
 		return p.AWSAccountID
 	case p.IAMRoleARN != nil:
 		return p.IAMRoleARN
@@ -209,7 +221,19 @@ func IsRepositoryPolicyUpToDate(local, remote *string) bool {
 		return false
 	}
 
-	return cmp.Equal(localUnmarshalled, remoteUnmarshalled)
+	sortSlicesOpt := cmpopts.SortSlices(func(x, y interface{}) bool {
+		if a, ok := x.(string); ok {
+			if b, ok := y.(string); ok {
+				return a < b
+			}
+		}
+		// Note: Unknown types in slices will not cause a panic, but
+		// may not be sorted correctly. Depending on how AWS handles
+		// these, it may cause constant updates - but better this than
+		// panicing.
+		return false
+	})
+	return cmp.Equal(localUnmarshalled, remoteUnmarshalled, cmpopts.EquateEmpty(), sortSlicesOpt)
 }
 
 // RawPolicyData parses and formats the RepositoryPolicy struct


Test patch:
diff --git a/pkg/clients/ecr/repository_policy_test.go b/pkg/clients/ecr/repository_policy_test.go
index 34381f390..b497c167d 100644
--- a/pkg/clients/ecr/repository_policy_test.go
+++ b/pkg/clients/ecr/repository_policy_test.go
@@ -20,7 +20,10 @@ var (
 	boolCheck   = true
 	testID      = "id"
 	policy      = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"*"}],"Version":"2012-10-17"}`
-	params      = v1alpha1.RepositoryPolicyParameters{
+	cpxPolicy   = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":{"AWS":["arn:aws:iam::111122223333:userARN","111122223334","arn:aws:iam::111122223333:roleARN"]}}],"Version":"2012-10-17"}`
+	// Note: different sort order of principals than input above
+	cpxRemPolicy = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":{"AWS":["111122223334","arn:aws:iam::111122223333:userARN","arn:aws:iam::111122223333:roleARN"]}}],"Version":"2012-10-17"}`
+	params       = v1alpha1.RepositoryPolicyParameters{
 		Policy: &v1alpha1.RepositoryPolicyBody{
 			Version: "2012-10-17",
 			Statements: []v1alpha1.RepositoryPolicyStatement{
@@ -117,7 +120,9 @@ func TestSerializeRepositoryPolicyStatement(t *testing.T) {
 							IAMUserARN: aws.String("arn:aws:iam::111122223333:userARN"),
 						},
 						{
-							AWSAccountID: aws.String("111122223333"),
+							// Note: should be converted to full ARN when serialized
+							// to avoid needless updates.
+							AWSAccountID: aws.String("111122223334"),
 						},
 						{
 							IAMRoleARN: aws.String("arn:aws:iam::111122223333:roleARN"),
@@ -141,7 +146,7 @@ func TestSerializeRepositoryPolicyStatement(t *testing.T) {
 					},
 				}),
 			),
-			out: `{"Condition":{"test":{"test":"testKey","test2":"testKey2"}},"Action":"ecr:DescribeRepositories","Effect":"Allow","Principal":{"AWS":["arn:aws:iam::111122223333:userARN","111122223333","arn:aws:iam::111122223333:roleARN"]},"Sid":"1"}`,
+			out: `{"Condition":{"test":{"test":"testKey","test2":"testKey2"}},"Action":"ecr:DescribeRepositories","Effect":"Allow","Principal":{"AWS":["arn:aws:iam::111122223333:userARN","arn:aws:iam::111122223334:root","arn:aws:iam::111122223333:roleARN"]},"Sid":"1"}`,
 		},
 	}
 
@@ -228,6 +233,22 @@ func TestIsRepositoryPolicyUpToDate(t *testing.T) {
 			},
 			want: false,
 		},
+		"SameFieldsPrincipalPolicy": {
+			args: args{
+				local:  cpxPolicy,
+				remote: cpxRemPolicy,
+			},
+			want: true,
+		},
+		"SameFieldsNumericPrincipals": {
+			args: args{
+				// This is to test that our slice sorting does not
+				// panic with unexpected value types.
+				local:  `{"Statement":[{"Effect":"Allow","Action":"ecr:ListImages","Principal":[2,1,"foo","bar"]}],"Version":"2012-10-17"}`,
+				remote: `{"Statement":[{"Effect":"Allow","Action":"ecr:ListImages","Principal":[2,1,"bar","foo"]}],"Version":"2012-10-17"}`,
+			},
+			want: true,
+		},
 	}
 
 	for name, tc := range cases {