Skip to content

fix two targetGroup related bug #1635

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions pkg/deploy/elbv2/target_group_synthesizer.go
Original file line number Diff line number Diff line change
Expand Up @@ -162,9 +162,6 @@ func isSDKTargetGroupRequiresReplacement(sdkTG TargetGroupWithTags, resTG *elbv2
if string(resTG.Spec.TargetType) != awssdk.StringValue(sdkTG.TargetGroup.TargetType) {
return true
}
if resTG.Spec.Port != awssdk.Int64Value(sdkTG.TargetGroup.Port) {
return true
}
if string(resTG.Spec.Protocol) != awssdk.StringValue(sdkTG.TargetGroup.Protocol) {
return true
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/deploy/elbv2/target_group_synthesizer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -553,12 +553,12 @@ func Test_isSDKTargetGroupRequiresReplacement(t *testing.T) {
want: false,
},
{
name: "targetType change need replacement",
name: "port-only change shouldn't need replacement",
args: args{
sdkTG: TargetGroupWithTags{
TargetGroup: &elbv2sdk.TargetGroup{
TargetType: awssdk.String("instance"),
Port: awssdk.Int64(8080),
TargetType: awssdk.String("ip"),
Port: awssdk.Int64(9090),
Protocol: awssdk.String("HTTP"),
TargetGroupName: awssdk.String("my-tg"),
},
Expand All @@ -572,15 +572,15 @@ func Test_isSDKTargetGroupRequiresReplacement(t *testing.T) {
},
},
},
want: true,
want: false,
},
{
name: "port change need replacement",
name: "targetType change need replacement",
args: args{
sdkTG: TargetGroupWithTags{
TargetGroup: &elbv2sdk.TargetGroup{
TargetType: awssdk.String("ip"),
Port: awssdk.Int64(9090),
TargetType: awssdk.String("instance"),
Port: awssdk.Int64(8080),
Protocol: awssdk.String("HTTP"),
TargetGroupName: awssdk.String("my-tg"),
},
Expand Down
35 changes: 26 additions & 9 deletions pkg/ingress/model_build_target_group.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
"strconv"
)

const (
Expand Down Expand Up @@ -109,7 +110,7 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
if err != nil {
return elbv2model.TargetGroupSpec{}, err
}
targetGroupAttributes, err := t.buildTargetGroupAttributes(ctx, svcAndIngAnnotations)
tgAttributes, err := t.buildTargetGroupAttributes(ctx, svcAndIngAnnotations)
if err != nil {
return elbv2model.TargetGroupSpec{}, err
}
Expand All @@ -121,26 +122,25 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
if err != nil {
return elbv2model.TargetGroupSpec{}, err
}
targetGroupPort := 1
if svcPort.TargetPort.Type == intstr.Int {
targetGroupPort = svcPort.TargetPort.IntValue()
}
name := t.buildTargetGroupName(ctx, k8s.NamespacedName(ing), svc, svcPort.TargetPort, targetType, tgProtocol)

tgPort := t.buildTargetGroupPort(ctx, targetType, svcPort)
name := t.buildTargetGroupName(ctx, k8s.NamespacedName(ing), svc, port, tgPort, targetType, tgProtocol)
return elbv2model.TargetGroupSpec{
Name: name,
TargetType: targetType,
Port: int64(targetGroupPort),
Port: tgPort,
Protocol: tgProtocol,
HealthCheckConfig: &healthCheckConfig,
TargetGroupAttributes: targetGroupAttributes,
TargetGroupAttributes: tgAttributes,
Tags: tags,
}, nil
}

var invalidTargetGroupNamePattern = regexp.MustCompile("[[:^alnum:]]")

// buildTargetGroupName will calculate the targetGroup's name.
func (t *defaultModelBuildTask) buildTargetGroupName(_ context.Context,
ingKey types.NamespacedName, svc *corev1.Service, port intstr.IntOrString,
ingKey types.NamespacedName, svc *corev1.Service, port intstr.IntOrString, tgPort int64,
targetType elbv2model.TargetType, tgProtocol elbv2model.Protocol) string {
uuidHash := sha256.New()
_, _ = uuidHash.Write([]byte(t.clusterName))
Expand All @@ -149,6 +149,7 @@ func (t *defaultModelBuildTask) buildTargetGroupName(_ context.Context,
_, _ = uuidHash.Write([]byte(ingKey.Name))
_, _ = uuidHash.Write([]byte(svc.UID))
_, _ = uuidHash.Write([]byte(port.String()))
_, _ = uuidHash.Write([]byte(strconv.Itoa(int(tgPort))))
_, _ = uuidHash.Write([]byte(targetType))
_, _ = uuidHash.Write([]byte(tgProtocol))
uuid := hex.EncodeToString(uuidHash.Sum(nil))
Expand All @@ -171,6 +172,22 @@ func (t *defaultModelBuildTask) buildTargetGroupTargetType(_ context.Context, sv
}
}

// buildTargetGroupPort constructs the TargetGroup's port.
// Note: TargetGroup's port is not in the data path as we always register targets with port specified.
// so this settings don't really matter to our controller, and we do our best to use the most appropriate port as targetGroup's port to avoid UX confusing.
func (t *defaultModelBuildTask) buildTargetGroupPort(_ context.Context, targetType elbv2model.TargetType, svcPort corev1.ServicePort) int64 {
if targetType == elbv2model.TargetTypeInstance {
return int64(svcPort.NodePort)
}
if svcPort.TargetPort.Type == intstr.Int {
return int64(svcPort.TargetPort.IntValue())
}

// when a literal targetPort is used, we just use a fixed 1 here as this setting is not in the data path.
// also, under extreme edge case, it can actually be different ports for different pods.
return 1
}

func (t *defaultModelBuildTask) buildTargetGroupProtocol(_ context.Context, svcAndIngAnnotations map[string]string) (elbv2model.Protocol, error) {
rawBackendProtocol := string(t.defaultBackendProtocol)
_ = t.annotationParser.ParseStringAnnotation(annotations.IngressSuffixBackendProtocol, &rawBackendProtocol, svcAndIngAnnotations)
Expand Down
185 changes: 185 additions & 0 deletions pkg/ingress/model_build_target_group_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
package ingress

import (
"context"
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
"testing"
)

func Test_defaultModelBuildTask_buildTargetGroupName(t *testing.T) {
type args struct {
ingKey types.NamespacedName
svc *corev1.Service
port intstr.IntOrString
tgPort int64
targetType elbv2model.TargetType
tgProtocol elbv2model.Protocol
}
tests := []struct {
name string
args args
want string
}{
{
name: "standard case",
args: args{
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
svc: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "name-1",
UID: "my-uuid",
},
},
port: intstr.FromString("http"),
tgPort: 8080,
targetType: elbv2model.TargetTypeIP,
tgProtocol: elbv2model.ProtocolHTTP,
},
want: "k8s-ns1-name1-59797694c2",
},
{
name: "standard case - port differs",
args: args{
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
svc: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "name-1",
UID: "my-uuid",
},
},
port: intstr.FromInt(80),
tgPort: 8080,
targetType: elbv2model.TargetTypeIP,
tgProtocol: elbv2model.ProtocolHTTP,
},
want: "k8s-ns1-name1-70ebbeea02",
},
{
name: "standard case - tgPort differs",
args: args{
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
svc: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "name-1",
UID: "my-uuid",
},
},
port: intstr.FromString("http"),
tgPort: 9090,
targetType: elbv2model.TargetTypeIP,
tgProtocol: elbv2model.ProtocolHTTP,
},
want: "k8s-ns1-name1-cf545f64e8",
},
{
name: "standard case - targetType differs",
args: args{
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
svc: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "name-1",
UID: "my-uuid",
},
},
port: intstr.FromString("http"),
tgPort: 8080,
targetType: elbv2model.TargetTypeInstance,
tgProtocol: elbv2model.ProtocolHTTP,
},
want: "k8s-ns1-name1-e66dadb781",
},
{
name: "standard case - protocol differs",
args: args{
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
svc: &corev1.Service{
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns-1",
Name: "name-1",
UID: "my-uuid",
},
},
port: intstr.FromString("http"),
tgPort: 8080,
targetType: elbv2model.TargetTypeIP,
tgProtocol: elbv2model.ProtocolHTTPS,
},
want: "k8s-ns1-name1-3e1463213f",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
task := &defaultModelBuildTask{}
got := task.buildTargetGroupName(context.Background(), tt.args.ingKey, tt.args.svc, tt.args.port, tt.args.tgPort, tt.args.targetType, tt.args.tgProtocol)
assert.Equal(t, tt.want, got)
})
}
}

