[Bug fix] Revive serviceReconciler.reconcileTargetsResource() to handle k8sService deletion de-registering targets

controllers/service_controller.go

@@ -18,17 +18,23 @@ 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"
+	lattice_runtime "github.com/aws/aws-application-networking-k8s/pkg/runtime"
 	"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 (
@@ -96,15 +102,60 @@ 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)
+			if err := r.reconcileTargetsResource(ctx, svc, tg.TargetGroupKey.RouteName); err != nil {
+				return err
+			}
+		}
 		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
+	return nil
+}
+
+func (r *serviceReconciler) reconcileTargetsResource(ctx context.Context, svc *corev1.Service, routename string) error {
+	if _, _, err := r.buildAndDeployModel(ctx, svc, routename); err != nil {
+		return err
+	}
+	return nil
+}
+
+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
 }

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) {