fix(s3) compare unmarshaled policy

Signed-off-by: Tom Dott (external expert on behalf of DB Netz AG) <[email protected]> fix(s3) add command to JSONNormalize add unit test for policy fix format fix format format
crossplane-contrib · Jun 1, 2023 · 36b4db9 · 36b4db9
1 parent 37542c0
commit 36b4db9
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 3 deletions.
diff --git a/pkg/controller/s3/bucket/policy.go b/pkg/controller/s3/bucket/policy.go
@@ -27,6 +27,7 @@ import (
 	"github.com/crossplane/crossplane-runtime/pkg/meta"
 
 	"github.com/crossplane-contrib/provider-aws/apis/s3/v1beta1"
+	aws "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	awsclient "github.com/crossplane-contrib/provider-aws/pkg/clients"
 	"github.com/crossplane-contrib/provider-aws/pkg/clients/s3"
 )
@@ -73,12 +74,36 @@ func (e *PolicyClient) Observe(ctx context.Context, cr *v1beta1.Bucket) (Resourc
 	if policy == nil && resp.Policy != nil && getBucketPolicyDeletionPolicy(cr) == v1beta1.BucketPolicyDeletionPolicyIfNull {
 		return NeedsDeletion, nil
 	}
-	if cmp.Equal(policy, resp.Policy) {
+
+	if EqualsJSON(aws.StringValue(policy), aws.StringValue(resp.Policy)) {
 		return Updated, nil
 	}
+
 	return NeedsUpdate, nil
 }
 
+// JSONNormalize bring JsonStrings to an []byte
+func JSONNormalize(jStr string) *string {
+	var iface any
+	err := json.Unmarshal([]byte(jStr), &iface)
+	if err != nil {
+		return &jStr
+	}
+
+	jRaw, err := json.Marshal(iface)
+	if err != nil {
+		return &jStr
+	}
+	return aws.String(string(jRaw))
+}
+
+// EqualsJSON whether two JSON structs are equal
+func EqualsJSON(a, b string) bool {
+	pa := JSONNormalize(a)
+	pb := JSONNormalize(b)
+	return cmp.Equal(pa, pb)
+}
+
 // formatBucketPolicy parses and formats the bucket.Spec.BucketPolicy struct
 func (e *PolicyClient) formatBucketPolicy(cr *v1beta1.Bucket) (*string, error) {
 	if cr.Spec.ForProvider.Policy == nil {

diff --git a/pkg/controller/s3/bucket/policy_test.go b/pkg/controller/s3/bucket/policy_test.go
@@ -69,12 +69,13 @@ func TestPolicyObserve(t *testing.T) {
 				Principal: &common.BucketPrincipal{
 					AllowAnon: true,
 				},
-				Action:   []string{"s3:GetBucket"},
-				Resource: []string{"arn:aws:s3:::test.s3.crossplane.com"},
+				Action:   []string{"s3:GetObject"},
+				Resource: []string{"arn:aws:s3:::test.s3.crossplane.com/*"},
 			},
 		},
 	}
 
+	testPolicyRawShuffled := "{\"Statement\":[{\"Effect\":\"Allow\",\"Action\":\"s3:ListBucket\",\"Principal\":\"*\",\"Resource\":\"arn:aws:s3:::test.s3.crossplane.com\"}],\"Version\":\"2012-10-17\"}"
 	testPolicyRaw := makeRawPolicy(testPolicy)
 	testPolicyOtherRaw := makeRawPolicy(testPolicyOther)
 
@@ -198,6 +199,24 @@ func TestPolicyObserve(t *testing.T) {
 				err:    nil,
 			},
 		},
+		"NoUpdateExistsWithshuffledPolicy": {
+			args: args{
+				b: s3testing.Bucket(s3testing.WithPolicy(testPolicy)),
+				cl: NewPolicyClient(fake.MockBucketClient{
+					MockBucketPolicyClient: fake.MockBucketPolicyClient{
+						MockGetBucketPolicy: func(ctx context.Context, input *s3.GetBucketPolicyInput, opts []func(*s3.Options)) (*s3.GetBucketPolicyOutput, error) {
+							return &s3.GetBucketPolicyOutput{
+								Policy: &testPolicyRawShuffled,
+							}, nil
+						},
+					},
+				}),
+			},
+			want: want{
+				status: Updated,
+				err:    nil,
+			},
+		},
 	}
 
 	for name, tc := range cases {