From edb39d19277e820b93419a1f50730827e461e023 Mon Sep 17 00:00:00 2001 From: Shulin Jia Date: Wed, 14 Feb 2024 11:00:59 -0800 Subject: [PATCH 1/3] In service_manager, if tags of an association cannot be fetched, skip instead of error --- pkg/deploy/lattice/service_manager.go | 13 ++++++------- pkg/deploy/lattice/service_synthesizer.go | 4 +++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/deploy/lattice/service_manager.go b/pkg/deploy/lattice/service_manager.go index 013df611..3cf1c911 100644 --- a/pkg/deploy/lattice/service_manager.go +++ b/pkg/deploy/lattice/service_manager.go @@ -204,29 +204,28 @@ func (m *defaultServiceManager) getAllAssociations(ctx context.Context, svcSum * func (m *defaultServiceManager) updateAssociations(ctx context.Context, svc *Service, svcSum *SvcSummary) error { assocs, err := m.getAllAssociations(ctx, svcSum) if err != nil { - return err + return fmt.Errorf("in updateAssociations, getAllAssociations failed with %w", err) } toCreate, toDelete, err := associationsDiff(svc, assocs) if err != nil { - return err + return fmt.Errorf("in updateAssociations, associationsDiff failed with %w", err) } for _, snName := range toCreate { err := m.createAssociation(ctx, svcSum.Id, snName) if err != nil { - return err + return fmt.Errorf("in updateAssociations, createAssociations failed with %w", err) } } for _, assoc := range toDelete { isManaged, err := m.cloud.IsArnManaged(ctx, *assoc.Arn) if err != nil { - return err - } - if isManaged { + m.log.Errorf("in updateAssociations failed when attempting IsArnManaged check. Skipping delete association. service: %s, association: %s, %s", svc.LatticeServiceName(), assoc.Arn, err) + } else if isManaged { err = m.deleteAssociation(ctx, assoc.Arn) if err != nil { - return err + return fmt.Errorf("in updateAssociations, deleteAssociation failed with %w", err) } } } diff --git a/pkg/deploy/lattice/service_synthesizer.go b/pkg/deploy/lattice/service_synthesizer.go index 5a025d78..1adefc8b 100644 --- a/pkg/deploy/lattice/service_synthesizer.go +++ b/pkg/deploy/lattice/service_synthesizer.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" + "github.com/aws/aws-application-networking-k8s/pkg/utils" + "github.com/aws/aws-application-networking-k8s/pkg/deploy/externaldns" "github.com/aws/aws-application-networking-k8s/pkg/model/core" model "github.com/aws/aws-application-networking-k8s/pkg/model/lattice" @@ -38,7 +40,7 @@ func (s *serviceSynthesizer) Synthesize(ctx context.Context) error { var svcErr error for _, resService := range resServices { - svcName := fmt.Sprintf("%s-%s", resService.Spec.RouteName, resService.Spec.RouteNamespace) + svcName := utils.LatticeServiceName(resService.Spec.RouteName, resService.Spec.RouteNamespace) s.log.Debugf("Synthesizing service: %s", svcName) if resService.IsDeleted { err := s.serviceManager.Delete(ctx, resService) From e8e79e26a85e71bb48acaedb0ab5b1bd1c161374 Mon Sep 17 00:00:00 2001 From: Shulin Jia Date: Sun, 25 Feb 2024 17:48:23 -0800 Subject: [PATCH 2/3] address comments; fix log, add todo about checking error code --- pkg/deploy/lattice/service_manager.go | 23 ++++++++++++++----- pkg/deploy/lattice/service_network_manager.go | 13 ++++++++++- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/pkg/deploy/lattice/service_manager.go b/pkg/deploy/lattice/service_manager.go index 3cf1c911..52fc409d 100644 --- a/pkg/deploy/lattice/service_manager.go +++ b/pkg/deploy/lattice/service_manager.go @@ -204,28 +204,39 @@ func (m *defaultServiceManager) getAllAssociations(ctx context.Context, svcSum * func (m *defaultServiceManager) updateAssociations(ctx context.Context, svc *Service, svcSum *SvcSummary) error { assocs, err := m.getAllAssociations(ctx, svcSum) if err != nil { - return fmt.Errorf("in updateAssociations, getAllAssociations failed with %w", err) + return err } toCreate, toDelete, err := associationsDiff(svc, assocs) if err != nil { - return fmt.Errorf("in updateAssociations, associationsDiff failed with %w", err) + return err } for _, snName := range toCreate { err := m.createAssociation(ctx, svcSum.Id, snName) if err != nil { - return fmt.Errorf("in updateAssociations, createAssociations failed with %w", err) + return err } } for _, assoc := range toDelete { isManaged, err := m.cloud.IsArnManaged(ctx, *assoc.Arn) if err != nil { - m.log.Errorf("in updateAssociations failed when attempting IsArnManaged check. Skipping delete association. service: %s, association: %s, %s", svc.LatticeServiceName(), assoc.Arn, err) - } else if isManaged { + // TODO check for vpclattice.ErrCodeAccessDeniedException or a new error type ErrorCodeNotFoundException + // when the api no longer responds with a 404 NotFoundException instead of either of the above. + // ErrorCodeNotFoundException currently not part of the golang sdk for the lattice api. This a is a distinct + // error from vpclattice.ErrCodeResourceNotFoundException. + + // In a scenario that the service association is created by a foreign account, + // the owner account's controller cannot read the tags of this ServiceNetworkServiceAssociation, + // and AccessDeniedException is expected. + m.log.Warnf("skipping update associations service: %s, association: %s, error: %s", svc.LatticeServiceName(), *assoc.Arn, err) + + continue + } + if isManaged { err = m.deleteAssociation(ctx, assoc.Arn) if err != nil { - return fmt.Errorf("in updateAssociations, deleteAssociation failed with %w", err) + return err } } } diff --git a/pkg/deploy/lattice/service_network_manager.go b/pkg/deploy/lattice/service_network_manager.go index 23d3ed76..31976c33 100644 --- a/pkg/deploy/lattice/service_network_manager.go +++ b/pkg/deploy/lattice/service_network_manager.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "golang.org/x/exp/slices" "github.com/aws/aws-application-networking-k8s/pkg/aws/services" @@ -100,7 +101,17 @@ func (m *defaultServiceNetworkManager) DeleteVpcAssociation(ctx context.Context, owned, err := m.cloud.IsArnManaged(ctx, *snva.Arn) if err != nil { - return err + // TODO check for vpclattice.ErrCodeAccessDeniedException or a new error type ErrorCodeNotFoundException + // when the api no longer responds with a 404 NotFoundException instead of either of the above. + // ErrorCodeNotFoundException currently not part of the golang sdk for the lattice api. This a is a distinct + // error from vpclattice.ErrCodeResourceNotFoundException. + + // In a scenario that the vpc association is created by a foreign account, + // the owner account's controller cannot read the tags of this ServiceNetworkVpcAssociation, + // and AccessDeniedException is expected. + m.log.Warnf("skipping delete vpc association, association: %s, error: %s", *snva.Arn, err) + + return nil } if !owned { m.log.Infof("Association %s for %s not owned by controller, skipping deletion", *snva.Arn, snName) From 63ee7edc17c71d4e5c385d5e8bb836e3b8531305 Mon Sep 17 00:00:00 2001 From: Shulin Jia Date: Sun, 25 Feb 2024 18:13:18 -0800 Subject: [PATCH 3/3] add unit test --- pkg/deploy/lattice/service_manager_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/deploy/lattice/service_manager_test.go b/pkg/deploy/lattice/service_manager_test.go index bc017459..867c0610 100644 --- a/pkg/deploy/lattice/service_manager_test.go +++ b/pkg/deploy/lattice/service_manager_test.go @@ -3,6 +3,8 @@ package lattice import ( "context" "errors" + "testing" + pkg_aws "github.com/aws/aws-application-networking-k8s/pkg/aws" mocks "github.com/aws/aws-application-networking-k8s/pkg/aws/services" "github.com/aws/aws-application-networking-k8s/pkg/model/core" @@ -12,7 +14,6 @@ import ( "github.com/aws/aws-sdk-go/service/vpclattice" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" - "testing" ) func TestServiceManagerInteg(t *testing.T) { @@ -102,11 +103,13 @@ func TestServiceManagerInteg(t *testing.T) { // sn-delete - managed by gateway and want to delete // sn-create - managed by gateway and want to create // sn-foreign - not managed by gateway and exists in lattice (should keep) + // sn-foreign-ram - managed in a different account. management check will be skipped (should keep) t.Run("update service's associations", func(t *testing.T) { snKeep := "sn-keep" snDelete := "sn-delete" snAdd := "sn-add" snForeign := "sn-foreign" + snForeignRAM := "sn-foreign-ram" svc := &Service{ Spec: model.ServiceSpec{ @@ -142,7 +145,7 @@ func TestServiceManagerInteg(t *testing.T) { ListServiceNetworkServiceAssociationsAsList(gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, req *ListSnSvcAssocsReq) ([]*SnSvcAssocSummary, error) { assocs := []*SnSvcAssocSummary{} - for _, sn := range []string{snKeep, snDelete, snForeign} { + for _, sn := range []string{snKeep, snDelete, snForeign, snForeignRAM, snForeignRAM} { assocs = append(assocs, &SnSvcAssocSummary{ Arn: aws.String(sn + "-arn"), Id: aws.String(sn + "-id"), @@ -154,18 +157,20 @@ func TestServiceManagerInteg(t *testing.T) { }). Times(1) - // return managed by gateway controller tags for all associations except for foreign + // return managed by gateway controller tags for all associations except for foreign and foreign ram mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). DoAndReturn(func(_ context.Context, req *vpclattice.ListTagsForResourceInput, _ ...interface{}) (*vpclattice.ListTagsForResourceOutput, error) { if *req.ResourceArn == snForeign+"-arn" { return &vpclattice.ListTagsForResourceOutput{}, nil + } else if *req.ResourceArn == snForeignRAM+"-arn" { + return nil, errors.New("some_error") } else { return &vpclattice.ListTagsForResourceOutput{ Tags: cl.DefaultTags(), }, nil } }). - Times(2) // delete and foreign + Times(3) // delete and foreign and foreign ram // assert we call create and delete associations on managed resources mockLattice.EXPECT().