Return 404 for invalid backendRefs. Remove redundant IsDeleted for mo…

…del.Listener (#497)

pkg/deploy/lattice/listener_synthesizer.go

@@ -45,19 +45,15 @@ func (l *listenerSynthesizer) Synthesize(ctx context.Context) error {
 			return err
 		}
 
-		// deletes are deferred to the later logic comparing existing listeners
-		// to current listeners.
-		if !listener.IsDeleted {
-			status, err := l.listenerMgr.Upsert(ctx, listener, svc)
-			if err != nil {
-				listenerErr = errors.Join(listenerErr,
-					fmt.Errorf("failed ListenerManager.Upsert %s-%s due to err %s",
-						listener.Spec.K8SRouteName, listener.Spec.K8SRouteNamespace, err))
-				continue
-			}
-
-			listener.Status = &status
+		status, err := l.listenerMgr.Upsert(ctx, listener, svc)
+		if err != nil {
+			listenerErr = errors.Join(listenerErr,
+				fmt.Errorf("failed ListenerManager.Upsert %s-%s due to err %s",
+					listener.Spec.K8SRouteName, listener.Spec.K8SRouteNamespace, err))
+			continue
 		}
+
+		listener.Status = &status
 	}
 
 	if listenerErr != nil {
@@ -86,8 +82,8 @@ func (l *listenerSynthesizer) Synthesize(ctx context.Context) error {
 func (l *listenerSynthesizer) shouldDelete(listenerToFind *model.Listener, stackListeners []*model.Listener) bool {
 	for _, candidate := range stackListeners {
 		if candidate.Spec.Port == listenerToFind.Spec.Port && candidate.Spec.Protocol == listenerToFind.Spec.Protocol {
-			// found a match, delete if match is deleted
-			return candidate.IsDeleted
+			// found a match, do not delete
+			return false
 		}
 	}
 	// there is no matching listener

pkg/deploy/lattice/listener_synthesizer_test.go

@@ -44,37 +44,6 @@ func Test_SynthesizeListenerCreate(t *testing.T) {
 	assert.Nil(t, err)
 }
 
-func Test_SynthesizeListenerDeleteNoOp(t *testing.T) {
-	c := gomock.NewController(t)
-	defer c.Finish()
-	ctx := context.TODO()
-	mockListenerMgr := NewMockListenerManager(c)
-
-	stack := core.NewDefaultStack(core.StackID{Name: "foo", Namespace: "bar"})
-
-	svc := &model.Service{
-		ResourceMeta: core.NewResourceMeta(stack, "AWS:VPCServiceNetwork::Service", "stack-svc-id"),
-		Status:       &model.ServiceStatus{Id: "svc-id"},
-	}
-	assert.NoError(t, stack.AddResource(svc))
-
-	l := &model.Listener{
-		ResourceMeta: core.NewResourceMeta(stack, "AWS:VPCServiceNetwork::Listener", "l-id"),
-		Spec: model.ListenerSpec{
-			StackServiceId: "stack-svc-id",
-		},
-		IsDeleted: true, // <-- the bit that matters
-	}
-	assert.NoError(t, stack.AddResource(l))
-
-	// since we aren't returning any resources, we interpret that to mean the listener is already deleted
-	mockListenerMgr.EXPECT().List(ctx, gomock.Any()).Return([]*vpclattice.ListenerSummary{}, nil)
-
-	ls := NewListenerSynthesizer(gwlog.FallbackLogger, mockListenerMgr, stack)
-	err := ls.Synthesize(ctx)
-	assert.Nil(t, err)
-}
-
 func Test_SynthesizeListenerCreateWithReconcile(t *testing.T) {
 	c := gomock.NewController(t)
 	defer c.Finish()

pkg/deploy/lattice/rule_manager.go

@@ -97,9 +97,24 @@ func (r *defaultRuleManager) buildLatticeRule(modelRule *model.Rule) (*vpclattic
 	updateMatchFromRule(&httpMatch, modelRule)
 	gro.Match = &vpclattice.RuleMatch{HttpMatch: &httpMatch}
 
-	if len(modelRule.Spec.Action.TargetGroups) > 0 {
+	// check if we have at least one valid target group
+	var hasValidTargetGroup bool
+	for _, tg := range modelRule.Spec.Action.TargetGroups {
+		if tg.LatticeTgId != model.InvalidBackendRefTgId {
+			hasValidTargetGroup = true
+			break
+		}
+	}
+
+	if hasValidTargetGroup {
 		var latticeTGs []*vpclattice.WeightedTargetGroup
 		for _, ruleTg := range modelRule.Spec.Action.TargetGroups {
+			// skip any invalid TGs - eventually VPC Lattice may support weighted fixed response
+			// and this logic can be more in line with the spec
+			if ruleTg.LatticeTgId == model.InvalidBackendRefTgId {
+				continue
+			}
+
 			latticeTG := vpclattice.WeightedTargetGroup{
 				TargetGroupIdentifier: aws.String(ruleTg.LatticeTgId),
 				Weight:                aws.Int64(ruleTg.Weight),
@@ -114,7 +129,7 @@ func (r *defaultRuleManager) buildLatticeRule(modelRule *model.Rule) (*vpclattic
 			},
 		}
 	} else {
-		r.log.Debugf("There are no target groups, defaulting to 404 Fixed response")
+		r.log.Debugf("There are no valid target groups, defaulting to 404 Fixed response")
 		gro.Action = &vpclattice.RuleAction{
 			FixedResponse: &vpclattice.FixedResponseAction{
 				StatusCode: aws.Int64(404),
@@ -237,7 +252,7 @@ func (r *defaultRuleManager) updateIfNeeded(
 
 	_, err := r.cloud.Lattice().UpdateRuleWithContext(ctx, &uri)
 	if err != nil {
-		return model.RuleStatus{}, fmt.Errorf("Failed UpdateRule %d for %s, %s due to %s",
+		return model.RuleStatus{}, fmt.Errorf("failed UpdateRule %d for %s, %s due to %s",
 			ruleToUpdate.Priority, latticeListenerId, latticeSvcId, err)
 	}
 
@@ -375,7 +390,7 @@ func (r *defaultRuleManager) Delete(ctx context.Context, ruleId string, serviceI
 
 	_, err := r.cloud.Lattice().DeleteRuleWithContext(ctx, &deleteInput)
 	if err != nil {
-		return fmt.Errorf("Failed DeleteRule %s/%s/%s due to %s", serviceId, listenerId, ruleId, err)
+		return fmt.Errorf("failed DeleteRule %s/%s/%s due to %s", serviceId, listenerId, ruleId, err)
 	}
 
 	r.log.Infof("Success DeleteRule %s/%s/%s", serviceId, listenerId, ruleId)

pkg/deploy/lattice/rule_manager_test.go

@@ -65,6 +65,67 @@ func Test_Create(t *testing.T) {
 		},
 	}
 
+	rInvalidBR := &model.Rule{
+		Spec: model.RuleSpec{
+			Priority: 1,
+			Action: model.RuleAction{
+				TargetGroups: []*model.RuleTargetGroup{
+					{
+						LatticeTgId: model.InvalidBackendRefTgId,
+						Weight:      1,
+					},
+				},
+			},
+			PathMatchPrefix: true,
+			PathMatchValue:  "/foo",
+		},
+	}
+
+	rTwoInvalidBR := &model.Rule{
+		Spec: model.RuleSpec{
+			Priority: 1,
+			Action: model.RuleAction{
+				TargetGroups: []*model.RuleTargetGroup{
+					{
+						LatticeTgId: model.InvalidBackendRefTgId,
+						Weight:      1,
+					},
+					{
+						LatticeTgId: model.InvalidBackendRefTgId,
+						Weight:      1,
+					},
+				},
+			},
+			PathMatchPrefix: true,
+			PathMatchValue:  "/foo",
+		},
+	}
+
+	rOneValidBR := &model.Rule{
+		Spec: model.RuleSpec{
+			Priority: 1,
+			Method:   "POST",
+			Action: model.RuleAction{
+				TargetGroups: []*model.RuleTargetGroup{
+					{
+						LatticeTgId: model.InvalidBackendRefTgId,
+						Weight:      1,
+					},
+					{
+						LatticeTgId: model.InvalidBackendRefTgId,
+						Weight:      1,
+					},
+					{
+						LatticeTgId: "tg-id",
+						Weight:      1,
+					},
+				},
+			},
+			PathMatchPrefix: true,
+			PathMatchValue:  "/foo",
+		},
+	}
+
 	t.Run("test create", func(t *testing.T) {
 		mockLattice.EXPECT().GetRulesAsList(ctx, gomock.Any()).Return(
 			[]*vpclattice.GetRuleOutput{}, nil)
@@ -183,6 +244,55 @@ func Test_Create(t *testing.T) {
 		assert.Nil(t, err)
 		assert.Equal(t, "existing-arn", ruleStatus.Arn)
 	})
+
+	t.Run("test create - invalid backendRefs", func(t *testing.T) {
+		mockLattice.EXPECT().GetRulesAsList(ctx, gomock.Any()).Return(
+			[]*vpclattice.GetRuleOutput{}, nil).Times(2)
+
+		mockLattice.EXPECT().CreateRuleWithContext(ctx, gomock.Any()).DoAndReturn(
+			func(ctx context.Context, input *vpclattice.CreateRuleInput, i ...interface{}) (*vpclattice.CreateRuleOutput, error) {
+				assert.Equal(t, int64(404), aws.Int64Value(input.Action.FixedResponse.StatusCode))
+
+				return &vpclattice.CreateRuleOutput{
+					Arn:  aws.String("arn"),
+					Id:   aws.String("id"),
+					Name: aws.String("name"),
+				}, nil
+			}).Times(2)
+
+		rm := NewRuleManager(gwlog.FallbackLogger, cloud)
+		ruleStatus, err := rm.Upsert(ctx, rInvalidBR, l, svc)
+		assert.Nil(t, err)
+		assert.Equal(t, "arn", ruleStatus.Arn)
+
+		// result should be the same so long as all backendRefs are invalid
+		ruleStatus, err = rm.Upsert(ctx, rTwoInvalidBR, l, svc)
+		assert.Nil(t, err)
+		assert.Equal(t, "arn", ruleStatus.Arn)
+	})
+
+	t.Run("test create - one valid backendRef, two invalid", func(t *testing.T) {
+		mockLattice.EXPECT().GetRulesAsList(ctx, gomock.Any()).Return(
+			[]*vpclattice.GetRuleOutput{}, nil)
+
+		mockLattice.EXPECT().CreateRuleWithContext(ctx, gomock.Any()).DoAndReturn(
+			func(ctx context.Context, input *vpclattice.CreateRuleInput, i ...interface{}) (*vpclattice.CreateRuleOutput, error) {
+				assert.Equal(t, "POST", aws.StringValue(input.Match.HttpMatch.Method))
+				assert.Equal(t, 1, len(input.Action.Forward.TargetGroups))
+				assert.Equal(t, "tg-id", aws.StringValue(input.Action.Forward.TargetGroups[0].TargetGroupIdentifier))
+
+				return &vpclattice.CreateRuleOutput{
+					Arn:  aws.String("arn"),
+					Id:   aws.String("id"),
+					Name: aws.String("name"),
+				}, nil
+			})
+
+		rm := NewRuleManager(gwlog.FallbackLogger, cloud)
+		ruleStatus, err := rm.Upsert(ctx, rOneValidBR, l, svc)
+		assert.Nil(t, err)
+		assert.Equal(t, "arn", ruleStatus.Arn)
+	})
 }
 
 func Test_CreateWithTempPriority(t *testing.T) {

pkg/deploy/lattice/rule_synthesizer.go

@@ -49,6 +49,12 @@ func (r *ruleSynthesizer) resolveRuleTgIds(ctx context.Context, modelRule *model
 		}
 
 		if rtg.StackTargetGroupId != "" {
+			if rtg.StackTargetGroupId == model.InvalidBackendRefTgId {
+				r.log.Debugf("Rule TG has an invalid backendref, setting TG id to invalid")
+				rtg.LatticeTgId = model.InvalidBackendRefTgId
+				continue
+			}
+
 			r.log.Debugf("Fetching TG %d from the stack (ID %s)", i, rtg.StackTargetGroupId)
 
 			stackTg := &model.TargetGroup{}

pkg/deploy/lattice/rule_synthesizer_test.go

@@ -166,6 +166,9 @@ func Test_resolveRuleTgs(t *testing.T) {
 					{
 						StackTargetGroupId: "stack-tg-id",
 					},
+					{
+						StackTargetGroupId: model.InvalidBackendRefTgId,
+					},
 				},
 			},
 		},
@@ -195,4 +198,5 @@ func Test_resolveRuleTgs(t *testing.T) {
 
 	assert.Equal(t, "svc-export-tg-id", r.Spec.Action.TargetGroups[0].LatticeTgId)
 	assert.Equal(t, "tg-id", r.Spec.Action.TargetGroups[1].LatticeTgId)
+	assert.Equal(t, model.InvalidBackendRefTgId, r.Spec.Action.TargetGroups[2].LatticeTgId)
 }

pkg/gateway/model_build_listener.go

@@ -5,8 +5,6 @@ import (
 	"errors"
 	"fmt"
 	model "github.com/aws/aws-application-networking-k8s/pkg/model/lattice"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
-
 	"k8s.io/apimachinery/pkg/types"
 	gwv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1"
 )
@@ -27,9 +25,6 @@ func (t *latticeServiceModelBuildTask) extractListenerInfo(
 	t.log.Debugf("Building Listener for Route %s-%s", t.route.Name(), t.route.Namespace())
 	gw, err := t.getGateway(ctx)
 	if err != nil {
-		if apierrors.IsNotFound(err) && !t.route.DeletionTimestamp().IsZero() {
-			return 0, string(protocol), nil // ok if we're deleting the route
-		}
 		return 0, "", err
 	}
 
@@ -83,6 +78,10 @@ func (t *latticeServiceModelBuildTask) buildListeners(ctx context.Context, stack
 	if len(t.route.Spec().ParentRefs()) == 0 {
 		t.log.Debugf("No ParentRefs on route %s-%s, nothing to do", t.route.Name(), t.route.Namespace())
 	}
+	if !t.route.DeletionTimestamp().IsZero() {
+		t.log.Debugf("Route %s-%s is deleted, skipping listener build", t.route.Name(), t.route.Namespace())
+		return nil
+	}
 
 	for _, parentRef := range t.route.Spec().ParentRefs() {
 		if parentRef.Name != t.route.Spec().ParentRefs()[0].Name {
@@ -112,7 +111,6 @@ func (t *latticeServiceModelBuildTask) buildListeners(ctx context.Context, stack
 
 		t.log.Debugf("Added listener %s-%s to the stack (ID %s)",
 			modelListener.Spec.K8SRouteName, modelListener.Spec.K8SRouteNamespace, modelListener.ID())
-		modelListener.IsDeleted = !t.route.DeletionTimestamp().IsZero()
 	}
 
 	return nil

pkg/gateway/model_build_rule.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 
@@ -31,6 +30,7 @@ const (
 )
 
 func (t *latticeServiceModelBuildTask) buildRules(ctx context.Context, stackListenerId string) error {
+	// note we only build rules for non-deleted routes
 	t.log.Debugf("Processing %d rules", len(t.route.Spec().Rules()))
 
 	for i, rule := range t.route.Spec().Rules() {
@@ -254,9 +254,16 @@ func (t *latticeServiceModelBuildTask) getTargetGroupsForRuleAction(ctx context.
 			// generate the actual target group model for the backendRef
 			_, tg, err := t.brTgBuilder.Build(ctx, t.route, backendRef, t.stack)
 			if err != nil {
-				return nil, err
+				ibre := &InvalidBackendRefError{}
+				if !errors.As(err, &ibre) {
+					return nil, err
+				}
+
+				t.log.Infof("Invalid backendRef found on route %s", t.route.Name())
+				ruleTG.StackTargetGroupId = model.InvalidBackendRefTgId
+			} else {
+				ruleTG.StackTargetGroupId = tg.ID()
 			}
-			ruleTG.StackTargetGroupId = tg.ID()
 		}
 
 		tgList = append(tgList, &ruleTG)