From c37cd3b9e8947da9abddc902e00cf30a7756384c Mon Sep 17 00:00:00 2001 From: Amine Date: Thu, 20 Jun 2024 01:44:58 -0700 Subject: [PATCH] fix(networkacl): Correct update order and entries/associations handlers (#202) The custom update logic for `NetworkACL` resources was syncing the `Entries` before the `Tags`. This could lead to issues if the `Entries` sync failed as the Tags would not be updated.. This change also updates the order to sync `Tags` before `Entries`. It also uses `DeepCopy` when passing the resource to `createAssociation` and `createEntries to avoid modifying the original desired state. Additionally, the exit functions in `hooks.go` were fixed to properly handle the error return value. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. --- apis/v1alpha1/ack-generate-metadata.yaml | 2 +- pkg/resource/network_acl/hooks.go | 17 +++++++++-------- pkg/resource/network_acl/sdk.go | 6 ++++-- .../sdk_create_post_set_output.go.tpl | 6 ++++-- 4 files changed, 18 insertions(+), 13 deletions(-) diff --git a/apis/v1alpha1/ack-generate-metadata.yaml b/apis/v1alpha1/ack-generate-metadata.yaml index 042b9100..1135700e 100755 --- a/apis/v1alpha1/ack-generate-metadata.yaml +++ b/apis/v1alpha1/ack-generate-metadata.yaml @@ -1,5 +1,5 @@ ack_generate_info: - build_date: "2024-06-19T06:55:08Z" + build_date: "2024-06-19T08:11:53Z" build_hash: 14cef51778d471698018b6c38b604181a6948248 go_version: go1.22.4 version: v0.34.0 diff --git a/pkg/resource/network_acl/hooks.go b/pkg/resource/network_acl/hooks.go index cd9aaa3b..47809f4b 100644 --- a/pkg/resource/network_acl/hooks.go +++ b/pkg/resource/network_acl/hooks.go @@ -39,7 +39,7 @@ func (rm *resourceManager) customUpdateNetworkAcl( ) (updated *resource, err error) { rlog := ackrtlog.FromContext(ctx) exit := rlog.Trace("rm.customUpdateNetworkAcl") - defer exit(err) + defer func(err error) { exit(err) }(err) // Default `updated` to `desired` because it is likely // EC2 `modify` APIs do NOT return output, only errors. @@ -48,12 +48,6 @@ func (rm *resourceManager) customUpdateNetworkAcl( // (now updated.Spec) reflects the latest resource state. updated = rm.concreteResource(desired.DeepCopy()) - if delta.DifferentAt("Spec.Entries") { - if err := rm.syncEntries(ctx, desired, latest); err != nil { - return nil, err - } - } - if delta.DifferentAt("Spec.Tags") { if err := tags.Sync( ctx, rm.sdkapi, rm.metrics, *latest.ko.Status.ID, @@ -63,6 +57,12 @@ func (rm *resourceManager) customUpdateNetworkAcl( } } + if delta.DifferentAt("Spec.Entries") { + if err := rm.syncEntries(ctx, desired, latest); err != nil { + return nil, err + } + } + if delta.DifferentAt("Spec.Associations") { if err := rm.syncAssociation(ctx, desired, latest); err != nil { return nil, err @@ -297,7 +297,8 @@ func (rm *resourceManager) syncEntries( ) (err error) { rlog := ackrtlog.FromContext(ctx) exit := rlog.Trace("rm.syncEntries") - defer exit(err) + defer func(err error) { exit(err) }(err) + toAdd := []*svcapitypes.NetworkACLEntry{} toDelete := []*svcapitypes.NetworkACLEntry{} toUpdate := []*svcapitypes.NetworkACLEntry{} diff --git a/pkg/resource/network_acl/sdk.go b/pkg/resource/network_acl/sdk.go index fd04a067..7f8d9dfb 100644 --- a/pkg/resource/network_acl/sdk.go +++ b/pkg/resource/network_acl/sdk.go @@ -363,7 +363,8 @@ func (rm *resourceManager) sdkCreate( if len(desired.ko.Spec.Associations) > 0 { ko.Spec.Associations = desired.ko.Spec.Associations - if err := rm.createAssociation(ctx, &resource{ko}); err != nil { + copy := ko.DeepCopy() + if err := rm.createAssociation(ctx, &resource{copy}); err != nil { rlog.Debug("Error while syncing Association", err) } } @@ -371,7 +372,8 @@ func (rm *resourceManager) sdkCreate( if len(desired.ko.Spec.Entries) > 0 { //desired rules are overwritten by NetworkACL's default rules ko.Spec.Entries = append(ko.Spec.Entries, desired.ko.Spec.Entries...) - if err := rm.createEntries(ctx, &resource{ko}); err != nil { + copy := ko.DeepCopy() + if err := rm.createEntries(ctx, &resource{copy}); err != nil { rlog.Debug("Error while syncing routes", err) } } diff --git a/templates/hooks/network_acl/sdk_create_post_set_output.go.tpl b/templates/hooks/network_acl/sdk_create_post_set_output.go.tpl index 5cdc5caf..b2429be1 100644 --- a/templates/hooks/network_acl/sdk_create_post_set_output.go.tpl +++ b/templates/hooks/network_acl/sdk_create_post_set_output.go.tpl @@ -4,7 +4,8 @@ if len(desired.ko.Spec.Associations) > 0 { ko.Spec.Associations = desired.ko.Spec.Associations - if err := rm.createAssociation(ctx, &resource{ko}); err != nil { + copy := ko.DeepCopy() + if err := rm.createAssociation(ctx, &resource{copy}); err != nil { rlog.Debug("Error while syncing Association", err) } } @@ -12,7 +13,8 @@ if len(desired.ko.Spec.Entries) > 0 { //desired rules are overwritten by NetworkACL's default rules ko.Spec.Entries = append(ko.Spec.Entries, desired.ko.Spec.Entries...) - if err := rm.createEntries(ctx, &resource{ko}); err != nil { + copy := ko.DeepCopy() + if err := rm.createEntries(ctx, &resource{copy}); err != nil { rlog.Debug("Error while syncing routes", err) } }