diff --git a/controllers/route_controller.go b/controllers/route_controller.go index b38bb7f0..e9e9a7d6 100644 --- a/controllers/route_controller.go +++ b/controllers/route_controller.go @@ -49,6 +49,8 @@ import ( "github.com/aws/aws-application-networking-k8s/pkg/k8s" "github.com/aws/aws-application-networking-k8s/pkg/model/core" lattice_runtime "github.com/aws/aws-application-networking-k8s/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" + "sigs.k8s.io/controller-runtime/pkg/predicate" ) var routeTypeToFinalizer = map[core.RouteType]string{ @@ -109,7 +111,7 @@ func RegisterAllRouteControllers( svcImportEventHandler := eventhandlers.NewServiceImportEventHandler(log, mgrClient) builder := ctrl.NewControllerManagedBy(mgr). - For(routeInfo.gatewayApiType). + For(routeInfo.gatewayApiType, builder.WithPredicates(predicate.GenerationChangedPredicate{})). Watches(&source.Kind{Type: &gwv1beta1.Gateway{}}, gwEventHandler). Watches(&source.Kind{Type: &corev1.Service{}}, svcEventHandler.MapToRoute(routeInfo.routeType)). Watches(&source.Kind{Type: &mcsv1alpha1.ServiceImport{}}, svcImportEventHandler.MapToRoute(routeInfo.routeType)). @@ -337,6 +339,21 @@ func (r *routeReconciler) reconcileUpsert(ctx context.Context, req ctrl.Request, } if _, err := r.buildAndDeployModel(ctx, route); err != nil { + if services.IsConflictError(err) { + // Stop reconciliation of this route if the route cannot be owned / has conflict + route.Status().UpdateParentRefs(route.Spec().ParentRefs()[0], config.LatticeGatewayControllerName) + route.Status().UpdateRouteCondition(metav1.Condition{ + Type: string(gwv1beta1.RouteConditionAccepted), + Status: metav1.ConditionFalse, + ObservedGeneration: route.K8sObject().GetGeneration(), + Reason: "Conflicted", + Message: err.Error(), + }) + if err = r.client.Status().Update(ctx, route.K8sObject()); err != nil { + return fmt.Errorf("failed to update route status for conflict due to err %w", err) + } + return nil + } return err } diff --git a/controllers/route_controller_test.go b/controllers/route_controller_test.go index 29e7aad3..cfe1809a 100644 --- a/controllers/route_controller_test.go +++ b/controllers/route_controller_test.go @@ -166,6 +166,7 @@ func TestRouteReconciler_ReconcileCreates(t *testing.T) { ClusterName: config.ClusterName, }).AnyTimes() mockCloud.EXPECT().DefaultTags().Return(mocks.Tags{}).AnyTimes() + mockCloud.EXPECT().DefaultTagsMergedWith(gomock.Any()).Return(mocks.Tags{}).AnyTimes() // we expect a fair number of lattice calls mockLattice.EXPECT().FindServiceNetwork(ctx, gomock.Any(), gomock.Any()).Return( diff --git a/pkg/aws/cloud.go b/pkg/aws/cloud.go index 20185ddb..78d6d7de 100644 --- a/pkg/aws/cloud.go +++ b/pkg/aws/cloud.go @@ -8,6 +8,7 @@ import ( "github.com/aws/aws-sdk-go/service/vpclattice" "golang.org/x/exp/maps" + "context" "github.com/aws/aws-application-networking-k8s/pkg/aws/services" "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" ) @@ -36,11 +37,14 @@ type Cloud interface { // creates lattice tags with default values populated and merges them with provided tags DefaultTagsMergedWith(services.Tags) services.Tags - // check if tags map has managedBy tag + // Retrieve tags and check if tags map has managedBy tag ContainsManagedBy(tags services.Tags) bool // check if managedBy tag set for lattice resource - IsArnManaged(arn string) (bool, error) + IsArnManaged(ctx context.Context, arn string) (bool, error) + + // check ownership and acquire if it is not owned by anyone. + CheckAndAcquireOwnershipFromTags(ctx context.Context, arn string, tags services.Tags) (bool, error) } // NewCloud constructs new Cloud implementation. @@ -107,22 +111,58 @@ func (c *defaultCloud) DefaultTagsMergedWith(tags services.Tags) services.Tags { return newTags } -func (c *defaultCloud) ContainsManagedBy(tags services.Tags) bool { +func (c *defaultCloud) getManagedByFromTags(tags services.Tags) string { tag, ok := tags[TagManagedBy] if !ok || tag == nil { - return false + return "" } - return *tag == c.managedByTag + return *tag } -func (c *defaultCloud) IsArnManaged(arn string) (bool, error) { +func (c *defaultCloud) getManagedBy(ctx context.Context, arn string) (string, error) { tagsReq := &vpclattice.ListTagsForResourceInput{ResourceArn: &arn} - resp, err := c.lattice.ListTagsForResource(tagsReq) + resp, err := c.lattice.ListTagsForResourceWithContext(ctx, tagsReq) + if err != nil { + return "", err + } + return c.getManagedByFromTags(resp.Tags), nil +} + +func (c *defaultCloud) ContainsManagedBy(tags services.Tags) bool { + return c.isOwner(c.getManagedByFromTags(tags)) +} + +func (c *defaultCloud) IsArnManaged(ctx context.Context, arn string) (bool, error) { + managedBy, err := c.getManagedBy(ctx, arn) if err != nil { return false, nil } - isManaged := c.ContainsManagedBy(resp.Tags) - return isManaged, nil + return c.isOwner(managedBy), nil +} + +func (c *defaultCloud) CheckAndAcquireOwnershipFromTags(ctx context.Context, arn string, tags services.Tags) (bool, error) { + // For resources that need backwards compatibility - not having managedBy is considered as owned by controller. + managedBy := c.getManagedByFromTags(tags) + if managedBy == "" { + err := c.acquireOwnership(ctx, arn) + if err != nil { + return false, err + } + return true, nil + } + return c.isOwner(managedBy), nil +} + +func (c *defaultCloud) acquireOwnership(ctx context.Context, arn string) error { + _, err := c.Lattice().TagResourceWithContext(ctx, &vpclattice.TagResourceInput{ + ResourceArn: &arn, + Tags: c.DefaultTags(), + }) + return err +} + +func (c *defaultCloud) isOwner(managedBy string) bool { + return managedBy == c.managedByTag } func getManagedByTag(cfg CloudConfig) string { diff --git a/pkg/aws/cloud_mocks.go b/pkg/aws/cloud_mocks.go index 0d8c2f29..0dccd2ad 100644 --- a/pkg/aws/cloud_mocks.go +++ b/pkg/aws/cloud_mocks.go @@ -5,6 +5,7 @@ package aws import ( + context "context" reflect "reflect" services "github.com/aws/aws-application-networking-k8s/pkg/aws/services" @@ -34,6 +35,21 @@ func (m *MockCloud) EXPECT() *MockCloudMockRecorder { return m.recorder } +// CheckAndAcquireOwnershipFromTags mocks base method. +func (m *MockCloud) CheckAndAcquireOwnershipFromTags(arg0 context.Context, arg1 string, arg2 map[string]*string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CheckAndAcquireOwnershipFromTags", arg0, arg1, arg2) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CheckAndAcquireOwnershipFromTags indicates an expected call of CheckAndAcquireOwnershipFromTags. +func (mr *MockCloudMockRecorder) CheckAndAcquireOwnershipFromTags(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CheckAndAcquireOwnershipFromTags", reflect.TypeOf((*MockCloud)(nil).CheckAndAcquireOwnershipFromTags), arg0, arg1, arg2) +} + // Config mocks base method. func (m *MockCloud) Config() CloudConfig { m.ctrl.T.Helper() @@ -91,18 +107,18 @@ func (mr *MockCloudMockRecorder) DefaultTagsMergedWith(arg0 interface{}) *gomock } // IsArnManaged mocks base method. -func (m *MockCloud) IsArnManaged(arg0 string) (bool, error) { +func (m *MockCloud) IsArnManaged(arg0 context.Context, arg1 string) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsArnManaged", arg0) + ret := m.ctrl.Call(m, "IsArnManaged", arg0, arg1) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } // IsArnManaged indicates an expected call of IsArnManaged. -func (mr *MockCloudMockRecorder) IsArnManaged(arg0 interface{}) *gomock.Call { +func (mr *MockCloudMockRecorder) IsArnManaged(arg0, arg1 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsArnManaged", reflect.TypeOf((*MockCloud)(nil).IsArnManaged), arg0) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsArnManaged", reflect.TypeOf((*MockCloud)(nil).IsArnManaged), arg0, arg1) } // Lattice mocks base method. diff --git a/pkg/aws/cloud_test.go b/pkg/aws/cloud_test.go index 41e2cfbd..028fc0dd 100644 --- a/pkg/aws/cloud_test.go +++ b/pkg/aws/cloud_test.go @@ -9,6 +9,8 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" + "context" + "fmt" "github.com/aws/aws-application-networking-k8s/pkg/aws/services" ) @@ -46,38 +48,38 @@ func TestIsArnManaged(t *testing.T) { t.Run("arn sent", func(t *testing.T) { arn := "arn" - mockLattice.EXPECT().ListTagsForResource(gomock.Any()). + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). DoAndReturn( - func(req *vpclattice.ListTagsForResourceInput) (*vpclattice.ListTagsForResourceOutput, error) { + func(_ context.Context, req *vpclattice.ListTagsForResourceInput, _ ...interface{}) (*vpclattice.ListTagsForResourceOutput, error) { assert.Equal(t, arn, *req.ResourceArn) return &vpclattice.ListTagsForResourceOutput{}, nil }) - cl.IsArnManaged(arn) + cl.IsArnManaged(context.Background(), arn) }) t.Run("is managed", func(t *testing.T) { arn := "arn" - mockLattice.EXPECT().ListTagsForResource(gomock.Any()). + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). Return(&vpclattice.ListTagsForResourceOutput{ Tags: cl.DefaultTags(), }, nil) - managed, err := cl.IsArnManaged(arn) + managed, err := cl.IsArnManaged(context.Background(), arn) assert.Nil(t, err) assert.True(t, managed) }) t.Run("not managed", func(t *testing.T) { - mockLattice.EXPECT().ListTagsForResource(gomock.Any()). + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). Return(&vpclattice.ListTagsForResourceOutput{}, nil) - managed, err := cl.IsArnManaged("arn") + managed, err := cl.IsArnManaged(context.Background(), "arn") assert.Nil(t, err) assert.False(t, managed) }) t.Run("error", func(t *testing.T) { - mockLattice.EXPECT().ListTagsForResource(gomock.Any()). + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). Return(nil, errors.New(":(")) - managed, err := cl.IsArnManaged("arn") + managed, err := cl.IsArnManaged(context.Background(), "arn") assert.Nil(t, err) assert.False(t, managed) }) @@ -129,3 +131,66 @@ func Test_DefaultTagsMergedWith(t *testing.T) { assert.Equal(t, expected, actual) }) } + +func Test_CheckAndAcquireOwnershipFromTags(t *testing.T) { + c := gomock.NewController(t) + defer c.Finish() + + mockLattice := services.NewMockLattice(c) + cfg := CloudConfig{VpcId: "vpc-id", AccountId: "account-id", ClusterName: "cluster"} + cloud := NewDefaultCloud(mockLattice, cfg) + + tcs := []struct { + name string + tags services.Tags + owned bool + tryAcquire bool + isErr bool + }{ + { + name: "no ownership tag acquires ownership", + tags: services.Tags{}, + owned: true, + tryAcquire: true, + isErr: false, + }, + { + name: "proper ownership tag considered valid", + tags: cloud.DefaultTags(), + owned: true, + tryAcquire: false, + isErr: false, + }, + { + name: "improper ownership tag considered invalid", + tags: services.Tags{ + TagManagedBy: aws.String("not/this/owner"), + }, + owned: false, + tryAcquire: false, + isErr: false, + }, + } + + for i, tc := range tcs { + t.Run(tc.name, func(t *testing.T) { + arn := fmt.Sprintf("arn-%d", i) + + tagResourceCallCount := 0 + if tc.tryAcquire { + tagResourceCallCount = 1 + } + mockLattice.EXPECT().TagResourceWithContext(gomock.Any(), &vpclattice.TagResourceInput{ResourceArn: aws.String(arn), Tags: cloud.DefaultTags()}). + Return(&vpclattice.TagResourceOutput{}, nil).Times(tagResourceCallCount) + + res, err := cloud.CheckAndAcquireOwnershipFromTags(context.Background(), arn, tc.tags) + + assert.Equal(t, tc.owned, res) + if tc.isErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/deploy/externaldns/dnsendpoint_manager.go b/pkg/deploy/externaldns/dnsendpoint_manager.go index 3004f5e5..3a148af9 100644 --- a/pkg/deploy/externaldns/dnsendpoint_manager.go +++ b/pkg/deploy/externaldns/dnsendpoint_manager.go @@ -35,8 +35,8 @@ func NewDnsEndpointManager(log gwlog.Logger, k8sClient client.Client) *defaultDn func (s *defaultDnsEndpointManager) Create(ctx context.Context, service *latticemodel.Service) error { namespacedName := types.NamespacedName{ - Namespace: service.Spec.Namespace, - Name: service.Spec.Name + "-dns", + Namespace: service.Spec.RouteNamespace, + Name: service.Spec.RouteName + "-dns", } if service.Spec.CustomerDomainName == "" { s.log.Debugf("Skipping creation of %s: detected no custom domain", namespacedName) @@ -52,8 +52,8 @@ func (s *defaultDnsEndpointManager) Create(ctx context.Context, service *lattice err error ) routeNamespacedName := types.NamespacedName{ - Namespace: service.Spec.Namespace, - Name: service.Spec.Name, + Namespace: service.Spec.RouteNamespace, + Name: service.Spec.RouteName, } if service.Spec.RouteType == core.GrpcRouteType { route, err = core.GetGRPCRoute(ctx, s.k8sClient, routeNamespacedName) diff --git a/pkg/deploy/externaldns/dnsendpoint_manager_test.go b/pkg/deploy/externaldns/dnsendpoint_manager_test.go index 6fcd2372..72527103 100644 --- a/pkg/deploy/externaldns/dnsendpoint_manager_test.go +++ b/pkg/deploy/externaldns/dnsendpoint_manager_test.go @@ -39,8 +39,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "No customer domain name - skips creation", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "", }, Status: &model.ServiceStatus{ @@ -53,8 +55,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "No service dns - skips creation", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -67,8 +71,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "No parent route - skips creation", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -82,8 +88,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Create new DNSEndpoint if not existing already", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -98,8 +106,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Return error on creation failure", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -115,8 +125,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Update DNSEndpoint if existing already", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -142,8 +154,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "DNSEndpoint existing already, but skip if it is the same", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -169,8 +183,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Return error on update failure", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -197,8 +213,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Skips creation when DNSEndpoint CRD is not found", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -215,8 +233,10 @@ func TestCreateDnsEndpoint(t *testing.T) { name: "Return error on unexpected lookup failure", service: model.Service{ Spec: model.ServiceSpec{ - Name: "service", - Namespace: "default", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service", + RouteNamespace: "default", + }, CustomerDomainName: "custom-domain", }, Status: &model.ServiceStatus{ @@ -236,13 +256,13 @@ func TestCreateDnsEndpoint(t *testing.T) { mockClient.EXPECT().Scheme().Return(runtime.NewScheme()).AnyTimes() mockClient.EXPECT().Get(gomock.Any(), gomock.Eq(types.NamespacedName{ - Namespace: tt.service.Spec.Namespace, - Name: tt.service.Spec.Name, + Namespace: tt.service.Spec.RouteNamespace, + Name: tt.service.Spec.RouteName, }), gomock.Any()).Return(tt.routeGetErr).AnyTimes() mockClient.EXPECT().Get(gomock.Any(), gomock.Eq(types.NamespacedName{ - Namespace: tt.service.Spec.Namespace, - Name: tt.service.Spec.Name + "-dns", + Namespace: tt.service.Spec.RouteNamespace, + Name: tt.service.Spec.RouteName + "-dns", }), gomock.Any()).DoAndReturn(func(ctx context.Context, name types.NamespacedName, ep *endpoint.DNSEndpoint, _ ...interface{}) error { tt.existingEndpoint.DeepCopyInto(ep) return tt.dnsGetErr diff --git a/pkg/deploy/lattice/service_manager.go b/pkg/deploy/lattice/service_manager.go index 838ce550..97719ae2 100644 --- a/pkg/deploy/lattice/service_manager.go +++ b/pkg/deploy/lattice/service_manager.go @@ -98,7 +98,7 @@ func (m *defaultServiceManager) newCreateSvcReq(svc *Service) *CreateSvcReq { svcName := svc.LatticeServiceName() req := &vpclattice.CreateServiceInput{ Name: &svcName, - Tags: m.cloud.DefaultTags(), + Tags: m.cloud.DefaultTagsMergedWith(svc.Spec.ToTags()), } if svc.Spec.CustomerDomainName != "" { @@ -124,6 +124,43 @@ func svcStatusFromCreateSvcResp(resp *CreateSvcResp) ServiceInfo { return svcInfo } +func (m *defaultServiceManager) checkAndUpdateTags(ctx context.Context, svc *Service, svcSum *SvcSummary) error { + tagsResp, err := m.cloud.Lattice().ListTagsForResourceWithContext(ctx, &vpclattice.ListTagsForResourceInput{ + ResourceArn: svcSum.Arn, + }) + if err != nil { + return err + } + + owned, err := m.cloud.CheckAndAcquireOwnershipFromTags(ctx, *svcSum.Arn, tagsResp.Tags) + if err != nil { + return err + } + if !owned { + return services.NewConflictError("service", svc.Spec.RouteNamespace+"/"+svc.Spec.RouteName, + fmt.Sprintf("Found existing resource not owned by controller: %s", *svcSum.Arn)) + } + + tagFields := model.ServiceTagFieldsFromTags(tagsResp.Tags) + switch { + case tagFields.RouteName == "" && tagFields.RouteNamespace == "": + // backwards compatibility: If the service has no identification tags, consider this controller has + // correct information and add tags + _, err = m.cloud.Lattice().TagResourceWithContext(ctx, &vpclattice.TagResourceInput{ + ResourceArn: svcSum.Arn, + Tags: svc.Spec.ToTags(), + }) + return err + case tagFields != svc.Spec.ServiceTagFields: + // Considering these scenarios: + // - two services with same namespace-name but different routeType + // - two services with conflict edge case such as my-namespace/service & my/namespace-service + return services.NewConflictError("service", svc.Spec.RouteName+"/"+svc.Spec.RouteNamespace, + fmt.Sprintf("Found existing resource with conflicting service name: %s", *svcSum.Arn)) + } + return nil +} + func (m *defaultServiceManager) updateServiceAndAssociations(ctx context.Context, svc *Service, svcSum *SvcSummary) (ServiceInfo, error) { if svc.Spec.CustomerCertARN != "" { updReq := &UpdateSvcReq{ @@ -184,7 +221,7 @@ func (m *defaultServiceManager) updateAssociations(ctx context.Context, svc *Ser } for _, assoc := range toDelete { - isManaged, err := m.cloud.IsArnManaged(*assoc.Arn) + isManaged, err := m.cloud.IsArnManaged(ctx, *assoc.Arn) if err != nil { return err } @@ -301,6 +338,10 @@ func (m *defaultServiceManager) Upsert(ctx context.Context, svc *Service) (Servi if svcSum == nil { svcInfo, err = m.createServiceAndAssociate(ctx, svc) } else { + err = m.checkAndUpdateTags(ctx, svc, svcSum) + if err != nil { + return ServiceInfo{}, err + } svcInfo, err = m.updateServiceAndAssociations(ctx, svc, svcSum) } if err != nil { @@ -319,6 +360,12 @@ func (m *defaultServiceManager) Delete(ctx context.Context, svc *Service) error } } + err = m.checkAndUpdateTags(ctx, svc, svcSum) + if err != nil { + m.log.Infof("Service %s is either invalid or not owned. Skipping VPC Lattice resource deletion.", svc.LatticeServiceName()) + return nil + } + err = m.deleteAllAssociations(ctx, svcSum) if err != nil { return err diff --git a/pkg/deploy/lattice/service_manager_test.go b/pkg/deploy/lattice/service_manager_test.go index 167c0ccd..1a9fd7e5 100644 --- a/pkg/deploy/lattice/service_manager_test.go +++ b/pkg/deploy/lattice/service_manager_test.go @@ -5,6 +5,7 @@ import ( "errors" 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" model "github.com/aws/aws-application-networking-k8s/pkg/model/lattice" "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" "github.com/aws/aws-sdk-go/aws" @@ -30,8 +31,11 @@ func TestServiceManagerInteg(t *testing.T) { svc := &Service{ Spec: model.ServiceSpec{ - Name: "svc", - Namespace: "ns", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "svc", + RouteNamespace: "ns", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{"sn"}, CustomerDomainName: "dns", CustomerCertARN: "cert-arn", @@ -106,8 +110,11 @@ func TestServiceManagerInteg(t *testing.T) { svc := &Service{ Spec: model.ServiceSpec{ - Name: "svc", - Namespace: "ns", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "svc", + RouteNamespace: "ns", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{snKeep, snAdd}, }, } @@ -122,6 +129,14 @@ func TestServiceManagerInteg(t *testing.T) { }, nil). Times(1) + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req *vpclattice.ListTagsForResourceInput, _ ...interface{}) (*vpclattice.ListTagsForResourceOutput, error) { + return &vpclattice.ListTagsForResourceOutput{ + Tags: cl.DefaultTagsMergedWith(svc.Spec.ToTags()), + }, nil + }). + Times(1) // for service only + // 3 associations exist in lattice: keep, delete, and foreign mockLattice.EXPECT(). ListServiceNetworkServiceAssociationsAsList(gomock.Any(), gomock.Any()). @@ -140,8 +155,8 @@ func TestServiceManagerInteg(t *testing.T) { Times(1) // return managed by gateway controller tags for all associations except for foreign - mockLattice.EXPECT().ListTagsForResource(gomock.Any()). - DoAndReturn(func(req *vpclattice.ListTagsForResourceInput) (*vpclattice.ListTagsForResourceOutput, error) { + 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 { @@ -193,11 +208,67 @@ func TestServiceManagerInteg(t *testing.T) { assert.Equal(t, "svc-arn", status.Arn) }) + t.Run("backfilling service tags", func(t *testing.T) { + svc := &Service{ + Spec: model.ServiceSpec{ + ServiceTagFields: model.ServiceTagFields{ + RouteName: "svc", + RouteNamespace: "ns", + RouteType: core.HttpRouteType, + }, + ServiceNetworkNames: []string{"sn"}, + }, + } + + // service exists in lattice + mockLattice.EXPECT(). + FindService(gomock.Any(), gomock.Any()). + Return(&vpclattice.ServiceSummary{ + Arn: aws.String("svc-arn"), + Id: aws.String("svc-id"), + Name: aws.String(svc.LatticeServiceName()), + }, nil). + Times(1) + + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req *vpclattice.ListTagsForResourceInput, _ ...interface{}) (*vpclattice.ListTagsForResourceOutput, error) { + return &vpclattice.ListTagsForResourceOutput{ + Tags: map[string]*string{}, + }, nil + }). + Times(1) + + mockLattice.EXPECT().TagResourceWithContext(gomock.Any(), gomock.Eq(&vpclattice.TagResourceInput{ + ResourceArn: aws.String("svc-arn"), + Tags: cl.DefaultTags(), + })).Times(1) + + mockLattice.EXPECT().TagResourceWithContext(gomock.Any(), gomock.Eq(&vpclattice.TagResourceInput{ + ResourceArn: aws.String("svc-arn"), + Tags: svc.Spec.ToTags(), + })).Times(1) + + mockLattice.EXPECT().ListServiceNetworkServiceAssociationsAsList(gomock.Any(), gomock.Any()).Times(1) + mockLattice.EXPECT(). + CreateServiceNetworkServiceAssociationWithContext(gomock.Any(), gomock.Any()). + DoAndReturn( + func(_ context.Context, req *CreateSnSvcAssocReq, _ ...interface{}) (*CreateSnSvcAssocResp, error) { + return &CreateSnSvcAssocResp{Status: aws.String(vpclattice.ServiceNetworkServiceAssociationStatusActive)}, nil + }). + Times(1) + + status, err := m.Upsert(ctx, svc) + assert.Nil(t, err) + assert.Equal(t, "svc-arn", status.Arn) + }) + t.Run("delete service and association", func(t *testing.T) { svc := &Service{ Spec: model.ServiceSpec{ - Name: "svc", - Namespace: "ns", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "svc", + RouteNamespace: "ns", + }, ServiceNetworkNames: []string{"sn"}, }, } @@ -210,6 +281,15 @@ func TestServiceManagerInteg(t *testing.T) { Id: aws.String("svc-id"), Name: aws.String(svc.LatticeServiceName()), }, nil) + + mockLattice.EXPECT().ListTagsForResourceWithContext(gomock.Any(), gomock.Any()). + DoAndReturn(func(_ context.Context, req *vpclattice.ListTagsForResourceInput, _ ...interface{}) (*vpclattice.ListTagsForResourceOutput, error) { + return &vpclattice.ListTagsForResourceOutput{ + Tags: cl.DefaultTagsMergedWith(svc.Spec.ToTags()), + }, nil + }). + Times(1) // for service only + mockLattice.EXPECT(). ListServiceNetworkServiceAssociationsAsList(gomock.Any(), gomock.Any()). Return([]*SnSvcAssocSummary{ @@ -243,8 +323,11 @@ func TestCreateSvcReq(t *testing.T) { m := NewServiceManager(gwlog.FallbackLogger, cl) spec := model.ServiceSpec{ - Name: "name", - Namespace: "ns", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "svc", + RouteNamespace: "ns", + RouteType: core.HttpRouteType, + }, CustomerDomainName: "dns", CustomerCertARN: "cert-arn", } @@ -255,6 +338,7 @@ func TestCreateSvcReq(t *testing.T) { req := m.newCreateSvcReq(svcModel) + assert.Equal(t, req.Tags, cl.DefaultTagsMergedWith(spec.ToTags())) assert.Equal(t, *req.Name, svcModel.LatticeServiceName()) assert.Equal(t, *req.CustomDomainName, spec.CustomerDomainName) assert.Equal(t, *req.CertificateArn, spec.CustomerCertARN) diff --git a/pkg/deploy/lattice/service_synthesizer.go b/pkg/deploy/lattice/service_synthesizer.go index 1ef6514b..c0698467 100644 --- a/pkg/deploy/lattice/service_synthesizer.go +++ b/pkg/deploy/lattice/service_synthesizer.go @@ -38,19 +38,20 @@ func (s *serviceSynthesizer) Synthesize(ctx context.Context) error { var svcErr error for _, resService := range resServices { - s.log.Debugf("Synthesizing service: %s-%s", resService.Spec.Name, resService.Spec.Namespace) + svcName := fmt.Sprintf("%s-%s", resService.Spec.RouteName, resService.Spec.RouteNamespace) + s.log.Debugf("Synthesizing service: %s", svcName) if resService.IsDeleted { err := s.serviceManager.Delete(ctx, resService) if err != nil { svcErr = errors.Join(svcErr, - fmt.Errorf("failed ServiceManager.Delete %s-%s due to %s", resService.Spec.Name, resService.Spec.Namespace, err)) + fmt.Errorf("failed ServiceManager.Delete %s due to %w", svcName, err)) continue } } else { serviceStatus, err := s.serviceManager.Upsert(ctx, resService) if err != nil { svcErr = errors.Join(svcErr, - fmt.Errorf("failed ServiceManager.Upsert %s-%s due to %s", resService.Spec.Name, resService.Spec.Namespace, err)) + fmt.Errorf("failed ServiceManager.Upsert %s due to %w", svcName, err)) continue } @@ -58,7 +59,7 @@ func (s *serviceSynthesizer) Synthesize(ctx context.Context) error { err = s.dnsEndpointManager.Create(ctx, resService) if err != nil { svcErr = errors.Join(svcErr, - fmt.Errorf("failed DnsEndpointManager.Create %s-%s due to %s", resService.Spec.Name, resService.Spec.Namespace, err)) + fmt.Errorf("failed DnsEndpointManager.Create %s due to %w", svcName, err)) svcErr = err continue diff --git a/pkg/deploy/lattice/service_synthesizer_test.go b/pkg/deploy/lattice/service_synthesizer_test.go index 2caf4bb6..360909bb 100644 --- a/pkg/deploy/lattice/service_synthesizer_test.go +++ b/pkg/deploy/lattice/service_synthesizer_test.go @@ -160,8 +160,10 @@ func Test_SynthesizeService(t *testing.T) { mockDnsManager := externaldns.NewMockDnsEndpointManager(c) spec := model.ServiceSpec{ - Name: tt.httpRoute.Name, - Namespace: tt.httpRoute.Namespace, + ServiceTagFields: model.ServiceTagFields{ + RouteName: tt.httpRoute.Name, + RouteNamespace: tt.httpRoute.Namespace, + }, } latticeService, err := model.NewLatticeService(stack, spec) diff --git a/pkg/deploy/stack_deployer.go b/pkg/deploy/stack_deployer.go index be07b0d4..5bef4f5e 100644 --- a/pkg/deploy/stack_deployer.go +++ b/pkg/deploy/stack_deployer.go @@ -107,38 +107,38 @@ func (d *latticeServiceStackDeployer) Deploy(ctx context.Context, stack core.Sta //Handle targetGroups creation request if err := targetGroupSynthesizer.SynthesizeCreate(ctx); err != nil { - return fmt.Errorf("error during tg synthesis %s", err) + return fmt.Errorf("error during tg synthesis %w", err) } //Handle targets "reconciliation" request (register intend-to-be-registered targets and deregister intend-to-be-registered targets) if err := targetsSynthesizer.Synthesize(ctx); err != nil { - return fmt.Errorf("error during target synthesis %s", err) + return fmt.Errorf("error during target synthesis %w", err) } // Handle latticeService "reconciliation" request if err := serviceSynthesizer.Synthesize(ctx); err != nil { - return fmt.Errorf("error during service synthesis %s", err) + return fmt.Errorf("error during service synthesis %w", err) } //Handle latticeService listeners "reconciliation" request if err := listenerSynthesizer.Synthesize(ctx); err != nil { - return fmt.Errorf("error during listener synthesis %s", err) + return fmt.Errorf("error during listener synthesis %w", err) } //Handle latticeService listener's rules "reconciliation" request if err := ruleSynthesizer.Synthesize(ctx); err != nil { - return fmt.Errorf("error during rule synthesis %s", err) + return fmt.Errorf("error during rule synthesis %w", err) } //Handle targetGroup deletion request if err := targetGroupSynthesizer.SynthesizeDelete(ctx); err != nil { - return fmt.Errorf("error during tg delete synthesis %s", err) + return fmt.Errorf("error during tg delete synthesis %w", err) } // Do garbage collection for not-in-use targetGroups //TODO: run SynthesizeSDKTargetGroups(ctx) as a global garbage collector scheduled background task (i.e., run it as a goroutine in main.go) if err := targetGroupSynthesizer.SynthesizeUnusedDelete(ctx); err != nil { - return fmt.Errorf("error during tg unused delete synthesis %s", err) + return fmt.Errorf("error during tg unused delete synthesis %w", err) } return nil diff --git a/pkg/gateway/model_build_lattice_service.go b/pkg/gateway/model_build_lattice_service.go index 769344f4..448decb4 100644 --- a/pkg/gateway/model_build_lattice_service.go +++ b/pkg/gateway/model_build_lattice_service.go @@ -100,9 +100,11 @@ func (t *latticeServiceModelBuildTask) buildLatticeService(ctx context.Context) } spec := model.ServiceSpec{ - Name: t.route.Name(), - Namespace: t.route.Namespace(), - RouteType: routeType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: t.route.Name(), + RouteNamespace: t.route.Namespace(), + RouteType: routeType, + }, } for _, parentRef := range t.route.Spec().ParentRefs() { diff --git a/pkg/gateway/model_build_lattice_service_test.go b/pkg/gateway/model_build_lattice_service_test.go index 56d387e7..efd33639 100644 --- a/pkg/gateway/model_build_lattice_service_test.go +++ b/pkg/gateway/model_build_lattice_service_test.go @@ -90,10 +90,12 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "test", + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "test", + RouteType: core.HttpRouteType, + }, CustomerDomainName: "test1.test.com", - RouteType: core.HttpRouteType, ServiceNetworkNames: []string{"gateway1"}, }, }, @@ -124,9 +126,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "default", - RouteType: core.HttpRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "default", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{"gateway1"}, }, }, @@ -156,9 +160,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "test", - RouteType: core.GrpcRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "test", + RouteType: core.GrpcRouteType, + }, ServiceNetworkNames: []string{"gateway1"}, }, }, @@ -212,9 +218,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service2", - Namespace: "ns1", - RouteType: core.HttpRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service2", + RouteNamespace: "ns1", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{"gateway2"}, }, }, @@ -262,9 +270,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "default", - RouteType: core.HttpRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "default", + RouteType: core.HttpRouteType, + }, CustomerCertARN: "cert-arn", ServiceNetworkNames: []string{"gateway1"}, }, @@ -336,9 +346,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "default", - RouteType: core.HttpRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "default", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{"gateway1"}, }, }, @@ -373,9 +385,11 @@ func Test_LatticeServiceModelBuild(t *testing.T) { }, }), expected: model.ServiceSpec{ - Name: "service1", - Namespace: "default", - RouteType: core.HttpRouteType, + ServiceTagFields: model.ServiceTagFields{ + RouteName: "service1", + RouteNamespace: "default", + RouteType: core.HttpRouteType, + }, ServiceNetworkNames: []string{"gateway1", "gateway2"}, }, }, @@ -411,8 +425,8 @@ func Test_LatticeServiceModelBuild(t *testing.T) { assert.Equal(t, tt.wantIsDeleted, svc.IsDeleted) - assert.Equal(t, tt.expected.Name, svc.Spec.Name) - assert.Equal(t, tt.expected.Namespace, svc.Spec.Namespace) + assert.Equal(t, tt.expected.RouteName, svc.Spec.RouteName) + assert.Equal(t, tt.expected.RouteNamespace, svc.Spec.RouteNamespace) assert.Equal(t, tt.expected.CustomerCertARN, svc.Spec.CustomerCertARN) assert.Equal(t, tt.expected.CustomerDomainName, svc.Spec.CustomerDomainName) assert.Equal(t, tt.expected.RouteType, svc.Spec.RouteType) diff --git a/pkg/model/lattice/service.go b/pkg/model/lattice/service.go index 3a95c7a9..75a47f6a 100644 --- a/pkg/model/lattice/service.go +++ b/pkg/model/lattice/service.go @@ -1,6 +1,7 @@ package lattice import ( + "github.com/aws/aws-application-networking-k8s/pkg/aws/services" "github.com/aws/aws-application-networking-k8s/pkg/model/core" "github.com/aws/aws-application-networking-k8s/pkg/utils" ) @@ -13,12 +14,10 @@ type Service struct { } type ServiceSpec struct { - Name string `json:"name"` - Namespace string `json:"namespace"` - RouteType core.RouteType `json:"routetype"` - ServiceNetworkNames []string `json:"servicenetworkhnames"` - CustomerDomainName string `json:"customerdomainname"` - CustomerCertARN string `json:"customercertarn"` + ServiceTagFields + ServiceNetworkNames []string `json:"servicenetworkhnames"` + CustomerDomainName string `json:"customerdomainname"` + CustomerCertARN string `json:"customercertarn"` } type ServiceStatus struct { @@ -27,6 +26,29 @@ type ServiceStatus struct { Dns string `json:"dns"` } +type ServiceTagFields struct { + RouteName string + RouteNamespace string + RouteType core.RouteType +} + +func ServiceTagFieldsFromTags(tags map[string]*string) ServiceTagFields { + return ServiceTagFields{ + RouteName: getMapValue(tags, K8SRouteNameKey), + RouteNamespace: getMapValue(tags, K8SRouteNamespaceKey), + RouteType: core.RouteType(getMapValue(tags, K8SRouteTypeKey)), + } +} + +func (t *ServiceTagFields) ToTags() services.Tags { + rt := string(t.RouteType) + return services.Tags{ + K8SRouteNameKey: &t.RouteName, + K8SRouteNamespaceKey: &t.RouteNamespace, + K8SRouteTypeKey: &rt, + } +} + func NewLatticeService(stack core.Stack, spec ServiceSpec) (*Service, error) { id := spec.LatticeServiceName() @@ -49,5 +71,5 @@ func (s *Service) LatticeServiceName() string { } func (s *ServiceSpec) LatticeServiceName() string { - return utils.LatticeServiceName(s.Name, s.Namespace) + return utils.LatticeServiceName(s.RouteName, s.RouteNamespace) } diff --git a/pkg/model/lattice/targetgroup.go b/pkg/model/lattice/targetgroup.go index bab8e484..365a26f9 100644 --- a/pkg/model/lattice/targetgroup.go +++ b/pkg/model/lattice/targetgroup.go @@ -19,6 +19,9 @@ const ( K8SRouteNamespaceKey = aws.TagBase + "RouteNamespace" K8SSourceTypeKey = aws.TagBase + "SourceTypeKey" + // Service specific tags + K8SRouteTypeKey = aws.TagBase + "RouteType" + MaxNamespaceLength = 55 MaxNameLength = 55 RandomSuffixLength = 10