From fd65255239ba6a671f52c5c242f398a81dc1a5b0 Mon Sep 17 00:00:00 2001 From: Xabi Date: Mon, 26 Jun 2023 13:11:52 +0200 Subject: [PATCH] fix(topic): Improve topic policy json diff check Signed-off-by: Xabi --- pkg/clients/sns/topic.go | 35 ++++++++++++++-- pkg/clients/sns/topic_test.go | 57 +++++++++++++++++++++++++- pkg/controller/sns/topic/controller.go | 9 ++-- 3 files changed, 93 insertions(+), 8 deletions(-) diff --git a/pkg/clients/sns/topic.go b/pkg/clients/sns/topic.go index 82f29285dc..4e4683129c 100644 --- a/pkg/clients/sns/topic.go +++ b/pkg/clients/sns/topic.go @@ -23,6 +23,7 @@ import ( "github.com/crossplane-contrib/provider-aws/apis/sns/v1beta1" awsclients "github.com/crossplane-contrib/provider-aws/pkg/clients" + policyutils "github.com/crossplane-contrib/provider-aws/pkg/utils/policy" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/sns" @@ -152,18 +153,46 @@ func GenerateTopicObservation(attr map[string]string) v1beta1.TopicObservation { } // IsSNSTopicUpToDate checks if object is up to date -func IsSNSTopicUpToDate(p v1beta1.TopicParameters, attr map[string]string) bool { +func IsSNSTopicUpToDate(p v1beta1.TopicParameters, attr map[string]string) (bool, error) { fifoTopic, _ := strconv.ParseBool(attr[string(TopicFifoTopic)]) + policyChanged, err := IsSNSPolicyChanged(p, attr) + if err != nil { + return false, err + } + return aws.ToString(p.DeliveryPolicy) == attr[string(TopicDeliveryPolicy)] && aws.ToString(p.DisplayName) == attr[string(TopicDisplayName)] && aws.ToString(p.KMSMasterKeyID) == attr[string(TopicKmsMasterKeyID)] && aws.ToBool(p.FifoTopic) == fifoTopic && - aws.ToString(p.Policy) == attr[string(TopicPolicy)] + !policyChanged, nil } -func getTopicAttributes(p v1beta1.TopicParameters) map[string]string { +// 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 { + return false, nil + } + + currPolicy, err := policyutils.ParsePolicyString(attr[string(TopicPolicy)]) + if err != nil { + return false, err + } + specPolicy, err := policyutils.ParsePolicyString(awsclients.StringValue(p.Policy)) + if err != nil { + return false, err + } + + equalPolicies, _ := policyutils.ArePoliciesEqal(&currPolicy, &specPolicy) + + return equalPolicies, nil +} + +func getTopicAttributes(p v1beta1.TopicParameters) map[string]string { topicAttr := make(map[string]string) topicAttr[string(TopicDeliveryPolicy)] = aws.ToString(p.DeliveryPolicy) diff --git a/pkg/clients/sns/topic_test.go b/pkg/clients/sns/topic_test.go index deccfb37e2..1d0eef9da6 100644 --- a/pkg/clients/sns/topic_test.go +++ b/pkg/clients/sns/topic_test.go @@ -29,6 +29,8 @@ import ( awssns "github.com/aws/aws-sdk-go-v2/service/sns" awssnstypes "github.com/aws/aws-sdk-go-v2/service/sns/types" "github.com/google/go-cmp/cmp" + + awsclients "github.com/crossplane-contrib/provider-aws/pkg/clients" ) var ( @@ -95,6 +97,12 @@ func withAttrFifoTopic(b *bool) topicAttrModifier { } } +func withAttrPolicy(s *string) topicAttrModifier { + return func(attr *map[string]string) { + (*attr)[string(TopicPolicy)] = *s + } +} + // topic Observation Modifier type topicObservationModifier func(*v1beta1.TopicObservation) @@ -190,7 +198,6 @@ func TestGenerateCreateTopicInput(t *testing.T) { } func TestGetChangedAttributes(t *testing.T) { - type args struct { p v1beta1.TopicParameters attr *map[string]string @@ -330,11 +337,57 @@ func TestIsSNSTopicUpToDate(t *testing.T) { }, want: 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" + } + ] + }`)), + ), + 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" + }`), + }, + }, + want: false, + }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { - got := IsSNSTopicUpToDate(tc.args.p, *tc.args.attr) + got, err := IsSNSTopicUpToDate(tc.args.p, *tc.args.attr) + if err != nil { + t.Error(err) + } if diff := cmp.Diff(tc.want, got); diff != "" { t.Errorf("Topic : -want, +got:\n%s", diff) } diff --git a/pkg/controller/sns/topic/controller.go b/pkg/controller/sns/topic/controller.go index e5c1c444f1..75fd09e763 100644 --- a/pkg/controller/sns/topic/controller.go +++ b/pkg/controller/sns/topic/controller.go @@ -125,15 +125,19 @@ func (e *external) Observe(ctx context.Context, mgd resource.Managed) (managed.E // GenerateObservation for SNS Topic cr.Status.AtProvider = snsclient.GenerateTopicObservation(res.Attributes) + upToDate, err := snsclient.IsSNSTopicUpToDate(cr.Spec.ForProvider, res.Attributes) + if err != nil { + return managed.ExternalObservation{}, err + } + return managed.ExternalObservation{ ResourceExists: true, - ResourceUpToDate: snsclient.IsSNSTopicUpToDate(cr.Spec.ForProvider, res.Attributes), + ResourceUpToDate: upToDate, ResourceLateInitialized: !reflect.DeepEqual(current, &cr.Spec.ForProvider), }, nil } func (e *external) Create(ctx context.Context, mgd resource.Managed) (managed.ExternalCreation, error) { - cr, ok := mgd.(*v1beta1.Topic) if !ok { return managed.ExternalCreation{}, errors.New(errUnexpectedObject) @@ -170,7 +174,6 @@ func (e *external) Update(ctx context.Context, mgd resource.Managed) (managed.Ex AttributeValue: aws.String(v), TopicArn: aws.String(meta.GetExternalName(cr)), }) - } return managed.ExternalUpdate{}, awsclient.Wrap(err, errUpdate) }