Repository: crossplane-contrib/provider-aws (go)

Issue:
SNS/Topic policy not applied by aws provider in crossplane
Given the following manifest, AWS provider in Crossplane does not apply the policy. Instead, it creates a Topic with the default policy.

```
apiVersion: sns.aws.crossplane.io/v1beta1
kind: Topic
metadata:
  annotations:
    argocd.argoproj.io/sync-wave: '-50'
  labels:
    app.kubernetes.io/instance: demo-application-dev
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: fairmoney-base-chart
    argocd.argoproj.io/instance: demo-application-dev
    fairmoney.io/application: demo-application
    helm.sh/chart: fairmoney-base-chart-0.1.0
  name: fairmoney-dev-demo-application-uploads
spec:
  deletionPolicy: Delete
  forProvider:
    name: fairmoney-dev-demo-application-uploads
    policy: |
      {
        "Version": "2008-10-17",
        "Statement": [
          {
            "Sid": "AllowPublishFromS3",
            "Effect": "Allow",
            "Principal": {
              "Service": "s3.amazonaws.com"
            },
            "Action": "sns:Publish",
            "Resource": "arn:aws:sns:eu-west-1:027159582536:fairmoney-dev-demo-application-uploads",
            "Condition": {
              "ArnLike": {
                "aws:SourceArn": [
                    "arn:aws:s3:*:*:fairmoney-dev-demo-application"
                ]
              }
            }
          }
        ]
      }
    region: eu-west-1
```
**Workaround:** adding or modifying the displayName parameter will also apply the policy.

Pull request description:
fix(sns): Topic policy update
### Description of your changes

Compare topic policies using structural diff add it to update attributes if they are not semantically equal.

Fixes #1896

Also fixed a bug in the generic resource policy parser regarding multi value conditions.

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

Unit tests and the existing topic example.

[contribution process]: https://git.io/fj2m9


