Repository: crossplane/provider-aws (go)

Issue:
ECR RepositoryPolicy constantly reconciles
### What happened?
Seeing a very high rate (upwards of 10 per minute per resource) of reconciles for `repositorypolicy.ecr.aws.crossplane.io` against `provider-aws` master, even though all policies exist, are up to date and not late-initialized.

Given that there can only be one policy per repository, I would expect to see a similar reconcile rate as seen with repositories but this is not the case.

It looks like this is caused by the AWS API constantly returning array items inside my repository policy (in this case AWS Principals) in a different order. I've confirmed this locally - no two subsequent calls to `aws ecr get-repository-policy` for a particular repository returns the same policy order.

This is correctly ignored by `provider-aws` and marked as up to date (as we ignore sort order when comparing the policy as of #701), but it looks like when the controller persists the 'updated' resource `status` back to the Kubernetes API, it causes the requeue period that `provider-aws` requested to be ignored, and the resource is requeued straight away.

This ends up in an endless loop of updates that has some other nasty side effects like blowing out the AWS SDK connection pool.

### How can we reproduce it?

Apply an ECR `repositorypolicy` with the following spec, with many different AWS account ID's:

```yaml
spec:
   deletionPolicy: Delete
   forProvider:
     policy:
       id: AllowAccounts
       statements:
       - action:
         - ecr:BatchCheckLayerAvailability
         - ecr:BatchGetImage
         - ecr:GetDownloadUrlForLayer
         effect: Allow
         principal:
           awsPrincipals:
           - awsAccountId: arn:aws:iam::3398....7229:root
           - awsAccountId: arn:aws:iam::9718....6735:root
           - awsAccountId: arn:aws:iam::8433....0030:root
           - awsAccountId: arn:aws:iam::4883....5958:root
           - awsAccountId: arn:aws:iam::9418....5648:root
           - awsAccountId: arn:aws:iam::3468....4853:root
         sid: AllowPullNonProdAccounts
       version: "2008-10-17"
     region: us-east-1
```

And then monitor the number of reconciles. I worked out what was going on by dumping the policy inside kubernetes at set intervals and then diffing the yaml output.

### What environment did it happen in?
Crossplane version: v1.3
Provider-AWS version: 009f048d509ba924ce6c456740ab946666a49bb6

### Fix
I wonder if `policyText` should even be available on this resource, as this is just a different representation of a user-defined setting and the resource should be updated if these don't match.

Pull request:
Remove ECR policyText observation
### Description of your changes

Amazon ECR's `GetRepositoryPolicyRequest` API appears to return policy
statement entries in any order it feels like - this is properly handled
by Crossplane when identifying if a policy needs an update, but we fall
foul of Kubernetes' handling of resource watches, where the changing
`policyText` contents of the resource trigger a new reconcile straight
after the previous one.

This causes constant API observations, and with enough ECR policies and
a short enough poll interval, blows out the connection pool of the AWS
SDK.

Removing the `policyText` observation field and using the data available
from the API response call to detect updates avoids this situation from 
occurring.

Signed-off-by: Ben Agricola <bagricola@squiz.co.uk>

Fixes #780 

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

Extra test added to make sure the resource correctly returns up- and not-up-to-date on appropriate policies. Have tested on a real cluster that was experiencing the issue in question to confirm that it resolves the original problem.

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/apis/ecr/v1alpha1/policy_types.go b/apis/ecr/v1alpha1/policy_types.go
index dc265dbab..9b9d5a143 100644
--- a/apis/ecr/v1alpha1/policy_types.go
+++ b/apis/ecr/v1alpha1/policy_types.go
@@ -235,10 +235,7 @@ type RepositoryPolicySpec struct {
 }
 
 // RepositoryPolicyObservation keeps the state for the external resource
-type RepositoryPolicyObservation struct {
-	// The JSON repository policy text associated with the repository.
-	PolicyText string `json:"policyText,omitempty"`
-}
+type RepositoryPolicyObservation struct{}
 
 // A RepositoryPolicyStatus represents the observed state of a repository policy
 type RepositoryPolicyStatus struct {
diff --git a/package/crds/ecr.aws.crossplane.io_repositorypolicies.yaml b/package/crds/ecr.aws.crossplane.io_repositorypolicies.yaml
index 75065bd9a..1739817d5 100644
--- a/package/crds/ecr.aws.crossplane.io_repositorypolicies.yaml
+++ b/package/crds/ecr.aws.crossplane.io_repositorypolicies.yaml
@@ -485,11 +485,6 @@ spec:
               atProvider:
                 description: RepositoryPolicyObservation keeps the state for the external
                   resource
-                properties:
-                  policyText:
-                    description: The JSON repository policy text associated with the
-                      repository.
-                    type: string
                 type: object
               conditions:
                 description: Conditions of the resource.
diff --git a/pkg/controller/ecr/repositorypolicy/controller.go b/pkg/controller/ecr/repositorypolicy/controller.go
index 9919fdc58..895ee20cd 100644
--- a/pkg/controller/ecr/repositorypolicy/controller.go
+++ b/pkg/controller/ecr/repositorypolicy/controller.go
@@ -109,10 +109,6 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 		return managed.ExternalObservation{}, errors.Wrap(err, errGet)
 	}
 
-	if response.PolicyText != nil {
-		cr.Status.AtProvider.PolicyText = *response.PolicyText
-	}
-
 	current := cr.Spec.ForProvider.DeepCopy()
 	ecr.LateInitializeRepositoryPolicy(&cr.Spec.ForProvider, response)
 
@@ -120,7 +116,7 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 
 	return managed.ExternalObservation{
 		ResourceExists:          true,
-		ResourceUpToDate:        ecr.IsRepositoryPolicyUpToDate(&policyData, &cr.Status.AtProvider.PolicyText),
+		ResourceUpToDate:        ecr.IsRepositoryPolicyUpToDate(&policyData, response.PolicyText),
 		ResourceLateInitialized: !cmp.Equal(current, &cr.Spec.ForProvider),
 	}, nil
 }


