Skip to content

In service_manager, if tags of an association cannot be fetched, skip instead of error #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 27, 2024
Merged

In service_manager, if tags of an association cannot be fetched, skip instead of error #600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
edb39d1
In service_manager, if tags of an association cannot be fetched, skip…
shulin-sq Feb 14, 2024
e8e79e2
address comments; fix log, add todo about checking error code
shulin-sq Feb 26, 2024
63ee7ed
add unit test
shulin-sq Feb 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion pkg/deploy/lattice/service_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,17 @@ func (m *defaultServiceManager) updateAssociations(ctx context.Context, svc *Ser
for _, assoc := range toDelete {
isManaged, err := m.cloud.IsArnManaged(ctx, *assoc.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 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)
Copy link
Contributor

@zijun726911 zijun726911 Feb 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we mimic this logic to only ignore get tags AccessDenied and still return other err? for example:

	for _, assoc := range toDelete {
		isManaged, err := m.cloud.IsArnManaged(ctx, *assoc.Arn)
		if err != nil {
			aerr, ok := err.(awserr.Error)
			if ok && aerr.Code() == vpclattice.ErrCodeAccessDeniedException {
				// 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.
				continue
			} else {
				return err
			}
		}
		if isManaged {
			err = m.deleteAssociation(ctx, assoc.Arn)
			if err != nil {
				return err
			}
		}
	}

@solmonk can you review this PR?

Copy link
Contributor

@solmonk solmonk Feb 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think this way makes more sense in terms of consistency.

Expand Down
13 changes: 9 additions & 4 deletions pkg/deploy/lattice/service_manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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) {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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"),
Expand All @@ -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().
Expand Down
13 changes: 12 additions & 1 deletion pkg/deploy/lattice/service_network_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"

"golang.org/x/exp/slices"

"github.com/aws/aws-application-networking-k8s/pkg/aws/services"
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 3 additions & 1 deletion pkg/deploy/lattice/service_synthesizer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Copy link
Contributor Author

@shulin-sq shulin-sq Feb 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor fix on error messaging. was confusing to see a non truncated service name here.

s.log.Debugf("Synthesizing service: %s", svcName)
if resService.IsDeleted {
err := s.serviceManager.Delete(ctx, resService)
Expand Down