From 1049581a8032e8c39ee60699f5b0159b82e6c883 Mon Sep 17 00:00:00 2001 From: Zijun Wang Date: Tue, 5 Sep 2023 19:23:47 -0700 Subject: [PATCH 1/2] Revive (r *serviceReconciler) reconcileTargetsResource() to handle k8sService deletion --- controllers/service_controller.go | 54 ++++++++++-- .../integration/deregister_targets_test.go | 84 ++++++++++--------- 2 files changed, 95 insertions(+), 43 deletions(-) diff --git a/controllers/service_controller.go b/controllers/service_controller.go index dd8597bc..fffe4694 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -18,17 +18,22 @@ package controllers import ( "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/aws/aws-application-networking-k8s/pkg/aws" "github.com/aws/aws-application-networking-k8s/pkg/deploy" "github.com/aws/aws-application-networking-k8s/pkg/gateway" "github.com/aws/aws-application-networking-k8s/pkg/k8s" "github.com/aws/aws-application-networking-k8s/pkg/latticestore" + "github.com/aws/aws-application-networking-k8s/pkg/model/core" + latticemodel "github.com/aws/aws-application-networking-k8s/pkg/model/lattice" "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" - corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/tools/record" - ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -103,8 +108,47 @@ func (r *serviceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct return ctrl.Result{}, client.IgnoreNotFound(err) } if !svc.DeletionTimestamp.IsZero() { + tgName := latticestore.TargetGroupName(svc.Name, svc.Namespace) + tgs := r.datastore.GetTargetGroupsByName(tgName) + for _, tg := range tgs { + r.log.Debugf("deletion request for tgName: %s: at timestamp: %s", tg.TargetGroupKey.Name, svc.DeletionTimestamp) + r.reconcileTargetsResource(ctx, svc, tg.TargetGroupKey.RouteName) + } r.finalizerManager.RemoveFinalizers(ctx, svc, serviceFinalizer) + } else { + if err := r.finalizerManager.AddFinalizers(ctx, svc, serviceFinalizer); err != nil { + r.eventRecorder.Event(svc, corev1.EventTypeWarning, k8s.ServiceEventReasonFailedAddFinalizer, fmt.Sprintf("failed and finalizer: %s", err)) + } } + r.log.Infow("reconciled", "name", req.Name) return ctrl.Result{}, nil } + +func (r *serviceReconciler) reconcileTargetsResource(ctx context.Context, svc *corev1.Service, routename string) { + if err := r.finalizerManager.AddFinalizers(ctx, svc, serviceFinalizer); err != nil { + r.eventRecorder.Event(svc, corev1.EventTypeWarning, k8s.ServiceEventReasonFailedAddFinalizer, fmt.Sprintf("failed and finalizer: %s", err)) + } + r.buildAndDeployModel(ctx, svc, routename) +} + +func (r *serviceReconciler) buildAndDeployModel(ctx context.Context, svc *corev1.Service, routename string) (core.Stack, *latticemodel.Targets, error) { + stack, latticeTargets, err := r.modelBuilder.Build(ctx, svc, routename) + if err != nil { + r.eventRecorder.Event(svc, corev1.EventTypeWarning, + k8s.ServiceEventReasonFailedBuildModel, fmt.Sprintf("failed build model: %s", err)) + return nil, nil, err + } + + jsonStack, _ := r.stackMashaller.Marshal(stack) + r.log.Debugw("successfully built model", "stack", jsonStack) + + if err := r.stackDeployer.Deploy(ctx, stack); err != nil { + r.eventRecorder.Event(svc, corev1.EventTypeWarning, + k8s.ServiceEventReasonFailedDeployModel, fmt.Sprintf("failed deploy model: %s", err)) + return nil, nil, err + } + + r.log.Debugw("successfully deployed model", "service", svc.Name) + return stack, latticeTargets, err +} diff --git a/test/suites/integration/deregister_targets_test.go b/test/suites/integration/deregister_targets_test.go index 5e02cbb3..1222c5d1 100644 --- a/test/suites/integration/deregister_targets_test.go +++ b/test/suites/integration/deregister_targets_test.go @@ -15,67 +15,75 @@ import ( "github.com/aws/aws-application-networking-k8s/test/pkg/test" ) -var _ = Describe("Deregister Targets", func() { +var _ = Describe("Deregister Targets", Ordered, func() { var ( - deployment *appsv1.Deployment - service *v1.Service + deployments = make([]*appsv1.Deployment, 2) + services = make([]*v1.Service, 2) + targetGroups = make([]*vpclattice.TargetGroupSummary, 2) pathMatchHttpRoute *v1beta1.HTTPRoute - targetGroup *vpclattice.TargetGroupSummary ) - BeforeEach(func() { - deployment, service = testFramework.NewHttpApp(test.HTTPAppOptions{ - Name: "target-deregistration-test", + BeforeAll(func() { + deployments[0], services[0] = testFramework.NewHttpApp(test.HTTPAppOptions{ + Name: "target-deregistration-test-1", + Namespace: k8snamespace, + }) + deployments[1], services[1] = testFramework.NewHttpApp(test.HTTPAppOptions{ + Name: "target-deregistration-test-2", Namespace: k8snamespace, }) pathMatchHttpRoute = testFramework.NewPathMatchHttpRoute( - testGateway, []client.Object{service}, "http", "", k8snamespace) + testGateway, []client.Object{services[0], services[1]}, "http", "", k8snamespace) testFramework.ExpectCreated( ctx, pathMatchHttpRoute, - service, - deployment, + services[0], + deployments[0], + services[1], + deployments[1], ) route, _ := core.NewRoute(pathMatchHttpRoute) _ = testFramework.GetVpcLatticeService(ctx, route) - // Verify VPC Lattice Target Group exists - targetGroup = testFramework.GetTargetGroup(ctx, service) - Expect(*targetGroup.VpcIdentifier).To(Equal(test.CurrentClusterVpcId)) - Expect(*targetGroup.Protocol).To(Equal("HTTP")) + for i, service := range services { + // Verify VPC Lattice Target Group exists + targetGroups[i] = testFramework.GetTargetGroup(ctx, service) + Expect(*targetGroups[i]).To(Not(BeNil())) - // Verify VPC Lattice Targets exist - targets := testFramework.GetTargets(ctx, targetGroup, deployment) - Expect(*targetGroup.Port).To(BeEquivalentTo(80)) - for _, target := range targets { - Expect(*target.Port).To(BeEquivalentTo(service.Spec.Ports[0].TargetPort.IntVal)) - Expect(*target.Status).To(Or( - Equal(vpclattice.TargetStatusInitial), - Equal(vpclattice.TargetStatusHealthy), - )) + // Verify VPC Lattice Targets exist + targets := testFramework.GetTargets(ctx, targetGroups[i], deployments[i]) + Expect(*targetGroups[i].Port).To(BeEquivalentTo(80)) + for _, target := range targets { + Expect(*target.Port).To(BeEquivalentTo(service.Spec.Ports[0].TargetPort.IntVal)) + Expect(*target.Status).To(Or( + Equal(vpclattice.TargetStatusInitial), + Equal(vpclattice.TargetStatusHealthy), + )) + } } }) - AfterEach(func() { + It("Kubernetes Service deletion deregisters targets", func() { + testFramework.ExpectDeleted(ctx, services[0]) + verifyNoTargetsForTargetGroup(targetGroups[0]) + }) + + It("Kubernetes Deployment deletion triggers targets de-registering", func() { + testFramework.ExpectDeleted(ctx, services[1]) + verifyNoTargetsForTargetGroup(targetGroups[1]) + }) + + AfterAll(func() { // Lattice targets draining time for test cases in this file is actually unavoidable, - //because these test cases logic themselves test lattice targets de-registering before delete the httproute + // because these test cases logic themselves test lattice targets de-registering before delete the httproute testFramework.ExpectDeletedThenNotFound(ctx, pathMatchHttpRoute, - service, - deployment, + services[0], + deployments[0], + services[1], + deployments[1], ) }) - - //It("Kubernetes Service deletion deregisters targets", func() { - // Fail("Currently controller have a bug, service deletion does NOT trigger targets de-registering, need to further investigate the root cause") - // testFramework.ExpectDeleted(ctx, service) - // verifyNoTargetsForTargetGroup(targetGroup) - //}) - - It("Kubernetes Deployment deletion triggers targets de-registering", func() { - testFramework.ExpectDeleted(ctx, deployment) - verifyNoTargetsForTargetGroup(targetGroup) - }) }) func verifyNoTargetsForTargetGroup(targetGroup *vpclattice.TargetGroupSummary) { From 07dbac63501abf2f1574d21c6c82ac5d42fe1843 Mon Sep 17 00:00:00 2001 From: Zijun Wang Date: Wed, 6 Sep 2023 11:09:44 -0700 Subject: [PATCH 2/2] - Remove reconcileTargetsResource finalizer - Add reconcileTargetsResource error handling logic --- controllers/service_controller.go | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/controllers/service_controller.go b/controllers/service_controller.go index fffe4694..046ec6ad 100644 --- a/controllers/service_controller.go +++ b/controllers/service_controller.go @@ -33,6 +33,7 @@ import ( "github.com/aws/aws-application-networking-k8s/pkg/latticestore" "github.com/aws/aws-application-networking-k8s/pkg/model/core" latticemodel "github.com/aws/aws-application-networking-k8s/pkg/model/lattice" + lattice_runtime "github.com/aws/aws-application-networking-k8s/pkg/runtime" "github.com/aws/aws-application-networking-k8s/pkg/utils/gwlog" ) @@ -101,18 +102,24 @@ func RegisterServiceController( // For more details, check Reconcile and its Result here: // - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.10.0/pkg/reconcile func (r *serviceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { + return lattice_runtime.HandleReconcileError(r.reconcile(ctx, req)) +} + +func (r *serviceReconciler) reconcile(ctx context.Context, req ctrl.Request) error { r.log.Infow("reconcile", "name", req.Name) svc := &corev1.Service{} if err := r.client.Get(ctx, req.NamespacedName, svc); err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) + return client.IgnoreNotFound(err) } if !svc.DeletionTimestamp.IsZero() { tgName := latticestore.TargetGroupName(svc.Name, svc.Namespace) tgs := r.datastore.GetTargetGroupsByName(tgName) for _, tg := range tgs { r.log.Debugf("deletion request for tgName: %s: at timestamp: %s", tg.TargetGroupKey.Name, svc.DeletionTimestamp) - r.reconcileTargetsResource(ctx, svc, tg.TargetGroupKey.RouteName) + if err := r.reconcileTargetsResource(ctx, svc, tg.TargetGroupKey.RouteName); err != nil { + return err + } } r.finalizerManager.RemoveFinalizers(ctx, svc, serviceFinalizer) } else { @@ -122,14 +129,14 @@ func (r *serviceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct } r.log.Infow("reconciled", "name", req.Name) - return ctrl.Result{}, nil + return nil } -func (r *serviceReconciler) reconcileTargetsResource(ctx context.Context, svc *corev1.Service, routename string) { - if err := r.finalizerManager.AddFinalizers(ctx, svc, serviceFinalizer); err != nil { - r.eventRecorder.Event(svc, corev1.EventTypeWarning, k8s.ServiceEventReasonFailedAddFinalizer, fmt.Sprintf("failed and finalizer: %s", err)) +func (r *serviceReconciler) reconcileTargetsResource(ctx context.Context, svc *corev1.Service, routename string) error { + if _, _, err := r.buildAndDeployModel(ctx, svc, routename); err != nil { + return err } - r.buildAndDeployModel(ctx, svc, routename) + return nil } func (r *serviceReconciler) buildAndDeployModel(ctx context.Context, svc *corev1.Service, routename string) (core.Stack, *latticemodel.Targets, error) {