Test patch:
diff --git a/pkg/controller/ecr/repositorypolicy/controller_test.go b/pkg/controller/ecr/repositorypolicy/controller_test.go
index dcd844455..7aaa46e69 100644
--- a/pkg/controller/ecr/repositorypolicy/controller_test.go
+++ b/pkg/controller/ecr/repositorypolicy/controller_test.go
@@ -41,10 +41,11 @@ import (
 
 var (
 	// an arbitrary managed resource
-	unexpectedItem resource.Managed
-	repositoryName = "testRepo"
-	policy         = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"*"}],"Version":"2012-10-17"}`
-	boolCheck      = true
+	unexpectedItem   resource.Managed
+	repositoryName   = "testRepo"
+	policy           = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"*"}],"Version":"2012-10-17"}`
+	needUpdatePolicy = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"blah"}],"Version":"2012-10-17"}`
+	boolCheck        = true
 
 	params = v1alpha1.RepositoryPolicyParameters{
 		Policy: &v1alpha1.RepositoryPolicyBody{
@@ -61,10 +62,6 @@ var (
 		},
 	}
 
-	observation = v1alpha1.RepositoryPolicyObservation{
-		PolicyText: policy,
-	}
-
 	errBoom = errors.New("boom")
 )
 
@@ -80,10 +77,6 @@ func withConditions(c ...xpv1.Condition) repositoryPolicyModifier {
 	return func(r *v1alpha1.RepositoryPolicy) { r.Status.ConditionedStatus.Conditions = c }
 }
 
-func withObservation(o *v1alpha1.RepositoryPolicyObservation) repositoryPolicyModifier {
-	return func(r *v1alpha1.RepositoryPolicy) { r.Status.AtProvider = *o }
-}
-
 func withPolicy(s *v1alpha1.RepositoryPolicyParameters) repositoryPolicyModifier {
 	return func(r *v1alpha1.RepositoryPolicy) { r.Spec.ForProvider = *s }
 }
@@ -132,7 +125,6 @@ func TestObserve(t *testing.T) {
 			},
 			want: want{
 				cr: repositoryPolicy(withPolicy(&params),
-					withObservation(&observation),
 					withConditions(xpv1.Available())),
 				result: managed.ExternalObservation{
 					ResourceExists:   true,
@@ -149,6 +141,28 @@ func TestObserve(t *testing.T) {
 				err: errors.New(errUnexpectedObject),
 			},
 		},
+		"NeedUpdateInput": {
+			args: args{
+				ecr: &fake.MockRepositoryPolicyClient{
+					MockGet: func(input *awsecr.GetRepositoryPolicyInput) awsecr.GetRepositoryPolicyRequest {
+						return awsecr.GetRepositoryPolicyRequest{
+							Request: &aws.Request{HTTPRequest: &http.Request{}, Retryer: aws.NoOpRetryer{}, Data: &awsecr.GetRepositoryPolicyOutput{
+								PolicyText: &needUpdatePolicy,
+							}},
+						}
+					},
+				},
+				cr: repositoryPolicy(withPolicy(&params)),
+			},
+			want: want{
+				cr: repositoryPolicy(withPolicy(&params),
+					withConditions(xpv1.Available())),
+				result: managed.ExternalObservation{
+					ResourceExists:   true,
+					ResourceUpToDate: false,
+				},
+			},
+		},
 		"ClientError": {
 			args: args{
 				ecr: &fake.MockRepositoryPolicyClient{