Return 404 for invalid backendRefs

[erikfuller]

What type of PR is this?
bug fix

Which issue does this PR fix:
#493

What does this PR do / Why do we need it:
First, this PR aligns the implementation more with the gateway spec for invalid backendRefs. With this change, invalid backendRefs will either be omitted when another backendRef exists that is valid or will configure the rule to return a 404.

Second, when building listener model objects, we will no longer create model objects for deleted listeners. As I was making this change, I simplified the listener logic that was tracking deletes. Since we never actually delete listeners and they are deleted when the service is deleted, I've removed the unneeded code.

Testing done on this change:

Manual Testing
The following shows adding a route with an invalid backendRef, then adding the service (so the backendRef is valid), then deleting the service again (returns to an invalid backendRef).

First, the backendRef results in a fixedResponse 404

└─[$] <git:(tg-svc-fix*)> [laptop] kubectl apply -f examples/my-hotel-gateway.yaml
└─[$] <git:(tg-svc-fix*)> [laptop] kubectl apply -f examples/inventory-route.yaml
└─[$] <git:(tg-svc-fix*)> [laptop] aws vpc-lattice get-rule --service-identifier svc-X --listener-identifier listener-X --rule-identifier rule-X
{
    "action": {
        "fixedResponse": {
            "statusCode": 404
        }
    },
    "arn": "arn:aws:vpc-lattice:...",
...
    "priority": 1
}

Next, we get a real target group after creating the service

└─[$] <git:(tg-svc-fix*)> [laptop] kubectl apply -f examples/inventory-ver1.yaml
└─[$] <git:(tg-svc-fix*)> [laptop] aws vpc-lattice get-rule --service-identifier svc-X --listener-identifier listener-X --rule-identifier rule-X
{
    "action": {
        "forward": {
            "targetGroups": [
                {
                    "targetGroupIdentifier": "tg-X",
                    "weight": 10
                }
            ]
        }
    },
    "arn": "arn:aws:vpc-lattice:...",
...
}

Then we delete the service and get a fixed response again

└─[$] <git:(tg-svc-fix*)> [laptop] kubectl delete -f examples/inventory-ver1.yaml
└─[$] <git:(tg-svc-fix*)> [laptop] aws vpc-lattice get-rule --service-identifier svc-X --listener-identifier listener-X --rule-identifier rule-X
{
    "action": {
        "fixedResponse": {
            "statusCode": 404
        }
    },
    "arn": "arn:aws:vpc-lattice:...",
    ...
}

Automation added to e2e:
Changed deregister_targets_test.go to delete_target_groups_test.go. What we actually want are target groups for a service to be deleted when that service is deleted. So I've updated the test to reflect this.

I also saw some issues in validating targets for target groups in https_listener_weighted_rule_with_service_export_import_test.go so fixed the logic that was checking the wrong lengths in framework.go

Will this PR introduce any new dependencies?:
No

Will this break upgrades or downgrades. Has updating a running cluster been tested?:
n/a

Does this PR introduce any user-facing change?:

Reconciliation is now more tolerant of invalid backendRefs. Deleted services or incorrect references will no longer block route reconciliation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[erikfuller]

This test was actually succeeding previously because it was supposed to error, but it wasn't erroring for the reason the test intended. The Build() method checks and errors immediately for ServiceImport types. We no longer create model TG objects for service imports, but rather create a SvcImportTargetGroup directly against the rule (see rule.go).

I removed this test because it's misleading as written and was making it harder to write tests against the InvalidBackendRef cases.

[erikfuller]

This condition was evaluating as true when there were zero targets, which is logically incorrect. What we actually want here is for the pod ips to all be present in the list of targets.

[zijun726911]

QQ: now, which part of controller code is responsible to do "delete the k8s Service trigger deleting Unused Target Groups"? I think should be targetGroupSynthesizer.SynthesizeUnusedDelete() ?

Seems to understand changes in one function also need to have heavy knowledge on how other functions works... a little bit hard to trace code..

[erikfuller]

Yes, unfortunately at the moment you kind of need to know what's happening elsewhere to know if what's being done locally is OK. The unit tests help explain some of that and enforce the invariants. Part of the issue with keeping this logic entirely separate was we were doing a lot of redundant/throw-away work. For example, we would create models for listeners that were going to be deleted then do nothing with them. I think this is a good example of optimisation being at odds with properties like decoupling/modularization. Having said that, I think we can look at making this better. My intuition is that as part of #463 we'll be able to have better performance AND readability.

To answer the specific question, any unused target groups or target groups which reference invalid services will be deleted as part of the SynthesizeUnusedDelete(). We can and should run this cleanup on a schedule rather than only when a change is made.