Repository: crossplane/provider-aws (go)

Issue:
iam.IAMRole.AssumeRolePolicyDocument: IsUpToDate fails, constandly updating
### What happened?
IAMRole is constantly updating because the policy order changes

With the following policy:
```yaml
{
    "Version":"2012-10-17",
    "Statement":[
        {
            "Effect":"Allow",
            "Principal":{
                "Federated":"arn:aws:iam::...:oidc-provider/oidc.eks.eu-north-1.amazonaws.com/id/..."
            },
            "Action":"sts:AssumeRoleWithWebIdentity",
            "Condition":{
                "StringEquals":{
                    "oidc.eks.eu-north-1.amazonaws.com/id/...:a":"system:serviceaccount:default:app",
                    "oidc.eks.eu-north-1.amazonaws.com/id/...:b":"sts.amazonaws.com"
                }
            }
        }
    ]
}
```

I see that the two map elements in `StringEquals` changes order.

### How can we reproduce it?
See policy above.

### What environment did it happen in?
Crossplane version: 0.19.0
EKS 1.21



Pull request description:
iam.iamrole: Use improved policy comparision function
### Description of your changes
Reuse the improved code for IAM policy comparision from the ECR package.

Currently two IAM assume role policy documents do not match when they
use maps in condition keys, because JSON-encoded Go maps are not ordered.

This has already been handled in the ECR package, so we can reuse this
code. It should probably be moved to a common package.

Add a test to trigger a diff in a policy, and also verify that the error
code from diff is as expected.

Updates #704
Fixes #792

cc @benagricola 

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
* existing unit test works
* "for ever updating" IAMRole in test env is now synced and ready
* modified iam policy in aws console, verified that it was rolled back to the desired state

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/pkg/clients/aws.go b/pkg/clients/aws.go
index a05e90677..7cd91a7f9 100644
--- a/pkg/clients/aws.go
+++ b/pkg/clients/aws.go
@@ -42,6 +42,8 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	jsonpatch "github.com/evanphx/json-patch"
 	"github.com/go-ini/ini"
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -650,6 +652,36 @@ func DiffLabels(local, remote map[string]string) (addOrModify map[string]string,
 	return
 }
 
