Use new policy checker for iam.roles

pkg/clients/iam/role.go

@@ -12,19 +12,13 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/resource"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
-	"github.com/pkg/errors"
+	"k8s.io/utils/ptr"
 
 	"github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1"
 	"github.com/crossplane-contrib/provider-aws/pkg/clients/iam/convert"
 	"github.com/crossplane-contrib/provider-aws/pkg/utils/jsonpatch"
 	"github.com/crossplane-contrib/provider-aws/pkg/utils/pointer"
-	legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old"
-)
-
-const (
-	errCheckUpToDate      = "unable to determine if external resource is up to date"
-	errPolicyJSONEscape   = "malformed AssumeRolePolicyDocument JSON"
-	errPolicyJSONUnescape = "malformed AssumeRolePolicyDocument escaping"
+	"github.com/crossplane-contrib/provider-aws/pkg/utils/policy"
 )
 
 // RoleClient is the external client used for Role Custom Resource
@@ -89,14 +83,18 @@ func GenerateRoleObservation(role iamtypes.Role) v1beta1.RoleExternalStatus {
 
 // GenerateRole assigns the in RoleParamters to role.
 func GenerateRole(in v1beta1.RoleParameters, role *iamtypes.Role) error {
-	if in.AssumeRolePolicyDocument != "" {
-		s, err := legacypolicy.CompactAndEscapeJSON(in.AssumeRolePolicyDocument)
-		if err != nil {
-			return errors.Wrap(err, errPolicyJSONEscape)
+	// iamtypes.Role has url-encoded policy document, while RoleParameters has plain.
+	// Assign policy from `in` only if it is different from the one in `role`.
+	if escapedPolicyDoc := role.AssumeRolePolicyDocument; escapedPolicyDoc != nil {
+		policyDoc, err := url.QueryUnescape(*escapedPolicyDoc)
+		if err != nil || !policy.ArePolicyDocumentsEqual(policyDoc, in.AssumeRolePolicyDocument) {
+			role.AssumeRolePolicyDocument = nil
 		}
-
-		role.AssumeRolePolicyDocument = &s
 	}
+	if role.AssumeRolePolicyDocument == nil && in.AssumeRolePolicyDocument != "" {
+		role.AssumeRolePolicyDocument = ptr.To(url.QueryEscape(in.AssumeRolePolicyDocument))
+	}
+
 	role.Description = in.Description
 	role.MaxSessionDuration = in.MaxSessionDuration
 	role.Path = in.Path
@@ -158,53 +156,22 @@ func CreatePatch(in *iamtypes.Role, target *v1beta1.RoleParameters) (*v1beta1.Ro
 	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 legacypolicy.IsPolicyUpToDate(&jsonA, &jsonB), nil
-}
-
 // IsRoleUpToDate checks whether there is a change in any of the modifiable fields in role.
 func IsRoleUpToDate(in v1beta1.RoleParameters, observed iamtypes.Role) (bool, string, error) {
 	desired := (&convert.ConverterImpl{}).DeepCopyAWSRole(&observed)
 	if err := GenerateRole(in, desired); err != nil {
 		return false, "", err
 	}
 
-	policyUpToDate, err := isAssumeRolePolicyUpToDate(desired.AssumeRolePolicyDocument, observed.AssumeRolePolicyDocument)
-	if err != nil {
-		return false, "", err
-	}
-
 	diff := cmp.Diff(desired, &observed,
 		cmpopts.IgnoreInterfaces(struct{ resource.AttributeReferencer }{}),
-		cmpopts.IgnoreFields(observed, "AssumeRolePolicyDocument", "CreateDate", "PermissionsBoundary.PermissionsBoundaryType", "RoleLastUsed"),
+		cmpopts.IgnoreFields(observed, "CreateDate", "PermissionsBoundary.PermissionsBoundaryType", "RoleLastUsed"),
 		cmpopts.IgnoreTypes(document.NoSerde{}), cmpopts.SortSlices(lessTag))
-	if diff == "" && policyUpToDate {
+	if diff == "" {
 		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 !policyUpToDate {
-		diff += "\ndesired assume role policy: "
-		diff += *desired.AssumeRolePolicyDocument
-		diff += "\nobserved assume role policy: "
-		diff += *observed.AssumeRolePolicyDocument
-	}
 	return false, diff, nil
 }
 

pkg/clients/iam/role_test.go

@@ -1,7 +1,8 @@
 package iam
 
 import (
-	"strings"
+	"net/url"
+	"regexp"
 	"testing"
 	"time"
 
@@ -10,10 +11,10 @@ import (
 	"github.com/aws/smithy-go/document"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"k8s.io/utils/ptr"
 
 	"github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1"
 	"github.com/crossplane-contrib/provider-aws/pkg/utils/pointer"
-	legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old"
 )
 
 var (
@@ -31,8 +32,19 @@ var (
 		  }
 		]
 	   }`
+	assumeRolePolicyDocumentWithArrays = `{
+		"Statement": [
+			{
+				"Effect": "Allow",
+				"Principal": {
+					"Service": ["eks.amazonaws.com"]
+				},
+				"Action": ["sts:AssumeRole"]
+			}
+		],
+		"Version": "2012-10-17"
+	}`
 	assumeRolePolicyDocument2 = `{
-		"Version": "2012-10-17",
 		"Statement": [
 		  {
 			"Effect": "Allow",
@@ -44,15 +56,20 @@ var (
 			  "StringEquals": {"foo": "bar"}
 			}
 		  }
-		]
-	   }`
+		],
+		"Version": "2012-10-17"
+	}`
 	roleID             = "some Id"
 	roleName           = "some name"
 	tagKey             = "key"
 	tagValue           = "value"
 	permissionBoundary = "arn:aws:iam::111111111111:policy/permission-boundary"
 	createDate         = time.Now()
 	region             = "us-east-1"
+	// There are flaky failures when \s+ is used in line matchers to match diff lines.
+	// Instead, this regex collapses all whitespaces into a single space,
+	// and line matchers use single space.
+	compactSpaceRegex = regexp.MustCompile(`\s+`)
 )
 
 func roleParams(m ...func(*v1beta1.RoleParameters)) *v1beta1.RoleParameters {
@@ -69,14 +86,6 @@ func roleParams(m ...func(*v1beta1.RoleParameters)) *v1beta1.RoleParameters {
 	return o
 }
 
-func escapedPolicyJSON() *string {
-	p, err := legacypolicy.CompactAndEscapeJSON(assumeRolePolicyDocument)
-	if err == nil {
-		return &p
-	}
-	return nil
-}
-
 func role(m ...func(*iamtypes.Role)) *iamtypes.Role {
 	o := &iamtypes.Role{
 		Description:              &description,
@@ -256,12 +265,12 @@ func TestIsRoleUpToDate(t *testing.T) {
 	cases := map[string]struct {
 		args     args
 		want     bool
-		wantDiff string
+		wantDiff []*regexp.Regexp
 	}{
 		"SameFields": {
 			args: args{
 				role: iamtypes.Role{
-					AssumeRolePolicyDocument: escapedPolicyJSON(),
+					AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)),
 					Description:              &description,
 					MaxSessionDuration:       pointer.ToIntAsInt32(1),
 					Path:                     pointer.ToOrNilIfZeroValue("/"),
@@ -282,12 +291,38 @@ func TestIsRoleUpToDate(t *testing.T) {
 				},
 			},
 			want:     true,
-			wantDiff: "",
+			wantDiff: nil,
+		},
+		"SameFieldsWithDifferentPolicyFormat": {
+			args: args{
+				role: iamtypes.Role{
+					AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocumentWithArrays)),
+					Description:              &description,
+					MaxSessionDuration:       pointer.ToIntAsInt32(1),
+					Path:                     pointer.ToOrNilIfZeroValue("/"),
+					Tags: []iamtypes.Tag{{
+						Key:   pointer.ToOrNilIfZeroValue("key1"),
+						Value: pointer.ToOrNilIfZeroValue("value1"),
+					}},
+				},
+				p: v1beta1.RoleParameters{
+					Description:              &description,
+					AssumeRolePolicyDocument: assumeRolePolicyDocument,
+					MaxSessionDuration:       pointer.ToIntAsInt32(1),
+					Path:                     pointer.ToOrNilIfZeroValue("/"),
+					Tags: []v1beta1.Tag{{
+						Key:   "key1",
+						Value: "value1",
+					}},
+				},
+			},
+			want:     true,
+			wantDiff: nil,
 		},
 		"AWSInitializedFields": {
 			args: args{
 				role: iamtypes.Role{
-					AssumeRolePolicyDocument: escapedPolicyJSON(),
+					AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)),
 					CreateDate:               &createDate,
 					Description:              &description,
 					MaxSessionDuration:       pointer.ToIntAsInt32(1),
@@ -318,12 +353,12 @@ func TestIsRoleUpToDate(t *testing.T) {
 				},
 			},
 			want:     true,
-			wantDiff: "",
+			wantDiff: nil,
 		},
 		"DifferentPolicy": {
 			args: args{
 				role: iamtypes.Role{
-					AssumeRolePolicyDocument: escapedPolicyJSON(),
+					AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)),
 					Description:              &description,
 					MaxSessionDuration:       pointer.ToIntAsInt32(1),
 					Path:                     pointer.ToOrNilIfZeroValue("/"),
@@ -336,15 +371,16 @@ func TestIsRoleUpToDate(t *testing.T) {
 				},
 			},
 			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`,
+			wantDiff: []*regexp.Regexp{
+				regexp.MustCompile("Found observed difference in IAM role"),
+				regexp.MustCompile(`- AssumeRolePolicyDocument: &"(%\w\w)+Statement`),
+				regexp.MustCompile(`\+ AssumeRolePolicyDocument: &"(%\w\w)+Version`),
+			},
 		},
 		"DifferentFields": {
 			args: args{
 				role: iamtypes.Role{
-					AssumeRolePolicyDocument: &assumeRolePolicyDocument,
+					AssumeRolePolicyDocument: ptr.To(url.QueryEscape(assumeRolePolicyDocument)),
 					Description:              &description,
 					MaxSessionDuration:       pointer.ToIntAsInt32(1),
 					Path:                     pointer.ToOrNilIfZeroValue("//"),
@@ -364,8 +400,12 @@ observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%
 					}},
 				},
 			},
-			want:     false,
-			wantDiff: "Found observed difference in IAM role",
+			want: false,
+			wantDiff: []*regexp.Regexp{
+				regexp.MustCompile("Found observed difference in IAM role"),
+				regexp.MustCompile(`- Path: &"/"`),
+				regexp.MustCompile(`\+ Path: &"//"`),
+			},
 		},
 	}
 
@@ -378,15 +418,15 @@ observed assume role policy: %7B%22Version%22%3A%222012-10-17%22%2C%22Statement%
 			if diff := cmp.Diff(tc.want, got); diff != "" {
 				t.Errorf("r: -want, +got:\n%s", diff)
 			}
-			if tc.wantDiff == "" {
-				if tc.wantDiff != testDiff {
-					t.Errorf("r: -want, +got:\n%s", testDiff)
+			if tc.wantDiff == nil {
+				if diff := cmp.Diff("", testDiff); diff != "" {
+					t.Errorf("r: -want, +got:\n%s", diff)
 				}
-			}
-
-			if tc.wantDiff == "Found observed difference in IAM role" {
-				if !strings.Contains(testDiff, tc.wantDiff) {
-					t.Errorf("r: -want, +got:\n%s", testDiff)
+			} else {
+				for _, wantDiff := range tc.wantDiff {
+					if !wantDiff.MatchString(compactSpaceRegex.ReplaceAllString(testDiff, " ")) {
+						t.Errorf("expected:\n%s\nto match:\n%s", testDiff, wantDiff.String())
+					}
 				}
 			}
 		})

pkg/controller/iam/role/controller_test.go

@@ -18,6 +18,7 @@ package role
 
 import (
 	"context"
+	"net/url"
 	"testing"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
@@ -36,7 +37,6 @@ import (
 	"github.com/crossplane-contrib/provider-aws/pkg/clients/iam/fake"
 	errorutils "github.com/crossplane-contrib/provider-aws/pkg/utils/errors"
 	"github.com/crossplane-contrib/provider-aws/pkg/utils/pointer"
-	legacypolicy "github.com/crossplane-contrib/provider-aws/pkg/utils/policy/old"
 )
 
 var (
@@ -82,11 +82,7 @@ func withArn(s string) roleModifier {
 
 func withPolicy() roleModifier {
 	return func(r *v1beta1.Role) {
-		p, err := legacypolicy.CompactAndEscapeJSON(policy)
-		if err != nil {
-			return
-		}
-		r.Spec.ForProvider.AssumeRolePolicyDocument = p
+		r.Spec.ForProvider.AssumeRolePolicyDocument = url.QueryEscape(policy)
 	}
 }
 

pkg/utils/policy/compare.go

@@ -10,3 +10,17 @@ func ArePoliciesEqal(a, b *Policy) (equal bool, diff string) {
 	diff = cmp.Diff(a, b)
 	return diff == "", diff
 }
+
+// ArePolicyDocumentsEqual determines if the two policy documents can be considered equal.
+func ArePolicyDocumentsEqual(a, b string) bool {
+	policyA, err := ParsePolicyString(a)
+	if err != nil {
+		return a == b
+	}
+	policyB, err := ParsePolicyString(b)
+	if err != nil {
+		return false
+	}
+	eq, _ := ArePoliciesEqal(&policyA, &policyB)
+	return eq
+}