func Test_defaultModelBuildTask_buildTargetGroupPort(t *testing.T) {
type args struct {
targetType elbv2model.TargetType
svcPort corev1.ServicePort
}
tests := []struct {
name string
args args
want int64
}{
{
name: "instance targetGroup should use nodePort as port",
args: args{
targetType: elbv2model.TargetTypeInstance,
svcPort: corev1.ServicePort{
Name: "http",
Port: 80,
TargetPort: intstr.FromInt(8080),
NodePort: 32768,
},
},
want: 32768,
},
{
name: "ip targetGroup with numeric targetPort should use targetPort as port",
args: args{
targetType: elbv2model.TargetTypeIP,
svcPort: corev1.ServicePort{
Name: "http",
Port: 80,
TargetPort: intstr.FromInt(8080),
NodePort: 32768,
},
},
want: 8080,
},
{
name: "ip targetGroup with literal targetPort should use 1 as port",
args: args{
targetType: elbv2model.TargetTypeIP,
svcPort: corev1.ServicePort{
Name: "http",
Port: 80,
TargetPort: intstr.FromString("http"),
NodePort: 32768,
},
},
want: 1,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
task := &defaultModelBuildTask{}
got := task.buildTargetGroupPort(context.Background(), tt.args.targetType, tt.args.svcPort)
assert.Equal(t, tt.want, got)
})
}
}
Loading