Skip to content

Commit 8ac0f32

Browse files
M00nF1shM00nF1sh
authored
fix two targetGroup related bug (#1635)
* fix two targetGroup related bug * address PR comments * address PR comments
1 parent 7b823fb commit 8ac0f32

File tree

5 files changed

+563
-35
lines changed

5 files changed

+563
-35
lines changed

‎pkg/deploy/elbv2/target_group_synthesizer.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -162,9 +162,6 @@ func isSDKTargetGroupRequiresReplacement(sdkTG TargetGroupWithTags, resTG *elbv2
162162
if string(resTG.Spec.TargetType) != awssdk.StringValue(sdkTG.TargetGroup.TargetType) {
163163
return true
164164
}
165-
if resTG.Spec.Port != awssdk.Int64Value(sdkTG.TargetGroup.Port) {
166-
return true
167-
}
168165
if string(resTG.Spec.Protocol) != awssdk.StringValue(sdkTG.TargetGroup.Protocol) {
169166
return true
170167
}

‎pkg/deploy/elbv2/target_group_synthesizer_test.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -553,12 +553,12 @@ func Test_isSDKTargetGroupRequiresReplacement(t *testing.T) {
553553
want: false,
554554
},
555555
{
556-
name: "targetType change need replacement",
556+
name: "port-only change shouldn't need replacement",
557557
args: args{
558558
sdkTG: TargetGroupWithTags{
559559
TargetGroup: &elbv2sdk.TargetGroup{
560-
TargetType: awssdk.String("instance"),
561-
Port: awssdk.Int64(8080),
560+
TargetType: awssdk.String("ip"),
561+
Port: awssdk.Int64(9090),
562562
Protocol: awssdk.String("HTTP"),
563563
TargetGroupName: awssdk.String("my-tg"),
564564
},
@@ -572,15 +572,15 @@ func Test_isSDKTargetGroupRequiresReplacement(t *testing.T) {
572572
},
573573
},
574574
},
575-
want: true,
575+
want: false,
576576
},
577577
{
578-
name: "port change need replacement",
578+
name: "targetType change need replacement",
579579
args: args{
580580
sdkTG: TargetGroupWithTags{
581581
TargetGroup: &elbv2sdk.TargetGroup{
582-
TargetType: awssdk.String("ip"),
583-
Port: awssdk.Int64(9090),
582+
TargetType: awssdk.String("instance"),
583+
Port: awssdk.Int64(8080),
584584
Protocol: awssdk.String("HTTP"),
585585
TargetGroupName: awssdk.String("my-tg"),
586586
},

‎pkg/ingress/model_build_target_group.go

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"sigs.k8s.io/aws-load-balancer-controller/pkg/annotations"
1818
"sigs.k8s.io/aws-load-balancer-controller/pkg/k8s"
1919
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
20+
"strconv"
2021
)
2122

2223
const (
@@ -109,7 +110,7 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
109110
if err != nil {
110111
return elbv2model.TargetGroupSpec{}, err
111112
}
112-
targetGroupAttributes, err := t.buildTargetGroupAttributes(ctx, svcAndIngAnnotations)
113+
tgAttributes, err := t.buildTargetGroupAttributes(ctx, svcAndIngAnnotations)
113114
if err != nil {
114115
return elbv2model.TargetGroupSpec{}, err
115116
}
@@ -121,26 +122,25 @@ func (t *defaultModelBuildTask) buildTargetGroupSpec(ctx context.Context,
121122
if err != nil {
122123
return elbv2model.TargetGroupSpec{}, err
123124
}
124-
targetGroupPort := 1
125-
if svcPort.TargetPort.Type == intstr.Int {
126-
targetGroupPort = svcPort.TargetPort.IntValue()
127-
}
128-
name := t.buildTargetGroupName(ctx, k8s.NamespacedName(ing), svc, svcPort.TargetPort, targetType, tgProtocol)
125+
126+
tgPort := t.buildTargetGroupPort(ctx, targetType, svcPort)
127+
name := t.buildTargetGroupName(ctx, k8s.NamespacedName(ing), svc, port, tgPort, targetType, tgProtocol)
129128
return elbv2model.TargetGroupSpec{
130129
Name: name,
131130
TargetType: targetType,
132-
Port: int64(targetGroupPort),
131+
Port: tgPort,
133132
Protocol: tgProtocol,
134133
HealthCheckConfig: &healthCheckConfig,
135-
TargetGroupAttributes: targetGroupAttributes,
134+
TargetGroupAttributes: tgAttributes,
136135
Tags: tags,
137136
}, nil
138137
}
139138

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

141+
// buildTargetGroupName will calculate the targetGroup's name.
142142
func (t *defaultModelBuildTask) buildTargetGroupName(_ context.Context,
143-
ingKey types.NamespacedName, svc *corev1.Service, port intstr.IntOrString,
143+
ingKey types.NamespacedName, svc *corev1.Service, port intstr.IntOrString, tgPort int64,
144144
targetType elbv2model.TargetType, tgProtocol elbv2model.Protocol) string {
145145
uuidHash := sha256.New()
146146
_, _ = uuidHash.Write([]byte(t.clusterName))
@@ -149,6 +149,7 @@ func (t *defaultModelBuildTask) buildTargetGroupName(_ context.Context,
149149
_, _ = uuidHash.Write([]byte(ingKey.Name))
150150
_, _ = uuidHash.Write([]byte(svc.UID))
151151
_, _ = uuidHash.Write([]byte(port.String()))
152+
_, _ = uuidHash.Write([]byte(strconv.Itoa(int(tgPort))))
152153
_, _ = uuidHash.Write([]byte(targetType))
153154
_, _ = uuidHash.Write([]byte(tgProtocol))
154155
uuid := hex.EncodeToString(uuidHash.Sum(nil))
@@ -171,6 +172,22 @@ func (t *defaultModelBuildTask) buildTargetGroupTargetType(_ context.Context, sv
171172
}
172173
}
173174

175+
// buildTargetGroupPort constructs the TargetGroup's port.
176+
// Note: TargetGroup's port is not in the data path as we always register targets with port specified.
177+
// 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.
178+
func (t *defaultModelBuildTask) buildTargetGroupPort(_ context.Context, targetType elbv2model.TargetType, svcPort corev1.ServicePort) int64 {
179+
if targetType == elbv2model.TargetTypeInstance {
180+
return int64(svcPort.NodePort)
181+
}
182+
if svcPort.TargetPort.Type == intstr.Int {
183+
return int64(svcPort.TargetPort.IntValue())
184+
}
185+
186+
// when a literal targetPort is used, we just use a fixed 1 here as this setting is not in the data path.
187+
// also, under extreme edge case, it can actually be different ports for different pods.
188+
return 1
189+
}
190+
174191
func (t *defaultModelBuildTask) buildTargetGroupProtocol(_ context.Context, svcAndIngAnnotations map[string]string) (elbv2model.Protocol, error) {
175192
rawBackendProtocol := string(t.defaultBackendProtocol)
176193
_ = t.annotationParser.ParseStringAnnotation(annotations.IngressSuffixBackendProtocol, &rawBackendProtocol, svcAndIngAnnotations)

‎pkg/ingress/model_build_target_group_test.go

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
package ingress
2+
3+
import (
4+
"context"
5+
"github.com/stretchr/testify/assert"
6+
corev1 "k8s.io/api/core/v1"
7+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
8+
"k8s.io/apimachinery/pkg/types"
9+
"k8s.io/apimachinery/pkg/util/intstr"
10+
elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2"
11+
"testing"
12+
)
13+
14+
func Test_defaultModelBuildTask_buildTargetGroupName(t *testing.T) {
15+
type args struct {
16+
ingKey types.NamespacedName
17+
svc *corev1.Service
18+
port intstr.IntOrString
19+
tgPort int64
20+
targetType elbv2model.TargetType
21+
tgProtocol elbv2model.Protocol
22+
}
23+
tests := []struct {
24+
name string
25+
args args
26+
want string
27+
}{
28+
{
29+
name: "standard case",
30+
args: args{
31+
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
32+
svc: &corev1.Service{
33+
ObjectMeta: metav1.ObjectMeta{
34+
Namespace: "ns-1",
35+
Name: "name-1",
36+
UID: "my-uuid",
37+
},
38+
},
39+
port: intstr.FromString("http"),
40+
tgPort: 8080,
41+
targetType: elbv2model.TargetTypeIP,
42+
tgProtocol: elbv2model.ProtocolHTTP,
43+
},
44+
want: "k8s-ns1-name1-59797694c2",
45+
},
46+
{
47+
name: "standard case - port differs",
48+
args: args{
49+
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
50+
svc: &corev1.Service{
51+
ObjectMeta: metav1.ObjectMeta{
52+
Namespace: "ns-1",
53+
Name: "name-1",
54+
UID: "my-uuid",
55+
},
56+
},
57+
port: intstr.FromInt(80),
58+
tgPort: 8080,
59+
targetType: elbv2model.TargetTypeIP,
60+
tgProtocol: elbv2model.ProtocolHTTP,
61+
},
62+
want: "k8s-ns1-name1-70ebbeea02",
63+
},
64+
{
65+
name: "standard case - tgPort differs",
66+
args: args{
67+
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
68+
svc: &corev1.Service{
69+
ObjectMeta: metav1.ObjectMeta{
70+
Namespace: "ns-1",
71+
Name: "name-1",
72+
UID: "my-uuid",
73+
},
74+
},
75+
port: intstr.FromString("http"),
76+
tgPort: 9090,
77+
targetType: elbv2model.TargetTypeIP,
78+
tgProtocol: elbv2model.ProtocolHTTP,
79+
},
80+
want: "k8s-ns1-name1-cf545f64e8",
81+
},
82+
{
83+
name: "standard case - targetType differs",
84+
args: args{
85+
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
86+
svc: &corev1.Service{
87+
ObjectMeta: metav1.ObjectMeta{
88+
Namespace: "ns-1",
89+
Name: "name-1",
90+
UID: "my-uuid",
91+
},
92+
},
93+
port: intstr.FromString("http"),
94+
tgPort: 8080,
95+
targetType: elbv2model.TargetTypeInstance,
96+
tgProtocol: elbv2model.ProtocolHTTP,
97+
},
98+
want: "k8s-ns1-name1-e66dadb781",
99+
},
100+
{
101+
name: "standard case - protocol differs",
102+
args: args{
103+
ingKey: types.NamespacedName{Namespace: "ns-1", Name: "name-1"},
104+
svc: &corev1.Service{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Namespace: "ns-1",
107+
Name: "name-1",
108+
UID: "my-uuid",
109+
},
110+
},
111+
port: intstr.FromString("http"),
112+
tgPort: 8080,
113+
targetType: elbv2model.TargetTypeIP,
114+
tgProtocol: elbv2model.ProtocolHTTPS,
115+
},
116+
want: "k8s-ns1-name1-3e1463213f",
117+
},
118+
}
119+
for _, tt := range tests {
120+
t.Run(tt.name, func(t *testing.T) {
121+
task := &defaultModelBuildTask{}
122+
got := task.buildTargetGroupName(context.Background(), tt.args.ingKey, tt.args.svc, tt.args.port, tt.args.tgPort, tt.args.targetType, tt.args.tgProtocol)
123+
assert.Equal(t, tt.want, got)
124+
})
125+
}
126+
}
127+
128+
func Test_defaultModelBuildTask_buildTargetGroupPort(t *testing.T) {
129+
type args struct {
130+
targetType elbv2model.TargetType
131+
svcPort corev1.ServicePort
132+
}
133+
tests := []struct {
134+
name string
135+
args args
136+
want int64
137+
}{
138+
{
139+
name: "instance targetGroup should use nodePort as port",
140+
args: args{
141+
targetType: elbv2model.TargetTypeInstance,
142+
svcPort: corev1.ServicePort{
143+
Name: "http",
144+
Port: 80,
145+
TargetPort: intstr.FromInt(8080),
146+
NodePort: 32768,
147+
},
148+
},
149+
want: 32768,
150+
},
151+
{
152+
name: "ip targetGroup with numeric targetPort should use targetPort as port",
153+
args: args{
154+
targetType: elbv2model.TargetTypeIP,
155+
svcPort: corev1.ServicePort{
156+
Name: "http",
157+
Port: 80,
158+
TargetPort: intstr.FromInt(8080),
159+
NodePort: 32768,
160+
},
161+
},
162+
want: 8080,
163+
},
164+
{
165+
name: "ip targetGroup with literal targetPort should use 1 as port",
166+
args: args{
167+
targetType: elbv2model.TargetTypeIP,
168+
svcPort: corev1.ServicePort{
169+
Name: "http",
170+
Port: 80,
171+
TargetPort: intstr.FromString("http"),
172+
NodePort: 32768,
173+
},
174+
},
175+
want: 1,
176+
},
177+
}
178+
for _, tt := range tests {
179+
t.Run(tt.name, func(t *testing.T) {
180+
task := &defaultModelBuildTask{}
181+
got := task.buildTargetGroupPort(context.Background(), tt.args.targetType, tt.args.svcPort)
182+
assert.Equal(t, tt.want, got)
183+
})
184+
}
185+
}

0 commit comments

Comments
 (0)