From d42b14288be4d8ebcdccb0a7ccc2dddde7c28e4b Mon Sep 17 00:00:00 2001 From: arush sharma Date: Sun, 27 Apr 2025 15:17:50 -0700 Subject: [PATCH 1/3] add customprecompare for Policy and RedrivePolicy --- apis/v1alpha1/ack-generate-metadata.yaml | 8 +-- apis/v1alpha1/generator.yaml | 6 ++ generator.yaml | 6 ++ go.mod | 1 + go.sum | 2 + pkg/resource/queue/delta.go | 15 +---- pkg/resource/queue/hooks.go | 75 ++++++++++++++++++++++++ 7 files changed, 95 insertions(+), 18 deletions(-) diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 767681a..2140f98 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,13 +1,13 @@ ack_generate_info: - build_date: "2025-03-27T16:10:01Z" - build_hash: 980cb1e4734f673d16101cf55206b84ca639ec01 + build_date: "2025-04-29T03:41:47Z" + build_hash: ab8fa0bbefe77c9682f53251412fd8d1002ba30f go_version: go1.24.1 - version: v0.44.0 + version: v0.44.0-3-gab8fa0b api_directory_checksum: 2627dc306e3a83c86c04050c6c4336451459e728 api_version: v1alpha1 aws_sdk_go_version: v1.32.6 generator_config_info: - file_checksum: 662a51e8e4a1225d04aa0121d55827e0a9a054af + file_checksum: b332aeda9a33b58316273296754ee470b9568b59 original_file_name: generator.yaml last_modification: reason: API generation diff --git a/apis/v1alpha1/generator.yaml b/apis/v1alpha1/generator.yaml index 5ba4da4..212fac6 100644 --- a/apis/v1alpha1/generator.yaml +++ b/apis/v1alpha1/generator.yaml @@ -12,6 +12,8 @@ resources: values: - All hooks: + delta_pre_compare: + code: customPreCompare(delta, a, b) sdk_get_attributes_pre_set_output: template_path: hooks/queue/sdk_get_attributes_pre_set_output.go.tpl sdk_get_attributes_post_set_output: @@ -75,6 +77,8 @@ resources: service_name: iam resource: Policy path: Spec.PolicyDocument + compare: + is_ignored: true ReceiveMessageWaitTimeSeconds: is_attribute: true type: string @@ -92,6 +96,8 @@ resources: RedrivePolicy: is_attribute: true type: string + compare: + is_ignored: true RedriveAllowPolicy: is_attribute: true type: string diff --git a/generator.yaml b/generator.yaml index 5ba4da4..212fac6 100644 --- a/generator.yaml +++ b/generator.yaml @@ -12,6 +12,8 @@ resources: values: - All hooks: + delta_pre_compare: + code: customPreCompare(delta, a, b) sdk_get_attributes_pre_set_output: template_path: hooks/queue/sdk_get_attributes_pre_set_output.go.tpl sdk_get_attributes_post_set_output: @@ -75,6 +77,8 @@ resources: service_name: iam resource: Policy path: Spec.PolicyDocument + compare: + is_ignored: true ReceiveMessageWaitTimeSeconds: is_attribute: true type: string @@ -92,6 +96,8 @@ resources: RedrivePolicy: is_attribute: true type: string + compare: + is_ignored: true RedriveAllowPolicy: is_attribute: true type: string diff --git a/go.mod b/go.mod index d760fca..38c5491 100644 --- a/go.mod +++ b/go.mod @@ -13,6 +13,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/sqs v1.37.10 github.com/aws/smithy-go v1.22.2 github.com/go-logr/logr v1.4.2 + github.com/micahhausler/aws-iam-policy v0.4.2 github.com/spf13/pflag v1.0.5 k8s.io/api v0.32.1 k8s.io/apimachinery v0.32.1 diff --git a/go.sum b/go.sum index cc0ff6b..5909c40 100644 --- a/go.sum +++ b/go.sum @@ -113,6 +113,8 @@ github.com/mailru/easyjson v0.7.7 h1:UGYAvKxe3sBsEDzO8ZeWOSlIQfWFlxbzLZe7hwFURr0 github.com/mailru/easyjson v0.7.7/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc= github.com/mattn/go-isatty v0.0.14/go.mod h1:7GGIvUiUoEMVVmxf/4nioHXj79iQHKdU27kJ6hsGG94= github.com/mattn/go-runewidth v0.0.9/go.mod h1:H031xJmbD/WCDINGzjvQ9THkh0rPKHF+m2gUSrubnMI= +github.com/micahhausler/aws-iam-policy v0.4.2 h1:HF7bERLnpqEmffV9/wTT4jZ7TbSNVk0JbpXo1Cj3up0= +github.com/micahhausler/aws-iam-policy v0.4.2/go.mod h1:Ojgst9ZFn+VEEJpqtuw/LxVGqEf2+hwWBlkYWvF/XWM= github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd h1:TRLaZ9cD/w8PVh93nsPXa1VrQ6jlwL5oN8l14QlcNfg= github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q= diff --git a/pkg/resource/queue/delta.go b/pkg/resource/queue/delta.go index 7504614..085d834 100644 --- a/pkg/resource/queue/delta.go +++ b/pkg/resource/queue/delta.go @@ -42,6 +42,7 @@ func newResourceDelta( delta.Add("", a, b) return delta } + customPreCompare(delta, a, b) if ackcompare.HasNilDifference(a.ko.Spec.ContentBasedDeduplication, b.ko.Spec.ContentBasedDeduplication) { delta.Add("Spec.ContentBasedDeduplication", a.ko.Spec.ContentBasedDeduplication, b.ko.Spec.ContentBasedDeduplication) @@ -95,13 +96,6 @@ func newResourceDelta( delta.Add("Spec.MessageRetentionPeriod", a.ko.Spec.MessageRetentionPeriod, b.ko.Spec.MessageRetentionPeriod) } } - if ackcompare.HasNilDifference(a.ko.Spec.Policy, b.ko.Spec.Policy) { - delta.Add("Spec.Policy", a.ko.Spec.Policy, b.ko.Spec.Policy) - } else if a.ko.Spec.Policy != nil && b.ko.Spec.Policy != nil { - if *a.ko.Spec.Policy != *b.ko.Spec.Policy { - delta.Add("Spec.Policy", a.ko.Spec.Policy, b.ko.Spec.Policy) - } - } if !reflect.DeepEqual(a.ko.Spec.PolicyRef, b.ko.Spec.PolicyRef) { delta.Add("Spec.PolicyRef", a.ko.Spec.PolicyRef, b.ko.Spec.PolicyRef) } @@ -126,13 +120,6 @@ func newResourceDelta( delta.Add("Spec.RedriveAllowPolicy", a.ko.Spec.RedriveAllowPolicy, b.ko.Spec.RedriveAllowPolicy) } } - if ackcompare.HasNilDifference(a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) { - delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) - } else if a.ko.Spec.RedrivePolicy != nil && b.ko.Spec.RedrivePolicy != nil { - if *a.ko.Spec.RedrivePolicy != *b.ko.Spec.RedrivePolicy { - delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) - } - } if ackcompare.HasNilDifference(a.ko.Spec.SQSManagedSSEEnabled, b.ko.Spec.SQSManagedSSEEnabled) { delta.Add("Spec.SQSManagedSSEEnabled", a.ko.Spec.SQSManagedSSEEnabled, b.ko.Spec.SQSManagedSSEEnabled) } else if a.ko.Spec.SQSManagedSSEEnabled != nil && b.ko.Spec.SQSManagedSSEEnabled != nil { diff --git a/pkg/resource/queue/hooks.go b/pkg/resource/queue/hooks.go index 1d1037f..3531aaf 100644 --- a/pkg/resource/queue/hooks.go +++ b/pkg/resource/queue/hooks.go @@ -14,13 +14,18 @@ package queue import ( + "bytes" "context" + "encoding/json" "fmt" + "reflect" ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1" + ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" ackrtlog "github.com/aws-controllers-k8s/runtime/pkg/runtime/log" svcsdk "github.com/aws/aws-sdk-go-v2/service/sqs" "github.com/aws/aws-sdk-go/aws/arn" + policy "github.com/micahhausler/aws-iam-policy/policy" ) // syncTags examines the Tags in the supplied Queue and calls the @@ -143,3 +148,73 @@ func (rm *resourceManager) getQueueNameFromARN(tmpARN ackv1alpha1.AWSResourceNam } return queueARN.Resource, nil } + +// customPreCompare is the entry point for custom comparison logic +func customPreCompare( + delta *ackcompare.Delta, + a *resource, + b *resource, +) { + comparePolicy(delta, a, b) + compareRedrivePolicy(delta, a, b) +} + +// comparePolicy compares the Policy fields of two resources by unmarshalling +// them into policy.Policy structs and using reflect.DeepEqual. +func comparePolicy( + delta *ackcompare.Delta, + a *resource, + b *resource, +) { + if a.ko.Spec.Policy == b.ko.Spec.Policy { + // both are nil or equal + return + } + if a.ko.Spec.Policy == nil || b.ko.Spec.Policy == nil { + // one is nil and the other is not + delta.Add("Spec.Policy", a.ko.Spec.Policy, b.ko.Spec.Policy) + return + } + var policyA policy.Policy + decoderA := json.NewDecoder(bytes.NewBufferString(*a.ko.Spec.Policy)) + decoderA.DisallowUnknownFields() + errA := decoderA.Decode(&policyA) + + var policyB policy.Policy + decoderB := json.NewDecoder(bytes.NewBufferString(*b.ko.Spec.Policy)) + decoderB.DisallowUnknownFields() + errB := decoderB.Decode(&policyB) + + if errA != nil || errB != nil || !reflect.DeepEqual(policyA, policyB) { + delta.Add("Spec.Policy", a.ko.Spec.Policy, b.ko.Spec.Policy) + } +} + +// compareRedrivePolicy compares the RedrivePolicy fields of two resources by +// unmarshalling them into interface{} and using reflect.DeepEqual. +// since RedrivePolicy is a JSON string, we need to unmarshal it +// into an interface{} and then compare the two interface{}s. +func compareRedrivePolicy( + delta *ackcompare.Delta, + a *resource, + b *resource, +) { + if a.ko.Spec.RedrivePolicy == b.ko.Spec.RedrivePolicy { + // both are nil or equal + return + } + if a.ko.Spec.RedrivePolicy == nil || b.ko.Spec.RedrivePolicy == nil { + // one is nil and the other is not + delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) + return + } + var redriveA interface{} + errA := json.Unmarshal([]byte(*a.ko.Spec.RedrivePolicy), &redriveA) + + var redriveB interface{} + errB := json.Unmarshal([]byte(*b.ko.Spec.RedrivePolicy), &redriveB) + + if errA != nil || errB != nil || !reflect.DeepEqual(redriveA, redriveB) { + delta.Add("Spec.RedrivePolicy", a.ko.Spec.RedrivePolicy, b.ko.Spec.RedrivePolicy) + } +} From c28d9df984e2ee3ca881a035a82639c03e8e1880 Mon Sep 17 00:00:00 2001 From: arush sharma Date: Wed, 30 Apr 2025 00:54:47 -0700 Subject: [PATCH 2/3] add unit tests --- go.mod | 2 + pkg/resource/queue/hooks_test.go | 218 +++++++++++++++++++++++++++++++ 2 files changed, 220 insertions(+) create mode 100644 pkg/resource/queue/hooks_test.go diff --git a/go.mod b/go.mod index 38c5491..13c04a1 100644 --- a/go.mod +++ b/go.mod @@ -15,6 +15,7 @@ require ( github.com/go-logr/logr v1.4.2 github.com/micahhausler/aws-iam-policy v0.4.2 github.com/spf13/pflag v1.0.5 + github.com/stretchr/testify v1.9.0 k8s.io/api v0.32.1 k8s.io/apimachinery v0.32.1 k8s.io/client-go v0.32.1 @@ -62,6 +63,7 @@ require ( github.com/modern-go/reflect2 v1.0.2 // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect + github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect github.com/prometheus/client_golang v1.19.1 // indirect github.com/prometheus/client_model v0.6.1 // indirect github.com/prometheus/common v0.55.0 // indirect diff --git a/pkg/resource/queue/hooks_test.go b/pkg/resource/queue/hooks_test.go new file mode 100644 index 0000000..59a8ca5 --- /dev/null +++ b/pkg/resource/queue/hooks_test.go @@ -0,0 +1,218 @@ +package queue + +import ( + "reflect" + "testing" + + ackcompare "github.com/aws-controllers-k8s/runtime/pkg/compare" + svcapitypes "github.com/aws-controllers-k8s/sqs-controller/apis/v1alpha1" +) + +func strPtr(s string) *string { + return &s +} + +func TestComparePolicy(t *testing.T) { + basePolicyJSON := `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":"*"},"Action":"sqs:SendMessage","Resource":"arn:aws:sqs:us-east-1:123456789012:MyQueue"}]}` + equivalentPolicyJSON := `{ + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": { "AWS": "*" }, + "Action": "sqs:SendMessage", + "Resource": "arn:aws:sqs:us-east-1:123456789012:MyQueue" + } + ] + }` + differentPolicyJSON := `{"Version":"2012-10-17","Statement":[{"Effect":"Deny","Principal":{"AWS":"*"},"Action":"sqs:SendMessage","Resource":"arn:aws:sqs:us-east-1:123456789012:MyQueue"}]}` + invalidJSON := `{"Version": "2012-10-17", "Statement": [` + + tests := []struct { + name string + policyA *string + policyB *string + expectDiff bool + }{ + { + name: "both nil", + policyA: nil, + policyB: nil, + expectDiff: false, + }, + { + name: "a nil, b not nil", + policyA: nil, + policyB: strPtr(basePolicyJSON), + expectDiff: true, + }, + { + name: "both equal pointers", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(basePolicyJSON), + expectDiff: false, + }, + { + name: "semantically equivalent", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(equivalentPolicyJSON), + expectDiff: false, + }, + { + name: "different policies", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(differentPolicyJSON), + expectDiff: true, + }, + { + name: "a invalid, b valid", + policyA: strPtr(invalidJSON), + policyB: strPtr(basePolicyJSON), + expectDiff: true, + }, + { + name: "both invalid", + policyA: strPtr(invalidJSON), + policyB: strPtr(`{"another": "invalid`), + expectDiff: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + delta := ackcompare.NewDelta() + resA := &resource{ko: &svcapitypes.Queue{Spec: svcapitypes.QueueSpec{Policy: tt.policyA}}} + resB := &resource{ko: &svcapitypes.Queue{Spec: svcapitypes.QueueSpec{Policy: tt.policyB}}} + + comparePolicy(delta, resA, resB) + + diffCount := 0 + if delta.Differences != nil { + diffCount = len(delta.Differences) + } + + if tt.expectDiff { + if diffCount != 1 { + t.Fatalf("Expected exactly one difference, got %d", diffCount) + } + diff := delta.Differences[0] + if diff == nil { + t.Fatalf("Difference object should not be nil when diff expected") + } + expectedPath := ackcompare.NewPath("Spec.Policy") + if !reflect.DeepEqual(expectedPath, diff.Path) { + t.Errorf("Expected path %v, got %v", expectedPath, diff.Path) + } + if !reflect.DeepEqual(tt.policyA, diff.A) { + t.Errorf("Expected diff.A %v, got %v", tt.policyA, diff.A) + } + if !reflect.DeepEqual(tt.policyB, diff.B) { + t.Errorf("Expected diff.B %v, got %v", tt.policyB, diff.B) + } + } else { + if diffCount != 0 { + t.Errorf("Expected no differences, got %d", diffCount) + } + } + }) + } +} + +func TestCompareRedrivePolicy(t *testing.T) { + basePolicyJSON := `{"deadLetterTargetArn":"arn:aws:sqs:us-east-1:123456789012:dlq","maxReceiveCount":10}` + equivalentPolicyJSON := `{ + "deadLetterTargetArn": "arn:aws:sqs:us-east-1:123456789012:dlq", + "maxReceiveCount": 10 + }` + differentPolicyJSON := `{"deadLetterTargetArn":"arn:aws:sqs:us-east-1:123456789012:other-dlq","maxReceiveCount":5}` + invalidJSON := `{"deadLetterTargetArn":` + + tests := []struct { + name string + policyA *string + policyB *string + expectDiff bool + }{ + { + name: "both nil", + policyA: nil, + policyB: nil, + expectDiff: false, + }, + { + name: "a nil, b not nil", + policyA: nil, + policyB: strPtr(basePolicyJSON), + expectDiff: true, + }, + { + name: "both equal pointers", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(basePolicyJSON), + expectDiff: false, + }, + { + name: "semantically equivalent", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(equivalentPolicyJSON), + expectDiff: false, + }, + { + name: "different policies", + policyA: strPtr(basePolicyJSON), + policyB: strPtr(differentPolicyJSON), + expectDiff: true, + }, + { + name: "a invalid, b valid", + policyA: strPtr(invalidJSON), + policyB: strPtr(basePolicyJSON), + expectDiff: true, + }, + { + name: "both invalid", + policyA: strPtr(invalidJSON), + policyB: strPtr(`{"another": "invalid`), + expectDiff: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + delta := ackcompare.NewDelta() + resA := &resource{ko: &svcapitypes.Queue{Spec: svcapitypes.QueueSpec{RedrivePolicy: tt.policyA}}} + resB := &resource{ko: &svcapitypes.Queue{Spec: svcapitypes.QueueSpec{RedrivePolicy: tt.policyB}}} + + compareRedrivePolicy(delta, resA, resB) + + diffCount := 0 + if delta.Differences != nil { + diffCount = len(delta.Differences) + } + + if tt.expectDiff { + if diffCount != 1 { + t.Fatalf("Expected exactly one difference, got %d", diffCount) + } + diff := delta.Differences[0] + if diff == nil { + t.Fatalf("Difference object should not be nil when diff expected") + } + expectedPath := ackcompare.NewPath("Spec.RedrivePolicy") + if !reflect.DeepEqual(expectedPath, diff.Path) { + t.Errorf("Expected path %v, got %v", expectedPath, diff.Path) + } + if !reflect.DeepEqual(tt.policyA, diff.A) { + t.Errorf("Expected diff.A %v, got %v", tt.policyA, diff.A) + } + if !reflect.DeepEqual(tt.policyB, diff.B) { + t.Errorf("Expected diff.B %v, got %v", tt.policyB, diff.B) + } + } else { + if diffCount != 0 { + t.Errorf("Expected no differences, got %d", diffCount) + } + } + }) + } +} \ No newline at end of file From 52ef97d9a7141679088ef406e9e712b07c05a627 Mon Sep 17 00:00:00 2001 From: arush sharma Date: Wed, 30 Apr 2025 11:24:15 -0700 Subject: [PATCH 3/3] address comments --- pkg/resource/queue/hooks_test.go | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/pkg/resource/queue/hooks_test.go b/pkg/resource/queue/hooks_test.go index 59a8ca5..b43cd35 100644 --- a/pkg/resource/queue/hooks_test.go +++ b/pkg/resource/queue/hooks_test.go @@ -1,3 +1,16 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file is distributed +// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either +// express or implied. See the License for the specific language governing +// permissions and limitations under the License. + package queue import ( @@ -86,10 +99,7 @@ func TestComparePolicy(t *testing.T) { comparePolicy(delta, resA, resB) - diffCount := 0 - if delta.Differences != nil { - diffCount = len(delta.Differences) - } + diffCount := len(delta.Differences) if tt.expectDiff { if diffCount != 1 { @@ -184,11 +194,8 @@ func TestCompareRedrivePolicy(t *testing.T) { resB := &resource{ko: &svcapitypes.Queue{Spec: svcapitypes.QueueSpec{RedrivePolicy: tt.policyB}}} compareRedrivePolicy(delta, resA, resB) - - diffCount := 0 - if delta.Differences != nil { - diffCount = len(delta.Differences) - } + + diffCount := len(delta.Differences) if tt.expectDiff { if diffCount != 1 {