+// IsPolicyUpToDate Marshall policies to json for a compare to get around string ordering
+func IsPolicyUpToDate(local, remote *string) bool {
+	var localUnmarshalled interface{}
+	var remoteUnmarshalled interface{}
+
+	var err error
+	err = json.Unmarshal([]byte(*local), &localUnmarshalled)
+	if err != nil {
+		return false
+	}
+	err = json.Unmarshal([]byte(*remote), &remoteUnmarshalled)
+	if err != nil {
+		return false
+	}
+
+	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)
+}
+
 // CleanError Will remove the requestID from a awserr.Error and return a new awserr.
 // If not awserr it will return the original error
 func CleanError(err error) error {
diff --git a/pkg/clients/ecr/repository_policy.go b/pkg/clients/ecr/repository_policy.go
index cd90d57cd..d313a9f45 100644
--- a/pkg/clients/ecr/repository_policy.go
+++ b/pkg/clients/ecr/repository_policy.go
@@ -9,8 +9,6 @@ import (
 	"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"
@@ -206,36 +204,6 @@ func tryFirst(slc []string) interface{} {
 	return slc
 }
 
-// IsRepositoryPolicyUpToDate Marshall policies to json for a compare to get around string ordering
-func IsRepositoryPolicyUpToDate(local, remote *string) bool {
-	var localUnmarshalled interface{}
-	var remoteUnmarshalled interface{}
-
-	var err error
-	err = json.Unmarshal([]byte(*local), &localUnmarshalled)
-	if err != nil {
-		return false
-	}
-	err = json.Unmarshal([]byte(*remote), &remoteUnmarshalled)
-	if err != nil {
-		return false
-	}
-
-	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
 func RawPolicyData(original *v1alpha1.RepositoryPolicy) (string, error) {
 	if original == nil {
diff --git a/pkg/clients/iam/iamrole.go b/pkg/clients/iam/iamrole.go
index d7085ea30..c404ec2b9 100644
--- a/pkg/clients/iam/iamrole.go
+++ b/pkg/clients/iam/iamrole.go
@@ -2,6 +2,7 @@ package iam
 
 import (
 	"encoding/json"
+	"net/url"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/iam"
@@ -17,8 +18,9 @@ import (
 )
 
 const (
-	errCheckUpToDate    = "unable to determine if external resource is up to date"
-	errPolicyJSONEscape = "malformed AssumeRolePolicyDocument JSON"
+	errCheckUpToDate      = "unable to determine if external resource is up to date"
+	errPolicyJSONEscape   = "malformed AssumeRolePolicyDocument JSON"
+	errPolicyJSONUnescape = "malformed AssumeRolePolicyDocument escaping"
 )
 
 // RoleClient is the external client used for IAMRole Custom Resource
@@ -136,6 +138,24 @@ func CreatePatch(in *iam.Role, target *v1beta1.IAMRoleParameters) (*v1beta1.IAMR
 	return patch, nil
 }
 
+func isAssumeRolePolicyUpToDate(a, b *string) (bool, error) {
+	if a == nil || b == nil {
+		return a == b, nil
+	}
+
+	jsonA, err := url.QueryUnescape(*a)
+	if err != nil {
+		return false, errors.Wrap(err, errPolicyJSONUnescape)
+	}
+
+	jsonB, err := url.QueryUnescape(*b)
+	if err != nil {
+		return false, errors.Wrap(err, errPolicyJSONUnescape)
+	}
+
+	return awsclients.IsPolicyUpToDate(&jsonA, &jsonB), nil
+}
+
 // IsRoleUpToDate checks whether there is a change in any of the modifiable fields in role.
 func IsRoleUpToDate(in v1beta1.IAMRoleParameters, observed iam.Role) (bool, string, error) {
 	generated, err := copystructure.Copy(&observed)
@@ -151,16 +171,20 @@ func IsRoleUpToDate(in v1beta1.IAMRoleParameters, observed iam.Role) (bool, stri
 		return false, "", err
 	}
 
-	diff := cmp.Diff(desired, &observed, cmpopts.IgnoreInterfaces(struct{ resource.AttributeReferencer }{}))
+	policyUpToDate, err := isAssumeRolePolicyUpToDate(desired.AssumeRolePolicyDocument, observed.AssumeRolePolicyDocument)
+	if err != nil {
+		return false, "", err
+	}
 
-	if diff == "" {
+	diff := cmp.Diff(desired, &observed, cmpopts.IgnoreInterfaces(struct{ resource.AttributeReferencer }{}), cmpopts.IgnoreFields(observed, "AssumeRolePolicyDocument"))
+	if diff == "" && policyUpToDate {
 		return true, diff, nil
 	}
 
 	diff = "Found observed difference in IAM role\n" + diff
 
 	// Add extra logging for AssumeRolePolicyDocument because cmp.Diff doesn't show the full difference
-	if *desired.AssumeRolePolicyDocument != *observed.AssumeRolePolicyDocument {
+	if !policyUpToDate {
 		diff += "\ndesired assume role policy: "
 		diff += *desired.AssumeRolePolicyDocument
 		diff += "\nobserved assume role policy: "
diff --git a/pkg/controller/ecr/repositorypolicy/controller.go b/pkg/controller/ecr/repositorypolicy/controller.go
index 895ee20cd..466932327 100644
--- a/pkg/controller/ecr/repositorypolicy/controller.go
+++ b/pkg/controller/ecr/repositorypolicy/controller.go
@@ -116,7 +116,7 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 
 	return managed.ExternalObservation{
 		ResourceExists:          true,
-		ResourceUpToDate:        ecr.IsRepositoryPolicyUpToDate(&policyData, response.PolicyText),
+		ResourceUpToDate:        awsclient.IsPolicyUpToDate(&policyData, response.PolicyText),
 		ResourceLateInitialized: !cmp.Equal(current, &cr.Spec.ForProvider),
 	}, nil
 }


Test patch:
diff --git a/pkg/clients/aws_test.go b/pkg/clients/aws_test.go
index a9579a04a..37ea9afaf 100644
--- a/pkg/clients/aws_test.go
+++ b/pkg/clients/aws_test.go
@@ -457,3 +457,70 @@ func TestDiffLabels(t *testing.T) {
 		})
 	}
 }
+
+const (
+	policy = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"*"}],"Version":"2012-10-17"}`
+
+	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"}`
+)
+
+func TestIsPolicyUpToDate(t *testing.T) {
+	type args struct {
+		local  string
+		remote string
+	}
+
+	cases := map[string]struct {
+		args args
+		want bool
+	}{
+		"SameFields": {
+			args: args{
+				local:  "{\"testone\": \"one\", \"testtwo\": \"two\"}",
+				remote: "{\"testtwo\": \"two\", \"testone\": \"one\"}",
+			},
+			want: true,
+		},
+		"SameFieldsRealPolicy": {
+			args: args{
+				local:  policy,
+				remote: `{"Statement":[{"Effect":"Allow","Action":"ecr:ListImages","Principal":"*"}],"Version":"2012-10-17"}`,
+			},
+			want: true,
+		},
+		"DifferentFields": {
+			args: args{
+				local:  "{\"testone\": \"one\", \"testtwo\": \"two\"}",
+				remote: "{\"testthree\": \"three\", \"testone\": \"one\"}",
+			},
+			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 {
+		t.Run(name, func(t *testing.T) {
+			got := IsPolicyUpToDate(&tc.args.local, &tc.args.remote)
+			if diff := cmp.Diff(tc.want, got); diff != "" {
+				t.Errorf("r: -want, +got:\n%s", diff)
+			}
+		})
+	}
+}
diff --git a/pkg/clients/ecr/repository_policy_test.go b/pkg/clients/ecr/repository_policy_test.go
index b497c167d..11ba86f56 100644
--- a/pkg/clients/ecr/repository_policy_test.go
+++ b/pkg/clients/ecr/repository_policy_test.go
@@ -20,10 +20,7 @@ var (
 	boolCheck   = true
 	testID      = "id"
 	policy      = `{"Statement":[{"Action":"ecr:ListImages","Effect":"Allow","Principal":"*"}],"Version":"2012-10-17"}`
-	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{
+	params      = v1alpha1.RepositoryPolicyParameters{
 		Policy: &v1alpha1.RepositoryPolicyBody{
 			Version: "2012-10-17",
 			Statements: []v1alpha1.RepositoryPolicyStatement{
@@ -202,65 +199,6 @@ func TestLateInitializePolicy(t *testing.T) {
 	}
 }
 
-func TestIsRepositoryPolicyUpToDate(t *testing.T) {
-	type args struct {
-		local  string
-		remote string
-	}
-
-	cases := map[string]struct {
-		args args
-		want bool
-	}{
-		"SameFields": {
-			args: args{
-				local:  "{\"testone\": \"one\", \"testtwo\": \"two\"}",
-				remote: "{\"testtwo\": \"two\", \"testone\": \"one\"}",
-			},
-			want: true,
-		},
-		"SameFieldsRealPolicy": {
-			args: args{
-				local:  policy,
-				remote: `{"Statement":[{"Effect":"Allow","Action":"ecr:ListImages","Principal":"*"}],"Version":"2012-10-17"}`,
-			},
-			want: true,
-		},
-		"DifferentFields": {
-			args: args{
-				local:  "{\"testone\": \"one\", \"testtwo\": \"two\"}",
-				remote: "{\"testthree\": \"three\", \"testone\": \"one\"}",
-			},
-			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 {
-		t.Run(name, func(t *testing.T) {
-			got := IsRepositoryPolicyUpToDate(&tc.args.local, &tc.args.remote)
-			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("r: -want, +got:\n%s", diff)
-			}
-		})
-	}
-}
-
 func TestFormat(t *testing.T) {
 	type formatarg struct {
 		cr *v1alpha1.RepositoryPolicy
diff --git a/pkg/clients/iam/iamrole_test.go b/pkg/clients/iam/iamrole_test.go
index 9d9a22653..560c66875 100644
--- a/pkg/clients/iam/iamrole_test.go
+++ b/pkg/clients/iam/iamrole_test.go
@@ -29,6 +29,21 @@ var (
 		  }
 		]
 	   }`
+	assumeRolePolicyDocument2 = `{
+		"Version": "2012-10-17",
+		"Statement": [
+		  {
+			"Effect": "Allow",
+			"Principal": {
+			  "Service": "eks.amazonaws.com"
+			},
+			"Action": "sts:AssumeRole",
+			"Condition": {
+			  "StringEquals": {"foo": "bar"}
+			}
+		  }
+		]
+	   }`
 	roleID   = "some Id"
 	roleName = "some name"
 	tagKey   = "key"
@@ -254,6 +269,27 @@ func TestIsRoleUpToDate(t *testing.T) {
 			want:     true,
 			wantDiff: "",
 		},
+		"DifferentPolicy": {
+			args: args{
+				role: iam.Role{
+					AssumeRolePolicyDocument: escapedPolicyJSON(),
+					Description:              &description,
+					MaxSessionDuration:       aws.Int64(1),
+					Path:                     aws.String("/"),
+				},
+				p: v1beta1.IAMRoleParameters{
+					Description:              &description,
+					AssumeRolePolicyDocument: assumeRolePolicyDocument2,
+					MaxSessionDuration:       aws.Int64(1),
+					Path:                     aws.String("/"),
+				},
+			},
+			want: false,
+			wantDiff: `Found observed difference in IAM role
+
+desired assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B%22Effect%22%3A%22Allow%22%2C%22Principal%22%3A%7B%22Service%22%3A%22eks.amazonaws.com%22%7D%2C%22Action%22%3A%22sts%3AAssumeRole%22%2C%22Condition%22%3A%7B%22StringEquals%22%3A%7B%22foo%22%3A%22bar%22%7D%7D%7D%5D%7D
+observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%22%3A%5B%7B%22Effect%22%3A%22Allow%22%2C%22Principal%22%3A%7B%22Service%22%3A%22eks.amazonaws.com%22%7D%2C%22Action%22%3A%22sts%3AAssumeRole%22%7D%5D%7D`,
+		},
 		"DifferentFields": {
 			args: args{
 				role: iam.Role{
@@ -284,7 +320,10 @@ func TestIsRoleUpToDate(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			got, testDiff, _ := IsRoleUpToDate(tc.args.p, tc.args.role)
+			got, testDiff, err := IsRoleUpToDate(tc.args.p, tc.args.role)
+			if err != nil {
+				t.Errorf("r: unexpected error: %v", err)
+			}
 			if diff := cmp.Diff(tc.want, got); diff != "" {
 				t.Errorf("r: -want, +got:\n%s", diff)
 			}