Patch:
diff --git a/pkg/clients/sns/topic.go b/pkg/clients/sns/topic.go
index de9128f8c..5c526f13b 100644
--- a/pkg/clients/sns/topic.go
+++ b/pkg/clients/sns/topic.go
@@ -18,12 +18,13 @@ package sns
 
 import (
 	"context"
-	"errors"
 	"strconv"
 
 	"github.com/aws/aws-sdk-go-v2/aws"
 	"github.com/aws/aws-sdk-go-v2/service/sns"
 	snstypes "github.com/aws/aws-sdk-go-v2/service/sns/types"
+	"github.com/pkg/errors"
+	"k8s.io/utils/ptr"
 
 	"github.com/crossplane-contrib/provider-aws/apis/sns/v1beta1"
 	awsclients "github.com/crossplane-contrib/provider-aws/pkg/clients"
@@ -117,16 +118,24 @@ func LateInitializeTopicAttr(in *v1beta1.TopicParameters, attrs map[string]strin
 // Please see https://docs.aws.amazon.com/sns/latest/api/API_SetTopicAttributes.html
 // So we need to compare each topic attribute and call SetTopicAttribute for ones which has
 // changed.
-func GetChangedAttributes(p v1beta1.TopicParameters, attrs map[string]string) map[string]string {
+func GetChangedAttributes(p v1beta1.TopicParameters, attrs map[string]string) (map[string]string, error) {
 	topicAttrs := getTopicAttributes(p)
 	changedAttrs := make(map[string]string)
 	for k, v := range topicAttrs {
-		if v != attrs[k] {
+		if k == string(TopicPolicy) {
+			isPolicyUpToDate, err := isSNSPolicyUpToDate(v, attrs[string(TopicPolicy)])
+			if err != nil {
+				return nil, errors.Wrap(err, "cannot compare policies")
+			}
+			if !isPolicyUpToDate {
+				changedAttrs[k] = v
+			}
+		} else if v != attrs[k] {
 			changedAttrs[k] = v
 		}
 	}
 
-	return changedAttrs
+	return changedAttrs, nil
 }
 
 // GenerateTopicObservation is used to produce TopicObservation from attributes
@@ -156,7 +165,7 @@ func GenerateTopicObservation(attr map[string]string) v1beta1.TopicObservation {
 func IsSNSTopicUpToDate(p v1beta1.TopicParameters, attr map[string]string) (bool, error) {
 	fifoTopic, _ := strconv.ParseBool(attr[string(TopicFifoTopic)])
 
-	policyChanged, err := IsSNSPolicyChanged(p, attr)
+	isPolicyUpToDate, err := isSNSPolicyUpToDate(ptr.Deref(p.Policy, ""), attr[string(TopicPolicy)])
 	if err != nil {
 		return false, err
 	}
@@ -165,30 +174,26 @@ func IsSNSTopicUpToDate(p v1beta1.TopicParameters, attr map[string]string) (bool
 		aws.ToString(p.DisplayName) == attr[string(TopicDisplayName)] &&
 		aws.ToString(p.KMSMasterKeyID) == attr[string(TopicKmsMasterKeyID)] &&
 		aws.ToBool(p.FifoTopic) == fifoTopic &&
-		!policyChanged, nil
+		isPolicyUpToDate, nil
 }
 
 // IsSNSPolicyChanged determines whether a SNS topic policy needs to be updated
-func IsSNSPolicyChanged(p v1beta1.TopicParameters, attr map[string]string) (bool, error) {
-	currPolicyStr := attr[string(TopicPolicy)]
-	specPolicyStr := awsclients.StringValue(p.Policy)
-
-	if currPolicyStr == specPolicyStr {
+func isSNSPolicyUpToDate(specPolicyStr, currPolicyStr string) (bool, error) {
+	if specPolicyStr == "" {
+		return currPolicyStr == "", nil
+	} else if currPolicyStr == "" {
 		return false, nil
 	}
 
-	currPolicy, err := policyutils.ParsePolicyString(attr[string(TopicPolicy)])
+	currPolicy, err := policyutils.ParsePolicyString(currPolicyStr)
 	if err != nil {
-		return false, err
+		return false, errors.Wrap(err, "current policy")
 	}
-
-	specPolicy, err := policyutils.ParsePolicyString(awsclients.StringValue(p.Policy))
+	specPolicy, err := policyutils.ParsePolicyString(specPolicyStr)
 	if err != nil {
-		return false, err
+		return false, errors.Wrap(err, "spec policy")
 	}
-
 	equalPolicies, _ := policyutils.ArePoliciesEqal(&currPolicy, &specPolicy)
-
 	return equalPolicies, nil
 }
 
diff --git a/pkg/controller/sns/topic/controller.go b/pkg/controller/sns/topic/controller.go
index 5f30d82d3..69fd1df38 100644
--- a/pkg/controller/sns/topic/controller.go
+++ b/pkg/controller/sns/topic/controller.go
@@ -47,6 +47,7 @@ const (
 	errCreate           = "failed to create the SNS Topic"
 	errDelete           = "failed to delete the SNS Topic"
 	errUpdate           = "failed to update the SNS Topic"
+	errGetChangedAttr   = "failed to get changed topic attributes"
 )
 
 // SetupSNSTopic adds a controller that reconciles Topic.
@@ -176,7 +177,10 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex
 	}
 
 	// Update Topic Attributes
-	attrs := snsclient.GetChangedAttributes(cr.Spec.ForProvider, resp.Attributes)
+	attrs, err := snsclient.GetChangedAttributes(cr.Spec.ForProvider, resp.Attributes)
+	if err != nil {
+		return managed.ExternalUpdate{}, errors.Wrap(err, errGetChangedAttr)
+	}
 	for k, v := range attrs {
 		_, err = e.client.SetTopicAttributes(ctx, &awssns.SetTopicAttributesInput{
 			AttributeName:  aws.String(k),
diff --git a/pkg/utils/policy/convert.go b/pkg/utils/policy/convert.go
index 8c240e854..f71ddac4f 100644
--- a/pkg/utils/policy/convert.go
+++ b/pkg/utils/policy/convert.go
@@ -109,15 +109,19 @@ func convertResourcePolicyConditions(conditions []common.Condition) ConditionMap
 
 			switch {
 			case c.ConditionStringValue != nil:
-				set[c.ConditionKey] = *c.ConditionStringValue
+				set[c.ConditionKey] = ConditionSettingsValue{*c.ConditionStringValue}
 			case c.ConditionBooleanValue != nil:
-				set[c.ConditionKey] = *c.ConditionBooleanValue
+				set[c.ConditionKey] = ConditionSettingsValue{*c.ConditionBooleanValue}
 			case c.ConditionNumericValue != nil:
-				set[c.ConditionKey] = *c.ConditionNumericValue
+				set[c.ConditionKey] = ConditionSettingsValue{*c.ConditionNumericValue}
 			case c.ConditionDateValue != nil:
-				set[c.ConditionKey] = c.ConditionDateValue.Time.Format("2006-01-02T15:04:05-0700")
+				set[c.ConditionKey] = ConditionSettingsValue{c.ConditionDateValue.Time.Format("2006-01-02T15:04:05-0700")}
 			case c.ConditionListValue != nil:
-				set[c.ConditionKey] = c.ConditionListValue
+				listVal := make(ConditionSettingsValue, len(c.ConditionListValue))
+				for i, val := range c.ConditionListValue {
+					listVal[i] = val
+				}
+				set[c.ConditionKey] = listVal
 			}
 		}
 		m[cc.OperatorKey] = set
diff --git a/pkg/utils/policy/parse.go b/pkg/utils/policy/parse.go
index 4d2e633ec..ec642cac5 100644
--- a/pkg/utils/policy/parse.go
+++ b/pkg/utils/policy/parse.go
@@ -14,6 +14,15 @@ func ParsePolicyString(raw string) (Policy, error) {
 	return ParsePolicyBytes([]byte(raw))
 }
 
+// ParsePolicyStringPtr from a raw JSON string pointer.
+func ParsePolicyStringPtr(raw *string) (*Policy, error) {
+	if raw == nil {
+		return nil, nil
+	}
+	pol, err := ParsePolicyBytes([]byte(*raw))
+	return &pol, err
+}
+
 // ParsePolicyObject parses a policy from an object (i.e. an API struct) which
 // can be marshalled into JSON.
 func ParsePolicyObject(obj any) (Policy, error) {
diff --git a/pkg/utils/policy/types.go b/pkg/utils/policy/types.go
index 1e188a49f..943f40bdb 100644
--- a/pkg/utils/policy/types.go
+++ b/pkg/utils/policy/types.go
@@ -137,21 +137,55 @@ type ConditionMap map[string]ConditionSettings
 // ConditionSettings is a map of keys and values.
 // Depending on the type of operation, the values can strings, integers,
 // bools or lists of strings.
-type ConditionSettings map[string]any
+type ConditionSettings map[string]ConditionSettingsValue
+
+// // UnmarshalJSON unmarshals data into m.
+// func (m *ConditionSettings) UnmarshalJSON(data []byte) error {
+// 	res := map[string]ConditionSettingsValue{}
+// 	if err := json.Unmarshal(data, &res); err != nil {
+// 		return err
+// 	}
+// 	for k, v := range res {
+// 		// AWS converts bools into strings in conditions.
+// 		if b, isBool := v.(bool); isBool {
+// 			res[k] = strconv.FormatBool(b)
+// 		}
+// 	}
+// 	*m = res
+// 	return nil
+// }
+
+// ConditionSettingsValue represents a value for condition mapping.
+// It can be any kind of value but should be one of strings, integers, bools,
+// lists or slices of them.
+//
+// It contains a custom unmarshaller that is able to parse single items and
+// converts them into slices.
+type ConditionSettingsValue []any
 
 // UnmarshalJSON unmarshals data into m.
-func (m *ConditionSettings) UnmarshalJSON(data []byte) error {
-	res := map[string]any{}
-	if err := json.Unmarshal(data, &res); err != nil {
-		return err
+func (m *ConditionSettingsValue) UnmarshalJSON(data []byte) error {
+	var resSlice []any
+
+	// Try unmarshalling into an array first
+	if err := json.Unmarshal(data, &resSlice); err != nil {
+		// If that does not work, try to unmarshal a single value which may have
+		// any form.
+		var resSingle any
+		if err := json.Unmarshal(data, &resSingle); err != nil {
+			return err
+		}
+		resSlice = []any{resSingle}
 	}
-	for k, v := range res {
+
+	for k, v := range resSlice {
 		// AWS converts bools into strings in conditions.
 		if b, isBool := v.(bool); isBool {
-			res[k] = strconv.FormatBool(b)
+			resSlice[k] = strconv.FormatBool(b)
 		}
 	}
-	*m = res
+
+	*m = resSlice
 	return nil
 }
 


Test patch:
diff --git a/pkg/clients/sns/testdata/policy_a.json b/pkg/clients/sns/testdata/policy_a.json
new file mode 100644
index 000000000..08f318709
--- /dev/null
+++ b/pkg/clients/sns/testdata/policy_a.json
@@ -0,0 +1,17 @@
+{
+    "Statement": [
+        {
+            "Sid": "PublishToTopic",
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": "arn:aws:iam::*****:role/my-role"
+            },
+            "Action": [
+                "SNS:Publish",
+                "SNS:GetTopicAttributes"
+            ],
+            "Resource": "arn:aws:sns:eu-west-1:******:my-queue"
+        }
+    ],
+    "Version": "2012-10-17"
+}
diff --git a/pkg/clients/sns/testdata/policy_a2.json b/pkg/clients/sns/testdata/policy_a2.json
new file mode 100644
index 000000000..a2a560b8c
--- /dev/null
+++ b/pkg/clients/sns/testdata/policy_a2.json
@@ -0,0 +1,17 @@
+{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "PublishToTopic",
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": "arn:aws:iam::*****:role/my-role"
+            },
+            "Action": [
+                "SNS:Publish",
+                "SNS:GetTopicAttributes"
+            ],
+            "Resource": "arn:aws:sns:eu-west-1:******:my-queue"
+        }
+    ]
+}
diff --git a/pkg/clients/sns/testdata/policy_b.json b/pkg/clients/sns/testdata/policy_b.json
new file mode 100644
index 000000000..6aee2d6ec
--- /dev/null
+++ b/pkg/clients/sns/testdata/policy_b.json
@@ -0,0 +1,16 @@
+{
+    "Version": "2012-10-17",
+    "Statement": [
+        {
+            "Sid": "PublishToTopic",
+            "Effect": "Allow",
+            "Principal": {
+                "AWS": "arn:aws:iam::*****:role/my-role"
+            },
+            "Action": [
+                "SNS:Publish"
+            ],
+            "Resource": "arn:aws:sns:eu-west-1:******:my-queue-2"
+        }
+    ]
+}
diff --git a/pkg/clients/sns/topic_test.go b/pkg/clients/sns/topic_test.go
index 235aa9eb0..3f242fa8e 100644
--- a/pkg/clients/sns/topic_test.go
+++ b/pkg/clients/sns/topic_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package sns
 
 import (
+	_ "embed"
 	"strconv"
 	"testing"
 
@@ -24,11 +25,11 @@ import (
 	awssns "github.com/aws/aws-sdk-go-v2/service/sns"
 	awssnstypes "github.com/aws/aws-sdk-go-v2/service/sns/types"
 	"github.com/aws/smithy-go/document"
+	"github.com/crossplane/crossplane-runtime/pkg/test"
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 
 	"github.com/crossplane-contrib/provider-aws/apis/sns/v1beta1"
-	awsclients "github.com/crossplane-contrib/provider-aws/pkg/clients"
 )
 
 var (
@@ -47,13 +48,22 @@ var (
 	tagValue1         = "value-1"
 	tagKey2           = "name-2"
 	tagValue2         = "value-2"
+
+	//go:embed testdata/policy_a.json
+	testPolicyA string
+
+	//go:embed testdata/policy_a2.json
+	testPolicyA2 string
+
+	//go:embed testdata/policy_b.json
+	testPolicyB string
 )
 
 // Topic Attribute Modifier
-type topicAttrModifier func(*map[string]string)
+type topicAttrModifier func(map[string]string)
 
-func topicAttributes(m ...topicAttrModifier) *map[string]string {
-	attr := &map[string]string{}
+func topicAttributes(m ...topicAttrModifier) map[string]string {
+	attr := map[string]string{}
 
 	for _, f := range m {
 		f(attr)
@@ -63,41 +73,40 @@ func topicAttributes(m ...topicAttrModifier) *map[string]string {
 }
 
 func withOwner(s *string) topicAttrModifier {
-	return func(attr *map[string]string) {
-		(*attr)[string(TopicOwner)] = *s
+	return func(attr map[string]string) {
+		attr[string(TopicOwner)] = *s
 	}
 }
 
 func withARN(s *string) topicAttrModifier {
-	return func(attr *map[string]string) {
-		(*attr)[string(TopicARN)] = *s
+	return func(attr map[string]string) {
+		attr[string(TopicARN)] = *s
 	}
 }
 
 func withTopicSubs(confirmed, pending, deleted string) topicAttrModifier {
-	return func(attr *map[string]string) {
-		a := *attr
-		a[string(TopicSubscriptionsConfirmed)] = confirmed
-		a[string(TopicSubscriptionsPending)] = pending
-		a[string(TopicSubscriptionsDeleted)] = deleted
+	return func(attr map[string]string) {
+		attr[string(TopicSubscriptionsConfirmed)] = confirmed
+		attr[string(TopicSubscriptionsPending)] = pending
+		attr[string(TopicSubscriptionsDeleted)] = deleted
 	}
 }
 
 func withAttrDisplayName(s *string) topicAttrModifier {
-	return func(attr *map[string]string) {
-		(*attr)[string(TopicDisplayName)] = *s
+	return func(attr map[string]string) {
+		attr[string(TopicDisplayName)] = *s
 	}
 }
 
 func withAttrFifoTopic(b *bool) topicAttrModifier {
-	return func(attr *map[string]string) {
-		(*attr)[string(TopicFifoTopic)] = strconv.FormatBool(*b)
+	return func(attr map[string]string) {
+		attr[string(TopicFifoTopic)] = strconv.FormatBool(*b)
 	}
 }
 
 func withAttrPolicy(s *string) topicAttrModifier {
-	return func(attr *map[string]string) {
-		(*attr)[string(TopicPolicy)] = *s
+	return func(attr map[string]string) {
+		attr[string(TopicPolicy)] = *s
 	}
 }
 
@@ -198,12 +207,17 @@ func TestGenerateCreateTopicInput(t *testing.T) {
 func TestGetChangedAttributes(t *testing.T) {
 	type args struct {
 		p    v1beta1.TopicParameters
-		attr *map[string]string
+		attr map[string]string
+	}
+
+	type want struct {
+		attrs map[string]string
+		err   error
 	}
 
 	cases := map[string]struct {
 		args args
-		want *map[string]string
+		want want
 	}{
 		"NoChange": {
 			args: args{
@@ -215,7 +229,9 @@ func TestGetChangedAttributes(t *testing.T) {
 					withAttrDisplayName(&topicDisplayName),
 				),
 			},
-			want: topicAttributes(),
+			want: want{
+				attrs: topicAttributes(),
+			},
 		},
 		"Change": {
 			args: args{
@@ -227,9 +243,11 @@ func TestGetChangedAttributes(t *testing.T) {
 					withAttrDisplayName(&topicDisplayName2),
 				),
 			},
-			want: topicAttributes(
-				withAttrDisplayName(&topicDisplayName),
-			),
+			want: want{
+				attrs: topicAttributes(
+					withAttrDisplayName(&topicDisplayName),
+				),
+			},
 		},
 		"ChangeFifo": {
 			args: args{
@@ -241,25 +259,58 @@ func TestGetChangedAttributes(t *testing.T) {
 					withAttrFifoTopic(&falseFlag),
 				),
 			},
-			want: topicAttributes(
-				withAttrFifoTopic(&trueFlag),
-			),
+			want: want{
+				attrs: topicAttributes(
+					withAttrFifoTopic(&trueFlag),
+				),
+			},
+		},
+		"SamePolicyButDifferentFormat": {
+			args: args{
+				p: v1beta1.TopicParameters{
+					Policy: &testPolicyA,
+				},
+				attr: topicAttributes(
+					withAttrPolicy(&testPolicyA2),
+				),
+			},
+			want: want{
+				attrs: topicAttributes(),
+			},
+		},
+		"ChangedPolicy": {
+			args: args{
+				p: v1beta1.TopicParameters{
+					Policy: &testPolicyA,
+				},
+				attr: topicAttributes(
+					withAttrPolicy(&testPolicyB),
+				),
+			},
+			want: want{
+				attrs: topicAttributes(
+					withAttrPolicy(&testPolicyA),
+				),
+			},
 		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			c := GetChangedAttributes(tc.args.p, *tc.args.attr)
-			if diff := cmp.Diff(*tc.want, c); diff != "" {
+			attr, err := GetChangedAttributes(tc.args.p, tc.args.attr)
+			if diff := cmp.Diff(tc.want.attrs, attr); diff != "" {
 				t.Errorf("GetChangedAttributes(...): -want, +got:\n%s", diff)
 			}
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("err: -want, +got:\n%s", diff)
+			}
 		})
 	}
 }
 
 func TestGenerateTopicObservation(t *testing.T) {
 	cases := map[string]struct {
-		in  *map[string]string
+		in  map[string]string
 		out *v1beta1.TopicObservation
 	}{
 		"AllFilled": {
@@ -295,7 +346,7 @@ func TestGenerateTopicObservation(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			observation := GenerateTopicObservation(*tc.in)
+			observation := GenerateTopicObservation(tc.in)
 			if diff := cmp.Diff(*tc.out, observation); diff != "" {
 				t.Errorf("GenerateTopicObservation(...): -want, +got:\n%s", diff)
 			}
@@ -306,12 +357,16 @@ func TestGenerateTopicObservation(t *testing.T) {
 func TestIsSNSTopicUpToDate(t *testing.T) {
 	type args struct {
 		p    v1beta1.TopicParameters
-		attr *map[string]string
+		attr map[string]string
+	}
+	type want struct {
+		isUpToDate bool
+		err        error
 	}
 
 	cases := map[string]struct {
 		args args
-		want bool
+		want want
 	}{
 		"SameFieldsAndAllFilled": {
 			args: args{
@@ -324,7 +379,9 @@ func TestIsSNSTopicUpToDate(t *testing.T) {
 					FifoTopic:   &topicFifo,
 				},
 			},
-			want: true,
+			want: want{
+				isUpToDate: true,
+			},
 		},
 		"DifferentFields": {
 			args: args{
@@ -333,61 +390,46 @@ func TestIsSNSTopicUpToDate(t *testing.T) {
 				),
 				p: v1beta1.TopicParameters{},
 			},
-			want: false,
+			want: want{
+				isUpToDate: false,
+			},
+		},
+		"UpdateOnDifferentPolicy": {
+			args: args{
+				attr: topicAttributes(
+					withAttrPolicy(&testPolicyA),
+				),
+				p: v1beta1.TopicParameters{
+					Policy: &testPolicyB,
+				},
+			},
+			want: want{
+				isUpToDate: false,
+			},
 		},
 		"NoUpdateExistsWithshuffledPolicy": {
 			args: args{
 				attr: topicAttributes(
-					withAttrPolicy(awsclients.String(`{
-						"Version": "2012-10-17",
-						"Statement": [
-							{
-								"Sid": "PublishToTopic",
-								"Effect": "Allow",
-								"Principal": {
-									"AWS": "arn:aws:iam::*****:role/my-role"
-								},
-								"Action": [
-									"SNS:Publish",
-									"SNS:GetTopicAttributes"
-								],
-								"Resource": "arn:aws:sns:eu-west-1:******:my-queue"
-							}
-						]
-					}`)),
+					withAttrPolicy(&testPolicyA),
 				),
 				p: v1beta1.TopicParameters{
-					Policy: awsclients.String(`{
-						"Statement": [
-							{
-								"Sid": "PublishToTopic",
-								"Effect": "Allow",
-								"Principal": {
-									"AWS": "arn:aws:iam::*****:role/my-role"
-								},
-								"Action": [
-									"SNS:Publish",
-									"SNS:GetTopicAttributes"
-								],
-								"Resource": "arn:aws:sns:eu-west-1:******:my-queue"
-							}
-						],
-						"Version": "2012-10-17"
-					}`),
+					Policy: &testPolicyA2,
 				},
 			},
-			want: false,
+			want: want{
+				isUpToDate: true,
+			},
 		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			got, err := IsSNSTopicUpToDate(tc.args.p, *tc.args.attr)
-			if err != nil {
-				t.Error(err)
+			isUpToDate, err := IsSNSTopicUpToDate(tc.args.p, tc.args.attr)
+			if diff := cmp.Diff(tc.want.isUpToDate, isUpToDate); diff != "" {
+				t.Errorf("isUpToDate: -want, +got:\n%s", diff)
 			}
-			if diff := cmp.Diff(tc.want, got); diff != "" {
-				t.Errorf("Topic : -want, +got:\n%s", diff)
+			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
+				t.Errorf("err: -want, +got:\n%s", diff)
 			}
 		})
 	}
diff --git a/pkg/utils/policy/parse_test.go b/pkg/utils/policy/parse_test.go
index 19c6b641f..5ca598907 100644
--- a/pkg/utils/policy/parse_test.go
+++ b/pkg/utils/policy/parse_test.go
@@ -35,7 +35,7 @@ func TestParsePolicy(t *testing.T) {
 			want: want{
 				policy: &Policy{
 					Version: "2012-10-17",
-					Statements: []Statement{
+					Statements: StatementList{
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
@@ -56,10 +56,10 @@ func TestParsePolicy(t *testing.T) {
 							},
 							Condition: ConditionMap{
 								"StringEquals": ConditionSettings{
-									"aws:SourceAccount": "111111111111",
+									"aws:SourceAccount": ConditionSettingsValue{"111111111111"},
 								},
 								"ArnLike": ConditionSettings{
-									"aws:SourceArn": "arn:aws:s3:::EXAMPLE-SOURCE-BUCKET",
+									"aws:SourceArn": ConditionSettingsValue{"arn:aws:s3:::EXAMPLE-SOURCE-BUCKET"},
 								},
 							},
 						},
@@ -74,7 +74,7 @@ func TestParsePolicy(t *testing.T) {
 			want: want{
 				policy: &Policy{
 					Version: "2012-10-17",
-					Statements: []Statement{
+					Statements: StatementList{
 						{
 							SID: ptr.To("AllowPutObjectS3ServerAccessLogsPolicy"),
 							Principal: &Principal{
@@ -104,7 +104,7 @@ func TestParsePolicy(t *testing.T) {
 									},
 								},
 								"ArnLike": ConditionSettings{
-									"aws:SourceArn": "arn:aws:s3:::EXAMPLE-SOURCE-BUCKET",
+									"aws:SourceArn": ConditionSettingsValue{"arn:aws:s3:::EXAMPLE-SOURCE-BUCKET"},
 								},
 							},
 						},
@@ -122,7 +122,10 @@ func TestParsePolicy(t *testing.T) {
 							},
 							Condition: ConditionMap{
 								"ForAllValues:StringNotEquals": ConditionSettings{
-									"aws:PrincipalServiceNamesList": "logging.s3.amazonaws.com",
+									"aws:PrincipalServiceNamesList": ConditionSettingsValue{
+										"logging.s3.amazonaws.com",
+										"s3.amazonaws.com",
+									},
 								},
 							},
 						},
diff --git a/pkg/utils/policy/testdata/UnmarshalArrays.json b/pkg/utils/policy/testdata/UnmarshalArrays.json
index 5d94c6cd5..0596271dd 100644
--- a/pkg/utils/policy/testdata/UnmarshalArrays.json
+++ b/pkg/utils/policy/testdata/UnmarshalArrays.json
@@ -40,7 +40,10 @@
             "Resource": "arn:aws:s3:::DOC-EXAMPLE-BUCKET-logs\/*",
             "Condition": {
                 "ForAllValues:StringNotEquals": {
-                    "aws:PrincipalServiceNamesList": "logging.s3.amazonaws.com"
+                    "aws:PrincipalServiceNamesList": [
+                        "logging.s3.amazonaws.com",
+                        "s3.amazonaws.com"
+                    ]
                 }
             }
         }