fix(iam/policy): Treat single items as array

Signed-off-by: Maximilian Blatt (external expert on behalf of DB Netz) <maximilian.blatt-extern@deutschebahn.com>

pkg/clients/iam/policy.go

@@ -2,17 +2,16 @@ package iam
 
 import (
 	"context"
-	"net/url"
 
 	"github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/iam"
 	iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
 	"github.com/aws/aws-sdk-go-v2/service/sts"
-	"github.com/google/go-cmp/cmp"
 
 	awsclients "github.com/crossplane-contrib/provider-aws/pkg/clients"
+	policyutils "github.com/crossplane-contrib/provider-aws/pkg/utils/policy"
 )
 
 // PolicyClient is the external client used for Policy Custom Resource
@@ -44,29 +43,21 @@ func NewSTSClient(cfg aws.Config) STSClient {
 }
 
 // IsPolicyUpToDate checks whether there is a change in any of the modifiable fields in policy.
-func IsPolicyUpToDate(in v1beta1.PolicyParameters, policy iamtypes.PolicyVersion) (bool, error) {
-	// The AWS API returns Policy Document as an escaped string.
-	// Due to differences in the methods to escape a string, the comparison result between
-	// the spec.Document and policy.Document can sometimes be false negative (due to spaces, line feeds).
-	// Escaping with a common method and then comparing is a safe way.
-
-	if *policy.Document == "" || in.Document == "" {
-		return false, nil
-	}
-
-	unescapedPolicy, err := url.QueryUnescape(aws.ToString(policy.Document))
-	if err != nil {
-		return false, nil // nolint:nilerr
+func IsPolicyUpToDate(in v1beta1.PolicyParameters, policy iamtypes.PolicyVersion) (bool, string, error) {
+	externalPolicyRaw := awsclients.StringValue(policy.Document)
+	if externalPolicyRaw == "" || in.Document == "" {
+		return false, "", nil
 	}
 
-	compactPolicy, err := awsclients.CompactAndEscapeJSON(unescapedPolicy)
+	externpolicy, err := policyutils.ParsePolicyString(externalPolicyRaw)
 	if err != nil {
-		return false, err
+		return false, "", err
 	}
-	compactSpecPolicy, err := awsclients.CompactAndEscapeJSON(in.Document)
+	specPolicy, err := policyutils.ParsePolicyString(in.Document)
 	if err != nil {
-		return false, err
+		return false, "", err
 	}
 
-	return cmp.Equal(compactPolicy, compactSpecPolicy), nil
+	areEqual, diff := policyutils.ArePoliciesEqal(&specPolicy, &externpolicy)
+	return areEqual, diff, nil
 }

pkg/clients/iam/policy_test.go

@@ -3,10 +3,11 @@ package iam
 import (
 	"testing"
 
-	"github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1"
-
 	iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"
+	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
+
+	"github.com/crossplane-contrib/provider-aws/apis/iam/v1beta1"
 )
 
 var (
@@ -35,17 +36,33 @@ var (
 		  }
 		]
 	   }`
+	document3 = `{
+		"Version": "2012-10-17",
+		"Statement": [
+		  {
+			"Effect": "Allow",
+			"Principal": {
+			  "Service": "eks.amazonaws.com"
+			},
+			"Action": ["sts:AssumeRole"]
+		  }
+		]
+	  }`
 )
 
 func TestIsPolicyUpToDate(t *testing.T) {
 	type args struct {
 		p       v1beta1.PolicyParameters
 		version iamtypes.PolicyVersion
 	}
+	type want struct {
+		areEqual bool
+		err      error
+	}
 
 	cases := map[string]struct {
 		args args
-		want bool
+		want want
 	}{
 		"SameFields": {
 			args: args{
@@ -56,7 +73,9 @@ func TestIsPolicyUpToDate(t *testing.T) {
 					Document: &document1,
 				},
 			},
-			want: true,
+			want: want{
+				areEqual: true,
+			},
 		},
 		"DifferentFields": {
 			args: args{
@@ -67,7 +86,9 @@ func TestIsPolicyUpToDate(t *testing.T) {
 					Document: &document2,
 				},
 			},
-			want: false,
+			want: want{
+				areEqual: false,
+			},
 		},
 		"EmptyPolicy": {
 			args: args{
@@ -76,16 +97,37 @@ func TestIsPolicyUpToDate(t *testing.T) {
 					Document: &document2,
 				},
 			},
-			want: false,
+			want: want{
+				areEqual: false,
+			},
+		},
+		"SameFieldsSingleAction": {
+			args: args{
+				p: v1beta1.PolicyParameters{
+					Document: document1,
+				},
+				version: iamtypes.PolicyVersion{
+					Document: &document3,
+				},
+			},
+			want: want{
+				areEqual: true,
+			},
 		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			got, _ := IsPolicyUpToDate(tc.args.p, tc.args.version)
-			if diff := cmp.Diff(tc.want, got); diff != "" {
+			areEqual, diff, err := IsPolicyUpToDate(tc.args.p, tc.args.version)
+			if diff := cmp.Diff(tc.want.areEqual, areEqual); diff != "" {
 				t.Errorf("r: -want, +got:\n%s", diff)
 			}
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("r: -want, +got:\n%s", diff)
+			}
+			if diff != "" {
+				t.Logf("r: -want, +got:\n%s", diff)
+			}
 		})
 	}
 }

pkg/controller/iam/policy/controller.go

@@ -154,7 +154,7 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 		return managed.ExternalObservation{}, awsclient.Wrap(err, errPolicyVersion)
 	}
 
-	update, err := iam.IsPolicyUpToDate(cr.Spec.ForProvider, *versionRsp.PolicyVersion)
+	update, diff, err := iam.IsPolicyUpToDate(cr.Spec.ForProvider, *versionRsp.PolicyVersion)
 
 	if err != nil {
 		return managed.ExternalObservation{}, awsclient.Wrap(err, errUpToDate)
@@ -169,6 +169,7 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E
 	return managed.ExternalObservation{
 		ResourceExists:   true,
 		ResourceUpToDate: update && areRolesUpdated,
+		Diff:             diff,
 	}, nil
 }