Update target group info using TargetGroupPolicy #357
Conversation
Pull Request Test Coverage Report for Build 5989893375
💛 - Coveralls |
| @@ -51,14 +51,14 @@ spec: | |||
| healthyThresholdCount: | |||
| description: The number of consecutive successful health checks | |||
| required before considering an unhealthy target healthy. | |||
| format: int32 | |||
There was a problem hiding this comment.
What's the motivation behind changing these? Just curious.
| targetGroup.Spec.Config.K8SHTTPRouteName, config.VpcID) | ||
| } else { | ||
| tgName = targetGroup.Spec.Name |
There was a problem hiding this comment.
Are we certain we want to change the tg name even for the short version? If I understand it right, before it was targetGroup.Spec.Name, now it's <targetGroup.Spec.Name>-<protocol>-<protocolVersion>
There was a problem hiding this comment.
Yes this is intended, its supposed to be a key change to unblock GRPC serviceexport use case.
| utils.Truncate(defaultName, 70), | ||
| utils.Truncate(routeName, 20), | ||
| utils.Truncate(vpcId, 21), |
There was a problem hiding this comment.
How did we decide on these numbers? I get VPC id length, but not the other two. Can we add constants for these to give them some description?
There was a problem hiding this comment.
will add descriptions to this, but this is mostly for just fitting in 128b limit. Wanted to put most spaces for the name while preserving some minimum length for routes
This should be eventually changed to some configurable format, as discussed in #315
| matcher = func(s string) bool { | ||
| match := strings.HasPrefix(s, targetGroup.Spec.Name) | ||
| if targetGroup.Spec.Config.ProtocolVersion == vpclattice.TargetGroupProtocolVersionGrpc { | ||
| return match && strings.HasSuffix(s, vpclattice.TargetGroupProtocolVersionGrpc) | ||
| } | ||
| return match | ||
| } |
There was a problem hiding this comment.
Should we be looking for dashes as well since they're kind of our delimiter?
Can what's missing in the middle be used to specify two different services that are both imported? Will we ever match more that one target group with this?
There was a problem hiding this comment.
Looking at this comment now I think we should match smarter than this, let me change this part
| for _, r := range resp { | ||
| if aws.StringValue(r.Name) == targetGroup { | ||
| glog.V(6).Info("targetgroup ", targetGroup, " already exists with arn ", *r.Arn, "\n") | ||
| if matcher(*r.Name) { |
There was a problem hiding this comment.
further from above, should we be validating we only have at most one match?
| // We have finished rule reconciliation at this point. | ||
| // If a target group under HTTPRoute does not have any service, it is stale. | ||
| isUsed := t.isTargetGroupUsedByaHTTPRoute(ctx, tgName, httpRoute) && | ||
| len(sdkTG.getTargetGroupOutput.ServiceArns) > 0 |
There was a problem hiding this comment.
I'm having a hard time understanding why lines 270-288 matter, looks like we're just using it for logging or is there some side effect happening I can't see?
Maybe don't worry about it, this super long function could really use some refactoring.
There was a problem hiding this comment.
this is the function that removes dangling/stale target groups.
| @@ -371,14 +374,29 @@ func Test_SynthesizeSDKTargetGroups(t *testing.T) { | |||
| wantDataStoreError: nil, | |||
| wantDataStoreStatus: "", | |||
| }, | |||
| { | |||
| name: "Delete SDK TargetGroup since it is dangling with no service associated", | |||
There was a problem hiding this comment.
When would this case happen under normal circumstances? Want to make sure we're not being over eager with our deletes.
There was a problem hiding this comment.
This happens when changing target group protocol - it should create tg-https-http1 and remove tg-http-http1 for example. Due to how we have detected inUse target group before, I need to make a change in behavior here (also related to the comment above)
| for _, policy := range policyList.Items { | ||
| targetRef := policy.Spec.TargetRef | ||
| if targetRef == nil { | ||
| continue |
There was a problem hiding this comment.
Should we log or is this expected?
There was a problem hiding this comment.
I think we can ignore this, my ground rule here is to only care about TargetGroupPolicy that is relevant.
| return latticemodel.TargetGroupSpec{}, err | ||
| } | ||
| protocol := "HTTP" | ||
| protocolVersion := vpclattice.TargetGroupProtocolVersionHttp1 |
There was a problem hiding this comment.
yes those are default values
| protocol = *tgp.Spec.Protocol | ||
| } | ||
| if tgp.Spec.ProtocolVersion != nil { | ||
| protocolVersion = *tgp.Spec.ProtocolVersion |
There was a problem hiding this comment.
do we need to do any translation on these for the API, like ToUpper() or anything?
There was a problem hiding this comment.
I can double check but I don't think so. we already have validations on YAML file.
| @@ -372,6 +391,23 @@ func (t *latticeServiceModelBuildTask) buildTargetGroupSpec(ctx context.Context, | |||
|
|
|||
| tgName := latticestore.TargetGroupName(string(httpBackendRef.Name()), namespace) | |||
|
|
|||
| tgp, err := getAttachedTargetGroupPolicy(ctx, client, string(httpBackendRef.Name()), namespace) | |||
There was a problem hiding this comment.
following code block looks identical to line 198-213
There was a problem hiding this comment.
it is right now, but will be quite different with GRPC serviceexport follow-up change
| @@ -420,6 +458,57 @@ func (t *latticeServiceModelBuildTask) buildTargetGroupName(_ context.Context, b | |||
| } | |||
| } | |||
|
|
|||
| func getAttachedTargetGroupPolicy(ctx context.Context, k8sClient client.Client, svcName, svcNamespace string) (*v1alpha1.TargetGroupPolicy, error) { | |||
| policyList := &v1alpha1.TargetGroupPolicyList{} | |||
| err := k8sClient.List(ctx, policyList) | |||
There was a problem hiding this comment.
Any chance to use field selector for List() here to avoid for loop?
What type of PR is this?
feature
Which issue does this PR fix:
#304 #315 (partial) #69
What does this PR do / Why do we need it:
Now supporting configuring target group protocols and health checks by attaching TargetGroupPolicy CRD to k8s Service.
This PR includes breaking change on Lattice target group naming scheme. Now TG names on VPC Lattice include protocol and protocol version - this is necessary for avoiding conflicts and supporting a few service export scenarios.
I also have increased the name size to allow longer names.
Whenever a TargetGroupPolicy is applied for existing target group, if protocol / protocolVersion is changed:
Since we have this order it should be happening quickly without downtime.
If only HC is changed, none of these happens, it will be just calling UpdateTargetGroup API.
WIP things:
If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:
Testing done on this change:
Did manual tests, unit test changes pending
Automation added to e2e:
Will this PR introduce any new dependencies?:
Will this break upgrades or downgrades. Has updating a running cluster been tested?:
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.